IDL: Basic dictionary inheritance support

IDL: Basic dictionary inheritance support

There are some specs that use dictionary inheritance.
This CL is the first step to add dictionary inheritance support.

Suppose we have following definitions:
  dictionary Base { long baseMember; };
  dictionary Derived : Base { DOMString derivedMember; };
The code generator will create "Derived" impl class which inherits
from "Base". "Derived" class has accessor methods for "baseMember"
along with those of "derivedMember".

Blink developers can use derived dictionaries as method arguments
and can upcast them. However, there is no way to downcast a base
dictionary to a derived dictionary for now. Following CLs should
support downcasting.

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

[bashi, 2014-11-19 01:13:26]

Patchset #1 (id:1) has been deleted

[bashi, 2014-11-19 01:59:07]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org, jl@opera.com

[bashi, 2014-11-19 01:59:07]

PTAL?

[Jens Widell, 2014-11-19 06:41:47]

LGTM

https://codereview.chromium.org/740453004/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/740453004/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:307: return interfaces_info.get(name,
{}).get('implemented_as') or name
Pass 'name' as the default, to the second get()?

https://codereview.chromium.org/740453004/diff/20001/Source/bindings/template...
File Source/bindings/templates/dictionary_v8.h (right):

https://codereview.chromium.org/740453004/diff/20001/Source/bindings/template...
Source/bindings/templates/dictionary_v8.h:24: void toV8Internal({{cpp_class}}&,
v8::Handle<v8::Object> dest, v8::Handle<v8::Object> creationContext,
v8::Isolate*);
We could call this toV8{{cpp_class}}() instead. That way, we can call the parent
dictionary's version without explicit casts.

[bashi, 2014-11-19 07:35:22]

Thank you for review!

We (bindings team in Tokyo) discussed offline how to support downcast. Even
though I'm not sure that I could provide an efficient way to downcast in this
direction, I'd like to give it a try.

https://codereview.chromium.org/740453004/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/740453004/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:307: return interfaces_info.get(name,
{}).get('implemented_as') or name
On 2014/11/19 06:41:47, Jens Widell wrote:
> Pass 'name' as the default, to the second get()?

It seems that get() returns None when the associated value is None. e.g.

{'implemented_as': None}.get('implemented_as', 'foo') -> None

https://codereview.chromium.org/740453004/diff/20001/Source/bindings/template...
File Source/bindings/templates/dictionary_v8.h (right):

https://codereview.chromium.org/740453004/diff/20001/Source/bindings/template...
Source/bindings/templates/dictionary_v8.h:24: void toV8Internal({{cpp_class}}&,
v8::Handle<v8::Object> dest, v8::Handle<v8::Object> creationContext,
v8::Isolate*);
On 2014/11/19 06:41:47, Jens Widell wrote:
> We could call this toV8{{cpp_class}}() instead. That way, we can call the
parent
> dictionary's version without explicit casts.

This looks much better:) Done.

[Jens Widell, 2014-11-19 07:38:53]

https://codereview.chromium.org/740453004/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/740453004/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:307: return interfaces_info.get(name,
{}).get('implemented_as') or name
On 2014/11/19 07:35:22, bashi1 wrote:
> On 2014/11/19 06:41:47, Jens Widell wrote:
> > Pass 'name' as the default, to the second get()?
> 
> It seems that get() returns None when the associated value is None. e.g.
> 
> {'implemented_as': None}.get('implemented_as', 'foo') -> None

Ah, yes, it does.

Let's not pass 'name' as the default, to the second get(). :-)

[haraken, 2014-11-19 09:11:27]

LGTM

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_dictionary.py (right):

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_dictionary.py:56: 'v8_class': 'V8' + cpp_class,

v8_types.v8_type(cpp_class) ?

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_dictionary.py:64: 'parent_v8_class': 'V8' +
parent_cpp_class,

v8_types.v8_type(parent_cpp_class) ?

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/template...
File Source/bindings/templates/dictionary_impl.cpp (right):

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/template...
Source/bindings/templates/dictionary_impl.cpp:27:
{{parent_cpp_class}}::trace(visitor);

Would you move this down to the tail of this trace method? We normally trace
parent classes at the tail.

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/template...
File Source/bindings/templates/dictionary_v8.cpp (right):

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/template...
Source/bindings/templates/dictionary_v8.cpp:71: void
toV8{{cpp_class}}({{cpp_class}}& impl, v8::Handle<v8::Object> dest,
v8::Handle<v8::Object> creationContext, v8::Isolate* isolate)

dest => dictionary ?

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/template...
File Source/bindings/templates/dictionary_v8.h (right):

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/template...
Source/bindings/templates/dictionary_v8.h:24: void
toV8{{cpp_class}}({{cpp_class}}&, v8::Handle<v8::Object> dest,
v8::Handle<v8::Object> creationContext, v8::Isolate*);

dest => dictionary ?

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/tests/re...
File Source/bindings/tests/results/core/TestDictionaryDerived.h (right):

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/TestDictionaryDerived.h:16: class
TestDictionaryDerivedImplementedAs : public TestDictionary {

Add final.

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/TestDictionaryDerived.h:29: virtual void
trace(Visitor*);

Can we add override ?

[bashi, 2014-11-19 11:04:43]

Thank you for review!

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_dictionary.py (right):

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_dictionary.py:56: 'v8_class': 'V8' + cpp_class,
On 2014/11/19 09:11:26, haraken wrote:
> 
> v8_types.v8_type(cpp_class) ?
> 

Done.

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_dictionary.py:64: 'parent_v8_class': 'V8' +
parent_cpp_class,
On 2014/11/19 09:11:26, haraken wrote:
> 
> v8_types.v8_type(parent_cpp_class) ?

Done.

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/template...
File Source/bindings/templates/dictionary_impl.cpp (right):

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/template...
Source/bindings/templates/dictionary_impl.cpp:27:
{{parent_cpp_class}}::trace(visitor);
On 2014/11/19 09:11:26, haraken wrote:
> 
> Would you move this down to the tail of this trace method? We normally trace
> parent classes at the tail.

Done.

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/template...
File Source/bindings/templates/dictionary_v8.cpp (right):

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/template...
Source/bindings/templates/dictionary_v8.cpp:71: void
toV8{{cpp_class}}({{cpp_class}}& impl, v8::Handle<v8::Object> dest,
v8::Handle<v8::Object> creationContext, v8::Isolate* isolate)
On 2014/11/19 09:11:26, haraken wrote:
> 
> dest => dictionary ?

Done.

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/template...
File Source/bindings/templates/dictionary_v8.h (right):

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/template...
Source/bindings/templates/dictionary_v8.h:24: void
toV8{{cpp_class}}({{cpp_class}}&, v8::Handle<v8::Object> dest,
v8::Handle<v8::Object> creationContext, v8::Isolate*);
On 2014/11/19 09:11:26, haraken wrote:
> 
> dest => dictionary ?

Done.

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/tests/re...
File Source/bindings/tests/results/core/TestDictionaryDerived.h (right):

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/TestDictionaryDerived.h:16: class
TestDictionaryDerivedImplementedAs : public TestDictionary {
On 2014/11/19 09:11:26, haraken wrote:
> 
> Add final.

It somewhat difficult to know whether the class doesn't have subclasses (need to
know all inheritance hierarchy). I'd like to address it later. Added a FIXME
comment.

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/TestDictionaryDerived.h:29: virtual void
trace(Visitor*);
On 2014/11/19 09:11:26, haraken wrote:
> 
> Can we add override ?

Unfortunately, no. We use the same template for the base class and derived
class.

[haraken, 2014-11-19 11:50:33]

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/tests/re...
File Source/bindings/tests/results/core/TestDictionaryDerived.h (right):

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/TestDictionaryDerived.h:16: class
TestDictionaryDerivedImplementedAs : public TestDictionary {
On 2014/11/19 11:04:43, bashi1 wrote:
> On 2014/11/19 09:11:26, haraken wrote:
> > 
> > Add final.
> 
> It somewhat difficult to know whether the class doesn't have subclasses (need
to
> know all inheritance hierarchy). I'd like to address it later. Added a FIXME
> comment.

Makes sense. You don't need to try hard to add final. Simplicity of the IDL
compiler would be more important.

https://codereview.chromium.org/740453004/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/TestDictionaryDerived.h:29: virtual void
trace(Visitor*);
On 2014/11/19 11:04:43, bashi1 wrote:
> On 2014/11/19 09:11:26, haraken wrote:
> > 
> > Can we add override ?
> 
> Unfortunately, no. We use the same template for the base class and derived
> class.

Ditto.

[bashi, 2014-11-19 13:18:42]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2014-11-19 13:19:29]

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

[commit-bot: I haz the power, 2014-11-19 15:04:02]

Committed patchset #3 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185600