900279 - Fix up Gaia notifications implementation in bug 782211 for notification API spec

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Jason Smith [:jsmith]]

The rebased implementation for gaia notifications in bug 782211 doesn't work on gaia master. We need to fix up the implementation to be able to land it to have notification API spec integration in Gaia.

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: patch v1

this is the patch that originally landed in bug 782211

Comment 2

[Julien Wajsberg [:julienw]]

Attachment: patch v2

This patch works for me. Actually the Chrome Event for showing the notification has not changed.

I also added 2 tests for this change.

William, would you please review this change ?

There is a pull request at https://github.com/mozilla-b2g/gaia/pull/11298 (to run Travis) if you prefer.

Comment 3

[Julien Wajsberg [:julienw]]

cc mike so that he knows about this

Comment 4

[Julien Wajsberg [:julienw]]

Comment on attachment 784376 [details] [diff] [review]
patch v2

Etienne, if you feel like reviewing this (again) when you come back ;)

Comment 5

[Etienne Segonzac (:etienne)]

Comment on attachment 784376 [details] [diff] [review]
patch v2

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

r=me with the weird code in |clearLockScreen| removed.

If it's not a mistake then we probably need to talk :)

::: apps/system/js/notifications.js
@@ +403,5 @@
> +    event.initCustomEvent('mozContentEvent', true, true, {
> +      type: 'desktop-notification-close',
> +      id: notificationNode.dataset.notificationId
> +    });
> +    window.dispatchEvent(event);

Not sure why we have this here?
Is this a copy and paste mistake? What's notificationNode?

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 784376 [details] [diff] [review]
patch v2

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

::: apps/system/js/notifications.js
@@ +403,5 @@
> +    event.initCustomEvent('mozContentEvent', true, true, {
> +      type: 'desktop-notification-close',
> +      id: notificationNode.dataset.notificationId
> +    });
> +    window.dispatchEvent(event);

mmm right, it probably was in the first patch and I haven't changed anything here. Maybe notificationNode was existing in the 6-month-old code.

I don't exactly know what "clearLockScreen" is supposed to do. Is it only clearing the notifications displayed in the lockscreen, but they still are in the notification screen ?

Comment 7

[William Chen [:wchen]]

Comment on attachment 784376 [details] [diff] [review]
patch v2

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

Sorry for the delay. I haven't looked at this Gaia code in a while.

::: apps/system/js/notifications.js
@@ +403,5 @@
> +    event.initCustomEvent('mozContentEvent', true, true, {
> +      type: 'desktop-notification-close',
> +      id: notificationNode.dataset.notificationId
> +    });
> +    window.dispatchEvent(event);

This looks like a mistake in the original patch. It shouldn't be sending the close event unless the notification is being removed from the notification screen.

Comment 8

[Etienne Segonzac (:etienne)]

(In reply to Julien Wajsberg [:julienw] from comment #6)
> I don't exactly know what "clearLockScreen" is supposed to do. Is it only
> clearing the notifications displayed in the lockscreen, but they still are
> in the notification screen ?

Yep.

Comment 9

[Julien Wajsberg [:julienw]]

master: b2438e3ce5ea183424ede61ad005dc66e7082989

after I pushed bug 901856, I noticed that the onclick/onclose notifications don't work, at least on B2G. More bugs for Gregor and Mike ;)

Comment 10

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #9)
> master: b2438e3ce5ea183424ede61ad005dc66e7082989
> 
> after I pushed bug 901856, I noticed that the onclick/onclose notifications
> don't work, at least on B2G. More bugs for Gregor and Mike ;)

Can you file followup bugs for that.

Comment 11

[Jason Smith [:jsmith]]

Verified per completion of initial test pass.