-
Notifications
You must be signed in to change notification settings - Fork 159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Are operations on the [[BackingMap]]
and [[BackingSet]]
page-hookable? And if not, why not?
#254
Comments
Option 3 makes sense as a quick fix. /cc @yuki3 for Blink binding questions about the map/set iterators not having the right prototype chain. I personally would have preferred not to have a backing map/set at all, i.e. just keep the map/set data internal instead of indirecting it through another ES object. But that's a larger change. And if we fix this then it should be an unobservable change. |
It's not quite unobservable, because of the iterator question. Not having a backing map/set at all requires reimplementing and respecifying a bunch of iterator infrastructure... |
I see, I didn't quite realize that ES's MapIterator/SetIterator had references to their creator Maps/Sets. It seems this could be fixable by patching ES to instead have them take a reference to their creator's [[MapData]]/[[SetData]], but I only did a quick scan of the spec. |
In practice that would require that engines support a MapData/SetData existing outside of a Map/Set, right? Right now those are just spec abstractions, not actual things that the gc needs to know about as separately-allocated objects, for example. |
I assume engines would still keep supporting them as abstractions, e.g. as C++ ordered maps. Both Web IDL maplikes and JS engine Maps could keep C++ ordered maps, and you'd create a MapIterator from such C++ ordered maps. |
Right, but the point is that then the MapIterator needs to keep the thing alive. Right now it just keeps alive a Map by referencing it and the GC does the rest. |
In the end the goal would be to have the same observable semantics as the current spec + your 3, but just less contortions to wrap a Web IDL object around a backing JS map and then operate on it with the original value of various methods and such. Maybe I'm the only one who thinks the wrapping setup is weird though. Not a high priority to fix. |
OK. Note, also, that either Maps know about their iterators and update them on mutation or you have to leak memory on mutation (by implementing the spec's algorithm word for word), or you leak memory while any iterator is live (which means you also have to know about your iterators). Sharing the actual Map implementation of the "let's not leak" option may not be very easy from outside the ES implementation in browsers... |
But ok, if the intent is to end up with current spec plus option 3, then it probably doesn't matter too much. Assuming that the observable semantics in fact end up the same, of course. |
Let's consider it a derailment, and focus this thread on your original problem, and the proposed solution of option 3 :) |
Blink's implementation for setlike/maplike is not great so far, and it's simply broken in many ways. We are also aware that NodeList.prototype.forEach is not Array.prototype.forEach, etc. It's just because that we first aimed to support iterators of simple use cases without supporting details. There is no special reason. Don't mind too much on this. Option 3 sounds reasonable to me, too. |
I am working on RTCStatsReport right now. Returning a MapIterator in WebKit requires creating a JS backing map/backing set. I would prefer removing the backing set/backing map idea and come up with a solution that allows exposing directly any map-like structure. That might require defining some sort of MapLikeIterator/SetLikeIterator. |
/cc @rakuco FYI. You might be interested in fixing this issue in Blink. |
These will be helpful in solving whatwg/webidl#254.
The specification still seems to use “forwards that name to the internal set object” as described at the start of the issue. Chrome/Edge/Firefox at least all appear to behave like “option 3”. Is it accurate to say that it’s the spec text which is mistaken here and that implementations should follow the behavior agents are already implementing?
Is the “if” in this sentence just like “if [thing we know], then [we can infer],” or is the realm actually unspecified? Given “internally create a new object implementing the interface” doesn’t account for initializing [[BackingSet]] — which I guess is part of what that “Define those properties imperatively instead” note is about — I searched a bit instead for text that might “ambiently” establish how a phrase like “newly created Set object” should be interpreted, but I couldn’t find anything. |
At the time I made the comment it was not clearly specified. I don't know where things stand now. |
I might as well shave this yak while I'm here, since I want to fix #824 as well, and presenting an Infra object would do that well. So it sounds like there's agreement on approach 3 from the OP:
I presume this applies to the other methods as well, so the backing set/map can only be observed by internal spec algorithms (otherwise, I believe step 7 of https://webidl.spec.whatwg.org/#es-map-get-has would have the same author-hooking issue). It also looks like the only thing preventing us from just switching the backing store entirely to an Infra map/set is that we're using JS's MapIterator/SetIterator algos, which expect to be called on an actual ES Map or Set; they don't expose this object to the author, but they're written to depend on it when fetching the items. We manually reproduce a bunch of the other map/set/etc method algos, tho - is there something I'm missing preventing us from just copying over the iterator algos, too? I'm gonna start hacking at a PR assuming the answer is "no" and it would be fine to just copy the algos over. |
I'm not quite sure what you have in mind, so the following might not apply, but the main observable constraint is that maplike[Symbol.iterator]().__proto__ === (new Map())[Symbol.iterator]().__proto__ I'm not sure how to do that if we just copy a bunch of stuff over. |
The ES MapIterator algo just (1) checks the object has a [[MapData]], (2) creates a closure that iterates over the [[MapData]] and yields IterResults, and then (3) makes an iterator from that closure with |
Fixes #254. Fixes #824. This switches maplike and setlike interfaces from using a [[BackingMap]]/[[BackingSet]] ES Map/Set to use an Infra map/set instead. As explained in #254 and #824, this makes the interfaces vastly easier to work with in spec-ese (you can directly manipulate an Infra map/set, rather than having to carefully perform the ES algo dance) , and removes some undesired behavior (as currently defined, the algorithms look up and utilize the equivalent methods on Map.prototype and Set.prototype, which can be author-supplied; no implementation does this).
I was sure I had filed this before, but I can't find it...
Consider this testcase:
What should get alerted? Per spec as currently written we end up at https://heycam.github.io/webidl/#es-set-size we end up (in step 5) doing:
where set is the
[[BackingSet]]
of the object. And the[[BackingSet]]
is defined like so:So what should get alerted? Browsers alert 0 if there are no font-face rules. As an implementor, I would prefer to alert 0 in general. But if the
[[BackingSet]]
is created in the same Realm as the object itself, then you ought to get 42, right?We should specify how this actually works. I see three possible options:
[[BackingSet]]
is created in the global of the IDL object, leave everything as is. Then this testcase should alert 42, which doesn't match browsers.[[BackingSet]]
is created in a separate global, leave everything as is. Then this testcase should alert 0.size
getter invokes the original value of theSet.prototype.size
getter. Then this testcase should alert 0. Same with the various other forwarding getters.Options 2 and 3 are not black-box identical. The reason is that
entries
will return you an iterator, and you can tell which global theSet
's internalentries
function comes from by looking at the prototype of that iterator. In fact, you can distinguish not only options 2 and 3 from each other but also different variants of option 2 (e.g. a single separate global for all interfaces, one separate global per interface, one separate global per object, etc).My personal feeling is that we should go with option 3, with the
[[BackingSet]]
created in the IDL object's global. That's certainly how maplike/setlike are implemented in Gecko. Note that FontFaceSet is NOT implemented as a setlike in Gecko for the moment, though. It looks like we ship no actual setlikes so far, and only one maplike: RTCStatsReport. Unfortunately, I have yet to figure out how to get my hands on an RTCStatsReport to see how it behaves in other browsers...Doing black-box testing of FontFaceSet in Blink, it looks like the iterator it returns from
entries()
doesn't have%SetIteratorPrototype%
(from any global) as its prototype. So Blink is not following the spec for this part either so far. Grepping Blink's idl, this is the only setlike, and the only maplikes are the MIDIInput/OutputMaps. Those don't use an object with%MapIteratorPrototype%
as its proto either. That makes it hard to tell whether Blink is doing option 2 or option 3, of course, since at least forentries
it's not doing either one.@tobie @domenic @heycam @annevk
The text was updated successfully, but these errors were encountered: