whatwg / notifications

Notifications API Standard
https://notifications.spec.whatwg.org/
Other
137 stars 49 forks source link

Custom data object [SameObject] extended attribute #18

Open robertbindar opened 10 years ago

robertbindar commented 10 years ago

According to the WEBIDL spec [1] we should have an interface type for the custom data object when used with the [SameObject] attribute [2].

[1] http://heycam.github.io/webidl/#SameObject [2] notifications.spec.whatwg.org/#api

annevk commented 10 years ago

@bzbarsky @heycam it would be nice to use that here. I guess the alternative is detailing in prose that the structured clone is only made once so that the same object is returned each time. Any thoughts?

heycam commented 10 years ago

I wonder if [SameObject] should just become [Constant], as Gecko's bindings supports. Then we could use it for any type (including the any type!).

annevk commented 10 years ago

Works I think. You might want to review usage in http://dom.spec.whatwg.org/ to be sure.

heycam commented 10 years ago

In section 5.4, I think:

The data attribute must return a structured clone of notification's data.

should be just:

The data attribute must return the notification's data.

since the constructor already handles the initial assignment of the structured clone to data.

annevk commented 10 years ago

No, the "notification" is shared across workers and documents. Notification objects are per-document, per-worker, etc. so need their own copy.

heycam commented 10 years ago

Oh I see, the "notification" is the abstract thing that one or more Notification object get created to represent.

Still I don't think there is any need to add any additional wording about making the structured clone once, even if we don't use [SameObject] or [Constant] here, since the step in the constructor only runs once.

annevk commented 10 years ago

But otherwise each time you get the data property the getter will run which creates a structured clone of the abstract thing.

heycam commented 10 years ago

OK. It's not exactly clear whether "must return a structured clone of notification's data" should mean a new structured clone every time you fetch the IDL attribute or just any structured clone (which therefore could be the same one). With the [SameObject] or [Constant] it would force you to interpret it as "only do the structured clone once" I guess.

annevk commented 10 years ago

Yeah that was the idea. Otherwise I'd have to store an object alongside the Notification object that's created the first time you get the data property and then returned on subsequent gets.

bzbarsky commented 10 years ago

[SameObject]/[Constant] doesn't do what it sounds like people think it does.

It's just a promise that the implementation will return the same thing each time, for ease of reading/understanding the API. It doesn't actually enforce this in any way.

It sounds like what you're asking for here is more like Gecko's [Cached], which says that after the first time the getter or method is called the result is cached by the Web IDL layer and returned for subsequent calls.

I suppose we could make [SameObject] or [Constant] imply the equivalent of [Cached] at least in spec terms. That would be OK by me; we'd have to clearly define it by saying that these extended attributes lead to the creation of a corresponding internal slot and the exact behavior of that external slot. Fwiw, Gecko's behavior is that if the value in the internal is not JS undefined then it's returned, otherwise the getter/method is called and the return value stored in the internal slot before returning. This does mean that if a method is declared as returning any and actually returns undefined we'll keep calling the method, but in practice no one does that.

heycam commented 10 years ago

Technically it's a spec bug if the spec author uses [SameObject] and then requires in prose that a different object is returned at some point. So it is an "enforcement" as long as the spec has no bugs!

annevk commented 10 years ago

I see. I would prefer having [Cached] then, indeed. If [SameObject] does not actually help in writing text, I'm not sure why we would have it.

annevk commented 10 years ago

https://www.w3.org/Bugs/Public/show_bug.cgi?id=26524

johnmellor commented 9 years ago

By the way, I guess this is working as expected?:

// Prerequisite: a single notification with data is currently showing.
Promises.all([
    serviceWorkerRegistration.getNotifications().then(function(ns) { return ns[0]; }),
    serviceWorkerRegistration.getNotifications().then(function(ns) { return ns[0]; })
]).then(function(notification1, notification2) {
    // notification1 !== notification2
    // notification1.data !== notification2.data
})

It's a little odd that you get different instances of the SameObject for different references to the same underlying notification, but I suppose that's normal for WebIDL...

annevk commented 9 years ago

IDL describes instances and [SameObject] only pertains to an instance. It knows nothing about underlying objects. So yes, that is working as expected.