The default bug view has changed. See this FAQ.

Implement Notification ServiceWorker API

RESOLVED FIXED in Firefox 42

Status

()

Core
DOM: Workers
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: nsm, Assigned: nsm)

Tracking

({dev-doc-complete})

33 Branch
mozilla42
x86_64
Linux
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed, relnote-firefox 43+)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(11 attachments, 7 obsolete attachments)

39 bytes, text/x-review-board-request
baku
: review+
Details | Review
39 bytes, text/x-review-board-request
baku
: review+
Details | Review
39 bytes, text/x-review-board-request
baku
: review+
Details | Review
39 bytes, text/x-review-board-request
Details | Review
39 bytes, text/x-review-board-request
Details | Review
39 bytes, text/x-review-board-request
Details | Review
39 bytes, text/x-review-board-request
Details | Review
9.70 KB, patch
baku
: review+
Details | Diff | Splinter Review
43.69 KB, patch
Details | Diff | Splinter Review
1.46 KB, patch
nsm
: review+
Details | Diff | Splinter Review
1002 bytes, patch
nsm
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
Blocks: 1038811
Created attachment 8602391 [details]
MozReview Request: bz://1114554/nsm

/r/8285 - Bug 1114554 - Patch 1 - Notification ServiceWorker API stubs. r?wchen
/r/8287 - Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r?wchen
/r/8289 - Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope
/r/8291 - Bug 1114554 - Patch 4 - ServiceWorkerRegistration.getNotifications() on main thread
/r/8293 - getNotifications() on worker thread
/r/8295 - ServiceWorkerRegistration.getNotifications() tests

Pull down these commits:

hg pull -r 3d0f232588f28a06d450d4fe8fccf879596a9ba0 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8602391 - Flags: review?(wchen)
https://reviewboard.mozilla.org/r/8285/#review7573

::: dom/base/nsGkAtomList.h:825
(Diff revision 1)
> +GK_ATOM(onnotificationclick, "notificationclick")

notificationclick -> onnotificationclick

::: dom/webidl/PushManager.webidl:10
(Diff revision 1)
> -[JSImplementation="@mozilla.org/push/PushManager;1",
> +[JSImplementation="@mozilla.org/push/PushManager;1"]

Part of a different patch?
https://reviewboard.mozilla.org/r/8287/#review7679

::: dom/notification/Notification.cpp:197
(Diff revision 1)
> +    //XXXnsm Is it guaranteed mCallback will be called in case of failure?

I don't think you can make any guarantees about mCallback being called, there are a few places where the code can fail before getting to the callback. In fact, if the call to Get fails here, then mCallback is probably not going to be called.

::: dom/notification/Notification.cpp:1687
(Diff revision 1)
> +    p->MaybeReject(result);

It looks like we should return here.

::: dom/notification/Notification.cpp:1668
(Diff revision 1)
> +  NotificationPermission permission = NotificationPermission::Denied;

This block of code is almost identical to Notification::GetPermission, it may be worthwhile to factor it out. Also, will this ever be executed on main thread given that this is a service worker API?

::: dom/notification/Notification.h:250
(Diff revision 1)
> +  // Callers MUST bind the Notification to the correct DETH parent using

DETH?

::: dom/notification/Notification.h:335
(Diff revision 1)
> +  CreateAndShowUnsafe(nsIGlobalObject* aGlobal,

I'm not sure I see why this is unsafe or why we have a restriction against non-stack references. Once we return to reference to script, we can't stop it from getting put on the heap. Also, my understanding of NotificationRef is that it is meant to make lifecycle management a little easier within a function (so a function doesn't try to manually handle references after it passed ownership to a runnable or observer), rather than making uniquely owned references on the heap (because that's not going to happen when we give a reference to script).

::: dom/notification/NotificationStorage.js:202
(Diff revision 1)
> -        callback.handle(notification.id,
> +        Services.tm.currentThread.dispatch(

Perhaps add a comment that you're making this asynchronous to match the asynchronous behavour of fetching from the database.

::: dom/notification/Notification.cpp
(Diff revision 1)
> -    BindToOwner(window);

Is it necessary to remove this? It seems like all we've done is just moved BindToOwner to all the main thread callers.

::: dom/notification/Notification.h:294
(Diff revision 1)
> +    MOZ_ASSERT(!mAlertName.IsEmpty());

It looks like we are only using this method on the main thread so how about we get rid of SetAlertName and set it the first time we call GetAlertName.
https://reviewboard.mozilla.org/r/8289/#review7727

In general, there are a few fixmes and tab characters that should be fixed up in the uploaded patches.

::: dom/notification/NotificationStorage.js:7
(Diff revision 1)
> -const DEBUG = false;
> +const DEBUG = true;

This should be changed back to DEBUG = false

::: dom/notification/Notification.cpp:1324
(Diff revision 1)
> +    // Notification and can release ownership at the end of this function.

Instead of saying that we don't care about the Notification, how about just saying that the observer does not need a reference to the Notification.
https://reviewboard.mozilla.org/r/8287/#review7955

> It looks like we are only using this method on the main thread so how about we get rid of SetAlertName and set it the first time we call GetAlertName.

I've kept the function, but it is only called by GetAlertName().

> This block of code is almost identical to Notification::GetPermission, it may be worthwhile to factor it out. Also, will this ever be executed on main thread given that this is a service worker API?

This is a Service Worker API in the sense of being associated with SWs. The actual API is exposed on ServiceWorkerRegistration which is available on all threads.

> Is it necessary to remove this? It seems like all we've done is just moved BindToOwner to all the main thread callers.

I don't remember exactly why I had made these main thread only, it is likely that some version of my changes required that. I'll push a followup if this change isn't needed after i look at the rest of the patches.

> I'm not sure I see why this is unsafe or why we have a restriction against non-stack references. Once we return to reference to script, we can't stop it from getting put on the heap. Also, my understanding of NotificationRef is that it is meant to make lifecycle management a little easier within a function (so a function doesn't try to manually handle references after it passed ownership to a runnable or observer), rather than making uniquely owned references on the heap (because that's not going to happen when we give a reference to script).

I get what you mean now. The intention was "Don't hold weak/raw references to this Notification, since it may go away". I have modified the comment and method name.
https://reviewboard.mozilla.org/r/8289/#review7981

My hg export isn't showing any bad whitespace or FIXMEs :/ Was this comment for patch 3?
Comment on attachment 8602391 [details]
MozReview Request: bz://1114554/nsm

/r/8285 - Bug 1114554 - Patch 1 - Notification ServiceWorker API stubs. r?wchen
/r/8287 - Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r?wchen
/r/8289 - Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope
/r/8291 - Bug 1114554 - Patch 4 - ServiceWorkerRegistration.getNotifications() on main thread
/r/8293 - Bug 1114554 - Patch 5 - getNotifications() on worker thread.
/r/8295 - Bug 1114554 - Patch 6 - ServiceWorkerRegistration.getNotifications() tests.

Pull down these commits:

hg pull -r d6263edb1339aa78cdf29ee812de79493d966a8f https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8602391 [details]
MozReview Request: bz://1114554/nsm

/r/8285 - Bug 1114554 - Patch 7 - Call BindToOwner on all threads

Pull down this commit:

hg pull -r 350f7ab964f833300e889e6bcf0e3b75749b5732 https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8602391 [details]
MozReview Request: bz://1114554/nsm

/r/9293 - Bug 1114554 - Patch 1 - Notification ServiceWorker API stubs. r?wchen
/r/9295 - Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r?wchen
/r/9297 - Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope
/r/9299 - Bug 1114554 - Patch 4 - ServiceWorkerRegistration.getNotifications() on main thread
/r/9301 - Bug 1114554 - Patch 5 - getNotifications() on worker thread.
/r/9303 - Bug 1114554 - Patch 6 - ServiceWorkerRegistration.getNotifications() tests.
/r/8285 - Bug 1114554 - Patch 7 - Call BindToOwner on all threads

Pull down these commits:

hg pull -r 350f7ab964f833300e889e6bcf0e3b75749b5732 https://reviewboard-hg.mozilla.org/gecko/
Keywords: dev-doc-needed
Blocks: 1172006
https://reviewboard.mozilla.org/r/9293/#review9193
https://reviewboard.mozilla.org/r/9295/#review9187

::: dom/notification/Notification.cpp:1664
(Diff revision 1)
> +  // FIXME(nsm): ensure aGlobal's principal is allowed to load scope.

This FIXME

::: dom/workers/ServiceWorkerRegistration.cpp:986
(Diff revision 1)
> -  MOZ_ASSERT(false);
> +  // FIXME(nsm): Check for active worker.

This FIXME

::: dom/notification/Notification.cpp:701
(Diff revision 1)
> -  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(aGlobal.GetAsSupports());
> +  // FIXME(nsm): If the sticky flag is set, throw an error.

I was referring to these FIXMEs. It looks like we don't support "sticky" yet, so that comment is OK. It looks like you already took care of the next FIXME so you should remove it.

This patch looks like it has some tab characters in it.
https://reviewboard.mozilla.org/r/9297/#review9201
https://reviewboard.mozilla.org/r/9299/#review9203
https://reviewboard.mozilla.org/r/9301/#review9195

::: dom/notification/Notification.cpp:1808
(Diff revision 1)
> +      // FIXME(nsm): Handle this and other failures.

This FIXME. Although it looks like you're already handling errors so maybe you just forgot to remove the comment.
https://reviewboard.mozilla.org/r/9303/#review9199

Update the XXXXXX to the correct bug number.
https://reviewboard.mozilla.org/r/8285/#review9191

::: dom/notification/Notification.cpp:781
(Diff revision 3)
> -    notification->BindToOwner(aGlobal);
> +  notification->BindToOwner(aGlobal);

Can we do this in the notification constructor?
Comment on attachment 8602391 [details]
MozReview Request: bz://1114554/nsm
Attachment #8602391 - Attachment is obsolete: true
Attachment #8602391 - Flags: review?(wchen)
Attachment #8618946 - Flags: review?(wchen)
Attachment #8618947 - Flags: review?(wchen)
Attachment #8618948 - Flags: review?(wchen)
Attachment #8618949 - Flags: review?(wchen)
Attachment #8618950 - Flags: review?(wchen)
Attachment #8618951 - Flags: review?(wchen)
Attachment #8618952 - Flags: review?(wchen)
Attachment #8618953 - Flags: review?(wchen)
Attachment #8618954 - Flags: review?(wchen)
Attachment #8618955 - Flags: review?(wchen)
Attachment #8618956 - Flags: review?(wchen)
Attachment #8618957 - Flags: review?(wchen)
Created attachment 8618946 [details]
MozReview Request: Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope
Created attachment 8618947 [details]
MozReview Request: Bug 1114554 - Patch 4 - ServiceWorkerRegistration.getNotifications() on main thread
Created attachment 8618948 [details]
MozReview Request: Bug 1114554 - Patch 5 - getNotifications() on worker thread.
Created attachment 8618949 [details]
MozReview Request: Bug 1114554 - Patch 6 - ServiceWorkerRegistration.getNotifications() tests.
Created attachment 8618950 [details]
MozReview Request: Bug 1114554 - Patch 1 - Notification ServiceWorker API stubs. r=wchen
Created attachment 8618951 [details]
MozReview Request: Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r=wchen
Created attachment 8618952 [details]
MozReview Request: Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope. r=wchen
Created attachment 8618953 [details]
MozReview Request: Bug 1114554 - Patch 4 - ServiceWorkerRegistration.getNotifications() on main thread. r=wchen
Created attachment 8618954 [details]
MozReview Request: Bug 1114554 - Patch 5 - getNotifications() on worker thread. r=wchen
Created attachment 8618955 [details]
MozReview Request: Bug 1114554 - Patch 6 - ServiceWorkerRegistration.getNotifications() tests. r=wchen
Created attachment 8618956 [details]
MozReview Request: Bug 1114554 - Patch 7 - Call BindToOwner on all threads. r=wchen
Created attachment 8618957 [details]
MozReview Request: Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r?wchen
Attachment #8618950 - Attachment description: MozReview Request: Bug 1114554 - Patch 1 - Notification ServiceWorker API stubs. r?wchen → MozReview Request: Bug 1114554 - Patch 1 - Notification ServiceWorker API stubs. r=wchen
Attachment #8618950 - Flags: review?(wchen)
Comment on attachment 8618950 [details]
MozReview Request: Bug 1114554 - Patch 1 - Notification ServiceWorker API stubs. r=wchen

Bug 1114554 - Patch 1 - Notification ServiceWorker API stubs. r=wchen
Comment on attachment 8618951 [details]
MozReview Request: Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r=wchen

Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r?wchen

Refactor creation and show dispatch so Notification constructor and showNotification can use it.
Move persistence to ShowInternal.
NotificationStorage calls callback async even when fetching from cache, simply to have similar semantics.
Calls to Notification::Get() are performed async since persistence is now async after being moved to ShowInternal().
Both are in accordance with the spec where the "append to list of notifications" operation is performed in the "show steps" which are performed in parallel from API invocations.
Comment on attachment 8618952 [details]
MozReview Request: Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope. r=wchen

Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope
Attachment #8618952 - Flags: review?(wchen)
Comment on attachment 8618953 [details]
MozReview Request: Bug 1114554 - Patch 4 - ServiceWorkerRegistration.getNotifications() on main thread. r=wchen

Bug 1114554 - Patch 4 - ServiceWorkerRegistration.getNotifications() on main thread
Attachment #8618953 - Flags: review?(wchen)
Comment on attachment 8618954 [details]
MozReview Request: Bug 1114554 - Patch 5 - getNotifications() on worker thread. r=wchen

Bug 1114554 - Patch 5 - getNotifications() on worker thread.
Comment on attachment 8618955 [details]
MozReview Request: Bug 1114554 - Patch 6 - ServiceWorkerRegistration.getNotifications() tests. r=wchen

Bug 1114554 - Patch 6 - ServiceWorkerRegistration.getNotifications() tests.
Attachment #8618955 - Flags: review?(wchen)
Comment on attachment 8618956 [details]
MozReview Request: Bug 1114554 - Patch 7 - Call BindToOwner on all threads. r=wchen

Bug 1114554 - Patch 7 - Call BindToOwner on all threads
Attachment #8618946 - Attachment is obsolete: true
Attachment #8618946 - Flags: review?(wchen)
Attachment #8618947 - Attachment is obsolete: true
Attachment #8618947 - Flags: review?(wchen)
Attachment #8618948 - Attachment is obsolete: true
Attachment #8618948 - Flags: review?(wchen)
Attachment #8618949 - Attachment is obsolete: true
Attachment #8618949 - Flags: review?(wchen)
Attachment #8618950 - Attachment is obsolete: true
Attachment #8618951 - Attachment is obsolete: true
Attachment #8618951 - Flags: review?(wchen)
Attachment #8618952 - Attachment is obsolete: true
Attachment #8618953 - Attachment is obsolete: true
Attachment #8618954 - Attachment is obsolete: true
Attachment #8618954 - Flags: review?(wchen)
Attachment #8618955 - Attachment is obsolete: true
Attachment #8618956 - Attachment is obsolete: true
Attachment #8618956 - Flags: review?(wchen)
Attachment #8618957 - Attachment is obsolete: true
Attachment #8618957 - Flags: review?(wchen)
Comment on attachment 8618951 [details]
MozReview Request: Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r=wchen

Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r?wchen

Refactor creation and show dispatch so Notification constructor and showNotification can use it.
Move persistence to ShowInternal.
NotificationStorage calls callback async even when fetching from cache, simply to have similar semantics.
Calls to Notification::Get() are performed async since persistence is now async after being moved to ShowInternal().
Both are in accordance with the spec where the "append to list of notifications" operation is performed in the "show steps" which are performed in parallel from API invocations.
Attachment #8618951 - Attachment is obsolete: false
Attachment #8618951 - Flags: review?(wchen)
Comment on attachment 8618951 [details]
MozReview Request: Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r=wchen

Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r?wchen

Refactor creation and show dispatch so Notification constructor and showNotification can use it.
Move persistence to ShowInternal.
NotificationStorage calls callback async even when fetching from cache, simply to have similar semantics.
Calls to Notification::Get() are performed async since persistence is now async after being moved to ShowInternal().
Both are in accordance with the spec where the "append to list of notifications" operation is performed in the "show steps" which are performed in parallel from API invocations.
Comment on attachment 8618952 [details]
MozReview Request: Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope. r=wchen

Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope
Attachment #8618952 - Attachment is obsolete: false
Attachment #8618952 - Flags: review?(wchen)
Comment on attachment 8618951 [details]
MozReview Request: Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r=wchen

Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r?wchen

Refactor creation and show dispatch so Notification constructor and showNotification can use it.
Move persistence to ShowInternal.
NotificationStorage calls callback async even when fetching from cache, simply to have similar semantics.
Calls to Notification::Get() are performed async since persistence is now async after being moved to ShowInternal().
Both are in accordance with the spec where the "append to list of notifications" operation is performed in the "show steps" which are performed in parallel from API invocations.
Comment on attachment 8618952 [details]
MozReview Request: Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope. r=wchen

Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope
Comment on attachment 8618952 [details]
MozReview Request: Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope. r=wchen

Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope
Something is wrong with RB, but please review patches 2, 5, and 7.
Comment on attachment 8618951 [details]
MozReview Request: Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r=wchen

https://reviewboard.mozilla.org/r/9295/#review9659
Attachment #8618951 - Flags: review?(wchen) → review+
Comment on attachment 8618954 [details]
MozReview Request: Bug 1114554 - Patch 5 - getNotifications() on worker thread. r=wchen

https://reviewboard.mozilla.org/r/9301/#review9671

::: dom/notification/Notification.cpp:1754
(Diff revisions 1 - 2)
>      MutexAutoLock lock(mPromiseProxy->GetCleanUpLock());

Let's also add MOZ_ASSERT(mPromiseProxy) here to make it easier to catch multiple calls to Done.
Attachment #8618954 - Flags: review+
Comment on attachment 8618956 [details]
MozReview Request: Bug 1114554 - Patch 7 - Call BindToOwner on all threads. r=wchen

https://reviewboard.mozilla.org/r/8285/#review9673

::: dom/notification/Notification.h:292
(Diff revision 4)
>    // BindToOwner().

We can get rid of this comment now.
Attachment #8618956 - Flags: review+
Created attachment 8622682 [details] [diff] [review]
Patch 3.1 - ServiceWorker principal fixes.
Assignee: nobody → nsm.nikhil
Attachment #8622682 - Flags: review?(amarchesini)
Comment on attachment 8622682 [details] [diff] [review]
Patch 3.1 - ServiceWorker principal fixes.

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

::: dom/notification/Notification.cpp
@@ +1004,2 @@
>    {
>      AssertIsOnMainThread();

MOZ_ASSERT(aPrincipal)

@@ +1195,1 @@
>    {

MOZ_ASSERT(aPrincipal)
then if it's more than 80chars, new line.
Attachment #8622682 - Flags: review?(amarchesini) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=975170035cf0
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c0faf010556
Attachment #8618950 - Attachment is obsolete: false
Comment on attachment 8618950 [details]
MozReview Request: Bug 1114554 - Patch 1 - Notification ServiceWorker API stubs. r=wchen

Bug 1114554 - Patch 1 - Notification ServiceWorker API stubs. r=wchen
Attachment #8618951 - Attachment description: MozReview Request: Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r?wchen → MozReview Request: Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r=wchen
Attachment #8618951 - Flags: review+ → review?(wchen)
Comment on attachment 8618951 [details]
MozReview Request: Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r=wchen

Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r=wchen

Refactor creation and show dispatch so Notification constructor and showNotification can use it.
Move persistence to ShowInternal.
NotificationStorage calls callback async even when fetching from cache, simply to have similar semantics.
Calls to Notification::Get() are performed async since persistence is now async after being moved to ShowInternal().
Both are in accordance with the spec where the "append to list of notifications" operation is performed in the "show steps" which are performed in parallel from API invocations.
Comment on attachment 8618952 [details]
MozReview Request: Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope. r=wchen

Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope. r=wchen


Bug 1114554 - Patch 3.1 - ServiceWorker principal fixes. r=baku

Bug 1162088 introduced origin attributes that ServiceWorkerManager callers have to use. This patch updates notificationclick events to work.
Attachment #8618952 - Attachment description: MozReview Request: Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope → MozReview Request: Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope. r=wchen
Attachment #8618952 - Flags: review?(wchen)
Comment on attachment 8618953 [details]
MozReview Request: Bug 1114554 - Patch 4 - ServiceWorkerRegistration.getNotifications() on main thread. r=wchen

Bug 1114554 - Patch 4 - ServiceWorkerRegistration.getNotifications() on main thread. r=wchen
Attachment #8618953 - Attachment description: MozReview Request: Bug 1114554 - Patch 4 - ServiceWorkerRegistration.getNotifications() on main thread → MozReview Request: Bug 1114554 - Patch 4 - ServiceWorkerRegistration.getNotifications() on main thread. r=wchen
Attachment #8618953 - Attachment is obsolete: false
Comment on attachment 8618954 [details]
MozReview Request: Bug 1114554 - Patch 5 - getNotifications() on worker thread. r=wchen

Bug 1114554 - Patch 5 - getNotifications() on worker thread. r=wchen
Attachment #8618954 - Attachment description: MozReview Request: Bug 1114554 - Patch 5 - getNotifications() on worker thread. → MozReview Request: Bug 1114554 - Patch 5 - getNotifications() on worker thread. r=wchen
Attachment #8618954 - Attachment is obsolete: false
Attachment #8618954 - Flags: review+ → review?(wchen)
Comment on attachment 8618955 [details]
MozReview Request: Bug 1114554 - Patch 6 - ServiceWorkerRegistration.getNotifications() tests. r=wchen

Bug 1114554 - Patch 6 - ServiceWorkerRegistration.getNotifications() tests. r=wchen
Attachment #8618955 - Attachment description: MozReview Request: Bug 1114554 - Patch 6 - ServiceWorkerRegistration.getNotifications() tests. → MozReview Request: Bug 1114554 - Patch 6 - ServiceWorkerRegistration.getNotifications() tests. r=wchen
Attachment #8618955 - Attachment is obsolete: false
Comment on attachment 8618956 [details]
MozReview Request: Bug 1114554 - Patch 7 - Call BindToOwner on all threads. r=wchen

Bug 1114554 - Patch 7 - Call BindToOwner on all threads. r=wchen
Attachment #8618956 - Attachment description: MozReview Request: Bug 1114554 - Patch 7 - Call BindToOwner on all threads → MozReview Request: Bug 1114554 - Patch 7 - Call BindToOwner on all threads. r=wchen
Attachment #8618956 - Attachment is obsolete: false
Attachment #8618956 - Flags: review+ → review?(wchen)
Created attachment 8626048 [details] [diff] [review]
notificationevent patch

Ehsan, could I just get dom peer sign off on this. Thanks
Attachment #8626048 - Flags: review?(ehsan)
Attachment #8618956 - Flags: review?(wchen)
Attachment #8618954 - Flags: review?(wchen)
Attachment #8618951 - Flags: review?(wchen)
Comment on attachment 8618952 [details]
MozReview Request: Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope. r=wchen

https://reviewboard.mozilla.org/r/9297/#review10457

I just reviewed the WebIDL file and it looks good to me.

::: dom/notification/NotificationEvent.h:63
(Diff revision 3)
> +  {

This can be:

Notification* Notification_() const
{
  return mNotification;
}
Attachment #8618952 - Flags: review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cc25f539b1f
Comment on attachment 8618951 [details]
MozReview Request: Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r=wchen

https://reviewboard.mozilla.org/r/9295/#review10477

Ship It!
Attachment #8618951 - Flags: review+
Comment on attachment 8618950 [details]
MozReview Request: Bug 1114554 - Patch 1 - Notification ServiceWorker API stubs. r=wchen

https://reviewboard.mozilla.org/r/9293/#review10479

Ship It!
Attachment #8618950 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d860fd12534
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce8a768782f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/75324b6862a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/f92abe5ec784
https://hg.mozilla.org/integration/mozilla-inbound/rev/15bdf9c78e6e
https://hg.mozilla.org/integration/mozilla-inbound/rev/404830c1ecf7
https://hg.mozilla.org/integration/mozilla-inbound/rev/5578d5c280a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/247ca4bf258e

Updated

2 years ago
Attachment #8626048 - Flags: review?(ehsan)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cbf74b9d533

Comment 65

2 years ago
Created attachment 8632258 [details] [diff] [review]
b2g-desktop crash fix

We crashed when a notification was removed and the application that created the notification had previously been killed.
It happened only in b2g-desktop. This fixes the crash and we need it until we remove the close event from implementation.

Nikhil I assigned you as a reviewer, but if you want, you can forward it to someone else.
Attachment #8632258 - Flags: review?(nsm.nikhil)
Attachment #8632258 - Flags: review?(nsm.nikhil) → review+
Quoting email from Robert about why this check is enough to prevent the crash. Note that this issue affects b2g-desktop (where the same process seems to be used for apps) and is more of a testing issue than something that could happen in production.

"""The thing is if it's the case of b2g where we have an app as a different process,
AlertsHelper.jsm is gonna see the listener as invalid (the process was killed right?) and unpersist the notification by itself from the parent process (and it's going to send a system message to start the app and notify it about the close event, but this is not gonna be the case in the nearest future).

If it's the case of b2g-desktop then the listener is gonna be seen as valid from AlertsHelper.jsm 
(AlertsService.js does not just die right away after the app's window is killed -- b2g-desktop is using the same process for its apps--), so an async message is sent from AlertsHelper.jsm to AlertsService.js, then

AlertsService.js is trying to notify the oberver by calling .observe("alertfinished"), this is where we crashed because the window is not valid anymore, but the observer was still alive (maybe still not GC'd yet?).
If we throw an error from Notification.cpp in this use-case, AlertsService.js is gonna catch the exception and unpersist the notification by itself."""
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7e527b495b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/22f2bd48a54a
https://hg.mozilla.org/integration/mozilla-inbound/rev/efc270ffbd34
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e06a7f9dbb0
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c560af0970a
https://hg.mozilla.org/integration/mozilla-inbound/rev/315e191855b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/dab42661d239
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f2f2f3f4c4d
https://hg.mozilla.org/integration/mozilla-inbound/rev/a737b7b67901
https://hg.mozilla.org/integration/mozilla-inbound/rev/deff1c84785f
https://hg.mozilla.org/mozilla-central/rev/22f2bd48a54a
https://hg.mozilla.org/mozilla-central/rev/efc270ffbd34
https://hg.mozilla.org/mozilla-central/rev/2e06a7f9dbb0
https://hg.mozilla.org/mozilla-central/rev/8c560af0970a
https://hg.mozilla.org/mozilla-central/rev/315e191855b2
https://hg.mozilla.org/mozilla-central/rev/dab42661d239
https://hg.mozilla.org/mozilla-central/rev/1f2f2f3f4c4d
https://hg.mozilla.org/mozilla-central/rev/a737b7b67901
https://hg.mozilla.org/mozilla-central/rev/deff1c84785f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Relevant compat information added to:

https://developer.mozilla.org/en-US/docs/Web/API/Notifications_API
https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerGlobalScope
https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerGlobalScope/onnotificationclick
https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration
https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration/getNotifications
https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration/showNotification

Also needs to be added to Firefox 42 for developers, when Jean-Yves creates it, so I've not ddc'd the bug yet.
Added https://developer.mozilla.org/en-US/Firefox/Releases/42#Service_Workers
Keywords: dev-doc-needed → dev-doc-complete

Comment 72

2 years ago
Release Note Request (optional, but appreciated)
[Why is this notable]: "Real" Push Notifications for Websites available in your favourite browser!
[Suggested wording]: Implemented Notifications API for Service Workers
[Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/docs/Web/API/Notifications_API
relnote-firefox: --- → ?
(In reply to William Chen [:wchen] from comment #4)
> ::: dom/notification/NotificationStorage.js:7
> (Diff revision 1)
> > -const DEBUG = false;
> > +const DEBUG = true;
> 
> This should be changed back to DEBUG = false

This doesn't seem to have happened -- the patch that landed here included this DEBUG = true change:
 http://hg.mozilla.org/mozilla-central/rev/2e06a7f9dbb0#l7.10

nsm, can that change be reverted?

(I ran across this because I saw some various NotificationStorage.js terminalspew for various things on mail.google.com today, in up-to-date Nightly with my normal browsing profile. I dug in to find where that logging was coming from, and ran across this DEBUG variable which seems to control logging in this file.)
Flags: needinfo?(nsm.nikhil)
Created attachment 8660979 [details] [diff] [review]
disable debugging
Flags: needinfo?(nsm.nikhil)
Attachment #8660979 - Flags: review?(dholbert)
Comment on attachment 8660979 [details] [diff] [review]
disable debugging

Looks good to me - thanks!

(One optional nit: in the commit message, maybe s/debugging/debug logging/? Unless there's further stuff [beyond logging] that this variable controls.)
Attachment #8660979 - Flags: review?(dholbert) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d862877309f
Created attachment 8660995 [details] [diff] [review]
disable debugging

Only this new patch needs to land
Attachment #8660979 - Attachment is obsolete: true
Attachment #8660995 - Flags: review+
Keywords: checkin-needed

Comment 78

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9738f1b3f11a
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9738f1b3f11a
This has been mentioned in the 42 release notes with "Ship Push messaging with disabled web notifications from ServiceWorkers" as wording. I moved it to 43 as it has been disabled in 42 by bug 1208560.
relnote-firefox: ? → 43+
Depends on: 1255298
You need to log in before you can comment on or make changes to this bug.