Issue 386963003: [WIP][NotForLand] IDL dictionary support

Issue 386963003: [WIP][NotForLand] IDL dictionary support (Closed)

Created:
6 years, 5 months ago by bashi

Modified:
6 years ago

Reviewers:
haraken, Nils Barth (inactive)

CC:
tasak (please_use_google.com)

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

Project:
blink

Visibility:
Public.

More Reviews

Description

[WIP][NotForReview] IDL dictionary support TODOs: - GN version. - Composite types (arrays and sequences) support - Nullable support - Inheritance support (For WebAnimation, WebCrypto) Usage examples: - core/dom/SomeDictionary{,User}.idl BUG=321462

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Total comments: 93

Patch Set 7 : #

Patch Set 8 : #

Patch Set 9 : #

Patch Set 10 : sequence and array support #

Created: 6 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1199 lines, -77 lines)			Patch
A	LayoutTests/fast/dom/some-dictionary.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+34 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/dom/some-dictionary-expected.txt	View	1 2 3 4 5 6 7 8 9	1 chunk	+20 lines, -0 lines	0 comments	Download
M	Source/bindings/core/v8/DictionaryHelperForCore.cpp	View	1 2 3 4 5 6 7 8 9	1 chunk	+30 lines, -0 lines	0 comments	Download
M	Source/bindings/core/v8/generated.gyp	View	1 2 3	2 chunks	+27 lines, -0 lines	0 comments	Download
M	Source/bindings/core/v8/generated.gypi	View	1 2 3	2 chunks	+2 lines, -0 lines	0 comments	Download
A	Source/bindings/scripts/aggregate_generated_dictionaries.py	View	1 2 3 4 5 6	1 chunk	+61 lines, -0 lines	0 comments	Download
M	Source/bindings/scripts/code_generator_v8.py	View	1 2 3 4 5 6	4 chunks	+102 lines, -24 lines	0 comments	Download
M	Source/bindings/scripts/compute_interfaces_info_individual.py	View	1 2 3 4 5 6	5 chunks	+14 lines, -7 lines	0 comments	Download
M	Source/bindings/scripts/generate_global_constructors.py	View		2 chunks	+2 lines, -1 line	0 comments	Download
M	Source/bindings/scripts/idl_compiler.py	View	1 2 3 4 5 6	4 chunks	+13 lines, -13 lines	0 comments	Download
M	Source/bindings/scripts/idl_definitions.py	View		2 chunks	+6 lines, -2 lines	0 comments	Download
M	Source/bindings/scripts/idl_reader.py	View		1 chunk	+13 lines, -11 lines	0 comments	Download
M	Source/bindings/scripts/idl_types.py	View	1 2 3 4	5 chunks	+14 lines, -0 lines	0 comments	Download
M	Source/bindings/scripts/interface_dependency_resolver.py	View	1 2 3 4 5 6	1 chunk	+4 lines, -0 lines	0 comments	Download
M	Source/bindings/scripts/scripts.gypi	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/bindings/scripts/utilities.py	View		2 chunks	+8 lines, -0 lines	0 comments	Download
A	Source/bindings/scripts/v8_dictionary.py	View	1 2 3 4 5 6 7 8 9	1 chunk	+167 lines, -0 lines	0 comments	Download
M	Source/bindings/scripts/v8_methods.py	View	1 2 3 4 5 6	2 chunks	+6 lines, -2 lines	0 comments	Download
M	Source/bindings/scripts/v8_types.py	View	1 2 3 4 5 6 7 8 9	6 chunks	+14 lines, -7 lines	0 comments	Download
A	Source/bindings/templates/dictionary_impl.h	View	1 2 3 4 5 6 7 8	1 chunk	+43 lines, -0 lines	0 comments	Download
A	Source/bindings/templates/dictionary_impl.cpp	View	1 2 3 4 5 6 7 8	1 chunk	+36 lines, -0 lines	0 comments	Download
A	Source/bindings/templates/dictionary_v8.h	View	1 2 3 4 5 6	1 chunk	+31 lines, -0 lines	0 comments	Download
A	Source/bindings/templates/dictionary_v8.cpp	View	1 2 3 4 5 6 7 8	1 chunk	+48 lines, -0 lines	0 comments	Download
M	Source/bindings/templates/templates.gypi	View		1 chunk	+4 lines, -0 lines	0 comments	Download
A	Source/bindings/tests/idls/TestDictionary.idl	View	1 2 3 4 5 6 7 8 9	1 chunk	+18 lines, -0 lines	0 comments	Download
M	Source/bindings/tests/idls/TestObject.idl	View	1	2 chunks	+4 lines, -10 lines	0 comments	Download
A	Source/bindings/tests/results/TestDictionary.h	View	1 2 3 4 5 6 7 8 9	1 chunk	+82 lines, -0 lines	0 comments	Download
A	Source/bindings/tests/results/TestDictionary.cpp	View	1 2 3 4 5 6 7 8 9	1 chunk	+78 lines, -0 lines	0 comments	Download
A	Source/bindings/tests/results/V8TestDictionary.h	View	1 2 3 4 5 6	1 chunk	+31 lines, -0 lines	0 comments	Download
A	Source/bindings/tests/results/V8TestDictionary.cpp	View	1 2 3 4 5 6 7 8 9	1 chunk	+92 lines, -0 lines	0 comments	Download
M	Source/bindings/tests/results/V8TestObject.cpp	View	1 2 3 4 5 6	3 chunks	+67 lines, -0 lines	0 comments	Download
M	Source/core/core.gyp	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/core.gypi	View	1 2 3 4 5 6	2 chunks	+6 lines, -0 lines	0 comments	Download
A	Source/core/dom/SomeDictionary.idl	View	1 2 3 4 5 6 7 8 9	1 chunk	+13 lines, -0 lines	0 comments	Download
A	Source/core/dom/SomeDictionaryUser.h	View	1 2 3 4 5 6 7 8 9	1 chunk	+41 lines, -0 lines	0 comments	Download
A	Source/core/dom/SomeDictionaryUser.cpp	View	1 2 3 4 5 6 7 8 9	1 chunk	+46 lines, -0 lines	0 comments	Download
A	Source/core/dom/SomeDictionaryUser.idl	View	1 2 3 4	1 chunk	+7 lines, -0 lines	0 comments	Download
M	Source/core/testing/Internals.h	View	1 2 3 4 5 6	3 chunks	+4 lines, -0 lines	0 comments	Download
M	Source/core/testing/Internals.cpp	View	1 2 3 4 5 6	1 chunk	+7 lines, -0 lines	0 comments	Download
M	Source/core/testing/Internals.idl	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download

Messages

Total messages: 13 (0 generated)

Expand Messages | Collapse Messages

bashi

Haraken-san, Could you glance over this CL? This is a proof-of-concept patch for dictionary support. ...

6 years, 5 months ago (2014-07-18 09:58:53 UTC) #1

Nils Barth (inactive)

Bunch of Python nits, but overall looks structurally good! Key question to be careful of: ...

6 years, 5 months ago (2014-07-18 21:52:35 UTC) #2

haraken

Sorry about the review delay. Overall the approach looks good! https://codereview.chromium.org/386963003/diff/100001/Source/bindings/core/v8/DictionaryHelperForCore.cpp File Source/bindings/core/v8/DictionaryHelperForCore.cpp (right): https://codereview.chromium.org/386963003/diff/100001/Source/bindings/core/v8/DictionaryHelperForCore.cpp#newcode393 ...

6 years, 5 months ago (2014-07-21 16:11:59 UTC) #3

bashi

Thank you for detailed review, Nils and Haraken-san! > Key question to be careful of: ...

6 years, 5 months ago (2014-07-22 02:33:59 UTC) #4

haraken

> IIUC, if we change a dictionary IDL, we will try to re-generate all bindings ...

6 years, 5 months ago (2014-07-22 04:30:11 UTC) #5

> IIUC, if we change a dictionary IDL, we will try to re-generate all bindings
> .h/.cpp files, but this isn't generally a big problem because we use
> only_if_changed=True. Both DOM(impl) class and bindings class for the
dictionary
> will be changed, but the bindings .h/.cpp files that only use the dictionary
> should not be changed, so no unnecessary recompilation happens.

I'm just curious but why do we need to try to re-generate all bindings when any
dictionary IDL is touched (even though I understand it doesn't trigger full
recompilation)?

https://codereview.chromium.org/386963003/diff/100001/Source/bindings/tests/r...
File Source/bindings/tests/results/TestDictionary.h (right):

https://codereview.chromium.org/386963003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/TestDictionary.h:17: class TestDictionary : public
GarbageCollectedFinalized<TestDictionary> {
On 2014/07/22 02:33:58, bashi1 wrote:
> On 2014/07/21 16:11:59, haraken wrote:
> > 
> > It makes sense to enable oilpan by default for IDL dictionaries. There is no
> > reason to use transition types for new things :)
> 
> :)
> 
> Should I remove Finalized suffix if I want to make the class be a superclass
of
> some class? (for dictionary inheritance support)

"Finalized" is not related to inheritance. "Finalized" controls whether oilpan's
GC should call a destructor or not. If you don't need any destructor of this
dictionary hierarchy, you can omit "Finalized". Otherwise you need "Finalized".

Specifically, you need "Finalized" if the dictionary has members that need
destructors such as RefPtr, String etc. I think you can just add "Finalized"
always.

https://codereview.chromium.org/386963003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/TestDictionary.h:45: bool m_hasBooleanMember;
On 2014/07/22 02:33:58, bashi1 wrote:
> On 2014/07/21 16:11:59, haraken wrote:
> > 
> > Can we use Nullable<T>?
> 
> These members are not nullable (no ? suffix in IDL definition).
> 
> IIUC, we need to treat nullable member and non-nullable member separately.
> For non-nullable members,
> - If a JavaScript object doesn't have the property, it should be treated as
> undefined, and |hasXXX()| can be used by blink to check whether it is defined
or
> not.
> - When we create a JavaScript object from dictionary impl class, it should
check
> |hasXXX()| before setting the property.
> 
> For nullable members, we will use Nullable<T> as you suggested (along with
> hasXXX() method, for consistency).
> 
> The CL doesn't support nullable yet, but I'm going to implement it in next
> iteration:)

Nullable<T> is not a class just to represent IDL's nullable value, so you can
use Nullable<T> to implement something that might not have a valid value (but
I'm not sure if you can use Nullable<T> in this case.)

How are you planning to implement nullable values? Like this?

  // Non-nullable long.
  int m_longMember;
  bool m_hasLongMember;

  // Nullable long.
  Nullable<int> m_longMember;
  bool m_hasLongMember;

I wonder if we really need both Nullable<> and m_hasXXXMember, since the fact
that m_hasLongMember is false can imply that the value is null. (Probably I'm
not understanding nullable correctly.)

https://codereview.chromium.org/386963003/diff/100001/Source/bindings/tests/r...
File Source/bindings/tests/results/V8TestDictionary.cpp (right):

https://codereview.chromium.org/386963003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/V8TestDictionary.cpp:16: TestDictionary*
V8TestDictionary::toNativeWithTypeCheck(v8::Isolate* isolate,
v8::Handle<v8::Value> v8Value)
On 2014/07/22 02:33:58, bashi1 wrote:
> On 2014/07/21 16:11:59, haraken wrote:
> > 
> > I'd rename this to toNative().
> > 
> > toNativeWithTypeCheck() is expected to return 0 if |v8Value| is a value of
> wrong
> > type. So the difference between toNative() and toNativeWithTypeCheck()
should
> be
> > whether we check if(v8Value.IsEmpty() || !v8Value->IsObject()) or not.
> > toNative() should not have the check, and toNativeWithTypeCheck() should
have
> > the check.
> 
> Thank you for the explanation. I was wondering I should use toNative() or
> toNativeWithTypeCheck(). Renamed, but I want to confirm that we call this
> toNative() even if its properties can come from toNativeWithTypeCheck()?
suppose
> we have a following definition:
> 
>  dictionary A {
>    Node nodeMember;
>  };
> 
> We will call V8Node::toNativeWithTypeCheck() for nodeMember (in
DictionaryHelper
> for now).

Understood, then we can probably:

- Add the if(v8Value.IsEmpty() || !v8Value->IsObject()) check to this method.
- Name this method toNativeWithTypeCheck().
- Don't define toNative().

What do you think?

bashi

> I'm just curious but why do we need to try to re-generate all bindings ...

6 years, 5 months ago (2014-07-22 05:57:35 UTC) #6

> I'm just curious but why do we need to try to re-generate all bindings when
any
dictionary IDL is touched (even though I understand it doesn't trigger full
recompilation)?

Actually, no need to re-generate all bindings. I thought gyp tries to run
bindings_{core,modules}_v8_generated_individual for all IDL files when an IDL
file has changed. I would be wrong here...

FYI: heading home today, will continue nullable work tomorrow.

https://codereview.chromium.org/386963003/diff/100001/Source/bindings/tests/r...
File Source/bindings/tests/results/TestDictionary.h (right):

https://codereview.chromium.org/386963003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/TestDictionary.h:17: class TestDictionary : public
GarbageCollectedFinalized<TestDictionary> {
On 2014/07/22 04:30:10, haraken wrote:
> On 2014/07/22 02:33:58, bashi1 wrote:
> > On 2014/07/21 16:11:59, haraken wrote:
> > > 
> > > It makes sense to enable oilpan by default for IDL dictionaries. There is
no
> > > reason to use transition types for new things :)
> > 
> > :)
> > 
> > Should I remove Finalized suffix if I want to make the class be a superclass
> of
> > some class? (for dictionary inheritance support)
> 
> "Finalized" is not related to inheritance. "Finalized" controls whether
oilpan's
> GC should call a destructor or not. If you don't need any destructor of this
> dictionary hierarchy, you can omit "Finalized". Otherwise you need
"Finalized".
> 
> Specifically, you need "Finalized" if the dictionary has members that need
> destructors such as RefPtr, String etc. I think you can just add "Finalized"
> always.

I see. Thanks for the explanation!

https://codereview.chromium.org/386963003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/TestDictionary.h:45: bool m_hasBooleanMember;
On 2014/07/22 04:30:10, haraken wrote:
> On 2014/07/22 02:33:58, bashi1 wrote:
> > On 2014/07/21 16:11:59, haraken wrote:
> > > 
> > > Can we use Nullable<T>?
> > 
> > These members are not nullable (no ? suffix in IDL definition).
> > 
> > IIUC, we need to treat nullable member and non-nullable member separately.
> > For non-nullable members,
> > - If a JavaScript object doesn't have the property, it should be treated as
> > undefined, and |hasXXX()| can be used by blink to check whether it is
defined
> or
> > not.
> > - When we create a JavaScript object from dictionary impl class, it should
> check
> > |hasXXX()| before setting the property.
> > 
> > For nullable members, we will use Nullable<T> as you suggested (along with
> > hasXXX() method, for consistency).
> > 
> > The CL doesn't support nullable yet, but I'm going to implement it in next
> > iteration:)
> 
> Nullable<T> is not a class just to represent IDL's nullable value, so you can
> use Nullable<T> to implement something that might not have a valid value (but
> I'm not sure if you can use Nullable<T> in this case.)
> 
> How are you planning to implement nullable values? Like this?
> 
>   // Non-nullable long.
>   int m_longMember;
>   bool m_hasLongMember;
> 
>   // Nullable long.
>   Nullable<int> m_longMember;
>   bool m_hasLongMember;
This is exactly what I was going to implement. For a member of which type is an
interface, there might be no difference between nullable and non-nullable:

    // Nullable or non-nullable Element
    RefPtrWillBeMember<Element> m_elementMember;

> 
> I wonder if we really need both Nullable<> and m_hasXXXMember, since the fact
> that m_hasLongMember is false can imply that the value is null. (Probably I'm
> not understanding nullable correctly.)

Hmm, it depends on whether we want to handle the notion of 'present'.
http://heycam.github.io/webidl/#dfn-present

Specifically, given
 dictionary A {
  long? longValue;
 };

do we want to distinguish following x and y?
 var x = {}; // longValue is not present
 var y = {longValue: null}; // longValue is present, but null

That being said, I noticed that we may not need to distinguish them when we
convert a JavaScript object to a DOM impl object.
http://heycam.github.io/webidl/#es-dictionary

If I read the spec correctly, null should be treated as undefined.

I'm totally fine using Nullable<T>, but I think we should have hasXXXMember()
method because |longMember().isNull()| looks strange if longMember is
non-nullable.

https://codereview.chromium.org/386963003/diff/100001/Source/bindings/tests/r...
File Source/bindings/tests/results/V8TestDictionary.cpp (right):

https://codereview.chromium.org/386963003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/V8TestDictionary.cpp:16: TestDictionary*
V8TestDictionary::toNativeWithTypeCheck(v8::Isolate* isolate,
v8::Handle<v8::Value> v8Value)
On 2014/07/22 04:30:10, haraken wrote:
> On 2014/07/22 02:33:58, bashi1 wrote:
> > On 2014/07/21 16:11:59, haraken wrote:
> > > 
> > > I'd rename this to toNative().
> > > 
> > > toNativeWithTypeCheck() is expected to return 0 if |v8Value| is a value of
> > wrong
> > > type. So the difference between toNative() and toNativeWithTypeCheck()
> should
> > be
> > > whether we check if(v8Value.IsEmpty() || !v8Value->IsObject()) or not.
> > > toNative() should not have the check, and toNativeWithTypeCheck() should
> have
> > > the check.
> > 
> > Thank you for the explanation. I was wondering I should use toNative() or
> > toNativeWithTypeCheck(). Renamed, but I want to confirm that we call this
> > toNative() even if its properties can come from toNativeWithTypeCheck()?
> suppose
> > we have a following definition:
> > 
> >  dictionary A {
> >    Node nodeMember;
> >  };
> > 
> > We will call V8Node::toNativeWithTypeCheck() for nodeMember (in
> DictionaryHelper
> > for now).
> 
> Understood, then we can probably:
> 
> - Add the if(v8Value.IsEmpty() || !v8Value->IsObject()) check to this method.
> - Name this method toNativeWithTypeCheck().
> - Don't define toNative().
> 
> What do you think?

I think this method should be toNative() because it is convenient if method
never returns 0. When a toNativeWithTypeCheck() call fails for a member, we can
either:
- return an empty dictionary object(all members are considered as undefined or
null), or
- ignore the conversion error (the member is considered as undefined or null)

The latter is easy to implement. The former can be more safety? (I'm not sure).

haraken

https://codereview.chromium.org/386963003/diff/100001/Source/bindings/tests/results/TestDictionary.h File Source/bindings/tests/results/TestDictionary.h (right): https://codereview.chromium.org/386963003/diff/100001/Source/bindings/tests/results/TestDictionary.h#newcode45 Source/bindings/tests/results/TestDictionary.h:45: bool m_hasBooleanMember; On 2014/07/22 05:57:35, bashi1 wrote: > On ...

6 years, 5 months ago (2014-07-22 07:23:19 UTC) #7

https://codereview.chromium.org/386963003/diff/100001/Source/bindings/tests/r...
File Source/bindings/tests/results/TestDictionary.h (right):

https://codereview.chromium.org/386963003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/TestDictionary.h:45: bool m_hasBooleanMember;
On 2014/07/22 05:57:35, bashi1 wrote:
> On 2014/07/22 04:30:10, haraken wrote:
> > On 2014/07/22 02:33:58, bashi1 wrote:
> > > On 2014/07/21 16:11:59, haraken wrote:
> > > > 
> > > > Can we use Nullable<T>?
> > > 
> > > These members are not nullable (no ? suffix in IDL definition).
> > > 
> > > IIUC, we need to treat nullable member and non-nullable member separately.
> > > For non-nullable members,
> > > - If a JavaScript object doesn't have the property, it should be treated
as
> > > undefined, and |hasXXX()| can be used by blink to check whether it is
> defined
> > or
> > > not.
> > > - When we create a JavaScript object from dictionary impl class, it should
> > check
> > > |hasXXX()| before setting the property.
> > > 
> > > For nullable members, we will use Nullable<T> as you suggested (along with
> > > hasXXX() method, for consistency).
> > > 
> > > The CL doesn't support nullable yet, but I'm going to implement it in next
> > > iteration:)
> > 
> > Nullable<T> is not a class just to represent IDL's nullable value, so you
can
> > use Nullable<T> to implement something that might not have a valid value
(but
> > I'm not sure if you can use Nullable<T> in this case.)
> > 
> > How are you planning to implement nullable values? Like this?
> > 
> >   // Non-nullable long.
> >   int m_longMember;
> >   bool m_hasLongMember;
> > 
> >   // Nullable long.
> >   Nullable<int> m_longMember;
> >   bool m_hasLongMember;
> This is exactly what I was going to implement. For a member of which type is
an
> interface, there might be no difference between nullable and non-nullable:
> 
>     // Nullable or non-nullable Element
>     RefPtrWillBeMember<Element> m_elementMember;
> 
> > 
> > I wonder if we really need both Nullable<> and m_hasXXXMember, since the
fact
> > that m_hasLongMember is false can imply that the value is null. (Probably
I'm
> > not understanding nullable correctly.)
> 
> Hmm, it depends on whether we want to handle the notion of 'present'.
> http://heycam.github.io/webidl/#dfn-present
> 
> Specifically, given
>  dictionary A {
>   long? longValue;
>  };
> 
> do we want to distinguish following x and y?
>  var x = {}; // longValue is not present
>  var y = {longValue: null}; // longValue is present, but null
> 
> That being said, I noticed that we may not need to distinguish them when we
> convert a JavaScript object to a DOM impl object.
> http://heycam.github.io/webidl/#es-dictionary
> 
> If I read the spec correctly, null should be treated as undefined.
> 
> I'm totally fine using Nullable<T>, but I think we should have hasXXXMember()
> method because |longMember().isNull()| looks strange if longMember is
> non-nullable.

I'm not fully understanding the spec, but if Nullable<T> is enough to support
what we need, I'd prefer using Nullable<T> (because Nullable<T> is introduced to
support these purposes; if it's not enough, we might want to consider improving
the implementation of Nullable<T>). Of course I'm fine with introducing
hasXXXMember() methods.

https://codereview.chromium.org/386963003/diff/100001/Source/bindings/tests/r...
File Source/bindings/tests/results/V8TestDictionary.cpp (right):

https://codereview.chromium.org/386963003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/V8TestDictionary.cpp:16: TestDictionary*
V8TestDictionary::toNativeWithTypeCheck(v8::Isolate* isolate,
v8::Handle<v8::Value> v8Value)
On 2014/07/22 05:57:35, bashi1 wrote:
> On 2014/07/22 04:30:10, haraken wrote:
> > On 2014/07/22 02:33:58, bashi1 wrote:
> > > On 2014/07/21 16:11:59, haraken wrote:
> > > > 
> > > > I'd rename this to toNative().
> > > > 
> > > > toNativeWithTypeCheck() is expected to return 0 if |v8Value| is a value
of
> > > wrong
> > > > type. So the difference between toNative() and toNativeWithTypeCheck()
> > should
> > > be
> > > > whether we check if(v8Value.IsEmpty() || !v8Value->IsObject()) or not.
> > > > toNative() should not have the check, and toNativeWithTypeCheck() should
> > have
> > > > the check.
> > > 
> > > Thank you for the explanation. I was wondering I should use toNative() or
> > > toNativeWithTypeCheck(). Renamed, but I want to confirm that we call this
> > > toNative() even if its properties can come from toNativeWithTypeCheck()?
> > suppose
> > > we have a following definition:
> > > 
> > >  dictionary A {
> > >    Node nodeMember;
> > >  };
> > > 
> > > We will call V8Node::toNativeWithTypeCheck() for nodeMember (in
> > DictionaryHelper
> > > for now).
> > 
> > Understood, then we can probably:
> > 
> > - Add the if(v8Value.IsEmpty() || !v8Value->IsObject()) check to this
method.
> > - Name this method toNativeWithTypeCheck().
> > - Don't define toNative().
> > 
> > What do you think?
> 
> I think this method should be toNative() because it is convenient if method
> never returns 0. When a toNativeWithTypeCheck() call fails for a member, we
can
> either:
> - return an empty dictionary object(all members are considered as undefined or
> null), or
> - ignore the conversion error (the member is considered as undefined or null)
> 
> The latter is easy to implement. The former can be more safety? (I'm not
sure).

Makes sense; I agree that we keep the current implementation and call it
toNative().

haraken

Either way I think this CL is mostly ready except for whether we can use ...

6 years, 5 months ago (2014-07-22 07:27:12 UTC) #8

bashi

FYI: Patch set 9 uses Nullable<T> for primitives. I noticed that modifying an IDL dictionary ...

6 years, 5 months ago (2014-07-23 03:50:06 UTC) #9

Nils Barth (inactive)

On 2014/07/23 03:50:06, bashi1 wrote: > FYI: Patch set 9 uses Nullable<T> for primitives. > ...

6 years, 5 months ago (2014-07-23 13:39:41 UTC) #10

Nils Barth (inactive)

On 2014/07/23 13:39:41, Nils Barth wrote: > On 2014/07/23 03:50:06, bashi1 wrote: > > FYI: ...

6 years, 5 months ago (2014-07-23 14:18:27 UTC) #11

On 2014/07/23 13:39:41, Nils Barth wrote:
> On 2014/07/23 03:50:06, bashi1 wrote:
> > FYI: Patch set 9 uses Nullable<T> for primitives.
> > 
> > I noticed that modifying an IDL dictionary definition doesn't trigger
> necessary
> > recompilation. Specifically, modifying SomeDictionary.idl triggers
> > {V8,}SomeDictionary.{cpp,h} generation, but SomeDictionary.{cpp,h} aren't
> > recompiled. Investigating.
> 
> You need to make a separate GYP rule; copy:
> bindings_core_v8_generated_individual
> to:
> dictionaries_core_v8_generated_individual
> ...and specify different 'outputs'
> (that's why they're not being regenerated)
> 
> You'll need a separate list in the .gypi too:
> core_idl_dictionary_files
> ...which is fed into both the bindings_* rule and the dictionaries_* rule,
> since you're generating bindings and dictionaries.
> 
> (The alternative is to generate stub dictionary implementation for all
bindings
> files, which is ugly.)
> 
> You might be able to avoid having a separate impl_dir by having two build
rules,
> instead saying "generate dict" instead of "generate bindings" as a flag.
> 
> Also, I think you can omit the aggregate dictionary files; we've been
discussing
> removing this for binding anyway, and it's significant added complexity.
> Instead, you can just list the output generated dictionary impl files
directly.
> 
> Does this make sense?

To followup, the issue is that dictionaries have *both* bindings *and* impl
generated.
GYP (and builds generally) need to know their inputs *and* outputs.
So given Foo.idl, if it's a regular IDL file, we just generate V8Foo.{h,cpp},
but if it's a dictionary we generate V8Foo.{h,cpp} *and* Foo.{h,cpp} (impl).

There are 3 ways to handle this:
1. 2 rules, bindings_* and (dict_)impl_*;
* rule A generates V8Foo.{h,cpp}, rule B generates Foo.{h,cpp}
* regular IDL files are only in rule A, dictionaries are in both A & B
2. 2 rules, bindings_* and dict_*;
* rule A generates V8Foo.{h,cpp}, rule B generates V8Foo.{h,cpp} and Foo.{h,cpp}
* regular IDL files are only in rule A, dictionaries are only in B
3. 1 rule, bindings_
* generates V8Foo.{h,cpp} and Foo.{h,cpp} always; if it's not a dictionary,
Foo.{h,cpp} is a stub

My proposal is basically 1 (rule B generates impl only), as this is a bit
simpler structurally
in the compiler (each time you produce exactly 2 outputs; just need a flag to
say --generate-impl).
However, #2 is also ok if you prefer (dict IDLs only processed once).
...and #3 is just *ugly*.

In any case, could you update the docs to specify "the IDL compiler doesn't just
generate bindings;
we also generate impl files for dictionaries":
http://www.chromium.org/developers/design-documents/idl-compiler
http://www.chromium.org/developers/design-documents/idl-build

bashi

On 2014/07/23 14:18:27, Nils Barth wrote: > On 2014/07/23 13:39:41, Nils Barth wrote: > > ...

6 years, 5 months ago (2014-07-24 23:58:52 UTC) #12

On 2014/07/23 14:18:27, Nils Barth wrote:
> On 2014/07/23 13:39:41, Nils Barth wrote:
> > On 2014/07/23 03:50:06, bashi1 wrote:
> > > FYI: Patch set 9 uses Nullable<T> for primitives.
> > > 
> > > I noticed that modifying an IDL dictionary definition doesn't trigger
> > necessary
> > > recompilation. Specifically, modifying SomeDictionary.idl triggers
> > > {V8,}SomeDictionary.{cpp,h} generation, but SomeDictionary.{cpp,h} aren't
> > > recompiled. Investigating.
> > 
> > You need to make a separate GYP rule; copy:
> > bindings_core_v8_generated_individual
> > to:
> > dictionaries_core_v8_generated_individual
> > ...and specify different 'outputs'
> > (that's why they're not being regenerated)
> > 
> > You'll need a separate list in the .gypi too:
> > core_idl_dictionary_files
> > ...which is fed into both the bindings_* rule and the dictionaries_* rule,
> > since you're generating bindings and dictionaries.
> > 
> > (The alternative is to generate stub dictionary implementation for all
> bindings
> > files, which is ugly.)
> > 
> > You might be able to avoid having a separate impl_dir by having two build
> rules,
> > instead saying "generate dict" instead of "generate bindings" as a flag.
> > 
> > Also, I think you can omit the aggregate dictionary files; we've been
> discussing
> > removing this for binding anyway, and it's significant added complexity.
> > Instead, you can just list the output generated dictionary impl files
> directly.
> > 
> > Does this make sense?
> 
> To followup, the issue is that dictionaries have *both* bindings *and* impl
> generated.
> GYP (and builds generally) need to know their inputs *and* outputs.
> So given Foo.idl, if it's a regular IDL file, we just generate V8Foo.{h,cpp},
> but if it's a dictionary we generate V8Foo.{h,cpp} *and* Foo.{h,cpp} (impl).
> 
> There are 3 ways to handle this:
> 1. 2 rules, bindings_* and (dict_)impl_*;
> * rule A generates V8Foo.{h,cpp}, rule B generates Foo.{h,cpp}
> * regular IDL files are only in rule A, dictionaries are in both A & B
> 2. 2 rules, bindings_* and dict_*;
> * rule A generates V8Foo.{h,cpp}, rule B generates V8Foo.{h,cpp} and
Foo.{h,cpp}
> * regular IDL files are only in rule A, dictionaries are only in B
> 3. 1 rule, bindings_
> * generates V8Foo.{h,cpp} and Foo.{h,cpp} always; if it's not a dictionary,
> Foo.{h,cpp} is a stub
> 
> My proposal is basically 1 (rule B generates impl only), as this is a bit
> simpler structurally
> in the compiler (each time you produce exactly 2 outputs; just need a flag to
> say --generate-impl).
> However, #2 is also ok if you prefer (dict IDLs only processed once).
> ...and #3 is just *ugly*.
> 

Thank you so much for detailed proposals! They are really helpful.
I like #1 and will try to do that. One thing I'm getting stuck with the proposal
is how I can specify sub directories for 'outputs' files.
For bindings, We generate V8* files into the same directory (e.g.
gen/blink/bindings/core/v8), so 'outputs' for binding rule can be written like:

 '<(bindings_core_v8_output_dir)/V8<(RULE_INPUT_ROOT).cpp'
 '<(bindings_core_v8_output_dir)/V8<(RULE_INPUT_ROOT).h'

On the other hand, we want to generate dictionary impls into their corresponding
directories. For example:
- modules/webmidi/FooDictionary.idl ->
gen/blink/modules/webmidi/FooDictionary.{cpp,h}
- modules/serviceworkers/BarDictionary.idl ->
gen/blink/modules/serviceworkers/BarDictionary.{cpp,h}

I need to figure out how I can specify sub directories in the rule.
Investigating.

> In any case, could you update the docs to specify "the IDL compiler doesn't
just
> generate bindings;
> we also generate impl files for dictionaries":
> http://www.chromium.org/developers/design-documents/idl-compiler
> http://www.chromium.org/developers/design-documents/idl-build

Yes. I'll update the documents when I land the patches. (I also planning to
write how developers can use generated dictionary impl)

Nils Barth (inactive)

6 years, 5 months ago (2014-07-25 14:07:15 UTC) #13

On 2014/07/24 23:58:52, bashi1 wrote:
> Thank you so much for detailed proposals! They are really helpful.
> I like #1 and will try to do that.

No problem!

> One thing I'm getting stuck with the proposal
> is how I can specify sub directories for 'outputs' files.
> For bindings, We generate V8* files into the same directory (e.g.
> gen/blink/bindings/core/v8), so 'outputs' for binding rule can be written
like:
> 
>  '<(bindings_core_v8_output_dir)/V8<(RULE_INPUT_ROOT).cpp'
>  '<(bindings_core_v8_output_dir)/V8<(RULE_INPUT_ROOT).h'
> 
> On the other hand, we want to generate dictionary impls into their
corresponding
> directories. For example:
> - modules/webmidi/FooDictionary.idl ->
> gen/blink/modules/webmidi/FooDictionary.{cpp,h}
> - modules/serviceworkers/BarDictionary.idl ->
> gen/blink/modules/serviceworkers/BarDictionary.{cpp,h}
> 
> I need to figure out how I can specify sub directories in the rule.
> Investigating.

Easiest (and clearest!) way to do that is to have a GYP file in the directory in
question!
The idea is that the build files for each directory should be responsible for
what's generated in that directory.

E.g.,
modules/webmidi/generated.gyp:
'modules_webmidi_output_dir' = <(blink_output_dir)>/modules/webmidi

'dictionary_impl':
  (dictionary-generation-script)

This can be done with v. little duplication, by using a "template" for the GYP.

The other alternative is to just dump all dictionary impl files into the
component-specific output dir,
like gen/blink/core and gen/blink/modules.
...but that's admittedly a bit messy.

Expand Messages | Collapse Messages