IDL: Add forEach() method to iterable<>/maplike<>/setlike<> interfaces

IDL: Add forEach() method to iterable<>/maplike<>/setlike<> interfaces

Also add an implementation of forEach() in the Iterable<> helper class,
so that current interfaces get the new method for free.

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

[Jens Widell, 2015-01-22 15:11:14]

jl@opera.com changed reviewers:
+ haraken@chromium.org

[Jens Widell, 2015-01-22 15:11:14]

PTAL

I'm in particular not sure the actual forEach() implementation is done right, so
please comment on that.

[haraken, 2015-01-22 15:17:38]

On 2015/01/22 15:11:14, Jens Widell wrote:
> PTAL
> 
> I'm in particular not sure the actual forEach() implementation is done right,
so
> please comment on that.

yhirano-san: Would you take a look at Iterable.h?

[haraken, 2015-01-22 15:27:58]

haraken@chromium.org changed reviewers:
+ yhirano@chromium.org

[jsbell, 2015-01-22 19:53:53]

jsbell@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2015-01-22 19:53:54]

https://codereview.chromium.org/807263007/diff/20001/LayoutTests/fast/js/iter...
File LayoutTests/fast/js/iterable-object.html (right):

https://codereview.chromium.org/807263007/diff/20001/LayoutTests/fast/js/iter...
LayoutTests/fast/js/iterable-object.html:31: internals.forEach(function (value,
key, object) {
Can you add cases for:

* Calling forEach w/ a non-function argument? (will throw)

* Calling forEach w/o a thisArg? (should be global or undefined, depending on
strictness, I think?)

[Jens Widell, 2015-01-23 11:08:14]

https://codereview.chromium.org/807263007/diff/20001/LayoutTests/fast/js/iter...
File LayoutTests/fast/js/iterable-object.html (right):

https://codereview.chromium.org/807263007/diff/20001/LayoutTests/fast/js/iter...
LayoutTests/fast/js/iterable-object.html:31: internals.forEach(function (value,
key, object) {
On 2015/01/22 19:53:54, jsbell wrote:
> Can you add cases for:
> 
> * Calling forEach w/ a non-function argument? (will throw)
> 
> * Calling forEach w/o a thisArg? (should be global or undefined, depending on
> strictness, I think?)
> 

Done.

Also added cases for the callback function throwing, and some other thisArg
variants, with and without a strict mode callback function.

[yhirano, 2015-01-23 11:49:03]

Can you remove forEach FIXMEs from modules/webmidi/MIDI{Input, Output}Map.idl?

https://codereview.chromium.org/807263007/diff/40001/LayoutTests/http/tests/f...
File LayoutTests/http/tests/fetch/script-tests/headers.js (right):

https://codereview.chromium.org/807263007/diff/40001/LayoutTests/http/tests/f...
LayoutTests/http/tests/fetch/script-tests/headers.js:135: assert_true(thisObject
=== this);
I think assert_equals uses the strict comparison, right? Then you can use
assert_equals here.

https://codereview.chromium.org/807263007/diff/40001/Source/core/dom/Iterable.h
File Source/core/dom/Iterable.h (right):

https://codereview.chromium.org/807263007/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:49: v8::HandleScope handleScope(isolate);
Can you tell me why v8::Context::Scope is necessary here?

[Jens Widell, 2015-01-23 11:53:55]

https://codereview.chromium.org/807263007/diff/40001/LayoutTests/http/tests/f...
File LayoutTests/http/tests/fetch/script-tests/headers.js (right):

https://codereview.chromium.org/807263007/diff/40001/LayoutTests/http/tests/f...
LayoutTests/http/tests/fetch/script-tests/headers.js:135: assert_true(thisObject
=== this);
On 2015/01/23 11:49:03, yhirano wrote:
> I think assert_equals uses the strict comparison, right? Then you can use
> assert_equals here.

Indeed it does, thanks. Changed to use assert_equals().

https://codereview.chromium.org/807263007/diff/40001/Source/core/dom/Iterable.h
File Source/core/dom/Iterable.h (right):

https://codereview.chromium.org/807263007/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:49: v8::HandleScope handleScope(isolate);
On 2015/01/23 11:49:03, yhirano wrote:
> Can you tell me why v8::Context::Scope is necessary here?

I don't know that it is, to be honest. I put it here because V8ScriptRunner.h
has this comment:

  // For the following methods, the caller sites have to hold
  // a HandleScope and a ContextScope.

[haraken, 2015-01-23 11:55:39]

https://codereview.chromium.org/807263007/diff/40001/Source/core/dom/Iterable.h
File Source/core/dom/Iterable.h (right):

https://codereview.chromium.org/807263007/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:49: v8::HandleScope handleScope(isolate);
On 2015/01/23 11:53:55, Jens Widell wrote:
> On 2015/01/23 11:49:03, yhirano wrote:
> > Can you tell me why v8::Context::Scope is necessary here?
> 
> I don't know that it is, to be honest. I put it here because V8ScriptRunner.h
> has this comment:
> 
>   // For the following methods, the caller sites have to hold
>   // a HandleScope and a ContextScope.

Given that forEach is always called from JavaScript, we should already be in a
correct ContextScope and have a HandleScope. Thus I guess we won't need either
of them.

[Jens Widell, 2015-01-23 11:58:14]

https://codereview.chromium.org/807263007/diff/40001/Source/core/dom/Iterable.h
File Source/core/dom/Iterable.h (right):

https://codereview.chromium.org/807263007/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:49: v8::HandleScope handleScope(isolate);
On 2015/01/23 11:55:39, haraken wrote:
> On 2015/01/23 11:53:55, Jens Widell wrote:
> > On 2015/01/23 11:49:03, yhirano wrote:
> > > Can you tell me why v8::Context::Scope is necessary here?
> > 
> > I don't know that it is, to be honest. I put it here because
V8ScriptRunner.h
> > has this comment:
> > 
> >   // For the following methods, the caller sites have to hold
> >   // a HandleScope and a ContextScope.
> 
> Given that forEach is always called from JavaScript, we should already be in a
> correct ContextScope and have a HandleScope. Thus I guess we won't need either
> of them.

Yeah, that makes sense.

[Jens Widell, 2015-01-26 11:08:10]

On 2015/01/23 11:49:03, yhirano wrote:
> Can you remove forEach FIXMEs from modules/webmidi/MIDI{Input, Output}Map.idl?

Done.

[yhirano, 2015-01-26 11:19:27]

lgtm

[Jens Widell, 2015-01-26 13:07:31]

On 2015/01/26 11:19:27, yhirano wrote:
> lgtm

Thanks!

haraken@: Do you want to have another look at this?

[haraken, 2015-01-27 01:56:31]

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

https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:335: def generated_argument(idl_type,
name, is_optional=False, default_value=None, extended_attributes=None):

It looks like default_value is unused.

https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:374: forEach_extended_attributes =
used_extended_attributes.copy()

Can't we just use used_extended_attributes? In other words, is it important that
forEach_extended_attributes doesn't have 'RaisesException: None'?

https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:204: 'ThisValue':
'ScriptValue(scriptState, info.This())',

Just to confirm: Are you sure that we should use info.This(), not info.Holder()?

https://codereview.chromium.org/807263007/diff/120001/Source/core/dom/Iterable.h
File Source/core/dom/Iterable.h (right):

https://codereview.chromium.org/807263007/diff/120001/Source/core/dom/Iterabl...
Source/core/dom/Iterable.h:47: v8::TryCatch tryCatch(isolate);

I guess you don't need this TryCatch (see the below comment).

https://codereview.chromium.org/807263007/diff/120001/Source/core/dom/Iterabl...
Source/core/dom/Iterable.h:66: args[1] = toV8(key, creationContext, isolate);

Nit: It is interesting that the order is value => key, not key => value.

https://codereview.chromium.org/807263007/diff/120001/Source/core/dom/Iterabl...
Source/core/dom/Iterable.h:70: if (tryCatch.HasCaught()) {

Can we use exceptionState.hadException()? We don't want to mix TryCatch and
ExceptionState.

[Jens Widell, 2015-01-27 07:19:23]

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

https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:335: def generated_argument(idl_type,
name, is_optional=False, default_value=None, extended_attributes=None):
On 2015/01/27 01:56:30, haraken wrote:
> 
> It looks like default_value is unused.

Yes, not needed at the moment. Not on the radar that it will be either, so might
as well remove it.

https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:374: forEach_extended_attributes =
used_extended_attributes.copy()
On 2015/01/27 01:56:30, haraken wrote:
> 
> Can't we just use used_extended_attributes? In other words, is it important
that
> forEach_extended_attributes doesn't have 'RaisesException: None'?

I am essentially using used_extended_attributes. I'm cloning it to modify
'CallWith' (adding 'ThisValue').

https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:204: 'ThisValue':
'ScriptValue(scriptState, info.This())',
On 2015/01/27 01:56:30, haraken wrote:
> 
> Just to confirm: Are you sure that we should use info.This(), not
info.Holder()?

No, I'm not sure. What's the difference between the two, exactly?

Technically, we could do without this altogether. Iterable<> would just need a
way to "find itself," since it's a mixin class and not a ScriptWrappable.
Something like

  virtual v8::Local<v8::Object> getThis() = 0;

or such would work. I figured this [CallWith=ThisValue] approach would be
simpler.

https://codereview.chromium.org/807263007/diff/120001/Source/core/dom/Iterable.h
File Source/core/dom/Iterable.h (right):

https://codereview.chromium.org/807263007/diff/120001/Source/core/dom/Iterabl...
Source/core/dom/Iterable.h:66: args[1] = toV8(key, creationContext, isolate);
On 2015/01/27 01:56:30, haraken wrote:
> 
> Nit: It is interesting that the order is value => key, not key => value.

Yeah, in this context it seems a bit backwards. This is compatible with the
native [].forEach() though.

https://codereview.chromium.org/807263007/diff/120001/Source/core/dom/Iterabl...
Source/core/dom/Iterable.h:70: if (tryCatch.HasCaught()) {
On 2015/01/27 01:56:30, haraken wrote:
> 
> Can we use exceptionState.hadException()? We don't want to mix TryCatch and
> ExceptionState.

Since I'm not passing 'exceptionState' to V8ScriptRunner::callFunction(), it
can't throw that way. I don't know if there's another "call function" utility
function that does take an ExceptionState that I can use instead.

Technically, we have to "mix" ExceptionState and TryCatch in some places, namely
at the boundary points between "Blink" and "V8". This is one such boundary point
(we're calling into V8 from Blink, after all.)

Also, since this is a loop, it's somewhat advantageous to declare the TryCatch
at this level, outside the loop, to limit the overhead.

[haraken, 2015-01-27 08:47:11]

https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
Source/bindings/scripts/v8_utilities.py:204: 'ThisValue':
'ScriptValue(scriptState, info.This())',
On 2015/01/27 07:19:23, Jens Widell wrote:
> On 2015/01/27 01:56:30, haraken wrote:
> > 
> > Just to confirm: Are you sure that we should use info.This(), not
> info.Holder()?
> 
> No, I'm not sure. What's the difference between the two, exactly?
> 
> Technically, we could do without this altogether. Iterable<> would just need a
> way to "find itself," since it's a mixin class and not a ScriptWrappable.
> Something like
> 
>   virtual v8::Local<v8::Object> getThis() = 0;
> 
> or such would work. I figured this [CallWith=ThisValue] approach would be
> simpler.

info.Holder() points to a reciever, and info.This() points to an object on which
the DOM attribute/method exists. For example, in a case of div.appendChild(),
info.Holder() points to the div, and info.This() points to
div.__proto__.__proto__.__proto__.__proto__, on which the appendChild is
defined.

[Jens Widell, 2015-01-27 09:07:20]

On 2015/01/27 08:47:11, haraken wrote:
>
https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
> File Source/bindings/scripts/v8_utilities.py (right):
> 
>
https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
> Source/bindings/scripts/v8_utilities.py:204: 'ThisValue':
> 'ScriptValue(scriptState, info.This())',
> On 2015/01/27 07:19:23, Jens Widell wrote:
> > On 2015/01/27 01:56:30, haraken wrote:
> > > 
> > > Just to confirm: Are you sure that we should use info.This(), not
> > info.Holder()?
> > 
> > No, I'm not sure. What's the difference between the two, exactly?
> > 
> > Technically, we could do without this altogether. Iterable<> would just need
a
> > way to "find itself," since it's a mixin class and not a ScriptWrappable.
> > Something like
> > 
> >   virtual v8::Local<v8::Object> getThis() = 0;
> > 
> > or such would work. I figured this [CallWith=ThisValue] approach would be
> > simpler.
> 
> info.Holder() points to a reciever, and info.This() points to an object on
which
> the DOM attribute/method exists. For example, in a case of div.appendChild(),
> info.Holder() points to the div, and info.This() points to
> div.__proto__.__proto__.__proto__.__proto__, on which the appendChild is
> defined.

Given that, it certainly sounds like it's info.Holder() that I want.

[Jens Widell, 2015-01-27 11:20:05]

On 2015/01/27 09:07:20, Jens Widell wrote:
> On 2015/01/27 08:47:11, haraken wrote:
> >
>
https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
> > File Source/bindings/scripts/v8_utilities.py (right):
> > 
> >
>
https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
> > Source/bindings/scripts/v8_utilities.py:204: 'ThisValue':
> > 'ScriptValue(scriptState, info.This())',
> > On 2015/01/27 07:19:23, Jens Widell wrote:
> > > On 2015/01/27 01:56:30, haraken wrote:
> > > > 
> > > > Just to confirm: Are you sure that we should use info.This(), not
> > > info.Holder()?
> > > 
> > > No, I'm not sure. What's the difference between the two, exactly?
> > > 
> > > Technically, we could do without this altogether. Iterable<> would just
need
> a
> > > way to "find itself," since it's a mixin class and not a ScriptWrappable.
> > > Something like
> > > 
> > >   virtual v8::Local<v8::Object> getThis() = 0;
> > > 
> > > or such would work. I figured this [CallWith=ThisValue] approach would be
> > > simpler.
> > 
> > info.Holder() points to a reciever, and info.This() points to an object on
> which
> > the DOM attribute/method exists. For example, in a case of
div.appendChild(),
> > info.Holder() points to the div, and info.This() points to
> > div.__proto__.__proto__.__proto__.__proto__, on which the appendChild is
> > defined.
> 
> Given that, it certainly sounds like it's info.Holder() that I want.

I can't really seem to work out a way to make these two different. For Internals
and forEach(), the method is actually on internals.__proto__, but 'thisValue'
(as observed by the callback function called by forEach()) is always
'internals', whether I use info.This() or info.Holder().

Calling forEach() with some other this value (i.e. using
internals.forEach.apply(...)) just triggers an "invalid invocation" error.

So I guess I don't really understand how to determine which one to use, or even
if it matters. :-)

[haraken, 2015-01-27 22:50:00]

On 2015/01/27 11:20:05, Jens Widell wrote:
> On 2015/01/27 09:07:20, Jens Widell wrote:
> > On 2015/01/27 08:47:11, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
> > > File Source/bindings/scripts/v8_utilities.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
> > > Source/bindings/scripts/v8_utilities.py:204: 'ThisValue':
> > > 'ScriptValue(scriptState, info.This())',
> > > On 2015/01/27 07:19:23, Jens Widell wrote:
> > > > On 2015/01/27 01:56:30, haraken wrote:
> > > > > 
> > > > > Just to confirm: Are you sure that we should use info.This(), not
> > > > info.Holder()?
> > > > 
> > > > No, I'm not sure. What's the difference between the two, exactly?
> > > > 
> > > > Technically, we could do without this altogether. Iterable<> would just
> need
> > a
> > > > way to "find itself," since it's a mixin class and not a
ScriptWrappable.
> > > > Something like
> > > > 
> > > >   virtual v8::Local<v8::Object> getThis() = 0;
> > > > 
> > > > or such would work. I figured this [CallWith=ThisValue] approach would
be
> > > > simpler.
> > > 
> > > info.Holder() points to a reciever, and info.This() points to an object on
> > which
> > > the DOM attribute/method exists. For example, in a case of
> div.appendChild(),
> > > info.Holder() points to the div, and info.This() points to
> > > div.__proto__.__proto__.__proto__.__proto__, on which the appendChild is
> > > defined.
> > 
> > Given that, it certainly sounds like it's info.Holder() that I want.
> 
> I can't really seem to work out a way to make these two different. For
Internals
> and forEach(), the method is actually on internals.__proto__, but 'thisValue'
> (as observed by the callback function called by forEach()) is always
> 'internals', whether I use info.This() or info.Holder().
> 
> Calling forEach() with some other this value (i.e. using
> internals.forEach.apply(...)) just triggers an "invalid invocation" error.
> 
> So I guess I don't really understand how to determine which one to use, or
even
> if it matters. :-)

Sorry about the confusion; my explanation was the opposite...

Correction:

info.This() points to a reciever, and info.Holder() points to an object on which
the DOM attribute/method exists. For example, in a case of div.appendChild(),
info.This() points to the div, and info.Holder() points to
div.__proto__.__proto__.__proto__.__proto__, on which the appendChild is
defined.

Then does it make sense to you to use info.Holder()?

[Jens Widell, 2015-01-28 07:44:00]

On 2015/01/27 22:50:00, haraken wrote:
> On 2015/01/27 11:20:05, Jens Widell wrote:
> > On 2015/01/27 09:07:20, Jens Widell wrote:
> > > On 2015/01/27 08:47:11, haraken wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
> > > > File Source/bindings/scripts/v8_utilities.py (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
> > > > Source/bindings/scripts/v8_utilities.py:204: 'ThisValue':
> > > > 'ScriptValue(scriptState, info.This())',
> > > > On 2015/01/27 07:19:23, Jens Widell wrote:
> > > > > On 2015/01/27 01:56:30, haraken wrote:
> > > > > > 
> > > > > > Just to confirm: Are you sure that we should use info.This(), not
> > > > > info.Holder()?
> > > > > 
> > > > > No, I'm not sure. What's the difference between the two, exactly?
> > > > > 
> > > > > Technically, we could do without this altogether. Iterable<> would
just
> > need
> > > a
> > > > > way to "find itself," since it's a mixin class and not a
> ScriptWrappable.
> > > > > Something like
> > > > > 
> > > > >   virtual v8::Local<v8::Object> getThis() = 0;
> > > > > 
> > > > > or such would work. I figured this [CallWith=ThisValue] approach would
> be
> > > > > simpler.
> > > > 
> > > > info.Holder() points to a reciever, and info.This() points to an object
on
> > > which
> > > > the DOM attribute/method exists. For example, in a case of
> > div.appendChild(),
> > > > info.Holder() points to the div, and info.This() points to
> > > > div.__proto__.__proto__.__proto__.__proto__, on which the appendChild is
> > > > defined.
> > > 
> > > Given that, it certainly sounds like it's info.Holder() that I want.
> > 
> > I can't really seem to work out a way to make these two different. For
> Internals
> > and forEach(), the method is actually on internals.__proto__, but
'thisValue'
> > (as observed by the callback function called by forEach()) is always
> > 'internals', whether I use info.This() or info.Holder().
> > 
> > Calling forEach() with some other this value (i.e. using
> > internals.forEach.apply(...)) just triggers an "invalid invocation" error.
> > 
> > So I guess I don't really understand how to determine which one to use, or
> even
> > if it matters. :-)
> 
> Sorry about the confusion; my explanation was the opposite...
> 
> Correction:
> 
> info.This() points to a reciever, and info.Holder() points to an object on
which
> the DOM attribute/method exists. For example, in a case of div.appendChild(),
> info.This() points to the div, and info.Holder() points to
> div.__proto__.__proto__.__proto__.__proto__, on which the appendChild is
> defined.
> 
> Then does it make sense to you to use info.Holder()?

If info.Holder() is the prototype object (forEach() would typically be defined
on a prototype object) it seems wrong to use for my purposes here; it's the
actual iterable object that the forEach() implementation needs.

But when I tested this, info.This() and info.Holder() both gave the same correct
result. So I guess at this point I'm quite confused about which one to use. :-)

[haraken, 2015-01-28 07:49:38]

On 2015/01/28 07:44:00, Jens Widell wrote:
> On 2015/01/27 22:50:00, haraken wrote:
> > On 2015/01/27 11:20:05, Jens Widell wrote:
> > > On 2015/01/27 09:07:20, Jens Widell wrote:
> > > > On 2015/01/27 08:47:11, haraken wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
> > > > > File Source/bindings/scripts/v8_utilities.py (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
> > > > > Source/bindings/scripts/v8_utilities.py:204: 'ThisValue':
> > > > > 'ScriptValue(scriptState, info.This())',
> > > > > On 2015/01/27 07:19:23, Jens Widell wrote:
> > > > > > On 2015/01/27 01:56:30, haraken wrote:
> > > > > > > 
> > > > > > > Just to confirm: Are you sure that we should use info.This(), not
> > > > > > info.Holder()?
> > > > > > 
> > > > > > No, I'm not sure. What's the difference between the two, exactly?
> > > > > > 
> > > > > > Technically, we could do without this altogether. Iterable<> would
> just
> > > need
> > > > a
> > > > > > way to "find itself," since it's a mixin class and not a
> > ScriptWrappable.
> > > > > > Something like
> > > > > > 
> > > > > >   virtual v8::Local<v8::Object> getThis() = 0;
> > > > > > 
> > > > > > or such would work. I figured this [CallWith=ThisValue] approach
would
> > be
> > > > > > simpler.
> > > > > 
> > > > > info.Holder() points to a reciever, and info.This() points to an
object
> on
> > > > which
> > > > > the DOM attribute/method exists. For example, in a case of
> > > div.appendChild(),
> > > > > info.Holder() points to the div, and info.This() points to
> > > > > div.__proto__.__proto__.__proto__.__proto__, on which the appendChild
is
> > > > > defined.
> > > > 
> > > > Given that, it certainly sounds like it's info.Holder() that I want.
> > > 
> > > I can't really seem to work out a way to make these two different. For
> > Internals
> > > and forEach(), the method is actually on internals.__proto__, but
> 'thisValue'
> > > (as observed by the callback function called by forEach()) is always
> > > 'internals', whether I use info.This() or info.Holder().
> > > 
> > > Calling forEach() with some other this value (i.e. using
> > > internals.forEach.apply(...)) just triggers an "invalid invocation" error.
> > > 
> > > So I guess I don't really understand how to determine which one to use, or
> > even
> > > if it matters. :-)
> > 
> > Sorry about the confusion; my explanation was the opposite...
> > 
> > Correction:
> > 
> > info.This() points to a reciever, and info.Holder() points to an object on
> which
> > the DOM attribute/method exists. For example, in a case of
div.appendChild(),
> > info.This() points to the div, and info.Holder() points to
> > div.__proto__.__proto__.__proto__.__proto__, on which the appendChild is
> > defined.
> > 
> > Then does it make sense to you to use info.Holder()?
> 
> If info.Holder() is the prototype object (forEach() would typically be defined
> on a prototype object) it seems wrong to use for my purposes here; it's the
> actual iterable object that the forEach() implementation needs.
> 
> But when I tested this, info.This() and info.Holder() both gave the same
correct
> result. So I guess at this point I'm quite confused about which one to use.
:-)

What happens if you do something like:

xhr = new XMLHttpRequest();
xhr.__proto__ = MIDIInputMap.prototype;
xhr.forEach(...);  // This should throw TypeError.

or:

map = ...; // Create an object of MIDIInputMap.
obj = Object.create(map);
obj.forEach(...);  // This should throw TypeError.

[Jens Widell, 2015-01-28 07:56:43]

On 2015/01/28 07:49:38, haraken wrote:
> On 2015/01/28 07:44:00, Jens Widell wrote:
> > On 2015/01/27 22:50:00, haraken wrote:
> > > On 2015/01/27 11:20:05, Jens Widell wrote:
> > > > On 2015/01/27 09:07:20, Jens Widell wrote:
> > > > > On 2015/01/27 08:47:11, haraken wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
> > > > > > File Source/bindings/scripts/v8_utilities.py (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
> > > > > > Source/bindings/scripts/v8_utilities.py:204: 'ThisValue':
> > > > > > 'ScriptValue(scriptState, info.This())',
> > > > > > On 2015/01/27 07:19:23, Jens Widell wrote:
> > > > > > > On 2015/01/27 01:56:30, haraken wrote:
> > > > > > > > 
> > > > > > > > Just to confirm: Are you sure that we should use info.This(),
not
> > > > > > > info.Holder()?
> > > > > > > 
> > > > > > > No, I'm not sure. What's the difference between the two, exactly?
> > > > > > > 
> > > > > > > Technically, we could do without this altogether. Iterable<> would
> > just
> > > > need
> > > > > a
> > > > > > > way to "find itself," since it's a mixin class and not a
> > > ScriptWrappable.
> > > > > > > Something like
> > > > > > > 
> > > > > > >   virtual v8::Local<v8::Object> getThis() = 0;
> > > > > > > 
> > > > > > > or such would work. I figured this [CallWith=ThisValue] approach
> would
> > > be
> > > > > > > simpler.
> > > > > > 
> > > > > > info.Holder() points to a reciever, and info.This() points to an
> object
> > on
> > > > > which
> > > > > > the DOM attribute/method exists. For example, in a case of
> > > > div.appendChild(),
> > > > > > info.Holder() points to the div, and info.This() points to
> > > > > > div.__proto__.__proto__.__proto__.__proto__, on which the
appendChild
> is
> > > > > > defined.
> > > > > 
> > > > > Given that, it certainly sounds like it's info.Holder() that I want.
> > > > 
> > > > I can't really seem to work out a way to make these two different. For
> > > Internals
> > > > and forEach(), the method is actually on internals.__proto__, but
> > 'thisValue'
> > > > (as observed by the callback function called by forEach()) is always
> > > > 'internals', whether I use info.This() or info.Holder().
> > > > 
> > > > Calling forEach() with some other this value (i.e. using
> > > > internals.forEach.apply(...)) just triggers an "invalid invocation"
error.
> > > > 
> > > > So I guess I don't really understand how to determine which one to use,
or
> > > even
> > > > if it matters. :-)
> > > 
> > > Sorry about the confusion; my explanation was the opposite...
> > > 
> > > Correction:
> > > 
> > > info.This() points to a reciever, and info.Holder() points to an object on
> > which
> > > the DOM attribute/method exists. For example, in a case of
> div.appendChild(),
> > > info.This() points to the div, and info.Holder() points to
> > > div.__proto__.__proto__.__proto__.__proto__, on which the appendChild is
> > > defined.
> > > 
> > > Then does it make sense to you to use info.Holder()?
> > 
> > If info.Holder() is the prototype object (forEach() would typically be
defined
> > on a prototype object) it seems wrong to use for my purposes here; it's the
> > actual iterable object that the forEach() implementation needs.
> > 
> > But when I tested this, info.This() and info.Holder() both gave the same
> correct
> > result. So I guess at this point I'm quite confused about which one to use.
> :-)
> 
> What happens if you do something like:
> 
> xhr = new XMLHttpRequest();
> xhr.__proto__ = MIDIInputMap.prototype;
> xhr.forEach(...);  // This should throw TypeError.
> 
> or:
> 
> map = ...; // Create an object of MIDIInputMap.
> obj = Object.create(map);
> obj.forEach(...);  // This should throw TypeError.

Both throw an "invalid invocation" error, thrown (I assume) by V8 because of the
v8::Signature used when the method was installed.

[haraken, 2015-01-28 08:09:55]

On 2015/01/28 07:56:43, Jens Widell wrote:
> On 2015/01/28 07:49:38, haraken wrote:
> > On 2015/01/28 07:44:00, Jens Widell wrote:
> > > On 2015/01/27 22:50:00, haraken wrote:
> > > > On 2015/01/27 11:20:05, Jens Widell wrote:
> > > > > On 2015/01/27 09:07:20, Jens Widell wrote:
> > > > > > On 2015/01/27 08:47:11, haraken wrote:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
> > > > > > > File Source/bindings/scripts/v8_utilities.py (right):
> > > > > > > 
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
> > > > > > > Source/bindings/scripts/v8_utilities.py:204: 'ThisValue':
> > > > > > > 'ScriptValue(scriptState, info.This())',
> > > > > > > On 2015/01/27 07:19:23, Jens Widell wrote:
> > > > > > > > On 2015/01/27 01:56:30, haraken wrote:
> > > > > > > > > 
> > > > > > > > > Just to confirm: Are you sure that we should use info.This(),
> not
> > > > > > > > info.Holder()?
> > > > > > > > 
> > > > > > > > No, I'm not sure. What's the difference between the two,
exactly?
> > > > > > > > 
> > > > > > > > Technically, we could do without this altogether. Iterable<>
would
> > > just
> > > > > need
> > > > > > a
> > > > > > > > way to "find itself," since it's a mixin class and not a
> > > > ScriptWrappable.
> > > > > > > > Something like
> > > > > > > > 
> > > > > > > >   virtual v8::Local<v8::Object> getThis() = 0;
> > > > > > > > 
> > > > > > > > or such would work. I figured this [CallWith=ThisValue] approach
> > would
> > > > be
> > > > > > > > simpler.
> > > > > > > 
> > > > > > > info.Holder() points to a reciever, and info.This() points to an
> > object
> > > on
> > > > > > which
> > > > > > > the DOM attribute/method exists. For example, in a case of
> > > > > div.appendChild(),
> > > > > > > info.Holder() points to the div, and info.This() points to
> > > > > > > div.__proto__.__proto__.__proto__.__proto__, on which the
> appendChild
> > is
> > > > > > > defined.
> > > > > > 
> > > > > > Given that, it certainly sounds like it's info.Holder() that I want.
> > > > > 
> > > > > I can't really seem to work out a way to make these two different. For
> > > > Internals
> > > > > and forEach(), the method is actually on internals.__proto__, but
> > > 'thisValue'
> > > > > (as observed by the callback function called by forEach()) is always
> > > > > 'internals', whether I use info.This() or info.Holder().
> > > > > 
> > > > > Calling forEach() with some other this value (i.e. using
> > > > > internals.forEach.apply(...)) just triggers an "invalid invocation"
> error.
> > > > > 
> > > > > So I guess I don't really understand how to determine which one to
use,
> or
> > > > even
> > > > > if it matters. :-)
> > > > 
> > > > Sorry about the confusion; my explanation was the opposite...
> > > > 
> > > > Correction:
> > > > 
> > > > info.This() points to a reciever, and info.Holder() points to an object
on
> > > which
> > > > the DOM attribute/method exists. For example, in a case of
> > div.appendChild(),
> > > > info.This() points to the div, and info.Holder() points to
> > > > div.__proto__.__proto__.__proto__.__proto__, on which the appendChild is
> > > > defined.
> > > > 
> > > > Then does it make sense to you to use info.Holder()?
> > > 
> > > If info.Holder() is the prototype object (forEach() would typically be
> defined
> > > on a prototype object) it seems wrong to use for my purposes here; it's
the
> > > actual iterable object that the forEach() implementation needs.
> > > 
> > > But when I tested this, info.This() and info.Holder() both gave the same
> > correct
> > > result. So I guess at this point I'm quite confused about which one to
use.
> > :-)
> > 
> > What happens if you do something like:
> > 
> > xhr = new XMLHttpRequest();
> > xhr.__proto__ = MIDIInputMap.prototype;
> > xhr.forEach(...);  // This should throw TypeError.
> > 
> > or:
> > 
> > map = ...; // Create an object of MIDIInputMap.
> > obj = Object.create(map);
> > obj.forEach(...);  // This should throw TypeError.
> 
> Both throw an "invalid invocation" error, thrown (I assume) by V8 because of
the
> v8::Signature used when the method was installed.

Thanks, I'm totally confused with info.Holder() and info.This() and looked back
a document I wrote 2.5 years ago...
https://docs.google.com/document/d/116ne63RJnCsxCJdpqe2_NZPJlbSDy_MAHIxK1jy0w...

I remember that info.Holder() is pointing to something weird and our plan is to
replace info.Holder() with info.This() (in the process of moving DOM attributes
to prototype chains). So I think info.This() is fine in this case.

LGTM.

[Jens Widell, 2015-01-28 08:11:38]

On 2015/01/28 08:09:55, haraken wrote:
> On 2015/01/28 07:56:43, Jens Widell wrote:
> > On 2015/01/28 07:49:38, haraken wrote:
> > > On 2015/01/28 07:44:00, Jens Widell wrote:
> > > > On 2015/01/27 22:50:00, haraken wrote:
> > > > > On 2015/01/27 11:20:05, Jens Widell wrote:
> > > > > > On 2015/01/27 09:07:20, Jens Widell wrote:
> > > > > > > On 2015/01/27 08:47:11, haraken wrote:
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
> > > > > > > > File Source/bindings/scripts/v8_utilities.py (right):
> > > > > > > > 
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
> > > > > > > > Source/bindings/scripts/v8_utilities.py:204: 'ThisValue':
> > > > > > > > 'ScriptValue(scriptState, info.This())',
> > > > > > > > On 2015/01/27 07:19:23, Jens Widell wrote:
> > > > > > > > > On 2015/01/27 01:56:30, haraken wrote:
> > > > > > > > > > 
> > > > > > > > > > Just to confirm: Are you sure that we should use
info.This(),
> > not
> > > > > > > > > info.Holder()?
> > > > > > > > > 
> > > > > > > > > No, I'm not sure. What's the difference between the two,
> exactly?
> > > > > > > > > 
> > > > > > > > > Technically, we could do without this altogether. Iterable<>
> would
> > > > just
> > > > > > need
> > > > > > > a
> > > > > > > > > way to "find itself," since it's a mixin class and not a
> > > > > ScriptWrappable.
> > > > > > > > > Something like
> > > > > > > > > 
> > > > > > > > >   virtual v8::Local<v8::Object> getThis() = 0;
> > > > > > > > > 
> > > > > > > > > or such would work. I figured this [CallWith=ThisValue]
approach
> > > would
> > > > > be
> > > > > > > > > simpler.
> > > > > > > > 
> > > > > > > > info.Holder() points to a reciever, and info.This() points to an
> > > object
> > > > on
> > > > > > > which
> > > > > > > > the DOM attribute/method exists. For example, in a case of
> > > > > > div.appendChild(),
> > > > > > > > info.Holder() points to the div, and info.This() points to
> > > > > > > > div.__proto__.__proto__.__proto__.__proto__, on which the
> > appendChild
> > > is
> > > > > > > > defined.
> > > > > > > 
> > > > > > > Given that, it certainly sounds like it's info.Holder() that I
want.
> > > > > > 
> > > > > > I can't really seem to work out a way to make these two different.
For
> > > > > Internals
> > > > > > and forEach(), the method is actually on internals.__proto__, but
> > > > 'thisValue'
> > > > > > (as observed by the callback function called by forEach()) is always
> > > > > > 'internals', whether I use info.This() or info.Holder().
> > > > > > 
> > > > > > Calling forEach() with some other this value (i.e. using
> > > > > > internals.forEach.apply(...)) just triggers an "invalid invocation"
> > error.
> > > > > > 
> > > > > > So I guess I don't really understand how to determine which one to
> use,
> > or
> > > > > even
> > > > > > if it matters. :-)
> > > > > 
> > > > > Sorry about the confusion; my explanation was the opposite...
> > > > > 
> > > > > Correction:
> > > > > 
> > > > > info.This() points to a reciever, and info.Holder() points to an
object
> on
> > > > which
> > > > > the DOM attribute/method exists. For example, in a case of
> > > div.appendChild(),
> > > > > info.This() points to the div, and info.Holder() points to
> > > > > div.__proto__.__proto__.__proto__.__proto__, on which the appendChild
is
> > > > > defined.
> > > > > 
> > > > > Then does it make sense to you to use info.Holder()?
> > > > 
> > > > If info.Holder() is the prototype object (forEach() would typically be
> > defined
> > > > on a prototype object) it seems wrong to use for my purposes here; it's
> the
> > > > actual iterable object that the forEach() implementation needs.
> > > > 
> > > > But when I tested this, info.This() and info.Holder() both gave the same
> > > correct
> > > > result. So I guess at this point I'm quite confused about which one to
> use.
> > > :-)
> > > 
> > > What happens if you do something like:
> > > 
> > > xhr = new XMLHttpRequest();
> > > xhr.__proto__ = MIDIInputMap.prototype;
> > > xhr.forEach(...);  // This should throw TypeError.
> > > 
> > > or:
> > > 
> > > map = ...; // Create an object of MIDIInputMap.
> > > obj = Object.create(map);
> > > obj.forEach(...);  // This should throw TypeError.
> > 
> > Both throw an "invalid invocation" error, thrown (I assume) by V8 because of
> the
> > v8::Signature used when the method was installed.
> 
> Thanks, I'm totally confused with info.Holder() and info.This() and looked
back
> a document I wrote 2.5 years ago...
>
https://docs.google.com/document/d/116ne63RJnCsxCJdpqe2_NZPJlbSDy_MAHIxK1jy0w...

I'm not allowed to see that document. Sounds like I might want to. :-)

[Jens Widell, 2015-01-28 08:15:03]

The CQ bit was checked by jl@opera.com

[haraken, 2015-01-28 08:16:02]

On 2015/01/28 08:11:38, Jens Widell wrote:
> On 2015/01/28 08:09:55, haraken wrote:
> > On 2015/01/28 07:56:43, Jens Widell wrote:
> > > On 2015/01/28 07:49:38, haraken wrote:
> > > > On 2015/01/28 07:44:00, Jens Widell wrote:
> > > > > On 2015/01/27 22:50:00, haraken wrote:
> > > > > > On 2015/01/27 11:20:05, Jens Widell wrote:
> > > > > > > On 2015/01/27 09:07:20, Jens Widell wrote:
> > > > > > > > On 2015/01/27 08:47:11, haraken wrote:
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
> > > > > > > > > File Source/bindings/scripts/v8_utilities.py (right):
> > > > > > > > > 
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/807263007/diff/120001/Source/bindings/scripts...
> > > > > > > > > Source/bindings/scripts/v8_utilities.py:204: 'ThisValue':
> > > > > > > > > 'ScriptValue(scriptState, info.This())',
> > > > > > > > > On 2015/01/27 07:19:23, Jens Widell wrote:
> > > > > > > > > > On 2015/01/27 01:56:30, haraken wrote:
> > > > > > > > > > > 
> > > > > > > > > > > Just to confirm: Are you sure that we should use
> info.This(),
> > > not
> > > > > > > > > > info.Holder()?
> > > > > > > > > > 
> > > > > > > > > > No, I'm not sure. What's the difference between the two,
> > exactly?
> > > > > > > > > > 
> > > > > > > > > > Technically, we could do without this altogether. Iterable<>
> > would
> > > > > just
> > > > > > > need
> > > > > > > > a
> > > > > > > > > > way to "find itself," since it's a mixin class and not a
> > > > > > ScriptWrappable.
> > > > > > > > > > Something like
> > > > > > > > > > 
> > > > > > > > > >   virtual v8::Local<v8::Object> getThis() = 0;
> > > > > > > > > > 
> > > > > > > > > > or such would work. I figured this [CallWith=ThisValue]
> approach
> > > > would
> > > > > > be
> > > > > > > > > > simpler.
> > > > > > > > > 
> > > > > > > > > info.Holder() points to a reciever, and info.This() points to
an
> > > > object
> > > > > on
> > > > > > > > which
> > > > > > > > > the DOM attribute/method exists. For example, in a case of
> > > > > > > div.appendChild(),
> > > > > > > > > info.Holder() points to the div, and info.This() points to
> > > > > > > > > div.__proto__.__proto__.__proto__.__proto__, on which the
> > > appendChild
> > > > is
> > > > > > > > > defined.
> > > > > > > > 
> > > > > > > > Given that, it certainly sounds like it's info.Holder() that I
> want.
> > > > > > > 
> > > > > > > I can't really seem to work out a way to make these two different.
> For
> > > > > > Internals
> > > > > > > and forEach(), the method is actually on internals.__proto__, but
> > > > > 'thisValue'
> > > > > > > (as observed by the callback function called by forEach()) is
always
> > > > > > > 'internals', whether I use info.This() or info.Holder().
> > > > > > > 
> > > > > > > Calling forEach() with some other this value (i.e. using
> > > > > > > internals.forEach.apply(...)) just triggers an "invalid
invocation"
> > > error.
> > > > > > > 
> > > > > > > So I guess I don't really understand how to determine which one to
> > use,
> > > or
> > > > > > even
> > > > > > > if it matters. :-)
> > > > > > 
> > > > > > Sorry about the confusion; my explanation was the opposite...
> > > > > > 
> > > > > > Correction:
> > > > > > 
> > > > > > info.This() points to a reciever, and info.Holder() points to an
> object
> > on
> > > > > which
> > > > > > the DOM attribute/method exists. For example, in a case of
> > > > div.appendChild(),
> > > > > > info.This() points to the div, and info.Holder() points to
> > > > > > div.__proto__.__proto__.__proto__.__proto__, on which the
appendChild
> is
> > > > > > defined.
> > > > > > 
> > > > > > Then does it make sense to you to use info.Holder()?
> > > > > 
> > > > > If info.Holder() is the prototype object (forEach() would typically be
> > > defined
> > > > > on a prototype object) it seems wrong to use for my purposes here;
it's
> > the
> > > > > actual iterable object that the forEach() implementation needs.
> > > > > 
> > > > > But when I tested this, info.This() and info.Holder() both gave the
same
> > > > correct
> > > > > result. So I guess at this point I'm quite confused about which one to
> > use.
> > > > :-)
> > > > 
> > > > What happens if you do something like:
> > > > 
> > > > xhr = new XMLHttpRequest();
> > > > xhr.__proto__ = MIDIInputMap.prototype;
> > > > xhr.forEach(...);  // This should throw TypeError.
> > > > 
> > > > or:
> > > > 
> > > > map = ...; // Create an object of MIDIInputMap.
> > > > obj = Object.create(map);
> > > > obj.forEach(...);  // This should throw TypeError.
> > > 
> > > Both throw an "invalid invocation" error, thrown (I assume) by V8 because
of
> > the
> > > v8::Signature used when the method was installed.
> > 
> > Thanks, I'm totally confused with info.Holder() and info.This() and looked
> back
> > a document I wrote 2.5 years ago...
> >
>
https://docs.google.com/document/d/116ne63RJnCsxCJdpqe2_NZPJlbSDy_MAHIxK1jy0w...
> 
> I'm not allowed to see that document. Sounds like I might want to. :-)

Sorry, this is a public version;
https://docs.google.com/a/chromium.org/document/d/13y2STcAdxGxobEidnRzJs5sHmQ...

[commit-bot: I haz the power, 2015-01-28 08:16:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/807263007/160001

[commit-bot: I haz the power, 2015-01-28 09:47:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-28 09:47:55]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/4...)

[Jens Widell, 2015-01-28 10:21:16]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2015-01-28 10:21:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/807263007/200001

[commit-bot: I haz the power, 2015-01-28 11:35:53]

Committed patchset #11 (id:200001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189102