IDL: Binding code generation for dictionaries

IDL: Binding code generation for dictionaries

This CL adds V8 binding code generation for IDL dictionaries.
- Each dictionary will have its own IDL file because we generate binding
  code from dedicated template(dictionary_v8.{cpp,h}).
- Generated binding will contain toV8() and toNative() functions. They are
  similar to interfaces' toV8() and toNative(), but there are a little
  differences. toV8() never creates a wrapper object. It always returns
  a new V8 object.  Similarly, toNative() always creates a new DOM object.
  This behavior should confirm that dictionaries are passed by value.
- IDL dictionaries are required to be GarbageCollected because we don't want
  to add further Oilpan transition pain.
- This CL removes TestDictDerived. We are currently not supporting
  inheritance.
- Modified IdlDictionary and IdlDictionaryMember to code generation work.
- Added "is_dictionary" flag to interfaces_info so that the code generator
  can handle IDL dictionary correctly.
- Added "is_dictionary" property to IdlType. This is needed for binding code
  generation.
- Added v8_dictionary.py. This creates template context for dictionaries.

Generated binding code won't be able to compile at this point. We need
DOM implementation. In follow-up CLs, we are planning to auto-generate
DOM implementation.

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

[bashi, 2014-07-23 10:04:04]

Haraken-san, Sakamoto-san, PTAL?

This becomes unexpectedly large CL. I may be able to make this CL smaller, but
without having unnecessary code modification(or tentative test results), this
might be a minimum CL to work..

[haraken, 2014-07-23 15:49:01]

LGTM

https://codereview.chromium.org/409373002/diff/1/Source/bindings/scripts/v8_d...
File Source/bindings/scripts/v8_dictionary.py (right):

https://codereview.chromium.org/409373002/diff/1/Source/bindings/scripts/v8_d...
Source/bindings/scripts/v8_dictionary.py:15: DICTIONARY_H_INCLUDES_V8 =
frozenset([

DICTIONARY_H_INCLUDES_V8 => DICTIONARY_H_INCLUDES (for consistency with other
python files)

I think you can use DICTIONARY_IMPL_H_INCLUDES for Dictionary implementations.

https://codereview.chromium.org/409373002/diff/1/Source/bindings/scripts/v8_d...
Source/bindings/scripts/v8_dictionary.py:20: DICTIONARY_CPP_INCLUDES_V8 =
frozenset([

DICTIONARY_CPP_INCLUDES_V8 => DICTIONARY_CPP_INCLUDES

https://codereview.chromium.org/409373002/diff/1/Source/bindings/scripts/v8_d...
Source/bindings/scripts/v8_dictionary.py:36: 

Unnecessary empty lines.

https://codereview.chromium.org/409373002/diff/1/Source/bindings/scripts/v8_d...
Source/bindings/scripts/v8_dictionary.py:37: def
dictionary_context_v8(dictionary):

dictionary_context_v8 => dictionary_context ?

I think you can use dictionary_impl_context for Dictionary implementations.

https://codereview.chromium.org/409373002/diff/1/Source/bindings/scripts/v8_d...
Source/bindings/scripts/v8_dictionary.py:50: def member_context_v8(member):

member_context_v8 => member_context ?

https://codereview.chromium.org/409373002/diff/1/Source/bindings/scripts/v8_d...
Source/bindings/scripts/v8_dictionary.py:57: if member.default_value.is_null:

Just help me understand: When do we hit this branch? Can we add a test case for
this?

https://codereview.chromium.org/409373002/diff/1/Source/bindings/scripts/v8_d...
Source/bindings/scripts/v8_dictionary.py:59: default_value =
str(member.default_value)

default_value => cpp_default_value

https://codereview.chromium.org/409373002/diff/1/Source/bindings/scripts/v8_d...
Source/bindings/scripts/v8_dictionary.py:66: default_value, v8_default_value =
default_values()

decalut_value => cpp_decault_value

https://codereview.chromium.org/409373002/diff/1/Source/bindings/templates/di...
File Source/bindings/templates/dictionary_v8.h (right):

https://codereview.chromium.org/409373002/diff/1/Source/bindings/templates/di...
Source/bindings/templates/dictionary_v8.h:21: v8::Handle<v8::Value>
toV8({{cpp_class}}*, v8::Handle<v8::Object> creationContext, v8::Isolate*);

Drop |creationContext|.

https://codereview.chromium.org/409373002/diff/1/Source/bindings/tests/idls/T...
File Source/bindings/tests/idls/TestDictionary.idl (right):

https://codereview.chromium.org/409373002/diff/1/Source/bindings/tests/idls/T...
Source/bindings/tests/idls/TestDictionary.idl:8: long longMember = 1;

Would you add a bit more test cases? Although you don't need to cover all types,
you might want to add more things you've already supported.

[jsbell, 2014-07-23 16:44:28]

Do you have a design doc or rough sketch of what the API of the generated DOM
object will look like?

I'm curious about how 'not specified', nullable, and defaulted values will be
represented. The former is the newish case, handled today by a boolean out-value
in some get() permutations, as not many specs make use of it.

I can wait for the CL, I suppose. (I'm just excited!)

[bashi, 2014-07-25 01:59:03]

@haraken, thank you for the review! Could you take another look? There is a
minor logic change.

@jsbell, thanks for your interest.

Could you refer TestDictionary.{cpp,h} in the WIP patch[1]?

In our current plan, we don't distingish 'not specified' and null, per spec says
that both undefined and null are treated as undefined, and unless otherwise the
default value is specified, undefined means 'not present'[2]. We are going to
use Nullable<T> for all primitive types, arrays and sequences to implement the
notion of 'present' (note: arrays and sequences in the WIP patch don't use
Nullable, but I'll revise it soon). The impl classes will have |hasXXX()|
methods which tell whether members are present. Do you have cases in which you
need to distinguish a member is 'not specified' or 'null'? I'd like to gather
possible customer's feedback as much as possible, so please let me know your
concerns. Thanks!

[1] https://codereview.chromium.org/386963003/
[2] http://heycam.github.io/webidl/#es-dictionary

https://codereview.chromium.org/409373002/diff/1/Source/bindings/scripts/v8_d...
File Source/bindings/scripts/v8_dictionary.py (right):

https://codereview.chromium.org/409373002/diff/1/Source/bindings/scripts/v8_d...
Source/bindings/scripts/v8_dictionary.py:15: DICTIONARY_H_INCLUDES_V8 =
frozenset([
On 2014/07/23 15:49:01, haraken wrote:
> 
> DICTIONARY_H_INCLUDES_V8 => DICTIONARY_H_INCLUDES (for consistency with other
> python files)
> 
> I think you can use DICTIONARY_IMPL_H_INCLUDES for Dictionary implementations.

Done.

https://codereview.chromium.org/409373002/diff/1/Source/bindings/scripts/v8_d...
Source/bindings/scripts/v8_dictionary.py:20: DICTIONARY_CPP_INCLUDES_V8 =
frozenset([
On 2014/07/23 15:49:01, haraken wrote:
> 
> DICTIONARY_CPP_INCLUDES_V8 => DICTIONARY_CPP_INCLUDES

Done.

https://codereview.chromium.org/409373002/diff/1/Source/bindings/scripts/v8_d...
Source/bindings/scripts/v8_dictionary.py:36: 
On 2014/07/23 15:49:01, haraken wrote:
> 
> Unnecessary empty lines.

Done.

https://codereview.chromium.org/409373002/diff/1/Source/bindings/scripts/v8_d...
Source/bindings/scripts/v8_dictionary.py:37: def
dictionary_context_v8(dictionary):
On 2014/07/23 15:49:01, haraken wrote:
> 
> dictionary_context_v8 => dictionary_context ?
> 
> I think you can use dictionary_impl_context for Dictionary implementations.

Done.

https://codereview.chromium.org/409373002/diff/1/Source/bindings/scripts/v8_d...
Source/bindings/scripts/v8_dictionary.py:50: def member_context_v8(member):
On 2014/07/23 15:49:01, haraken wrote:
> 
> member_context_v8 => member_context ?

Done.

https://codereview.chromium.org/409373002/diff/1/Source/bindings/scripts/v8_d...
Source/bindings/scripts/v8_dictionary.py:57: if member.default_value.is_null:
On 2014/07/23 15:49:01, haraken wrote:
> 
> Just help me understand: When do we hit this branch? Can we add a test case
for
> this?

dictionary {
  double? doubleOrNullMember = null;
}

Added a test case.

https://codereview.chromium.org/409373002/diff/1/Source/bindings/scripts/v8_d...
Source/bindings/scripts/v8_dictionary.py:59: default_value =
str(member.default_value)
On 2014/07/23 15:49:01, haraken wrote:
> 
> default_value => cpp_default_value

Done.

https://codereview.chromium.org/409373002/diff/1/Source/bindings/scripts/v8_d...
Source/bindings/scripts/v8_dictionary.py:66: default_value, v8_default_value =
default_values()
On 2014/07/23 15:49:01, haraken wrote:
> 
> decalut_value => cpp_decault_value

Done.

https://codereview.chromium.org/409373002/diff/1/Source/bindings/templates/di...
File Source/bindings/templates/dictionary_v8.h (right):

https://codereview.chromium.org/409373002/diff/1/Source/bindings/templates/di...
Source/bindings/templates/dictionary_v8.h:21: v8::Handle<v8::Value>
toV8({{cpp_class}}*, v8::Handle<v8::Object> creationContext, v8::Isolate*);
On 2014/07/23 15:49:01, haraken wrote:
> 
> Drop |creationContext|.

We need |creationContext| if a dictionary member is an interface(e.g. Element)
because we will call toV8() for the interface member. Added a test case.

https://codereview.chromium.org/409373002/diff/1/Source/bindings/tests/idls/T...
File Source/bindings/tests/idls/TestDictionary.idl (right):

https://codereview.chromium.org/409373002/diff/1/Source/bindings/tests/idls/T...
Source/bindings/tests/idls/TestDictionary.idl:8: long longMember = 1;
On 2014/07/23 15:49:01, haraken wrote:
> 
> Would you add a bit more test cases? Although you don't need to cover all
types,
> you might want to add more things you've already supported.

Added string, interface, nullable cases. I didn't add array and sequence cases
because I'm not sure they work for all types (e.g. sequence<SomeInterface>).

https://codereview.chromium.org/409373002/diff/20001/Source/bindings/tests/re...
File Source/bindings/tests/results/V8TestDictionary.cpp (right):

https://codereview.chromium.org/409373002/diff/20001/Source/bindings/tests/re...
Source/bindings/tests/results/V8TestDictionary.cpp:57:
v8Object->Set(v8String(isolate, "doubleOrNullMember"), v8::Null(isolate));
a nullable type with the default value "null".

https://codereview.chromium.org/409373002/diff/20001/Source/bindings/tests/re...
Source/bindings/tests/results/V8TestDictionary.cpp:67:
v8Object->Set(v8String(isolate, "stringOrNullMember"), String("default string
value").isNull() ? v8::Handle<v8::Value>(v8::Null(isolate)) : v8String(isolate,
String("default string value")));
Oh, we should fix this (and fixed).

[haraken, 2014-07-25 02:09:47]

Still LGTM. It looks good to land this and iterate!

https://codereview.chromium.org/409373002/diff/1/Source/bindings/templates/di...
File Source/bindings/templates/dictionary_v8.h (right):

https://codereview.chromium.org/409373002/diff/1/Source/bindings/templates/di...
Source/bindings/templates/dictionary_v8.h:21: v8::Handle<v8::Value>
toV8({{cpp_class}}*, v8::Handle<v8::Object> creationContext, v8::Isolate*);
On 2014/07/25 01:59:02, bashi1 wrote:
> On 2014/07/23 15:49:01, haraken wrote:
> > 
> > Drop |creationContext|.
> 
> We need |creationContext| if a dictionary member is an interface(e.g. Element)
> because we will call toV8() for the interface member. Added a test case.

ah, I just meant you can remove the variable name from the method declaration.

[bashi, 2014-07-25 02:18:06]

https://codereview.chromium.org/409373002/diff/1/Source/bindings/templates/di...
File Source/bindings/templates/dictionary_v8.h (right):

https://codereview.chromium.org/409373002/diff/1/Source/bindings/templates/di...
Source/bindings/templates/dictionary_v8.h:21: v8::Handle<v8::Value>
toV8({{cpp_class}}*, v8::Handle<v8::Object> creationContext, v8::Isolate*);
On 2014/07/25 02:09:46, haraken wrote:
> On 2014/07/25 01:59:02, bashi1 wrote:
> > On 2014/07/23 15:49:01, haraken wrote:
> > > 
> > > Drop |creationContext|.
> > 
> > We need |creationContext| if a dictionary member is an interface(e.g.
Element)
> > because we will call toV8() for the interface member. Added a test case.
> 
> ah, I just meant you can remove the variable name from the method declaration.

Oh.. Done.

[bashi, 2014-07-25 02:18:10]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2014-07-25 02:19:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bashi@chromium.org/409373002/60001

[commit-bot: I haz the power, 2014-07-25 03:22:01]

Change committed as 178894

[jsbell, 2014-07-25 17:20:35]

On 2014/07/25 01:59:03, bashi1 wrote:
> @jsbell, thanks for your interest.
> 
> Could you refer TestDictionary.{cpp,h} in the WIP patch[1]?

Thanks for the link (starred, and I'll try and review) and summary!
 
> Do you have cases in which you
> need to distinguish a member is 'not specified' or 'null'? I'd like to gather
> possible customer's feedback as much as possible, so please let me know your
> concerns.

It looks like the specs I'm involved in are all updated with defaults, apart
from Service Worker (which is pretty rough). So I don't think I have specific
needs at the moment, but I'll keep an eye out; it caught my interest for some
reason I don't remember...

[bashi, 2014-07-26 12:49:50]

On 2014/07/25 17:20:35, jsbell wrote:
> On 2014/07/25 01:59:03, bashi1 wrote:
> > @jsbell, thanks for your interest.
> > 
> > Could you refer TestDictionary.{cpp,h} in the WIP patch[1]?
> 
> Thanks for the link (starred, and I'll try and review) and summary!
>  
> > Do you have cases in which you
> > need to distinguish a member is 'not specified' or 'null'? I'd like to
gather
> > possible customer's feedback as much as possible, so please let me know your
> > concerns.
> 
> It looks like the specs I'm involved in are all updated with defaults, apart
> from Service Worker (which is pretty rough). So I don't think I have specific
> needs at the moment, but I'll keep an eye out; it caught my interest for some
> reason I don't remember...

Thanks for the input. IMHO, we can defer providing a way to check whether a
member is specified until we really need it. I'd appreciate if you could keep an
eye on this:)