- From: <bugzilla@jessica.w3.org>
- Date: Sat, 04 Oct 2014 14:18:43 +0000
- To: public-script-coord@w3.org
https://www.w3.org/Bugs/Public/show_bug.cgi?id=26183
--- Comment #37 from Cameron McCormack <cam@mcc.id.au> ---
Thanks very much for the detailed review, Boris.
(In reply to Boris Zbarsky from comment #35)
> 1) In http://heycam.github.io/webidl/#es-iterators, this bit:
>
> If the interface does not have an iterator
>
> should probably be:
>
> If the interface does not have an iterable declaration
>
> ?
Yes. I also fixed some other remaining references to #dfn-iterator.
https://github.com/heycam/webidl/commit/0251e92e1f78f954ee6d520cab248109ee44c1fd
> 2) For the indexed case, why can we not just say that the value of
> @@iterator is literally the initial value of Array.prototype[@@iterator]
> (which is %ArrayProto_values% as it happens)? I see no particular need to
> enforce belonging to the right interface, or do security check bits here;
> none of that stuff is needed for sanity in this case imo.
I guess you're right; array iterators are intentionally generic, so we
shouldn't need to do a check on |this|. The security check bits can of course
then go. Convenient it has a %name% already!
https://github.com/heycam/webidl/commit/006100af0c3206762d472762b6ecc4b757861b73
> 3) For the maplike/setlike cases, I guess we can't just have @@iterator
> point to the same object as Map.prototype[@@iterator] and
> Set.prototype.[@@iterator] because those do the branding checks Domenic
> points out? That would have avoided having the bug where "value" should
> probably be "key+value" for the Map to match how ES Map behaves. :(
Yeah, that's why. :( I completely missed that Map.prototype.@@iterator ===
Map.prototype.entries and not values. But I think we ought to make
<maplikeinterface>.prototype.@@iterator ===
<maplikeinterface>.prototype.entries, and similarly for setlike interfaces and
"values".
https://github.com/heycam/webidl/commit/030a5ecbf18a39a5f724d300734b71caba792b28
https://github.com/heycam/webidl/commit/56c2185c86a78b0ecb8696a8a6467f301bcb03f9
> 4) In the forEach steps for iterable declarations, why not make sure
> callbackFn is callable before getting "values"? In fact, why not define the
> property location, as well as current steps 1-4 and 7-8 of its behavior by
> just saying that it should act as if the interface had:
>
> void forEach(Function callback, optional any thisArg = undefined);
>
> at the end of its IDL? That would also allow you to defer to the invoke a
> callback steps for parts of step 11, and remove the necessity for step 12, I
> think (because now you'd only be specifying prose for an IDL function, not
> the whole thing from start to finish).
I guess we could do that. I'm usually a bit wary of defining things "as if"
some other things, since I've ready plenty of spec text that did something like
which worked well for the usual case but was broken from some edge cases. I
suppose I should be able to get it right though. :-)
https://github.com/heycam/webidl/commit/ce4bf1eed4a4e978cee642f91be0d6622425113e
Let me know if you think that looks OK and I'll try with it for
setlike/maplike.
> 5) For the forEach for setlike/maplike bits, seems you could do something
> similar.
>
> 6) For the setlike/maplike forEach, is a new callbackWrapper created on
> each forEach call, or is one created then reused? I believe this is
> page-detectable via examining the stack...
Ergh. I wonder what the best way to write "create a function one that captures
an environment with these variables" is. Maybe it's just easier not to use
{Map,Set}.prototype.forEach...
Added a note for the moment.
> 7) For the setlike/maplike forEach, we should be consistent about whether
> the third arg of callbackWrapper is "o" or "object".
It should be object. I removed o since we don't use it.
https://github.com/heycam/webidl/commit/8ddacdf9189e2a8edc2158b079007a8111b17821
> 8) Is the callbackWrapper anything more than callbackFn.bind(thisArg)
> (assuming a new one is created every time and that we're talking canonical
> bind here)? If so, is it worth making that clearer?
It's also fixing up the third argument passed to the callback to be the maplike
object (and not the [[BackingMap]] inside it).
> 9) By the time I got to "entries" I was getting tired of the copy/pasted
> "The location of the property is determined as follows" bit. Could we
> refactor that into a definition or something.
Yes, that copying and pasting sucks. And I think I'm missing that wording on
some members. I filed bug 26970 for this.
> 10) "declared use" should be "declared using".
https://github.com/heycam/webidl/commit/58c1f96f517b925c6774230644db00d29cb81ac6
> 11) The section on "values" has the wrong property name, and no definition
> of the behavior the function should have.
It's pointing to @@iterator so doesn't need its own behaviour definition.
Fixed keys -> values though.
> 12) The link to "iterator" in the first paragraph of the "Iterator prototype
> object" section is broken.
Fixed that to be "iterable declaration".
> 13) I don't follow step 4 of "forwards to the internal map object". Where
> is "this Function that implements the size property" coming from there?
> Copy/paste error?
Gah. Fixed by removing the name of the function we're defining (here and in
various other functions).
> 14) In "forwards to the internal map object" after step 7 need to check
> whether "function" is callable, right?
Yes.
https://github.com/heycam/webidl/commit/bb36088f097b2f2a239fb6bd77643512945b9dca
> 15) Looks like the idea is that the implementation can interpose some sort
> of logic of its own for clear/set/delete but not for
> entries/get/has/keys/values, right? This does mean that the [[BackingMap]]
> can't be lazily computed/updated, correct?
That script could mess with the forwarded-to property and therefore you can't
just pretend you have a backing map/set is is something I realised in the
shower this morning, and makes me wonder if it's a good idea or not to have the
backing map/set at all. For example for the Font Loading API, it would be a
pain to have to use an actual JS Map object as the storage for the FontFace
objects. Interested to know what you and other think about this.
> 16) Is [MapClass] still a thing? "Maplike declarations" paragraph 2 makes
> it sound like it is, but I assume that should say "maplike declaration"?
Yeah that should have been changed to "maplike declarations".
https://github.com/heycam/webidl/commit/9a52bc13afa96836cc71b552612a281214d49150
> 17) Similarly "Setlike declarations" paragraph 2 should not talk about
> "[SetClass]", I assume.
Yep. Especially since [SetClass] never even got added to the spec.
> 18) "forwards to the internal set object" step 4 looks like it also has a
> copy/paste issue.
Fixed above.
> 19) "forwards to the internal set object" after step 7 need to check that
> "function" is callable.
Fixed above.
> 20) Iterator prototypes should have a method named @@iterator on them that
> returns the "this" value. See what %ArrayIteratorPrototype%[@@iterator]
> does in ES6.
Ah so it should.
https://github.com/heycam/webidl/commit/9c9b0e71e13f834737ebb92d14a4937242c1528d
> 21) We need to define what the .name is for these @@iterator functions.
> Actually, for all Functions in Web IDL, now that it looks like .name is an
> ES6 thing. But we can reuse the "Unless otherwise specified, this value is
> the name that is given to the function in this specification..." language
> ES6 has for most cases; just not this case.
Yeah... oh, looks like we have bug 22392 for that.
> 22) I think Domenic is right that actual type-enforcement is totally missing
> here for the mutator methods when those are not otherwise specified. Again,
> I think it would be good to define behavior here in terms of things acting
> as if they were effectively added to the IDL, which would get you the
> "this"-checks for free, and argument coercions to the right types for
> free... Would require a bit of finagling to, e.g., wrap up the "function"
> being called through to into an IDL Function. Or conversions back from IDL
> to ES values before doing the [[Call]]. Or some such.
Done earlier, though not by doing "as if" stuff. I suppose we could treat the
backing maps/sets as objects that implement callback interfaces, and call them
that way?
> 23) The backing map and set are just chaining up to their proto, right? So
> if someone changes Set.prototype.entries that will change the behavior of
> .entries() on all setlikes? That's pretty nice!
Yes that's true. Sounds nice, but the point about not being able to implement
APIs as if you had a backing map/set you bring up in (15) is a worry. :(
--
You are receiving this mail because:
You are on the CC list for the bug.
Received on Saturday, 4 October 2014 14:18:47 UTC