Introduce toV8NoInline

Introduce toV8NoInline

In order to make it enable to resolve a Promise with a DOM object without
including generated headers, I added toV8NoInline template function declaration to V8Binding.h.
Each specialized implementation will be generated in the corresponding generated source.

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

[abarth-chromium, 2013-11-22 05:53:35]

I don't fully understand what's causing this problem, but this seems like an ok
solution.  Maybe we should always generate these stubs?

https://codereview.chromium.org/78713009/diff/40001/Source/core/dom/DOMError.idl
File Source/core/dom/DOMError.idl (right):

https://codereview.chromium.org/78713009/diff/40001/Source/core/dom/DOMError....
Source/core/dom/DOMError.idl:31: ] interface  DOMError {
Can you remove the extra space between interface and DOMError

[abarth-chromium, 2013-11-22 05:53:40]

LGTM

[haraken, 2013-11-22 06:01:17]

https://codereview.chromium.org/78713009/diff/40001/Source/bindings/scripts/c...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/78713009/diff/40001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.pm:4760: void
ScriptPromiseResolver::resolve(${nativeType}* impl, v8::Handle<v8::Object>
creationContext)

It looks weird to generate ScriptPromiseResolver methods in generated files.
Would you elaborate on what problem you're going to solve?

(c.f., we generate toEventTarget() to help EventTarget methods to get
EventTarget objects, but we never generate EventTarget methods in generated
files.)

[Nils Barth (inactive), 2013-11-22 06:07:13]

On 2013/11/22 06:01:17, haraken wrote:
> Would you elaborate on what problem you're going to solve?

There's some background at this bug:
template<class T> ScriptPromiseResolver::resolve(PassRefPtr<T>) is not useful
https://code.google.com/p/chromium/issues/detail?id=321569

[yhirano, 2013-11-22 06:12:00]

https://codereview.chromium.org/78713009/diff/40001/Source/core/dom/DOMError.idl
File Source/core/dom/DOMError.idl (right):

https://codereview.chromium.org/78713009/diff/40001/Source/core/dom/DOMError....
Source/core/dom/DOMError.idl:31: ] interface  DOMError {
On 2013/11/22 05:53:35, abarth wrote:
> Can you remove the extra space between interface and DOMError

Done.

[yhirano, 2013-11-22 06:17:14]

On 2013/11/22 05:53:35, abarth wrote:
> Maybe we should always generate these stubs?
I introduce this attribute in fear of binary size increase.
If it is not a problem, I would love to stop introducing the attribute and
generates functions unconditionally.
FYI: In my local build environment, binary size doesn't differ dramatically:
131574864 (generates functions only when the attribute is specified) vs
131575144 (generates functions always)

[yhirano, 2013-11-22 06:32:31]

https://codereview.chromium.org/78713009/diff/40001/Source/bindings/scripts/c...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/78713009/diff/40001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.pm:4760: void
ScriptPromiseResolver::resolve(${nativeType}* impl, v8::Handle<v8::Object>
creationContext)
On 2013/11/22 06:01:17, haraken wrote:
> 
> It looks weird to generate ScriptPromiseResolver methods in generated files.
> Would you elaborate on what problem you're going to solve?
> 
> (c.f., we generate toEventTarget() to help EventTarget methods to get
> EventTarget objects, but we never generate EventTarget methods in generated
> files.)
I want to implement

template<typename T>
void ScriptPromiseResolver::resolve(T* impl, v8::Handle<v8::Object>)
(and reject)

.

One possible solution is declaring
template<typename T>
v8::Handle<v8::Value> toV8NonInline(T*, v8::Handle<v8::Object>, v8::Isolate*);
in V8Binding.h and implementing each specialized function in the corresponding
generated source.
Then we can implement generic resolve and reject in ScriptPromiseResolver.h by
using toV8NonInline.

Is it more preferable?

[haraken, 2013-11-22 06:51:57]

Thanks, I understood the problem.

> One possible solution is declaring
> template<typename T>
> v8::Handle<v8::Value> toV8NonInline(T*, v8::Handle<v8::Object>, v8::Isolate*);
> in V8Binding.h and implementing each specialized function in the corresponding
> generated source.
> Then we can implement generic resolve and reject in ScriptPromiseResolver.h by
> using toV8NonInline.
> 
> Is it more preferable?

Yeah, this looks better to me rather than generating template specializations
for resolve, reject etc in the future.

How much will toV8NoInline increase binary size? In my understanding,
toV8NoInline just redirects to toV8.

[haraken, 2013-11-22 06:56:14]

> Yeah, this looks better to me rather than generating template specializations
> for resolve, reject etc in the future.
> 
> How much will toV8NoInline increase binary size? In my understanding,
> toV8NoInline just redirects to toV8.

hmm, given the binary size increase you observed when you omit
[PromiseResolutionTarget] IDL attribute, the binary size introduced by
toV8NoInline wouldn't be negligible.

[haraken, 2013-11-22 07:04:12]

I have no better idea than the current CL, so I'm OK with it. However, it's ugly
to auto-generate template specializaitons of other classes to generated files.

[yhirano, 2013-11-22 07:12:56]

On 2013/11/22 06:51:57, haraken wrote:
> Thanks, I understood the problem.
> 
> > One possible solution is declaring
> > template<typename T>
> > v8::Handle<v8::Value> toV8NonInline(T*, v8::Handle<v8::Object>,
v8::Isolate*);
> > in V8Binding.h and implementing each specialized function in the
corresponding
> > generated source.
> > Then we can implement generic resolve and reject in ScriptPromiseResolver.h
by
> > using toV8NonInline.
> > 
> > Is it more preferable?
> 
> Yeah, this looks better to me rather than generating template specializations
> for resolve, reject etc in the future.
> 
> How much will toV8NoInline increase binary size? In my understanding,
> toV8NoInline just redirects to toV8.

before: 131574864
after:  131575056

Note: toV8NoInline is not used by anyone, so the difference should be 0.

[haraken, 2013-11-22 07:19:10]

> Note: toV8NoInline is not used by anyone, so the difference should be 0.

Is it true even if we generate toV8NoInline for all generated cpp files? Will
toV8NoInline be instantiated only for toV8NoInline that is actually used by
PromiseResolver? If so, that seems like a better solution to me.

[yhirano, 2013-11-22 07:38:58]

On 2013/11/22 07:19:10, haraken wrote:
> > Note: toV8NoInline is not used by anyone, so the difference should be 0.
> 
> Is it true even if we generate toV8NoInline for all generated cpp files? Will
> toV8NoInline be instantiated only for toV8NoInline that is actually used by
> PromiseResolver? If so, that seems like a better solution to me.

A bit more information about the measurement:

before: 131574864
 - PS3 is already applied  
after:  131575056
 - PS3 is already applied
 - Generates toV8NoInline for all DOM classes (but they are not called by anyone
since resolve and reject is still generated and call toV8 directly in the
generated files).


Generating toV8NoInline only when PromiseResolutionTarget is specified is a
possible option.
If we do so, we need an additional comment at V8Binding, like this:

// Converts a DOM object into a v8 value.
// The implementation will be done in a generated file if certain condition is
satisfied.
// For details, see <URL of the blink WebIDL attribute document>.
// DO NOT USE THIS FUNCTION if you do not fully understand this comment.
template<typename t>
v8::Handle<v8::Value> toV8NoInline(T*, v8::Handle<v8::Object>, v8::Isolate*);

[haraken, 2013-11-22 07:50:29]

On 2013/11/22 07:38:58, yhirano wrote:
> On 2013/11/22 07:19:10, haraken wrote:
> > > Note: toV8NoInline is not used by anyone, so the difference should be 0.
> > 
> > Is it true even if we generate toV8NoInline for all generated cpp files?
Will
> > toV8NoInline be instantiated only for toV8NoInline that is actually used by
> > PromiseResolver? If so, that seems like a better solution to me.
> 
> A bit more information about the measurement:
> 
> before: 131574864
>  - PS3 is already applied  
> after:  131575056
>  - PS3 is already applied
>  - Generates toV8NoInline for all DOM classes (but they are not called by
anyone
> since resolve and reject is still generated and call toV8 directly in the
> generated files).

Only 100 byte? This difference looks small. How about generating toV8NoInline
always?

[yhirano, 2013-11-22 07:56:45]

On 2013/11/22 07:50:29, haraken wrote:
> Only 100 byte? This difference looks small. How about generating toV8NoInline
> always?

Thanks, will do.

[yhirano, 2013-11-22 09:26:50]

haraken@, PTAL if you have time

[haraken, 2013-11-23 15:03:17]

https://codereview.chromium.org/78713009/diff/420001/Source/bindings/scripts/...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/78713009/diff/420001/Source/bindings/scripts/...
Source/bindings/scripts/code_generator_v8.pm:4755: if
(!$interface->extendedAttributes->{"DoNotGenerateToV8"} &&
!$interface->extendedAttributes->{"CustomToV8"}) {

Probably we can auto-generate toV8NoInline when [CustomToV8] is specified. Then
you can avoid hand-writing toV8NoInline in V8ArrayBufferCustom.cpp.

https://codereview.chromium.org/78713009/diff/420001/Source/bindings/v8/V8Bin...
File Source/bindings/v8/V8Binding.h (right):

https://codereview.chromium.org/78713009/diff/420001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.h:730: // Each specialized implementation will be
generated.

Would you add more comment about what this method is for?

e.g., "This is a no-inline version of toV8(). If you want to call toV8() from
templates without creating #include cycles, you can use toV8NoInline() instead."

[yhirano, 2013-11-25 01:50:51]

https://codereview.chromium.org/78713009/diff/420001/Source/bindings/scripts/...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/78713009/diff/420001/Source/bindings/scripts/...
Source/bindings/scripts/code_generator_v8.pm:4755: if
(!$interface->extendedAttributes->{"DoNotGenerateToV8"} &&
!$interface->extendedAttributes->{"CustomToV8"}) {
On 2013/11/23 15:03:18, haraken wrote:
> 
> Probably we can auto-generate toV8NoInline when [CustomToV8] is specified.
Then
> you can avoid hand-writing toV8NoInline in V8ArrayBufferCustom.cpp.

ConsoleBase has toV8Custom attribute but the custom toV8 function is not
defined, so generating toV8NoInline for it causes a link error with DEBUG mode.
Should we fix ConsoleBase?

https://codereview.chromium.org/78713009/diff/420001/Source/bindings/v8/V8Bin...
File Source/bindings/v8/V8Binding.h (right):

https://codereview.chromium.org/78713009/diff/420001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.h:730: // Each specialized implementation will be
generated.
On 2013/11/23 15:03:18, haraken wrote:
> 
> Would you add more comment about what this method is for?
> 
> e.g., "This is a no-inline version of toV8(). If you want to call toV8() from
> templates without creating #include cycles, you can use toV8NoInline()
instead."

Done.

[haraken, 2013-11-25 02:39:22]

https://codereview.chromium.org/78713009/diff/420001/Source/bindings/scripts/...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/78713009/diff/420001/Source/bindings/scripts/...
Source/bindings/scripts/code_generator_v8.pm:4755: if
(!$interface->extendedAttributes->{"DoNotGenerateToV8"} &&
!$interface->extendedAttributes->{"CustomToV8"}) {
> ConsoleBase has toV8Custom attribute but the custom toV8 function is not
> defined, so generating toV8NoInline for it causes a link error with DEBUG
mode.
> Should we fix ConsoleBase?

Yes! It won't make sense to have [toV8Custom] but doesn't have a custom toV8
function.

[yhirano, 2013-11-25 03:36:48]

https://codereview.chromium.org/78713009/diff/420001/Source/bindings/scripts/...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/78713009/diff/420001/Source/bindings/scripts/...
Source/bindings/scripts/code_generator_v8.pm:4755: if
(!$interface->extendedAttributes->{"DoNotGenerateToV8"} &&
!$interface->extendedAttributes->{"CustomToV8"}) {
On 2013/11/25 02:39:23, haraken wrote:
> > ConsoleBase has toV8Custom attribute but the custom toV8 function is not
> > defined, so generating toV8NoInline for it causes a link error with DEBUG
> mode.
> > Should we fix ConsoleBase?
> 
> Yes! It won't make sense to have [toV8Custom] but doesn't have a custom toV8
> function.

Done.
We can't omit toV8NoInline in V8ArrayBufferCustom.cpp because V8ArrayBuffer is
not generated by the generator.

[haraken, 2013-11-25 03:48:05]

Thanks, LGTM.

[commit-bot: I haz the power, 2013-11-25 06:25:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/78713009/680001

[commit-bot: I haz the power, 2013-11-25 07:08:21]

Change committed as 162600