1114554 - Implement Notification ServiceWorker API

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: 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/

Comment 2

[William Chen [: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?

Comment 3

[William Chen [:wchen]]

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

[William Chen [:wchen]]

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.

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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/

Comment 10

[William Chen [:wchen]]

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

Comment 11

[William Chen [:wchen]]

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 12

[William Chen [:wchen]]

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

Comment 13

[William Chen [:wchen]]

https://reviewboard.mozilla.org/r/9299/#review9203

Comment 14

[William Chen [:wchen]]

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

[William Chen [:wchen]]

https://reviewboard.mozilla.org/r/9303/#review9199

Update the XXXXXX to the correct bug number.

Comment 16

[William Chen [:wchen]]

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 18

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope

Comment 19

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1114554 - Patch 4 - ServiceWorkerRegistration.getNotifications() on main thread

Comment 20

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1114554 - Patch 5 - getNotifications() on worker thread.

Comment 21

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1114554 - Patch 6 - ServiceWorkerRegistration.getNotifications() tests.

Comment 22

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1114554 - Patch 1 - Notification ServiceWorker API stubs. r=wchen

Comment 23

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r=wchen

Comment 24

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1114554 - Patch 3 - Fire notificationclick event on ServiceWorkerGlobalScope. r=wchen

Comment 25

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1114554 - Patch 4 - ServiceWorkerRegistration.getNotifications() on main thread. r=wchen

Comment 26

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1114554 - Patch 5 - getNotifications() on worker thread. r=wchen

Comment 27

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1114554 - Patch 6 - ServiceWorkerRegistration.getNotifications() tests. r=wchen

Comment 28

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1114554 - Patch 7 - Call BindToOwner on all threads. r=wchen

Comment 29

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1114554 - Patch 2 - ServiceWorkerRegistration.showNotification(). r?wchen

Comment 30

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 31

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 32

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 33

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 34

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 35

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Bug 1114554 - Patch 6 - ServiceWorkerRegistration.getNotifications() tests.

Comment 36

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 37

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 38

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 39

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 40

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 41

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 42

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 43

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Something is wrong with RB, but please review patches 2, 5, and 7.

Comment 44

[William Chen [:wchen]]

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

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

Comment 45

[William Chen [:wchen]]

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.

Comment 46

[William Chen [:wchen]]

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.

Comment 47

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 3.1 - ServiceWorker principal fixes.

Comment 48

[Andrea Marchesini [:baku]]

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.

Comment 49

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 50

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 51

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 52

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 53

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 54

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 55

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 56

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 57

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 58

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: notificationevent patch

Ehsan, could I just get dom peer sign off on this. Thanks

Comment 59

[Andrea Marchesini [:baku]]

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;
}

Comment 60

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 61

[Andrea Marchesini [:baku]]

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

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

Ship It!

Comment 62

[Andrea Marchesini [:baku]]

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!

Comment 63

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 64

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 65

[Robert Bindar]

Attachment: 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.

Comment 66

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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."""

Comment 67

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 68

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

[Ryan VanderMeulen [:RyanVM]]

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

Comment 70

[Chris Mills [:cmills]]

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

[Jean-Yves Perrier [:teoli]]

Added https://developer.mozilla.org/en-US/Firefox/Releases/42#Service_Workers

Comment 72

[Florian Bender]

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

Comment 73

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(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.)

Comment 74

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: disable debugging

Comment 75

[Daniel Holbert [:dholbert] (PTO until 2/24)]

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.)

Comment 76

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 77

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: disable debugging

Only this new patch needs to land

Comment 78

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9738f1b3f11a

Comment 79

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9738f1b3f11a

Comment 80

[Sylvestre Ledru [:Sylvestre]]

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.