Make persistent notifications work like the spec.

RESOLVED FIXED in Firefox 50

Status

()

Firefox for Android
General
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: SUN Haitao, Assigned: SUN Haitao)

Tracking

(Blocks: 3 bugs)

Trunk
Firefox 50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48+ wontfix, firefox49 affected, fennec48+, firefox50 fixed)

Details

(URL)

Attachments

(6 attachments, 17 obsolete attachments)

1.18 KB, patch
jchen
: review+
Details | Diff | Splinter Review
1.61 KB, patch
Tomcat
: checkin+
Details | Diff | Splinter Review
8.04 KB, patch
kitcambridge
: review+
Details | Diff | Splinter Review
10.73 KB, patch
kitcambridge
: review+
Tomcat
: checkin+
Details | Diff | Splinter Review
14.97 KB, patch
Details | Diff | Splinter Review
6.56 KB, patch
kitcambridge
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Assignee: nobody → sunhaitao
Blocks: 1264797
(Assignee)

Comment 1

2 years ago
Some mechanisms introduced in Bug 1252650 is planned to be reused.
Depends on: 1252650
(Assignee)

Comment 2

2 years ago
Created attachment 8742117 [details] [diff] [review]
1-Bug_1252650___Add_a__data__parameter_to_GeckoThread_createServices.patch

Based on changeset 101ccfb6b257 & 587da5c67437.
Attachment #8742117 - Flags: review?(nchen)
(Assignee)

Comment 3

2 years ago
Created attachment 8742118 [details] [diff] [review]
2-Bug_1252650___Add_a__showAlertInternal__method_to_nsIAlertService.patch
Attachment #8742118 - Flags: review?(nalexander)
(Assignee)

Comment 4

2 years ago
Created attachment 8742119 [details] [diff] [review]
3-Bug_1252650___Add_a_component_to_build_notification_persistent_data.patch
Attachment #8742119 - Flags: review?(nalexander)
(Assignee)

Comment 5

2 years ago
Created attachment 8742120 [details] [diff] [review]
4-Bug_1252650___Add_a__showPersistentAlertNotification__method_to_GeckoAppShell.patch
Attachment #8742120 - Flags: review?(nalexander)
(Assignee)

Comment 6

2 years ago
Created attachment 8742121 [details] [diff] [review]
5-Bug_1252650___Add_a_component_to_handle__persisent_notification_click_.patch
Attachment #8742121 - Flags: review?(nalexander)
(Assignee)

Comment 7

2 years ago
Created attachment 8742122 [details] [diff] [review]
6-Bug_1252650___Make_services_can_show_notifications.patch
Attachment #8742122 - Flags: review?(nchen)
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
Attachment #8742117 - Flags: review?(nchen) → review+
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?
(Assignee)

Comment 10

2 years ago
(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.
jchen: can you review the entirety of this patch series?  Thanks!
Flags: needinfo?(nchen)
(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.
Flags: needinfo?(nchen)
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.
Attachment #8742118 - Flags: review?(nalexander) → review?(kcambridge)
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.
Attachment #8742122 - Flags: review?(nchen) → feedback+
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).
Attachment #8742119 - Flags: review?(nalexander) → review-
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.
Attachment #8742120 - Flags: review?(nalexander) → review?(kcambridge)
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.
Attachment #8742118 - Flags: review?(kcambridge) → review-
(Assignee)

Comment 18

2 years ago
(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.
Flags: needinfo?(nchen)
(Assignee)

Comment 19

2 years ago
(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.
(Assignee)

Comment 20

2 years ago
(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.
Flags: needinfo?(kcambridge)
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.
Attachment #8742120 - Flags: review?(kcambridge) → review?(wchen)
(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.
Flags: needinfo?(kcambridge)
(Assignee)

Comment 23

2 years ago
(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.
(Assignee)

Comment 24

2 years ago
(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.
(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.
Flags: needinfo?(nchen)
(Assignee)

Comment 26

2 years ago
Created attachment 8742812 [details] [diff] [review]
6-Bug_1252650___Make_services_can_show_notifications.patch
Attachment #8742122 - Attachment is obsolete: true
Attachment #8742812 - Flags: review?(nchen)
(Assignee)

Comment 27

2 years ago
(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 on attachment 8742812 [details] [diff] [review]
6-Bug_1252650___Make_services_can_show_notifications.patch

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

Thanks!
Attachment #8742812 - Flags: review?(nchen) → review+
(Assignee)

Comment 29

2 years ago
(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?
Flags: needinfo?(nchen)
(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.
Flags: needinfo?(nchen)
(Assignee)

Comment 31

2 years ago
(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 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.
Attachment #8742120 - Flags: review?(wchen) → review-
(Assignee)

Comment 33

2 years ago
(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.
(Assignee)

Comment 34

2 years ago
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?
Flags: needinfo?(wchen)
Flags: needinfo?(kcambridge)
Depends on: 1267419
I landed part 6 early to make bug 1267419 work.
Keywords: leave-open
tracking-fennec: --- → ?
(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.
(Assignee)

Comment 37

2 years ago
(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.
(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.
Flags: needinfo?(wchen)
(Assignee)

Updated

2 years ago
Depends on: 1265828
(Assignee)

Comment 39

2 years ago
(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 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.
Flags: needinfo?(kcambridge)
Attachment #8742118 - Flags: review-
(Assignee)

Comment 41

2 years ago
(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 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).
Attachment #8742121 - Flags: review?(nalexander)

Updated

2 years ago
Blocks: 1269921
[Tracking Requested - why for this release]: needed for Push Notifications feature
tracking-fennec: ? → 48+
tracking-firefox48: --- → ?
Blocks: 1262251
Any updates on the new approach, Haitao?
Flags: needinfo?(sunhaitao)
(Assignee)

Comment 45

2 years ago
Almost done. Some extra stubs for desktop platforms may be needed. Is there a mechanism to make a method of XPCOM Interface Android-only?
Flags: needinfo?(sunhaitao) → needinfo?(kcambridge)
(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.
Flags: needinfo?(kcambridge)
(Assignee)

Comment 47

2 years ago
In that case, Let's review the main part and forge stubs in parallel.
(Assignee)

Comment 48

2 years ago
Created attachment 8751564 [details] [diff] [review]
1-Bug_1252650___Add_a__data__parameter_to_GeckoThread_createServices.patch

Slightly modified to avoid changing existing codes that call `getIntentToCreateServices`.
Attachment #8742117 - Attachment is obsolete: true
Attachment #8751564 - Flags: review?(nchen)
(Assignee)

Comment 49

2 years ago
Created attachment 8751565 [details] [diff] [review]
2-Bug_1252650___Add_a__showPersistentNotification__method_to_nsIAlertService.patch
Attachment #8742118 - Attachment is obsolete: true
Attachment #8751565 - Flags: review?(kcambridge)
(Assignee)

Comment 50

2 years ago
Created attachment 8751567 [details] [diff] [review]
4-Bug_1252650___Add_a__showPersistentAlertNotification__method_to_GeckoAppShell.patch
Attachment #8742119 - Attachment is obsolete: true
Attachment #8742120 - Attachment is obsolete: true
Attachment #8751567 - Flags: review?(kcambridge)
(Assignee)

Comment 51

2 years ago
Created attachment 8751572 [details] [diff] [review]
5-Bug_1252650___Add_a_component_to_handle__persisent_notification_click_.patch
Attachment #8742121 - Attachment is obsolete: true
Attachment #8751572 - Flags: review?(kcambridge)
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);
Attachment #8751564 - Flags: review?(nchen) → review+
(Assignee)

Comment 53

2 years ago
(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?
Blocks: 1269920
(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.
(Assignee)

Comment 55

2 years ago
Created attachment 8752590 [details] [diff] [review]
1-Bug_1252650___Add_a__data__parameter_to_GeckoThread_createServices.patch
Attachment #8751564 - Attachment is obsolete: true
Attachment #8752590 - Flags: checkin?
(Assignee)

Comment 56

2 years ago
Created attachment 8752593 [details] [diff] [review]
2-Bug_1252650___Add_a__showPersistentNotification__method_to_nsIAlertService.patch

Desktop platform stubs are included.
Attachment #8751565 - Attachment is obsolete: true
Attachment #8751565 - Flags: review?(kcambridge)
Attachment #8752593 - Flags: review?(kcambridge)
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.
Attachment #8751572 - Flags: review?(kcambridge) → review+
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.
Attachment #8752593 - Flags: review?(kcambridge) → review+
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"?
Attachment #8751567 - Flags: review?(wchen)
Attachment #8751567 - Flags: review?(nchen)
Attachment #8751567 - Flags: review?(kcambridge)
(Assignee)

Comment 60

2 years ago
Created attachment 8753112 [details] [diff] [review]
2-Bug_1252650___Add_a__showPersistentNotification__method_to_nsIAlertService.patch

Sorry, I uploaded a wrong version.
Attachment #8752593 - Attachment is obsolete: true
Attachment #8753112 - Flags: review?(kcambridge)
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.
Attachment #8753112 - Flags: review?(kcambridge)
(Assignee)

Comment 62

2 years ago
Created attachment 8753114 [details] [diff] [review]
2-Bug_1264815___Add_a__showPersistentNotification__method_to_nsIAlertService.patch

My fault, I made the same mistake once again.
Attachment #8753112 - Attachment is obsolete: true
Attachment #8753114 - Flags: review?(kcambridge)
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!
Attachment #8753114 - Flags: review?(kcambridge) → review+
(Assignee)

Comment 64

2 years ago
(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.
(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 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
Attachment #8752590 - Flags: checkin? → checkin+
(Assignee)

Comment 67

2 years ago
(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?
(Assignee)

Comment 68

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

So no event will fire twice.
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)
Attachment #8751567 - Flags: review?(nchen) → review+
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.
Attachment #8751567 - Flags: review?(wchen) → review+
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.
(Assignee)

Comment 72

2 years ago
Created attachment 8757569 [details] [diff] [review]
1-Bug_1264815___Add_a__data__parameter_to_GeckoThread_createServices__r_jchen.patch

The rebased right version that includes suggested modifications in Comment 52.
Attachment #8752590 - Attachment is obsolete: true
Attachment #8757569 - Flags: checkin?
(Assignee)

Comment 73

2 years ago
Created attachment 8757572 [details] [diff] [review]
3-Bug_1264815___Rename__serviceWorkerRegistrationID__to__serviceWorkerRegistrationScope_.patch

Rename `serviceWorkerRegistrationID` to `serviceWorkerRegistrationScope` to make things clearer.
Attachment #8757572 - Flags: review?(kcambridge)
(Assignee)

Comment 74

2 years ago
Created attachment 8757576 [details] [diff] [review]
5-Bug_1264815___Add_a_component_to_handle__persisent_notification_click_.patch
Attachment #8751572 - Attachment is obsolete: true
Attachment #8757576 - Flags: review?(kcambridge)
(Assignee)

Comment 75

2 years ago
Created attachment 8757577 [details] [diff] [review]
4-Bug_1264815___Add_a__showPersistentAlertNotification__method_to_GeckoAppShell.patch

Did I understand you right?
Attachment #8751567 - Attachment is obsolete: true
Attachment #8757577 - Flags: review?(wchen)
(Assignee)

Updated

2 years ago
Attachment #8753114 - Flags: checkin?
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.
Attachment #8757572 - Flags: review?(kcambridge) → review+
Comment on attachment 8757576 [details] [diff] [review]
5-Bug_1264815___Add_a_component_to_handle__persisent_notification_click_.patch

Looks great!
Attachment #8757576 - Flags: review?(kcambridge) → review+
(Assignee)

Comment 78

2 years ago
Created attachment 8757669 [details] [diff] [review]
3-Bug_1264815___Rename__serviceWorkerRegistrationID__to__serviceWorkerRegistrationScope_.patch

I forgot them for using a case-sensitive search.
Attachment #8757669 - Flags: review?(kcambridge)
(Assignee)

Comment 79

2 years ago
Created attachment 8757670 [details] [diff] [review]
3-Bug_1264815___Rename__serviceWorkerRegistrationID__to__serviceWorkerRegistrationScope_.patch
Attachment #8757572 - Attachment is obsolete: true
Attachment #8757669 - Attachment is obsolete: true
Attachment #8757669 - Flags: review?(kcambridge)
Attachment #8757670 - Flags: review?(kcambridge)
Attachment #8757670 - Flags: review?(kcambridge) → review+
(Assignee)

Updated

2 years ago
Attachment #8757670 - Flags: checkin?
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
Flags: needinfo?(sunhaitao)
(Assignee)

Comment 81

2 years ago
I believe this is because part 6 already landed earlier. See https://hg.mozilla.org/mozilla-central/rev/8d30d67837bd.
Flags: needinfo?(sunhaitao)
(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

2 years ago
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

Updated

2 years ago
Attachment #8753114 - Flags: checkin? → checkin+

Updated

2 years ago
Attachment #8757569 - Flags: checkin? → checkin+

Updated

2 years ago
Attachment #8757670 - Flags: checkin? → checkin+
(Assignee)

Comment 84

2 years ago
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.
Keywords: leave-open
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
Flags: needinfo?(sunhaitao)

Comment 86

2 years ago
Backout by cbook@mozilla.com:
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
(Assignee)

Comment 87

2 years ago
Created attachment 8758114 [details] [diff] [review]
4-Bug_1264815___Add_a__showPersistentAlertNotification__method_to_GeckoAppShell.patch

Try this version?
Attachment #8757577 - Attachment is obsolete: true
Attachment #8757577 - Flags: review?(wchen)
Flags: needinfo?(sunhaitao) → needinfo?(cbook)
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 ?
Flags: needinfo?(cbook)
(Assignee)

Comment 89

2 years ago
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.
(Assignee)

Comment 90

2 years ago
But I haven't got Level 1 Commit Access yet, what's the best plan now?
Flags: needinfo?(cbook)
kcambridge, since you reviewed some of the patches here, do you think you could vouch for L1 access ?
Flags: needinfo?(cbook) → needinfo?(kcambridge)
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.
Flags: needinfo?(kcambridge) → needinfo?(sunhaitao)
(Assignee)

Comment 93

2 years ago
https://bugzilla.mozilla.org/show_bug.cgi?id=1277130
Flags: needinfo?(sunhaitao) → needinfo?(kcambridge)
(Assignee)

Comment 94

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eccb34e05630
(Assignee)

Updated

2 years ago
Flags: needinfo?(kcambridge)
(Assignee)

Comment 95

2 years ago
What's next then?

BTW, is there a recommended way to tell a test failure is known or new?
Flags: needinfo?(cbook)
(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
Flags: needinfo?(cbook)
Keywords: dev-doc-needed
(Assignee)

Comment 97

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=902fdaec3477
(Assignee)

Comment 98

2 years ago
Created attachment 8763232 [details] [diff] [review]
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.
Attachment #8753114 - Attachment is obsolete: true
Attachment #8763232 - Flags: review?(kcambridge)
Attachment #8763232 - Flags: review?(kcambridge) → review+
(Assignee)

Comment 99

2 years ago
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.
(Assignee)

Comment 100

2 years ago
Is there any extra step I should perform?
Flags: needinfo?(cbook)
(In reply to SUN Haitao from comment #100)
> Is there any extra step I should perform?

should be ok to request checkin needed
Flags: needinfo?(cbook)
Keywords: checkin-needed

Comment 102

2 years ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed

Comment 103

2 years ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
(Assignee)

Updated

2 years ago
Blocks: 1282222

Updated

2 years ago
tracking-firefox48: ? → +

Updated

2 years ago
status-firefox49: --- → affected
Hi SUN, 
Can we let this ride the train from 50 or 49 and won't fix in 48?
Flags: needinfo?(sunhaitao)
(Assignee)

Comment 105

2 years ago
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.
Flags: needinfo?(sunhaitao)
Based on comment #105, mark 48 as won't fix.
status-firefox48: affected → wontfix
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.