934731 - Notification.get() needs to return existing Notifications rather than always create new ones

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, defect)

Description

[Michael Henretty [:mikehenrty][:mhenretty]]

According to the Notification.get whatwg spec:

> If the current global object already exposes the notification as Notification
> object, push that object to [the return value].

Right now we are always creating new notifications for each call to Notification.get(). We need to create a `live` cache in C++ land so we can return these objects when fetched.

Comment 1

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

Some tests also need fixes:
deleteAllNotifications() and the "Test adding a notification, and making sure get returns it" presume that the promise will be resolved synchronously, but Promises aren't required to do that. In addition, the port to workers makes get() async on the main thread too.

Two more tests that should be added:
1) Identity check to ensure this bug is really fixed.
2) Create multiple notifications and ensure that get() returns then "in creation order" per-spec.

Comment 2

[Michael Henretty [:mikehenrty][:mhenretty]]

"Test adding a notification, and making sure get returns it" should work even if the promise is resolved asynchronously. It assigns the `done` function to the onclose handler of a notification we manually close in the .then calllback.

Comment 3

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

(In reply to Michael Henretty [:mhenretty] from comment #2)
> "Test adding a notification, and making sure get returns it" should work
> even if the promise is resolved asynchronously. It assigns the `done`
> function to the onclose handler of a notification we manually close in the
> .then calllback.

currently, that notification is shadowed by the 'var notification' created inside the for loop. I think it's working because deleteAllNotifications() is calling the close later or something, though I haven't really verified this.

Comment 4

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Nikhil Marathe [:nsm] from comment #3)
> currently, that notification is shadowed by the 'var notification' created
> inside the for loop. I think it's working because deleteAllNotifications()
> is calling the close later or something, though I haven't really verified
> this.

Hrm, if I fix deleteNotifications to make it wait for it to finish, it looks like the tests pass. Is this test breaking when moving Notifications to a worker?

-  function deleteAllNotifications() {
+  function deleteAllNotifications(done) {
     var promise = Notification.get();
     promise.then(function (notifications) {
       notifications.forEach(function(notification) {
         notification.close();
       });
+      done();
     });
   }

Comment 5

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

mhenretty, any ETA for this? I'd like to avoid a ugly merge if this can be done within this week. Otherwise I'd like to continue working on the worker patch with the current implementation.

Comment 6

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #5)
> mhenretty, any ETA for this? I'd like to avoid a ugly merge if this can be
> done within this week. Otherwise I'd like to continue working on the worker
> patch with the current implementation.

Unfortunately, I won't be able to get to this during this week. I have two 1.3 blockers that need to be addressed first, and it's looking like they will be taking most of my time. Go ahead and continue your worker patch, and I'll work with you later on fixing this once notifications are on workers.

Comment 8

[Michael Henretty [:mikehenrty][:mhenretty]]

Due to recent discussions in the WHATWG (https://github.com/whatwg/notifications/issues/9), Notification.get has changed to Notification.getClones This guarantees we are always creating new JavaScript objects when fetching notifications. As such, this bug is no longer valid.

http://notifications.spec.whatwg.org/#dom-notification-getclones