bindings: Retires manual dispatching in createV8{HTML,SVG}Wrapper, etc.

bindings: Retires manual dispatching in createV8{HTML,SVG}Wrapper, etc.

Retires manual dispatching of 'wrap' in createV8{HTML,SVG}Wrapper in
V8{HTML,SVG}ElementWrapperFactory.cpp and its dependents.

This is part of the V8 binding refactoring (Issue 235436) and also
this CL fixes Issue 408160 by removing wrong downcasts by toHTMLUnknownElement().

There are much more code to be removed as part of the refactoring, but let me remove the code in this CL first.  Will continue to remove more unnecessary code.

BUG=408160, 235436
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181440

[Yuki, 2014-09-03 14:08:46]

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

[Yuki, 2014-09-03 14:08:47]

Could you review this CL?

[haraken, 2014-09-03 14:58:52]

This is a fantastic CL!! This is a much more secure way to dispatch wrap methods
of correct type.

Let me take a close look tomorrow.

[haraken, 2014-09-04 02:17:46]

https://codereview.chromium.org/531183003/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8ImageDataCustom.cpp (left):

https://codereview.chromium.org/531183003/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8ImageDataCustom.cpp:49:
wrapper->ForceSet(v8AtomicString(isolate, "data"), pixelArray, v8::ReadOnly);

It looks wrong to remove this code. We still need this. That's why a couple of
ImageData tests are failing in try bots.

https://codereview.chromium.org/531183003/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8MessageEventCustom.cpp (left):

https://codereview.chromium.org/531183003/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8MessageEventCustom.cpp:48: static void
ensureWrapperCreatedAndAssociated(MessageEvent* eventImpl,
v8::Handle<v8::Object> eventWrapper, v8::Isolate* isolate)

Why do we have the setHiddenValue code in both V8MessageEventCustom.cpp and
MessageEvent.cpp.

https://codereview.chromium.org/531183003/diff/40001/Source/build/scripts/tem...
File Source/build/scripts/templates/ElementWrapperFactory.cpp.tmpl (left):

https://codereview.chromium.org/531183003/diff/40001/Source/build/scripts/tem...
Source/build/scripts/templates/ElementWrapperFactory.cpp.tmpl:1: {% from
"macros.tmpl" import license %}

I hope you completely remove ElementWrapperFactory.{h,cpp} and
make_element_factory.py in a follow-up CL :)

https://codereview.chromium.org/531183003/diff/40001/Source/build/scripts/tem...
Source/build/scripts/templates/ElementWrapperFactory.cpp.tmpl:37: if
(!RuntimeEnabledFeatures::{{list[0].runtimeEnabled}}Enabled())

It is wrong to just remove this code. We need to fallback to an
HTMLUnknownElement::wrap() if the element is not runtime-enabled.

https://codereview.chromium.org/531183003/diff/40001/Source/build/scripts/tem...
Source/build/scripts/templates/ElementWrapperFactory.cpp.tmpl:59:
createWrapperFunction = createV8{{namespace}}DirectWrapper;

I think it's correct to just remove createV8HTMLDirectWrapper in this CL, but
the reason is never trivial. If I'm understanding things correctly, it's correct
because:

- createV8HTMLDirectWrapper was used to redirect X::wrap to
V8HTMLElement::createWrapper for X that doesn't have a corresponding IDL file
(e.g., <noscript>).

- V8HTMLElement::createWrapper is doing exactly the same thing as
ScriptWrappable::wrap.

- Thus it's correct to remove createV8HTMLDirectWrapper and make X::wrap use
ScriptWrappable::wrap.

https://codereview.chromium.org/531183003/diff/40001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/531183003/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.cpp:3281: v8::Handle<v8::Object>
Element::wrapCustomElement(v8::Handle<v8::Object> creationContext, v8::Isolate*
isolate)

Why didn't you remove CustomElementWrapper in
bindings/core/v8/CustomElementWrapper.cpp when introducing
Element::wrapCustomElement?

[Yuki, 2014-09-04 04:47:29]

I'll investigate test failures, please hold on.  I'll ping you once I fix it.

https://codereview.chromium.org/531183003/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8ImageDataCustom.cpp (left):

https://codereview.chromium.org/531183003/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8ImageDataCustom.cpp:49:
wrapper->ForceSet(v8AtomicString(isolate, "data"), pixelArray, v8::ReadOnly);
On 2014/09/04 02:17:46, haraken wrote:
> 
> It looks wrong to remove this code. We still need this. That's why a couple of
> ImageData tests are failing in try bots.

We already have the new version of this code at
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

So it should be fine to remove this code.

https://codereview.chromium.org/531183003/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8MessageEventCustom.cpp (left):

https://codereview.chromium.org/531183003/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8MessageEventCustom.cpp:48: static void
ensureWrapperCreatedAndAssociated(MessageEvent* eventImpl,
v8::Handle<v8::Object> eventWrapper, v8::Isolate* isolate)
On 2014/09/04 02:17:46, haraken wrote:
> 
> Why do we have the setHiddenValue code in both V8MessageEventCustom.cpp and
> MessageEvent.cpp.

It's because we have two paths to create wrappers.  blink::wrap(MessageEvent*
impl, ...) and blink::ScriptWrappable::wrap(...).

This CL retires the former old one.
We're moving to the new code step by step.  Some of the old code is no longer
used, and some of the old code is used via different paths.

https://codereview.chromium.org/531183003/diff/40001/Source/build/scripts/tem...
File Source/build/scripts/templates/ElementWrapperFactory.cpp.tmpl (left):

https://codereview.chromium.org/531183003/diff/40001/Source/build/scripts/tem...
Source/build/scripts/templates/ElementWrapperFactory.cpp.tmpl:1: {% from
"macros.tmpl" import license %}
On 2014/09/04 02:17:46, haraken wrote:
> 
> I hope you completely remove ElementWrapperFactory.{h,cpp} and
> make_element_factory.py in a follow-up CL :)

Yes, will do.

https://codereview.chromium.org/531183003/diff/40001/Source/build/scripts/tem...
Source/build/scripts/templates/ElementWrapperFactory.cpp.tmpl:37: if
(!RuntimeEnabledFeatures::{{list[0].runtimeEnabled}}Enabled())
On 2014/09/04 02:17:46, haraken wrote:
> 
> It is wrong to just remove this code. We need to fallback to an
> HTMLUnknownElement::wrap() if the element is not runtime-enabled.

Hmm.  If the element is not runtime-enabled, then the DOM object is never
created, I think.  I'm seeing
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...
and
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...

If the flag is not enabled, then a HTMLUnknownElement is instantiated instead of
T and its wrap method uses V8HTMLUnknownElement as expected instead of V8T.

In anycase, if the DOM object is created as an instance of class T, it's wrong
to wrap it as V8HTMLUnknownElement, it must be wrapped as V8T.

https://codereview.chromium.org/531183003/diff/40001/Source/build/scripts/tem...
Source/build/scripts/templates/ElementWrapperFactory.cpp.tmpl:59:
createWrapperFunction = createV8{{namespace}}DirectWrapper;
On 2014/09/04 02:17:46, haraken wrote:
> 
> I think it's correct to just remove createV8HTMLDirectWrapper in this CL, but
> the reason is never trivial. If I'm understanding things correctly, it's
correct
> because:
> 
> - createV8HTMLDirectWrapper was used to redirect X::wrap to
> V8HTMLElement::createWrapper for X that doesn't have a corresponding IDL file
> (e.g., <noscript>).
> 
> - V8HTMLElement::createWrapper is doing exactly the same thing as
> ScriptWrappable::wrap.
> 
> - Thus it's correct to remove createV8HTMLDirectWrapper and make X::wrap use
> ScriptWrappable::wrap.

That's right.  In addition, it must be fine to use V8T for an instance of T
rather than V8HTMLElement (although T is a subclass of HTMLElement).

https://codereview.chromium.org/531183003/diff/40001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/531183003/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.cpp:3281: v8::Handle<v8::Object>
Element::wrapCustomElement(v8::Handle<v8::Object> creationContext, v8::Isolate*
isolate)
On 2014/09/04 02:17:46, haraken wrote:
> 
> Why didn't you remove CustomElementWrapper in
> bindings/core/v8/CustomElementWrapper.cpp when introducing
> Element::wrapCustomElement?

Just because I'd like to keep this CL small as much as possible.  We have a lot
of code to be removed in not only CustomElementWrapper but also
CustomElementConstructorBuilder, CustomElementBinding, etc.

Will remove (part of) them in separate CLs.

In a word, CustomeElementXxx need to manage prototype chain, but no need to
manage WrapperTypeInfo.

[haraken, 2014-09-04 04:54:53]

https://codereview.chromium.org/531183003/diff/40001/Source/build/scripts/tem...
File Source/build/scripts/templates/ElementWrapperFactory.cpp.tmpl (left):

https://codereview.chromium.org/531183003/diff/40001/Source/build/scripts/tem...
Source/build/scripts/templates/ElementWrapperFactory.cpp.tmpl:37: if
(!RuntimeEnabledFeatures::{{list[0].runtimeEnabled}}Enabled())
On 2014/09/04 04:47:29, Yuki wrote:
> On 2014/09/04 02:17:46, haraken wrote:
> > 
> > It is wrong to just remove this code. We need to fallback to an
> > HTMLUnknownElement::wrap() if the element is not runtime-enabled.
> 
> Hmm.  If the element is not runtime-enabled, then the DOM object is never
> created, I think.  I'm seeing
>
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...
> and
>
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...
> 
> If the flag is not enabled, then a HTMLUnknownElement is instantiated instead
of
> T and its wrap method uses V8HTMLUnknownElement as expected instead of V8T.
> 
> In anycase, if the DOM object is created as an instance of class T, it's wrong
> to wrap it as V8HTMLUnknownElement, it must be wrapped as V8T.

What happens if:

<div id="foo">
<audio></audio>
</div>
<script>
div = document.getElementById("foo");
div.firstChild;  // I think this should return a wrapper of HTMLUnknownElement
if the runtime-enabled flag is not on for webaudio.
</script>

https://codereview.chromium.org/531183003/diff/40001/Source/build/scripts/tem...
Source/build/scripts/templates/ElementWrapperFactory.cpp.tmpl:59:
createWrapperFunction = createV8{{namespace}}DirectWrapper;
On 2014/09/04 04:47:29, Yuki wrote:
> On 2014/09/04 02:17:46, haraken wrote:
> > 
> > I think it's correct to just remove createV8HTMLDirectWrapper in this CL,
but
> > the reason is never trivial. If I'm understanding things correctly, it's
> correct
> > because:
> > 
> > - createV8HTMLDirectWrapper was used to redirect X::wrap to
> > V8HTMLElement::createWrapper for X that doesn't have a corresponding IDL
file
> > (e.g., <noscript>).
> > 
> > - V8HTMLElement::createWrapper is doing exactly the same thing as
> > ScriptWrappable::wrap.
> > 
> > - Thus it's correct to remove createV8HTMLDirectWrapper and make X::wrap use
> > ScriptWrappable::wrap.
> 
> That's right.  In addition, it must be fine to use V8T for an instance of T
> rather than V8HTMLElement (although T is a subclass of HTMLElement).

There is no V8T for the classes that had been using createV8HTMLDirectWrapper.
For example, HTMLNoScriptElement doesn't have a corresponding IDL file and thus
V8HTMLNoScriptElement is not created.

Either way I think it's safe to just remove the DirectWrapper code.

https://codereview.chromium.org/531183003/diff/40001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/531183003/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.cpp:3281: v8::Handle<v8::Object>
Element::wrapCustomElement(v8::Handle<v8::Object> creationContext, v8::Isolate*
isolate)
On 2014/09/04 04:47:29, Yuki wrote:
> On 2014/09/04 02:17:46, haraken wrote:
> > 
> > Why didn't you remove CustomElementWrapper in
> > bindings/core/v8/CustomElementWrapper.cpp when introducing
> > Element::wrapCustomElement?
> 
> Just because I'd like to keep this CL small as much as possible.  We have a
lot
> of code to be removed in not only CustomElementWrapper but also
> CustomElementConstructorBuilder, CustomElementBinding, etc.
> 
> Will remove (part of) them in separate CLs.
> 
> In a word, CustomeElementXxx need to manage prototype chain, but no need to
> manage WrapperTypeInfo.

OK. Then can we create a helper method so that we don't need to duplicate the
code even if it will be removed in the near future? Since various developers are
modifying the code base, it is important to keep it as clean as possible.

[Yuki, 2014-09-04 14:10:15]

Found the cause of unittest failures.

We still need two [Custom=Wrap] for MessageEvent and ImageData,
because wrap function is called only when Custom=Wrap at the following point.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I'm going to fix this point in a separate CL.  I leave Custom=Wrap for
MessageEvent and ImageData in this CL.

https://codereview.chromium.org/531183003/diff/40001/Source/build/scripts/tem...
File Source/build/scripts/templates/ElementWrapperFactory.cpp.tmpl (left):

https://codereview.chromium.org/531183003/diff/40001/Source/build/scripts/tem...
Source/build/scripts/templates/ElementWrapperFactory.cpp.tmpl:37: if
(!RuntimeEnabledFeatures::{{list[0].runtimeEnabled}}Enabled())
On 2014/09/04 04:54:52, haraken wrote:
> On 2014/09/04 04:47:29, Yuki wrote:
> > On 2014/09/04 02:17:46, haraken wrote:
> > > 
> > > It is wrong to just remove this code. We need to fallback to an
> > > HTMLUnknownElement::wrap() if the element is not runtime-enabled.
> > 
> > Hmm.  If the element is not runtime-enabled, then the DOM object is never
> > created, I think.  I'm seeing
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...
> > and
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...
> > 
> > If the flag is not enabled, then a HTMLUnknownElement is instantiated
instead
> of
> > T and its wrap method uses V8HTMLUnknownElement as expected instead of V8T.
> > 
> > In anycase, if the DOM object is created as an instance of class T, it's
wrong
> > to wrap it as V8HTMLUnknownElement, it must be wrapped as V8T.
> 
> What happens if:
> 
> <div id="foo">
> <audio></audio>
> </div>
> <script>
> div = document.getElementById("foo");
> div.firstChild;  // I think this should return a wrapper of HTMLUnknownElement
> if the runtime-enabled flag is not on for webaudio.
> </script>

As talked offline, if the runtime flag is not enabled, then a HTMLUnknownElement
is instantiated and wrapped as V8HTMLUnknownElement as same as before.  Should
be no problem.

https://codereview.chromium.org/531183003/diff/40001/Source/build/scripts/tem...
Source/build/scripts/templates/ElementWrapperFactory.cpp.tmpl:59:
createWrapperFunction = createV8{{namespace}}DirectWrapper;
On 2014/09/04 04:54:52, haraken wrote:
> On 2014/09/04 04:47:29, Yuki wrote:
> > On 2014/09/04 02:17:46, haraken wrote:
> > > 
> > > I think it's correct to just remove createV8HTMLDirectWrapper in this CL,
> but
> > > the reason is never trivial. If I'm understanding things correctly, it's
> > correct
> > > because:
> > > 
> > > - createV8HTMLDirectWrapper was used to redirect X::wrap to
> > > V8HTMLElement::createWrapper for X that doesn't have a corresponding IDL
> file
> > > (e.g., <noscript>).
> > > 
> > > - V8HTMLElement::createWrapper is doing exactly the same thing as
> > > ScriptWrappable::wrap.
> > > 
> > > - Thus it's correct to remove createV8HTMLDirectWrapper and make X::wrap
use
> > > ScriptWrappable::wrap.
> > 
> > That's right.  In addition, it must be fine to use V8T for an instance of T
> > rather than V8HTMLElement (although T is a subclass of HTMLElement).
> 
> There is no V8T for the classes that had been using createV8HTMLDirectWrapper.
> For example, HTMLNoScriptElement doesn't have a corresponding IDL file and
thus
> V8HTMLNoScriptElement is not created.
> 
> Either way I think it's safe to just remove the DirectWrapper code.

Yes, there are such cases.  In those cases, they are handled by the superclass
which has a corresponding V8T class.  For the case of HTMLNoScriptElement, the
superclass HTMLElement takes care of wrapping and V8HTMLElement is used.

https://codereview.chromium.org/531183003/diff/40001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/531183003/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.cpp:3281: v8::Handle<v8::Object>
Element::wrapCustomElement(v8::Handle<v8::Object> creationContext, v8::Isolate*
isolate)
On 2014/09/04 04:54:52, haraken wrote:
> On 2014/09/04 04:47:29, Yuki wrote:
> > On 2014/09/04 02:17:46, haraken wrote:
> > > 
> > > Why didn't you remove CustomElementWrapper in
> > > bindings/core/v8/CustomElementWrapper.cpp when introducing
> > > Element::wrapCustomElement?
> > 
> > Just because I'd like to keep this CL small as much as possible.  We have a
> lot
> > of code to be removed in not only CustomElementWrapper but also
> > CustomElementConstructorBuilder, CustomElementBinding, etc.
> > 
> > Will remove (part of) them in separate CLs.
> > 
> > In a word, CustomeElementXxx need to manage prototype chain, but no need to
> > manage WrapperTypeInfo.
> 
> OK. Then can we create a helper method so that we don't need to duplicate the
> code even if it will be removed in the near future? Since various developers
are
> modifying the code base, it is important to keep it as clean as possible.

Okay, then, I've removed CustomElementWrapper.{h,cpp} in this CL.

[haraken, 2014-09-04 15:51:29]

LGTM.

> I'm going to fix this point in a separate CL.  I leave Custom=Wrap for
> MessageEvent and ImageData in this CL.

I'm OK with this if you plan to fix it shortly, but please add a FIXME in
ImageData.idl and MessageEvent.idl. It's confusing that T.idl has [Custom=Wrap]
but V8TCustom.cpp doesn't have V8T::wrap methods.

[haraken, 2014-09-04 15:53:10]

Also you might want to start counting # of lines you removed (except for
bindings/tests/) and the binary size of a stripped Release build you decreased.
Those are important metrics of this work :)

[Yuki, 2014-09-05 04:37:05]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2014-09-05 04:37:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yukishiino@chromium.org/531183003/120001

[commit-bot: I haz the power, 2014-09-05 05:49:07]

Committed patchset #7 (id:120001) as 181440