Introduce ES6 iterator for DOM objects.

Introduce ES6 iterator for DOM objects.

This CL introduces blink::Iterator class and "Iterable" IDL attribute to make
it enable for DOM objects to become iteratable.

BUG=401699
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181294

[yhirano, 2014-08-20 05:58:18]

This CL depends on https://code.google.com/p/v8/source/detail?r=23196 and
https://codereview.chromium.org/459413002/.
I will modify the code generator and remove the custom implementation in the
future.

[haraken, 2014-08-20 06:19:04]

> I will modify the code generator and remove the custom implementation in the
> future.

Do you have a concrete plan for this? Custom wrap()s are error-prone, so we
don't want to increase them if possible.

[yhirano, 2014-08-20 06:29:56]

On 2014/08/20 06:19:04, haraken wrote:
> > I will modify the code generator and remove the custom implementation in the
> > future.
> 
> Do you have a concrete plan for this? Custom wrap()s are error-prone, so we
> don't want to increase them if possible.

I'm planning to introduce an IDL attribute for the feature although I haven't
decided
how to implement it yet. Maybe I will introduce [SymbolKeyed] attribute for a
method-or-attribute or [Iterable] for a class. I'll remove the custom wrapper
before
I use Iterator in actual classes.

[haraken, 2014-08-20 06:37:18]

On 2014/08/20 06:29:56, yhirano wrote:
> On 2014/08/20 06:19:04, haraken wrote:
> > > I will modify the code generator and remove the custom implementation in
the
> > > future.
> > 
> > Do you have a concrete plan for this? Custom wrap()s are error-prone, so we
> > don't want to increase them if possible.
> 
> I'm planning to introduce an IDL attribute for the feature although I haven't
> decided
> how to implement it yet. Maybe I will introduce [SymbolKeyed] attribute for a
> method-or-attribute or [Iterable] for a class. I'll remove the custom wrapper
> before
> I use Iterator in actual classes.

Hmm, I'm never intending to block your work, but if you plan to remove the
custom wrap methods before actual customers start using Iterators, I'd prefer
improving the IDL compiler from the beginning. Adding an IDL attribute is fine
:)

[yhirano, 2014-08-20 06:47:50]

On 2014/08/20 06:37:18, haraken wrote:
> Hmm, I'm never intending to block your work, but if you plan to remove the
> custom wrap methods before actual customers start using Iterators, I'd prefer
> improving the IDL compiler from the beginning. Adding an IDL attribute is fine
> :)
OK, I will update the CL. Please wait for a while...

[arv (Not doing code reviews), 2014-08-20 14:18:38]

haraken knows the Blink side better than I do. Adding him as reviewer.

I agree that custom wrap is bad but I'm OK removing that in another CL since I
find custom wrap easier to understand than codegen.

We should also provide a custom IDL attribute to use
Array.prototype[Symbol.iterator] a the iterator since this is the common case
used by all array like DOM objects.

https://codereview.chromium.org/483163003/diff/20001/Source/core/dom/Iterator...
File Source/core/dom/Iterator.idl (right):

https://codereview.chromium.org/483163003/diff/20001/Source/core/dom/Iterator...
Source/core/dom/Iterator.idl:7: NoInterfaceObject
The iterator prototype also needs a [Symbol.iterator] method that returns this.

https://codereview.chromium.org/483163003/diff/20001/Source/core/dom/Iterator...
Source/core/dom/Iterator.idl:7: NoInterfaceObject
The iterator prototype also needs a [Symbol.iterator] method that returns this.

https://codereview.chromium.org/483163003/diff/20001/Source/core/testing/Inte...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/483163003/diff/20001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:151: virtual ScriptValue next(ScriptState*
scriptState, ExceptionState& exceptionState) OVERRIDE
This looks reasonable but I'm wondering if we should just use C++ iterators
behind the scene and then have a single adaptor class that wraps it?

[abarth-chromium, 2014-08-20 18:22:42]

https://codereview.chromium.org/483163003/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8InternalsCustom.cpp (right):

https://codereview.chromium.org/483163003/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8InternalsCustom.cpp:6: // generator.
Please don't introduce any custom bindings for Internals.

Why are we adding this class anyway?  Can't we just test a real web API that
uses this functionality?

[yhirano, 2014-08-21 11:55:20]

I removed the custom wrapper.
PTAL.

[yhirano, 2014-08-21 11:55:27]

https://codereview.chromium.org/483163003/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8InternalsCustom.cpp (right):

https://codereview.chromium.org/483163003/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8InternalsCustom.cpp:6: // generator.
On 2014/08/20 18:22:42, abarth wrote:
> Please don't introduce any custom bindings for Internals.
> 
> Why are we adding this class anyway?  Can't we just test a real web API that
> uses this functionality?

Removed the custom wrapper.

https://codereview.chromium.org/483163003/diff/20001/Source/core/dom/Iterator...
File Source/core/dom/Iterator.idl (right):

https://codereview.chromium.org/483163003/diff/20001/Source/core/dom/Iterator...
Source/core/dom/Iterator.idl:7: NoInterfaceObject
On 2014/08/20 14:18:38, arv wrote:
> The iterator prototype also needs a [Symbol.iterator] method that returns
this.

Done.

https://codereview.chromium.org/483163003/diff/20001/Source/core/testing/Inte...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/483163003/diff/20001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:151: virtual ScriptValue next(ScriptState*
scriptState, ExceptionState& exceptionState) OVERRIDE
On 2014/08/20 14:18:38, arv wrote:
> This looks reasonable but I'm wondering if we should just use C++ iterators
> behind the scene and then have a single adaptor class that wraps it?

Sorry I don't got your idea, can you explain it in a bit more detail?

[Jens Widell, 2014-08-21 13:08:53]

https://codereview.chromium.org/483163003/diff/80001/Source/bindings/template...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/483163003/diff/80001/Source/bindings/template...
Source/bindings/templates/methods.cpp:467: {% elif internal_namespace %}
You could skip this conditional if |internal_namespace| was either '' (default)
or '::symbol' (from caller) and just make the call

{{cpp_class}}V8Internal{{internal_namespace}}::{{method.name}}Method{{world_suffix}}(info);

OTOH, I'm not entirely sure why we bother with the explicit namespace here at
all; just calling {{method.name}}Method(info) would work too.

[yhirano, 2014-08-21 13:19:58]

https://codereview.chromium.org/483163003/diff/80001/Source/bindings/template...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/483163003/diff/80001/Source/bindings/template...
Source/bindings/templates/methods.cpp:467: {% elif internal_namespace %}
On 2014/08/21 13:08:53, Jens Widell wrote:
> You could skip this conditional if |internal_namespace| was either ''
(default)
> or '::symbol' (from caller) and just make the call
> 
>
{{cpp_class}}V8Internal{{internal_namespace}}::{{method.name}}Method{{world_suffix}}(info);
> 
> OTOH, I'm not entirely sure why we bother with the explicit namespace here at
> all; just calling {{method.name}}Method(info) would work too.

Done.
I introduced a new namespace to avoid name conflict against a string-keyed
method with the same name.
In this case, interfaces can have usual 'iterator' method.

[Jens Widell, 2014-08-21 13:58:02]

https://codereview.chromium.org/483163003/diff/80001/Source/bindings/template...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/483163003/diff/80001/Source/bindings/template...
Source/bindings/templates/methods.cpp:467: {% elif internal_namespace %}
On 2014/08/21 13:19:58, yhirano wrote:
> On 2014/08/21 13:08:53, Jens Widell wrote:
> > You could skip this conditional if |internal_namespace| was either ''
> (default)
> > or '::symbol' (from caller) and just make the call
> > 
> >
>
{{cpp_class}}V8Internal{{internal_namespace}}::{{method.name}}Method{{world_suffix}}(info);
> > 
> > OTOH, I'm not entirely sure why we bother with the explicit namespace here
at
> > all; just calling {{method.name}}Method(info) would work too.
> 
> Done.

Thanks.

> I introduced a new namespace to avoid name conflict against a string-keyed
> method with the same name.
> In this case, interfaces can have usual 'iterator' method.

Yes, adding the namespace is all good.

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.cpp:120:
v8::Handle<v8::FunctionTemplate>
V8DOMConfiguration::functionTemplateForMethod(v8::Handle<v8::Signature>
signature, const SymbolKeyedMethodConfiguration& callback, v8::Isolate* isolate)
This is identical to the other functionTemplateForMethod() except for the type
of the |callback| argument, yes?

Might it be worth-while to have a local template helper, just to avoid the
duplicated code?

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/scripts...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:108:
includes.add('bindings/core/v8/V8Iterator.h')
Is this needed? I thought it was at first, but then I realized I hadn't run
'gclient runhooks' after applying your patch, so Iterator.idl wasn't processed.
Now that I have, this include seems to still be added with this line commented
out.

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:175: 'is_iterable': is_iterable,
I'd slightly prefer if we dropped this and just used {% if
method.iterator_method %} in the templates instead.

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/tests/r...
File Source/bindings/tests/results/V8TestInterface.cpp (right):

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/V8TestInterface.cpp:25: #include
"bindings/tests/v8/V8Iterator.h"
Please help me out: I don't understand why this #include added here.

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/V8TestInterface.cpp:1421: namespace symbol {
Nit: it would be nice with a blank after this line.

[arv (Not doing code reviews), 2014-08-21 15:36:25]

On 2014/08/21 at 11:55:27, yhirano wrote:

>
https://codereview.chromium.org/483163003/diff/20001/Source/core/testing/Inte...
> File Source/core/testing/Internals.cpp (right):
> 
>
https://codereview.chromium.org/483163003/diff/20001/Source/core/testing/Inte...
> Source/core/testing/Internals.cpp:151: virtual ScriptValue next(ScriptState*
scriptState, ExceptionState& exceptionState) OVERRIDE
> On 2014/08/20 14:18:38, arv wrote:
> > This looks reasonable but I'm wondering if we should just use C++ iterators
> > behind the scene and then have a single adaptor class that wraps it?
> 
> Sorry I don't got your idea, can you explain it in a bit more detail?

My point was that, for someone to implement an iterator, exposed to JS they have
to implement next. Instead of doing that it might make sense to allow them to
pass in a C++ iterator (begin(), end() iter++) instead. Not a big deal.

[haraken, 2014-08-21 16:15:59]

The overall approach looks good.

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/templat...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:982: static const
V8DOMConfiguration::SymbolKeyedMethodConfiguration symbolIteratorConfiguration =
{ v8::Symbol::GetIterator,
{{cpp_class}}V8Internal::symbol::iteratorMethodCallback, 0, 0,
V8DOMConfiguration::ExposedToAllScripts };

Don't mix "SymbolKeyedMethodConfiguration" and
"symbolIteratorMethodConfiguration" :)

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:982: static const
V8DOMConfiguration::SymbolKeyedMethodConfiguration symbolIteratorConfiguration =
{ v8::Symbol::GetIterator,
{{cpp_class}}V8Internal::symbol::iteratorMethodCallback, 0, 0,
V8DOMConfiguration::ExposedToAllScripts };

You'll need to support iteratorMethodCallbackForMainWorld as well.

That said, I'm OK to completely drop the support for per-world-bindings for
iterator methods. We've been trying to decrease per-world-bindings and don't
have a plan to support it for new features.

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/tests/r...
File Source/bindings/tests/results/V8TestInterface.cpp (right):

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/V8TestInterface.cpp:1421: namespace symbol {
> I introduced a new namespace to avoid name conflict against a string-keyed
> method with the same name.
> In this case, interfaces can have usual 'iterator' method.

I guess we can just use "iteratorMethod" until it causes actual conflict. (We
already have some builtin getters/setters/methods that might conflict with
actual getters/setters/methods on interfaces.)

I'd prefer not adding extra complexity (i.e., extra namespace) to the IDL
compiler, but this is not a big deal anyway.

https://codereview.chromium.org/483163003/diff/100001/Source/core/testing/Int...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/483163003/diff/100001/Source/core/testing/Int...
Source/core/testing/Internals.cpp:147: class InternalsIterator : public Iterator
{

Add FINAL.

https://codereview.chromium.org/483163003/diff/100001/Source/core/testing/Int...
Source/core/testing/Internals.cpp:160: result->Set(v8String(isolate, "done"),
v8::Boolean::New(isolate, done));

v8::Boolean::New() => v8Boolean()

[yhirano, 2014-08-25 06:08:49]

On 2014/08/21 15:36:25, arv wrote:
> On 2014/08/21 at 11:55:27, yhirano wrote:
> 
> >
>
https://codereview.chromium.org/483163003/diff/20001/Source/core/testing/Inte...
> > File Source/core/testing/Internals.cpp (right):
> > 
> >
>
https://codereview.chromium.org/483163003/diff/20001/Source/core/testing/Inte...
> > Source/core/testing/Internals.cpp:151: virtual ScriptValue next(ScriptState*
> scriptState, ExceptionState& exceptionState) OVERRIDE
> > On 2014/08/20 14:18:38, arv wrote:
> > > This looks reasonable but I'm wondering if we should just use C++
iterators
> > > behind the scene and then have a single adaptor class that wraps it?
> > 
> > Sorry I don't got your idea, can you explain it in a bit more detail?
> 
> My point was that, for someone to implement an iterator, exposed to JS they
have
> to implement next. Instead of doing that it might make sense to allow them to
> pass in a C++ iterator (begin(), end() iter++) instead. Not a big deal.

Imagine an iterator iterates over a container. The container should be reachable
by the garbage collector while the iterator is reachable in the JavaScript
manner, but C++ (STL) iterators don't offer such functionality by default. Using
C++ iterator here is not a trivial problem, I think.

[yhirano, 2014-08-25 06:09:26]

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.cpp:120:
v8::Handle<v8::FunctionTemplate>
V8DOMConfiguration::functionTemplateForMethod(v8::Handle<v8::Signature>
signature, const SymbolKeyedMethodConfiguration& callback, v8::Isolate* isolate)
On 2014/08/21 13:58:01, Jens Widell wrote:
> This is identical to the other functionTemplateForMethod() except for the type
> of the |callback| argument, yes?
> 
> Might it be worth-while to have a local template helper, just to avoid the
> duplicated code?

Done.

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/scripts...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:108:
includes.add('bindings/core/v8/V8Iterator.h')
On 2014/08/21 13:58:01, Jens Widell wrote:
> Is this needed? I thought it was at first, but then I realized I hadn't run
> 'gclient runhooks' after applying your patch, so Iterator.idl wasn't
processed.
> Now that I have, this include seems to still be added with this line commented
> out.

You're right, thanks!

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:175: 'is_iterable': is_iterable,
On 2014/08/21 13:58:02, Jens Widell wrote:
> I'd slightly prefer if we dropped this and just used {% if
> method.iterator_method %} in the templates instead.

Done.

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/templat...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:982: static const
V8DOMConfiguration::SymbolKeyedMethodConfiguration symbolIteratorConfiguration =
{ v8::Symbol::GetIterator,
{{cpp_class}}V8Internal::symbol::iteratorMethodCallback, 0, 0,
V8DOMConfiguration::ExposedToAllScripts };
On 2014/08/21 16:15:58, haraken wrote:
> 
> Don't mix "SymbolKeyedMethodConfiguration" and
> "symbolIteratorMethodConfiguration" :)

Done.

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/tests/r...
File Source/bindings/tests/results/V8TestInterface.cpp (right):

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/V8TestInterface.cpp:25: #include
"bindings/tests/v8/V8Iterator.h"
On 2014/08/21 13:58:02, Jens Widell wrote:
> Please help me out: I don't understand why this #include added here.

I don't know either. Given that V8Node.h is in the directory as well, it seems
that the code generator thinks DOM classes are placed on the directory.
(bindings/core/v8/V8Iterator.h has gone since I deleted the manual include
instruction in v8_interface.py).

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/V8TestInterface.cpp:1421: namespace symbol {
On 2014/08/21 13:58:02, Jens Widell wrote:
> Nit: it would be nice with a blank after this line.

Deleted

https://codereview.chromium.org/483163003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/V8TestInterface.cpp:1421: namespace symbol {
On 2014/08/21 16:15:58, haraken wrote:
> > I introduced a new namespace to avoid name conflict against a string-keyed
> > method with the same name.
> > In this case, interfaces can have usual 'iterator' method.
> 
> I guess we can just use "iteratorMethod" until it causes actual conflict. (We
> already have some builtin getters/setters/methods that might conflict with
> actual getters/setters/methods on interfaces.)
> 
> I'd prefer not adding extra complexity (i.e., extra namespace) to the IDL
> compiler, but this is not a big deal anyway.

OK, I deleted the namespace. Let's think about the conflict when it is needed.

https://codereview.chromium.org/483163003/diff/100001/Source/core/testing/Int...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/483163003/diff/100001/Source/core/testing/Int...
Source/core/testing/Internals.cpp:147: class InternalsIterator : public Iterator
{
On 2014/08/21 16:15:58, haraken wrote:
> 
> Add FINAL.

Done.

https://codereview.chromium.org/483163003/diff/100001/Source/core/testing/Int...
Source/core/testing/Internals.cpp:160: result->Set(v8String(isolate, "done"),
v8::Boolean::New(isolate, done));
On 2014/08/21 16:15:58, haraken wrote:
> 
> v8::Boolean::New() => v8Boolean()

Done.

[haraken, 2014-08-25 06:22:00]

Mostly looks good.

Jens and arv might want to confirm.

https://codereview.chromium.org/483163003/diff/140001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8DOMConfiguration.h (right):

https://codereview.chromium.org/483163003/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.h:137: v8::Local<v8::Name>
getName(v8::Isolate* isolate) const { return v8AtomicString(isolate, name); }

getName => name

Blink doesn't use 'get'.

https://codereview.chromium.org/483163003/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.h:138: v8::FunctionCallback
getCallbackForWorld(const DOMWrapperWorld& world) const

getCallbackForWorld => callbackForWorld

https://codereview.chromium.org/483163003/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.h:151: v8::Local<v8::Name>
getName(v8::Isolate* isolate) const { return getSymbol(isolate); }

getName => name

https://codereview.chromium.org/483163003/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.h:152: v8::FunctionCallback
getCallbackForWorld(const DOMWrapperWorld& world) const

getCallbackForWorld => callbackForWorld

https://codereview.chromium.org/483163003/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.h:159: v8::FunctionCallback
callbackForMainWorld;

Now that you don't support per-world-bindings for Iterators, can you drop
callbackForMainWorld from SymbolKeyedMethodConfiguration?

https://codereview.chromium.org/483163003/diff/140001/Source/bindings/templat...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/483163003/diff/140001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:113: {% for world_suffix in
iterator_method.world_suffixes %}

Ditto. You don't need to support per-world-bindings for Iterators.

[yhirano, 2014-08-25 06:54:58]

https://codereview.chromium.org/483163003/diff/140001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8DOMConfiguration.h (right):

https://codereview.chromium.org/483163003/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.h:137: v8::Local<v8::Name>
getName(v8::Isolate* isolate) const { return v8AtomicString(isolate, name); }
On 2014/08/25 06:22:00, haraken wrote:
> 
> getName => name
> 
> Blink doesn't use 'get'.
renamed to methodName to avoid conflict.

https://codereview.chromium.org/483163003/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.h:138: v8::FunctionCallback
getCallbackForWorld(const DOMWrapperWorld& world) const
On 2014/08/25 06:22:00, haraken wrote:
> 
> getCallbackForWorld => callbackForWorld

Done.

https://codereview.chromium.org/483163003/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.h:151: v8::Local<v8::Name>
getName(v8::Isolate* isolate) const { return getSymbol(isolate); }
On 2014/08/25 06:22:00, haraken wrote:
> 
> getName => name

Done.

https://codereview.chromium.org/483163003/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.h:152: v8::FunctionCallback
getCallbackForWorld(const DOMWrapperWorld& world) const
On 2014/08/25 06:22:00, haraken wrote:
> 
> getCallbackForWorld => callbackForWorld

Done.

https://codereview.chromium.org/483163003/diff/140001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.h:159: v8::FunctionCallback
callbackForMainWorld;
On 2014/08/25 06:22:00, haraken wrote:
> 
> Now that you don't support per-world-bindings for Iterators, can you drop
> callbackForMainWorld from SymbolKeyedMethodConfiguration?

Done.

https://codereview.chromium.org/483163003/diff/140001/Source/bindings/templat...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/483163003/diff/140001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:113: {% for world_suffix in
iterator_method.world_suffixes %}
On 2014/08/25 06:22:00, haraken wrote:
> 
> Ditto. You don't need to support per-world-bindings for Iterators.

Done.

[Jens Widell, 2014-08-25 07:01:38]

LGTM

[haraken, 2014-08-25 07:11:09]

LGTM

https://codereview.chromium.org/483163003/diff/160001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8DOMConfiguration.h (right):

https://codereview.chromium.org/483163003/diff/160001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.h:152: v8::FunctionCallback
callbackForWorld(const DOMWrapperWorld& world) const

Remove |world|.

[yhirano, 2014-08-25 07:39:28]

https://codereview.chromium.org/483163003/diff/160001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8DOMConfiguration.h (right):

https://codereview.chromium.org/483163003/diff/160001/Source/bindings/core/v8...
Source/bindings/core/v8/V8DOMConfiguration.h:152: v8::FunctionCallback
callbackForWorld(const DOMWrapperWorld& world) const
On 2014/08/25 07:11:09, haraken wrote:
> 
> Remove |world|.

Done.

[yhirano, 2014-08-28 01:34:57]

arv@: Do you have other comments?

[sof, 2014-08-28 08:10:56]

When this has landed, it'd be good to document [Iterable] in
http://www.chromium.org/blink/webidl/blink-idl-extended-attributes

[yhirano, 2014-08-28 08:22:59]

On 2014/08/28 08:10:56, sof wrote:
> When this has landed, it'd be good to document [Iterable] in
> http://www.chromium.org/blink/webidl/blink-idl-extended-attributes

Yes, I will do.

[jsbell, 2014-08-28 16:50:33]

Drive-by comment without looking at the patch.

Since it seems like we're taking the lead here (yay!), is there anything that
should be added to the discussion in
https://www.w3.org/Bugs/Public/show_bug.cgi?id=26183 ?

[arv (Not doing code reviews), 2014-09-02 14:31:24]

LGTM

https://codereview.chromium.org/483163003/diff/200001/Source/core/dom/Iterator.h
File Source/core/dom/Iterator.h (right):

https://codereview.chromium.org/483163003/diff/200001/Source/core/dom/Iterato...
Source/core/dom/Iterator.h:23: Iterator* iterator(ScriptState*, ExceptionState&)
{ return this; }
This seems acceptable since our infrastructure makes this the logical
implementation... but in practice these do not need to go through the C++ code
and it is unclear if we want the type check etc.

https://codereview.chromium.org/483163003/diff/200001/Source/core/testing/Int...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/483163003/diff/200001/Source/core/testing/Int...
Source/core/testing/Internals.cpp:161: return ScriptValue(scriptState, result);
Once these becomes common we might want to introduce a helper...

[yhirano, 2014-09-03 06:42:29]

https://codereview.chromium.org/483163003/diff/200001/Source/core/testing/Int...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/483163003/diff/200001/Source/core/testing/Int...
Source/core/testing/Internals.cpp:161: return ScriptValue(scriptState, result);
On 2014/09/02 14:31:24, arv wrote:
> Once these becomes common we might want to introduce a helper...

Yeah, the next CL has v8ResultIterator helper function. I will make
InternalsIterator use it.

[yhirano, 2014-09-03 06:55:12]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-09-03 06:55:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yhirano@chromium.org/483163003/220001

[yhirano, 2014-09-03 07:03:26]

The CQ bit was unchecked by yhirano@chromium.org

[yhirano, 2014-09-03 07:07:05]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-09-03 07:08:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yhirano@chromium.org/483163003/220001

[yhirano, 2014-09-03 07:21:05]

https://codereview.chromium.org/483163003/diff/200001/Source/core/testing/Int...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/483163003/diff/200001/Source/core/testing/Int...
Source/core/testing/Internals.cpp:161: return ScriptValue(scriptState, result);
On 2014/09/03 06:42:29, yhirano wrote:
> On 2014/09/02 14:31:24, arv wrote:
> > Once these becomes common we might want to introduce a helper...
> 
> Yeah, the next CL has v8ResultIterator helper function. I will make
> InternalsIterator use it.

https://codereview.chromium.org/513203002/diff/220001/Source/core/testing/Int...

[commit-bot: I haz the power, 2014-09-03 08:06:38]

Committed patchset #12 (id:220001) as 181294

[yhirano, 2014-09-03 09:07:33]

On 2014/08/28 08:10:56, sof wrote:
> When this has landed, it'd be good to document [Iterable] in
> http://www.chromium.org/blink/webidl/blink-idl-extended-attributes

Done.