bindings: Make the sequence conversion code more compliant with WebIDL.

bindings: Make the sequence conversion code more compliant with WebIDL.

The existing bindings still referenced WebIDL array types that were removed
from the spec in 2015, and converting JS types to IDL sequences with an
algorithm that expected either a JS array (in which case it just iterated
over each element) or an object that adhered to an outdated protocol where a
"length" property was expected.

The current WebIDL sequence conversion steps are simpler and described in
https://heycam.github.io/webidl/#es-sequence. In short, JS objects are
converted to sequences via @@iterator and the regular iteration protocol of
calling next() and checking the returned object.

This works with arrays as well as any object that implements a valid
Symbol.iterator property. It opens the way for custom iterators, the lack of
which was causing some layout tests to fail. In other words, any iterable JS
object can now be converted into a sequence<>.

The downside is that the Blink-V8 communication required for calling
@@iterator, invoking next() and checking objects on every iteration step is
costlier than what we had before, so the "fast case" has been preserved for
JS arrays (even though it is not 100% clear if this is a valid shortcut to
take, it at least does not introduce any regression compared to the previous
code).

This is part 1: several functions in V8Binding.h that implement the old
conversion algorithm (e.g. ToMemberNativeArray(), ToImplArray(),
ToV8Sequence() and ToImplSequence()) have not been removed yet, and their
callers in the code base have not yet been changed. This will come at a
later point in the future.

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

Review-Url: https://codereview.chromium.org/2826673004
Cr-Commit-Position: refs/heads/master@{#465649}
Committed: https://chromium.googlesource.com/chromium/src/+/941cc09b668fbbe90f3e8fd65e8773e1d38bd66f

[Raphael Kubo da Costa (rakuco), 2017-04-18 16:56:19]

PTAL. This was also spun off https://codereview.chromium.org/2810843002/. I'm
following the suggestion bashi@ made there and separating the changes to the
bindings themselves from the removal of code from V8Binding.h and changing their
callers.

As with the original CL, the biggest drawback is that the fast path for JS
arrays most likely does not take all the factors it should into consideration:
see domenic's comment in
https://bugs.chromium.org/p/chromium/issues/detail?id=690428#c11 and his w-p-t
bug https://github.com/w3c/web-platform-tests/issues/5566 on everything that
needs to be inspected. Doing this probably requires help from V8, so at the
moment all I am doing is not changing the existing (incomplete) heuristics for
arrays so that this change at least does not make things worse compared to what
we had before.

[Yuki, 2017-04-19 12:31:25]

LGTM.

[Raphael Kubo da Costa (rakuco), 2017-04-19 14:43:16]

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-19 14:43:42]

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

[haraken, 2017-04-19 15:11:31]

LGTM

[Raphael Kubo da Costa (rakuco), 2017-04-19 15:20:43]

Description was changed from

==========
bindings: Make the sequence conversion code more compliant with WebIDL.

The existing bindings still referenced WebIDL array types that were removed
from the spec in 2015, and converting JS types to IDL sequences with an
algorithm that expected either a JS array (in which case it just iterated
over each element) or an object that adhered to an outdated protocol where a
"length" property was expected.

The current WebIDL sequence conversion steps are simpler and described in
https://heycam.github.io/webidl/#es-sequence. In short, JS objects are
converted to sequences via @@iterator and the regular iteration protocol of
calling next() and checking the returned object.

This works with arrays as well as any object that implements a valid
Symbol.iterator property. It opens the way for custom iterators, the lack of
which was causing some layout tests to fail. In other words, any iterable JS
object can now be converted into a sequence<>.

The downside is that the Blink-V8 communication required for calling
@@iterator, invoking next() and checking objects on every iteration step is
costlier than what we had before, so the "fast case" has been preserved for
JS arrays (even though it is not 100% clear if this is a valid shortcut to
take, it at least does not introduce any regression compared to the previous
code).

This is part 1: several functions in V8Binding.h that implement the old
conversion algorithm (e.g. ToMemberNativeArray(), ToImplArray(),
ToV8Sequence() and ToImplSequence()) have not been removed yet, and their
callers in the code base have not yet been changed. This will come at a
later point in the future.

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

to

==========
bindings: Make the sequence conversion code more compliant with WebIDL.

The existing bindings still referenced WebIDL array types that were removed
from the spec in 2015, and converting JS types to IDL sequences with an
algorithm that expected either a JS array (in which case it just iterated
over each element) or an object that adhered to an outdated protocol where a
"length" property was expected.

The current WebIDL sequence conversion steps are simpler and described in
https://heycam.github.io/webidl/#es-sequence. In short, JS objects are
converted to sequences via @@iterator and the regular iteration protocol of
calling next() and checking the returned object.

This works with arrays as well as any object that implements a valid
Symbol.iterator property. It opens the way for custom iterators, the lack of
which was causing some layout tests to fail. In other words, any iterable JS
object can now be converted into a sequence<>.

The downside is that the Blink-V8 communication required for calling
@@iterator, invoking next() and checking objects on every iteration step is
costlier than what we had before, so the "fast case" has been preserved for
JS arrays (even though it is not 100% clear if this is a valid shortcut to
take, it at least does not introduce any regression compared to the previous
code).

This is part 1: several functions in V8Binding.h that implement the old
conversion algorithm (e.g. ToMemberNativeArray(), ToImplArray(),
ToV8Sequence() and ToImplSequence()) have not been removed yet, and their
callers in the code base have not yet been changed. This will come at a
later point in the future.

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

[commit-bot: I haz the power, 2017-04-19 17:00:41]

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

[commit-bot: I haz the power, 2017-04-19 17:00:41]

Dry run: This issue passed the CQ dry run.

[Raphael Kubo da Costa (rakuco), 2017-04-19 17:12:48]

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

[commit-bot: I haz the power, 2017-04-19 17:13:15]

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-19 17:21:09]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492621968448120,
"parent_rev": "1137b41f7fc845c2e7c6555c1d6dee6f75fa8236", "commit_rev":
"941cc09b668fbbe90f3e8fd65e8773e1d38bd66f"}

[commit-bot: I haz the power, 2017-04-19 17:21:19]

Description was changed from

==========
bindings: Make the sequence conversion code more compliant with WebIDL.

The existing bindings still referenced WebIDL array types that were removed
from the spec in 2015, and converting JS types to IDL sequences with an
algorithm that expected either a JS array (in which case it just iterated
over each element) or an object that adhered to an outdated protocol where a
"length" property was expected.

The current WebIDL sequence conversion steps are simpler and described in
https://heycam.github.io/webidl/#es-sequence. In short, JS objects are
converted to sequences via @@iterator and the regular iteration protocol of
calling next() and checking the returned object.

This works with arrays as well as any object that implements a valid
Symbol.iterator property. It opens the way for custom iterators, the lack of
which was causing some layout tests to fail. In other words, any iterable JS
object can now be converted into a sequence<>.

The downside is that the Blink-V8 communication required for calling
@@iterator, invoking next() and checking objects on every iteration step is
costlier than what we had before, so the "fast case" has been preserved for
JS arrays (even though it is not 100% clear if this is a valid shortcut to
take, it at least does not introduce any regression compared to the previous
code).

This is part 1: several functions in V8Binding.h that implement the old
conversion algorithm (e.g. ToMemberNativeArray(), ToImplArray(),
ToV8Sequence() and ToImplSequence()) have not been removed yet, and their
callers in the code base have not yet been changed. This will come at a
later point in the future.

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

to

==========
bindings: Make the sequence conversion code more compliant with WebIDL.

The existing bindings still referenced WebIDL array types that were removed
from the spec in 2015, and converting JS types to IDL sequences with an
algorithm that expected either a JS array (in which case it just iterated
over each element) or an object that adhered to an outdated protocol where a
"length" property was expected.

The current WebIDL sequence conversion steps are simpler and described in
https://heycam.github.io/webidl/#es-sequence. In short, JS objects are
converted to sequences via @@iterator and the regular iteration protocol of
calling next() and checking the returned object.

This works with arrays as well as any object that implements a valid
Symbol.iterator property. It opens the way for custom iterators, the lack of
which was causing some layout tests to fail. In other words, any iterable JS
object can now be converted into a sequence<>.

The downside is that the Blink-V8 communication required for calling
@@iterator, invoking next() and checking objects on every iteration step is
costlier than what we had before, so the "fast case" has been preserved for
JS arrays (even though it is not 100% clear if this is a valid shortcut to
take, it at least does not introduce any regression compared to the previous
code).

This is part 1: several functions in V8Binding.h that implement the old
conversion algorithm (e.g. ToMemberNativeArray(), ToImplArray(),
ToV8Sequence() and ToImplSequence()) have not been removed yet, and their
callers in the code base have not yet been changed. This will come at a
later point in the future.

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

Review-Url: https://codereview.chromium.org/2826673004
Cr-Commit-Position: refs/heads/master@{#465649}
Committed:
https://chromium.googlesource.com/chromium/src/+/941cc09b668fbbe90f3e8fd65e87...
==========

[commit-bot: I haz the power, 2017-04-19 17:21:21]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/941cc09b668fbbe90f3e8fd65e87...

[bashi, 2017-04-19 23:14:37]

lgtm (sorry for late response; I'm OOO every Wed in JST)

[Nico, 2017-05-08 20:24:46]

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

[Nico, 2017-05-08 20:24:46]

minor style nit:

https://codereview.chromium.org/2826673004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/SequenceTest.h (right):

https://codereview.chromium.org/2826673004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/SequenceTest.h:44:
HeapVector<Member<Element>> m_elementSequence;
blink now uses style that's a lot closer to chromium style. Namely, capitalize
member functions, and call member variables like_this_ instead of m_likeThis.
(The method capitalization isn't done for IDL-exposed functions which at least
some of the methods in here are, but the member variable at least is wrong.)

[Raphael Kubo da Costa (rakuco), 2017-05-09 14:48:48]

On 2017/05/08 20:24:46, Nico wrote:
> minor style nit:
> 
>
https://codereview.chromium.org/2826673004/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/testing/SequenceTest.h (right):
> 
>
https://codereview.chromium.org/2826673004/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/testing/SequenceTest.h:44:
> HeapVector<Member<Element>> m_elementSequence;
> blink now uses style that's a lot closer to chromium style. Namely, capitalize
> member functions, and call member variables like_this_ instead of m_likeThis.
> (The method capitalization isn't done for IDL-exposed functions which at least
> some of the methods in here are, but the member variable at least is wrong.)

Thanks, I've sent https://codereview.chromium.org/2871933002 to fix the style
there.