Closed
Bug 1114554
Opened 10 years ago
Closed 10 years ago
Implement Notification ServiceWorker API
Categories
(Core :: DOM: Workers, defect)
Tracking
()
RESOLVED
FIXED
mozilla42
People
(Reporter: nsm, Assigned: nsm)
References
()
Details
(Keywords: dev-doc-complete)
Attachments
(11 files, 7 obsolete files)
39 bytes,
text/x-review-board-request
|
baku
:
review+
|
Details |
39 bytes,
text/x-review-board-request
|
baku
:
review+
|
Details |
39 bytes,
text/x-review-board-request
|
baku
:
review+
|
Details |
39 bytes,
text/x-review-board-request
|
Details | |
39 bytes,
text/x-review-board-request
|
Details | |
39 bytes,
text/x-review-board-request
|
Details | |
39 bytes,
text/x-review-board-request
|
Details | |
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 |
No description provided.
Assignee | ||
Updated•10 years ago
|
Depends on: ServiceWorkers-v1, 916893
Assignee | ||
Comment 1•10 years ago
|
||
/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)
Comment 2•10 years ago
|
||
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?
Comment 3•10 years ago
|
||
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.
Comment 4•10 years ago
|
||
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.
Assignee | ||
Comment 5•10 years ago
|
||
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.
Assignee | ||
Comment 6•10 years ago
|
||
https://reviewboard.mozilla.org/r/8289/#review7981
My hg export isn't showing any bad whitespace or FIXMEs :/ Was this comment for patch 3?
Assignee | ||
Comment 7•10 years ago
|
||
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/
Assignee | ||
Comment 8•10 years ago
|
||
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/
Assignee | ||
Comment 9•10 years ago
|
||
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/
Updated•10 years ago
|
Keywords: dev-doc-needed
Comment 11•10 years ago
|
||
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.
Comment 14•10 years ago
|
||
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.
Comment 15•10 years ago
|
||
https://reviewboard.mozilla.org/r/9303/#review9199
Update the XXXXXX to the correct bug number.
Comment 16•10 years ago
|
||
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?
Assignee | ||
Comment 17•10 years ago
|
||
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)
Assignee | ||
Comment 18•10 years ago
|
||
Assignee | ||
Comment 19•10 years ago
|
||
Assignee | ||
Comment 20•10 years ago
|
||
Assignee | ||
Comment 21•10 years ago
|
||
Assignee | ||
Comment 22•10 years ago
|
||
Assignee | ||
Comment 23•10 years ago
|
||
Assignee | ||
Comment 24•10 years ago
|
||
Assignee | ||
Comment 25•10 years ago
|
||
Assignee | ||
Comment 26•10 years ago
|
||
Assignee | ||
Comment 27•10 years ago
|
||
Assignee | ||
Comment 28•10 years ago
|
||
Assignee | ||
Comment 29•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
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)
Assignee | ||
Comment 30•10 years ago
|
||
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
Assignee | ||
Comment 31•10 years ago
|
||
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.
Assignee | ||
Comment 32•10 years ago
|
||
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)
Assignee | ||
Comment 33•10 years ago
|
||
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)
Assignee | ||
Comment 34•10 years ago
|
||
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.
Assignee | ||
Comment 35•10 years ago
|
||
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)
Assignee | ||
Comment 36•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Attachment #8618946 -
Attachment is obsolete: true
Attachment #8618946 -
Flags: review?(wchen)
Assignee | ||
Updated•10 years ago
|
Attachment #8618947 -
Attachment is obsolete: true
Attachment #8618947 -
Flags: review?(wchen)
Assignee | ||
Updated•10 years ago
|
Attachment #8618948 -
Attachment is obsolete: true
Attachment #8618948 -
Flags: review?(wchen)
Assignee | ||
Updated•10 years ago
|
Attachment #8618949 -
Attachment is obsolete: true
Attachment #8618949 -
Flags: review?(wchen)
Assignee | ||
Updated•10 years ago
|
Attachment #8618950 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8618951 -
Attachment is obsolete: true
Attachment #8618951 -
Flags: review?(wchen)
Assignee | ||
Updated•10 years ago
|
Attachment #8618952 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8618953 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8618954 -
Attachment is obsolete: true
Attachment #8618954 -
Flags: review?(wchen)
Assignee | ||
Updated•10 years ago
|
Attachment #8618955 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8618956 -
Attachment is obsolete: true
Attachment #8618956 -
Flags: review?(wchen)
Assignee | ||
Updated•10 years ago
|
Attachment #8618957 -
Attachment is obsolete: true
Attachment #8618957 -
Flags: review?(wchen)
Assignee | ||
Comment 37•10 years ago
|
||
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)
Assignee | ||
Comment 38•10 years ago
|
||
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.
Assignee | ||
Comment 39•10 years ago
|
||
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)
Assignee | ||
Comment 40•10 years ago
|
||
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.
Assignee | ||
Comment 41•10 years ago
|
||
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
Assignee | ||
Comment 42•10 years ago
|
||
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
Assignee | ||
Comment 43•10 years ago
|
||
Something is wrong with RB, but please review patches 2, 5, and 7.
Comment 44•10 years ago
|
||
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 45•10 years ago
|
||
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 46•10 years ago
|
||
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+
Assignee | ||
Comment 47•10 years ago
|
||
Assignee: nobody → nsm.nikhil
Attachment #8622682 -
Flags: review?(amarchesini)
Comment 48•10 years ago
|
||
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+
Assignee | ||
Comment 49•10 years ago
|
||
Assignee | ||
Comment 50•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8618950 -
Attachment is obsolete: false
Assignee | ||
Comment 51•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
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)
Assignee | ||
Comment 52•10 years ago
|
||
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.
Assignee | ||
Comment 53•10 years ago
|
||
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)
Assignee | ||
Comment 54•10 years ago
|
||
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
Assignee | ||
Comment 55•10 years ago
|
||
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)
Assignee | ||
Comment 56•10 years ago
|
||
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
Assignee | ||
Comment 57•10 years ago
|
||
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)
Assignee | ||
Comment 58•10 years ago
|
||
Ehsan, could I just get dom peer sign off on this. Thanks
Attachment #8626048 -
Flags: review?(ehsan)
Assignee | ||
Updated•10 years ago
|
Attachment #8618956 -
Flags: review?(wchen)
Assignee | ||
Updated•10 years ago
|
Attachment #8618954 -
Flags: review?(wchen)
Assignee | ||
Updated•10 years ago
|
Attachment #8618951 -
Flags: review?(wchen)
Comment 59•10 years ago
|
||
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+
Assignee | ||
Comment 60•10 years ago
|
||
Comment 61•10 years ago
|
||
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 62•10 years ago
|
||
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+
Assignee | ||
Comment 63•10 years ago
|
||
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•10 years ago
|
Attachment #8626048 -
Flags: review?(ehsan)
Assignee | ||
Comment 64•10 years ago
|
||
Comment 65•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8632258 -
Flags: review?(nsm.nikhil) → review+
Assignee | ||
Comment 66•10 years ago
|
||
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."""
Assignee | ||
Comment 67•10 years ago
|
||
Assignee | ||
Comment 68•10 years ago
|
||
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
Comment 69•10 years ago
|
||
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
Closed: 10 years ago
status-firefox42:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment 70•10 years ago
|
||
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.
Comment 71•9 years ago
|
||
Keywords: dev-doc-needed → dev-doc-complete
Comment 72•9 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:
--- → ?
Comment 73•9 years ago
|
||
(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)
Assignee | ||
Comment 74•9 years ago
|
||
Flags: needinfo?(nsm.nikhil)
Attachment #8660979 -
Flags: review?(dholbert)
Comment 75•9 years ago
|
||
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+
Assignee | ||
Comment 76•9 years ago
|
||
Assignee | ||
Comment 77•9 years ago
|
||
Only this new patch needs to land
Attachment #8660979 -
Attachment is obsolete: true
Attachment #8660995 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 78•9 years ago
|
||
Keywords: checkin-needed
Comment 79•9 years ago
|
||
Comment 80•9 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•