Make pair iterators inherit from %IteratorPrototype%.

Make pair iterators inherit from %IteratorPrototype%.

Adjust the prototype chain for iterator objects returned by interfaces that
have pair iterators.

The WebIDL spec says when an interface has pair iterators the iterators it
returns must be instances of the "default iterator object" whose
[[Prototype]] points to an "iterator prototype object" whose [[Prototype]],
on its turn, points to %IteratorPrototype%. next() must be implemented in
the "iterator prototype object", while %IteratorPrototype% provides
@@iterator.

In practice, this means the following should return true but was returning
false:

    Object.getPrototypeOf(Object.getPrototypeOf(new Headers().entries())) === Object.getPrototypeOf(Object.getPrototypeOf([][Symbol.iterator]()))

Previously, we just had the Iterator interface with both next() and
@@iterator added to its prototype object, which just inherited from
Object.prototype. We now adhere to the spec by making Iterator's prototype
object inherit by %IteratorPrototype% (which then inherits from
Object.prototype) instead. It also allows us to remove support for the
non-standard "Iterable" extended attribute we had in Blink.

The way we do it is not very pretty though: when the code for the Iterator
interface is generated, we create another function template with no
prototype and set the "prototype" property of its function object. When
|interfaceTemplate| is instantiated, its prototype.__proto__ will point to
the value of the "prototype" property we set, which is exactly what we want.

BUG=689576
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org

Review-Url: https://codereview.chromium.org/2777183004
Cr-Commit-Position: refs/heads/master@{#463224}
Committed: https://chromium.googlesource.com/chromium/src/+/6579200aa9189bce1cd61c05fb6eb4493f2302d8

[Raphael Kubo da Costa (rakuco), 2017-03-28 16:43:11]

PTAL. This is a work in progress, and I wanted to get feedback from you before
sending a patch to add new API to V8 (and writing layout tests for this).

The CL description should cover it all, I'm just not sure if adding an {% if
interface_name == "Iterable" %} is the way to go here. I couldn't think of any
other way for this to work, as it's only the Iterator interface that needs this
special treatment, with its prototype inheriting from something other than
%ObjectPrototype%. The way this is achieved with V8's API looks quite hackish as
well, but it's basically due to the way V8's API normally works and how it does
not expose intrinsic objects in its public interface.

[haraken, 2017-03-29 09:08:04]

On 2017/03/28 16:43:11, Raphael Kubo da Costa (rakuco) wrote:
> PTAL. This is a work in progress, and I wanted to get feedback from you before
> sending a patch to add new API to V8 (and writing layout tests for this).
> 
> The CL description should cover it all, I'm just not sure if adding an {% if
> interface_name == "Iterable" %} is the way to go here. I couldn't think of any
> other way for this to work, as it's only the Iterator interface that needs
this
> special treatment, with its prototype inheriting from something other than
> %ObjectPrototype%. The way this is achieved with V8's API looks quite hackish
as
> well, but it's basically due to the way V8's API normally works and how it
does
> not expose intrinsic objects in its public interface.

I'm fine with the {% if interface_name == 'Iterator' %}.

[Yuki, 2017-03-29 13:37:22]

On 2017/03/29 09:08:04, haraken wrote:
> On 2017/03/28 16:43:11, Raphael Kubo da Costa (rakuco) wrote:
> > PTAL. This is a work in progress, and I wanted to get feedback from you
before
> > sending a patch to add new API to V8 (and writing layout tests for this).
> > 
> > The CL description should cover it all, I'm just not sure if adding an {% if
> > interface_name == "Iterable" %} is the way to go here. I couldn't think of
any
> > other way for this to work, as it's only the Iterator interface that needs
> this
> > special treatment, with its prototype inheriting from something other than
> > %ObjectPrototype%. The way this is achieved with V8's API looks quite
hackish
> as
> > well, but it's basically due to the way V8's API normally works and how it
> does
> > not expose intrinsic objects in its public interface.
> 
> I'm fine with the {% if interface_name == 'Iterator' %}.

I'm fine with it, too.  And the approach looks good to me.

[bashi, 2017-03-29 23:13:04]

On 2017/03/29 13:37:22, Yuki wrote:
> On 2017/03/29 09:08:04, haraken wrote:
> > On 2017/03/28 16:43:11, Raphael Kubo da Costa (rakuco) wrote:
> > > PTAL. This is a work in progress, and I wanted to get feedback from you
> before
> > > sending a patch to add new API to V8 (and writing layout tests for this).
> > > 
> > > The CL description should cover it all, I'm just not sure if adding an {%
if
> > > interface_name == "Iterable" %} is the way to go here. I couldn't think of
> any
> > > other way for this to work, as it's only the Iterator interface that needs
> > this
> > > special treatment, with its prototype inheriting from something other than
> > > %ObjectPrototype%. The way this is achieved with V8's API looks quite
> hackish
> > as
> > > well, but it's basically due to the way V8's API normally works and how it
> > does
> > > not expose intrinsic objects in its public interface.
> > 
> > I'm fine with the {% if interface_name == 'Iterator' %}.
> 
> I'm fine with it, too.  And the approach looks good to me.

The approach looks good to me.

[Raphael Kubo da Costa (rakuco), 2017-03-30 14:33:15]

If anyone else's following this, I sent
https://codereview.chromium.org/2784543004/ to expose %IteratorPrototype% in V8.

If past experience is indicative of anything, getting anyone to review that
patch is going to be tough :(

[haraken, 2017-03-30 14:39:59]

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

[haraken, 2017-03-30 14:39:59]

> If anyone else's following this, I sent
https://codereview.chromium.org/2784543004/ to expose %IteratorPrototype% in V8.
>
> If past experience is indicative of anything, getting anyone to review that
patch is going to be tough :(

Toon, would you help review that V8-side patch?

[Raphael Kubo da Costa (rakuco), 2017-04-07 08:35:35]

Description was changed from

==========
WIP Make pair iterators inherit from %IteratorPrototype%.

Adjust the prototype chain for iterator objects returned by interfaces that
have pair iterators.

The WebIDL spec says when an interface has pair iterators the iterators it
returns must be instances of the "default iterator object" whose
[[Prototype]] points to an "iterator prototype object" whose [[Prototype]],
on its turn, points to %IteratorPrototype%. next() must be implemented in
the "iterator prototype object", while %IteratorPrototype% provides
@@iterator.

In practice, this means the following should return true but was returning
false:

    Object.getPrototypeOf(Object.getPrototypeOf(new Headers().entries())) ===
Object.getPrototypeOf(Object.getPrototypeOf([][Symbol.iterator]()))

Previously, we just had the Iterator interface with both next() and
@@iterator added to its prototype object, which just inherited from
Object.prototype. We now adhere to the spec by making Iterator's prototype
object inherit by %IteratorPrototype% (which then inherits from
Object.prototype) instead. It also allows us to remove support for the
non-standard "Iterable" extended attribute we had in Blink.

The way we do it is not very pretty though: when the code for the Iterator
interface is generated, we create another function template with no
prototype and set the "prototype" property of its function object. When
|interfaceTemplate| is instantiated, its prototype.__proto__ will point to
the value of the "prototype" property we set, which is exactly what we want.

BUG=689576
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org
==========

to

==========
Make pair iterators inherit from %IteratorPrototype%.

Adjust the prototype chain for iterator objects returned by interfaces that
have pair iterators.

The WebIDL spec says when an interface has pair iterators the iterators it
returns must be instances of the "default iterator object" whose
[[Prototype]] points to an "iterator prototype object" whose [[Prototype]],
on its turn, points to %IteratorPrototype%. next() must be implemented in
the "iterator prototype object", while %IteratorPrototype% provides
@@iterator.

In practice, this means the following should return true but was returning
false:

    Object.getPrototypeOf(Object.getPrototypeOf(new Headers().entries())) ===
Object.getPrototypeOf(Object.getPrototypeOf([][Symbol.iterator]()))

Previously, we just had the Iterator interface with both next() and
@@iterator added to its prototype object, which just inherited from
Object.prototype. We now adhere to the spec by making Iterator's prototype
object inherit by %IteratorPrototype% (which then inherits from
Object.prototype) instead. It also allows us to remove support for the
non-standard "Iterable" extended attribute we had in Blink.

The way we do it is not very pretty though: when the code for the Iterator
interface is generated, we create another function template with no
prototype and set the "prototype" property of its function object. When
|interfaceTemplate| is instantiated, its prototype.__proto__ will point to
the value of the "prototype" property we set, which is exactly what we want.

BUG=689576
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org
==========

[Raphael Kubo da Costa (rakuco), 2017-04-07 08:37:36]

raphael.kubo.da.costa@intel.com changed reviewers:
- verwaest@chromium.org

[Raphael Kubo da Costa (rakuco), 2017-04-07 08:37:37]

The V8 side of the patch has finally landed, so I'm updating this CL with some
tests I wrote. This is no longer a WIP, please take a look :)

[haraken, 2017-04-07 09:13:47]

LGTM

Great CL description & comment :)

[Yuki, 2017-04-07 09:25:03]

LGTM!

https://codereview.chromium.org/2777183004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
(right):

https://codereview.chromium.org/2777183004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:536: // The
WebIDL spec says when an interface has pair iterators the iterators it returns
must be
nit: Could you wrap lines within 80 columns?

https://codereview.chromium.org/2777183004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:551:
v8::Local<v8::FunctionTemplate> iteratorPrototypeTemplate =
v8::FunctionTemplate::New(isolate);
nit:

This is okay, but let me elaborate something.

AFAIK, V8 GC does not collect FunctionTemplates, so they can cause a leak.  It's
Blink's responsibility not to create FunctionTemplates unnecessarily too much.

At a glance, this code looks like creating a new FunctionTemplate on every call
(so this looks no good), however, actually this is okay because:
1) |interfaceTemplate| is cached in V8PerIsolateData, so we create only one
FunctionTemplate per interface, and
2) there is only one interface "Iterator" that creates this FunctionTemplate, so
no need to reuse this FunctionTemplate.

If we had two or more interfaces that use this FunctionTemplate, we should
register this FunctionTemplate in V8PerIsolateData (c.f.
V8PerIsolateData::{find,set}InterfaceTemplate), and reuse it.

So, probably it's better to comment that we don't need to cache this
FunctionTemplate in V8PerIsolateData because only "Iterator" needs it.

https://codereview.chromium.org/2777183004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:551:
v8::Local<v8::FunctionTemplate> iteratorPrototypeTemplate =
v8::FunctionTemplate::New(isolate);
nit: The name is a bit confusing.

Let me suppose
  F = iteratorPrototypeTemplate
then, I'd say that F.PrototypeTemplate() is supposed to be the intrinsic
iterator prototype *template* (but actually not, so we remove it) and
F.Get("prototype") is the intrinsic iterator prototype.  F itself is a (dummy)
interface template, and not a prototype object nor template.

What about intrinsicIteratorInterfaceTemplate for example?
(intrinsicIteratorPrototypesDummyInterfaceTemplate is too long, I think).  I'm
open to other options.

[bashi, 2017-04-07 09:33:31]

(yukishiino@ and haraken@ are right reviewers and you don't have to wait for my
review)

[Raphael Kubo da Costa (rakuco), 2017-04-07 09:45:46]

https://codereview.chromium.org/2777183004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
(right):

https://codereview.chromium.org/2777183004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:536: // The
WebIDL spec says when an interface has pair iterators the iterators it returns
must be
On 2017/04/07 09:25:03, Yuki wrote:
> nit: Could you wrap lines within 80 columns?

Done.

https://codereview.chromium.org/2777183004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:551:
v8::Local<v8::FunctionTemplate> iteratorPrototypeTemplate =
v8::FunctionTemplate::New(isolate);
On 2017/04/07 09:25:03, Yuki wrote:
> nit: The name is a bit confusing.
> 
> Let me suppose
>   F = iteratorPrototypeTemplate
> then, I'd say that F.PrototypeTemplate() is supposed to be the intrinsic
> iterator prototype *template* (but actually not, so we remove it) and
> F.Get("prototype") is the intrinsic iterator prototype.  F itself is a (dummy)
> interface template, and not a prototype object nor template.
> 
> What about intrinsicIteratorInterfaceTemplate for example?
> (intrinsicIteratorPrototypesDummyInterfaceTemplate is too long, I think).  I'm
> open to other options.

How about intrinsicIteratorPrototypeInterfaceTemplate? It's quite long but still
fits 80 columns :-)

https://codereview.chromium.org/2777183004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:551:
v8::Local<v8::FunctionTemplate> iteratorPrototypeTemplate =
v8::FunctionTemplate::New(isolate);
On 2017/04/07 09:25:03, Yuki wrote:
> nit:
> 
> This is okay, but let me elaborate something.
> 
> AFAIK, V8 GC does not collect FunctionTemplates, so they can cause a leak. 
It's
> Blink's responsibility not to create FunctionTemplates unnecessarily too much.
> 
> At a glance, this code looks like creating a new FunctionTemplate on every
call
> (so this looks no good), however, actually this is okay because:
> 1) |interfaceTemplate| is cached in V8PerIsolateData, so we create only one
> FunctionTemplate per interface, and
> 2) there is only one interface "Iterator" that creates this FunctionTemplate,
so
> no need to reuse this FunctionTemplate.
> 
> If we had two or more interfaces that use this FunctionTemplate, we should
> register this FunctionTemplate in V8PerIsolateData (c.f.
> V8PerIsolateData::{find,set}InterfaceTemplate), and reuse it.
> 
> So, probably it's better to comment that we don't need to cache this
> FunctionTemplate in V8PerIsolateData because only "Iterator" needs it.

Thanks for the explanation! I'm still wrapping my head around all the
isolate+context combinations in the bindings (and the multiple Script* classes),
so an explanation like that is very useful :-)

I'm adding a comment here that should hopefully clarify this to other readers.

[Raphael Kubo da Costa (rakuco), 2017-04-07 09:47:02]

Patch v3 is up, hopefully addressing Yuki's comments.

[Yuki, 2017-04-07 09:49:12]

LGTM.

https://codereview.chromium.org/2777183004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
(right):

https://codereview.chromium.org/2777183004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:558: // -
|interfaceTemplate| is is cached in V8PerIsolateData, so we create only
typo: s/is is/is/

[Raphael Kubo da Costa (rakuco), 2017-04-07 09:57:50]

Patch v4 is up with hopefully no typos :-)

I intend to land this next week to give time for the V8 CL to stick.

[Yuki, 2017-04-07 10:08:24]

The plan LGTM.

[Raphael Kubo da Costa (rakuco), 2017-04-10 09:04:46]

The CQ bit was checked by raphael.kubo.da.costa@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 09:04:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 09:15:08]

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

[commit-bot: I haz the power, 2017-04-10 09:15:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Raphael Kubo da Costa (rakuco), 2017-04-10 09:32:19]

The CQ bit was checked by raphael.kubo.da.costa@intel.com

[Raphael Kubo da Costa (rakuco), 2017-04-10 09:32:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2777183004/#ps100001
(title: "Fix the build after The Blink Rename")

[commit-bot: I haz the power, 2017-04-10 09:32:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 11:54:13]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491816739348280,
"parent_rev": "d09bddfdb6f6382732f654ca6273d4088c246d51", "commit_rev":
"6579200aa9189bce1cd61c05fb6eb4493f2302d8"}

[commit-bot: I haz the power, 2017-04-10 11:55:07]

Description was changed from

==========
Make pair iterators inherit from %IteratorPrototype%.

Adjust the prototype chain for iterator objects returned by interfaces that
have pair iterators.

The WebIDL spec says when an interface has pair iterators the iterators it
returns must be instances of the "default iterator object" whose
[[Prototype]] points to an "iterator prototype object" whose [[Prototype]],
on its turn, points to %IteratorPrototype%. next() must be implemented in
the "iterator prototype object", while %IteratorPrototype% provides
@@iterator.

In practice, this means the following should return true but was returning
false:

    Object.getPrototypeOf(Object.getPrototypeOf(new Headers().entries())) ===
Object.getPrototypeOf(Object.getPrototypeOf([][Symbol.iterator]()))

Previously, we just had the Iterator interface with both next() and
@@iterator added to its prototype object, which just inherited from
Object.prototype. We now adhere to the spec by making Iterator's prototype
object inherit by %IteratorPrototype% (which then inherits from
Object.prototype) instead. It also allows us to remove support for the
non-standard "Iterable" extended attribute we had in Blink.

The way we do it is not very pretty though: when the code for the Iterator
interface is generated, we create another function template with no
prototype and set the "prototype" property of its function object. When
|interfaceTemplate| is instantiated, its prototype.__proto__ will point to
the value of the "prototype" property we set, which is exactly what we want.

BUG=689576
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org
==========

to

==========
Make pair iterators inherit from %IteratorPrototype%.

Adjust the prototype chain for iterator objects returned by interfaces that
have pair iterators.

The WebIDL spec says when an interface has pair iterators the iterators it
returns must be instances of the "default iterator object" whose
[[Prototype]] points to an "iterator prototype object" whose [[Prototype]],
on its turn, points to %IteratorPrototype%. next() must be implemented in
the "iterator prototype object", while %IteratorPrototype% provides
@@iterator.

In practice, this means the following should return true but was returning
false:

    Object.getPrototypeOf(Object.getPrototypeOf(new Headers().entries())) ===
Object.getPrototypeOf(Object.getPrototypeOf([][Symbol.iterator]()))

Previously, we just had the Iterator interface with both next() and
@@iterator added to its prototype object, which just inherited from
Object.prototype. We now adhere to the spec by making Iterator's prototype
object inherit by %IteratorPrototype% (which then inherits from
Object.prototype) instead. It also allows us to remove support for the
non-standard "Iterable" extended attribute we had in Blink.

The way we do it is not very pretty though: when the code for the Iterator
interface is generated, we create another function template with no
prototype and set the "prototype" property of its function object. When
|interfaceTemplate| is instantiated, its prototype.__proto__ will point to
the value of the "prototype" property we set, which is exactly what we want.

BUG=689576
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org

Review-Url: https://codereview.chromium.org/2777183004
Cr-Commit-Position: refs/heads/master@{#463224}
Committed:
https://chromium.googlesource.com/chromium/src/+/6579200aa9189bce1cd61c05fb6e...
==========

[commit-bot: I haz the power, 2017-04-10 11:55:08]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/6579200aa9189bce1cd61c05fb6e...