Bug 159296 - DOM value iterable interfaces should use Array prototype methods
Summary: DOM value iterable interfaces should use Array prototype methods
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL: https://heycam.github.io/webidl/#es-i...
Keywords:
Depends on:
Blocks: 159528
  Show dependency treegraph
 
Reported: 2016-06-30 05:12 PDT by youenn fablet
Modified: 2016-07-14 04:17 PDT (History)
7 users (show)

See Also:


Attachments
Patch (52.70 KB, patch)
2016-06-30 05:23 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing and adding chrome test (60.08 KB, patch)
2016-07-01 05:12 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Beefing up log and using private slots approach (67.66 KB, patch)
2016-07-07 15:16 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (70.88 KB, patch)
2016-07-12 01:34 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (71.14 KB, patch)
2016-07-13 02:32 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (72.60 KB, patch)
2016-07-14 00:17 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-cq-02 for mac-yosemite (721.37 KB, application/zip)
2016-07-14 01:09 PDT, WebKit Commit Bot
no flags Details
Patch for landing (71.25 KB, patch)
2016-07-14 01:46 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-06-30 05:12:19 PDT
Value iterators should be using Array prototype methods (values, entries, keys, Symbol.Iterator, forEach)
Comment 1 youenn fablet 2016-06-30 05:23:31 PDT
Created attachment 282432 [details]
Patch
Comment 2 youenn fablet 2016-07-01 05:12:10 PDT
Created attachment 282538 [details]
Rebasing and adding chrome test
Comment 3 Chris Dumez 2016-07-01 09:24:02 PDT
Adding some JSC folks. The relevant spec seems to be at:
https://heycam.github.io/webidl/#es-iterable
Comment 4 youenn fablet 2016-07-07 08:31:33 PDT
Ping review?

(In reply to comment #3)
> Adding some JSC folks. The relevant spec seems to be at:
> https://heycam.github.io/webidl/#es-iterable

Right, in particular the lines:
//Entries
If the interface has a value iterator, then the Function object is the initial value of the “entries” data property of %ArrayPrototype% ([ECMA-262], section 6.1.7.4).
//Keys
If the interface has a value iterator, then the Function object is the initial value of the “keys” data property of %ArrayPrototype% ([ECMA-262], section 6.1.7.4).
...

The only downside I see here is that this path is making use of the "current value" of the prototype not, the "initial" one.
A follow-up patch may fix this for instance by adding private properties to the array prototype that would be used to populate value-iterable interfaces.
Comment 5 Mark Lam 2016-07-07 10:27:23 PDT
Comment on attachment 282538 [details]
Rebasing and adding chrome test

View in context: https://bugs.webkit.org/attachment.cgi?id=282538&action=review

> Source/WebCore/ChangeLog:14
> +        For value iterators, copy the iterator methods from Array prototype.
> +        This change applies only to NodeList at the moment.
> +        Copy of Array prototype iterator methods is disabled if the interface has no indexed getter.
> +

Please quote the spec justification after this for easy future reference.  Specifically:

According to https://heycam.github.io/webidl/#es-iterable,
[re: Entries] If the interface has a value iterator, then the Function object is the initial value of the “entries” data property of %ArrayPrototype% ([ECMA-262], section 6.1.7.4).
[re: Keys] If the interface has a value iterator, then the Function object is the initial value of the “keys” data property of %ArrayPrototype% ([ECMA-262], section 6.1.7.4).

> Source/WebCore/bindings/js/JSDOMIterator.cpp:38
> +static inline void copyProperty(JSC::ExecState& state, JSC::JSObject& from, JSC::JSObject& to, const JSC::Identifier& identifier, unsigned attributes = 0)
> +{
> +    JSC::JSValue value = from.get(&state, identifier);
> +    ASSERT(value);
> +    to.putDirect(state.vm(), identifier, value, attributes);
> +}

Since this function is only used as a convenience function inside addValueIterableMethods(), you can implement it as a lambda in addValueIterableMethods() like so:

    JSC::VM& vm = state->vm();
    auto copyProperty = [&] (const JSC::Identifier& identifier, unsigned attributes = 0) {
        JSC::JSValue = arrayPrototype->get(state, identifier);
        ASSERT(value);
        prototype.putDirect(vm, identifier, value, attributes);
    };

... and then call it like so:

    copyProperty(JSC::Identifier::fromString(state, "entries"));

Using a lambda here reduces the verbosity of the code as well as communicates that this function is bounded in scope to addValueIterableMethods().

> Source/WebCore/bindings/js/JSDOMIterator.cpp:52
> +    JSC::ExecState* state = globalObject.globalExec();
> +    ASSERT(state);
> +
> +    copyProperty(*state, *arrayPrototype, prototype, JSC::Identifier::fromString(state, "entries"));
> +    copyProperty(*state, *arrayPrototype, prototype, JSC::Identifier::fromString(state, "keys"));
> +    copyProperty(*state, *arrayPrototype, prototype, JSC::Identifier::fromString(state, "values"));
> +    copyProperty(*state, *arrayPrototype, prototype, JSC::Identifier::fromString(state, "forEach"));
> +    copyProperty(*state, *arrayPrototype, prototype, vm.propertyNames->iteratorSymbol, JSC::DontEnum);

In https://bugs.webkit.org/show_bug.cgi?id=159296#c4, you said:
The only downside I see here is that this path is making use of the "current value" of the prototype not, the "initial" one.
A follow-up patch may fix this for instance by adding private properties to the array prototype that would be used to populate value-iterable interfaces.

I think you can address that right now by doing the following:

1. In JSDOMGlobalObject, add WriteBarrier<JSFunction> fields for entries, keys, values , forEach, and iteratorSymbol.
2. In JSDOMGlobalObject::finishCreation(), copy those values from the ArrayPrototype.
3. In JSDOMGlobalObject::visitChildren(), visit those newly added fields.  See JSGlobalObject::visitChildren() for an example of how to do it.
4. In addValueIterableMethods(), copy the values from JSDOMGlobalObject's fields.

What do you think?
Comment 6 youenn fablet 2016-07-07 11:49:15 PDT
Thanks for the review.

(In reply to comment #5)
> Comment on attachment 282538 [details]
> Rebasing and adding chrome test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282538&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        For value iterators, copy the iterator methods from Array prototype.
> > +        This change applies only to NodeList at the moment.
> > +        Copy of Array prototype iterator methods is disabled if the interface has no indexed getter.
> > +
> 
> Please quote the spec justification after this for easy future reference. 
> Specifically:
> 
> According to https://heycam.github.io/webidl/#es-iterable,
> [re: Entries] If the interface has a value iterator, then the Function
> object is the initial value of the “entries” data property of
> %ArrayPrototype% ([ECMA-262], section 6.1.7.4).
> [re: Keys] If the interface has a value iterator, then the Function object
> is the initial value of the “keys” data property of %ArrayPrototype%
> ([ECMA-262], section 6.1.7.4).

OK

> 
> > Source/WebCore/bindings/js/JSDOMIterator.cpp:38
> > +static inline void copyProperty(JSC::ExecState& state, JSC::JSObject& from, JSC::JSObject& to, const JSC::Identifier& identifier, unsigned attributes = 0)
> > +{
> > +    JSC::JSValue value = from.get(&state, identifier);
> > +    ASSERT(value);
> > +    to.putDirect(state.vm(), identifier, value, attributes);
> > +}
> 
> Since this function is only used as a convenience function inside
> addValueIterableMethods(), you can implement it as a lambda in
> addValueIterableMethods() like so:
> 
>     JSC::VM& vm = state->vm();
>     auto copyProperty = [&] (const JSC::Identifier& identifier, unsigned
> attributes = 0) {
>         JSC::JSValue = arrayPrototype->get(state, identifier);
>         ASSERT(value);
>         prototype.putDirect(vm, identifier, value, attributes);
>     };
> 
> ... and then call it like so:
> 
>     copyProperty(JSC::Identifier::fromString(state, "entries"));
> 
> Using a lambda here reduces the verbosity of the code as well as
> communicates that this function is bounded in scope to
> addValueIterableMethods().

OK.
I was wondering whether there was not already a routine like this somewhere.

> > Source/WebCore/bindings/js/JSDOMIterator.cpp:52
> > +    JSC::ExecState* state = globalObject.globalExec();
> > +    ASSERT(state);
> > +
> > +    copyProperty(*state, *arrayPrototype, prototype, JSC::Identifier::fromString(state, "entries"));
> > +    copyProperty(*state, *arrayPrototype, prototype, JSC::Identifier::fromString(state, "keys"));
> > +    copyProperty(*state, *arrayPrototype, prototype, JSC::Identifier::fromString(state, "values"));
> > +    copyProperty(*state, *arrayPrototype, prototype, JSC::Identifier::fromString(state, "forEach"));
> > +    copyProperty(*state, *arrayPrototype, prototype, vm.propertyNames->iteratorSymbol, JSC::DontEnum);
> 
> In https://bugs.webkit.org/show_bug.cgi?id=159296#c4, you said:
> The only downside I see here is that this path is making use of the "current
> value" of the prototype not, the "initial" one.
> A follow-up patch may fix this for instance by adding private properties to
> the array prototype that would be used to populate value-iterable interfaces.
> 
> I think you can address that right now by doing the following:
> 
> 1. In JSDOMGlobalObject, add WriteBarrier<JSFunction> fields for entries,
> keys, values , forEach, and iteratorSymbol.
> 2. In JSDOMGlobalObject::finishCreation(), copy those values from the
> ArrayPrototype.
> 3. In JSDOMGlobalObject::visitChildren(), visit those newly added fields. 
> See JSGlobalObject::visitChildren() for an example of how to do it.
> 4. In addValueIterableMethods(), copy the values from JSDOMGlobalObject's
> fields.
> 
> What do you think?

Sure, this can be done this way.
Having these functions as private slots has some benefits though:
- Less boiler plate code in JSDOMGlobalObject
- Access to these functions from JS built-in code
For instance, @Array.@forEach would have been/would be handy for FetchHeaders.js builtin code.

I can update the patch either way.
Comment 7 Mark Lam 2016-07-07 12:03:06 PDT
(In reply to comment #6)
> Having these functions as private slots has some benefits though:
> - Less boiler plate code in JSDOMGlobalObject
> - Access to these functions from JS built-in code
> For instance, @Array.@forEach would have been/would be handy for
> FetchHeaders.js builtin code.

That sounds good too.
Comment 8 youenn fablet 2016-07-07 15:16:16 PDT
Created attachment 283063 [details]
Beefing up log and using private slots approach
Comment 9 youenn fablet 2016-07-07 15:17:24 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > Having these functions as private slots has some benefits though:
> > - Less boiler plate code in JSDOMGlobalObject
> > - Access to these functions from JS built-in code
> > For instance, @Array.@forEach would have been/would be handy for
> > FetchHeaders.js builtin code.
> 
> That sounds good too.

Proposed patch follows that strategy.
I also beefed up a bit the test to check that.
Comment 10 WebKit Commit Bot 2016-07-07 15:18:28 PDT
Attachment 283063 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ArrayPrototype.cpp:151:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 youenn fablet 2016-07-07 15:35:30 PDT
> For instance, @Array.@forEach would have been/would be handy for
> FetchHeaders.js builtin code.

See bug 159528 for a potential use of @forEach.
Comment 12 Mark Lam 2016-07-11 13:25:14 PDT
Comment on attachment 283063 [details]
Beefing up log and using private slots approach

View in context: https://bugs.webkit.org/attachment.cgi?id=283063&action=review

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:124
> +    putDirectWithoutTransition(vm, vm.propertyNames->builtinNames().entriesPrivateName(), get(globalObject->globalExec(),  vm.propertyNames->builtinNames().entriesPublicName()), ReadOnly);
> +    putDirectWithoutTransition(vm, vm.propertyNames->builtinNames().forEachPrivateName(), get(globalObject->globalExec(),  vm.propertyNames->builtinNames().forEachPublicName()), ReadOnly);
> +    putDirectWithoutTransition(vm, vm.propertyNames->builtinNames().keysPrivateName(), get(globalObject->globalExec(),  vm.propertyNames->builtinNames().keysPublicName()), ReadOnly);

Since we know that we putDirectWithoutTransition() the original values above, can you just getDirect the values instead of going thru get() which does a lot more work?

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:162
> +void ArrayPrototype::copyIterableMethods(ExecState& state, JSObject& prototype)
> +{
> +    VM& vm = state.vm();
> +    auto copyProperty = [&] (const Identifier& arrayIdentifier, const Identifier& otherIdentifier, unsigned attributes = 0) {
> +        JSValue value = get(&state, arrayIdentifier);
> +        ASSERT(value);
> +        prototype.putDirect(vm, otherIdentifier, value, attributes);
> +    };
> +
> +    copyProperty(vm.propertyNames->builtinNames().entriesPrivateName(), vm.propertyNames->builtinNames().entriesPublicName());
> +    copyProperty(vm.propertyNames->builtinNames().forEachPrivateName(), vm.propertyNames->builtinNames().forEachPublicName());
> +    copyProperty(vm.propertyNames->builtinNames().keysPrivateName(), vm.propertyNames->builtinNames().keysPublicName());
> +    copyProperty(vm.propertyNames->builtinNames().valuesPrivateName(), vm.propertyNames->builtinNames().valuesPublicName());
> +    copyProperty(vm.propertyNames->builtinNames().valuesPrivateName(), vm.propertyNames->builtinNames().iteratorSymbol(), DontEnum);
> +}

Is there a reason for copyIterableMethods() in ArrayPrototype instead of inlining it in JSDOMIterator's addValueIterableMethods()?  It seems to me that the properties to be copied is defined by JSDOMIterator and not by ArrayPrototype.  Hence, I think belongs in JSDOMIterator than here.
Comment 13 youenn fablet 2016-07-11 13:31:37 PDT
(In reply to comment #12)
> Comment on attachment 283063 [details]
> Beefing up log and using private slots approach
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283063&action=review
> 
> > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:124
> > +    putDirectWithoutTransition(vm, vm.propertyNames->builtinNames().entriesPrivateName(), get(globalObject->globalExec(),  vm.propertyNames->builtinNames().entriesPublicName()), ReadOnly);
> > +    putDirectWithoutTransition(vm, vm.propertyNames->builtinNames().forEachPrivateName(), get(globalObject->globalExec(),  vm.propertyNames->builtinNames().forEachPublicName()), ReadOnly);
> > +    putDirectWithoutTransition(vm, vm.propertyNames->builtinNames().keysPrivateName(), get(globalObject->globalExec(),  vm.propertyNames->builtinNames().keysPublicName()), ReadOnly);
> 
> Since we know that we putDirectWithoutTransition() the original values
> above, can you just getDirect the values instead of going thru get() which
> does a lot more work?

OK, I'll fix that

> > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:162
> > +void ArrayPrototype::copyIterableMethods(ExecState& state, JSObject& prototype)
> > +{
> > +    VM& vm = state.vm();
> > +    auto copyProperty = [&] (const Identifier& arrayIdentifier, const Identifier& otherIdentifier, unsigned attributes = 0) {
> > +        JSValue value = get(&state, arrayIdentifier);
> > +        ASSERT(value);
> > +        prototype.putDirect(vm, otherIdentifier, value, attributes);
> > +    };
> > +
> > +    copyProperty(vm.propertyNames->builtinNames().entriesPrivateName(), vm.propertyNames->builtinNames().entriesPublicName());
> > +    copyProperty(vm.propertyNames->builtinNames().forEachPrivateName(), vm.propertyNames->builtinNames().forEachPublicName());
> > +    copyProperty(vm.propertyNames->builtinNames().keysPrivateName(), vm.propertyNames->builtinNames().keysPublicName());
> > +    copyProperty(vm.propertyNames->builtinNames().valuesPrivateName(), vm.propertyNames->builtinNames().valuesPublicName());
> > +    copyProperty(vm.propertyNames->builtinNames().valuesPrivateName(), vm.propertyNames->builtinNames().iteratorSymbol(), DontEnum);
> > +}
> 
> Is there a reason for copyIterableMethods() in ArrayPrototype instead of
> inlining it in JSDOMIterator's addValueIterableMethods()?  It seems to me
> that the properties to be copied is defined by JSDOMIterator and not by
> ArrayPrototype.  Hence, I think belongs in JSDOMIterator than here.

I agree it belongs naturally to WebCore/JSDOMIterator.
The issue is that we would need to get access to JSC private names from WebCore.
This would require marking a bunch of header files as private plus adding the related WebCore proxy header files.
Comment 14 Mark Lam 2016-07-11 13:35:08 PDT
(In reply to comment #13)
> I agree it belongs naturally to WebCore/JSDOMIterator.
> The issue is that we would need to get access to JSC private names from
> WebCore.
> This would require marking a bunch of header files as private plus adding
> the related WebCore proxy header files.

Can you check how big a pain this actually is?  I would think that we probably want the JS bindings to be able to access this info.  Based on what you find, we can consider the options.  Thanks.
Comment 15 youenn fablet 2016-07-11 13:46:37 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > I agree it belongs naturally to WebCore/JSDOMIterator.
> > The issue is that we would need to get access to JSC private names from
> > WebCore.
> > This would require marking a bunch of header files as private plus adding
> > the related WebCore proxy header files.
> 
> Can you check how big a pain this actually is?  I would think that we
> probably want the JS bindings to be able to access this info.  Based on what
> you find, we can consider the options.  Thanks.

OK, I will try it tomorrow.
I think it will be like 4 or 5 new forwarding headers, including the generated JSCBuiltins.h
Comment 16 youenn fablet 2016-07-12 01:34:44 PDT
Created attachment 283398 [details]
Patch
Comment 17 WebKit Commit Bot 2016-07-12 01:36:52 PDT
Attachment 283398 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMIterator.cpp:43:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Mark Lam 2016-07-12 10:43:05 PDT
Comment on attachment 283398 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283398&action=review

LGTM (with suggested fixes), but please have someone who is more familiar with the DOM side take a closer look as well.  Thanks.

> Source/WebCore/ChangeLog:14
> +        [re: entries] If the interface has a value iterator, then the Function object is the initial value of the âentriesâ data property of %ArrayPrototype% ([ECMA-262], section 6.1.7.4).
> +        [re: keys] If the interface has a value iterator, then the Function object is the initial value of the âkeysâ data property of %ArrayPrototype% ([ECMA-262], section 6.1.7.4).
> +        [re: forEach] If the interface defines an indexed property getter, then the Function object is the initial value of the âforEachâ data property of %ArrayPrototype% ([ECMA-262], section 6.1.7.4).

Please fix the non-ascii characters around "âentriesâ","âkeysâ", and "âforEachâ".

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4213
> +    # FIXME: We should die if the next check is false. Need first to support setlike directly.

Please create a bug to track this (if one does not already exists) and add the url here.  That way, we can remove this FIXME when the bug is resolved.
Comment 19 Alex Christensen 2016-07-12 11:16:33 PDT
Comment on attachment 283398 [details]
Patch

Chrome and Firefox fail these new tests in different ways.  Are they both wrong?
Comment 20 youenn fablet 2016-07-12 13:06:57 PDT
> Chrome and Firefox fail these new tests in different ways.  Are they both
> wrong?

Firefox is only implementing for-of loop of NodeList, hence the various failures.

Chrome is passing fast/dom/NodeList/nodelist-iterable.html.
It is also passing most of nodelist-iterable.html, especially the bits that check the day-to-day behaviour of NodeList iterable.

The failing checks of nodelist-iterable.html are quite specific.
The first failing test is due to the fact that the function objects are not the same between NodeList and Array.
Although the spec mandates that, it can be seen more or less an implementation detail.

The second failing test in Chrome is the case of an iterator which is done=true after next() being called several time.
An item is then added to the array. Calling next() on the iterator should consistently return done=true.
This is the behaviour of Array and should also be the behaviour of DOM iterators.
WebKit is exhibiting this behaviour with the patch, chrome is not.

So, even though there is not full consistency with other browsers, I think we are safe.
Comment 21 youenn fablet 2016-07-12 13:09:10 PDT
(In reply to comment #18)
> Comment on attachment 283398 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283398&action=review
> 
> LGTM (with suggested fixes), but please have someone who is more familiar
> with the DOM side take a closer look as well.  Thanks.
> 
> > Source/WebCore/ChangeLog:14
> > +        [re: entries] If the interface has a value iterator, then the Function object is the initial value of the âentriesâ data property of %ArrayPrototype% ([ECMA-262], section 6.1.7.4).
> > +        [re: keys] If the interface has a value iterator, then the Function object is the initial value of the âkeysâ data property of %ArrayPrototype% ([ECMA-262], section 6.1.7.4).
> > +        [re: forEach] If the interface defines an indexed property getter, then the Function object is the initial value of the âforEachâ data property of %ArrayPrototype% ([ECMA-262], section 6.1.7.4).
> 
> Please fix the non-ascii characters around "âentriesâ","âkeysâ", and
> "âforEachâ".
OK

> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4213
> > +    # FIXME: We should die if the next check is false. Need first to support setlike directly.
> 
> Please create a bug to track this (if one does not already exists) and add
> the url here.  That way, we can remove this FIXME when the bug is resolved.

OK, bug number is 159140.
Comment 22 Mark Lam 2016-07-12 13:41:54 PDT
Comment on attachment 283398 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283398&action=review

>>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4213
>>> +    # FIXME: We should die if the next check is false. Need first to support setlike directly.
>> 
>> Please create a bug to track this (if one does not already exists) and add the url here.  That way, we can remove this FIXME when the bug is resolved.
> 
> OK, bug number is 159140.

Just to clarify.  Please add the bug url to the comment in the code before landing your patch.  Thanks.
Comment 23 youenn fablet 2016-07-13 02:32:08 PDT
Created attachment 283497 [details]
Patch
Comment 24 youenn fablet 2016-07-13 02:34:15 PDT
(In reply to comment #22)
> Comment on attachment 283398 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283398&action=review
> 
> >>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4213
> >>> +    # FIXME: We should die if the next check is false. Need first to support setlike directly.
> >> 
> >> Please create a bug to track this (if one does not already exists) and add the url here.  That way, we can remove this FIXME when the bug is resolved.
> > 
> > OK, bug number is 159140.
> 
> Just to clarify.  Please add the bug url to the comment in the code before
> landing your patch.  Thanks.

Sure, patch is updated accordingly.
I also tried to clarify a bit the binding generator by introducing IsKeyValueIterableInterface in addition to IsValueIterableInterface
Comment 25 WebKit Commit Bot 2016-07-13 02:34:48 PDT
Attachment 283497 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMIterator.cpp:43:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 Chris Dumez 2016-07-13 12:49:56 PDT
Comment on attachment 283497 [details]
Patch

r=me
Comment 27 youenn fablet 2016-07-14 00:17:01 PDT
Created attachment 283612 [details]
Patch for landing
Comment 28 WebKit Commit Bot 2016-07-14 01:09:29 PDT
Comment on attachment 283612 [details]
Patch for landing

Rejecting attachment 283612 [details] from commit-queue.

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight-referrer.html
Full output: http://webkit-queues.webkit.org/results/1678811
Comment 29 WebKit Commit Bot 2016-07-14 01:09:32 PDT
Created attachment 283623 [details]
Archive of layout-test-results from webkit-cq-02 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-02  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 30 youenn fablet 2016-07-14 01:46:35 PDT
Created attachment 283626 [details]
Patch for landing
Comment 31 WebKit Commit Bot 2016-07-14 04:17:05 PDT
Comment on attachment 283626 [details]
Patch for landing

Clearing flags on attachment: 283626

Committed r203222: <http://trac.webkit.org/changeset/203222>
Comment 32 WebKit Commit Bot 2016-07-14 04:17:12 PDT
All reviewed patches have been landed.  Closing bug.