Add IDLDictionaryBase

Add IDLDictionaryBase

Before this CL, given an IDL dictionary (e.g. FooDictionary)
the code generator generated ToV8() directly.
This is problematic when Blink wants to pass a
sub-dictionary of FooDictionary because generated
ToV8() doesn't convert members defined in the
sub-dictionary.

To solve this problem, add a base class which
provides toV8Impl() virtual function. The code
generator overrides toV8Impl() and ToV8() uses
them. ToV8() automatically calls appropriate
toV8Impl().

BUG=630210
Committed: https://crrev.com/190cffeadfe46c6419789d586551c877b24f082f
Cr-Commit-Position: refs/heads/master@{#408093}

[bashi, 2016-07-26 10:11:32]

The CQ bit was checked by bashi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 10:11:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bashi, 2016-07-26 10:40:02]

Description was changed from

==========
Add IDLDictionaryBase

Description to be written.

BUG=630210
==========

to

==========
Add IDLDictionaryBase

Before this CL, given an IDL dictionary (e.g. FooDictionary)
the code generator generated ToV8() directly.
This is problematic when Blink wants to pass a
sub-dictionary of FooDictionary because generated
ToV8() doesn't convert members defined in the
sub-dictionary.

To solve this problem, add a base class which
provides toV8Impl() virtual function. The code
generator overrides toV8Impl() and ToV8() uses
them. ToV8() automatically calls appropriate
toV8Impl().

BUG=630210
==========

[bashi, 2016-07-26 10:41:47]

The CQ bit was checked by bashi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 10:42:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bashi, 2016-07-26 10:43:14]

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

[bashi, 2016-07-26 10:43:15]

PTAL

[haraken, 2016-07-26 10:50:13]

Just help me understand:

If we have:

dictionary A { ... };

dictionary B : A { ... };

B::toV8Impl needs to call A::toV8Impl(), right? With this CL, it looks like
B::toV8Impl is not calling A::toV8Impl().

(Disclaimer: I'm not familiar with how sub-dictionaries are implemented.)

[Yuki, 2016-07-26 10:59:05]

On 2016/07/26 10:50:13, haraken wrote:
> Just help me understand:
> 
> If we have:
> 
> dictionary A { ... };
> 
> dictionary B : A { ... };
> 
> B::toV8Impl needs to call A::toV8Impl(), right? With this CL, it looks like
> B::toV8Impl is not calling A::toV8Impl().
> 
> (Disclaimer: I'm not familiar with how sub-dictionaries are implemented.)

https://codereview.chromium.org/2183623004/diff/20001/third_party/WebKit/Sour...
In toV8Impl(), both of toV8A() and toV8B() are called.

[Yuki, 2016-07-26 10:59:55]

LGTM with nits.

https://codereview.chromium.org/2183623004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.cpp (right):

https://codereview.chromium.org/2183623004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.cpp:9:
IDLDictionaryBase::IDLDictionaryBase()
nit: You can make this constructor an inline function.

https://codereview.chromium.org/2183623004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.cpp:13:
IDLDictionaryBase::~IDLDictionaryBase()
nit: ditto.  It's possible that compilers do optimization even if it's a virtual
function.  You can make this inline.

https://codereview.chromium.org/2183623004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.cpp:19: return
v8::Local<v8::Value>();
NOTREACHED()?

https://codereview.chromium.org/2183623004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/dictionary_impl.h (right):

https://codereview.chromium.org/2183623004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/dictionary_impl.h:27:
v8::Local<v8::Value> toV8Impl(v8::Local<v8::Object>, v8::Isolate*) const
override;
nit: Let's explicitly say |creationContext| because it's not trivial from the
type name.

[haraken, 2016-07-26 11:04:30]

On 2016/07/26 10:59:05, Yuki wrote:
> On 2016/07/26 10:50:13, haraken wrote:
> > Just help me understand:
> > 
> > If we have:
> > 
> > dictionary A { ... };
> > 
> > dictionary B : A { ... };
> > 
> > B::toV8Impl needs to call A::toV8Impl(), right? With this CL, it looks like
> > B::toV8Impl is not calling A::toV8Impl().
> > 
> > (Disclaimer: I'm not familiar with how sub-dictionaries are implemented.)
> 
>
https://codereview.chromium.org/2183623004/diff/20001/third_party/WebKit/Sour...
> In toV8Impl(), both of toV8A() and toV8B() are called.

Sorry, I was looking at totally different code...

LGTM.

[Yuki, 2016-07-26 12:22:55]

On 2016/07/26 11:04:30, haraken wrote:
> On 2016/07/26 10:59:05, Yuki wrote:
> > On 2016/07/26 10:50:13, haraken wrote:
> > > Just help me understand:
> > > 
> > > If we have:
> > > 
> > > dictionary A { ... };
> > > 
> > > dictionary B : A { ... };
> > > 
> > > B::toV8Impl needs to call A::toV8Impl(), right? With this CL, it looks
like
> > > B::toV8Impl is not calling A::toV8Impl().
> > > 
> > > (Disclaimer: I'm not familiar with how sub-dictionaries are implemented.)
> > 
> >
>
https://codereview.chromium.org/2183623004/diff/20001/third_party/WebKit/Sour...
> > In toV8Impl(), both of toV8A() and toV8B() are called.
> 
> Sorry, I was looking at totally different code...
> 
> LGTM.

haraken@ was right.  This CL and the current implementation seem not supporting
multi-level inheritance correctly.

dictionary A {};
dictionary B : A {};
dictionary C : B {};

C c;
toV8(c);

In this case, toV8(c) calls toV8B() and toV8C(), and never calls toV8A() IUCC.

The current implementation seems wrong, too.

[commit-bot: I haz the power, 2016-07-26 14:14:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-26 14:14:14]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[bashi, 2016-07-27 01:33:43]

The CQ bit was checked by bashi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 01:33:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bashi, 2016-07-27 02:36:33]

> haraken@ was right.  This CL and the current implementation seem not
supporting
> multi-level inheritance correctly.
> 
> dictionary A {};
> dictionary B : A {};
> dictionary C : B {};
> 
> C c;
> toV8(c);
> 
> In this case, toV8(c) calls toV8B() and toV8C(), and never calls toV8A() IUCC.
> 
> The current implementation seems wrong, too.

Good catch! It's wrong. Fixed.

https://codereview.chromium.org/2183623004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.cpp (right):

https://codereview.chromium.org/2183623004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.cpp:9:
IDLDictionaryBase::IDLDictionaryBase()
On 2016/07/26 10:59:55, Yuki wrote:
> nit: You can make this constructor an inline function.

I had a vague recollection that some linkers complained when I used inline
constructor/destructor but I don't remember why and it should be OK.

Done.

https://codereview.chromium.org/2183623004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.cpp:13:
IDLDictionaryBase::~IDLDictionaryBase()
On 2016/07/26 10:59:55, Yuki wrote:
> nit: ditto.  It's possible that compilers do optimization even if it's a
virtual
> function.  You can make this inline.

Done.

https://codereview.chromium.org/2183623004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.cpp:19: return
v8::Local<v8::Value>();
On 2016/07/26 10:59:55, Yuki wrote:
> NOTREACHED()?

Done.

https://codereview.chromium.org/2183623004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/dictionary_impl.h (right):

https://codereview.chromium.org/2183623004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/dictionary_impl.h:27:
v8::Local<v8::Value> toV8Impl(v8::Local<v8::Object>, v8::Isolate*) const
override;
On 2016/07/26 10:59:55, Yuki wrote:
> nit: Let's explicitly say |creationContext| because it's not trivial from the
> type name.

Done.

https://codereview.chromium.org/2183623004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/InternalDictionaryDerivedDerived.idl
(right):

https://codereview.chromium.org/2183623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/InternalDictionaryDerivedDerived.idl:5:
dictionary InternalDictionaryDerivedDerived : InternalDictionaryDerived {
This is poorly named but I couldn't come up with a good naming.

[commit-bot: I haz the power, 2016-07-27 03:09:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-27 03:09:52]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-07-27 04:24:42]

LGTM

[Yuki, 2016-07-27 05:38:33]

LGTM.

https://codereview.chromium.org/2183623004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.h (right):

https://codereview.chromium.org/2183623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.h:21:
IDLDictionaryBase() {}
I don't know why, but Blink's coding style is
    IDLDictionaryBase() { }  // one space between { and }.

https://codereview.chromium.org/2183623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.h:22: virtual
~IDLDictionaryBase() {}
ditto.

[bashi, 2016-07-27 09:01:20]

https://codereview.chromium.org/2183623004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.h (right):

https://codereview.chromium.org/2183623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.h:21:
IDLDictionaryBase() {}
On 2016/07/27 05:38:33, Yuki wrote:
> I don't know why, but Blink's coding style is
>     IDLDictionaryBase() { }  // one space between { and }.

Done.

https://codereview.chromium.org/2183623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.h:22: virtual
~IDLDictionaryBase() {}
On 2016/07/27 05:38:33, Yuki wrote:
> ditto.

Done.

[bashi, 2016-07-27 09:01:30]

The CQ bit was checked by bashi@chromium.org

[bashi, 2016-07-27 09:01:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2183623004/#ps60001
(title: "comment")

[commit-bot: I haz the power, 2016-07-27 09:01:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 11:04:56]

Description was changed from

==========
Add IDLDictionaryBase

Before this CL, given an IDL dictionary (e.g. FooDictionary)
the code generator generated ToV8() directly.
This is problematic when Blink wants to pass a
sub-dictionary of FooDictionary because generated
ToV8() doesn't convert members defined in the
sub-dictionary.

To solve this problem, add a base class which
provides toV8Impl() virtual function. The code
generator overrides toV8Impl() and ToV8() uses
them. ToV8() automatically calls appropriate
toV8Impl().

BUG=630210
==========

to

==========
Add IDLDictionaryBase

Before this CL, given an IDL dictionary (e.g. FooDictionary)
the code generator generated ToV8() directly.
This is problematic when Blink wants to pass a
sub-dictionary of FooDictionary because generated
ToV8() doesn't convert members defined in the
sub-dictionary.

To solve this problem, add a base class which
provides toV8Impl() virtual function. The code
generator overrides toV8Impl() and ToV8() uses
them. ToV8() automatically calls appropriate
toV8Impl().

BUG=630210
==========

[commit-bot: I haz the power, 2016-07-27 11:04:57]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-27 11:06:42]

Description was changed from

==========
Add IDLDictionaryBase

Before this CL, given an IDL dictionary (e.g. FooDictionary)
the code generator generated ToV8() directly.
This is problematic when Blink wants to pass a
sub-dictionary of FooDictionary because generated
ToV8() doesn't convert members defined in the
sub-dictionary.

To solve this problem, add a base class which
provides toV8Impl() virtual function. The code
generator overrides toV8Impl() and ToV8() uses
them. ToV8() automatically calls appropriate
toV8Impl().

BUG=630210
==========

to

==========
Add IDLDictionaryBase

Before this CL, given an IDL dictionary (e.g. FooDictionary)
the code generator generated ToV8() directly.
This is problematic when Blink wants to pass a
sub-dictionary of FooDictionary because generated
ToV8() doesn't convert members defined in the
sub-dictionary.

To solve this problem, add a base class which
provides toV8Impl() virtual function. The code
generator overrides toV8Impl() and ToV8() uses
them. ToV8() automatically calls appropriate
toV8Impl().

BUG=630210

Committed: https://crrev.com/190cffeadfe46c6419789d586551c877b24f082f
Cr-Commit-Position: refs/heads/master@{#408093}
==========

[commit-bot: I haz the power, 2016-07-27 11:06:43]

Patchset 4 (id:??) landed as
https://crrev.com/190cffeadfe46c6419789d586551c877b24f082f
Cr-Commit-Position: refs/heads/master@{#408093}