IDL: Use ALLOW_ONLY_INLINE_ALLOCATION() in dictionaries

IDL: Use ALLOW_ONLY_INLINE_ALLOCATION() in dictionaries

Before this CL, we allocate memory for IDL dictionaries each time we use
them. For performance sensitive methods, this may not be acceptable.
Avoid allocating them in the oilpan heap. Instead, put them on stack.
Dictionaries can be a member of collections, so we use
ALLOW_ONLY_INLINE_ALLOCATION().

Side effect of this change:
If an impl class implements an IDL method which returns a dictionary,
the impl method needs to take an argument for return value.

This CL also add blank lines in generated dictionary impl classes for
readability.

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

[bashi, 2014-10-15 13:08:01]

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

[bashi, 2014-10-15 13:08:01]

Haraken-san, PTAL?

[haraken, 2014-10-15 14:38:29]

LGTM

https://codereview.chromium.org/656073002/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/656073002/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:182: 'use_argument_for_return_value':
idl_type.is_dictionary,

use_output_parameter_for_result ?

'argument' is a bit confusing with a method argument.

'result' would be better for consistency with the following use_local_result.

https://codereview.chromium.org/656073002/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:313: if method.idl_type and
method.idl_type.is_dictionary:

Do we need the 'method.idl_type and' part?

https://codereview.chromium.org/656073002/diff/1/Source/bindings/templates/di...
File Source/bindings/templates/dictionary_v8.cpp (left):

https://codereview.chromium.org/656073002/diff/1/Source/bindings/templates/di...
Source/bindings/templates/dictionary_v8.cpp:18:
ASSERT(isUndefinedOrNull(v8Value) || v8Value->IsObject());

Can we drop the v8Value->IsObject() check?

https://codereview.chromium.org/656073002/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/656073002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:237: {{method.cpp_type}} cppValue;

I'd rename |cppValue| to |result| for consistency with line 242.

https://codereview.chromium.org/656073002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:242: {{method.cpp_type}} result =
{{cpp_value}};

We were trying to _always_ use local result to simplify the IDL compiler, but
what's the status of the work? (I remember we discussed a lot but I forgot the
conclusion :-)

[Jens Widell, 2014-10-15 15:39:16]

jl@opera.com changed reviewers:
+ jl@opera.com

[Jens Widell, 2014-10-15 15:39:17]

https://codereview.chromium.org/656073002/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/656073002/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:313: if method.idl_type and
method.idl_type.is_dictionary:
On 2014/10/15 14:38:28, haraken wrote:
> 
> Do we need the 'method.idl_type and' part?

Probably for constructors. Typically when you think "when does a method ever not
have that?" around here the answer is "if it's actually a constructor." :-)

https://codereview.chromium.org/656073002/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/656073002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:242: {{method.cpp_type}} result =
{{cpp_value}};
On 2014/10/15 14:38:29, haraken wrote:
> 
> We were trying to _always_ use local result to simplify the IDL compiler, but
> what's the status of the work? (I remember we discussed a lot but I forgot the
> conclusion :-)

https://codereview.chromium.org/386613002/

The status is that there wasn't really a conclusion, I think, and then  nothing
more happened. I had forgotten about it, to be honest.

[bashi, 2014-10-17 00:50:05]

Thank you for reviews!

https://codereview.chromium.org/656073002/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/656073002/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:182: 'use_argument_for_return_value':
idl_type.is_dictionary,
On 2014/10/15 14:38:28, haraken wrote:
> 
> use_output_parameter_for_result ?
> 
> 'argument' is a bit confusing with a method argument.
> 
> 'result' would be better for consistency with the following use_local_result.

Done.

https://codereview.chromium.org/656073002/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:313: if method.idl_type and
method.idl_type.is_dictionary:
On 2014/10/15 15:39:17, Jens Widell wrote:
> On 2014/10/15 14:38:28, haraken wrote:
> > 
> > Do we need the 'method.idl_type and' part?
> 
> Probably for constructors. Typically when you think "when does a method ever
not
> have that?" around here the answer is "if it's actually a constructor." :-)

Yes. Similar check exists on L298.

https://codereview.chromium.org/656073002/diff/1/Source/bindings/templates/di...
File Source/bindings/templates/dictionary_v8.cpp (left):

https://codereview.chromium.org/656073002/diff/1/Source/bindings/templates/di...
Source/bindings/templates/dictionary_v8.cpp:18:
ASSERT(isUndefinedOrNull(v8Value) || v8Value->IsObject());
On 2014/10/15 14:38:28, haraken wrote:
> 
> Can we drop the v8Value->IsObject() check?

Ah, maybe we can't. Even though the binding layer only calls toImpl() when
v8Object is null, undefined or an object, other callsite may call toImpl() with
non-object v8Value. We should throw an type error when v8Value isn't an object.

https://codereview.chromium.org/656073002/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/656073002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:237: {{method.cpp_type}} cppValue;
On 2014/10/15 14:38:29, haraken wrote:
> 
> I'd rename |cppValue| to |result| for consistency with line 242.

Done.

https://codereview.chromium.org/656073002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:242: {{method.cpp_type}} result =
{{cpp_value}};
On 2014/10/15 15:39:17, Jens Widell wrote:
> On 2014/10/15 14:38:29, haraken wrote:
> > 
> > We were trying to _always_ use local result to simplify the IDL compiler,
but
> > what's the status of the work? (I remember we discussed a lot but I forgot
the
> > conclusion :-)
> 
> https://codereview.chromium.org/386613002/
> 
> The status is that there wasn't really a conclusion, I think, and then 
nothing
> more happened. I had forgotten about it, to be honest.

I had forgotten too. Maybe we can revisit this when this becomes a real problem.

[haraken, 2014-10-17 00:57:12]

https://codereview.chromium.org/656073002/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/656073002/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:242: {{method.cpp_type}} result =
{{cpp_value}};
On 2014/10/17 00:50:05, bashi1 wrote:
> On 2014/10/15 15:39:17, Jens Widell wrote:
> > On 2014/10/15 14:38:29, haraken wrote:
> > > 
> > > We were trying to _always_ use local result to simplify the IDL compiler,
> but
> > > what's the status of the work? (I remember we discussed a lot but I forgot
> the
> > > conclusion :-)
> > 
> > https://codereview.chromium.org/386613002/
> > 
> > The status is that there wasn't really a conclusion, I think, and then 
> nothing
> > more happened. I had forgotten about it, to be honest.
> 
> I had forgotten too. Maybe we can revisit this when this becomes a real
problem.

The reason I asked this is that forcing a local result might make your situation
easier. Currently there are three ways to get a cpp result:

1) Use a local result.
2) Use a on-stack return value.
3) Use an output parameter to store a return value.

I just thought that removing 1) might help simplify the code. Duuno.

[bashi, 2014-10-17 01:30:50]

On 2014/10/17 00:57:12, haraken wrote:
>
https://codereview.chromium.org/656073002/diff/1/Source/bindings/templates/me...
> File Source/bindings/templates/methods.cpp (right):
> 
>
https://codereview.chromium.org/656073002/diff/1/Source/bindings/templates/me...
> Source/bindings/templates/methods.cpp:242: {{method.cpp_type}} result =
> {{cpp_value}};
> On 2014/10/17 00:50:05, bashi1 wrote:
> > On 2014/10/15 15:39:17, Jens Widell wrote:
> > > On 2014/10/15 14:38:29, haraken wrote:
> > > > 
> > > > We were trying to _always_ use local result to simplify the IDL
compiler,
> > but
> > > > what's the status of the work? (I remember we discussed a lot but I
forgot
> > the
> > > > conclusion :-)
> > > 
> > > https://codereview.chromium.org/386613002/
> > > 
> > > The status is that there wasn't really a conclusion, I think, and then 
> > nothing
> > > more happened. I had forgotten about it, to be honest.
> > 
> > I had forgotten too. Maybe we can revisit this when this becomes a real
> problem.
> 
> The reason I asked this is that forcing a local result might make your
situation
> easier. Currently there are three ways to get a cpp result:
> 
> 1) Use a local result.
> 2) Use a on-stack return value.
> 3) Use an output parameter to store a return value.
> 
> I just thought that removing 1) might help simplify the code. Duuno.

Hmm, I'm not sure we can simplify the code by removing 1. I'd like to keep them
at this moment.

[bashi, 2014-10-17 01:31:04]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2014-10-17 01:31:51]

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

[commit-bot: I haz the power, 2014-10-17 04:08:54]

Committed patchset #2 (id:20001) as 183855