1085293 - WebIDL codegen should support iterable<> members

Status: RESOLVED FIXED

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

Ehsan, this is the bug I mentioned yesterday.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Andrea, what's the timeframe you need this in?

Comment 3

[Andrea Marchesini [:baku]]

It's not a priority. When it will land, I'll use it.
It's for the URLSearchParams API and for FormData. Bugs 1085284 and bug 1085283.

Comment 5

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

There's some stuff in bug 1123516 that might be helpful once this starts up.

Comment 6

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

Conversation I had with bz on implementation of iterable (now needed for the headers part of the fetch spec, http://fetch.spec.whatwg.org/#headers-class):

<qDot> bz: So, for iterable, instead of filling in
entries/keys/values, I'll just create the function and leave iterator
object creation up to the binding implementor (unless there's an
indexed property on the interface), right?
<bz> qdot: generally speaking, yes
<bz> qdot: but we need the actual infrastructure for those iterator
objects too
<bz> Like auto-generate the relevant iterator prototype object and
have a way to create objects that it expects, etc
* bz is not quite sure what the best/cleanest way to do this is
<qDot> bz: Ah, ok, I was assuming that was somehow already there.
Damn.
<bz> Basically, for each iterable interface
<bz> we want a [NoInterfaceObject] interface for its iterator
prototype object
<bz> or somethign
<bz> er, something
<bz> So the C++ code can just create the relevant object and return it
and the bindings will wrap it
* bz ponders
<bz> yeah
<bz> So if Foo is iterable
<bz> we basically want a nointerfaceobject interface called
FooIterator to exist
<bz> And then the C++ is responsible for implementing it
<bz> Though if we cared we could perhaps autogenerate it
<bz> entirely
<qDot> That seems like something that could be done later though.
<bz> sure
* bz thinks some more
<bz> really, the right thing to do is to generate that
<bz> And have a C++ type called Iterator
<bz> templated on something
<bz> And have the nativeType for FooIterator be Iterator<Foo>
<bz> And implement that C++ Iterator class once
<bz> To basically implement the requirements for the next() method,
which are a tad annoying
<bz> and we don't want people implementing them over and over
<bz> Iterator<Foo> would hold a ref to a Foo
<bz> and ask it for things like the key/value at a given index.
<bz> Sound plausible?
<qDot> Yeah, that makes sense.
* bz is almost happy with that setup
<qDot> I can't really think of any other way to make it less work on
the binding implementor than that.
<bz> well, there's the question of minimizing the work needed to
implement this too
<bz> But I think synthesizing an interface like this with a hacked
nativeType should not be too painful

Comment 7

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

Attachment: Patch 1 (v1) - WIP: WebIDL Parser Changes for Iterable

Comment 8

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

Attachment: Patch 2 (v1) - Changes to Promise WebIDL argument naming to resolve conflict with Iterable

Comment 9

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

Attachment: Patch 3 (v1) - WIP: Binding Codegen Changes for WebIDL Iterable

Comment 10

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

Talking to :heycam on IRC, we've decided that single type iterators should function similar to how setlike functions, with keys() == values(), and entries() just return [value, value] for the iterator object value property. This still needs to be updated in the WebIDL spec, but the patches for this bug will reflect this idea.

Comment 11

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

Attachment: Patch 1 (v2) - Changes to Promise WebIDL argument naming to resolve conflict with Iterable

Comment 12

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

Attachment: Patch 2 (v2) - WIP: WebIDL Parser Changes and Tests for WebIDL Iterable

Comment 13

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

Attachment: Patch 3 (v2) - WIP: Codegen Changes and Binding Class for WebIDL Iterable

Comment 14

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

Attachment: Patch 4 (v1) - WIP: Tests for WebIDL Iterable Codegen

Comment 15

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

Attachment: Patch 2 (v3) - WebIDL Parser Changes and Tests for WebIDL Iterable

Due to iterable following a lot of the same rules as maplike and setlike, most of this patch is integrating iterable with all of the other maplike/setlike stuff already in there, so we can use the same logic for all of them. The actual iterable logic itself is fairly minimal.

Comment 16

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

Attachment: Patch 3 (v3) - Codegen Changes and Binding Class for WebIDL Iterable

Comment 17

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

Attachment: Patch 3 (v4) - Codegen Changes and Binding Class for WebIDL Iterable

Collapsed test patch down into implementation patch

Comment 18

[Boris Zbarsky [:bzbarsky]]

I'm sorry for the lag here.  Still catching up on other reviews, and my computer died this morning, so dealing with that ate up a good bit of today.  :(

I _might_ be able to get to this tomorrow (Friday), but Monday is more likely.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8662143 [details] [diff] [review]
Patch 1 (v2) - Changes to Promise WebIDL argument naming to resolve conflict with Iterable

Why is this needed?  In the spec, "iterable" is a valid value for ArgumentNameKeyword.  So shouldn't you just add it to the ArgumentName production in our parser?

Comment 20

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

(In reply to Boris Zbarsky [:bz] from comment #19)
> Comment on attachment 8662143 [details] [diff] [review]
> Patch 1 (v2) - Changes to Promise WebIDL argument naming to resolve conflict
> with Iterable
> 
> Why is this needed?  In the spec, "iterable" is a valid value for
> ArgumentNameKeyword.  So shouldn't you just add it to the ArgumentName
> production in our parser?

Ah, ok, wasn't aware it was valid. Fixed and added to parser, will just get rid of this patch.

Comment 21

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

Attachment: Patch 2 (v4) - WebIDL Parser Changes and Tests for WebIDL Iterable

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8667006 [details] [diff] [review]
Patch 2 (v4) - WebIDL Parser Changes and Tests for WebIDL Iterable

For review, it would have been nice to have the rename to maplikeOrSetlikeOrIterable in a separate patch from the substantive changes...  Ah, well.

>+        self.iteratorParentInterface = None

Maybe call it self.iterableInterface?  Maybe the naming you have is OK.  Not sure.

>+    def __init__(self, location, identifier, ifaceType, keyType, valueType, tagType):

That last argument isn't the type of tag, it's just the tag, right?  I'd call it "tag" or "typeTag" or something, not "tagType", because the other *Type arguments here are in fact IDLType, no?  Well, I guess ifaceType is not; it's a string.  Maybe call it ifaceKind?

>+        return self.valueType != None

I'd think "is not None" is more idiomatic.

>+    def addMethod(self, name, members, allowExistingOperations, returnType, args=[],

Need to document what "members" is.

>+# MaplikeOrSetlike adds a trait to an interface, like map or iteration

This is a strange comment to have right before defining the IDLIterable class.  Shouldn't this be a comment describing IDLIterable?  Looks like you ended up with copy/paste of this comment in two places...

>+class IDLIterable(IDLMaplikeOrSetlikeOrIterableBase):
>+        self.addMethod("entries", members, False, BuiltinTypes[IDLBuiltinType.Types.object],

Doesn't it actually return an instance of the relevant iterator interface?  Seems like it would be better to do that here instead of just claiming "object".  Same thing for "keys" and "values".

>+        # If an interface is iterable, we need to generate a new interface of
>+        # its matching iterator object.

s/interface of/interface for/

I'm not convinced about doing this in the middle of the production handling stuff.  It seems like it would fit better in IDLParser.finish.

>+            # TODO: Should this implement the BindingIterator interface 

You mean as in the "implements" keyword?  That might makes sense, yes, depending on how BindingIterator ends up being used in practice.

I do think this "next" method needs to be [Throws], since obviously it can do that.

>+++ b/dom/bindings/parser/tests/test_interface_maplikesetlike.py

This test should be renamed to test_interface_maplikesetlikeiterable.py, right?

r=me with the above addressed, but I definitely want to see an interdiff.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8663157 [details] [diff] [review]
Patch 3 (v4) - Codegen Changes and Binding Class for WebIDL Iterable

IterableIterator(Impl) might be a better name for the C++ class than BindingIterator...  It's not iterating bindings.

>+++ b/dom/bindings/BindingIterator.cpp
>+// Since this is used as the base of a templated class, we can't use the default
>+// wrapper cache CC here.

Wait, why not?  The templated class inheriting from us would just declare CC itself, traverse/unlink its own stuff, then forward things to us.  What exactly is the problem with that?  Is the issue that we can't use the normal macros with a template class because they don't work so well in a header and we can't put parts of the template in a .cpp file because then nothing would cause it to be instantiated or something?

Also, don't you need to CC mParent?  But see below.

Also also, does this really need to be wrappercached?  It's only returned from keys/entries/values, which are all [NewObject] methods (and should be marked so) and hence it seems like we could just avoid wrappercaching the iterator, no?

>+BindingIterator::BindingIterator(nsPIDOMWindow* aParent) :
>+mParent(aParent)

Weird indent there.  This should be more like:

BindingIterator::BindingIterator(nsPIDOMWindow* aParent)
  : mParent(aParent)

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

This is wrong.  More below.  Given that, I do think we can just use BindingIterator on the rhs of "implements" so that there is no BindingIteratorBinding stuff generated at all...

>+++ b/dom/bindings/BindingIterator.h

This file could use a nice documentation block, probably before the include guard so that it shows up in mxr, explaining what it's about and how one would use the classes it defines.  For example what methods the template parameter needs to support.

>+  typedef enum {
>+    Keys = 0,
>+    Values,
>+    Entries
>+  } BindingIteratorType;

Any reason this is in the templated class and not in BindingIterator?

>+  BindingIteratorImpl(T* aIterableObj, BindingIteratorType aIteratorType) :

Newline before this line, please.  And in general between the various method declarations in this class.

And member initializers should look like this:

  MyClass()
    : member1(arg)
    , member2(arg)
    , member3(arg)

>+    BindingIterator(aIterableObj->GetParentObject()),

Why not just return mIterableObj as the parent object, so we don't require aIterableObj to have a window as parent object (or indeed to have a parent object at all, if we nix the wrappercaching)?  Then, since we also nixed the wrappercaching, we could drop the CC impl from the BindingIterator class entirely, unless the template stuff requires it.

>+    return BindingIterator::WrapObject(aCx, aGivenProto);

This is wrong.  This will give the iterator the proto object of the BindingIterator interface, not of the right iterator interface for our iterable, no?

What we really want here is to call the WrapObject function for our iterator interface.  That probably means this function pointer needs to be a template argument, in addition to the iterable C++ type.

>+  virtual void Next(JSContext* aCx, JS::MutableHandle<JSObject*> result) override

If we switch to using "implements" with BindingIterator on the RHS, this won't need to be virtual.

Once you mark this stuff [Throws] there will be an ErrorResult arg here which you'll want to throw on and pass to CreateValueObject so _it_ can throw.

>+    bool done = (mIndex == mIterableObj->GetIterableLength() - 1);

No, that's wrong.  The right behavior for these iterators is that either "done" is true, or "done" is false and "value" is set to a value, no?

>+        // TODO: do something

Yeah, throw on the ErrorResult and return.  ;)

>+      JS::Rooted<JSObject*> pair(aCx, JS_NewArrayObject(aCx, 2));

If "pair" is null, throw on the ErrorResult and return.  But...

>+      JS_SetElement(aCx, pair, 0, key);

Yeah, don't do that.  You want to be using JS_DefineElement.  And you want the property to be enumerable.  And you need to handle the function call failing.

Rather then reinventing this wheel, I suggest defining two helper dictionaries in your webidl like so:

  dictionary IterableKeyOrValueResult {
    any value;
    boolean done = false;
  };

  dictionary IterableKeyAndValueResult {
    sequence<any> value = [];
    boolean done = false;
  };

and then this code can do, for the key/value cases:

  RootedDictionary<IterableKeyOrValueResult> dict(aCx);
  dict.mDone = whatever;
  dict.mValue = whatever;
  if (!ToJSValue(aCx, dict, &v)) // etc

and for the key-and-value case:

  RootedDictionary<IterableKeyAndValueResult> dict(aCx);
  dict.mDone = whatever;
  dict.mValue.Append(key);
  dict.mValue.Append(value);
  if (!ToJSValue(aCx, dict, &v)) // etc

The only difference between that and actually following the spec algorithm is that the order of the "done" and "value" properties will be backwards if the result object is enumerated, but since property enumeration order is not defined anyway...

Then you don't need the CreateValueObject function (which shouldn't be using JS_SetProperty, by the way) either.

If you don't want to do the dictionary thing (or are just curious), we should talk about how to set this stuff up properly.  Let me know.

>+  // Since we're templated on a binding

On a binding implementation type, no?

>+    if (mIterableObj) {
>+      mIterableObj = nullptr;

Why bother with the null-check?

>+    ImplCycleCollectionTraverse(nsCycleCollectionTraversalCallback& aCallback,

Why do you need this?  This overload of ImplCycleCollectionTraverse is already defined in nsRefPtr.h, which this file should be including anyway, right?

>+++ b/dom/bindings/Codegen.py

So some of these changes (the ones that change interface.maplikeOrSetlike to interface.maplikeOrSetlikeOrIterable) actually need to land in the same changeset as the parser patch, right?  Yet another reason the rename should have been its own patch.... I guess just fold the patches together when you go to push.

>+            # Iterable methods should be enumerable, maplike/setlike methods
>+            # should not.

Hmm, why not?  The spec seems to have all this enumerable...

>+                # Methods generated for a maplike/setlike/iterable declaration are not
>                 # enumerable.

This comment doesn't match the code.  Did you mean to remove it?

>         # Add the atoms cache type, even if we don't need it.
>         for d in descriptors:
>+            if d.interface.isIteratorInterface():
>+                continue

Why this change?

Also, why did we really need to skip forward-declaring the iterator interface type?

I guess the point is that the nativeType in this case is a template and the forward-declaration stuff isn't really good about declaring templates (and of course appending "Atoms" to the template class name won't go well).  If that's what's going on here, please document that clearly.

>-        def addHeaderBasedOnTypes(header, typeChecker):

Huh, I wonder when that became dead code....

>+class CGIterableMethodGenerator(CGThing):
>+        self.cgRoot.append(CGGeneric(dedent(

Why dedent() with %s instead of fill()?  fill() seems like a better idea here; more self-documenting replacements.

>+            // TODO (Bug 1173651): Xrays currently cannot wrap iterators.

Whyever not?

Bug 1023984 is about the SpiderMonkey built-in iterators, but our iterators here are Web IDL interfaces, so should get Xrays for free (and in particular, should end up creating the result objects in the Xray caller scope, not the target scope).  Have you tried testing this?  What fails?

>+            // TODO: We should probably actually check for iterable first/possibly second
>+            // type in the template here.

In what sense?

>+            typedef mozilla::dom::BindingIteratorImpl<%s> itrType;

Do you need the "mozilla::dom::" there?

>+            if(!ToJSValue(cx, itr, &v))

Add missing space after "if".

>+            // do something

Yeah, return false.

>+++ b/dom/bindings/Configuration.py
>+        # For generated iterator interfaces for other iterable interfaces, we

This comment isn't making sense to me.  The templated class is the derived class, not the base class...  Maybe more like:

        # For generated iterator interfaces for other iterable interfaces, we
        # just use BindingIteratorImpl as the native type, templated on the
        # nativeType of the iterable interface and on the relevant WrapObject
        # method.  That way we can have a templated implemenation for all the
        # duplicated iterator functionality.

>+            nativeTypeDefault = ("mozilla::dom::BindingIteratorImpl<mozilla::dom::%s>" %
>+                                 self.interface.iteratorParentInterface.identifier.name)

No, this isn't quite right.  What we really want here is the nativeType of the iterable interface's descriptor, right?

Getting that here is a bit of a pain; what we may want to do is make sure we do all the non-iterator Descriptors first in Configuration.__init__ and then create the iterator ones, at which point we'll have the iterable interface descriptor available and can get its nativeType here.

+        # Ignore iterable interface generated iterator types in type lists

Why?  Please explain.

>+++ b/dom/bindings/test/TestInterfaceIterableDouble.cpp
>+TestInterfaceIterableDouble::TestInterfaceIterableDouble(nsPIDOMWindow* aParent)
>+: mParent(aParent)

Indent.

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

Fix indent.

>+++ b/dom/bindings/test/TestInterfaceIterableSingle.cpp

Same indentation issues here.

>+++ b/dom/bindings/test/test_iterable.html
>+           // Properties are somewhere up the proto chain, hasOwnProperty won't

Yeah, but you should check the class string of both obj and owner.  Would have caught the WrapObject thing.

>deleted file mode 100644
>new file mode 100644

This needs to be an "hg mv", not a delete-and-readd.

Lots of changes needed here, so r- for now; would like to see the updated patch (and maybe interdiff, since I don't expect the codegen bits to need much change).

Comment 24

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

(In reply to Boris Zbarsky [:bz] from comment #23)
> >+    BindingIterator(aIterableObj->GetParentObject()),
> 
> Why not just return mIterableObj as the parent object, so we don't require
> aIterableObj to have a window as parent object (or indeed to have a parent
> object at all, if we nix the wrappercaching)?  Then, since we also nixed the
> wrappercaching, we could drop the CC impl from the BindingIterator class
> entirely, unless the template stuff requires it.

Ok, I'm trying to take out the wrapper caching, but I don't see how that gets us away from not having a CC impl. I still need to inherit nsISupports so that ToJSValue() and nsRefPtr<> will work, which is going to require CC'ing, right?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Inheriting nsISupports does not automatically require CC.

Comment 26

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

(In reply to Boris Zbarsky [:bz] from comment #23)
> >+            # Iterable methods should be enumerable, maplike/setlike methods
> >+            # should not.
> 
> Hmm, why not?  The spec seems to have all this enumerable...

The iterable functions should all be enumerable, but the spec says both the maplike and setlike members aren't. (https://heycam.github.io/webidl/#es-maplike)

Comment 27

[Boris Zbarsky [:bzbarsky]]

Huh, right you are. I was misreading this somehow.  OK, sounds good.

Comment 28

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

(In reply to Boris Zbarsky [:bz] from comment #23)
> >+++ b/dom/bindings/test/test_iterable.html
> >+           // Properties are somewhere up the proto chain, hasOwnProperty won't
> 
> Yeah, but you should check the class string of both obj and owner.  Would
> have caught the WrapObject thing.

So I'm not exactly sure what the test you're describing here should look like, can you give me a bit more detail?

Comment 29

[Boris Zbarsky [:bzbarsky]]

Hmm, that's because I was totally looking at the wrong object.  What you want is something like this, after the "key_itr = itr.keys()" bit in the test:

  is(Object.prototype.toString.call(key_itr), 
     "[object TestInterfaceIterableSingleIterator]",
     "iterator should have the right brand");
  // Not actually correct so far; should be "TestInterfaceIterableSingleIterator".
  is(Object.prototype.toString.call(Object.getPrototypeOf(key_itr)),
     "[object TestInterfaceIterableSingleIteratorPrototype]",
     "iterator prototype should have the right brand");

Comment 30

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

Attachment: Patch 2 (v5) - WebIDL Parser Changes and Tests for WebIDL Iterable; r=bz`

Fixes for review comments.

Comment 31

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

Attachment: Patch 2 (v4-v5 Interdiff) - WebIDL Parser Changes and Tests for WebIDL Iterable; r=bz`

Comment 32

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

Attachment: Patch 3 (v5) - Codegen Changes and Binding Class for WebIDL Iterable

Comment 33

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

Attachment: Patch 3 (v4-v5 Interdiff) - Codegen Changes and Binding Class for WebIDL Iterable

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8670995 [details] [diff] [review]
Patch 2 (v4-v5 Interdiff) - WebIDL Parser Changes and Tests for WebIDL Iterable; r=bz`

>+        implement underscore prefixed convenience functions would otherwise not

"which would".  Yes, I know, preexisting...

>+        - affectsNothing means that nothing changes due to this method, which
>+          affects caching behavior.

No, affects JIT optimization behavior.  

>+        # If we have an interfaces that are iterable, create their

s/an interfaces/interfaces/

>+        # iterator interfaces and add it to the productions array.

"and add them"

>+            for m in iface.members:
>+                if isinstance(m, IDLIterable):

Can't we use iface.maplikeOrSetlikeOrIterable here?  Or is that not set up until we finish() iface?  If so, please document that.  And I guess we can't do this after we finish() the interfaces because we want to insert an implements statement....

I'd be curious as to how this performs in practice; do you notice a difference between the time it takes to "mach build export" in dom/bindings with/without this patch?

>--- a/dom/bindings/parser/tests/test_interface_maplikesetlike.py
>+++ /dev/null

No, you really do need to use hg mv.  Please make sure this happens correctly when you actually go to check in.  In the meantime, it makes reviewing changes to the test rather hard.  :(

r=me, but I'd like to see an updated version of this patch that has actually reviewable changes to the test...  For now I'm taking it on faith that the test changes are reasonable.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8671011 [details] [diff] [review]
Patch 3 (v4-v5 Interdiff) - Codegen Changes and Binding Class for WebIDL Iterable

Gah.  Yet another delete/readd instead of rename leading to a huge diff.  Could you please do an interdiff like this but with a rename instead?

Comment 36

[Boris Zbarsky [:bzbarsky]]

Or at least diff with -M if you're using git?

Comment 37

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

Attachment: Patch 3 (v4-v5 Interdiff with rename fixes) - Codegen Changes and Binding Class for WebIDL Iterable

Comment 38

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

Attachment: Patch 1 (v1) - Rename Maplike/Setlike WebIDL Parser tests to include Iterable; r=bz

Comment 39

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

Attachment: Patch 2 (v6; v5 with rename fix) - WebIDL Parser Changes and Tests for WebIDL Iterable; r=bz`

Ok, This should make looking at the testing changes for the parser easier.

Comment 40

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

Attachment: Patch 3 (v5 - Renames) - Renaming of Maplike/Setlike test WebIDL interface to include Iterable

Comment 41

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

Attachment: Patch 3 (v6; v5 with rename fixes) - Codegen Changes and Binding Class for WebIDL Iterable

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8672783 [details] [diff] [review]
Patch 3 (v4-v5 Interdiff with rename fixes) - Codegen Changes and Binding Class for WebIDL Iterable

>+++ b/dom/bindings/Codegen.py
>@@ -15470,26 +15473,11 @@ class CGIterableMethodGenerator(CGThing):
>+            nsRefPtr<itrType> result(new itrType(self, itrType::IterableIteratorType::${itrMethod}, &${nativeType}IteratorBinding::Wrap));
>+            """, nativeType=descriptor.nativeType, itrMethod=methodName.title())))

Please line-wrap the C++ bit, probably after ${itrMethod}, so the generated code has a hope of fitting in 80 chars.  I would also prefer the substitutions to come one per line, starting on the line after the closing """.

Also, looking at this class now, it seems like it would be simpler to have it just inherit from CGGeneric and then in __init__ compute the relevant string and pass it to CGGeneric.__init__.  You can then remove the define() implementation too.

Finally, ${nativeType}IteratorBiding::Wrap is wrong if substituted with descriptor.nativeType.  For simplicity, consider the case of making Document iterable, where nativeType is "nsIDocument".

What you want here, I think, is &${ifaceName}IteratorBinding::Wrap, and descriptor.interface.identifier.name in the substitution.

>+++ b/dom/bindings/Configuration.py
>+            # Setting up descriptorsByName while iterating through interfaces
>+            # means we can get the nativeType of iterable iterfaces without
>+            # having to do multiple loops.

s/iterfaces/interfaces/.

So this is only true because the generated iterator interfaces come after their iterable interfaces  in parseData, right?  I can live with that as long as we document that dependency in the place where we add the iterator interfaces.

>@@ -349,13 +353,13 @@ class Descriptor(DescriptorProvider):
>+            nativeTypeDefault = ("mozilla::dom::IterableIterator<%s>" %
>+                                 (self.getDescriptor(self.interface.iterableInterface.identifier.name)).nativeType)

Is this actually within 80 chars?  If not, it might be worth factoring out the "iterable interface name" computation into a separate line....

>+++ b/dom/bindings/IterableIterator.h
>+/*

I think you want two * there on the doc comment to get mxr to pick it up.

So now that I look at this API contract, it seems like we're optimizing for the two-type iterator case, sort of.  Maybe file a followup to improve things so in the single-type iterator case both keys and values call GetValueAtIndex?  It might take a bit of thought about how to set that up without forcing the callee to have a GetKeyAtIndex defined in the single-type case, but I think it would be a bit nicer for iterable implementors.  I'm pretty happy to take a stab at that followup myself if you'd rather not deal with it.

>+  typedef bool (*WrapFunc)(JSContext* aCx, mozilla::dom::IterableIterator<T>* aObject, JS::Handle<JSObject*> aGivenProto, JS::MutableHandle<JSObject*> aReflector);

80 cols?  I recommend on line per argument here.  ;)

>+  template <typename DictType>
>+  void
>+  DictReturn(JSContext* aCx, JS::MutableHandle<JSObject*> aResult,

Why template?  This is only ever instantiated with DictType == IterableKeyOrValueResult, no?

>+  Next(JSContext* aCx, JS::MutableHandle<JSObject*> aResult, ErrorResult& aRv)
>+      dict.mValue.AppendElement(key, mozilla::fallible);
>+      dict.mValue.AppendElement(value, mozilla::fallible);

If you're going to do a fallible append, you better check the return value!

But I think these can just be infallible appends too.  Either way, as long as we don't have unchecked fallible appends.

>   nsRefPtr<T> mIterableObj;

As discussed on IRC, this does mean you need CC after all.  :(

r=me with those fixed, though it's probably a good idea to have me look over the CC impl you end up with.

Comment 43

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

Attachment: Rollup Patch for speed testing

Comment 44

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

(In reply to Boris Zbarsky [:bz] from comment #42)
> >+  template <typename DictType>
> >+  void
> >+  DictReturn(JSContext* aCx, JS::MutableHandle<JSObject*> aResult,
> 
> Why template?  This is only ever instantiated with DictType ==
> IterableKeyOrValueResult, no?

Nope, there's IterableKeyOrValueResult, and IterableKeyAndValueResult, depending on whether we're creating something for key()/value() or entries().

Comment 45

[Boris Zbarsky [:bzbarsky]]

> Nope, there's IterableKeyOrValueResult, and IterableKeyAndValueResult

Where do you use DictReturn with IterableKeyAndValueResult?

Comment 46

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

Oh, forgot I stopped calling DictReturn for key/value pair in entries 'cause it was too messy to generalize. Nevermind.

Comment 47

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

(In reply to Boris Zbarsky [:bz] from comment #42)

> >+  Next(JSContext* aCx, JS::MutableHandle<JSObject*> aResult, ErrorResult& aRv)
> >+      dict.mValue.AppendElement(key, mozilla::fallible);
> >+      dict.mValue.AppendElement(value, mozilla::fallible);
> 
> If you're going to do a fallible append, you better check the return value!
> 
> But I think these can just be infallible appends too.  Either way, as long
> as we don't have unchecked fallible appends.

So these are AppendElements on a Sequence, which inherits from FallibleTArray, which is why they're fallible. Would you recommend casting back to TArray for the infallible case? Right now I'm just adding return value checking and throwing if things don't work, just curious about your thoughts otherwise.

Comment 48

[Boris Zbarsky [:bzbarsky]]

> Right now I'm just adding return value checking and throwing if things don't work

Sounds good to me.

Comment 49

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

Attachment: Patch 2 (v5-v6 Interdiff) - WebIDL Parser Changes and Tests for WebIDL Iterable; r=bz`

Updated comments addressed in reviews, and tried to fix some line lengths.

Comment 50

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

Attachment: Patch 2 (v6) - WebIDL Parser Changes and Tests for WebIDL Iterable; r=bz`

Comment 51

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

Attachment: Patch 3 (v6-v7 Interdiff) - Codegen Changes and Binding Class for WebIDL Iterable

Added cycle collection, addressed review comments.

Comment 52

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

Attachment: Patch 3 (v6-v7 Interdiff) - Codegen Changes and Binding Class for WebIDL Iterable

Cycle collection, review comments (last patch was missing removal of template on DictReturn)

Comment 53

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

Attachment: Patch 3 (v6-v7 Interdiff) - Codegen Changes and Binding Class for WebIDL Iterable (

Ok. ACTUALLY the final diff now.

Comment 54

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

Attachment: Patch 3 (v7) - Codegen Changes and Binding Class for WebIDL Iterable

Comment 55

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8673783 [details] [diff] [review]
Patch 3 (v6-v7 Interdiff) - Codegen Changes and Binding Class for WebIDL Iterable (

You still need to address the "Is this actually within 80 chars?" comment in Configuration.py.

>+++ b/dom/bindings/IterableIterator.cpp
>+// Since this is used as the base of a templated class, we can't use the default
>+// wrapper cache CC here.

Luckily, you don't need the wrapper cache CC, nor any of the script holder class stuff, right?  Just straight CC.  So no need for the NS_IMPL_CYCLE_COLLECTION_TRACE* stuff, no need for NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS, no need for NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS (should just be NS_DECL_CYCLE_COLLECTION_CLASS).

The comment should probably say something about how the normal CC macros can't be used for our templated subclasses because they really want to go in a .cpp file but the template stuff needs to live in the header, so we have to implement CC in the superclass and add manual traverse/unlink hooks instead.

>+++ b/dom/bindings/IterableIterator.h
>+  virtual void UnlinkHelper() = 0;
>+  virtual void TraverseHelper(nsCycleCollectionTraversalCallback& cb) = 0;

These should be protected, right?

>+      if (!dict.mValue.AppendElement(key, mozilla::fallible)) {
>+        aRv.Throw(NS_ERROR_FAILURE);

NS_ERROR_OUT_OF_MEMORY, both places.

>+  virtual void UnlinkHelper() override

Can this be final, not just override?

>+    if (mIterableObj) {
>+      mIterableObj = nullptr;

Don't bother nullchecking.  Just set to null.

r=me

Comment 56

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

Attachment: Patch 3 (v8) - Codegen Changes and Binding Class for WebIDL Iterable; r=bz

Addressed review comments

Comment 57

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

Attachment: Patch 3 (v7-v8 Interdiff) - Codegen Changes and Binding Class for WebIDL Iterable

Addressed review comments

Comment 58

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91e9d59af87e
https://hg.mozilla.org/integration/mozilla-inbound/rev/96acfd0d21cf

Comment 59

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15104d2224f4

Comment 60

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

You've also got https://treeherder.mozilla.org/logviewer.html#?job_id=15650656&repo=mozilla-inbound on android.

Comment 61

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

My fault for forgetting to uncomment the pref'ing for test interfaces. Fixed.

Comment 62

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

Ok, it looks like the build breakage is due to some odd behavior in Codegen that I need a grownup to help with. 

Basically, when we build debug, no IterableIteratorBinding class is generated in IterableIteratorBinding.h, we just get the dictionary classes. However, when we build release, we generate an IterableIteratorBinding class, which tries to do a 

typedef IterableIterator NativeType;

Which doesn't work because IterableIterator is templated. This plus the forward declaration of class IterableIterator there is what's causing the build problems on both m-i and try. 

I'm honestly not sure why the class is generated in one build type but not in the other.

Comment 63

[Boris Zbarsky [:bzbarsky]]

> I'm honestly not sure why the class is generated in one build type but not in the other.

Oh, this is rather fun.  ;)

So in Configuration.py we have this bit:

            if iface.identifier.name not in config:
                # Completely skip consequential interfaces with no descriptor
                # if they have no interface object because chances are we
                # don't need to do anything interesting with them.
                if iface.isConsequential() and not iface.hasInterfaceObject():
                    self.optimizedOutDescriptorNames.add(iface.identifier.name)
                    continue

In a debug build, IterableIterator takes this codepath, because it's used on the RHS of "implements" (by the test iterable bindings), so isConsequential().  So we don't even create a descriptor for it.

In an opt build, so far, there are no iterable interfaces, so we don't take this codepath.  The confusion between interfaces and mixins strikes again.

I think your path of least resistance for now is to put this:

    'IterableIterator' : {
        'skipGen': True,
    },

in Bindings.conf, with a comment about how we can remove it once we have an actual non-test iterable interface.

Comment 64

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c58a3da47df4
https://hg.mozilla.org/integration/mozilla-inbound/rev/aafad06e5b40

Comment 65

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

Attachment: FINAL PATCH 1 - File Renames for WebIDL Iterable Implementation

Comment 66

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

Attachment: FINAL PATCH 2 - WebIDL Iterable Implementation and Tests; r=bz

Comment 67

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c58a3da47df4
https://hg.mozilla.org/mozilla-central/rev/aafad06e5b40