IDL compiler: [CustomToV8] interface

IDL compiler: [CustomToV8] interface

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

[Nils Barth (inactive), 2013-11-22 06:10:47]

https://codereview.chromium.org/82693003/diff/1/Source/bindings/scripts/code_...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/82693003/diff/1/Source/bindings/scripts/code_...
Source/bindings/scripts/code_generator_v8.pm:900: 
Inconsistent spacing, caught by template and fixed here too for consistency.

https://codereview.chromium.org/82693003/diff/1/Source/bindings/scripts/unsta...
File Source/bindings/scripts/unstable/v8_interface.py (left):

https://codereview.chromium.org/82693003/diff/1/Source/bindings/scripts/unsta...
Source/bindings/scripts/unstable/v8_interface.py:53:
'bindings/v8/ScriptController.h',
Now conditional; this makes a difference for 100+ generated files.
Code would be slightly simpler (2 lines) if this were always included, but
that's an extra header.

https://codereview.chromium.org/82693003/diff/1/Source/bindings/templates/int...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/82693003/diff/1/Source/bindings/templates/int...
Source/bindings/templates/interface.cpp:435: {% block create_wrapper %}
Split block in two so that {% if %} statement lines up with block, otherwise it
gets buried in the middle.
(One function is conditional, other one isn't.)

https://codereview.chromium.org/82693003/diff/1/Source/bindings/templates/int...
File Source/bindings/templates/interface.h (right):

https://codereview.chromium.org/82693003/diff/1/Source/bindings/templates/int...
Source/bindings/templates/interface.h:194: {% endif %}{# has_custom_to_v8 #}
*Really* long {% if %} block (almost half the file!)

[haraken, 2013-11-22 07:14:03]

LGTM

https://codereview.chromium.org/82693003/diff/1/Source/bindings/scripts/unsta...
File Source/bindings/scripts/unstable/v8_interface.py (right):

https://codereview.chromium.org/82693003/diff/1/Source/bindings/scripts/unsta...
Source/bindings/scripts/unstable/v8_interface.py:71:
includes.add('bindings/v8/ScriptController.h')

Why do we need ScriptController for [CustomToV8]?

https://codereview.chromium.org/82693003/diff/1/Source/bindings/templates/int...
File Source/bindings/templates/interface.h (right):

https://codereview.chromium.org/82693003/diff/1/Source/bindings/templates/int...
Source/bindings/templates/interface.h:117: inline void v8SetReturnValue(const
CallbackInfo& callbackInfo, {{cpp_class_name}}* impl)

Where are these methods used? If this is used only a few places, we can just
inline the methods.

[Nils Barth (inactive), 2013-12-03 02:52:28]

Revised;
1. header ScriptController.h is usually *not* needed, so put into branch!
2. the wrapper v8SetReturnValue templates are used for name resolution (w/o them
it's ambiguous); we always define these templates in the header, but usually
with logic, so having wrappers here keeps it consistent, I believe.

https://codereview.chromium.org/82693003/diff/1/Source/bindings/scripts/unsta...
File Source/bindings/scripts/unstable/v8_interface.py (right):

https://codereview.chromium.org/82693003/diff/1/Source/bindings/scripts/unsta...
Source/bindings/scripts/unstable/v8_interface.py:71:
includes.add('bindings/v8/ScriptController.h')
On 2013/11/22 07:14:04, haraken wrote:
> Why do we need ScriptController for [CustomToV8]?

Actually this was included for all *but* [CustomToV8].
I looked into this, and it's only actually needed for interfaces
that inherit from Document.
I've fixed the Perl code to only include it there;
this removes one include!

https://codereview.chromium.org/82693003/diff/1/Source/bindings/templates/int...
File Source/bindings/templates/interface.h (right):

https://codereview.chromium.org/82693003/diff/1/Source/bindings/templates/int...
Source/bindings/templates/interface.h:117: inline void v8SetReturnValue(const
CallbackInfo& callbackInfo, {{cpp_class_name}}* impl)
On 2013/11/22 07:14:04, haraken wrote:
> Where are these methods used?

They're used for disambiguation (in the generated code).
I tried removing these, and got various
"call of overloaded 'v8SetReturnValueFast' is ambiguous"
errors.

Normally we always generate these templates, with some logic in them; for
[CustomToV8] it just has these wrappers.

I think we'd need to change the code generator to change the call sites if we
wanted to remove these, because it would change *which* templates are defined in
the header depending on [CustomToV8].

Full(er) error:
call of overloaded 'v8SetReturnValueFast(const
v8::PropertyCallbackInfo<v8::Value>&, WebCore::DOMWindow*,
WebCore::DOMWindow*&)' is ambiguous
(candidates are: V8EventTarget.h and V8Window.h, in this case)

[commit-bot: I haz the power, 2013-12-03 02:53:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/82693003/90001

[haraken, 2013-12-03 03:21:25]

https://codereview.chromium.org/82693003/diff/1/Source/bindings/templates/int...
File Source/bindings/templates/interface.h (right):

https://codereview.chromium.org/82693003/diff/1/Source/bindings/templates/int...
Source/bindings/templates/interface.h:117: inline void v8SetReturnValue(const
CallbackInfo& callbackInfo, {{cpp_class_name}}* impl)
On 2013/12/03 02:52:28, Nils Barth wrote:
> On 2013/11/22 07:14:04, haraken wrote:
> > Where are these methods used?
> 
> They're used for disambiguation (in the generated code).
> I tried removing these, and got various
> "call of overloaded 'v8SetReturnValueFast' is ambiguous"
> errors.
> 
> Normally we always generate these templates, with some logic in them; for
> [CustomToV8] it just has these wrappers.
> 
> I think we'd need to change the code generator to change the call sites if we
> wanted to remove these, because it would change *which* templates are defined
in
> the header depending on [CustomToV8].
> 
> Full(er) error:
> call of overloaded 'v8SetReturnValueFast(const
> v8::PropertyCallbackInfo<v8::Value>&, WebCore::DOMWindow*,
> WebCore::DOMWindow*&)' is ambiguous
> (candidates are: V8EventTarget.h and V8Window.h, in this case)

Makes sense.

[commit-bot: I haz the power, 2013-12-03 05:17:03]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-12-03 08:54:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/82693003/90001

[commit-bot: I haz the power, 2013-12-03 10:33:59]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-12-04 03:04:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/82693003/130001

[commit-bot: I haz the power, 2013-12-04 04:09:46]

Retried try job too often on linux_blink for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-12-04 05:24:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/82693003/170001

[commit-bot: I haz the power, 2013-12-04 09:54:58]

Change committed as 163153