1123516 - implement support for maplike/setlike in Web IDL bindings

Status: RESOLVED FIXED

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

Description

[Cameron McCormack (:heycam)]

The CSS Font Loading API has a setlike interface.  There is a bunch of discussion in bug 1121332 about implementing maplike, which is related.  The current approach for setlike-things in the Web IDL spec needs review for sanity.

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

I need maplike for WebMIDI (Bug 836897), otherwise I'm just going to be implementing Maplike in the API itself and then changing it once this gets finished. Talked to bz about implementation details last night. I'll take a crack at implementing this, though it's going to be my first WebIDL addition so may need some guidance.

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v1) - WIP: Maplike/Setlike/Iterable WebIDL Parser Changes

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Putting bz's etherpad notes from our initial discussion here just so I don't lose them:

- Add parser support to enable flagging an interface as needing maplike and storing the key/value types on it.

- Add an extra internal slot to maplike interfaces for storing the Map (similar to the way they reserve slots for [Cached] values).

- Add an accessor on the binding (static method) for getting the Map out of the JS object so the C++ WrapObject method and other C++ bits can put stuff in the Map as needed.

- Have the bindings codegen autogenerate the various maplike method bits.  Would have to look to see how easy it is to hack this into the existing infrastructure.  In particular, we want to keep the existing argument conversions but change how we do the actual call.  It would be possible to make this a bit less turnkey but simpler to fit into the existing codegen in various way, but if we're trying to make this simple...

- I suspect that the requirement about @@iterator and .entries returning the same function object is pretty hard to do as a first cut, so maybe not worry about that for now.

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

C++ ownership/conversion of a JS Map structure, similar to what I need to generalize for this bug, happened for MediaKeyStatusMap in bug 1121332. Should be able to use this as a guide for implementing the Codegen portion of this bug.

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v2) - WIP: Maplike/Setlike/Iterable WebIDL Parser Changes

Been cleaning up parser bits while figuring out CodeGen side. Parser now deals with creating more members when there's a declaration. Also added lots more tests. Still having issues with shift/reduce conflict for the ReadOnly token though.

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Hey Peter, can you look at the WebIDL changes in the parser WIP patch on this bug, and possibly give me some guidance on fixing the shift/reduce error? Adding ReadOnly to the maplike/setlike rules cause there to be a shift/reduce conflict with the attribute rule, and I'm kinda stumped on how to resolve it.

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: Potential fix for shift/reduce issue

This seems to fix it for me, but I'm not entirely sure it's right. Here's the relevant error log:

state 174

    (19) InterfaceMembers -> ExtendedAttributeList . InterfaceMember InterfaceMembers
    (21) InterfaceMember -> . Const
    (22) InterfaceMember -> . AttributeOrOperationOrDeclaration
    (43) Const -> . CONST ConstType IDENTIFIER EQUALS ConstValue SEMICOLON
    (51) AttributeOrOperationOrDeclaration -> . Attribute
    (52) AttributeOrOperationOrDeclaration -> . Declaration
    (53) AttributeOrOperationOrDeclaration -> . Operation
    (61) Attribute -> . Qualifier AttributeRest
    (62) Attribute -> . Inherit AttributeRest
    (54) Declaration -> . IterableDeclaration
    (55) Declaration -> . MapSetDeclaration
    (68) Operation -> . Qualifiers OperationRest
    (69) Operation -> . STRINGIFIER SEMICOLON
    (70) Operation -> . JSONIFIER SEMICOLON
    (71) Qualifier -> . STATIC
    (72) Qualifier -> . STRINGIFIER
    (66) Inherit -> . INHERIT
    (67) Inherit -> .
    (56) IterableDeclaration -> . ITERABLE LT Type COMMA Type GT SEMICOLON
    (57) IterableDeclaration -> . ITERABLE LT Type GT SEMICOLON
    (58) IterableDeclaration -> . LEGACYITERABLE LT Type GT SEMICOLON
    (59) MapSetDeclaration -> . ReadOnly MAPLIKE LT Type COMMA Type GT SEMICOLON
    (60) MapSetDeclaration -> . ReadOnly SETLIKE LT Type GT SEMICOLON
    (73) Qualifiers -> . Qualifier
    (74) Qualifiers -> . Specials
    (64) ReadOnly -> . READONLY
    (65) ReadOnly -> .
    (75) Specials -> . Special Specials
    (76) Specials -> .
    (77) Special -> . GETTER
    (78) Special -> . SETTER
    (79) Special -> . CREATOR
    (80) Special -> . DELETER
    (81) Special -> . LEGACYCALLER

  ! shift/reduce conflict for READONLY resolved as shift
    CONST           shift and go to state 190
    STRINGIFIER     shift and go to state 219
    JSONIFIER       shift and go to state 220
    STATIC          shift and go to state 204
    INHERIT         shift and go to state 202
    ATTRIBUTE       reduce using rule 67 (Inherit -> .)
    ITERABLE        shift and go to state 216
    LEGACYITERABLE  shift and go to state 212
    READONLY        shift and go to state 210
    MAPLIKE         reduce using rule 65 (ReadOnly -> .)
    SETLIKE         reduce using rule 65 (ReadOnly -> .)
    VOID            reduce using rule 76 (Specials -> .)
    ANY             reduce using rule 76 (Specials -> .)
    LPAREN          reduce using rule 76 (Specials -> .)
    ARRAYBUFFER     reduce using rule 76 (Specials -> .)
    OBJECT          reduce using rule 76 (Specials -> .)
    SEQUENCE        reduce using rule 76 (Specials -> .)
    PROMISE         reduce using rule 76 (Specials -> .)
    MOZMAP          reduce using rule 76 (Specials -> .)
    DATE            reduce using rule 76 (Specials -> .)
    BOOLEAN         reduce using rule 76 (Specials -> .)
    BYTE            reduce using rule 76 (Specials -> .)
    OCTET           reduce using rule 76 (Specials -> .)
    FLOAT           reduce using rule 76 (Specials -> .)
    UNRESTRICTED    reduce using rule 76 (Specials -> .)
    DOUBLE          reduce using rule 76 (Specials -> .)
    DOMSTRING       reduce using rule 76 (Specials -> .)
    BYTESTRING      reduce using rule 76 (Specials -> .)
    USVSTRING       reduce using rule 76 (Specials -> .)
    UNSIGNED        reduce using rule 76 (Specials -> .)
    SCOPE           reduce using rule 76 (Specials -> .)
    IDENTIFIER      reduce using rule 76 (Specials -> .)
    SHORT           reduce using rule 76 (Specials -> .)
    LONG            reduce using rule 76 (Specials -> .)
    GETTER          shift and go to state 214
    SETTER          shift and go to state 206
    CREATOR         shift and go to state 209
    DELETER         shift and go to state 223
    LEGACYCALLER    shift and go to state 218

  ! READONLY        [ reduce using rule 67 (Inherit -> .) ]

    MapSetDeclaration              shift and go to state 207
    InterfaceMember                shift and go to state 203
    Qualifier                      shift and go to state 208
    Attribute                      shift and go to state 221
    Inherit                        shift and go to state 215
    Specials                       shift and go to state 213
    ReadOnly                       shift and go to state 222
    AttributeOrOperationOrDeclaration shift and go to state 224
    Declaration                    shift and go to state 211
    Const                          shift and go to state 205
    Operation                      shift and go to state 225
    Qualifiers                     shift and go to state 226
    Special                        shift and go to state 227
    IterableDeclaration            shift and go to state 217

With the patch that changes to:

state 174

    (19) InterfaceMembers -> ExtendedAttributeList . InterfaceMember InterfaceMembers
    (21) InterfaceMember -> . Const
    (22) InterfaceMember -> . AttributeOrOperationOrDeclaration
    (43) Const -> . CONST ConstType IDENTIFIER EQUALS ConstValue SEMICOLON
    (51) AttributeOrOperationOrDeclaration -> . Attribute
    (52) AttributeOrOperationOrDeclaration -> . Declaration
    (53) AttributeOrOperationOrDeclaration -> . Operation
    (61) Attribute -> . Qualifier AttributeRest
    (62) Attribute -> . INHERIT AttributeRest
    (63) Attribute -> . AttributeRest
    (54) Declaration -> . IterableDeclaration
    (55) Declaration -> . MapSetDeclaration
    (67) Operation -> . Qualifiers OperationRest
    (68) Operation -> . STRINGIFIER SEMICOLON
    (69) Operation -> . JSONIFIER SEMICOLON
    (70) Qualifier -> . STATIC
    (71) Qualifier -> . STRINGIFIER
    (64) AttributeRest -> . ReadOnly ATTRIBUTE Type AttributeName SEMICOLON
    (56) IterableDeclaration -> . ITERABLE LT Type COMMA Type GT SEMICOLON
    (57) IterableDeclaration -> . ITERABLE LT Type GT SEMICOLON
    (58) IterableDeclaration -> . LEGACYITERABLE LT Type GT SEMICOLON
    (59) MapSetDeclaration -> . ReadOnly MAPLIKE LT Type COMMA Type GT SEMICOLON
    (60) MapSetDeclaration -> . ReadOnly SETLIKE LT Type GT SEMICOLON
    (72) Qualifiers -> . Qualifier
    (73) Qualifiers -> . Specials
    (65) ReadOnly -> . READONLY
    (66) ReadOnly -> .
    (74) Specials -> . Special Specials
    (75) Specials -> .
    (76) Special -> . GETTER
    (77) Special -> . SETTER
    (78) Special -> . CREATOR
    (79) Special -> . DELETER
    (80) Special -> . LEGACYCALLER

    CONST           shift and go to state 190
    INHERIT         shift and go to state 215
    STRINGIFIER     shift and go to state 219
    JSONIFIER       shift and go to state 220
    STATIC          shift and go to state 204
    ITERABLE        shift and go to state 216
    LEGACYITERABLE  shift and go to state 212
    READONLY        shift and go to state 210
    ATTRIBUTE       reduce using rule 66 (ReadOnly -> .)
    MAPLIKE         reduce using rule 66 (ReadOnly -> .)
    SETLIKE         reduce using rule 66 (ReadOnly -> .)
    VOID            reduce using rule 75 (Specials -> .)
    ANY             reduce using rule 75 (Specials -> .)
    LPAREN          reduce using rule 75 (Specials -> .)
    ARRAYBUFFER     reduce using rule 75 (Specials -> .)
    OBJECT          reduce using rule 75 (Specials -> .)
    SEQUENCE        reduce using rule 75 (Specials -> .)
    PROMISE         reduce using rule 75 (Specials -> .)
    MOZMAP          reduce using rule 75 (Specials -> .)
    DATE            reduce using rule 75 (Specials -> .)
    BOOLEAN         reduce using rule 75 (Specials -> .)
    BYTE            reduce using rule 75 (Specials -> .)
    OCTET           reduce using rule 75 (Specials -> .)
    FLOAT           reduce using rule 75 (Specials -> .)
    UNRESTRICTED    reduce using rule 75 (Specials -> .)
    DOUBLE          reduce using rule 75 (Specials -> .)
    DOMSTRING       reduce using rule 75 (Specials -> .)
    BYTESTRING      reduce using rule 75 (Specials -> .)
    USVSTRING       reduce using rule 75 (Specials -> .)
    UNSIGNED        reduce using rule 75 (Specials -> .)
    SCOPE           reduce using rule 75 (Specials -> .)
    IDENTIFIER      reduce using rule 75 (Specials -> .)
    SHORT           reduce using rule 75 (Specials -> .)
    LONG            reduce using rule 75 (Specials -> .)
    GETTER          shift and go to state 214
    SETTER          shift and go to state 206
    CREATOR         shift and go to state 209
    DELETER         shift and go to state 223
    LEGACYCALLER    shift and go to state 218

    MapSetDeclaration              shift and go to state 207
    InterfaceMember                shift and go to state 202
    Qualifier                      shift and go to state 208
    AttributeRest                  shift and go to state 203
    Attribute                      shift and go to state 221
    Specials                       shift and go to state 213
    ReadOnly                       shift and go to state 222
    AttributeOrOperationOrDeclaration shift and go to state 224
    Declaration                    shift and go to state 211
    Const                          shift and go to state 205
    Operation                      shift and go to state 225
    Qualifiers                     shift and go to state 226
    Special                        shift and go to state 227
    IterableDeclaration            shift and go to state 217

Maybe Kyle or Boris know enough about yacc to give their opinion?

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Cool, patch works for me, thanks!

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Talked to :khuey, he recommended redirecting check to :heycam.

Comment 10

[Boris Zbarsky [:bzbarsky]]

So the issue here, as far as I can tell is that when we see the READONLY as our first token it's not clear whether the stack should become "Inherit Readonly" or just "Readonly", right?  In particular, that requires one more token of lookahead: if we're looking at READONLY MAPLIKE then it needs to just become "Readonly" but if we're looking at READONLY ATTRIBUTE it needs to become "Inherit Readonly".

If that analysis is correct, this seems like a bug in the spec grammar (which is only supposed to be one-token lookahead), and Peter's proposed solution of having explicitly separate INHERIT READONLY ATTRIBUTE and READONLY ATTRIBUTE rulesto avoid the problem makes sense to me...

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Clearing fb? to :heycam 'cause we took care of it on IRC.

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Hey :jorendorff! I'm implementing maplike/setlike in WebIDL now. Looks like I'll need to do work similar to bug 1121332 Patch 4 for the ES6 Set object in MapObject. Am I ok pretty much repeating for Set what was done for Map in that case?

Comment 13

[Jason Orendorff [:jorendorff]]

Yes, that'll be fine.

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Add public jsapi ES6 Map Delete function

Comment 15

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v1) - Add public jsapi ES6 Set convenience functions

Comment 16

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v3) - Implement maplike/setlike in WebIDL parser

Comment 17

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 4 (v1) - WIP: Add maplike/setlike binding function generator in WebIDL Codegen

Comment 18

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 5 (v1) - WIP: Add maplike/setlike convenience function generation to WebIDL Codegen

Comment 19

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 6 (v1) - WIP: WebIDL Parser Mochitests for maplike/setlike

Comment 20

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Updating WIP patches. Setlike/maplike declaration works, function generation in codegen seems to work. Right now, major issues are:

- Convenience functions (for accessing/mutating the map/set from C++) are a MESS due to trying to figure out argument generation (most likely copying what CGNativeMember does).
- We leak the container. Not sure why.
- We don't return the right thing when the container doesn't exist yet. Was trying to be fancy and lazily create the container. May drop that.
- Need to fill in/clean up comments.
- Need more tests. Way more tests.

Comment 21

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Oh yeah, and:

- We're missing the size property. Shouldn't be too bad to generate, just forgot.
- We're missing the ForEach function. This will require generation of a new callback type AND a new function.

Comment 22

[Jason Orendorff [:jorendorff]]

Comment on attachment 8574993 [details] [diff] [review]
Patch 1 (v1) - Add public jsapi ES6 Map Delete function

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

::: js/src/builtin/MapObject.cpp
@@ +1458,5 @@
>  
>  bool
> +MapObject::delete_(JSContext *cx, HandleObject obj, HandleValue key, bool *rval)
> +{
> +    MOZ_ASSERT(MapObject::is(obj));

extract() already asserts this (twice), so please remove this assertion.

Comment 23

[Jason Orendorff [:jorendorff]]

Comment on attachment 8574995 [details] [diff] [review]
Patch 2 (v1) - Add public jsapi ES6 Set convenience functions

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

Nice patch. r=me.

::: js/src/builtin/MapObject.cpp
@@ +1983,5 @@
> +uint32_t
> +SetObject::size(JSContext *cx, HandleObject obj)
> +{
> +    MOZ_ASSERT(SetObject::is(obj));
> +    ValueSet &set = extract(obj);

Please reemove the assertion, since extract() asserts this. Same thing in all the other methods.

@@ +2074,5 @@
> +    AutoHashableValueRooter k(cx);
> +
> +    if (!k.setValue(cx, key)) {
> +        return false;
> +    }

Style nit: Please drop the extra {} brackets here.

::: js/src/jsapi.h
@@ +4565,5 @@
> +extern JS_PUBLIC_API(bool)
> +SetKeys(JSContext *cx, HandleObject obj, MutableHandleValue rval);
> +
> +extern JS_PUBLIC_API(bool)
> +SetValues(JSContext *cx, HandleObject obj, MutableHandleValue rval);

SetKeys and SetValues are the same function, so please delete SetKeys.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8574996 [details] [diff] [review]
Patch 3 (v3) - Implement maplike/setlike in WebIDL parser

Some drive-by comments, from an incomplete reading (e.g. did not read expand() details):

.declaration is a pretty vague name.  I don't have a better proposal offhand yet, but we should come up with something useful.

Same for isDeclaration.  Maybe even isSetlikeOrMaplikeDeclaration()?  Seems excessive.  :(

Just checking for self.parent.declaration doesn't seem like it's enough.  Say A inherits from B, B inherits from C, A and C both have maplike declarations.  Add a test?  And push this check down to where we walk our entire ancestor chain?

I like how you just piggybacked on the existing attr slot setup!

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8574997 [details] [diff] [review]
Patch 4 (v1) - WIP: Add maplike/setlike binding function generator in WebIDL Codegen

I'm not sure why has() allows an uninitialized slot while all the other methods throw uncatchable exceptions in that case.  Seems fishy.

Why is the pack_slot stuff needed?

Who calls create_slot?

You're using fill() in various places where dedent() would do.

I didn't look at the details of generate_js_call, but the exception bits seem weird to me: if JS::${func} returned false, it's already thrown an exception, right?

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8575000 [details] [diff] [review]
Patch 5 (v1) - WIP: Add maplike/setlike convenience function generation to WebIDL Codegen

Really need to check ToJSValue return values.

You don't need to set reserved slot with the Map/Set if it was already in there, right?

Still not sure what and where is responsible for creating the Map/Set....

Comment 28

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

IRC conversation about forEach implementation details:

[14:32] <qDot> bz: I need to generate a Callback function out of basically
        thin air for forEach (which is the last thing holding up starting
        reviews on maplike/setlike, outside of addressing what you said in
        drive-bys). This is proving... difficult. Callbacks can only exist as
        a top level production, right?
[14:36] <bz> qdot: for forEach, do you need a function that closes over
        something?
[14:36] <bz> qdot: I guess you do, right?
[14:37] <bz> qdot: how many things does it need to close over?
[14:38] <qDot> bz: Yeah, we need a key, a value (for map), and the structure
        its operating on. Plus whatever it's supposed to use for "this" if we
        want to keep this in parity with ES6
[14:38] * bz looks at the spec
[14:41] <bz> ok
[14:41] <bz> so looking at the spec....
[14:41] <benc1> are there plans to add shapes to the css clip-path attribute?
[14:41] <bz> The callbackWrapper is supposed to take the first two arguments
        passed to it
[14:42] <bz> And "object"
[14:42] <bz> And the original callbackFn
[14:43] <bz> And call callbackFn with whatever "this" value the
        callbackWrapper has, and v, k, object as arguments
[14:43] <catalinb> mccr8: we hit that warning quite a lot in the service
        worker tests.
[14:43] <bz> So it needs to close over callbackFn and object, seems to me
[14:44] <mccr8> catalinb: ah, thanks. I guess it probably isn't my fault then.
[14:44] <bz> qdot: agreed?
[14:44] <qDot> bz: Yup, that sounds about like what I was thinking
[14:44] <bz> ok
[14:44] <bz> So we're in luck
[14:44] <bz> What spidermonkey will let us do is create a JSFunction that has
        the following properties:
[14:44] <bz> 1) Has two reserved slots we can use.
[14:44] <bz> 2) When called from JS, will call a C++ function of our choosing
[14:45] <bz> See js::NewFunctionWithReserved
[14:45] <bz> (and NewFunctionByIdWithReserved, but we can make this function
        anonymous, I'd think)
[14:46] <bz> So then we can just write a C++ function that implements the
        algorithm from the webidl spec
[14:46] <bz> That being the substeps of step 9 of
        http://heycam.github.io/webidl/#es-forEach
[14:46] <bz> Which gets callbackFn and object from its reserved slots
[14:47] <bz> And then in our impl of forEach, we just create a JSFunction for
        that C++ function
[14:47] <bz> Stick the right things into its reserved slots
[14:47] <bz> and pass it to the JS engine's forEeach impl, with the right
        second argument
[14:47] <bz> Make sense?
[14:49] <qDot> Oh, wow, ok. Yeah, that makes sense. Don't I still need to
        generate a callback type for the argument though?
[14:49] <bz> which argument?
[14:50] <qDot> For the argument to the forEach function on the interface that
        is declaring itself maplike/setlike?
[14:50] <bz> So forEach on the interface takes two arguments
[14:51] <bz> the first is a function
[14:51] <bz> the second is a thisValue
[14:51] <qDot> OH function not callback. Oops.
[14:51] <bz> agreed?
[14:51] <qDot> Yeah ok I had been turning that into callback in my head for
        some reason. 
[14:51] <bz> I mean, we have to check that the first arg is callable
[14:51] <bz> But past that, we just stick it in a reserved slot on our thing
[14:51] <bz> and then JS::Call it
[14:51] <qDot> Yeah but that's handled automatically already, I'm assuming.
[14:51] <qDot> Yeah.
[14:51] <bz> as needed.
[14:52] <bz> well, so
[14:52] <qDot> Ok, got it. I was making this way more complicated than
        necessary.
[14:52] <bz> You don't actually want to convert it to a Function
[14:52] <bz> In that we don't plan to use that Function in practice; we want
        the actual callable value
[14:52] <qDot> Yeah, this is sort of a problem with the way I'm doing things
        right now. I'm co-opt'ing the argument checking machinery, which is
        nice enough to also do a conversion for me that I end up never using.
[14:53] <bz> Well, you do want it for some of the cases
[14:53] <bz> Just not this one
[14:53] <qDot> There's a lot of times where I'm ignoring it now too :)
[14:53] <bz> as in, you want the conversion for some of the other cases
[14:53] <qDot> Oh yeah totally.
[14:53] * bz can't think of any other place where you want to ignore it ....
[14:54] <bz> because most of them are real non-round-trippable coercions
[14:54] <qDot> For places where we're passing values to jsapi that need to be
        JS::Value or whatever?
[14:54] <bz> But this one is not; it's just a check and then some work
[14:54] <bz> yes, but in general those need to be converted from the C++
        representation
[14:54] <bz> Here's a simple example
[14:54] * qDot can throw a generated binding on the bug if you want to see
          what's happening right now.
[14:55] <bz> maplike<long, long>;
[14:55] <bz> Say someone does .get({ valueOf: function() { return 5; } })
[14:55] <bz> That needs to end up passing 5, not an object, to the underlying
        Map.prototype.get
[14:56] <qDot> Ah hah. Well that may break right now then.
[14:56] <bz> ok

Comment 29

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 7 (v1) - Add forEach convenience function for maplike/setlike

Need to be able to call forEach from C++.

Comment 30

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v4) - Implement maplike/setlike in WebIDL parser

All spec required functions for maplike/setlike implemented, and changed name from IDLDeclaration to IDLMaplikeOrSetlike. Not pretty, but eh.

Comment 31

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 4 (v2) - Add maplike/setlike binding function generator in WebIDL Codegen

All functions generated, as well as @@iterator symbol.

Comment 32

[Jason Orendorff [:jorendorff]]

Comment on attachment 8579810 [details] [diff] [review]
Patch 7 (v1) - Add forEach convenience function for maplike/setlike

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

I think this patch has a problem: it will expose the private Map/Set object to anyone who calls the forEach method on the IDL maplike/setlike.

    maplike.forEach(function (key, value, map) {  // map receives the private Map object
        map.set(key, "potato");  // oh no, putting a value of the wrong type into the map
    });

It seems like you can fix this by writing new self-hosted functions in js/src/builtin/Map.js and Set.js that do what you want. They would be used solely to implement these APIs. I'll review your new patch once that's done, so I'm just clearing the r? flag for now.

::: js/src/builtin/MapObject.cpp
@@ +2211,5 @@
> +bool
> +SetObject::forEach(JSContext *cx, HandleObject obj, HandleValue callbackFn, HandleValue thisArg)
> +{
> +    MOZ_ASSERT(SetObject::is(obj));
> +    const char* fe = "SetForEach";

I have a bunch of style nits here. Sorry, JS has always had different style rules from the rest of Gecko...

Please delete this variable and just put the string "SetForEach" in the one place where it's used.

@@ +2212,5 @@
> +SetObject::forEach(JSContext *cx, HandleObject obj, HandleValue callbackFn, HandleValue thisArg)
> +{
> +    MOZ_ASSERT(SetObject::is(obj));
> +    const char* fe = "SetForEach";
> +    JS::Rooted<jsid> forEachId(cx, NameToId(cx->names().forEach));

Change JS::Rooted<jsid> to RootedId.

@@ +2214,5 @@
> +    MOZ_ASSERT(SetObject::is(obj));
> +    const char* fe = "SetForEach";
> +    JS::Rooted<jsid> forEachId(cx, NameToId(cx->names().forEach));
> +    JS::Rooted<JSFunction*> forEachFunc(cx);
> +    forEachFunc = JS::GetSelfHostedFunction(cx, fe, forEachId, 2);

RootedFunction forEachFun(cx, GetSelfHostedFunction(cx, "SetForEach", forEachId, 2);

@@ +2217,5 @@
> +    JS::Rooted<JSFunction*> forEachFunc(cx);
> +    forEachFunc = JS::GetSelfHostedFunction(cx, fe, forEachId, 2);
> +    if (!forEachFunc) {
> +        return false;
> +    }

Please drop the curly braces since the "if" and the body each fit on a single line.

@@ +2221,5 @@
> +    }
> +    JS::AutoValueVector args(cx);
> +    if(!args.append(callbackFn)) {
> +        return false;
> +    }

SM style requires a space after 'if'.

Factor out common code here. Come on -- we all know copying and pasting code is not a good engineering practice. Don't let me catch you at it again! :)

Since neither function uses any private fields of MapObject or SetObject, the common implementation can be just a plain old static function in this file (not a member of either class). We don't really need MapObject::forEach and SetObject::forEach; it's OK to leave them out.

Use this code for calling a function, instead of using AutoValueVector and JS::Call:

    InvokeArgs args(cx);
    if (!args.init(2))
        return false;
    args.setCallee(ObjectValue(forEachFun));
    args.setThis(UndefinedValue());
    args[0].set(callbackFn);
    args[1].set(thisArg);
    return Invoke(cx, args);
}

Comment 33

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Jason Orendorff [:jorendorff] from comment #32)
> Comment on attachment 8579810 [details] [diff] [review]
> Patch 7 (v1) - Add forEach convenience function for maplike/setlike
> 
> Review of attachment 8579810 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this patch has a problem: it will expose the private Map/Set object
> to anyone who calls the forEach method on the IDL maplike/setlike.
> 
>     maplike.forEach(function (key, value, map) {  // map receives the
> private Map object
>         map.set(key, "potato");  // oh no, putting a value of the wrong type
> into the map
>     });
> 
> It seems like you can fix this by writing new self-hosted functions in
> js/src/builtin/Map.js and Set.js that do what you want. They would be used
> solely to implement these APIs. I'll review your new patch once that's done,
> so I'm just clearing the r? flag for now.
> 

No it won't, because we don't pass it the callback directly from the user. We create our own callback (see ForEachHandler in BindingUtils.h/.cpp in Patch 4 for this bug), which wraps the callback and our own object, and pass /that/ to the self hosted forEach. forEach calls that with the value, key, and map/set, we unpack our objects from the slots, and then call the user callback with value, key, and the DOM object for the maplike/setlike interface, not the map/set.

Comment 34

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Oops, meant to ni? on comment 33. Anyways, will fix comments from review, just want to make sure the approach we took that I explained in comment 33 is ok with you too. :)

Comment 35

[Boris Zbarsky [:bzbarsky]]

Kyle, I'm not going to get a chance to look at these until Monday.

Comment 36

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Not doing reviews right now from comment #35)
> Kyle, I'm not going to get a chance to look at these until Monday.

Oh, yeah, totally didn't expect you to get to them before mid next week, even. Isn't really blocking me from anything.

Comment 37

[Jason Orendorff [:jorendorff]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #33)
> > I think this patch has a problem: it will expose the private Map/Set object
> > to anyone who calls the forEach method on the IDL maplike/setlike.
>
> No it won't, because we don't pass it the callback directly from the user.
> We create our own callback [...]

Oh, OK. That's sort of too bad, since hopping back and forth between C++ and JS isn't fast, but it'll work, and I don't see a way that's both faster and easier.

I'll go ahead and mark these r+. Thanks.

Comment 38

[Jason Orendorff [:jorendorff]]

Comment on attachment 8579810 [details] [diff] [review]
Patch 7 (v1) - Add forEach convenience function for maplike/setlike

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

r=me with the remaining review comments in comment 32 addressed.

Adding a 'maplike'/'setlike' argument to MapForEach/SetForEach, and more self-hosted functions in Map.js/Set.js, might simplify the changes in Codegen.py and eliminate the need for a wrapper JSNative. Feel free. But what you've got is fine with me too.

Comment 39

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

bz: Right now if we mark maplike/setlike as readonly, I don't generate the add/set/clear/delete functions at all (being able to add/edit objects from C++ even when readonly should happen as part of the helper patch here, once I finish that). However, I just realized those might be handy for js-implemented maplike/setlike interfaces. Should I just set those to ChromeOnly if maplike/setlike is set to readonly in the interface?

Comment 40

[Boris Zbarsky [:bzbarsky]]

My gut feeling would be to make them ChromeOnly and also prefix the name with '_' to make it clear that these are not for normal use.  And only when the interface is in fact JS-implemented.  There's some precedent for that sort of thing with _create.

Comment 41

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Actually, I think I may have to do that anyways to fulfill the spec. :heycam was asking about being able to override the add/set/clear/delete functions for the CSS font stuff, which I currently can't do nicely. The spec says overrides should be possible for those functions, and you /can/ override them, but then there's no way to call the original functions if you do override them currently, because then I just never actually generate the function code. :|

Comment 42

[Cameron McCormack (:heycam)]

One other issue I realised over the weekend is that it's going to be awkward for me to switch to setlike since I need to (a) keep the FontFace elements in the set in a certain order according to the spec, and (b) store some additional information along with each FontFace that is in the set.  This leads me to believe that it's going to be easier to keep my current setup where I just have some nsTArrays on the FontFaceSet object, and not have a backing Set at all.

Ideally, I would still be able to say |setlike<FontFace>;| in the IDL but somehow opt out of the backing Set object.  The codegen would still produce forEach, @@iterator, values, etc. for me, but in forms that defer part of their behaviour to methods I implement on FontFaceSet.

In the meantime, I have managed to get working implementations of forEach, values and @@iterator on FontFaceSet (patches going up soon in bug 1072101).  But at some point it would be good to work out how to move most of their implementation into the bindings code.

Comment 43

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Cameron McCormack (:heycam) from comment #42)

> Ideally, I would still be able to say |setlike<FontFace>;| in the IDL but
> somehow opt out of the backing Set object.  

This is my understanding of how iterable<> is supposed to work? Basically you just get those functions definitions but the backing storage is up to the implementer?

Comment 44

[Cameron McCormack (:heycam)]

Oh, right, yes that's what I want. :)  I think it's probably fine for classes that need to manage their own set storage to implement the entirety of their add, clear, delete, has methods (as they're not that tricky to write).

Comment 45

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Yeah, from my translation of the spec, that's what iterable<> seemed like. Since it /only/ has exposed iteration methods and @@iterable symbol handling, anything in relation to storage mutation would be done in chrome only anyways (basically assuming ALWAYS readonly from the content side). 

Now that I'm getting a bit better of an idea what's needed there, it might not be too bad to implement that after maplike/setlike lands. I'd already gotten the grammar in, it'd mostly be a matter of dropping the slot retrieval and just pointing to an non-binding function, and handling the @@iterator symbol hookup.

Not happening before the end of the week though. :/

Comment 46

[Cameron McCormack (:heycam)]

Great!  After next week is fine. :-)

Comment 47

[Boris Zbarsky [:bzbarsky]]

The hard part of implementing iterable<> is the actual Iterator implementation.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Note: need to check the enumerability bits here, since heycam says per spec the maplike/setlike stuff should be non-enumerable.

Comment 49

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 5 (v2) - Add maplike/setlike convenience function generation to WebIDL Codegen

Finally got all of the convenience functions for accessing containers from C++ set up. That said, this is /not/ pretty. I hacked up getArgs from CGNativeMember to do this, because we have way less options for what we'll need to generate arguments for. Only asking for fb? right now though because this seems like a complete hackjob and I'm guessing there's a better way to do it.

Comment 50

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v5) - Implement maplike/setlike in WebIDL parser

Turning off enumerable attribute for maplike/setlike properties

Comment 51

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 4 (v3) - Add maplike/setlike binding function generator in WebIDL Codegen

Turning off enumerable for maplike/setlike properties

Comment 52

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v6) - Implement maplike/setlike in WebIDL parser

Cleaned up checking container type for maplike/setlike, added chrome only generation for jsimplemented interfaces.

Comment 53

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 4 (v4) - Add maplike/setlike binding function generator in WebIDL Codegen

Cleaned up checking container type for maplike/setlike, added chrome only generation for jsimplemented interfaces.

Comment 54

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 5 (v3) - Add maplike/setlike convenience function generation to WebIDL Codegen

Cleaned up checking container type for maplike/setlike, added chrome only generation for jsimplemented interfaces.

Comment 55

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 8 (v1) - WIP: Test interfaces and mochitests for generated Maplike/Setlike bindings

Adding simple test coverage. Still need to add convenience function tests, and js implemented class tests.

Comment 56

[alexander :surkov (:asurkov)]

on a side note: is it supposed to be part of WEB IDL spec one day or is it something for Gecko internals only?

Comment 57

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Already part of the draft webidl spec, and partially implemented in chrome also. 

http://heycam.github.io/webidl/#dfn-maplike

Comment 58

[alexander :surkov (:asurkov)]

cool (sorry for continuing spamming here). Would these types benefit from having hasAnyOf and hasAllOf methods? If not then is there web idl extension mechanism to create types inherited from standard types?

Comment 59

[Cameron McCormack (:heycam)]

The intention is for the API to be just like the standard JS Set/Map objects, so I don't think we should add new methods here.  Of course, a spec writer can add whatever additional methods they like to the interface.

Comment 60

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8582661 [details] [diff] [review]
Patch 3 (v6) - Implement maplike/setlike in WebIDL parser

>@@ -689,6 +690,23 @@ class IDLInterface(IDLObjectWithScope, IDLExposureMixins):
>+                    raise WebIDLError("Interface declaration %s called on "

s/called/used/

>+                                      [self.location, member.location])

I think [self.maplikeOrSetlike.location, member.location] would make more sense here.

>+        # If we've got a maplike or setlike declaration, we'll be making all of
>+        # our required methods in Codegen. Check for possible conflicts now.

Shouldn't this happen after we pull in unforgeables from the parent, if any?  Worth adding a test for this.

>+            if self.parent.maplikeOrSetlike is not None and self.maplikeOrSetlike is not None:
>+                raise WebIDLError("Cannot have a declaration on %s that "
>+                                  "inherits from %s, which already has a declaration" %

This doesn't seem like a strong enough test.  Consider:

  interface A {
    setlike<long>;
  }
  interface B : A {};
  interface C : B {
    setlike<boolean>;
  }

Please add a test to this effect to the parser tests?

But also, we're repeating this check in the "if self.parent" block where we pull in unforgeables from the parent... and there we get it right (walk the whole ancestor chain).  We should really do this in only one spot.  And I claim that one spot is the "for ancestor in self.getInheritedInterfaces()" loop, where we are doing precisely what we want here: walking over all interfaces we inherit from.  And the location array for the WebIDLError we would raise in that case should be [self.maplikeOrSetlike.location, ancestor.maplikeOrSetlike.location], of course.

>+        return "declared '%s' of key '%s'" % (self.container_type, self.key_type)

"with key"?

>+    def expand(self, members, is_js_implemented):

We use isJSImplemented elsewhere in this file.

>+        def addMethod(name, check_attr_only, return_type, args=[], chrome_only=False):

check_attr_only should probably be called "allowExistingOperation" or something?

Also, check_attr_only and chrome_only are correlated.  check_attr_only is only true for mutators, and chrome_onyl is only true for mutators and only if is_js_implemented.  Not sure whether we want to just couple them together into a isMutator arg... what you have is probably better.

>+            for member in members:

So... per spec, this is supposed to check for conflicts on inherited interfaces too.  We should either do that or get the spec changed.  Either way, please add parser tests.

Also, the spec, and this code, disallows static attributes/operations, but I see no reason to do that...  Worth talking to heycam about.  And again, either way add parser tests.

>+                if member.identifier.name == name and \
>+                (not check_attr_only or (member.isConst() or member.isAttr())):

Probably better to use parens around the if condition instead of backslash here.  Certainly more likely to have sensible whitespace.  ;)

And you don't need the extra parens around the second "or".

>+                    raise WebIDLError("Interface %s member '%s' conflicts "
>+                                      "with reserved %s declaration method." %
>+                                      (self.identifier.name, member.identifier.name,

self.identifier.name is not the name of the interface here, right?  Need to fix that part.

>+        is_chrome_only = True if is_js_implemented and self.readonly else False

This is an overcomplicated way of writing:

  is_chrome_only = is_js_implemented and self.readonly

>+        # Both maplike and setlike have a size attribute

Shouldn't we check that the interface doesn't have this attribute?  The addMethod bits do that check, but you're not using addMethod here.  Please add parser test.

>+        members.append(IDLAttribute(self.location,
...
>+                                    inherit=False,

inherit=False is the default, no?  Why are we passing it here?

>+        # In order to take advantage of all of the method machinery in Codegen,
>+        # we generate our functions as if they were part of the interface
>+        # specification during parsing.

Shouldn't this be before we deal with "size" at least?  Or more generally in a docstring for this method?

>+        # void forEach(callback(key_type, value_type), thisPtr)

thisVal, not thisPtr.

Also, it's callback(value_type, key_type).

>+                             IDLArgument(self.location,
>+                                         IDLUnresolvedIdentifier(BuiltinLocation("<auto-generated-identifier>"), "thisArg"),
>+                                         BuiltinTypes[IDLBuiltinType.Types.object],
>+                                         optional=True)]

No, this is wrong.  This needs to be of type any, not of type object.  We should add a functional test that doing:

  maplike.forEach(function() {
    "use strict";
    ise(this, 5, "This value should be correct");
  }, 5)

works correctly and does not throw.  I bet with your current patches it does throw.

>+        if not self.readonly or is_js_implemented:
>+            # void clear()
>+            addMethod("clear", True, BuiltinTypes[IDLBuiltinType.Types.void], [], is_chrome_only)

So technically, this will mean that an interface using a readonly setlike can't have a "clear" attribute if it's JS-implemented, but can if it's not, right?  That seems a bit suboptimal.  Would it make sense to give these some sort of underscored names instead in the not readonly and is_js_implemented case?

>+                # Add returns the set object it just added to.

So... the spec at http://heycam.github.io/webidl/#es-add-delete has some problems:

1)  It returns the underlying Set object, not the setlike object, in the "add" case.

2)  In step 7, it performs the [[Get]] on "O", not on "set".

>+                addMethod("add", True, BuiltinTypes[IDLBuiltinType.Types.object], [getKeyArg()], is_chrome_only)

This should have an optional arg, no?  http://heycam.github.io/webidl/#es-add-delete says to use undefined if not passed...  But then conversion happens unconditionally.  That's actually neither the optional idl arg behavior (which would treat undefined as "not passed") nor the required idl arg behavior (which would check argc).  We should add a testcase for this once we have an actual setlike interfaces...

setlike "delete" as the same issue, as does setlike "has" and maplike "get", "has", "delete", "set".  I guess I'll see what your actual codegen for these is like in later patches...

>+        value_arg = IDLArgument(self.location,

I'd really prefer a separate IDLArgument object for each method, not one shared across all of them, please.  So make this a getValueArg function?

>+        # value_type get(key_type key)
>+        #
>+        # Note that instead of the value type, we're using any here. The
>+        # validity checks should happen as things are inserted into the map,
>+        # and using any as the return type makes code generation much simpler.

Do you mind filing a followup bug on this?  It seems like we should be able to have pretty simple codegen here anyway, with a bit of finagling, but provide much better type info to the JIT.

Also, seems to me like we should be able to mark the get and has methods as [Pure], right?  Followup is OK for this too.

>+        addMethod("get", True, BuiltinTypes[IDLBuiltinType.Types.any], [getKeyArg()])

Afaict per spec the second argument here should be False.  Please add a parser test.

And http://heycam.github.io/webidl/#es-map-get-has step 7 is bogus; should be doing the [[Get]] on "map", not "O".

http://heycam.github.io/webidl/#dfn-forwards-to-the-internal-map-object has the same problem in step 7.

As does http://heycam.github.io/webidl/#dfn-forwards-to-the-internal-set-object step 7.

As does http://heycam.github.io/webidl/#es-map-delete step 6.

As does http://heycam.github.io/webidl/#es-map-set step 6.

And http://heycam.github.io/webidl/#es-set-size step 5.

And http://heycam.github.io/webidl/#es-map-size step 5.

And http://heycam.github.io/webidl/#es-iterator the maplike/setlike case steps 4/5 should pass "backing", not "object" as the first arg of CreateMap/SetIterator.

>+            addMethod("set", True, BuiltinTypes[IDLBuiltinType.Types.object], [getKeyArg(), value_arg], is_chrome_only)

This has the same problem as setlike "add" in the spec: it returns the wrong object.

I guess I'll try to go through and just fix the obvious spec bugs here.

> class IDLAttribute(IDLInterfaceMember):
>+        self.maplikeOrSetlike = maplikeOrSetlike

Should we assert it's None or an IDLMaplikeOrSetlike like we do in IDLMethod?

>+    def isMaplikeOrSetlikeAttr(self):

Needs docs?

>@@ -3826,6 +4027,11 @@ class IDLMethod(IDLInterfaceMember, IDLScope):
>+    # true if this method was generated as part of a
>+    # maplike/setlike/etc declaration
>+    def isMaplikeOrSetlikeMethod(self):

Probably better to use a docstring for the docs.

>+            Maplike : ReadOnly MAPLIKE LT Type COMMA Type GT SEMICOLON

Hmm.  Do we want to disallow maplike<long, void> like the spec does now?  It's a silly thing to do, I guess... ok, let's disallow for now.

r=me on this part, I think, with the above bits addressed.  But please do add those parser tests exercising this stuff!  And I'd like to see the updated patch.

Comment 61

[Boris Zbarsky [:bzbarsky]]

Cameron, two spec questions on maplike/setlike:

1)  Do we really want to check for name conflicts between the maplike/setlike stuff and inherited interfaces?  Seems like we could just allow shadowing like we would for any other method.

2)  Do we really want to disallow name conflicts between maplike/setlike and static attributes/operations?  Those live on a different object, so I don't think there's a problem with having them "conflict"...

Comment 62

[Boris Zbarsky [:bzbarsky]]

https://github.com/heycam/webidl/pull/47 covers the obvious spec bugs.

Comment 63

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2; FINAL) - Add public jsapi ES6 Map Delete function; r=jorendorff

Nits from initial review addressed.

Comment 64

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v2; FINAL) - Add public jsapi ES6 Set convenience functions; r=jorendorff

Nits from initial review addressed

Comment 65

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 7 (v2) - Add forEach convenience function for maplike/setlike

Comments from initial review addressed

Comment 66

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8591079 [details] [diff] [review]
Patch 7 (v2) - Add forEach convenience function for maplike/setlike

So much for Patch 7 being final. Totally breaks forEach, though my functional tests caught this but the "TODO: Test forEach" obviously means otherwise. Fixing and putting in new patch.

Comment 67

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 7 (v3; FINAL) - Add forEach convenience function for maplike/setlike; r=jorendorff

Fixed forEach patch, now also has tests (in patch 8)

Comment 68

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v7) - Implement maplike/setlike in WebIDL parser

Comments addressed, and many more tests added to patch 7 and patch 8 (getting attached to bug here in a sec).

Addressing a couple of comments:


> >+        if not self.readonly or is_js_implemented:
> >+            # void clear()
> >+            addMethod("clear", True, BuiltinTypes[IDLBuiltinType.Types.void], [], is_chrome_only)
> 
> So technically, this will mean that an interface using a readonly setlike
> can't have a "clear" attribute if it's JS-implemented, but can if it's not,
> right?  That seems a bit suboptimal.  Would it make sense to give these some
> sort of underscored names instead in the not readonly and is_js_implemented
> case?

So this was a case of me having short circuiting the short circuiting for the if in the wrong order, so I just flipped them. Will that be enough, or do we want to add the underscore? Also, heycam brought up the idea of possibly keeping underscored version of the functions if they're shadowed or overridden?

> >+        value_arg = IDLArgument(self.location,
> 
> I'd really prefer a separate IDLArgument object for each method, not one
> shared across all of them, please.  So make this a getValueArg function?

We only use value_arg once, so it doesn't seem like it needs a generation function like key_arg did?

> 
> >+        # value_type get(key_type key)
> >+        #
> >+        # Note that instead of the value type, we're using any here. The
> >+        # validity checks should happen as things are inserted into the map,
> >+        # and using any as the return type makes code generation much simpler.
> 
> Do you mind filing a followup bug on this?  It seems like we should be able
> to have pretty simple codegen here anyway, with a bit of finagling, but
> provide much better type info to the JIT.

I'm not quite sure how to phrase the followup bug here. Are we wanting better return type information given to the JIT?

> Also, seems to me like we should be able to mark the get and has methods as
> [Pure], right?  Followup is OK for this too.

Went ahead and did this in the patch. I think we can also do that on keys/values/entries?

Comment 69

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 4 (v5) - Add maplike/setlike binding function generator in WebIDL Codegen

Lots of changes from the initial parser review affected the Codegen patch.

Comment 70

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 6 (v2) - WebIDL Parser Mochitests for maplike/setlike

Wow, didn't realize I hadn't updated this patch since I started the bug. Far more parser tests now, but this could probably use even more, and also wider testing (e.g. testing validity of shadows for ALL functions versus just one, even though these are all generated in basically the same way).

Comment 71

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 8 (v2) - Test interfaces and mochitests for generated Maplike/Setlike bindings

More mochitests, updated from parser test review fallout

Comment 72

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff between attachment 8582664 and attachment 8593124, just for reference

Comment 73

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8593124 [details] [diff] [review]
Patch 3 (v7) - Implement maplike/setlike in WebIDL parser

This doesn't seem to address all of the comments from comment 60.  In particular I'm not seeing any of the tests I asked for, and doesn't address the second comment I made (about the interaction of unforgeables and expand()).  I didn't bother reading past that point for the moment, honestly....

Comment 74

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok, something went extremely wrong in my patch export. Fixing and resubmitting.

Comment 75

[Boris Zbarsky [:bzbarsky]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #68)
> So this was a case of me having short circuiting the short circuiting for
> the if in the wrong order, so I just flipped them. 

I'm not sure this addresses the problem.  The problem I'm worried about is an interface like so:

  interface Foo {
    readonly setlike<long>;
    readonly attribute boolean clear;
  };

Per spec, this is perfectly acceptable, right?  And in your implementation it's perfectly fine as long as Foo is not JS-implemented.  But if it is, then is_js_implemented will be true, so we will addMethod("clear", True, ...) and that will fail because there is an attribute named "clear".  The order of checks in that if condition doesn't matter, because either way the if body is entered.

Now this is a somewhat contrived example, but I'd rather not end up with chrome code using the unprefixed names for now and then have to change it all if some example like this comes up...  Hence my suggestion for underscoring.

> Also, heycam brought up the idea of
> possibly keeping underscored version of the functions if they're shadowed or
> overridden?

Right, that might be nice too.  Another point for underscoring.  ;)

Seems to me we should just have the JS-implemented case always add underscored versions of all this stuff and be done with it.

> We only use value_arg once, so it doesn't seem like it needs a generation
> function like key_arg did?

Right you are.  But in that case, can you move its declaration to right next to its one use (inside the same if body, even, would be nice).

> I'm not quite sure how to phrase the followup bug here.

I filed bug 1155340.

> Went ahead and did this in the patch. I think we can also do that on
> keys/values/entries?

No, since those are not idempotent: they return a new object each time.  Come to think of it, we should disallow that combination in general, because clearly it's nonsensical.  Filed bug 1155342.

I do think you could mark these three with [DependsOn=DeviceState, Affects=Nothing], though.  Yay, fine-grainedness.  ;)

Comment 76

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v8) - Implement maplike/setlike in WebIDL parser

New version of WebIDL Parser patch, hopefully with /all/ of the review changes this time, including the new underscoring stuff. Also folded Patch 6 (parser tests) into this patch now.

Comment 77

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8593617 [details] [diff] [review]
Patch 3 (v8) - Implement maplike/setlike in WebIDL parser

Ok, trying this again, now with the full patch, plus stuff from comment #75 taken care of.

Comment 78

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 4 (v6) - Add maplike/setlike binding function generator in WebIDL Codegen

New Codegen patch, now deals with the eccentricities of using things considered Identifierless as special Maplike/Setlike functions on js-implemented classes.

Comment 79

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 8 (v3) - Test interfaces and mochitests for generated Maplike/Setlike bindings

Updated interface tests to work with new __ functions for js implemented classes.

Comment 80

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated part 3 interdiff (attachment 8593124 to attachment 8593617)

Comment 81

[Boris Zbarsky [:bzbarsky]]

Attachment: Er, the interdiff I _actually_ want: attachment 8582664 to attachment 8593617

Comment 82

[Cameron McCormack (:heycam)]

Per some IRC discussion just now:

(In reply to Not doing reviews right now from comment #61)
> 1)  Do we really want to check for name conflicts between the
> maplike/setlike stuff and inherited interfaces?  Seems like we could just
> allow shadowing like we would for any other method.

Let's disallow conflicting names coming from inherited interfaces, but allow overriding of the auto-generated add/clear/delete to come from the interface itself or any consequential interface.  Disallowing conflicting names from inherited interfaces helps avoid issues with [Unforgeable] on the ancestors ending up overriding the auto-generated methods.

> 2)  Do we really want to disallow name conflicts between maplike/setlike and
> static attributes/operations?  Those live on a different object, so I don't
> think there's a problem with having them "conflict"...

Yeah, that restriction should be removed.

Comment 83

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8593617 [details] [diff] [review]
Patch 3 (v8) - Implement maplike/setlike in WebIDL parser

>+                # our required methods in Codegen. Generate members now.

I think it's worth documenting that it has to happen at this point because otherwise those members won't get the right Exposed state on them.

>+            if ancestor.maplikeOrSetlike is not None and self.maplikeOrSetlike is not None:
...
>+                                  (self.identifier.name, self.parent.identifier.name),

The second element of that tuple should be ancestor.identifier.name.

>+                                  [ancestor.location, self.parent.location])

And those should presumably be [self.maplikeOrSetlike.location, ancestor.maplikeOrSetlike.location].  I did say that in my original review comments!

>+        # At this point, we have all of our members. If there's a
>+        # maplike/setlike call anywhere in the chain,

That's wrong, as we discussed on IRC.  We don't need to fail if we have a member shadowing a maplike or setlike on some ancestor.  So we only need to do something here if self.maplikeOrSetlike and we just need to compare it to the .members of all our ancestors.  Which we don't have to copy, since we're not going to modify the lists.

>+        self.reserved_ro_names = []
>+        self.reserved_rw_names = []

Those are pretty inscrutable names.  And the fact that those happen to correspond to what's reserved when we're readonly and what's reserved when we're read/write is somewhat beside the point by the time we're actually using those members.  I'd suggest renaming "reserved_ro_names" to "disallowed_member_names" and "reserved_rw_names" to "disallowed_non_method_names".

>+            # Check for restricted added functions

Maybe "# Check that there are no disallowed members"

>+            # Check for restricted const/attr names

And maybe "# Check that there are no disallowed non-method members".

So Cameron and I just discussed this stuff a bit and there are two changes that we want to make (see <https://github.com/heycam/webidl/issues/49>):  1)  Static operations/attributes should be excluded from these name-collision checks.  2) The things in disallowed_non_method_names (which might again want a better name) should be disallowed for non-methods on the interface itself, but disallowed for everything on its ancestors.  So we need to pass in whether this member list is for the interface itself or one of its ancestors.

>+        def addMethod(name, allowExistingOperations, return_type, args=[],

So... I didn't catch this during the previous review, but there's an issue here: if allowExistingOperations is true and there is an existing operation with that name we do NOT want to generate our own operation with that name, right?  Please add a test for this (e.g. by defining clear()/set()/add() with a signature different from the one we'd expand to so you can test that it's still there and no other method with the same name got added).

Local style is mostly to use camelCase for args, not under_score_case, but either way.

>+            Create a function object based on the parameters passed in. chrome_only is

s/function object/IDLMethod/?

>+            prefixed convenience functions that will not be available like they
>+            are in the

s/will not be available like they are in the/would otherwise not be available, unlike the case of/

>+            functions, so it is not valid for things like keys, values, etc...

Single period, not ellipsis.

>+        # Here and below, the short circuiting order matters.

This part doesn't make sense to me.  There's not short-circuiting going on.  What I see happening is that, for example, a non-readonly decl will get clear() and a js-implemented one will get __clear().  A non-readonly js-implemented one will get both, looks like.  That all seems fine; it's just the comment that's not making sense.

>but they may
>+        # be marked as chrome only if it's a read only maplike/setlike.

But the thing being marked chromeonly is the underscore-prefixed thing, which is a distinct thing from the one we add if not readonly.  Still not sure what this comment is trying to say.

>+            # Short circuit on js implemented, so we get the chrome only function

Again, not sure what this comment is talking about.

>+    def isMaplikeOrSetlikeAttr(self):

This is defined twice in this patch.  Please remove one of those definitions (the one without a comment).

Moving on to the test now:

>+                      "%s - Should be one production" % (prefix))

"Should have production count %d" and substitute in num_prods?  ;)

>+        num_expected_members = len(expected_members) + 1

Please document why "+ 1".

>+    set_ro_methods = [(x, WebIDL.IDLMethod) for x in ["has"]]

Why not "+ iterable_methods" at the end there, so you don't have to manually add it in everywhere below.

>+    set_rw_methods = [(x, WebIDL.IDLMethod) for x in ["add", "clear", "delete"]]

And this could have "+ set_ro_methods" at the end.

Same for the map ones.

>+    checkFailingInterface("Conflicting method and declaration", """

We probably want variants of this for all the relevant method names, and for the conflict being with methods, attrs, and consts.  Also with the conflicting thing placed on the interface itself, a thing pulled in via "implements", or an ancestor (direct or indirect).  It's probably OK to do a T-like thing, where we test all possible names on the interface itself and one name in different placement locations.

>+    checkFailingInterface("Conflicting rw method and attribute declaration", """
>+    checkFailingInterface("Conflicting rw method and const declaration", """

We want variants of these for all the relevant method names.  And again, with the const or attribute placed in the three or four possible different places.  Again, something T-like is OK here.

>+    checkPassingInterface("Interface with allowed overload", """

So we should check that the overload is allowed on the interface itself or something pulled in via implements, but NOT on an ancestor.  Again, for all the relevant names, something T-like OK.

>+    checkFailingInterface("Inheritance of declaration with child name collision", """

This should be passing; the spec doesn't disallow it.

>+    checkFailingInterface("Inheritance of multi-level declaration with child name collision", """

This should be passing too.

>+    checkFailingInterface("Inheritance of attribute collision with parent declaration", """

This should be passing.

>+    checkFailingInterface("Inheritance of multi-level attribute collision with parent declaration", """

And this.

>+    """, iterable_methods + set_ro_methods + [("clear", WebIDL.IDLAttribute)], num_prods=1)

Why is the num_prods=1 needed?

>+    """, iterable_methods + set_ro_methods + set_rw_chrome_methods + [("clear", WebIDL.IDLAttribute)], num_prods=1)

And here.

Might be good to add a test for a writable js-implemented set, which would have set_rw_methods + set_rw_chrome_methods.  And add tests for js-implemented map too.

Comment 84

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8593617 [details] [diff] [review]
Patch 3 (v8) - Implement maplike/setlike in WebIDL parser

Actually, I think DependsOn=DeviceState is wrong for entries/keys/values.  It should be DependsOn=Everything, since: 1) It depends on the state of the Map/Set, which is DOMState, and 2) We don't want it CSE'd so we can't use DependsOn=DOMState...

We do still want the Affects=Nothing, though.  And maybe a followup bug to have this yet more fine-grained.  :(

Comment 85

[Boris Zbarsky [:bzbarsky]]

Actually, I filed that followup: bug 1155796.

Comment 86

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8593622 [details] [diff] [review]
Patch 4 (v6) - Add maplike/setlike binding function generator in WebIDL Codegen

>+++ b/dom/bindings/BindingUtils.cpp
>+GetMaplikeSetlikeSlot(JSContext* cx, JS::Handle<JSObject*> obj, int slotIndex, JS::MutableHandle<JSObject*> container) {

Watch the 80-char width?

Also, maybe call this GetMaplikeSetlikeBackingObject?

>+  // Have to either root across the getter call or reget after.

There is no getter call here.  There's the call to Method, right?

>+  JS::Rooted<JS::Value> slotval(cx);

Why does this need to be rooted?  I don't think it does.

>+    container.set(Method(cx));

This is bad.  Need to add tests that would have caught the badness...

Specifically, consider the case when we're called via Xrays (so reflector != obj), or even just via funcFromWindow1.call(funcFromWindow2).  In that case, cx and reflector are not same-compartment and this will create the Map or Set in the compartment of cx.  But we want that Map or Set in the compartment of reflector; otherwise a later access _not_ via Xrays will get all confused.

All of which is to say we should enter the compartment of reflector here.

>+      //aRv.Throw(NS_ERROR_OUT_OF_MEMORY);

Take this out, please.

>+    container.set(slotval.toObjectOrNull());

I'd prefer:

   container.set(&slotval.toObject());

since that makes it clear that the object is never null.

>+bool GetMaplikeSlot(JSContext* cx, JS::Handle<JSObject*> obj, int slotIndex, JS::MutableHandle<JSObject*> container) {

GetMapFromMaplike?  Or GetMaplikeBackingMap?  And again, 80 chars.

>+bool GetSetlikeSlot(JSContext* cx, JS::Handle<JSObject*> obj, int slotIndex, JS::MutableHandle<JSObject*> container) {

Same here, both naming and line length.

>+ForEachHandler(JSContext* aCx, unsigned aArgc, JS::Value* aVp)
>+{
>+  JS::Rooted<JS::Value> callbackFn(aCx, js::GetFunctionNativeReserved(&args.callee(), 0));
>+  JS::Rooted<JS::Value> containerObj(aCx, js::GetFunctionNativeReserved(&args.callee(), 1));

Again, line lengths.  And also, I would prefer symbolic names (via #define) for those 0 and 1.  You can probably put those in BindingUtils.h near where you declare this function.

>+  if (aArgc != 3) {
>+    return false;
>+  }

How could that happen?  Aren't you invoking trusted code here?  This might makes sense as an assert instead.  In any case, returning false without throwing is a bit weird: it means uncatchable and unreportable exception.  I doubt you meant that here.

>+  // Replace the map/set object with our own object

Maybe "Replace the map/set object in the arguments list with ..."?

>+++ b/dom/bindings/BindingUtils.h
>+bool ForEachHandler(JSContext* aCx, unsigned aArgc, JS::Value* aVp);
>+bool GetMaplikeSlot(JSContext* cx, JS::Handle<JSObject*> obj,
>+                    int slotIndex, JS::MutableHandle<JSObject*> container);
>+bool GetSetlikeSlot(JSContext* cx, JS::Handle<JSObject*> obj,
>+                    int slotIndex, JS::MutableHandle<JSObject*> container);

Documentation needed.  In particular, need to carefully document the "obj" argument of the two getters, need to document whether "container" ends up in the compartment of cx or not (more on this below), "container" could probably use a better name (e.g. backingObj).  The ForEachHandler could use a pointer to the spec section it implements.

>+++ b/dom/bindings/Codegen.py
>@@ -2175,6 +2175,19 @@ class MethodDefiner(PropertyDefiner):
>+        if descriptor.interface.maplikeOrSetlike:
>+            for m in methods:
>+                if m.identifier.name == "entries":

Shouldn't this be "values" for the setlike case?  Needs a test.

>+                        "func_name": m.identifier.name,

Code style would be funcName, not func_name, I think.

>+                        "methodInfo": True,

This is the default.  No need to pass it explicitly.

>+                        "returnsPromise": False

This is also the default.

>@@ -2272,7 +2285,9 @@ class MethodDefiner(PropertyDefiner):
>+                func_name = m["func_name"] if "func_name" in m else m["name"]

I think

  func_name = m.get("funcName", m["name"])

would be more readable here.

>@@ -2365,7 +2380,8 @@ class AttrDefiner(PropertyDefiner):
>+            return ("JSPROP_SHARED" + enumerable +
>                     unforgeable)

This should all be on one line.

>+def getMaplikeOrSetlikeContainerFromSlot(maplikeOrSetlike):

I think thinking of the Set or Map as "Container" is a bit weird.  It's a BackingObject is what it is.  This applies to a bunch of thie code here.

>+        slot=memberReservedSlot(maplikeOrSetlike), func_prefix=func_prefix)

Separate lines for the separate substitution parameters, please.

>+def getMaplikeOrSetlikeGetterBody(attr):

Needs documentation.

>+    maplikeOrSetlike = attr.maplikeOrSetlike

Assert attr.isMaplikeOrSetlikeAttr()?

>+    if maplikeOrSetlike.isMaplike():
>+        func_prefix = 'Map'
>+    elif maplikeOrSetlike.isSetlike():
>+        func_prefix = 'Set'

This is repeated in CGMaplikeOrSetlikeMethodGenerator.  Maybe factor out?

>+        raise TypeError("Declaration type %s not recognized!" %
>+                        (maplikeOrSetlike.container_type))

No need for the extra parens around the thing folloing %.

>+    func_name = func_prefix + "Size"

Why this instead of just putting the "Size" in the template?

>+    return getMaplikeOrSetlikeContainerFromSlot(maplikeOrSetlike) + \
>+    fill("""

How about just a single fill() call, with the getMaplikeOrSetlikeContainerFromSlot(maplikeOrSetlike) being substituted into the fill template?

>+    uint32_t result(JS::${func_name}(cx, container));

Would prefer = to construction here.

Also, this is relying on some implementation details of MapSize/SetSize.  In particular, nothing here guarantees that container and cx are same-compartment.

I think we should fix this by making the BindingUtils methods ensure that their return value is in the context compartment.  That will incidentally make things work correctly once we have Map/Set Xrays working.  The other option would have been to enter the compartment of the backing object before working with it, but that would make it pretty hard to use with Xrays.

>+    """, func_name=func_name)

Substitution value on separate line, please.

>+class CGMaplikeOrSetlikeMethodGenerator(CGThing):
>+    """
>+    Class responsible for filling in the body of maplike/setlike/etc
>+    declarations on interfaces.

The body of _methods_ created for those declarations, right?  Also, there is no "etc" here.  Just "maplike or setlike declarations".

>+    def __init__(self, descriptor, method):
>+            # double underscore means this is a js-implemented chrome only
>+            # rw function. Truncate the double underscore so routing still
>+            # works.

"routing"?  You mean calling the right underlying JSAPI function?  If so, worth just saying that explicitly.

>+    def generate_js_call(self, needs_key=False, needs_value=False,

I find this function pretty hard to read and make sense of.  It seems to me like fundamentally what we have going on here is that each of the methods on this object wants to add the CGGeneric(getMaplikeOrSetlikeContainerFromSlot(self.maplikeOrSetlike)) to the beginning of self.cgRoot, so we could just do that up front in __init__.  After that, it might make sense to have helper methods for appendKeyArgConversion/appendValueArgConversion.  And maybe something for needs_object_result.  but needs_any_result/needs_self_result are only used in one place each, and I think it would be a lot simpler to reason about the structure of the code to be output if we just output it in those places...

The shared code for computing func_name could also be hoisted to __init__.  At that point, what are we really ending up sharing between the various cases?  needs_value is only used in one place as well, as is needs_foreach.  I guess we have two places that use needs_self_result; we could factor that out into a helper as well (e.g. implemented in terms of CGWrapper).

>+        func_name = self.func_prefix + self.impl_method_name.title()
>+        if self.method.identifier.name == "forEach":
>+            func_name = self.func_prefix + "ForEach"

How about:

  func_name = self.func_prefix + MakeNativeName(self.impl_method_name)

?

>+            if(${name}.WasPassed() && !ToJSValue(cx, ${name}.Value(), &${name}Val)) {

This produces wrong behavior.

Say we have a setlike<long> and someone does add(undefined).  Per spec, this should add 0.  But your code will add an actual undefined.  We need to add a testcase for that once we have actual implementations.  Please file a followup bug on adding said testcases.

This is why I was muttering about the optionality stuff in the parser patches.  You need argument conversions that don't match anything else we have in IDL so far.

I think the best option may be to NOT mark the arguments as optional (which would incidentally give the wrong .length for the functions too; we should add tests for _that_) but make the following changes to codegen:

1)  In CGMethodCall don't output the argc check for setlike/maplike methods.
2)  In CGArgumetnConverter use args.get(${index}) for replacementVariables["val"] when generating code for a maplike/setlike method (which means either passing in a boolean for that or passing in the IDLMember involved).
3)  Asserting in CGMethodCall that the case when we have multiple signatures is not a maplike/setlike method.

>+            JSFunction* func = js::NewFunctionWithReserved(cx, ForEachHandler, 3, 0, "ForEachHandler");

Please pass nullptr for the name argument, so we don't waste time atomizing things here.

>+            JS::Rooted<JSObject*> funcObj(cx, JS_GetFunctionObject(func));

You need to null-check func and return false if it's null before doing this.

>+          if (!JS::${func_name}(${args})) {
>+            ErrorResult rv;
>+            rv.Throw(NS_ERROR_FAILURE);
>+            return ThrowMethodFailedWithDetails(cx, rv, \"${iface}\", \"${method}\");
>+          }

That's all pointless, since if false got returned then an exception got thrown on cx already, so the thing on rv will just get ignored (specifically, it will land in mozilla::dom::Throw, which will happily ignore it).  What you want here is:

  if (!JS::${func_name}(${args})) {
    return false;
  }

>@@ -11148,7 +11359,7 @@ class CGDescriptor(CGThing):
>+                elif m.isMaplikeOrSetlikeMethod() or not m.isIdentifierLess() or m == descriptor.operations['Stringifier']:

This is now seriously going over 80 chars.  Put parens around the condition and line-break after the "or"s, please.

But also, we now seem to have lots of places testing something like "m.isMaplikeOrSetlikeMethod() or not m.isIdentifierLess()".  Can we factor that out into a separate function or something?

r- for now; we _definitely_ need to fix the compartment bits here.

Comment 87

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8582664 [details] [diff] [review]
Patch 5 (v3) - Add maplike/setlike convenience function generation to WebIDL Codegen

This seems to duplicate a bunch of argtype logic from elsewhere in codegen.  Could that not have been reused?

Comment 88

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8593624 [details] [diff] [review]
Patch 8 (v3) - Test interfaces and mochitests for generated Maplike/Setlike bindings

It might be good to have a build peer look over the moz.build change.

>+++ b/dom/bindings/test/TestInterfaceMaplike.cpp
>+TestInterfaceMaplike::TestInterfaceMaplike(JSContext* aCx,
>+                           nsPIDOMWindow* aParent,
>+                           ErrorResult& aRv) : mParent(aParent)

Fix indent.

And why does the ctor need to take an ErrorResult?

>+TestInterfaceMaplike::Constructor(const GlobalObject& aGlobal,
>+                          ErrorResult& aRv)

Fix indent.

>+++ b/dom/bindings/test/TestInterfaceMaplike.h
>+  // Various nsISupports stuff or whatnot

There's no nsISupports stuff there....  Just take this comment out.

Similar comments apply to TestInterfaceMaplikeObject.

>+  nsString id = NS_LITERAL_STRING("whatever");

NS_NAMED_LITERAL_STRING.

Similar comments apply to TestInterfaceSetlike.

>+++ b/dom/bindings/test/mochitest.ini
>\ No newline at end of file

Need a newline.

>+++ b/dom/bindings/test/test_bug1123516_maplike.html

Ah, so there's no need for a followup for tests.  ;)

>+             base_properties = [["has", "function"],

Worth including the expected function .length values in here.

>+                     ok(!obj.propertyIsEnumerable(p[0]), "object property " + p[0] + " is not enumerable");

This test is not testing what you think, because obj.propertyIsEnumerable("foo") returns false unless obj has an _own_ property named "foo".  What you may want to do instead is walk the proto chain of obj until you find an object which has a property descriptor for p[0], then test the things in that property descriptor.

Also maybe check that obj[p[0]].name is the right thing?

>+             // Simple map creation and functionality test

No need for the curlies around the stuff after this, right?  They're completely pointless.  Just separate the test sections with blank lines.

>+                 m1 = m.set("test", 1);

So yeah, somewhere around here you should m.set("undefinedTest", undefined) and test that m.get("undefinedTest") === 0.  Also that m.set("notEnoughArgsTest") means m.get("notEnoughArgsTest") === 0.  And that m.set() means m.get("undefined") === 0.

>+                 m.forEach(function() {

Please check that the args passed to the callback are correct.  And that it's invoked the right number of times, etc.

>+                 ok(iterable, "@@iterable symbol resolved correctly");

@@iterator.

Also, worth checking that m[Symbol.iterator].length and m[Symbol.iterator].name are the right things.

Similar comments for TestInterfaceSetlike.  But also, for that one:

>+                     is(e[0], "test", "iterable first array element should be key");

That's wrong.  e[0] should be "t".  e should be "test".  That's because for a Set or setlike @@iterator maps to values, not entries.

Also, please make sure the reason strings for the various assertions in this test are unique, so a failing test actually points uniquely to a test assertion.

>+                 ok(true, "Testing this override for forEach");

You want info() here, I think.  Same in a few other places.

>+             // Test defaulting arguments to undefined

This test is wrong, as mentioned in the codegen patch.  After m.set(), you should have k === "undefined" and v === 0.

Similar in the setlike undefined test.

May want to test not-enough-args behavior for delete() and get() as well.

r=me with the nits fixed.

Comment 89

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Fixing up tests, and trying to get the consequential interface tests correct is getting a little confusing. Specifically, the comment:

> We probably want variants of this for all the relevant method names, and for the conflict being with
> methods, attrs, and consts.  Also with the conflicting thing placed on the interface itself, a thing 
> pulled in via "implements", or an ancestor (direct or indirect). 

Due to the shadowing happening in the non-consequential interface, this should pass, right?

interface Foo1 {
  void entries();
};
interface Foo2 {
  maplike<long, long>;
};
Foo1 implements Foo2;

Right now it fails because we generate 2 methods called "entries", and the check for duplicate symbols doesn't take into account that the one we're pulling in from the maplike consequential interface can be thrown away.

Comment 90

[Boris Zbarsky [:bzbarsky]]

> Due to the shadowing happening in the non-consequential interface, this should pass,
> right?

Congrats, I think you found a spec bug.  Seems to me like this should in fact fail, just like this would fail per current spec if the maplike were on Foo1 and the method on Foo2.

Comment 91

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

While we're at it then:

interface Foo1 {
  maplike<long, long>;
};
interface Foo2 {
  setlike<long>;
};
interface Foo3 {
};
Foo3 implements Foo1;
Foo3 implements Foo2;

This also passes because Foo3 just pulls in all of the maplikes or setlikes and shadows functions based on order. It seems like this should fail?

Comment 92

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok, the static thing is confusing me also. ACcording to the spec bug filed over at https://github.com/heycam/webidl/issues/49, the following interface should work AND create two functions named has:

interface Foo {
  static boolean has();
  setlike<long>;
};

I take that to mean that something like

interface Foo {
  static boolean has();
  boolean has();
};

should also work? That currently fails due to an identifier conflict, which also means the setlike example fails, even if I skip the collision check on identifiers for maplike/setlikes.

Comment 93

[Boris Zbarsky [:bzbarsky]]

> the following interface should work AND create two functions named has:

Yes.

> That currently fails due to an identifier conflict

So per spec right now:

  The identifier of a static operation also MUST NOT be the same as the identifier of a
  regular operation defined on the same interface. 

it's not clear to me why that restriction is there, but given that it is, maybe we should undo the change we made in https://github.com/heycam/webidl/issues/49 pending a more general reconsideration of whether it's ok to have static and non-static stuff with the same name.  :(

Comment 94

[Boris Zbarsky [:bzbarsky]]

Oh, and yes, I'd think the example in comment 91 should fail.

Comment 95

[Boris Zbarsky [:bzbarsky]]

I guess for now what we can do is stick with disallowing name collisions between setlike/maplike and static stuff, since allowing it would take more rearchitecting than we want here.

Comment 96

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v9) - Implement maplike/setlike in WebIDL parser

Comment 97

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v10) - Implement maplike/setlike in WebIDL parser

Ok. Tried to take into account all reviews and comments on parser so far, plus what we talked about on IRC with the static override not working. I /think/ this covers everything. Also, I went ahead and fixed naming, just needed to turn off my PEP8 linter (which was already getting somewhat ignored anyways, as the 80 char issues in patch 4 made obvious :) ).

Comment 98

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Not doing reviews right now from comment #86)

> Specifically, consider the case when we're called via Xrays (so reflector !=
> obj), or even just via funcFromWindow1.call(funcFromWindow2).  In that case,
> cx and reflector are not same-compartment and this will create the Map or
> Set in the compartment of cx.  But we want that Map or Set in the
> compartment of reflector; otherwise a later access _not_ via Xrays will get
> all confused.
> 
> All of which is to say we should enter the compartment of reflector here.

Ok, added a simple test to dom/tests/mochitest/chrome/test_sandbox_bindings.xul:

      try {
        var maplike = Components.utils.evalInSandbox("new TestInterfaceMaplike();", sandbox);
        maplike.set('test', 1);
      } catch(e) {
        ok(false, "Shouldn't throw when creating cross-compartment maplike/setlike interfaces " + e)
      };

Sure enough, any try to create the backing object crashes. I tried adding a JSAutoCompartment to GetMaplikeSetlikeBackingObject in BindingUtils, like so:

if (slotval.isUndefined()) {
    {
      JSAutoCompartment(cx, reflector);
      JS::Rooted<JSObject*> newContainer(cx);
      newContainer.set(Method(cx));
      if (NS_WARN_IF(!newContainer)) {
        return false;
      }
      js::SetReservedSlot(reflector, slotIndex, JS::ObjectValue(*newContainer));
    }
  }

This still crashes in SetReservedSlot due to mismatched compartments between reflector and newContainer, though. Not quite sure why that happens?

Comment 99

[Boris Zbarsky [:bzbarsky]]

>      JSAutoCompartment(cx, reflector);

No name, so this is a temporary that's deallocated on the same line.  You want:

  JSAutoCompartment ac(cx, reflector);

and also, file a bug on JSAutoCompartment not using the MOZ_GUARD_OBJECT_NOTIFIER_PARAM machinery, which would totally have caught this with a nice assert for you.

Comment 100

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8582664 [details] [diff] [review]
Patch 5 (v3) - Add maplike/setlike convenience function generation to WebIDL Codegen

Marking obsolete, as I'm rewriting to (hopefully) use CGNativeMember and folding into Patch 4.

Comment 101

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8591077 [details] [diff] [review]
Patch 1 (v2; FINAL) - Add public jsapi ES6 Map Delete function; r=jorendorff

Moved patch to bug 1159469

Comment 102

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8591078 [details] [diff] [review]
Patch 2 (v2; FINAL) - Add public jsapi ES6 Set convenience functions; r=jorendorff

Moved patch to bug 1159469

Comment 103

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8593118 [details] [diff] [review]
Patch 7 (v3; FINAL) - Add forEach convenience function for maplike/setlike; r=jorendorff

Moved patch to bug 1159469

Comment 104

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff from attachment 8593617 to attachment 8596944

Comment 105

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8596944 [details] [diff] [review]
Patch 3 (v10) - Implement maplike/setlike in WebIDL parser

>+        # Generate members of maplike/setlike calls. Since generated members

Maybe s/calls/declarations/?

>+                raise WebIDLError("Maplike/setlike interface %s cannot have "
>+                                  "maplike/setlike interface %s as a "
>+                                  "consequential interface" %
>+                                  (self.identifier.name, iface.identifier.name),
>+                                  [self.location, iface.location])

I think using [self.maplikeOrSetlike.location, iface.maplikeOrSetlike.location] would make a better last argument here.

>+                raise WebIDLError("Cannot have maplike/setlike on %s that inherits "
>+                                  "or implements %s, which is already maplike/setlike" %

Just inherits.  The implements stuff is handled above this; that's the "consequential interface" bit.

>+            # If allowExistingOperations is True, and another operation exists
...
>+            # (http://heycam.github.io/webidl/#dfn-static-attribute)

Not sure what the spec link has to do with the comment above.  Just drop it, please?

>+                    [IDLExtendedAttribute(self.location, ("DependsOn", "DeviceState")),

See comment 84.

Moving on to the test:

>+        names. If method_members is not None, this means we expect the

There is no method_members in there.  Do you mean method_passes?  But that's a boolean, never None.  Please fix this comment to say what it's trying to say, whatever that is.

>+    shouldFail("Interface with consequential maplike/setlike interface child member collision",

There's no "child" here.  Just remove "child" from that string, I think.

>+    shouldFail("Maplike interface with consequential interface child member collision",

Likewise.

>+    shouldPass("Consequential Maplike interface with inherited interface child member collision",

Hmm.  This is odd.  If the maplike were directly on Foo3, this would fail, right?  I think it should likewise fail if the maplike is on something that gets mixed into Foo3, as in this testcase...  I agree that the spec doesn't say that right now; I suggest a followup bug for this, for both Gecko and the spec.  Please let me know if you'd rather I filed the latter.

r=me with those nits picked; most importantly the DependsOn bit.  This is looking great!

Comment 106

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Not doing reviews right now from comment #105)

> Hmm.  This is odd.  If the maplike were directly on Foo3, this would fail,
> right?  I think it should likewise fail if the maplike is on something that
> gets mixed into Foo3, as in this testcase...  I agree that the spec doesn't
> say that right now; I suggest a followup bug for this, for both Gecko and
> the spec.  Please let me know if you'd rather I filed the latter.

https://www.w3.org/Bugs/Public/show_bug.cgi?id=28583
https://bugzilla.mozilla.org/show_bug.cgi?id=1159833

Comment 107

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v11, FINAL) - Implement maplike/setlike in WebIDL parser; r=bz

Nits addressed. "Final" patch is as much as it may still change based on Codegen reviews. :)

Comment 108

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v12) - Implement maplike/setlike in WebIDL parser; r=bz

Annnd totally submitted a patch I didn't mean to (had removed Or from maplikeOrSetlike, which made things read weirdly). Fixed.

Comment 109

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 4 (v7) - Add maplike/setlike binding function generator in WebIDL Codegen

Comment 110

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 4 (v8) - Add maplike/setlike binding function generator in WebIDL Codegen

Cleanup and comments. Still trying to work out how helpers could be generated via CallbackMember, if that doesn't happen soon will at least request review for rest of patch.

Comment 111

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 4 (v9) - Add maplike/setlike binding function generator in WebIDL Codegen

Comment 112

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8601925 [details] [diff] [review]
Patch 4 (v9) - Add maplike/setlike binding function generator in WebIDL Codegen

>+++ b/dom/bindings/BindingUtils.cpp
>+GetMaplikeSetlikeBackingObject(JSContext* aCx, JS::Handle<JSObject*> aObj,
>+                               int aSlotIndex,

Should probably be size_t, actually.

>+                               JS::MutableHandle<JSObject*> aBackingObj) {

Curly on next line.

>+GetMaplikeBackingObject(JSContext* aCx, JS::Handle<JSObject*> aObj,
>+                        int aSlotIndex,
>+                        JS::MutableHandle<JSObject*> aBackingObj) {

Same two comments here.

>+GetSetlikeBackingObject(JSContext* aCx, JS::Handle<JSObject*> aObj,
>+                        int aSlotIndex,
>+                        JS::MutableHandle<JSObject*> aBackingObj) {

And here.

I just realized that we need to preserve the wrapper for objects with a wrappercache when we create a backing object for them.  Otherwise GC could just kill off our wrapper and the backing object with it...  That means we either need the actual C++ object pointer in here or the caller needs to pass in a function pointer to call on aObj to preserve the wrapper or something.

>+++ b/dom/bindings/BindingUtils.h
>+// Unpacks backing object (ES6 map/set) from the reserved slot on a
>+// maplike/setlike interfaces.

"from the reserved slot of a reflector for a maplike/setlike interface"?

> If backing object does not exist, creates backing
>+// object in the compartment of the interface object (the aObj argument)

No, it doesn't.  aObj might be a reflector or a cross-compartment wrapper to a reflector.  But the backing object will always be created into the compartment of the reflector involved.  The return value of these methods will always be in the context compartment, on the other hand.   Please document all that.

>+++ b/dom/bindings/Codegen.py
>@@ -1140,6 +1140,19 @@ class CGHeaders(CGWrapper):
>+        for desc in descriptors:
>+            if desc.interface.maplikeOrSetlike:
...
>+                break

You don't want to break here.  Consider the case when descriptors contains a setlike and a maplike and you see the setlike first.  Or contains two maplikes with different types, for that matter.  You really do want to go through all the descriptors.

>@@ -2160,7 +2173,9 @@ class MethodDefiner(PropertyDefiner):
>+                # Methods generated as part of a maplike/setlike interface are
>+                # not enumerable.

Maybe "Methods generated for a maplike/setlike declaration are not enumerable"?  Otehr stuff on the interface can be enumerable.

>@@ -2171,9 +2186,20 @@ class MethodDefiner(PropertyDefiner):
>+                    any("@@iterator" in r["name"] for r in regular))

Why is this an "in" test?  That seems odd to me.  Shouldn't it be equality?

>+                raise TypeError("Cannot have indexed getter/attr with "
>+                                "other members that generate @@iterator, "
>+                                "such as maplike/setlike or aliased "
>+                                "functions.")

Please include the interface name in there somewhere so someone seeing that exception has a hope of fixing things up.

>+                raise TypeError("Cannot have maplike/setlike interface with "
>+                                "other members that generate @@iterator, "
>+                                "such as indexed getters or aliased "
>+                                "functions.")

And here.

>@@ -2373,8 +2425,10 @@ class AttrDefiner(PropertyDefiner):
>+            # Attributes generated as part of a maplike/setlike interface are
>+            # not enumerable.

Again, "Attributes generated as part of a maplike/setlike declaration ..."

>@@ -7044,7 +7114,10 @@ class CGMethodCall(CGThing):
>+            # Skip required arguments check for maplike/setlike interfaces, as
>+            # they can have arguments which are not passed but are also not
>+            # optional.

They're optional.  They're just not treated as not present.

>@@ -12432,6 +12520,15 @@ class CGForwardDeclarations(CGWrapper):
>+            if d.interface and d.interface.maplikeOrSetlike:

How would d.interface ever test false?

>@@ -13117,6 +13214,7 @@ class CGNativeMember(ClassMethod):
>+

Why this addition of a blank line?  Please don't.

>@@ -13127,7 +13225,6 @@ class CGNativeMember(ClassMethod):
>-

And don't remove this one either.

> class CallbackMember(CGNativeMember):
>+                 needsPreserveColor=True):

How about wrapScope='CallbackPreserveColor()' and moving the XXXbz comment from futher on down to here.  And then passing wrappingScope='obj' in the new stuff.   I think that would be a lot clearer than needsPreserveColor.

>+def getMaplikeOrSetlikeErrorReturn(isHelperImpl):
>+            return;""")

Should have a newline before end of string here.

>+    return "return false;"

And a \n at the end of this one too.

>+def getMaplikeOrSetlikeBackingObject(maplikeOrSetlike, isHelperImpl=False):
>+        if(!Get${func_prefix}BackingObject(cx, obj, ${slot}, &backingObj)) {

Space after "if"

>+          ${errorReturn}

This should be $*{errorReturn}.  See the weird indent you get in the generated code otherwise.

>+def getMaplikeOrSetlikeSizeGetterBody(attr):
>+        ${getBackingObj}

$*{getBackingObj}

>+        uint32_t result(JS::${funcPrefix}Size(cx, backingObj));

Again, prefer = to consturction:

  uint32_t result = JS::${funcPrefix}Size(cx, backingObj);

I assume there are no plans to make this a size_t on the spidermonkey side?

>+class CGMaplikeOrSetlikeMethodGenerator(CGThing):
>+        # True is this will be implementation will be the body of a C++ helper
>+        # function.

s/this will be implementation // or something?

>+        # Generated required pre/post code for the method we're generating.

s/Generated/Generate/

Also, not sure what "pre" code we're generating.  The "post" code is getting placed in self.post, right?  That might be worth documenting more clearly... and giving it a better name too it's more like self.setResult.

>+        methodGenerator()

So what does this actually output?  Seems like it mostly puts the argument conversions into self.cgRoot and then puts something (or nothing, depending) in self.post?  And maybe modifies self.arguments in the process, hmm...  I'm not super-happy with the action at a distance involved, but I'm also not sure I have a better proposal offhand, other than having this call return a tuple (a list of CGThings, the value we're currently putting in self.arguments, and the value we're currently putting in self.post) or something.

>+        self.funcName = (self.maplikeOrSetlike.prefix +
>+                         MakeNativeName(impl_method_name))

Why does this need to be a member, instead of just a local?

>+              ${errorReturn}

$*{errorReturn}

>+            errorReturn=self.returnStmt)),
>+            post=self.post))

The indent here is very confusing.  I would prefer it if we indented the stuff inside the CGWrapper more or something so it's clear which part is fill() args and which part is CGWrapper args.  So maybe:

self.cgRoot.append(CGWrapper(
    CGGeneric(fill(
        ...)),
    post=self.post))


>+    def appendArgConversion(self, name):
>+            // Convert the type back to a JS::Value after argument conversion check.

I'm not sure this comment is worth it or what "check" it's talking about.

>+            if(!ToJSValue(cx, ${name}, &${name}Val)) {

Space after "if".

>+              ${errorReturn}

$*{errorReturn}


>+    def appendKeyArgConversion(self):
>+        Generates arguments for methods that require a key. Helper functions will use

s/arguments/the key argument/?

>+            self.arguments += ", argv[0]"

I think we should make self.arguments a list, and then ", ".join() it at the point when we want the arg string.

>+    def appendKeyValueArgConversion(self):

Maybe appendKeyAndValueArgConversion?

>+
>+
>+    def appendObjectResult(self):

Please be consistent about how many blank lines there are between these methods.  I think you only want one here.

>+    def forEach(self):
>+        void foreach(callback c, object o);

No, the second arg does not have to be an object.  It can be any value.  And it's "forEach".

>+        ForEach takes a callback, and a possible object to use as 'this'.

s/object/value/

>+        implementing maplike/setlike. In order to get the object in instead of
>+        the map/set backing object,

"In order to make sure that the third arg is our interface object instead of the map/set backing object,"

>+            if (!func) {
>+              return false;

So this is ok because we never call this code in the isHelperImpl case?  If so, please assert that somewhere here.

>+    class HelperFunction(CGAbstractMethod):
>+        Generates context creation code and rooted JSObject for interface for

There's no context creation going on here.  Just getting of a JSContext.

>+                JSObject* objForCx = ifaceObj->GetWrapper();

Nothing guarantees that objForCx is not null here.  You probably want to ToJSValue ifaceObj and get the object from there.  Except you don't have a cx, of course.

What you should probably do is just Init() the AutoJSAPI with no arguments.  Then enter the compartment of the unprivileged junk scope on the cx, then ToJSValue, then UncheckedUnwap to get the reflector.  Then enter the compartment of the reflector, and proceed.

>+                if (NS_WARN_IF(!jsapi.Init(objForCx))) {

This is bogus anyway because Init() can gc, so after this line objForCx might be garbage, since it's not a Rooted.  So it's a good thing we're planning to get rid of it.  ;)

>+                code = self.code)

Nix the spaces around '='.

Continuing with CGMaplikeOrSetlikeHelperFunctionGenerator...

>+        CallbackMember.__init__(self,
>+                                [BuiltinTypes[IDLBuiltinType.Types.void], args],
>+                                name, descriptor, True,

Why True for needThisHandling?  That seems pretty odd to me here, since we in fact do not need it.  I guess  this is trying to work around the fact that CallbackMember.getArgs uses self.needThisHandling to decide whether to append the exception-handling bits.  For that, how about we just call CGNativeMember.getArgs directly instead of calling CallbackMember.getArgs?  I guess the other issue is the getCallSetup behavior....  Maybe we should just have another optional arg for suppressing that, defaulting to needThisHandling.  Or better yet, just override getCallSetup() to return empty string?  Or, for that matter, put the stuff your HelperFunction thing does into getCallSetup?

>+    def getArgs(self, returnType, argList):
>+        # We don't need the context or the value. We'll generate those instead.
>+        args = CallbackMember.getArgs(self, returnType, argList)[2:]

Like I said above, CGNativeMember.getArgs.

>+        # If we need to get back a boolean from the backing object operation,
>+        # pass it as a reference, before the ErrorResult..
>+        if self.needsBoolReturn:
>+            args.insert(len(args) - 1, Argument("bool&", "aRetVal"))

Why not an actual return value?  That seems like it would make for a nicer API, from C++...  It does require the "error return" code to return false when we have needsBoolReturn, and requires a boolean on the stack for the JSAPI outparam, but I suspect that's worth it.  The boolean on the stack can be set up in getRvalDecl.  The actual returning can be handled in getResultConversion.

>+    def getArgcDecl(self):
>+        # Don't use argc checking since we're not using subarray on the
>+        # AutoValueVector.

More to the point, we just don't need argc for anything.

>+        return CGGeneric("")

How about return None instead?

>+class CGMaplikeOrSetlikeHelperGenerator(CGThing):
>+    Note that we inherit from CGNativeMember only to pick up the argument
>+    functions.

You're not inheriting from CGNativeMember at all....

What you probably _should_ do is inherit from CGNamespace.  Then as long as you produce the right list to pass to your superclass __init__, you don't even need to override declare/define, right?


>+        elif self.maplikeOrSetlike.isSetlike():

Shouldn't that be an else and assert?

>+++ b/dom/bindings/moz.build

Why are these bits here instead of dom/bindings/test/moz.build?  At least for the exports, if not the unified sources...

Also, I don't quite get the setup here.  The source is compiled debug-only, but the tests that use new TestInterfaceMaplike are run both debug and opt, no?  Do you need to list the tests as debug-only in the test manifest?

>+++ b/dom/bindings/parser/WebIDL.py
>+        maplike/setlike method. Interfaces with maplike/setlike will generated

s/will generated/will generate/

>+        methods starting with __ for chrome only backing object access inJS

s/inJS/in JS/ ?

>+++ b/dom/bindings/test/TestInterfaceMaplike.h

Could use some docs here as to what the point of this class is.  Same for the other new test headers.

>+++ b/dom/bindings/test/TestInterfaceMaplikeObject.cpp
>+TestInterfaceMaplikeObject::TestInterfaceMaplikeObject(JSContext* aCx,

Why the unused aCx arg?  Just ditch it.

>+++ b/dom/bindings/test/test_bug1123516_maplikesetlike.html
>+                     // Can't enumerate, so have to use array notation instead of hasOwnProperty

The issue is not "can't enumerate", but rather that the properties are somewhere up the proto chain, no?

>+                     isnot(obj[p[0]], undefined,
>+                           prefix + " object has property " + p[0]);

I think this code would be a lot more readable if the loop header did:

  for (var [name, type, args] of properties) {

and then you can use name instead of p[0], type instead of p[1], args for p[2].  And if you really wanted to go for readability, I guess use backquoted strings instead of "+" all over, but either way.

>+             // TODO: turned off until map/set unboxing is fixed in js engine

Is that still the case?  If so, cite bug# here please.

>+++ b/dom/tests/mochitest/chrome/test_sandbox_bindings.xul
>+   <script type="application/javascript">
> 
>+   
>+</script>

Why this addition?  Please remove.

>+      // enable caret browsing temporarily to test caret movement

No, that's not what you're enabling.  ;)

>+      var prefs = Components.classes["@mozilla.org/preferences-service;1"].
>+                             getService(Components.interfaces.nsIPrefBranch);
>+      prefs.setBoolPref("dom.expose_test_interfaces", true);

Shouldn't this use pushPrefEnv?

r=me with the above bits fixed.  This is looking awesome!

Comment 113

[Boris Zbarsky [:bzbarsky]]

So for patch 4...  I just looked at the codegen you mentioned in bug 1159469, and there's all that computation of unwrappedObj... which seems completely pointless.  We should probably skip doing that, though a followup bug for that is fine.

Comment 114

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok, patch is mostly done but we're still blocked on landing until bug 1159469 lands, not to mention having a once over on the new patch when it's up might be nice. 

> >+        methodGenerator()
> 
> So what does this actually output?  

This now returns a tuple. The code is slightly more complicated, but probably makes more sense overall.

> >+++ b/dom/bindings/moz.build
> 
> Why are these bits here instead of dom/bindings/test/moz.build?  At least
> for the exports, if not the unified sources...

The test stuff is in bindings moz.build instead of tests because we actually have to compile in the implementation as well as the bindings, and that requires being in libxul to pick up the js symbols. The stuff in the test directory build file is mostly for backing the JS implemented classes.

That said, I did fix the tests so they only run in debug, and added comments to things to make the location reasoning clear.

> 
> >+      var prefs = Components.classes["@mozilla.org/preferences-service;1"].
> >+                             getService(Components.interfaces.nsIPrefBranch);
> >+      prefs.setBoolPref("dom.expose_test_interfaces", true);
> 
> Shouldn't this use pushPrefEnv?

I moved this test out of sandbox and over to the bindings tests, since it will only be able to run in debug anyways. It still uses the prefs service though, as using pushPrefEnv only affects the current document, meaning the creation of the test object in the iframe no longer works because the dom test preference isn't set.

Comment 115

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Oh and


> Again, prefer = to consturction:
>
>  uint32_t result = JS::${funcPrefix}Size(cx, backingObj);
>
> I assume there are no plans to make this a size_t on the spidermonkey side?

I think the functions at the moment are just following what MapObject did, and it uses a uint32_t for right now. Happy to file a followup, but I'm guessing this needs to be changed all the way down through the es6 map implementation?

Comment 116

[Boris Zbarsky [:bzbarsky]]

> as using pushPrefEnv only affects the current document

Uh... no, it shouldn't.  It changes prefs, so affects anything that's using prefs.  Can you please double-check exactly what's going on there?

> I think the functions at the moment are just following what MapObject did

Yes, I understand that.  The question is what happens if what MapObject does changes.  And the real real question is whether it makes sense to use "auto" here instead of "uint32_t".

I guess if things change people will audit callers, so uint32_t is fine.

Comment 117

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Not doing reviews right now from comment #116)
> > as using pushPrefEnv only affects the current document
> 
> Uh... no, it shouldn't.  It changes prefs, so affects anything that's using
> prefs.  Can you please double-check exactly what's going on there?

Filed followup in bug 1168514, because something is definitely not right but it's not obvious to me at the moment.

Comment 118

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 4 (v10) - Add maplike/setlike binding function generator in WebIDL Codegen; r=bz

Addressing review comments from Comment 112. Figuring bz will want to have another look through this, and also can't land until bug 1159469 is finished and landed.

Comment 119

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff from attachment 8601925 to attachment 8610746

Comment 120

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8610746 [details] [diff] [review]
Patch 4 (v10) - Add maplike/setlike binding function generator in WebIDL Codegen; r=bz

I'd probably prefer using a bool* for the outparam to using a bool&.

>@@ -2189,41 +2188,43 @@ class MethodDefiner(PropertyDefiner):
>+                                "interface %s with other members "
...
>+                                self.descriptor.name)

self.descriptor.interface.identifier.name (it's not the same thing in some cases, sadly).

Same thing for the other TypeError you're adding here.

>             # Skip required arguments check for maplike/setlike interfaces, as
>+            # they can have arguments which are not passed, and are treated as
>+            # not present.

They're _not_ treated as not present.  They have, in effect, type-dependent default values....  Maybe "and are treated as if undefined had been explicitly passed"?

>+        # XXXbz we don't have anything better to use for 'obj',
>+        # really... etc

This comment should be up where the default value for wrapScope is set.  And the "This is set to 'obj'" comment should probably move to where we pass in 'obj' into the constructor.

>+        self.setResult = ""

This member seems to be unused.  Can it just go away?

>+            return self.mergeTuples(r, ([CGGeneric()], ["argv[1]"], []))

You should be able to use None instead of CGGeneric() here, I think.  CGList handles that.

But why can't that just be an empty list?

Same thing in the isHelperImpl case in appendBoolResult.

>+        is our interface object in instead of the map/set backing object, we

s/in instead/instead/

>+    def getResultConversion(self):
...
>+        return "return true;\n"

I would expect empty string when we don't have a boolean return value... and the function should probably return void in that case, right?

That is, for function that have a boolean return value it should be the actual return value, and for the others the return value should be void.  That will also require adjustment to getDefaultRetval, of course, and getCallsetup would need to use getDefaultRetval() for its early return (which it should have been anyway).

>                     // Can't enumerate, so have to use array notation instead of hasOwnProperty

This comment still needs fixing.

Looks lovely.

Comment 121

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Did a couple of try runs, first one had debug failures because I forgot to add the javascript test implementation to the package manifests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=328339e99feb

Redo of debugs after fixing package manifests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a680d8dece13

Comment 122

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 4 (v11) - Add maplike/setlike binding function generator in WebIDL Codegen; r=bz

Addressing review comments in Comment 120, adding test implementation file to package manifests

Comment 123

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 4 (v12) - Add maplike/setlike binding function generator in WebIDL Codegen; r=bz

Added test for cross-compartment issues discussed on bug 1159469, fb'ing 'cause you'll probably look at it even if I don't ask for anything. :)

Sorry it's across 2 bugs, this is getting kinda unwieldy again but hopefully this is the end of the changes.

Comment 124

[Boris Zbarsky [:bzbarsky]]

Do you mind posting the interdiff from v11 to v12?  Bugzilla's interdiff seems to give up the ghost on it...  If it's too much trouble, I can apply that patch locally and get the interdiff; just need to figure out which all bits need to go under it.

Comment 125

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 4 (v13) - Add maplike/setlike binding function generator in WebIDL Codegen; r=bz

More test updates, including a test to make sure we throw when trying to get an iterator via xrays.

Comment 126

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Interdiff - Patch 4 (v11) to Patch 4 (v13)

Here's the interdiff between Patch 4 v11 and Patch 4 v13

Comment 127

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8617536 [details] [diff] [review]
Interdiff - Patch 4 (v11) to Patch 4 (v13)

Various cases of weird indentation in here:

>+TestInterfaceSetlikeNode::TestInterfaceSetlikeNode(JSContext* aCx,
>+                                           nsPIDOMWindow* aParent)

>+TestInterfaceSetlikeNode::Constructor(const GlobalObject& aGlobal,
>+                                  ErrorResult& aRv)

>+TestInterfaceSetlikeNode::WrapObject(JSContext* aCx,
>+                                 JS::Handle<JSObject*> aGivenProto)

>+class TestInterfaceSetlikeNode final : public nsISupports,
>+                                   public nsWrapperCache

>+  explicit TestInterfaceSetlikeNode(JSContext* aCx,
>+                                nsPIDOMWindow* aParent);

(and also, no need for a two-arg ctor to be explicit).

>+++ b/dom/bindings/test/test_bug1123516_maplikesetlikechrome.xul

Why the kinda weird reindentation in this file?  

Looks good in general.

Comment 128

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 4 (v14) - Add maplike/setlike binding function generator in WebIDL Codegen; r=bz

Moving Xray check from MapObject to Codegen, since Mapobject doesn't have access to xpconnect in some cases.

Comment 129

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Interdiff - Patch 4 (v13) to Patch 4 (v14)

Comment 130

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8620812 [details] [diff] [review]
Interdiff - Patch 4 (v13) to Patch 4 (v14)

Looks great.

Comment 131

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 4 (v15) - Add maplike/setlike binding function generator in WebIDL Codegen; r=bz

Putting maplike/setlike chrome tests in the same pushPrefEnv block so there's not racing for SimpleTest.finish() (was causing intermittent orange on try).

Comment 132

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Interdiff - Patch 4 (v14) to Patch 4 (v15)

Interdiff for intermittent fix.

Comment 133

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/508488a1a714
https://hg.mozilla.org/integration/mozilla-inbound/rev/9749a0c63d81

Comment 134

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/508488a1a714
https://hg.mozilla.org/mozilla-central/rev/9749a0c63d81