bindings: Make some value iterator properties aliases to Array.prototype functions

bindings: Make some value iterator properties aliases to Array.prototype functions

The WebIDL spec states that value iterators in an interface should cause the
following operations to be added and be originally set to the corresponding
property of ECMAScript's %ArrayPrototype%:
* entries
* forEach
* keys
* values

Stop generating custom methods for those operations in the bindings when a
value iterator is defined; instead, just set properties with those names to
the corresponding intrinsics values from V8.

This allows us to get a lot of code for free, avoid duplicating iterator
code in Blink and remove the ValueIterable class altogether from the
bindings code.

However, doing so does require us to adjust non-conforming IDLs:
https://heycam.github.io/webidl/#idl-iterable states that "a value iterator
must only be declared on an interface that supports indexed properties", and
"supports indexed properties" means an interface has an indexed getter
operation as well as an integer-type attribute called "length". Some IDLs in
Blink only had an "iterable<V>" declaration without the accompanying getter
and "length" attribute. In the case of both CSSTransformValue and
CSSUnparsedValue, a spec bug was also filed upstream to tackle this at the
source.

Finally, the bindings generation code now raises a ValueError if an
interface only has an iterable<V> declaration without the accompanying
indexed property getter and length attribute.

BUG=545318, 632935
Review-Url: https://codereview.chromium.org/2683853005
Cr-Commit-Position: refs/heads/master@{#449857}
Committed: https://chromium.googlesource.com/chromium/src/+/c85bc01513148e381d656d88a7f8a773704d6c34

[Raphael Kubo da Costa (rakuco), 2017-02-08 08:05:02]

PTAL. This is the Blink half of the bug mentioned in the CL message.

Even if it looks fine to you, I'll give it a few days before landing so we don't
end up depending on bleeding-edge V8 API in Blink.

[haraken, 2017-02-08 08:25:14]

Do we have a test for this?

[Raphael Kubo da Costa (rakuco), 2017-02-08 08:52:47]

Looking at the failures in the Linux trybot it looks like we do :-) I'll check
if any of the expectation files needs to be updated.

[Raphael Kubo da Costa (rakuco), 2017-02-08 12:28:48]

bindings: Make some value iterator properties aliases to Array.prototype
functions

The WebIDL spec states that value iterators in an interface should cause the
following operations to be added and be originally set to the corresponding
property of ECMAScript's %ArrayPrototype%:
* entries
* forEach
* keys
* values

Stop generating custom methods for those operations in the bindings when a
value iterator is defined; instead, just set properties with those names to
the corresponding intrinsics values from V8.

This allows us to get a lot of code for free, avoid duplicating iterator
code in Blink and remove the ValueIterable class altogether from the
bindings code.

However, doing so does require us to adjust non-conforming IDLs:
https://heycam.github.io/webidl/#idl-iterable states that "a value iterator
must only be declared on an interface that supports indexed properties", and
"supports indexed properties" means an interface has an indexed getter
operation as well as an integer-type attribute called "length". Some IDLs in
Blink only had an "iterable<V>" declaration without the accompanying getter
and "length" attribute. In the case of both CSSTransformValue and
CSSUnparsedValue, a spec bug was also filed upstream to tackle this at the
source.

Finally, the bindings generation code now raises a ValueError if an
interface only has an iterable<V> declaration without the accompanying
indexed property getter and length attribute.

BUG=545318,632935

[Raphael Kubo da Costa (rakuco), 2017-02-08 12:38:44]

raphael.kubo.da.costa@intel.com changed reviewers:
+ meade@chromium.org

[Raphael Kubo da Costa (rakuco), 2017-02-08 12:38:45]

Patch v2 is up; my intention was to land these changes separately, but
apparently that's not possible without breaking some tests in the process so
everything's up now:
* The bindings changes from patch v1 (value iterators now define
entries/forEach/keys/values via V8's intrinsics)
* The ValueIterable class in bindings was removed altogether, as it is no longer
necessary or used anywhere.
* The IDL parser now rejects iterable<V> declarations without an accompanying
indexed getter and a length attribute, as specified in the WebIDL spec.
  - This required me to adjust some IDLs, including some non-test ones. CC'ing
meade@ as an FYI because of the changes to core/css/cssom/ (see also:
https://github.com/w3c/css-houdini-drafts/issues/358)

The changes to the CSS IDLs introduce some size_t -> unsigned long conversions
because of the bindings; this is not the first case, and I wonder if that's OK.

[Raphael Kubo da Costa (rakuco), 2017-02-08 12:42:47]

Description was changed from

==========
bindings: Make some value iterator properties aliases to Array.prototype
functions

The WebIDL spec states that value iterators in an interface should cause the
following operations to be added and be originally set to the corresponding
property of ECMAScript's %ArrayPrototype%:
* entries
* forEach
* keys
* values

Stop generating custom methods for those operations in the bindings when a
value iterator is defined; instead, just set properties with those names to
the corresponding intrinsics values from V8.

BUG=632935
R=haraken@chromium.org,jochen@chromium.org,yukishiino@chromium.org
==========

to

==========
bindings: Make some value iterator properties aliases to Array.prototype
functions

The WebIDL spec states that value iterators in an interface should cause the
following operations to be added and be originally set to the corresponding
property of ECMAScript's %ArrayPrototype%:
* entries
* forEach
* keys
* values

Stop generating custom methods for those operations in the bindings when a
value iterator is defined; instead, just set properties with those names to
the corresponding intrinsics values from V8.

This allows us to get a lot of code for free, avoid duplicating iterator
code in Blink and remove the ValueIterable class altogether from the
bindings code.

However, doing so does require us to adjust non-conforming IDLs:
https://heycam.github.io/webidl/#idl-iterable states that "a value iterator
must only be declared on an interface that supports indexed properties", and
"supports indexed properties" means an interface has an indexed getter
operation as well as an integer-type attribute called "length". Some IDLs in
Blink only had an "iterable<V>" declaration without the accompanying getter
and "length" attribute. In the case of both CSSTransformValue and
CSSUnparsedValue, a spec bug was also filed upstream to tackle this at the
source.

Finally, the bindings generation code now raises a ValueError if an
interface only has an iterable<V> declaration without the accompanying
indexed property getter and length attribute.

BUG=545318,632935
==========

[haraken, 2017-02-08 18:07:02]

On 2017/02/08 12:38:45, Raphael Kubo da Costa (rakuco) wrote:
> Patch v2 is up; my intention was to land these changes separately, but
> apparently that's not possible without breaking some tests in the process so
> everything's up now:
> * The bindings changes from patch v1 (value iterators now define
> entries/forEach/keys/values via V8's intrinsics)
> * The ValueIterable class in bindings was removed altogether, as it is no
longer
> necessary or used anywhere.
> * The IDL parser now rejects iterable<V> declarations without an accompanying
> indexed getter and a length attribute, as specified in the WebIDL spec.
>   - This required me to adjust some IDLs, including some non-test ones. CC'ing
> meade@ as an FYI because of the changes to core/css/cssom/ (see also:
> https://github.com/w3c/css-houdini-drafts/issues/358)
> 
> The changes to the CSS IDLs introduce some size_t -> unsigned long conversions
> because of the bindings; this is not the first case, and I wonder if that's
OK.

Would it be helpful to disable some tests during the process of landing those
patches?

Or are the changes in the CSS IDLs needed even after you land all patches?

[meade_UTC10, 2017-02-09 07:11:05]

On 2017/02/08 18:07:02, haraken wrote:
> On 2017/02/08 12:38:45, Raphael Kubo da Costa (rakuco) wrote:
> > Patch v2 is up; my intention was to land these changes separately, but
> > apparently that's not possible without breaking some tests in the process so
> > everything's up now:
> > * The bindings changes from patch v1 (value iterators now define
> > entries/forEach/keys/values via V8's intrinsics)
> > * The ValueIterable class in bindings was removed altogether, as it is no
> longer
> > necessary or used anywhere.
> > * The IDL parser now rejects iterable<V> declarations without an
accompanying
> > indexed getter and a length attribute, as specified in the WebIDL spec.
> >   - This required me to adjust some IDLs, including some non-test ones.
CC'ing
> > meade@ as an FYI because of the changes to core/css/cssom/ (see also:
> > https://github.com/w3c/css-houdini-drafts/issues/358)
> > 
> > The changes to the CSS IDLs introduce some size_t -> unsigned long
conversions
> > because of the bindings; this is not the first case, and I wonder if that's
> OK.
> 
> Would it be helpful to disable some tests during the process of landing those
> patches?
> 
> Or are the changes in the CSS IDLs needed even after you land all patches?

The CSS IDLs that are being changed are all in the unlaunched CSS Typed OM API -
so I don't think it's a problem. We should make the Typed OM objects conform to
web the IDL spec.

[Yuki, 2017-02-09 08:01:56]

LGTM.

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

https://codereview.chromium.org/2683853005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:520: {%+ if
is_global %}instanceTemplate{% else %}prototypeTemplate{% endif
%}->SetIntrinsicDataProperty(v8AtomicString(isolate, "entries"),
v8::kArrayProto_entries);
This is just fine, but I think [Global] or [PrimaryGlobal] actually never have
iterable<T>.  I think you can write this in the following way.

  {% if is_global %}
#error "iterable<T> on [Global] is not supported."
  {% endif %}
  prototypeTemplate->SetIntrinsicDataProperty(...);

[Raphael Kubo da Costa (rakuco), 2017-02-09 10:14:28]

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

https://codereview.chromium.org/2683853005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:520: {%+ if
is_global %}instanceTemplate{% else %}prototypeTemplate{% endif
%}->SetIntrinsicDataProperty(v8AtomicString(isolate, "entries"),
v8::kArrayProto_entries);
On 2017/02/09 08:01:56, Yuki wrote:
> This is just fine, but I think [Global] or [PrimaryGlobal] actually never have
> iterable<T>.

While I haven't encountered any uses of [Global]/[PrimaryGlobal] + iterable<> in
the wild, the spec does say it's possible.

From https://heycam.github.io/webidl/#es-iterable:
> The location of the property is determined as follows:
> * If the interface was declared with the [Global] or [PrimaryGlobal] extended
attribute, then the property exists on the single object that implements the
interface.
> * Otherwise, if the interface is a consequential interface of a [Global]- or
[PrimaryGlobal]-annotated interface, then the property exists on the single
object that implements the [Global]- or [PrimaryGlobal]-annotated interface as
well as on the consequential interface’s interface prototype object.
> * Otherwise, the property exists solely on the interface’s interface prototype
object.

[Raphael Kubo da Costa (rakuco), 2017-02-09 10:14:42]

On 2017/02/08 18:07:02, haraken wrote:
> Would it be helpful to disable some tests during the process of landing those
> patches?
> 
> Or are the changes in the CSS IDLs needed even after you land all patches?

The IDL changes are necessary even after the changes land, as to me it's the CSS
Typed OM spec that has non-compliant IDL interfaces. While the CL got bigger
compared to v1, IMHO the changes are still all logically connected.

[Yuki, 2017-02-09 13:03:19]

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

https://codereview.chromium.org/2683853005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl:520: {%+ if
is_global %}instanceTemplate{% else %}prototypeTemplate{% endif
%}->SetIntrinsicDataProperty(v8AtomicString(isolate, "entries"),
v8::kArrayProto_entries);
On 2017/02/09 10:14:28, Raphael Kubo da Costa (rakuco) wrote:
> On 2017/02/09 08:01:56, Yuki wrote:
> > This is just fine, but I think [Global] or [PrimaryGlobal] actually never
have
> > iterable<T>.
> 
> While I haven't encountered any uses of [Global]/[PrimaryGlobal] + iterable<>
in
> the wild, the spec does say it's possible.
> 
> From https://heycam.github.io/webidl/#es-iterable:
> > The location of the property is determined as follows:
> > * If the interface was declared with the [Global] or [PrimaryGlobal]
extended
> attribute, then the property exists on the single object that implements the
> interface.
> > * Otherwise, if the interface is a consequential interface of a [Global]- or
> [PrimaryGlobal]-annotated interface, then the property exists on the single
> object that implements the [Global]- or [PrimaryGlobal]-annotated interface as
> well as on the consequential interface’s interface prototype object.
> > * Otherwise, the property exists solely on the interface’s interface
prototype
> object.

Yes, that's true.  My point is that we don't want to introduce unused / untested
code in general.  We can add the support for [Global] later when it's really
needed.  But, meanwhile, we don't need it, we don't use it, and we don't test it
well.  Then, I think we don't want it to be landed.

[Raphael Kubo da Costa (rakuco), 2017-02-09 13:25:52]

Patch v3 is up implementing Yuki's suggestion on interface_base.cpp.tmpl.

[Yuki, 2017-02-09 13:28:44]

Thanks.  Still LGTM.

[haraken, 2017-02-09 14:39:24]

LGTM

[Raphael Kubo da Costa (rakuco), 2017-02-09 14:44:12]

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

[Raphael Kubo da Costa (rakuco), 2017-02-09 14:44:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2683853005/#ps60001
(title: "Rebased patch")

[commit-bot: I haz the power, 2017-02-09 14:44:31]

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

[Raphael Kubo da Costa (rakuco), 2017-02-09 14:44:32]

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

[Raphael Kubo da Costa (rakuco), 2017-02-09 14:45:42]

On 2017/02/09 14:44:31, commit-bot: I haz the power wrote:
> CQ is trying da patch.

Oops, I forgot about what I said about waiting for a few days to see if the V8
changes stick :-) I've unchecked the CQ bit and will land this tomorrow.

[Raphael Kubo da Costa (rakuco), 2017-02-11 10:54:20]

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

[commit-bot: I haz the power, 2017-02-11 10:54:32]

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-02-11 10:57:03]

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

[commit-bot: I haz the power, 2017-02-11 10:57:04]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Raphael Kubo da Costa (rakuco), 2017-02-11 10:59:26]

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

[Raphael Kubo da Costa (rakuco), 2017-02-11 10:59:26]

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/2683853005/#ps80001
(title: "Rebased patch")

[commit-bot: I haz the power, 2017-02-11 10:59:35]

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-02-11 11:23:22]

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

[commit-bot: I haz the power, 2017-02-11 11:23:23]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Raphael Kubo da Costa (rakuco), 2017-02-11 11:32:34]

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

[Raphael Kubo da Costa (rakuco), 2017-02-11 11:32:34]

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/2683853005/#ps100001
(title: "Remove constexpr from Internals.h to fix the Android build")

[commit-bot: I haz the power, 2017-02-11 11:32:46]

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-02-11 12:31:44]

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

[commit-bot: I haz the power, 2017-02-11 12:31:46]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_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-02-11 13:02:57]

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

[commit-bot: I haz the power, 2017-02-11 13:03:07]

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-02-11 13:37:04]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486818177840330,
"parent_rev": "01199c020ca74ac8d32a7d2939d0a090cf08d801", "commit_rev":
"c85bc01513148e381d656d88a7f8a773704d6c34"}

[commit-bot: I haz the power, 2017-02-11 13:37:36]

Description was changed from

==========
bindings: Make some value iterator properties aliases to Array.prototype
functions

The WebIDL spec states that value iterators in an interface should cause the
following operations to be added and be originally set to the corresponding
property of ECMAScript's %ArrayPrototype%:
* entries
* forEach
* keys
* values

Stop generating custom methods for those operations in the bindings when a
value iterator is defined; instead, just set properties with those names to
the corresponding intrinsics values from V8.

This allows us to get a lot of code for free, avoid duplicating iterator
code in Blink and remove the ValueIterable class altogether from the
bindings code.

However, doing so does require us to adjust non-conforming IDLs:
https://heycam.github.io/webidl/#idl-iterable states that "a value iterator
must only be declared on an interface that supports indexed properties", and
"supports indexed properties" means an interface has an indexed getter
operation as well as an integer-type attribute called "length". Some IDLs in
Blink only had an "iterable<V>" declaration without the accompanying getter
and "length" attribute. In the case of both CSSTransformValue and
CSSUnparsedValue, a spec bug was also filed upstream to tackle this at the
source.

Finally, the bindings generation code now raises a ValueError if an
interface only has an iterable<V> declaration without the accompanying
indexed property getter and length attribute.

BUG=545318,632935
==========

to

==========
bindings: Make some value iterator properties aliases to Array.prototype
functions

The WebIDL spec states that value iterators in an interface should cause the
following operations to be added and be originally set to the corresponding
property of ECMAScript's %ArrayPrototype%:
* entries
* forEach
* keys
* values

Stop generating custom methods for those operations in the bindings when a
value iterator is defined; instead, just set properties with those names to
the corresponding intrinsics values from V8.

This allows us to get a lot of code for free, avoid duplicating iterator
code in Blink and remove the ValueIterable class altogether from the
bindings code.

However, doing so does require us to adjust non-conforming IDLs:
https://heycam.github.io/webidl/#idl-iterable states that "a value iterator
must only be declared on an interface that supports indexed properties", and
"supports indexed properties" means an interface has an indexed getter
operation as well as an integer-type attribute called "length". Some IDLs in
Blink only had an "iterable<V>" declaration without the accompanying getter
and "length" attribute. In the case of both CSSTransformValue and
CSSUnparsedValue, a spec bug was also filed upstream to tackle this at the
source.

Finally, the bindings generation code now raises a ValueError if an
interface only has an iterable<V> declaration without the accompanying
indexed property getter and length attribute.

BUG=545318,632935

Review-Url: https://codereview.chromium.org/2683853005
Cr-Commit-Position: refs/heads/master@{#449857}
Committed:
https://chromium.googlesource.com/chromium/src/+/c85bc01513148e381d656d88a7f8...
==========

[commit-bot: I haz the power, 2017-02-11 13:37:38]

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