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

bindings: Make the sequence conversion code more complaint 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 change is implemented by moving all the conversion code to
NativeValueTraits like we already do for WebIDL record types, and making the
Python WebIDL compiler code use it instead. This allows us to get rid of:
- 2 ToMemberNativeArray() overloads
- 2 ToImplArray() overloads
- ToV8Sequence()
- ToImplSequence()
- 5 deprecated NativeValueTraits specializations
in addition to simplifying the Python code in v8_types.py by making it less
smart.

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

[Raphael Kubo da Costa (rakuco), 2017-04-10 17:27:58]

PTAL.

I probably still need to adjust some layout test expectations, but the actual
code should be ready. I'd like some feedback before asking for the owners of the
non-bindings bits to chime in.

The patch is quite big (I can try to split it up if you prefer), but it
essentially moves the JS->sequence conversion code to NativeValueTratsImpl.h and
adjusts callers as well as the WebIDL compiler. I was able to get rid of a lot
of code in V8Binding.h and simplify v8_types.py, both of which had always
bothered me before :-)

[jsbell, 2017-04-10 21:09:23]

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

[jsbell, 2017-04-10 21:09:24]

Drive-by (since I'll probably be on tap later anyway...)

I was looking at the storage/websql errors. 

* storage/websql/execute-sql-args.html - this one passes {length: 0} etc to
probe the legacy "looks iterable" handling. We can update the test.

... but most of the other tests are failing/timing out. I noticed a possible
glitch in the CL (noted inline) but do run the storage/websql tests explicitly
and make sure they pass.

https://codereview.chromium.org/2810843002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webdatabase/SQLTransaction.cpp (left):

https://codereview.chromium.org/2810843002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webdatabase/SQLTransaction.cpp:323:
ExecuteSQL(sql_statement, sql_values, callback, callback_error,
What happened to this ExecuteSQL() call?

[bashi, 2017-04-11 00:34:00]

Great improvement!

Generally looks good to me. I'd prefer splitting this CL into small parts.
Maybe:
- Update NativeValueTraits<IDLSequence>::NativeValue(), add tests, and use it in
the code generator
- Replace callers outside the binding layer and remove old code defined in
V8Binding.h

Also, how did you check this CL doesn't regress?

Your CL description is always awesome BTW :)

https://codereview.chromium.org/2810843002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h (right):

https://codereview.chromium.org/2810843002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:300: if
(value->IsArray()) {
We may want to ask v8 team this?

https://codereview.chromium.org/2810843002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:342: if
(!callback(element))
Help me understand: Why does the callback need to return bool? Isn't it enough
to check exception_state has an exception?

https://codereview.chromium.org/2810843002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:372: //
TODO(bashi): Support callable objects.
nit: My vague recollection is that yukishiino@ said that IsFunction() is always
true when it is callable. Maybe we can remove this comment.

[haraken, 2017-04-11 01:43:54]

The overall approach looks good to me.

Yeah, I'm also interested in the performance impact of this change. Can we
create a micro-benchmark in PerformanceTests/Bindings/ and measure the
performance impact?

[Yuki, 2017-04-11 07:17:11]

The change looks great!!

https://codereview.chromium.org/2810843002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h (right):

https://codereview.chromium.org/2810843002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:300: if
(value->IsArray()) {
On 2017/04/11 00:34:00, bashi wrote:
> We may want to ask v8 team this?

I think that this should be:

  if (value->Get(v8::Symbol::GetIterator) == v8::kArrayProto_values)

because I guess that author script can do the following:

  a = new Array(1);
  a[Symbol.iterator] = function myIter() { ... }

Then, we'd like to fall back to iteratorForEachSlow path.

https://codereview.chromium.org/2810843002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:337: if
(!V8Call(v8Object->Get(isolate->GetCurrentContext(), i), element,
nit: V8Call is deprecated.  Please use V8's ToLocal() or To() instead.

https://codereview.chromium.org/2810843002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:372: //
TODO(bashi): Support callable objects.
On 2017/04/11 00:34:00, bashi wrote:
> nit: My vague recollection is that yukishiino@ said that IsFunction() is
always
> true when it is callable. Maybe we can remove this comment.

Yes, a callable object is a function by definition.

You can simply write:
  if (!next->IsFunction())

https://codereview.chromium.org/2810843002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:377:
v8::Local<v8::Value> nextResult;
nit: s/nextResult/next_result/

Last week end, Blink finally changed the naming convention to Chromium's style. 
(Maybe you already noticed.  Other names look good.)

https://codereview.chromium.org/2810843002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:399: if
(!done->ToBoolean(context).ToLocal(&done_boolean)) {
nit: You can use done->BooleanValue(context).To(&done_boolean).

https://codereview.chromium.org/2810843002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/2810843002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:695: CORE_EXPORT bool
IsV8Sequence(v8::Local<v8::Value>,
nit: If possible, could you make v8::Isolate* the first argument?

We'd like to follow V8's style as much as possible.  V8's style is to put
v8::Isolate* or v8::Context first.

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

Thanks for the feedback, everyone. I've uploaded patch v2, which still contains
all the code I intend to land + code adjustments after the previous review
comments + lots of fixes to layout tests. Once everyone's OK with the code
as-is, I can split it up into smaller CLs. Please take another look.

The only layout test that I currently expect to fail is
external/wpt/webmessaging/with-ports/027.html. I fixed it upstream with
https://github.com/w3c/web-platform-tests/pull/5538, so it should waterfall to
Blink soon.

As for the points I did not address in patch v2:

> Also, how did you check this CL doesn't regress?

I ran the layout tests and the new unit tests I added, basically.

> Yeah, I'm also interested in the performance impact of this change. Can we
create a micro-benchmark in PerformanceTests/Bindings/ and measure the
performance impact?

Will do. This is a first for me, but I'll see what I can do there.

> I think that this should be:
>
>   if (value->Get(v8::Symbol::GetIterator) == v8::kArrayProto_values)
>
> because I guess that author script can do the following:
>
>   a = new Array(1);
>   a[Symbol.iterator] = function myIter() { ... }
>
> Then, we'd like to fall back to iteratorForEachSlow path.

That's what's not clear to me. I tested this on Firefox 52 and Epiphany with
WebKitGTK+ (so possibly a not-so-recent WebKit version), and in both cases when
an array defined @@iterator the code still seemed to prefer iterating via
length. Officially, I think this is a violation of the WebIDL spec, which only
mentions conversions using @@iterator, but I know that at least WebKit ran into
performance problems when implementing only the @@iterator conversion path
(https://bugs.webkit.org/show_bug.cgi?id=166834#c13), and the current code
checks if @@iterator hasn't changed in the array as well as some
JavaScriptCore-stuff which we can't currently do with V8's public API.

My idea here was to keep the existing behavior of just checking if we're dealing
with an Array so that we at least don't regress compared to the old code.

[bashi, 2017-04-13 08:03:12]

Still looks good to me.

On 2017/04/12 15:38:02, Raphael Kubo da Costa (rakuco) wrote:
> Thanks for the feedback, everyone. I've uploaded patch v2, which still
contains
> all the code I intend to land + code adjustments after the previous review
> comments + lots of fixes to layout tests. Once everyone's OK with the code
> as-is, I can split it up into smaller CLs. Please take another look.
> 
> The only layout test that I currently expect to fail is
> external/wpt/webmessaging/with-ports/027.html. I fixed it upstream with
> https://github.com/w3c/web-platform-tests/pull/5538, so it should waterfall to
> Blink soon.
> 
> As for the points I did not address in patch v2:
> 
> > Also, how did you check this CL doesn't regress?
> 
> I ran the layout tests and the new unit tests I added, basically.

Did you check any performance regressions? Maybe we want to run blink_perf
benchmarks. For example,

$ tools/perf/run_benchmark run --browser-executable=out/Release/content_shell
blink_perf.bindings

> 
> > Yeah, I'm also interested in the performance impact of this change. Can we
> create a micro-benchmark in PerformanceTests/Bindings/ and measure the
> performance impact?
> 
> Will do. This is a first for me, but I'll see what I can do there.
> 
> > I think that this should be:
> >
> >   if (value->Get(v8::Symbol::GetIterator) == v8::kArrayProto_values)
> >
> > because I guess that author script can do the following:
> >
> >   a = new Array(1);
> >   a[Symbol.iterator] = function myIter() { ... }
> >
> > Then, we'd like to fall back to iteratorForEachSlow path.
> 
> That's what's not clear to me. I tested this on Firefox 52 and Epiphany with
> WebKitGTK+ (so possibly a not-so-recent WebKit version), and in both cases
when
> an array defined @@iterator the code still seemed to prefer iterating via
> length. Officially, I think this is a violation of the WebIDL spec, which only
> mentions conversions using @@iterator, but I know that at least WebKit ran
into
> performance problems when implementing only the @@iterator conversion path
> (https://bugs.webkit.org/show_bug.cgi?id=166834#c13), and the current code
> checks if @@iterator hasn't changed in the array as well as some
> JavaScriptCore-stuff which we can't currently do with V8's public API.
> 
> My idea here was to keep the existing behavior of just checking if we're
dealing
> with an Array so that we at least don't regress compared to the old code.

[Raphael Kubo da Costa (rakuco), 2017-04-13 17:41:10]

On 2017/04/13 08:03:12, bashi wrote:
> Did you check any performance regressions? Maybe we want to run blink_perf
> benchmarks. For example,
> 
> $ tools/perf/run_benchmark run --browser-executable=out/Release/content_shell
> blink_perf.bindings

I did two runs with the existing code and two runs with this CL. I don't know
how to share the results.html I have with you, but from what I can see there was
no noticeable difference in the results. I did notice a very small difference
with my benchmark from https://codereview.chromium.org/2821493002/, so in patch
v4 I made some adjustments such as getting rid of the callbacks in the
conversion code and a few other minor optimizations that made the results more
similar.

This CL now has another performance test for objects with @@iterator which I
can't add separately. The difference in performance is very noticeable: I got
something like ~25k runs/second for the array case, and ~8k runs/second for the
@@iterator case.

[Yuki, 2017-04-14 08:04:00]

LGTM.  Sorry to be late.

[haraken, 2017-04-17 15:06:03]

On 2017/04/13 17:41:10, rakuco (OoO on the 17th) wrote:
> On 2017/04/13 08:03:12, bashi wrote:
> > Did you check any performance regressions? Maybe we want to run blink_perf
> > benchmarks. For example,
> > 
> > $ tools/perf/run_benchmark run
--browser-executable=out/Release/content_shell
> > blink_perf.bindings
> 
> I did two runs with the existing code and two runs with this CL. I don't know
> how to share the results.html I have with you, but from what I can see there
was
> no noticeable difference in the results. I did notice a very small difference
> with my benchmark from https://codereview.chromium.org/2821493002/, so in
patch
> v4 I made some adjustments such as getting rid of the callbacks in the
> conversion code and a few other minor optimizations that made the results more
> similar.
> 
> This CL now has another performance test for objects with @@iterator which I
> can't add separately. The difference in performance is very noticeable: I got
> something like ~25k runs/second for the array case, and ~8k runs/second for
the
> @@iterator case.

Sounds great.

Are you planning to split PS5 into pieces?

[Raphael Kubo da Costa (rakuco), 2017-04-18 15:55:49]

On 2017/04/17 15:06:03, haraken wrote:
> Are you planning to split PS5 into pieces?

Yes, I'll start working on it this week.

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

Description was changed from

==========
bindings: Make the sequence conversion code more complaint 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 change is implemented by moving all the conversion code to
NativeValueTraits like we already do for WebIDL record types, and making the
Python WebIDL compiler code use it instead. This allows us to get rid of:
- 2 ToMemberNativeArray() overloads
- 2 ToImplArray() overloads
- ToV8Sequence()
- ToImplSequence()
- 5 deprecated NativeValueTraits specializations
in addition to simplifying the Python code in v8_types.py by making it less
smart.

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

to

==========
bindings: Make the sequence conversion code more complaint 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 change is implemented by moving all the conversion code to
NativeValueTraits like we already do for WebIDL record types, and making the
Python WebIDL compiler code use it instead. This allows us to get rid of:
- 2 ToMemberNativeArray() overloads
- 2 ToImplArray() overloads
- ToV8Sequence()
- ToImplSequence()
- 5 deprecated NativeValueTraits specializations
in addition to simplifying the Python code in v8_types.py by making it less
smart.

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

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

The patches are being landed separately, so I guess it's time to close this CL.