IDL: ArrayBuffer and ArrayBufferView support for union types

IDL: ArrayBuffer and ArrayBufferView support for union types

(ArrayBuffer or ArrayBufferView) is a common union type usage. For example,
CSS font loading[1] and fetch API[2] use it. Implement and use them for
FontFace constructor.

Change for FontFace is covered by fast/css/fontface-arraybuffer.html

This CL also fixed two minor bugs:
- Collect union types from constructors (we didn't before)
- We need to use 'implemented_as' for forward class declarations, instead of
  'name'.

[1] http://dev.w3.org/csswg/css-font-loading/#fontface-interface
[2] https://fetch.spec.whatwg.org/#body-mixin

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

[bashi, 2014-11-04 00:48:01]

Patchset #1 (id:1) has been deleted

[bashi, 2014-11-04 01:17:03]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org, jl@opera.com, ksakamoto@chromium.org,
yukishiino@chromium.org

[bashi, 2014-11-04 01:17:03]

Haraken-san, Jens, Shiino-san: PTAL?

Sakamoto-san: FYI

https://codereview.chromium.org/699713003/diff/20001/Source/bindings/tests/re...
File Source/bindings/tests/results/core/UnionTypesCore.cpp (right):

https://codereview.chromium.org/699713003/diff/20001/Source/bindings/tests/re...
Source/bindings/tests/results/core/UnionTypesCore.cpp:233: void
V8StringOrArrayBufferOrArrayBufferView::toImpl(v8::Isolate* isolate,
v8::Handle<v8::Value> v8Value, StringOrArrayBufferOrArrayBufferView& impl,
ExceptionState& exceptionState)
shiino-san: could you please  take a look at this method? This method does V8 ->
impl conversion.

[haraken, 2014-11-04 02:20:14]

LGTM

https://codereview.chromium.org/699713003/diff/20001/Source/bindings/template...
File Source/bindings/templates/union.cpp (right):

https://codereview.chromium.org/699713003/diff/20001/Source/bindings/template...
Source/bindings/templates/union.cpp:10: {% macro
install_hasinstance_check(member) %}

"install_" sounds a bit strange. hasinstance_check or
to_impl_with_hasinstance_check ?

We normally use "install" to install properties onto V8DOMConfiguration.

[bashi, 2014-11-04 02:30:32]

Thank you for review!

https://codereview.chromium.org/699713003/diff/20001/Source/bindings/template...
File Source/bindings/templates/union.cpp (right):

https://codereview.chromium.org/699713003/diff/20001/Source/bindings/template...
Source/bindings/templates/union.cpp:10: {% macro
install_hasinstance_check(member) %}
On 2014/11/04 02:20:14, haraken wrote:
> 
> "install_" sounds a bit strange. hasinstance_check or
> to_impl_with_hasinstance_check ?
> 
> We normally use "install" to install properties onto V8DOMConfiguration.

Done.

[Yuki, 2014-11-04 06:35:14]

lgtm with minor comments.

https://codereview.chromium.org/699713003/diff/20001/Source/bindings/template...
File Source/bindings/templates/union.cpp (right):

https://codereview.chromium.org/699713003/diff/20001/Source/bindings/template...
Source/bindings/templates/union.cpp:10: {% macro
install_hasinstance_check(member) %}
On 2014/11/04 02:30:32, bashi1 wrote:
> On 2014/11/04 02:20:14, haraken wrote:
> > 
> > "install_" sounds a bit strange. hasinstance_check or
> > to_impl_with_hasinstance_check ?
> > 
> > We normally use "install" to install properties onto V8DOMConfiguration.
> 
> Done.

We already have to_impl_with_type_check block in interface.cpp, and it seems a
little confusing.

Plus, the essential part of the macro is not purely toImpl.  The macro is
essentially:
    impl.setType(V8Type::toImpl(v8Value));
which is essentially:
    impl = V8Type::toImpl(v8Value);
which includes an assignment in addition to toImpl.

So I'd prefer another name which represents/implies an assignment.  If 'install'
is not good, can we use 'set', 'assign' or something else?

Also I'd recommend to specify arguments for the input and output.
  {% macro foo(impl, v8Value, member) %}
  impl.setType(V8Type::toImpl(v8Value));
  {% endmacro %}

  {# A caller knows the actual input and output. #}
  {{ foo('impl', 'v8Value', member) }}
It makes scope of variables clearer.

https://codereview.chromium.org/699713003/diff/40001/Source/bindings/tests/re...
File Source/bindings/tests/results/core/UnionTypesCore.cpp (right):

https://codereview.chromium.org/699713003/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/UnionTypesCore.cpp:261: if (impl.isNull())
Why don't you use switch instead of if?

[Jens Widell, 2014-11-04 07:22:56]

LGTM

https://codereview.chromium.org/699713003/diff/20001/Source/bindings/template...
File Source/bindings/templates/union.cpp (right):

https://codereview.chromium.org/699713003/diff/20001/Source/bindings/template...
Source/bindings/templates/union.cpp:10: {% macro
install_hasinstance_check(member) %}
On 2014/11/04 06:35:14, Yuki wrote:
> On 2014/11/04 02:30:32, bashi1 wrote:
> > On 2014/11/04 02:20:14, haraken wrote:
> > > 
> > > "install_" sounds a bit strange. hasinstance_check or
> > > to_impl_with_hasinstance_check ?
> > > 
> > > We normally use "install" to install properties onto V8DOMConfiguration.
> > 
> > Done.
> 
> We already have to_impl_with_type_check block in interface.cpp, and it seems a
> little confusing.
> 
> Plus, the essential part of the macro is not purely toImpl.  The macro is
> essentially:
>     impl.setType(V8Type::toImpl(v8Value));
> which is essentially:
>     impl = V8Type::toImpl(v8Value);
> which includes an assignment in addition to toImpl.
> 
> So I'd prefer another name which represents/implies an assignment.  If
'install'
> is not good, can we use 'set', 'assign' or something else?

Oh, naming of things is always fun! :-)

How about "assign_and_return_if_hasinstance"?

[bashi, 2014-11-04 08:35:05]

Thank you for reviews!

https://codereview.chromium.org/699713003/diff/20001/Source/bindings/template...
File Source/bindings/templates/union.cpp (right):

https://codereview.chromium.org/699713003/diff/20001/Source/bindings/template...
Source/bindings/templates/union.cpp:10: {% macro
install_hasinstance_check(member) %}
On 2014/11/04 07:22:56, Jens Widell wrote:
> On 2014/11/04 06:35:14, Yuki wrote:
> > On 2014/11/04 02:30:32, bashi1 wrote:
> > > On 2014/11/04 02:20:14, haraken wrote:
> > > > 
> > > > "install_" sounds a bit strange. hasinstance_check or
> > > > to_impl_with_hasinstance_check ?
> > > > 
> > > > We normally use "install" to install properties onto V8DOMConfiguration.
> > > 
> > > Done.
> > 
> > We already have to_impl_with_type_check block in interface.cpp, and it seems
a
> > little confusing.
> > 
> > Plus, the essential part of the macro is not purely toImpl.  The macro is
> > essentially:
> >     impl.setType(V8Type::toImpl(v8Value));
> > which is essentially:
> >     impl = V8Type::toImpl(v8Value);
> > which includes an assignment in addition to toImpl.
> > 
> > So I'd prefer another name which represents/implies an assignment.  If
> 'install'
> > is not good, can we use 'set', 'assign' or something else?
> 
> Oh, naming of things is always fun! :-)
> 
> How about "assign_and_return_if_hasinstance"?

"assign_and_return_if_hasinstnace" sounds good:) Done.

https://codereview.chromium.org/699713003/diff/40001/Source/bindings/tests/re...
File Source/bindings/tests/results/core/UnionTypesCore.cpp (right):

https://codereview.chromium.org/699713003/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/UnionTypesCore.cpp:261: if (impl.isNull())
On 2014/11/04 06:35:14, Yuki wrote:
> Why don't you use switch instead of if?

Enums are private so it's not available here. Of course we can make this
function a friend function, but such change shouldn't be included in this CL.
Will revisit later.

[bashi, 2014-11-04 08:49:53]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2014-11-04 08:50:48]

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

[commit-bot: I haz the power, 2014-11-04 09:49:17]

Committed patchset #3 (id:60001) as 184824