1264815 - Make persistent notifications work like the spec.

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[SUN Haitao]

When a persistent notification (which is created through the showNotification() method) is tapped, a `notificationclick` event should be dispatched to its associated ServiceWorker no matter whether Fennec is running.

Comment 1

[SUN Haitao]

Some mechanisms introduced in Bug 1252650 is planned to be reused.

Comment 2

[SUN Haitao]

Attachment: 1-Bug_1252650___Add_a__data__parameter_to_GeckoThread_createServices.patch

Based on changeset 101ccfb6b257 & 587da5c67437.

Comment 3

[SUN Haitao]

Attachment: 2-Bug_1252650___Add_a__showAlertInternal__method_to_nsIAlertService.patch

Comment 4

[SUN Haitao]

Attachment: 3-Bug_1252650___Add_a_component_to_build_notification_persistent_data.patch

Comment 5

[SUN Haitao]

Attachment: 4-Bug_1252650___Add_a__showPersistentAlertNotification__method_to_GeckoAppShell.patch

Comment 6

[SUN Haitao]

Attachment: 5-Bug_1252650___Add_a_component_to_handle__persisent_notification_click_.patch

Comment 7

[SUN Haitao]

Attachment: 6-Bug_1252650___Make_services_can_show_notifications.patch

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8742117 [details] [diff] [review]
1-Bug_1252650___Add_a__data__parameter_to_GeckoThread_createServices.patch

Review of attachment 8742117 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/android/nsAppShell.cpp
@@ +270,5 @@
>      {
>          nsCString category(aCategory->ToCString());
>  
>          NS_CreateServicesFromCategory(
> +                category.get(), 

nit: extra space at the end

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8742122 [details] [diff] [review]
6-Bug_1252650___Make_services_can_show_notifications.patch

Review of attachment 8742122 [details] [diff] [review]:
-----------------------------------------------------------------

Why do we need this?

Comment 10

[SUN Haitao]

(In reply to Jim Chen [:jchen] [:darchons] from comment #9)
> Why do we need this?

Without this, the 'showNotification' method will not be callable from event handlers in service workers while Fennec is not running. My test shows that it throws a NullPointerException in such situation.

Comment 11

[Nick Alexander :nalexander [he/him]]

jchen: can you review the entirety of this patch series?  Thanks!

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Nick Alexander :nalexander from comment #11)
> jchen: can you review the entirety of this patch series?  Thanks!

I'll see what I can do.

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8742118 [details] [diff] [review]
2-Bug_1252650___Add_a__showAlertInternal__method_to_nsIAlertService.patch

Redirecting to :kitcambridge. In general, you should include a description of the patch with your patch, so reviewers can understand the changes your made.

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8742122 [details] [diff] [review]
6-Bug_1252650___Make_services_can_show_notifications.patch

Review of attachment 8742122 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/java/org/mozilla/gecko/GeckoService.java
@@ +177,5 @@
>              }
> +
> +            if (GeckoAppShell.notificationClient == null) {
> +                GeckoAppShell.setNotificationClient(new AppNotificationClient(getApplicationContext()));
> +            }

I think you should use ServiceNotificationClient. Also, move this to GeckoService.onCreate, and don't check for | notificationClient == null |, because setNotificationClient already checks for that.

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8742119 [details] [diff] [review]
3-Bug_1252650___Add_a_component_to_build_notification_persistent_data.patch

Review of attachment 8742119 [details] [diff] [review]:
-----------------------------------------------------------------

There are better ways to produce JSON than to create a new XPCOM component (e.g. use mozilla::JSONWriter).

Comment 16

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8742120 [details] [diff] [review]
4-Bug_1252650___Add_a__showPersistentAlertNotification__method_to_GeckoAppShell.patch

Review of attachment 8742120 [details] [diff] [review]:
-----------------------------------------------------------------

Redirecting to :kitcambridge. In particular, I'm not sure we should use JSON for persisting data inside dom/notification. Using JSON for serialization seems like an Android-specific implementation detail.

Comment 17

[Lina Butler [:lina]]

Comment on attachment 8742118 [details] [diff] [review]
2-Bug_1252650___Add_a__showAlertInternal__method_to_nsIAlertService.patch

Thanks for the patch!

Is there a reason we can't use `nsIAlertNotification.cookie` for this ("uniqueCookie" in Notification.cpp), and append the persistent data to it instead? I'd prefer we avoid adding a third way to pass opaque data along, when we already have `cookie` and `data`.

Soft r- for now.

Comment 18

[SUN Haitao]

(In reply to Jim Chen [:jchen] [:darchons] from comment #14)
> I think you should use ServiceNotificationClient. 
If we use ServiceNotificationClient here, what will happen when Fennec starts later? I'm afraid setNotification will not replace it with an AppNotificationClient. That may cause some troubles.

> Also, move this to GeckoService.onCreate, 
But this operation is unnecessary while start GeckoService for update addons.

> and don't check for | notificationClient == null |, because setNotificationClient
> already checks for that.
I check notificationClient to avoid an unnecessary call for this part may be called while Fennec is running.

Comment 19

[SUN Haitao]

(In reply to Jim Chen [:jchen] [:darchons] from comment #15)
> There are better ways to produce JSON than to create a new XPCOM component
> (e.g. use mozilla::JSONWriter).

Ah, that's much better. I failed to found it.

(In reply to Jim Chen [:jchen] [:darchons] from comment #16)
> Redirecting to :kitcambridge. In particular, I'm not sure we should use JSON
> for persisting data inside dom/notification. Using JSON for serialization
> seems like an Android-specific implementation detail.

JSON is chosen for the data will quite probably be handled by a JS XPCOM component finally.

Comment 20

[SUN Haitao]

(In reply to Kit Cambridge [:kitcambridge] from comment #17)
> Is there a reason we can't use `nsIAlertNotification.cookie` for this
> ("uniqueCookie" in Notification.cpp), and append the persistent data to it
> instead? I'd prefer we avoid adding a third way to pass opaque data along,
> when we already have `cookie` and `data`.

My concern is that currently `nsIAlertNotification.cookie` is an identifier-like thing with its own format. It may be confusing to APPEND complex structured data into it. And it is tricky to separate data from it while using. 

As for `nsIAlertNotification.data`, I believe it is for user app scripts. We'd better not overload it.

As for adding a new attribute to nsIAlertNotification, this may introduce a signature change for `nsIAlertNotification.init`. That will cause some extra revisions.

Comment 21

[Lina Butler [:lina]]

Comment on attachment 8742120 [details] [diff] [review]
4-Bug_1252650___Add_a__showPersistentAlertNotification__method_to_GeckoAppShell.patch

Review of attachment 8742120 [details] [diff] [review]:
-----------------------------------------------------------------

Wrote down some thoughts, but I'm not really qualified to review this. Redirecting (again) to :wchen. :-)

::: dom/notification/Notification.cpp
@@ +1754,5 @@
>      } else {
>        observer = new MainThreadNotificationObserver(Move(ownership));
>      }
>    } else {
> +    isPersistent = true;

Please cite "persistent notification" from the Notifications spec in a comment here.

@@ +1836,5 @@
>                     inPrivateBrowsing);
>    NS_ENSURE_SUCCESS_VOID(rv);
> +
> +  nsAutoString persistentData;
> +#ifdef MOZ_WIDGET_ANDROID

I wonder if it makes sense to move this into `NotificationStorage`.

* `NotificationStorage` is implemented in JS, and already has arguments for everything except the `originSuffix` and `isPersistent`.
* On Android, `NotificationStorage.put` could use `Messaging.jsm` to tell Fennec about the persistent notification.
* `GeckoAppShell.showAlertNotification` would then create the correct `notificationIntent` based on whether a persistent notification entry exists.

Assuming that works, I can see some problems with that:

* Shifting complexity. Fennec will need to track (and clean up) persistent notifications, instead of creating the intent directly.
* Overloading the storage interface to mean "store some additional state for Fennec, but only if the notification is persistent."
* Notification won't be shown if Fennec is stopped between `PersistNotification` and `alertService->ShowAlert`.

But it would remove `ShowAlertInternal` and `nsIPersistentData`, and move Fennec-specific code closer to where it's used.

Comment 22

[Lina Butler [:lina]]

(In reply to SUN Haitao from comment #20)
> My concern is that currently `nsIAlertNotification.cookie` is an
> identifier-like thing with its own format. It may be confusing to APPEND
> complex structured data into it. And it is tricky to separate data from it
> while using. 
> 
> As for `nsIAlertNotification.data`, I believe it is for user app scripts.
> We'd better not overload it.

Aye, that's a good point about appending a complex structure to the cookie. Also, you're right; we can't reuse `nsIAlertNotification.data`.

:wchen can provide better feedback than me, but, if we decide a separate method is the way to go, "ShowPersistentNotification" would be a more descriptive name.

Comment 23

[SUN Haitao]

(In reply to Kit Cambridge [:kitcambridge] from comment #21)
> But it would remove `ShowAlertInternal` and `nsIPersistentData`, and move
> Fennec-specific code closer to where it's used.

`nsIPersistenData` will be gone after I rewrite the serialization with `mozilla::JSONWriter`.

In a hindsight, this part is actually platform-neutral. So it's unnecessary to surrounding this with `#ifdef MOZ_WIDGET_ANDROID`. Although persistent data will be ignored on other platforms currently.

I DID investigate using notificationStorage earlier and found that is not quite simple:

* Some serializations are still required to passing `originSuffix`, `scope`, `id` and `origin`.
* As I mentioned in comments, the current implemention of GetByID doesn't find right records after a restart.

With such problems we found, I'm afraid that we need quite several strong reasons to try that idea.

Comment 24

[SUN Haitao]

(In reply to Kit Cambridge [:kitcambridge] from comment #22)
> :wchen can provide better feedback than me, but, if we decide a separate
> method is the way to go, "ShowPersistentNotification" would be a more
> descriptive name.

I think the new name is good. But a non-public `ShowAlertInternal` method may still be useful for `ShowPersistentNotification` and `ShowAlert` share a lot of logic.

Comment 25

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to SUN Haitao from comment #18)
> (In reply to Jim Chen [:jchen] [:darchons] from comment #14)
> > I think you should use ServiceNotificationClient. 
> If we use ServiceNotificationClient here, what will happen when Fennec
> starts later? I'm afraid setNotification will not replace it with an
> AppNotificationClient. That may cause some troubles.

Fennec normally uses ServiceNotificationClient [1]. AppNotificationClient is used for non-Fennec applications.

[1] https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java?rev=ae7413abfa4d#3852

> > Also, move this to GeckoService.onCreate, 
> But this operation is unnecessary while start GeckoService for update addons.

We should still set it as part of initialization.

> > and don't check for | notificationClient == null |, because setNotificationClient
> > already checks for that.
> I check notificationClient to avoid an unnecessary call for this part may be
> called while Fennec is running.

Just let setNotificationClient do the check for you.

Comment 26

[SUN Haitao]

Attachment: 6-Bug_1252650___Make_services_can_show_notifications.patch

Comment 27

[SUN Haitao]

(In reply to Jim Chen [:jchen] [:darchons] from comment #15)
> There are better ways to produce JSON than to create a new XPCOM component
> (e.g. use mozilla::JSONWriter).

JSONWriter use 8-bit strings only. But most of data here are stored in UTF-16 strings. What is the recommended way to handle such situation?

Comment 28

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8742812 [details] [diff] [review]
6-Bug_1252650___Make_services_can_show_notifications.patch

Review of attachment 8742812 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 29

[SUN Haitao]

(In reply to SUN Haitao from comment #27)
> JSONWriter use 8-bit strings only. But most of data here are stored in
> UTF-16 strings. What is the recommended way to handle such situation?

Any suggestions?

Comment 30

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to SUN Haitao from comment #29)
> (In reply to SUN Haitao from comment #27)
> > JSONWriter use 8-bit strings only. But most of data here are stored in
> > UTF-16 strings. What is the recommended way to handle such situation?
> 
> Any suggestions?

Sorry, meant to reply earlier. You can use NS_ConvertUTF16toUTF8 to convert to UTF-8 first. Or if you know the data is strictly ASCII (e.g. data encoded in base64), you can use NS_LossyConvertUTF16ToASCII.

Comment 31

[SUN Haitao]

(In reply to Jim Chen [:jchen] [:darchons] from comment #30)
> Sorry, meant to reply earlier. You can use NS_ConvertUTF16toUTF8 to convert
> to UTF-8 first. Or if you know the data is strictly ASCII (e.g. data encoded
> in base64), you can use NS_LossyConvertUTF16ToASCII.

Never mind. What bugs me most is that converting everything into UTF-8 and converting the result back into UTF-16 (the final result is needed to be an nsAString) seems quite inelegant.

I'll go with this plan first, and file followups for an elegant way later if necessary.

Comment 32

[William Chen [:wchen]]

Comment on attachment 8742120 [details] [diff] [review]
4-Bug_1252650___Add_a__showPersistentAlertNotification__method_to_GeckoAppShell.patch

Review of attachment 8742120 [details] [diff] [review]:
-----------------------------------------------------------------

From the more recent comments it looks like you don't need nsIPersistentData.

I also agree with Kit that this looks like something that belongs in NotificationStorage.

(In reply to SUN Haitao from comment #23)
> I DID investigate using notificationStorage earlier and found that is not
> quite simple:
> 
> * Some serializations are still required to passing `originSuffix`, `scope`,
> `id` and `origin`.
> * As I mentioned in comments, the current implemention of GetByID doesn't
> find right records after a restart.
> 
> With such problems we found, I'm afraid that we need quite several strong
> reasons to try that idea.

- NotificationStorage.Put alredy takes scope, id and origin in serialized form. For originSuffix, you may be able to get it from a princiapl from where you are planning to use it. You can also get it from the origin.
- If GetByID doesn't work, then the getNotifications API is probably also not going to work.

Since Android notifications can store data, it may make sense to alter the implementation for NotificationStorage to just use native android notifications. If we do that then the data should survive restarts and getNotifications should always reflect what is visible to the user. When we call NotificationStorage.Put, send the data to Fennec and keep it around until we show the alert, then have Fennec store that data into the Android Notification. When we call GetByID, send a request to Fennec to use Android API that gets the notifications, filter them by ID and return the resulting data from the Notifications.

Comment 33

[SUN Haitao]

(In reply to William Chen [:wchen] from comment #32)
> - If GetByID doesn't work, then the getNotifications API is probably also
> not going to work.

Let me investigate this first. The situation will have huge effect on what is a good plan here.

Comment 34

[SUN Haitao]

I've confirmed that `getNotifications` doesn't work properly either. So some enhancements to notificationStorage is required.

I suspect this is a cache-related problem can be fixed without replacing the storage mechanism completely on Android. so here is the plan:

0. Make notificationStorage work normally.

1. While displaying a persistent notification, save most persistent data into notificationStorage first, and pass a `origin` and a `id` to the Java code.

2. Make the Java code show a system notification which will awake GeckoService and send the `origin` and the `id` to a observer.

3. Make the observer fetch (and delete) other persistent data based on the `origin` and the `id`, and call `nsIServiceWorkerManager.sendNotificationClickEvent` with all persistent data.

For passing the `origin` and the `id`, I strongly suggest using a separate method. Passing them as side-effect of `notificationStorage.put` as Kit mentioned is neither simple nor DRY.

How about this plan?

Comment 35

[(inactive) Jim Chen [:jchen] [:darchons]]

I landed part 6 early to make bug 1267419 work.

Comment 36

[William Chen [:wchen]]

(In reply to SUN Haitao from comment #34)
> I've confirmed that `getNotifications` doesn't work properly either. So some
> enhancements to notificationStorage is required.
> 
> I suspect this is a cache-related problem can be fixed without replacing the
> storage mechanism completely on Android. so here is the plan:
> 
> 0. Make notificationStorage work normally.
> 
> 1. While displaying a persistent notification, save most persistent data
> into notificationStorage first, and pass a `origin` and a `id` to the Java
> code.
> 
> 2. Make the Java code show a system notification which will awake
> GeckoService and send the `origin` and the `id` to a observer.
> 
> 3. Make the observer fetch (and delete) other persistent data based on the
> `origin` and the `id`, and call
> `nsIServiceWorkerManager.sendNotificationClickEvent` with all persistent
> data.
> 
Kit has some patches coming that handle deleting the persistent data.
> For passing the `origin` and the `id`, I strongly suggest using a separate
> method. Passing them as side-effect of `notificationStorage.put` as Kit
> mentioned is neither simple nor DRY.
> 
You're right, sending the data as a side-effect isn't great. We could add additional optional parameters to nsIAlertNotification or add new API to the alert service, I'd prefer adding the optional parameters.
> How about this plan?

The plan sounds good to me, although we should try and factor out methods for sending the event to the worker so that the two paths (ServiceWorkerNotificationObserver and the Fennec notification observer you're planning to implement) share as much a possible.

Comment 37

[SUN Haitao]

(In reply to William Chen [:wchen] from comment #36)
> Kit has some patches coming that handle deleting the persistent data.
For which bug? Let's mark it as a depend-on.

> You're right, sending the data as a side-effect isn't great. We could add
> additional optional parameters to nsIAlertNotification or add new API to the
> alert service, I'd prefer adding the optional parameters.
I purpose we use a new method first to avoid having to adjust and TEST too much codes on Desktop platforms immediately. 

> The plan sounds good to me, although we should try and factor out methods
> for sending the event to the worker so that the two paths
> (ServiceWorkerNotificationObserver and the Fennec notification observer
> you're planning to implement) share as much a possible.
I suggest we finish this first and merge those two observers in follow-ups, for each platform may need its own revisions and tests. Handling all those work altogether may be complex and inflexible.

Comment 38

[William Chen [:wchen]]

(In reply to SUN Haitao from comment #37)
> (In reply to William Chen [:wchen] from comment #36)
> > Kit has some patches coming that handle deleting the persistent data.
> For which bug? Let's mark it as a depend-on.
Bug 1265828
> > You're right, sending the data as a side-effect isn't great. We could add
> > additional optional parameters to nsIAlertNotification or add new API to the
> > alert service, I'd prefer adding the optional parameters.
> I purpose we use a new method first to avoid having to adjust and TEST too
> much codes on Desktop platforms immediately. 
I don't think adding a new method to the interface is going to be any simpler, the back end alert service implementations on all platforms will have a new method to implement.
> > The plan sounds good to me, although we should try and factor out methods
> > for sending the event to the worker so that the two paths
> > (ServiceWorkerNotificationObserver and the Fennec notification observer
> > you're planning to implement) share as much a possible.
> I suggest we finish this first and merge those two observers in follow-ups,
> for each platform may need its own revisions and tests. Handling all those
> work altogether may be complex and inflexible.
I'm not saying we necessarily have to merge the two observers into one but we should factor out and reuse code where possible.

Comment 39

[SUN Haitao]

(In reply to William Chen [:wchen] from comment #38)
> I don't think adding a new method to the interface is going to be any
> simpler, the back end alert service implementations on all platforms will
> have a new method to implement.
But we can add dummy implementations for desktop platforms and bypass them.

> I'm not saying we necessarily have to merge the two observers into one but
> we should factor out and reuse code where possible.
I guess they can be merged at last. Let me try and see whether this is a good idea.

Comment 40

[Lina Butler [:lina]]

Comment on attachment 8742118 [details] [diff] [review]
2-Bug_1252650___Add_a__showAlertInternal__method_to_nsIAlertService.patch

(In reply to SUN Haitao from comment #34)
> For passing the `origin` and the `id`, I strongly suggest using a separate
> method. Passing them as side-effect of `notificationStorage.put` as Kit
> mentioned is neither simple nor DRY.
> 
> How about this plan?

Thanks for the explanation! Your approach of adding a separate method for persistent notifications makes more sense to me now, and I agree with you that overloading `notificationStorage.put` isn't good. Clearing the r- on attachment 8742118 [details] [diff] [review].

(In reply to SUN Haitao from comment #39)
> (In reply to William Chen [:wchen] from comment #38)
> > I don't think adding a new method to the interface is going to be any
> > simpler, the back end alert service implementations on all platforms will
> > have a new method to implement.
> But we can add dummy implementations for desktop platforms and bypass them.

What do you think about factoring that method out into a new interface, and only having `nsAlertsService` implement it? I think that would avoid having to change the Desktop implementations.

> > I'm not saying we necessarily have to merge the two observers into one but
> > we should factor out and reuse code where possible.
> I guess they can be merged at last. Let me try and see whether this is a
> good idea.

The `notificationclose` event will add more logic to the `ServiceWorkerNotificationObserver`, too. It'd be good to consolidate them where you can, but a follow-up makes sense if it gets too complex.

Comment 41

[SUN Haitao]

(In reply to Kit Cambridge [:kitcambridge] from comment #40)
> What do you think about factoring that method out into a new interface, and
> only having `nsAlertsService` implement it? I think that would avoid having
> to change the Desktop implementations.
>
> The `notificationclose` event will add more logic to the
> `ServiceWorkerNotificationObserver`, too. It'd be good to consolidate them
> where you can, but a follow-up makes sense if it gets too complex.r

My current idea is always using a single observer instance to watch all clicks on persistent notifications on Android. If this turns out to be a good idea, then we should make desktop implementations (and the `notificationclose` implementation) use it too. So I suggest we just limit the extra works on desktop implementation minimal right here right now, and handle other things in follow-ups.

Comment 42

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8742121 [details] [diff] [review]
5-Bug_1252650___Add_a_component_to_handle__persisent_notification_click_.patch

Review of attachment 8742121 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not going to be involved in this patch.  If this really needs review, please find an alternate (jchen or kitcambridge).

Comment 43

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: needed for Push Notifications feature

Comment 44

[Lina Butler [:lina]]

Any updates on the new approach, Haitao?

Comment 45

[SUN Haitao]

Almost done. Some extra stubs for desktop platforms may be needed. Is there a mechanism to make a method of XPCOM Interface Android-only?

Comment 46

[Lina Butler [:lina]]

(In reply to SUN Haitao from comment #45)
> Almost done. Some extra stubs for desktop platforms may be needed. Is there
> a mechanism to make a method of XPCOM Interface Android-only?

Cool, thanks! I don't think there is. Seems like an ifdef similar to https://dxr.mozilla.org/mozilla-central/rev/3461f3cae78495f100a0f7d3d2e0b89292d3ec02/toolkit/components/alerts/nsAlertsService.cpp#324-326 is the way to make something Android-only.

Comment 47

[SUN Haitao]

In that case, Let's review the main part and forge stubs in parallel.

Comment 48

[SUN Haitao]

Attachment: 1-Bug_1252650___Add_a__data__parameter_to_GeckoThread_createServices.patch

Slightly modified to avoid changing existing codes that call `getIntentToCreateServices`.

Comment 49

[SUN Haitao]

Attachment: 2-Bug_1252650___Add_a__showPersistentNotification__method_to_nsIAlertService.patch

Comment 50

[SUN Haitao]

Attachment: 4-Bug_1252650___Add_a__showPersistentAlertNotification__method_to_GeckoAppShell.patch

Comment 51

[SUN Haitao]

Attachment: 5-Bug_1252650___Add_a_component_to_handle__persisent_notification_click_.patch

Comment 52

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8751564 [details] [diff] [review]
1-Bug_1252650___Add_a__data__parameter_to_GeckoThread_createServices.patch

Review of attachment 8751564 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/java/org/mozilla/gecko/GeckoService.java
@@ +131,1 @@
>      public static Intent getIntentToCreateServices(final Context context, final String category) {

Change this method to return getIntentToCreateServices(context, category, /* data */ null);

Comment 53

[SUN Haitao]

(In reply to Jim Chen [:jchen] [:darchons] from comment #52)
> Change this method to return getIntentToCreateServices(context, category, /*
> data */ null);

I choose the current form in case we may need to tell passing null from passing no data. Maybe isn't important enough to worry about?

Comment 54

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to SUN Haitao from comment #53)
> (In reply to Jim Chen [:jchen] [:darchons] from comment #52)
> > Change this method to return getIntentToCreateServices(context, category, /*
> > data */ null);
> 
> I choose the current form in case we may need to tell passing null from
> passing no data. Maybe isn't important enough to worry about?

Right, it's better to simplify things. We can always change it later if needed.

Comment 55

[SUN Haitao]

Attachment: 1-Bug_1252650___Add_a__data__parameter_to_GeckoThread_createServices.patch

Comment 56

[SUN Haitao]

Attachment: 2-Bug_1252650___Add_a__showPersistentNotification__method_to_nsIAlertService.patch

Desktop platform stubs are included.

Comment 57

[Lina Butler [:lina]]

Comment on attachment 8751572 [details] [diff] [review]
5-Bug_1252650___Add_a_component_to_handle__persisent_notification_click_.patch

Review of attachment 8751572 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, assuming we don't call "PersistentNotification" while Fennec is running. Thanks!

::: dom/notification/NotificationDB.jsm
@@ +86,5 @@
>      for (let origin of origins) {
>        let canPut = notificationStorage.canPut(origin);
>        if (!canPut) {
> +        let persistentNotificationCount = 0;
> +        for (let id of Object.keys(notifications[origin])) {

Nit: for (let id in notifications[origin]) {

::: mobile/android/components/MobileComponents.manifest
@@ +116,5 @@
>  # AndroidActivitiesGlue.js
>  component {e4deb5f6-d5e3-4fce-bc53-901dd9951c48} AndroidActivitiesGlue.js
>  contract @mozilla.org/dom/activities/ui-glue;1 {e4deb5f6-d5e3-4fce-bc53-901dd9951c48}
> +
> +# PersistentNotification.js

Nit: "PersistentNotificationHandler" seems clearer to me, but it's up to you.

::: mobile/android/components/PersistentNotification.js
@@ +24,5 @@
> +PersistentNotification.prototype = {
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver]),
> +  classID: Components.ID("{75390fe7-f8a3-423a-b3b1-258d7eabed40}"),
> +
> +  observe(subject, topic, data) {

Is this service called for persistent notifications when Fennec is running, too? Or only when it needs to be started?

@@ +26,5 @@
> +  classID: Components.ID("{75390fe7-f8a3-423a-b3b1-258d7eabed40}"),
> +
> +  observe(subject, topic, data) {
> +    if (Services.appinfo.processType == Services.appinfo.PROCESS_TYPE_DEFAULT) {
> +      Cu.import("resource://gre/modules/NotificationDB.jsm");

Would it make sense to move this to the top of the file, with the other imports? Margaret said on IRC that Android doesn't support e10s; it seems like this will always be true.

@@ +34,5 @@
> +    notificationStorage.getByID(persistentInfo.origin, persistentInfo.id, {
> +      handle(id, title, dir, lang, body, tag, icon, data, behavior, serviceWorkerRegistrationID) {
> +        serviceWorkerManager.sendNotificationClickEvent(
> +          persistentInfo.originSuffix,
> +          persistentInfo.scope,

You could pass serviceWorkerRegistrationID here and omit the scope from persistentInfo, but let's keep it if you think it's clearer.

Comment 58

[Lina Butler [:lina]]

Comment on attachment 8752593 [details] [diff] [review]
2-Bug_1252650___Add_a__showPersistentNotification__method_to_nsIAlertService.patch

Please add stubs to https://dxr.mozilla.org/mozilla-central/source/toolkit/system/gnome/nsSystemAlertsService.cpp and https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/OSXNotificationCenter.mm, too.

r+ with those added.

Comment 59

[Lina Butler [:lina]]

Comment on attachment 8751567 [details] [diff] [review]
4-Bug_1252650___Add_a__showPersistentAlertNotification__method_to_GeckoAppShell.patch

Review of attachment 8751567 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for all the patches! I think wchen and jchen should review these, too, because I don't know C++ very well, or how Android intents and "createServices" works.

::: dom/notification/Notification.cpp
@@ +1668,5 @@
>  }
>  
> +struct StringWriteFunc : public JSONWriteFunc
> +{
> +  nsAString& mBuffer;

Should this be "nsString"?

Comment 60

[SUN Haitao]

Attachment: 2-Bug_1252650___Add_a__showPersistentNotification__method_to_nsIAlertService.patch

Sorry, I uploaded a wrong version.

Comment 61

[Lina Butler [:lina]]

Comment on attachment 8753112 [details] [diff] [review]
2-Bug_1252650___Add_a__showPersistentNotification__method_to_nsIAlertService.patch

Hmm, did you mean to upload a different one? This patch looks identical to the last one.

Comment 62

[SUN Haitao]

Attachment: 2-Bug_1264815___Add_a__showPersistentNotification__method_to_nsIAlertService.patch

My fault, I made the same mistake once again.

Comment 63

[Lina Butler [:lina]]

Comment on attachment 8753114 [details] [diff] [review]
2-Bug_1264815___Add_a__showPersistentNotification__method_to_nsIAlertService.patch

Review of attachment 8753114 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 64

[SUN Haitao]

(In reply to Kit Cambridge [:kitcambridge] from comment #57)

> Looks good, assuming we don't call "PersistentNotification" while Fennec is
> running. Thanks!
Actually, my plan is ALWAYS using PersistentNotification to handle persistent notifications on Android. And I suggest we use it on desktop also by making ServiceWorkerNotificationObserver invoke it. 

> Nit: "PersistentNotificationHandler" seems clearer to me, but it's up to you.
Nice suggestion, I like it.

> Would it make sense to move this to the top of the file, with the other
> imports? Margaret said on IRC that Android doesn't support e10s; it seems
> like this will always be true.
I guess we'd better keep it here for a while until we test using this component on desktop.

> You could pass serviceWorkerRegistrationID here and omit the scope from
> persistentInfo, but let's keep it if you think it's clearer.
If they always equal by design, maybe we should rename `serviceWorkerRegistrationID` to `serviceWorkerRegistrationScope` to make things clearer.

Comment 65

[Lina Butler [:lina]]

(In reply to SUN Haitao from comment #64)
> Actually, my plan is ALWAYS using PersistentNotification to handle
> persistent notifications on Android. And I suggest we use it on desktop also
> by making ServiceWorkerNotificationObserver invoke it. 

I see. I have a couple of questions, then. :-)

* Is it possible for "notificationclick" to fire twice if Fennec is running?

* The service worker manager runs in the content process under e10s (bug 1182117 tracks moving it to the parent). Were you thinking of using a message manager like NotificationStorage?

We probably don't need to worry about the second one on Android, but I'm curious about the first one, because ISTM ServiceWorkerNotificationObserver also calls SendNotificationClick.

> If they always equal by design, maybe we should rename
> `serviceWorkerRegistrationID` to `serviceWorkerRegistrationScope` to make
> things clearer.

Agreed.

Comment 66

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8752590 [details] [diff] [review]
1-Bug_1252650___Add_a__data__parameter_to_GeckoThread_createServices.patch

https://hg.mozilla.org/integration/fx-team/rev/079e427f6ac3

Comment 67

[SUN Haitao]

(In reply to Kit Cambridge [:kitcambridge] from comment #65)
> * Is it possible for "notificationclick" to fire twice if Fennec is running?

ServiceWorkerNotificationObserver only calls SendNotificationClick when a 'alertclickcallback' topic is observed. With this series of patch, on Android platform, tapping a persistent notification no longer creates any `alertclickcallback` topic. So no event will not fire twice.

> * The service worker manager runs in the content process under e10s (bug
> 1182117 tracks moving it to the parent). Were you thinking of using a
> message manager like NotificationStorage?

How about that we use PersistentNotificationHanlder on Android only until the SWM is moved into the parent process?

Comment 68

[SUN Haitao]

(In reply to SUN Haitao from comment #67)
> So no event will not fire twice.

So no event will fire twice.

Comment 69

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8751567 [details] [diff] [review]
4-Bug_1252650___Add_a__showPersistentAlertNotification__method_to_GeckoAppShell.patch

Review of attachment 8751567 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for the Android bits.

::: mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java
@@ +897,5 @@
>  
> +    @WrapForJNI(stubName = "ShowPersistentAlertNotificationWrapper")
> +    public static void showPersistentAlertNotification(
> +          String aPersistentData,
> +          String aImageUrl, String aAlertTitle, String aAlertText, 

Nit: remove trailing space (and in other places too)

Comment 70

[William Chen [:wchen]]

Comment on attachment 8751567 [details] [diff] [review]
4-Bug_1252650___Add_a__showPersistentAlertNotification__method_to_GeckoAppShell.patch

Review of attachment 8751567 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/notification/Notification.cpp
@@ +1668,5 @@
>  }
>  
> +struct StringWriteFunc : public JSONWriteFunc
> +{
> +  nsAString& mBuffer;

I think you should take Kit's suggestion and make this a nsString (then use JSONWriter::WriteFunc + static_cast to get the string when it's done). We can't annotate StringWriteFunc as MOZ_STACK_CLASS because it gets heap allocated (providing an opportunity for this class to outlive the buffer). In your particular use case it is safe but this pattern can have pit falls if we can't enforce stack only so I think we should be a bit more defensive and use nsString instead since it doesn't add any significant cost.

Also, please put this class into the anonymous namespace.

Comment 71

[William Chen [:wchen]]

On second thought, it would be strange to reuse StringWriteFunc in a way that is long-lived and if it did get reused for anything large, nsString would be a poor choice for building strings. It should be fine keeping it the way it is, just move it to the anonymous namespace. Maybe also add a comment that StringWriteFunc must not outlive the buffer.

Comment 72

[SUN Haitao]

Attachment: 1-Bug_1264815___Add_a__data__parameter_to_GeckoThread_createServices__r_jchen.patch

The rebased right version that includes suggested modifications in Comment 52.

Comment 73

[SUN Haitao]

Attachment: 3-Bug_1264815___Rename__serviceWorkerRegistrationID__to__serviceWorkerRegistrationScope_.patch

Rename `serviceWorkerRegistrationID` to `serviceWorkerRegistrationScope` to make things clearer.

Comment 74

[SUN Haitao]

Attachment: 5-Bug_1264815___Add_a_component_to_handle__persisent_notification_click_.patch

Comment 75

[SUN Haitao]

Attachment: 4-Bug_1264815___Add_a__showPersistentAlertNotification__method_to_GeckoAppShell.patch

Did I understand you right?

Comment 76

[Lina Butler [:lina]]

Comment on attachment 8757572 [details] [diff] [review]
3-Bug_1264815___Rename__serviceWorkerRegistrationID__to__serviceWorkerRegistrationScope_.patch

Nice! Bonus points if you rename "serviceWorkerRegistrationID" in Notification.{h, cpp}, too, unless that's already done in another patch.

Comment 77

[Lina Butler [:lina]]

Comment on attachment 8757576 [details] [diff] [review]
5-Bug_1264815___Add_a_component_to_handle__persisent_notification_click_.patch

Looks great!

Comment 78

[SUN Haitao]

Attachment: 3-Bug_1264815___Rename__serviceWorkerRegistrationID__to__serviceWorkerRegistrationScope_.patch

I forgot them for using a case-sensitive search.

Comment 79

[SUN Haitao]

Attachment: 3-Bug_1264815___Rename__serviceWorkerRegistrationID__to__serviceWorkerRegistrationScope_.patch

Comment 80

[Carsten Book [:Tomcat]]

part 6 has problems to apply: (can we also get a try run) thanks!

eg '1-3,5', or 's' to toggle the sort order between id & patch description) 1
adding 1264815 to series file
renamed 1264815 -> 6-Bug_1252650___Make_services_can_show_notifications.patch
applying 6-Bug_1252650___Make_services_can_show_notifications.patch
patching file mobile/android/base/java/org/mozilla/gecko/GeckoService.java
Hunk #1 FAILED at 81
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/java/org/mozilla/gecko/GeckoService.java.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 6-Bug_1252650___Make_services_can_show_notifications.patch

Comment 81

[SUN Haitao]

I believe this is because part 6 already landed earlier. See https://hg.mozilla.org/mozilla-central/rev/8d30d67837bd.

Comment 82

[Carsten Book [:Tomcat]]

(In reply to SUN Haitao from comment #81)
> I believe this is because part 6 already landed earlier. See
> https://hg.mozilla.org/mozilla-central/rev/8d30d67837bd.

ok landed part 1-5 then :) thanks!

Comment 83

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ca182106b9e6
https://hg.mozilla.org/integration/fx-team/rev/8f6cc2d22ba7
https://hg.mozilla.org/integration/fx-team/rev/9bcbe80a4a7a
https://hg.mozilla.org/integration/fx-team/rev/20d0b05e708b
https://hg.mozilla.org/integration/fx-team/rev/d049a47c24c1

Comment 84

[SUN Haitao]

Personally, I prefer to ask wchen for taking part 4 an extra look before landing part 4 & 5. But landing them right now should be okay also.

And this bug needn't be marked as 'leave-open' any more.

Comment 85

[Carsten Book [:Tomcat]]

seems there are issues in the patches like https://treeherder.mozilla.org/logviewer.html#?job_id=9646165&repo=fx-team and so i had to back this out

Comment 86

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/b98fa78343cd
Backed out changeset d049a47c24c1 
https://hg.mozilla.org/integration/fx-team/rev/dfaeab92707e
Backed out changeset 20d0b05e708b 
https://hg.mozilla.org/integration/fx-team/rev/f7aead3c999a
Backed out changeset 9bcbe80a4a7a 
https://hg.mozilla.org/integration/fx-team/rev/66d97d875f04
Backed out changeset 8f6cc2d22ba7 
https://hg.mozilla.org/integration/fx-team/rev/11bba688023e
Backed out changeset ca182106b9e6 for bustage

Comment 87

[SUN Haitao]

Attachment: 4-Bug_1264815___Add_a__showPersistentAlertNotification__method_to_GeckoAppShell.patch

Try this version?

Comment 88

[Carsten Book [:Tomcat]]

it seems there are more issues like https://treeherder.mozilla.org/logviewer.html#?job_id=9647179&repo=fx-team can we get a try run before trying to checkin again ?

Comment 89

[SUN Haitao]

I'll perform a try run later to make sure. 

But as for as I know, this series of patches has nothing to do with pygtk. I am afraid the second issue has another source.

Comment 90

[SUN Haitao]

But I haven't got Level 1 Commit Access yet, what's the best plan now?

Comment 91

[Carsten Book [:Tomcat]]

kcambridge, since you reviewed some of the patches here, do you think you could vouch for L1 access ?

Comment 92

[Lina Butler [:lina]]

I vouch.

Haitao, please feel free to link to this comment when you file your commit access request. Or flag me on the bug, and I can vouch there, too.

Comment 93

[SUN Haitao]

https://bugzilla.mozilla.org/show_bug.cgi?id=1277130

Comment 94

[SUN Haitao]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eccb34e05630

Comment 95

[SUN Haitao]

What's next then?

BTW, is there a recommended way to tell a test failure is known or new?

Comment 96

[Carsten Book [:Tomcat]]

(In reply to SUN Haitao from comment #95)
> What's next then?
> 
> BTW, is there a recommended way to tell a test failure is known or new?

yeah, when you look at the results you will see that treeherder suggests bugs sometimes. When there is a already filed bug suggested its mostly a known failure. 

When looking at the results something like https://treeherder.mozilla.org/logviewer.html#?job_id=21812748&repo=try could be related to your push, at least from the subject of this bug and the test failure

Comment 97

[SUN Haitao]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=902fdaec3477

Comment 98

[SUN Haitao]

Attachment: 2-Bug_1264815___Add_a__showPersistentNotification__method_to_nsIAlertService.patch

I finally figured out that there should be a stub method in https://dxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/notification/MockServices.js too.

Comment 99

[SUN Haitao]

As for as I can tell, the try run result looks good enough. It should be fine to land all those patches and close this issue now.

Comment 100

[SUN Haitao]

Is there any extra step I should perform?

Comment 101

[Carsten Book [:Tomcat]]

(In reply to SUN Haitao from comment #100)
> Is there any extra step I should perform?

should be ok to request checkin needed

Comment 102

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/62e7c9ec49bc
Add a 'data' parameter to GeckoThread.createServices; r=jchen
https://hg.mozilla.org/integration/fx-team/rev/92af6b44d000
Add a 'showPersistentNotification' method to nsIAlertService. r=kcambridge
https://hg.mozilla.org/integration/fx-team/rev/88fd1577bab5
Rename 'serviceWorkerRegistrationID' to 'serviceWorkerRegistrationScope'. r=kcambridge
https://hg.mozilla.org/integration/fx-team/rev/db97cb1def67
Add a 'showPersistentAlertNotification' method to GeckoAppShell. r=kcambridge
https://hg.mozilla.org/integration/fx-team/rev/be179bb8c58f
Add a component to handle 'persisent-notification-click'. r=kcambridge

Comment 103

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/62e7c9ec49bc
https://hg.mozilla.org/mozilla-central/rev/92af6b44d000
https://hg.mozilla.org/mozilla-central/rev/88fd1577bab5
https://hg.mozilla.org/mozilla-central/rev/db97cb1def67
https://hg.mozilla.org/mozilla-central/rev/be179bb8c58f

Comment 104

[Gerry Chang [:gchang]]

Hi SUN, 
Can we let this ride the train from 50 or 49 and won't fix in 48?

Comment 105

[SUN Haitao]

This feature needs Clients.openWindow() to be practically useful. Since Bug 1262251 hasn't been finished, I think just letting this ride the train is the best plan.

Comment 106

[Gerry Chang [:gchang]]

Based on comment #105, mark 48 as won't fix.