Closed Bug 1085293 Opened 5 years ago Closed 4 years ago

WebIDL codegen should support iterable<> members

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: baku, Assigned: qdot)

References

(Blocks 1 open bug)

Details

Attachments

(8 files, 23 obsolete files)

835 bytes, patch
Details | Diff | Splinter Review
780 bytes, patch
Details | Diff | Splinter Review
4.81 KB, patch
Details | Diff | Splinter Review
41.19 KB, patch
Details | Diff | Splinter Review
47.65 KB, patch
Details | Diff | Splinter Review
4.13 KB, patch
Details | Diff | Splinter Review
1.26 KB, patch
Details | Diff | Splinter Review
88.83 KB, patch
Details | Diff | Splinter Review
No description provided.
Blocks: 1085284
Ehsan, this is the bug I mentioned yesterday.
Flags: needinfo?(ehsan.akhgari)
Assignee: nobody → ehsan.akhgari
Flags: needinfo?(ehsan.akhgari)
Andrea, what's the timeframe you need this in?
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(amarchesini)
Duplicate of this bug: 1141301
There's some stuff in bug 1123516 that might be helpful once this starts up.
Depends on: 1123516
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
Assignee: ehsan → kyle
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.
Status: NEW → ASSIGNED
Attachment #8662143 - Flags: review?(bzbarsky)
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.
Attachment #8662144 - Attachment is obsolete: true
Attachment #8663029 - Flags: review?(bzbarsky)
Collapsed test patch down into implementation patch
Attachment #8662146 - Attachment is obsolete: true
Attachment #8663154 - Attachment is obsolete: true
Attachment #8663154 - Flags: review?(bzbarsky)
Attachment #8663157 - Flags: review?(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 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?
Attachment #8662143 - Flags: review?(bzbarsky) → review-
(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.
Attachment #8662143 - Attachment is obsolete: true
Attachment #8663029 - Attachment is obsolete: true
Attachment #8663029 - Flags: review?(bzbarsky)
Attachment #8667006 - Flags: review?(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.
Attachment #8667006 - Flags: review?(bzbarsky) → review+
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).
Attachment #8663157 - Flags: review?(bzbarsky) → review-
(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?
Flags: needinfo?(bzbarsky)
Inheriting nsISupports does not automatically require CC.
Flags: needinfo?(bzbarsky)
(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)
Flags: needinfo?(bzbarsky)
Huh, right you are. I was misreading this somehow.  OK, sounds good.
Flags: needinfo?(bzbarsky)
(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?
Flags: needinfo?(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");
Flags: needinfo?(bzbarsky)
Fixes for review comments.
Attachment #8667006 - Attachment is obsolete: true
Attachment #8663157 - Attachment is obsolete: true
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.
Attachment #8670995 - Flags: review?(bzbarsky) → review+
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?
Attachment #8671011 - Flags: review?(bzbarsky)
Attachment #8671007 - Flags: review?(bzbarsky)
Or at least diff with -M if you're using git?
Flags: needinfo?(kyle)
Attachment #8671011 - Attachment is obsolete: true
Flags: needinfo?(kyle)
Attachment #8672783 - Flags: review?(bzbarsky)
Ok, This should make looking at the testing changes for the parser easier.
Attachment #8670993 - Attachment is obsolete: true
Attachment #8670995 - Attachment is obsolete: true
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.
Attachment #8672783 - Flags: review?(bzbarsky) → review+
(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().
> Nope, there's IterableKeyOrValueResult, and IterableKeyAndValueResult

Where do you use DictReturn with IterableKeyAndValueResult?
Flags: needinfo?(kyle)
Oh, forgot I stopped calling DictReturn for key/value pair in entries 'cause it was too messy to generalize. Nevermind.
Flags: needinfo?(kyle)
(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.
> Right now I'm just adding return value checking and throwing if things don't work

Sounds good to me.
Updated comments addressed in reviews, and tried to fix some line lengths.
Added cycle collection, addressed review comments.
Attachment #8672783 - Attachment is obsolete: true
Attachment #8672798 - Attachment is obsolete: true
Attachment #8673193 - Attachment is obsolete: true
Attachment #8673781 - Flags: review?(bzbarsky)
Cycle collection, review comments (last patch was missing removal of template on DictReturn)
Attachment #8673781 - Attachment is obsolete: true
Attachment #8673781 - Flags: review?(bzbarsky)
Attachment #8673782 - Flags: review?(bzbarsky)
Ok. ACTUALLY the final diff now.
Attachment #8673782 - Attachment is obsolete: true
Attachment #8673782 - Flags: review?(bzbarsky)
Attachment #8673783 - Flags: review?(bzbarsky)
Attachment #8673783 - Attachment description: 0007-Codegen-Changes.patch → Patch 3 (v6-v7 Interdiff) - Codegen Changes and Binding Class for WebIDL Iterable (
Attachment #8672796 - Attachment is obsolete: false
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
Attachment #8673783 - Flags: review?(bzbarsky) → review+
Addressed review comments
Attachment #8673783 - Attachment is obsolete: true
Attachment #8673787 - Attachment is obsolete: true
My fault for forgetting to uncomment the pref'ing for test interfaces. Fixed.
Flags: needinfo?(kyle)
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.
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/mozilla-central/rev/c58a3da47df4
https://hg.mozilla.org/mozilla-central/rev/aafad06e5b40
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
No longer blocks: 1216348
Depends on: 1216348
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.