Add IDLDictionaryBase (issue 2183623004 by bashi@chromium.org)

0 views
Skip to first unread message

ba...@chromium.org

unread,
Jul 26, 2016, 6:43:15 AM7/26/16
to yukishiin...@chromium.org, har...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org
Reviewers: Yuki, haraken
CL: https://codereview.chromium.org/2183623004/

Message:
PTAL

Description:
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

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+93, -20 lines):
A third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.h
A third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.cpp
M third_party/WebKit/Source/bindings/core/v8/ToV8.h
M third_party/WebKit/Source/bindings/core/v8/v8.gypi
M third_party/WebKit/Source/bindings/scripts/v8_dictionary.py
M third_party/WebKit/Source/bindings/templates/dictionary_impl.h
M third_party/WebKit/Source/bindings/templates/dictionary_v8.h
M third_party/WebKit/Source/bindings/templates/dictionary_v8.cpp
M third_party/WebKit/Source/bindings/tests/results/core/TestDictionary.h
M third_party/WebKit/Source/bindings/tests/results/core/TestDictionary.cpp
M third_party/WebKit/Source/bindings/tests/results/core/TestDictionaryDerivedImplementedAs.h
M third_party/WebKit/Source/bindings/tests/results/core/TestInterfaceEventInit.h
M third_party/WebKit/Source/bindings/tests/results/core/TestPermissiveDictionary.h
M third_party/WebKit/Source/bindings/tests/results/core/TestPermissiveDictionary.cpp
M third_party/WebKit/Source/bindings/tests/results/core/V8TestDictionary.h
M third_party/WebKit/Source/bindings/tests/results/core/V8TestDictionary.cpp
M third_party/WebKit/Source/bindings/tests/results/core/V8TestDictionaryDerived.h
M third_party/WebKit/Source/bindings/tests/results/core/V8TestDictionaryDerived.cpp
M third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceEventInit.h
M third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceEventInit.cpp
M third_party/WebKit/Source/bindings/tests/results/core/V8TestPermissiveDictionary.h
M third_party/WebKit/Source/bindings/tests/results/core/V8TestPermissiveDictionary.cpp


har...@chromium.org

unread,
Jul 26, 2016, 6:50:14 AM7/26/16
to ba...@chromium.org, yukishiin...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org
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/

yukis...@chromium.org

unread,
Jul 26, 2016, 6:59:05 AM7/26/16
to ba...@chromium.org, har...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org

yukis...@chromium.org

unread,
Jul 26, 2016, 6:59:56 AM7/26/16
to ba...@chromium.org, har...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org
LGTM with nits.


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

https://codereview.chromium.org/2183623004/diff/20001/third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.cpp#newcode9
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/Source/bindings/core/v8/IDLDictionaryBase.cpp#newcode13
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/Source/bindings/core/v8/IDLDictionaryBase.cpp#newcode19
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/Source/bindings/templates/dictionary_impl.h
File third_party/WebKit/Source/bindings/templates/dictionary_impl.h
(right):

https://codereview.chromium.org/2183623004/diff/20001/third_party/WebKit/Source/bindings/templates/dictionary_impl.h#newcode27
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.

https://codereview.chromium.org/2183623004/

har...@chromium.org

unread,
Jul 26, 2016, 7:04:30 AM7/26/16
to ba...@chromium.org, yukishiin...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org
Sorry, I was looking at totally different code...

LGTM.


https://codereview.chromium.org/2183623004/

yukis...@chromium.org

unread,
Jul 26, 2016, 8:22:56 AM7/26/16
to ba...@chromium.org, har...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org
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.

https://codereview.chromium.org/2183623004/

ba...@chromium.org

unread,
Jul 26, 2016, 10:36:33 PM7/26/16
to yukishiin...@chromium.org, har...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org
> 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/Source/bindings/core/v8/IDLDictionaryBase.cpp
File third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.cpp
(right):

https://codereview.chromium.org/2183623004/diff/20001/third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.cpp#newcode9
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/Source/bindings/core/v8/IDLDictionaryBase.cpp#newcode13
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/Source/bindings/core/v8/IDLDictionaryBase.cpp#newcode19
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/Source/bindings/templates/dictionary_impl.h
File third_party/WebKit/Source/bindings/templates/dictionary_impl.h
(right):

https://codereview.chromium.org/2183623004/diff/20001/third_party/WebKit/Source/bindings/templates/dictionary_impl.h#newcode27
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/Source/core/testing/InternalDictionaryDerivedDerived.idl
File
third_party/WebKit/Source/core/testing/InternalDictionaryDerivedDerived.idl
(right):

https://codereview.chromium.org/2183623004/diff/40001/third_party/WebKit/Source/core/testing/InternalDictionaryDerivedDerived.idl#newcode5
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.

https://codereview.chromium.org/2183623004/

har...@chromium.org

unread,
Jul 27, 2016, 12:24:43 AM7/27/16
to ba...@chromium.org, yukishiin...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org

yukis...@chromium.org

unread,
Jul 27, 2016, 1:38:33 AM7/27/16
to ba...@chromium.org, har...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org
LGTM.


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

https://codereview.chromium.org/2183623004/diff/40001/third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.h#newcode21
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/Source/bindings/core/v8/IDLDictionaryBase.h#newcode22
third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.h:22:
virtual ~IDLDictionaryBase() {}
ditto.

https://codereview.chromium.org/2183623004/

ba...@chromium.org

unread,
Jul 27, 2016, 5:01:20 AM7/27/16
to yukishiin...@chromium.org, har...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org

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

https://codereview.chromium.org/2183623004/diff/40001/third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.h#newcode21
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/Source/bindings/core/v8/IDLDictionaryBase.h#newcode22
third_party/WebKit/Source/bindings/core/v8/IDLDictionaryBase.h:22:
virtual ~IDLDictionaryBase() {}
On 2016/07/27 05:38:33, Yuki wrote:
> ditto.

Done.

https://codereview.chromium.org/2183623004/

commit-bot@chromium.org via codereview.chromium.org

unread,
Jul 27, 2016, 5:01:49 AM7/27/16
to ba...@chromium.org, yukishiin...@chromium.org, har...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Jul 27, 2016, 7:04:57 AM7/27/16
to ba...@chromium.org, yukishiin...@chromium.org, har...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org
Committed patchset #4 (id:60001)

https://codereview.chromium.org/2183623004/

commit-bot@chromium.org via codereview.chromium.org

unread,
Jul 27, 2016, 7:06:44 AM7/27/16
to ba...@chromium.org, yukishiin...@chromium.org, har...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org
Patchset 4 (id:??) landed as
https://crrev.com/190cffeadfe46c6419789d586551c877b24f082f
Cr-Commit-Position: refs/heads/master@{#408093}

https://codereview.chromium.org/2183623004/
Reply all
Reply to author
Forward
0 new messages