IDL: Add build target for IDL dictionary impl generation in core

IDL: Add build target for IDL dictionary impl generation in core

This CL adds a build target named 'bindings_core_dictionary_impl_generated',
which generaets IDL dictionary impl files for core component.

We have to list generated impl files explicitly in gypi file because
(1) we don't want to use 'aggregated' build (unlike what we are doing for
v8 bindings), and (2) we want to generate impl files in corresponding
directories(e.g. core/css). It seems there is no easy way to make GYP (or GN)
workable satisfying both (1) and (2).

- Added "--generate-dictionary-impl" option flag to idl_compiler.py. If this
  option is specified, the compiler assumes that the passed argument is a file
  which contains paths of IDL dictionary files. The compiler generates DOM
  impl classes for each IDL dictionary files.
- Refactored IdlCompiler and CodeGenerator.
- Added a test for IDL dictionary. In layout tests,
  window.internals.dictionaryTest() returns an InternalDictionary instance.
  This instance has two methods: set() takes InternalDictionary as the
  argument and stores each value of all members. get() returns an
  InternalDictionary instance that contains values which were set via set().

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

[bashi, 2014-07-30 11:32:26]

Haraken-san,

I'm not asking for actual review but I want to ask your opinion about how we
should handle IDL dictionary impl generation.

I'm trying to follow Nils' suggestion
(https://codereview.chromium.org/386963003/#msg13). Having a GYP file in each
directory resolves my original problem(generating impl files in appropriate
directories). However, my GYP skill isn't enough to make things work correctly
in a simple way. In this approach, I can't figure out a way to avoid listing
dictionary IDL files multiple times. I also had to list generated files
explicitly to pass linking.

I'd appreciate if you have ideas to resolve these issues.

Other approaches would be dump all dictionary impl files in the
component-specific output directory (as Nils suggested). The disadvantage is
#include would look strange and I feel I don't want to do that.

WDYT?

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

https://codereview.chromium.org/429853002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/code_generator_v8.py:153: mode=MODE_ALL):
BTW, this is ugly. I'm now thinking that it'd be better to split bindings
generator and dictionary impl generator if we pass a command line flag to
idl_compiler ("--generate-dictionary-impl" in this CL) to indicate what code the
code generator should generate.

https://codereview.chromium.org/429853002/diff/20001/Source/core/core.gyp
File Source/core/core.gyp (right):

https://codereview.chromium.org/429853002/diff/20001/Source/core/core.gyp#new...
Source/core/core.gyp:284: '<(blink_core_output_dir)/css/FontFaceDescriptors.h',
Want to remove this.

https://codereview.chromium.org/429853002/diff/20001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/429853002/diff/20001/Source/core/core.gypi#ne...
Source/core/core.gypi:492: 'css/FontFaceDescriptors.idl',
It seems we need to write dictionary files twice(here and each generated.gypi
files)...

https://codereview.chromium.org/429853002/diff/20001/Source/core/core_generat...
File Source/core/core_generated.gyp (right):

https://codereview.chromium.org/429853002/diff/20001/Source/core/core_generat...
Source/core/core_generated.gyp:138:
'css/generated.gyp:idl_dictionary_core_css_generated',
Maybe not the right place.

Also, we will need to add further
'foo/generated.gyp:idl_dictionary_core_foo_generated' when we add a dictionary
in core/foo.

https://codereview.chromium.org/429853002/diff/20001/Source/core/css/FontFace.h
File Source/core/css/FontFace.h (right):

https://codereview.chromium.org/429853002/diff/20001/Source/core/css/FontFace...
Source/core/css/FontFace.h:1: /*
Just to show an example

https://codereview.chromium.org/429853002/diff/20001/Source/core/css/generate...
File Source/core/css/generated.gyp (right):

https://codereview.chromium.org/429853002/diff/20001/Source/core/css/generate...
Source/core/css/generated.gyp:13: 'FontFaceDescriptors.idl',
Another place where we need to list dictionary IDL files.

[haraken, 2014-07-30 19:57:36]

Added some comments although we'll probably need help from Nils at some point :)

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

https://codereview.chromium.org/429853002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/code_generator_v8.py:153: mode=MODE_ALL):
On 2014/07/30 11:32:26, bashi1 wrote:
> BTW, this is ugly. I'm now thinking that it'd be better to split bindings
> generator and dictionary impl generator if we pass a command line flag to
> idl_compiler ("--generate-dictionary-impl" in this CL) to indicate what code
the
> code generator should generate.

That seems better to me (assuming that it won't duplicate the generate_code
function a lot).

https://codereview.chromium.org/429853002/diff/20001/Source/core/core.gyp
File Source/core/core.gyp (right):

https://codereview.chromium.org/429853002/diff/20001/Source/core/core.gyp#new...
Source/core/core.gyp:284: '<(blink_core_output_dir)/css/FontFaceDescriptors.h',
On 2014/07/30 11:32:26, bashi1 wrote:
> Want to remove this.

I'm not sure if we can remove this. As long as we don't introduce aggregated
bindings (I think we decided not to introduce aggregated bindings for dictionary
impls), we'll need to write a list of generated files somewhere.

https://codereview.chromium.org/429853002/diff/20001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/429853002/diff/20001/Source/core/core.gypi#ne...
Source/core/core.gypi:492: 'css/FontFaceDescriptors.idl',
On 2014/07/30 11:32:26, bashi1 wrote:
> It seems we need to write dictionary files twice(here and each generated.gypi
> files)...

Can we probably define core_idl_dictionary_files in core.gypi, and add:

'includes': [
  'core/core.gypi'
]

to the gyp file that wants to use core_idl_dictionary_files? By including
core.gypi, you can use variables defined in the gypi file.

Nit: core_idl_dictionary_files => core_dictionary_idl_files (for consistency
with other xxx_idl_files)

https://codereview.chromium.org/429853002/diff/20001/Source/core/css/generate...
File Source/core/css/generated.gyp (right):

https://codereview.chromium.org/429853002/diff/20001/Source/core/css/generate...
Source/core/css/generated.gyp:1: # Copyright (c) 2014 The Chromium Authors. All
rights reserved.

We might want to avoid creating generated.gyp to each sub-directories of core/.
Probably can we add this code to core_generated.gyp?

[bashi, 2014-07-31 02:34:05]

https://codereview.chromium.org/429853002/diff/20001/Source/core/core.gyp
File Source/core/core.gyp (right):

https://codereview.chromium.org/429853002/diff/20001/Source/core/core.gyp#new...
Source/core/core.gyp:284: '<(blink_core_output_dir)/css/FontFaceDescriptors.h',
On 2014/07/30 19:57:36, haraken wrote:
> On 2014/07/30 11:32:26, bashi1 wrote:
> > Want to remove this.
> 
> I'm not sure if we can remove this. As long as we don't introduce aggregated
> bindings (I think we decided not to introduce aggregated bindings for
dictionary
> impls), we'll need to write a list of generated files somewhere.

Yes. We don't use aggregate build for dictionaries. Seems that I need to list
the generated files. Thanks!

https://codereview.chromium.org/429853002/diff/20001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/429853002/diff/20001/Source/core/core.gypi#ne...
Source/core/core.gypi:492: 'css/FontFaceDescriptors.idl',
On 2014/07/30 19:57:36, haraken wrote:
> On 2014/07/30 11:32:26, bashi1 wrote:
> > It seems we need to write dictionary files twice(here and each
generated.gypi
> > files)...
> 
> Can we probably define core_idl_dictionary_files in core.gypi, and add:
> 
> 'includes': [
>   'core/core.gypi'
> ]
> 
> to the gyp file that wants to use core_idl_dictionary_files? By including
> core.gypi, you can use variables defined in the gypi file.

The problem was that the relative paths can be different among this file and
other gyp files so I thought I can't share them. Anyway, I'll try different
approach.

> 
> Nit: core_idl_dictionary_files => core_dictionary_idl_files (for consistency
> with other xxx_idl_files)

Will do.

https://codereview.chromium.org/429853002/diff/20001/Source/core/css/generate...
File Source/core/css/generated.gyp (right):

https://codereview.chromium.org/429853002/diff/20001/Source/core/css/generate...
Source/core/css/generated.gyp:1: # Copyright (c) 2014 The Chromium Authors. All
rights reserved.
On 2014/07/30 19:57:36, haraken wrote:
> 
> We might want to avoid creating generated.gyp to each sub-directories of
core/.
> Probably can we add this code to core_generated.gyp?

Yeah, adding generated.gyp look a bit tricky and things got complicated more
than I expected. I'll try another approach.

[bashi, 2014-08-01 02:12:30]

Uploaded Patch Set #1. Diffs are:
- Removed generated.gyp from core/css
- Added bindings_core_dictionary_impl_generated target into
bindings/core/v8/generated.gyp
  This target takes dictionary IDL files as inputs and generated impl files as
outputs.
  These inputs and outputs are listed explicitly in core/core.gypi

Does this make sense? If so, I'm going to prepare for actual review.

[bashi, 2014-08-01 02:13:32]

On 2014/08/01 02:12:30, bashi1 wrote:
> Uploaded Patch Set #1. Diffs are:
> - Removed generated.gyp from core/css
> - Added bindings_core_dictionary_impl_generated target into
> bindings/core/v8/generated.gyp
>   This target takes dictionary IDL files as inputs and generated impl files as
> outputs.
>   These inputs and outputs are listed explicitly in core/core.gypi
> 
> Does this make sense? If so, I'm going to prepare for actual review.

s/#1/#2/

[haraken, 2014-08-01 02:37:45]

Overall, looks good to me!

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

https://codereview.chromium.org/429853002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/code_generator_v8.py:86: MODE_ALL = MODE_BINDINGS |
MODE_DICTIONARY_IMPL

Shall we simply use a set of strings like set("bindings", "dictionary_impl") ?

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

https://codereview.chromium.org/429853002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/idl_compiler.py:99: self.generating_mode =
MODE_DICTIONARY_IMPL

I think we can just use:

   self.generating_mode = "dictionary_impl"

[bashi, 2014-08-01 02:49:47]

Thanks! I'll upload new patch set for actual review.

BTW, these changes won't get running unless we actually add an IDL dictionary in
core/modules. Is it fine to include FontFaceDescritors in this CL? Or we should
add a testing IDL dictionary into core/testing (like PrivateScriptTest)?

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

https://codereview.chromium.org/429853002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/code_generator_v8.py:86: MODE_ALL = MODE_BINDINGS |
MODE_DICTIONARY_IMPL
On 2014/08/01 02:37:44, haraken wrote:
> 
> Shall we simply use a set of strings like set("bindings", "dictionary_impl") ?

I'm going to remove this flags.

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

https://codereview.chromium.org/429853002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/idl_compiler.py:99: self.generating_mode =
MODE_DICTIONARY_IMPL
On 2014/08/01 02:37:44, haraken wrote:
> 
> I think we can just use:
> 
>    self.generating_mode = "dictionary_impl"

Will remove this, too.

[haraken, 2014-08-01 03:55:54]

> BTW, these changes won't get running unless we actually add an IDL dictionary
in
> core/modules. Is it fine to include FontFaceDescritors in this CL? Or we
should
> add a testing IDL dictionary into core/testing (like PrivateScriptTest)?

It would be great to have a unit test that has good coverage about the
dictionary functionality :)

[bashi, 2014-08-05 05:36:23]

Added a layout test and updated the description. Build files becomes a bit
messy, given that we need to handle testing code separately. I attempt to make
Patch set #3 workable, but got stuck.

thakis@, nbarth@: do you have any idea about below?

I got an error on win bots. It seems that newly added target
“bindings_core_dictionary_impl_generated” runs wrong command “midl-wrapper”. I
don’t know what it is and why ninja ran the command(it should run blink’s idl
compiler). “bindings_core_dictionary_impl_generated” is added in
Source/bindings/core/v8/generated.gyp

Here is an error log.

FAILED: E:\b\depot_tools\python276_bin\python.exe gyp-win-tool midl-wrapper
environment.x86
obj\third_party\WebKit\Source\bindings\core\v8\bindings_core_dictionary_impl_generated.gen
TestingDictionary.tlb TestingDictionary.h TestingDictionary.dlldata.c
TestingDictionary_i.c TestingDictionary_p.c
..\..\third_party\WebKit\Source\core\testing\TestingDictionary.idl /char signed
/env win32 /Oicf
..\..\third_party\WebKit\Source\core\testing\TestingDictionary.idl(7) : error
MIDL2025 : syntax error : expecting ] or , near "GarbageCollected"

..\..\third_party\WebKit\Source\core\testing\TestingDictionary.idl(7) : error
MIDL2025 : syntax error : expecting an interface name or DispatchInterfaceName
or CoclassName or ModuleName or LibraryName or ContractName or a type
specification near "dictionary"

..\..\third_party\WebKit\Source\core\testing\TestingDictionary.idl(9) : error
MIDL2025 : syntax error : expecting an interface name or DispatchInterfaceName
or CoclassName or ModuleName or LibraryName or ContractName or a type
specification near "DOMString"

..\..\third_party\WebKit\Source\core\testing\TestingDictionary.idl(9) : error
MIDL2026 : cannot recover from earlier syntax errors; aborting compilation 

ninja: build stopped: subcommand failed.

https://codereview.chromium.org/429853002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/generated.gyp (right):

https://codereview.chromium.org/429853002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/generated.gyp:156: '-S',  # skip 'import site' to speed
up startup
Removed -S because this action runs once.

[Nico, 2014-08-05 15:37:30]

On 2014/08/05 05:36:23, bashi1 wrote:
> Added a layout test and updated the description. Build files becomes a bit
> messy, given that we need to handle testing code separately. I attempt to make
> Patch set #3 workable, but got stuck.
> 
> thakis@, nbarth@: do you have any idea about below?
> 
> I got an error on win bots. It seems that newly added target
> “bindings_core_dictionary_impl_generated” runs wrong command “midl-wrapper”. I
> don’t know what it is and why ninja ran the command(it should run blink’s idl
> compiler). “bindings_core_dictionary_impl_generated” is added in
> Source/bindings/core/v8/generated.gyp
> 
> Here is an error log.
> 
> FAILED: E:\b\depot_tools\python276_bin\python.exe gyp-win-tool midl-wrapper
> environment.x86
>
obj\third_party\WebKit\Source\bindings\core\v8\bindings_core_dictionary_impl_generated.gen
> TestingDictionary.tlb TestingDictionary.h TestingDictionary.dlldata.c
> TestingDictionary_i.c TestingDictionary_p.c
> ..\..\third_party\WebKit\Source\core\testing\TestingDictionary.idl /char
signed
> /env win32 /Oicf
> ..\..\third_party\WebKit\Source\core\testing\TestingDictionary.idl(7) : error
> MIDL2025 : syntax error : expecting ] or , near "GarbageCollected"
> 
> ..\..\third_party\WebKit\Source\core\testing\TestingDictionary.idl(7) : error
> MIDL2025 : syntax error : expecting an interface name or DispatchInterfaceName
> or CoclassName or ModuleName or LibraryName or ContractName or a type
> specification near "dictionary"
> 
> ..\..\third_party\WebKit\Source\core\testing\TestingDictionary.idl(9) : error
> MIDL2025 : syntax error : expecting an interface name or DispatchInterfaceName
> or CoclassName or ModuleName or LibraryName or ContractName or a type
> specification near "DOMString"
> 
> ..\..\third_party\WebKit\Source\core\testing\TestingDictionary.idl(9) : error
> MIDL2026 : cannot recover from earlier syntax errors; aborting compilation 

This looks like Visual Studio's IDL compiler is trying to compile these files.
gyp's ninja generator calls that automatically for .idl files in sources
sections (msvs does too):
https://code.google.com/p/chromium/codesearch#chromium/src/tools/gyp/pylib/gy...

I'm guessing these files weren't in sources sections previously?

Maybe they should have a different extension (.webidl)?

[haraken, 2014-08-05 15:41:32]

On 2014/08/05 15:37:30, Nico (away) wrote:
> On 2014/08/05 05:36:23, bashi1 wrote:
> > Added a layout test and updated the description. Build files becomes a bit
> > messy, given that we need to handle testing code separately. I attempt to
make
> > Patch set #3 workable, but got stuck.
> > 
> > thakis@, nbarth@: do you have any idea about below?
> > 
> > I got an error on win bots. It seems that newly added target
> > “bindings_core_dictionary_impl_generated” runs wrong command “midl-wrapper”.
I
> > don’t know what it is and why ninja ran the command(it should run blink’s
idl
> > compiler). “bindings_core_dictionary_impl_generated” is added in
> > Source/bindings/core/v8/generated.gyp
> > 
> > Here is an error log.
> > 
> > FAILED: E:\b\depot_tools\python276_bin\python.exe gyp-win-tool midl-wrapper
> > environment.x86
> >
>
obj\third_party\WebKit\Source\bindings\core\v8\bindings_core_dictionary_impl_generated.gen
> > TestingDictionary.tlb TestingDictionary.h TestingDictionary.dlldata.c
> > TestingDictionary_i.c TestingDictionary_p.c
> > ..\..\third_party\WebKit\Source\core\testing\TestingDictionary.idl /char
> signed
> > /env win32 /Oicf
> > ..\..\third_party\WebKit\Source\core\testing\TestingDictionary.idl(7) :
error
> > MIDL2025 : syntax error : expecting ] or , near "GarbageCollected"
> > 
> > ..\..\third_party\WebKit\Source\core\testing\TestingDictionary.idl(7) :
error
> > MIDL2025 : syntax error : expecting an interface name or
DispatchInterfaceName
> > or CoclassName or ModuleName or LibraryName or ContractName or a type
> > specification near "dictionary"
> > 
> > ..\..\third_party\WebKit\Source\core\testing\TestingDictionary.idl(9) :
error
> > MIDL2025 : syntax error : expecting an interface name or
DispatchInterfaceName
> > or CoclassName or ModuleName or LibraryName or ContractName or a type
> > specification near "DOMString"
> > 
> > ..\..\third_party\WebKit\Source\core\testing\TestingDictionary.idl(9) :
error
> > MIDL2026 : cannot recover from earlier syntax errors; aborting compilation 
> 
> This looks like Visual Studio's IDL compiler is trying to compile these files.
> gyp's ninja generator calls that automatically for .idl files in sources
> sections (msvs does too):
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/gyp/pylib/gy...
> 
> I'm guessing these files weren't in sources sections previously?
> 
> Maybe they should have a different extension (.webidl)?

Thanks Nico!

Do we have any case where we need to use Visual Studio's IDL compiler? (Maybe
are IDL files in extensions or something using the IDL compiler?)

[Nico, 2014-08-05 15:44:52]

On Tue, Aug 5, 2014 at 8:41 AM, <haraken@chromium.org> wrote:

> On 2014/08/05 15:37:30, Nico (away) wrote:
>
>> On 2014/08/05 05:36:23, bashi1 wrote:
>> > Added a layout test and updated the description. Build files becomes a
>> bit
>> > messy, given that we need to handle testing code separately. I attempt
>> to
>>
> make
>
>> > Patch set #3 workable, but got stuck.
>> >
>> > thakis@, nbarth@: do you have any idea about below?
>> >
>> > I got an error on win bots. It seems that newly added target
>> > "bindings_core_dictionary_impl_generated" runs wrong command
>> "midl-wrapper".
>>
> I
>
>> > don't know what it is and why ninja ran the command(it should run
>> blink's
>>
> idl
>
>> > compiler). "bindings_core_dictionary_impl_generated" is added in
>> > Source/bindings/core/v8/generated.gyp
>> >
>> > Here is an error log.
>> >
>> > FAILED: E:\b\depot_tools\python276_bin\python.exe gyp-win-tool
>> midl-wrapper
>> > environment.x86
>> >
>>
>
> obj\third_party\WebKit\Source\bindings\core\v8\bindings_
> core_dictionary_impl_generated.gen
>
>> > TestingDictionary.tlb TestingDictionary.h TestingDictionary.dlldata.c
>> > TestingDictionary_i.c TestingDictionary_p.c
>> > ..\..\third_party\WebKit\Source\core\testing\TestingDictionary.idl
>> /char
>> signed
>> > /env win32 /Oicf
>> > ..\..\third_party\WebKit\Source\core\testing\TestingDictionary.idl(7) :
>>
> error
>
>> > MIDL2025 : syntax error : expecting ] or , near "GarbageCollected"
>> >
>> > ..\..\third_party\WebKit\Source\core\testing\TestingDictionary.idl(7) :
>>
> error
>
>> > MIDL2025 : syntax error : expecting an interface name or
>>
> DispatchInterfaceName
>
>> > or CoclassName or ModuleName or LibraryName or ContractName or a type
>> > specification near "dictionary"
>> >
>> > ..\..\third_party\WebKit\Source\core\testing\TestingDictionary.idl(9) :
>>
> error
>
>> > MIDL2025 : syntax error : expecting an interface name or
>>
> DispatchInterfaceName
>
>> > or CoclassName or ModuleName or LibraryName or ContractName or a type
>> > specification near "DOMString"
>> >
>> > ..\..\third_party\WebKit\Source\core\testing\TestingDictionary.idl(9) :
>>
> error
>
>> > MIDL2026 : cannot recover from earlier syntax errors; aborting
>> compilation
>>
>
>  This looks like Visual Studio's IDL compiler is trying to compile these
>> files.
>> gyp's ninja generator calls that automatically for .idl files in sources
>> sections (msvs does too):
>>
>
> https://code.google.com/p/chromium/codesearch#chromium/
> src/tools/gyp/pylib/gyp/generator/ninja.py&l=530
>
>  I'm guessing these files weren't in sources sections previously?
>>
>
>  Maybe they should have a different extension (.webidl)?
>>
>
> Thanks Nico!
>
> Do we have any case where we need to use Visual Studio's IDL compiler?
> (Maybe
> are IDL files in extensions or something using the IDL compiler?)
>

Looks like there are 5 places where we do need this:
https://code.google.com/p/chromium/codesearch#search/&q=%5C.idl%20file:%5C.gy...

(The 6th hit is a test for the extension midl compiler, and unrelated. The
extensions code also uses non-msvs .idl files, they probably don't put them
in sources sections either. To see examples of the extensions stuff, remove
"-file:extensions" from the query.)

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[haraken, 2014-08-05 15:51:37]

I see... Let me ask one more question.

https://codereview.chromium.org/429853002/diff/160001/Source/bindings/core/v8...
File Source/bindings/core/v8/generated.gyp (right):

https://codereview.chromium.org/429853002/diff/160001/Source/bindings/core/v8...
Source/bindings/core/v8/generated.gyp:46: '<@(core_interface_idl_files)',

These IDL files are already written in the 'sources' section, but they are
complied with our IDL compiler.

The only difference I can see between 'bindings_core_v8_generated_individual'
and 'bindings_core_dictionary_impl_generated' is that the former is using
'rules' whereas the latter is using 'actions'. Will the difference affect how
the 'sources' section is treated?

[Nico, 2014-08-05 17:49:03]

https://codereview.chromium.org/429853002/diff/160001/Source/bindings/core/v8...
File Source/bindings/core/v8/generated.gyp (right):

https://codereview.chromium.org/429853002/diff/160001/Source/bindings/core/v8...
Source/bindings/core/v8/generated.gyp:46: '<@(core_interface_idl_files)',
On 2014/08/05 15:51:37, haraken wrote:
> 
> These IDL files are already written in the 'sources' section, but they are
> complied with our IDL compiler.
> 
> The only difference I can see between 'bindings_core_v8_generated_individual'
> and 'bindings_core_dictionary_impl_generated' is that the former is using
> 'rules' whereas the latter is using 'actions'. Will the difference affect how
> the 'sources' section is treated?

The code I linked to above apparently calls
https://code.google.com/p/chromium/codesearch#chromium/src/tools/gyp/pylib/gy...
and early-outs if that returns true. So it looks gyp's build-in idl rules on
Windows aren't added if there's a .idl rule on the target, but it does not check
for a .idl action.

[bashi, 2014-08-05 23:54:28]

On 2014/08/05 17:49:03, Nico (away) wrote:
>
https://codereview.chromium.org/429853002/diff/160001/Source/bindings/core/v8...
> File Source/bindings/core/v8/generated.gyp (right):
> 
>
https://codereview.chromium.org/429853002/diff/160001/Source/bindings/core/v8...
> Source/bindings/core/v8/generated.gyp:46: '<@(core_interface_idl_files)',
> On 2014/08/05 15:51:37, haraken wrote:
> > 
> > These IDL files are already written in the 'sources' section, but they are
> > complied with our IDL compiler.
> > 
> > The only difference I can see between
'bindings_core_v8_generated_individual'
> > and 'bindings_core_dictionary_impl_generated' is that the former is using
> > 'rules' whereas the latter is using 'actions'. Will the difference affect
how
> > the 'sources' section is treated?
> 
> The code I linked to above apparently calls
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/gyp/pylib/gy...
> and early-outs if that returns true. So it looks gyp's build-in idl rules on
> Windows aren't added if there's a .idl rule on the target, but it does not
check
> for a .idl action.

Thank you so much for the help, Nico!

I think we can take following approaches to solve the problem:
1. Modify GYP's HasExplicitIdlRules so that it can also check actions. (need to
investigate whether we can do it)
2. Use 'rules' for 'bindings_core_dictionary_impl_generated' target. Generates
all outputs in the same directory. We can add a new script which moves output
files into corresponding directory. For example, suppose we have
core/testing/A.idl. IDL compiler generates impl classes into
gen/blink/bindings/core/A.{cpp,h}, then a script moves them to
gen/blink/core/testing/A.{cpp,h}

Haraken-san, WDYT?

[haraken, 2014-08-06 00:48:24]

On 2014/08/05 23:54:28, bashi1 wrote:
> On 2014/08/05 17:49:03, Nico (away) wrote:
> >
>
https://codereview.chromium.org/429853002/diff/160001/Source/bindings/core/v8...
> > File Source/bindings/core/v8/generated.gyp (right):
> > 
> >
>
https://codereview.chromium.org/429853002/diff/160001/Source/bindings/core/v8...
> > Source/bindings/core/v8/generated.gyp:46: '<@(core_interface_idl_files)',
> > On 2014/08/05 15:51:37, haraken wrote:
> > > 
> > > These IDL files are already written in the 'sources' section, but they are
> > > complied with our IDL compiler.
> > > 
> > > The only difference I can see between
> 'bindings_core_v8_generated_individual'
> > > and 'bindings_core_dictionary_impl_generated' is that the former is using
> > > 'rules' whereas the latter is using 'actions'. Will the difference affect
> how
> > > the 'sources' section is treated?
> > 
> > The code I linked to above apparently calls
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/gyp/pylib/gy...
> > and early-outs if that returns true. So it looks gyp's build-in idl rules on
> > Windows aren't added if there's a .idl rule on the target, but it does not
> check
> > for a .idl action.
> 
> Thank you so much for the help, Nico!
> 
> I think we can take following approaches to solve the problem:
> 1. Modify GYP's HasExplicitIdlRules so that it can also check actions. (need
to
> investigate whether we can do it)
> 2. Use 'rules' for 'bindings_core_dictionary_impl_generated' target. Generates
> all outputs in the same directory. We can add a new script which moves output
> files into corresponding directory. For example, suppose we have
> core/testing/A.idl. IDL compiler generates impl classes into
> gen/blink/bindings/core/A.{cpp,h}, then a script moves them to
> gen/blink/core/testing/A.{cpp,h}
> 
> Haraken-san, WDYT?

If we can do 1, we don't need 2, right?

2 looks hacky but I'm OK if there is no other option we can take. The situation
might change when the build system is switched to GN.

[bashi, 2014-08-12 13:21:29]

On 2014/08/06 00:48:24, haraken wrote:
> On 2014/08/05 23:54:28, bashi1 wrote:
> > On 2014/08/05 17:49:03, Nico (away) wrote:
> > >
> >
>
https://codereview.chromium.org/429853002/diff/160001/Source/bindings/core/v8...
> > > File Source/bindings/core/v8/generated.gyp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/429853002/diff/160001/Source/bindings/core/v8...
> > > Source/bindings/core/v8/generated.gyp:46: '<@(core_interface_idl_files)',
> > > On 2014/08/05 15:51:37, haraken wrote:
> > > > 
> > > > These IDL files are already written in the 'sources' section, but they
are
> > > > complied with our IDL compiler.
> > > > 
> > > > The only difference I can see between
> > 'bindings_core_v8_generated_individual'
> > > > and 'bindings_core_dictionary_impl_generated' is that the former is
using
> > > > 'rules' whereas the latter is using 'actions'. Will the difference
affect
> > how
> > > > the 'sources' section is treated?
> > > 
> > > The code I linked to above apparently calls
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/gyp/pylib/gy...
> > > and early-outs if that returns true. So it looks gyp's build-in idl rules
on
> > > Windows aren't added if there's a .idl rule on the target, but it does not
> > check
> > > for a .idl action.
> > 
> > Thank you so much for the help, Nico!
> > 
> > I think we can take following approaches to solve the problem:
> > 1. Modify GYP's HasExplicitIdlRules so that it can also check actions. (need
> to
> > investigate whether we can do it)
> > 2. Use 'rules' for 'bindings_core_dictionary_impl_generated' target.
Generates
> > all outputs in the same directory. We can add a new script which moves
output
> > files into corresponding directory. For example, suppose we have
> > core/testing/A.idl. IDL compiler generates impl classes into
> > gen/blink/bindings/core/A.{cpp,h}, then a script moves them to
> > gen/blink/core/testing/A.{cpp,h}
> > 
> > Haraken-san, WDYT?
> 
> If we can do 1, we don't need 2, right?
Yes, and I did it and prepared GYP roll.
https://codereview.chromium.org/467483002/

I think PS#4 is ready for review. Could you take a look?

> 
> 2 looks hacky but I'm OK if there is no other option we can take. The
situation
> might change when the build system is switched to GN.

[bashi, 2014-08-12 13:23:06]

-nbarth@
+brettw@

brettw@, could you review GN stuff?

[Jens Widell, 2014-08-12 15:18:01]

https://codereview.chromium.org/429853002/diff/180001/Source/bindings/core/v8...
File Source/bindings/core/v8/generated.gyp (right):

https://codereview.chromium.org/429853002/diff/180001/Source/bindings/core/v8...
Source/bindings/core/v8/generated.gyp:149:
'<(bindings_scripts_output_dir)/lextab.py',
It would be somewhat nice to store lextab.py, parsetab.pickle and
cached_jinja_templates.stamp in a variable ("idl_cache_files" or something) to
avoid duplicating the information. This is the third instance in the tree, I
think.

https://codereview.chromium.org/429853002/diff/180001/Source/bindings/scripts...
File Source/bindings/scripts/compute_interfaces_info_individual.py (right):

https://codereview.chromium.org/429853002/diff/180001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:93: 
Two blank lines between functions.

[brettw, 2014-08-12 16:27:54]

I find the FIXMEs in this patch confusing. It isn't clear whether this is
something that you're planning on doing next week in a followup patch, or if
this is something that would be nice to get to eventually. Can you file a bug
talking about what should happen and why, and then reference that from the
different places you do FIXMEs?

[bashi, 2014-08-13 00:51:20]

Thank you for review, Jens!

> I find the FIXMEs in this patch confusing. It isn't clear whether this is
> something that you're planning on doing next week in a followup patch, or if
> this is something that would be nice to get to eventually. Can you file a bug
> talking about what should happen and why, and then reference that from the
> different places you do FIXMEs?

Sorry about the confusion. Added a comment in core/core.gypi. These FIXMEs
should be gone in a few weeks.

https://codereview.chromium.org/429853002/diff/180001/Source/bindings/core/v8...
File Source/bindings/core/v8/generated.gyp (right):

https://codereview.chromium.org/429853002/diff/180001/Source/bindings/core/v8...
Source/bindings/core/v8/generated.gyp:149:
'<(bindings_scripts_output_dir)/lextab.py',
On 2014/08/12 15:18:00, Jens Lindström wrote:
> It would be somewhat nice to store lextab.py, parsetab.pickle and
> cached_jinja_templates.stamp in a variable ("idl_cache_files" or something) to
> avoid duplicating the information. This is the third instance in the tree, I
> think.

Done.

https://codereview.chromium.org/429853002/diff/180001/Source/bindings/scripts...
File Source/bindings/scripts/compute_interfaces_info_individual.py (right):

https://codereview.chromium.org/429853002/diff/180001/Source/bindings/scripts...
Source/bindings/scripts/compute_interfaces_info_individual.py:93: 
On 2014/08/12 15:18:00, Jens Lindström wrote:
> Two blank lines between functions.

Done.

[brettw, 2014-08-14 22:26:29]

https://codereview.chromium.org/429853002/diff/220001/Source/core/core.gni
File Source/core/core.gni (right):

https://codereview.chromium.org/429853002/diff/220001/Source/core/core.gni#ne...
Source/core/core.gni:21: core_testing_dictionary_idl_files =
get_path_info(_gypi.core_testing_dictionary_idl_files, "abspath")
Can you wrap to 80 cols (put get_path_info on the next line).

https://codereview.chromium.org/429853002/diff/220001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/429853002/diff/220001/Source/core/core.gypi#n...
Source/core/core.gypi:3368: '<@(generated_core_testing_dictionary_files)',
Unless I'm misunderstanding something, I'm surprised this runs at all.  In
core.gni, this file gets slurped in and you assign this to
webcore_testing_files. But this will include this literal. Then when we call
get_path_info it will be converted to
"//third_party/WebKit/Source/core/<@(generated_core_testing_dictionary_files)"
which will then get passed around.

You should be able to verify this by just print(webcore_testing_files) in the
.gni file and running gn: "gn gen out/eraseme"

If you need this in GYP, you have to remove the '<@...' string from the list,
and you have to do it before calling get_path_info (well, you could do it after,
but then you would need to include the whole "//third_party/..." thing above
which is pretty weird.

You can see some code in Source/bindings/bindings.gni that does this. It
assignes to a temporary (beginning with _ to mark it as file-local) and then
subtracts the string from there.

[bashi, 2014-08-15 00:14:54]

https://codereview.chromium.org/429853002/diff/220001/Source/core/core.gni
File Source/core/core.gni (right):

https://codereview.chromium.org/429853002/diff/220001/Source/core/core.gni#ne...
Source/core/core.gni:21: core_testing_dictionary_idl_files =
get_path_info(_gypi.core_testing_dictionary_idl_files, "abspath")
On 2014/08/14 22:26:29, brettw wrote:
> Can you wrap to 80 cols (put get_path_info on the next line).

Done.

https://codereview.chromium.org/429853002/diff/220001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/429853002/diff/220001/Source/core/core.gypi#n...
Source/core/core.gypi:3368: '<@(generated_core_testing_dictionary_files)',
On 2014/08/14 22:26:29, brettw wrote:
> Unless I'm misunderstanding something, I'm surprised this runs at all.  In
> core.gni, this file gets slurped in and you assign this to
> webcore_testing_files. But this will include this literal. Then when we call
> get_path_info it will be converted to
> "//third_party/WebKit/Source/core/<@(generated_core_testing_dictionary_files)"
> which will then get passed around.
> 
> You should be able to verify this by just print(webcore_testing_files) in the
> .gni file and running gn: "gn gen out/eraseme"
> 
> If you need this in GYP, you have to remove the '<@...' string from the list,
> and you have to do it before calling get_path_info (well, you could do it
after,
> but then you would need to include the whole "//third_party/..." thing above
> which is pretty weird.
> 
> You can see some code in Source/bindings/bindings.gni that does this. It
> assignes to a temporary (beginning with _ to mark it as file-local) and then
> subtracts the string from there.

Thank you for the detailed explanation! I removed
'<@(generated_core_testing_dictionary_files)' from here and added it in
'sources' of 'webcore_testing'. (Did it for GN as well).

[brettw, 2014-08-15 00:16:19]

lgtm

[bashi, 2014-08-26 00:24:35]

Haraken-san, friendly remainder. Could you take a look when you have time?

[haraken, 2014-08-26 00:54:53]

On 2014/08/26 00:24:35, bashi1 wrote:
> Haraken-san, friendly remainder. Could you take a look when you have time?

Sorry, I missed this one. I'll certainly take a look today.

[haraken, 2014-08-26 04:00:41]

https://codereview.chromium.org/429853002/diff/260001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/429853002/diff/260001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:758: crbug.com/321462 [ Release ]
fast/dom/idl-dictionary-unittest.html [ Skip ]

Why do you need to skip idl-dictionary-unittest.html in release builds?

(private_script_unittest.html needs to be skipped for some complicated reason
specific to Blink-in-JS, and I don't think it applies for
idl-dictionary-unittest.html. I can explain if necessary.)

https://codereview.chromium.org/429853002/diff/260001/LayoutTests/fast/dom/id...
File LayoutTests/fast/dom/idl-dictionary-unittest.html (right):

https://codereview.chromium.org/429853002/diff/260001/LayoutTests/fast/dom/id...
LayoutTests/fast/dom/idl-dictionary-unittest.html:12:
shouldBe('dict.stringMember', '"defaultStringValue"');

shouldBeEqualToString

https://codereview.chromium.org/429853002/diff/260001/LayoutTests/fast/dom/id...
LayoutTests/fast/dom/idl-dictionary-unittest.html:17: invalidMember:
'invalid'});

I want to add more test cases about null, undefined, missing members etc.

https://codereview.chromium.org/429853002/diff/260001/LayoutTests/fast/dom/id...
LayoutTests/fast/dom/idl-dictionary-unittest.html:20:
shouldBe('dict.stringMember', '"modifiedString"');

shouldBeEqualToString

https://codereview.chromium.org/429853002/diff/260001/Source/bindings/scripts...
File Source/bindings/scripts/code_generator_v8.py (right):

https://codereview.chromium.org/429853002/diff/260001/Source/bindings/scripts...
Source/bindings/scripts/code_generator_v8.py:218:
use_relative_output_path=True):

Would you elaborate on why use_relative_output_path is needed?

I think we discussed this before, but I forgot what conclusion we've reached.

https://codereview.chromium.org/429853002/diff/260001/Source/bindings/scripts...
File Source/bindings/scripts/idl_compiler.py (right):

https://codereview.chromium.org/429853002/diff/260001/Source/bindings/scripts...
Source/bindings/scripts/idl_compiler.py:166: # |input_filename| should be a file
which contains list of IDL

list => a list

https://codereview.chromium.org/429853002/diff/260001/Source/bindings/scripts...
Source/bindings/scripts/idl_compiler.py:170: # |input_filename| should be path
of an IDL file.

path => a path

https://codereview.chromium.org/429853002/diff/260001/Source/bindings/tests/r...
File Source/bindings/tests/results/V8TestObject.cpp (right):

https://codereview.chromium.org/429853002/diff/260001/Source/bindings/tests/r...
Source/bindings/tests/results/V8TestObject.cpp:8196: testDictionaryArg =
TestDictionary::create();

Can you add a test case for this branch to a layout test?

https://codereview.chromium.org/429853002/diff/260001/Source/core/testing/Dic...
File Source/core/testing/DictionaryTest.cpp (right):

https://codereview.chromium.org/429853002/diff/260001/Source/core/testing/Dic...
Source/core/testing/DictionaryTest.cpp:25: m_stringMember =
testingDictionary->stringMember();

Just help me understand: Are you assuming that Dictionary users selectively use
the hasXXX() checks depending on whether the member is nullable or not? Or are
you assuming that Dictionary users (can) always use the hasXXX() checks?

Specifically, I wonder if we should or should not or can add the
testingDictionary->hasStringMember() check here.

https://codereview.chromium.org/429853002/diff/260001/Source/core/testing/Dic...
Source/core/testing/DictionaryTest.cpp:40:
result->setStringSequenceMember(m_stringSequenceMember);

Ditto. It's not clear to me when the Dictionary users need the if checks. I'd
like to understand the rule.

https://codereview.chromium.org/429853002/diff/260001/Source/core/testing/Dic...
Source/core/testing/DictionaryTest.cpp:44: void DictionaryTest::trace(Visitor*
visitor)

Drop |visitor|.

https://codereview.chromium.org/429853002/diff/260001/Source/core/testing/Dic...
File Source/core/testing/DictionaryTest.h (right):

https://codereview.chromium.org/429853002/diff/260001/Source/core/testing/Dic...
Source/core/testing/DictionaryTest.h:35: Nullable<bool> m_booleanOrNullMember;

I'm a bit getting confused with what DictionaryTest is testing.

Are you assuming that Dictionary users (e.g., FontFaceDescriptors) use
Nullable<T>? I was expecting that Nullable is just a concept of bindings.

https://codereview.chromium.org/429853002/diff/260001/Source/core/testing/Int...
File Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/429853002/diff/260001/Source/core/testing/Int...
Source/core/testing/Internals.idl:208: DictionaryTest dictionaryTest();

I don't quite understand why this needs to be Debug only.

[bashi, 2014-08-27 03:32:51]

Patchset #8 (id:280001) has been deleted

[bashi, 2014-08-27 05:06:32]

Thank you for review!

I added tests and noticed there are bugs when a member is explicitly specified
as undefined. It's not trivial to fix so I'd like to address the issue in
separate CL.

https://codereview.chromium.org/429853002/diff/260001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/429853002/diff/260001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:758: crbug.com/321462 [ Release ]
fast/dom/idl-dictionary-unittest.html [ Skip ]
On 2014/08/26 04:00:40, haraken wrote:
> 
> Why do you need to skip idl-dictionary-unittest.html in release builds?
> 
> (private_script_unittest.html needs to be skipped for some complicated reason
> specific to Blink-in-JS, and I don't think it applies for
> idl-dictionary-unittest.html. I can explain if necessary.)

Removed.

https://codereview.chromium.org/429853002/diff/260001/LayoutTests/fast/dom/id...
File LayoutTests/fast/dom/idl-dictionary-unittest.html (right):

https://codereview.chromium.org/429853002/diff/260001/LayoutTests/fast/dom/id...
LayoutTests/fast/dom/idl-dictionary-unittest.html:12:
shouldBe('dict.stringMember', '"defaultStringValue"');
On 2014/08/26 04:00:40, haraken wrote:
> 
> shouldBeEqualToString

Done.

https://codereview.chromium.org/429853002/diff/260001/LayoutTests/fast/dom/id...
LayoutTests/fast/dom/idl-dictionary-unittest.html:17: invalidMember:
'invalid'});
On 2014/08/26 04:00:40, haraken wrote:
> 
> I want to add more test cases about null, undefined, missing members etc.

Partially done. Not all tests passed and it's not trivial to fix so I'd like to
make another CL to fix them.

https://codereview.chromium.org/429853002/diff/260001/LayoutTests/fast/dom/id...
LayoutTests/fast/dom/idl-dictionary-unittest.html:20:
shouldBe('dict.stringMember', '"modifiedString"');
On 2014/08/26 04:00:40, haraken wrote:
> 
> shouldBeEqualToString

Done.

https://codereview.chromium.org/429853002/diff/260001/Source/bindings/scripts...
File Source/bindings/scripts/code_generator_v8.py (right):

https://codereview.chromium.org/429853002/diff/260001/Source/bindings/scripts...
Source/bindings/scripts/code_generator_v8.py:218:
use_relative_output_path=True):
On 2014/08/26 04:00:40, haraken wrote:
> 
> Would you elaborate on why use_relative_output_path is needed?
> 
> I think we discussed this before, but I forgot what conclusion we've reached.

I added this flag to avoid creating subdirectories under
Source/bindings/tests/results/. Without this flag, we will create
Source/bindings/tests/results/bindings/tests/idls directory and put the impl
output into the directory.

I don't think this is a good solution. Another solution is to check
'component_dir'. If it is 'tests', we don't use relative output path. I'll do
that in the next patch set.

https://codereview.chromium.org/429853002/diff/260001/Source/bindings/scripts...
File Source/bindings/scripts/idl_compiler.py (right):

https://codereview.chromium.org/429853002/diff/260001/Source/bindings/scripts...
Source/bindings/scripts/idl_compiler.py:166: # |input_filename| should be a file
which contains list of IDL
On 2014/08/26 04:00:40, haraken wrote:
> 
> list => a list

Done.

https://codereview.chromium.org/429853002/diff/260001/Source/bindings/scripts...
Source/bindings/scripts/idl_compiler.py:170: # |input_filename| should be path
of an IDL file.
On 2014/08/26 04:00:40, haraken wrote:
> 
> path => a path

Done.

https://codereview.chromium.org/429853002/diff/260001/Source/bindings/tests/r...
File Source/bindings/tests/results/V8TestObject.cpp (right):

https://codereview.chromium.org/429853002/diff/260001/Source/bindings/tests/r...
Source/bindings/tests/results/V8TestObject.cpp:8196: testDictionaryArg =
TestDictionary::create();
On 2014/08/26 04:00:40, haraken wrote:
> 
> Can you add a test case for this branch to a layout test?

Done.

https://codereview.chromium.org/429853002/diff/260001/Source/core/testing/Dic...
File Source/core/testing/DictionaryTest.cpp (right):

https://codereview.chromium.org/429853002/diff/260001/Source/core/testing/Dic...
Source/core/testing/DictionaryTest.cpp:25: m_stringMember =
testingDictionary->stringMember();
On 2014/08/26 04:00:40, haraken wrote:
> 
> Just help me understand: Are you assuming that Dictionary users selectively
use
> the hasXXX() checks depending on whether the member is nullable or not? Or are
> you assuming that Dictionary users (can) always use the hasXXX() checks?
> 
> Specifically, I wonder if we should or should not or can add the
> testingDictionary->hasStringMember() check here.

It depends on whether a member has a default value, rather than is nullable. We
can always use hasXXX() method, but when a member has a default value which is
not null, hasXXX() always returns true. That's why I omit hasStringMember()
check. We need to check hasXXX() when the member is primitive type and nullable,
because Nullable<double>::get() doesn't make sense when the value is null.

https://codereview.chromium.org/429853002/diff/260001/Source/core/testing/Dic...
Source/core/testing/DictionaryTest.cpp:40:
result->setStringSequenceMember(m_stringSequenceMember);
On 2014/08/26 04:00:40, haraken wrote:
> 
> Ditto. It's not clear to me when the Dictionary users need the if checks. I'd
> like to understand the rule.

It depends on how the dictionary is defined and the user of dictionary should
know which member has default value. For example, FontFaceDescriptors dictionary
is used by FontFace, and the author of FontFace should know how
FontFaceDescriptors is defined.

https://codereview.chromium.org/429853002/diff/260001/Source/core/testing/Dic...
Source/core/testing/DictionaryTest.cpp:44: void DictionaryTest::trace(Visitor*
visitor)
On 2014/08/26 04:00:40, haraken wrote:
> 
> Drop |visitor|.

Done.

https://codereview.chromium.org/429853002/diff/260001/Source/core/testing/Dic...
File Source/core/testing/DictionaryTest.h (right):

https://codereview.chromium.org/429853002/diff/260001/Source/core/testing/Dic...
Source/core/testing/DictionaryTest.h:35: Nullable<bool> m_booleanOrNullMember;
On 2014/08/26 04:00:40, haraken wrote:
> 
> I'm a bit getting confused with what DictionaryTest is testing.
> 
> Are you assuming that Dictionary users (e.g., FontFaceDescriptors) use
> Nullable<T>? I was expecting that Nullable is just a concept of bindings.

No. I used Nullable<T> here just for convenience. This class is essentially a
dictionary holder, which does nothing but holds all member values. All members
can be retrieved as a dictionary by calling get().

Users of dictionaries should just use member values (or set values to
dictionaries). You can see how users code looks at:
https://codereview.chromium.org/481453003/diff/20001/Source/core/css/FontFace...

https://codereview.chromium.org/429853002/diff/260001/Source/core/testing/Int...
File Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/429853002/diff/260001/Source/core/testing/Int...
Source/core/testing/Internals.idl:208: DictionaryTest dictionaryTest();
On 2014/08/26 04:00:40, haraken wrote:
> 
> I don't quite understand why this needs to be Debug only.

I just followed what privateScriptTest() is doing. Reverted the comment.

[haraken, 2014-08-27 08:37:40]

LGTM

https://codereview.chromium.org/429853002/diff/260001/Source/bindings/scripts...
File Source/bindings/scripts/code_generator_v8.py (right):

https://codereview.chromium.org/429853002/diff/260001/Source/bindings/scripts...
Source/bindings/scripts/code_generator_v8.py:218:
use_relative_output_path=True):
On 2014/08/27 05:06:32, bashi1 wrote:
> On 2014/08/26 04:00:40, haraken wrote:
> > 
> > Would you elaborate on why use_relative_output_path is needed?
> > 
> > I think we discussed this before, but I forgot what conclusion we've
reached.
> 
> I added this flag to avoid creating subdirectories under
> Source/bindings/tests/results/. Without this flag, we will create
> Source/bindings/tests/results/bindings/tests/idls directory and put the impl
> output into the directory.
> 
> I don't think this is a good solution. Another solution is to check
> 'component_dir'. If it is 'tests', we don't use relative output path. I'll do
> that in the next patch set.

That sounds nicer :)

https://codereview.chromium.org/429853002/diff/300001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/429853002/diff/300001/Source/core/core.gypi#n...
Source/core/core.gypi:3340: 'testing/TestingDictionary.idl',

Probably it's slightly better to rename testing/TestingDictionary to
testing/Dictionary. It's a bit confusing to have testing/TestingDictionary and
testing/DictionaryTest.

https://codereview.chromium.org/429853002/diff/300001/Source/core/testing/Int...
File Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/429853002/diff/300001/Source/core/testing/Int...
Source/core/testing/Internals.idl:204: // These is enabled only in Debug builds.

These => This

[bashi, 2014-08-27 10:54:02]

https://codereview.chromium.org/429853002/diff/300001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/429853002/diff/300001/Source/core/core.gypi#n...
Source/core/core.gypi:3340: 'testing/TestingDictionary.idl',
On 2014/08/27 08:37:39, haraken wrote:
> 
> Probably it's slightly better to rename testing/TestingDictionary to
> testing/Dictionary. It's a bit confusing to have testing/TestingDictionary and
> testing/DictionaryTest.

I'd like to avoid "Dictionary" because it sounds generic and there is no
namespace in IDL. How about InternalDictionary?

https://codereview.chromium.org/429853002/diff/300001/Source/core/testing/Int...
File Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/429853002/diff/300001/Source/core/testing/Int...
Source/core/testing/Internals.idl:204: // These is enabled only in Debug builds.
On 2014/08/27 08:37:39, haraken wrote:
> 
> These => This

Oh.. Done.

[haraken, 2014-08-27 10:54:45]

https://codereview.chromium.org/429853002/diff/300001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/429853002/diff/300001/Source/core/core.gypi#n...
Source/core/core.gypi:3340: 'testing/TestingDictionary.idl',
On 2014/08/27 10:54:01, bashi1 wrote:
> On 2014/08/27 08:37:39, haraken wrote:
> > 
> > Probably it's slightly better to rename testing/TestingDictionary to
> > testing/Dictionary. It's a bit confusing to have testing/TestingDictionary
and
> > testing/DictionaryTest.
> 
> I'd like to avoid "Dictionary" because it sounds generic and there is no
> namespace in IDL. How about InternalDictionary?

InternalDictionary sounds good.

[bashi, 2014-08-27 11:55:52]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2014-08-27 11:56:21]

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

[commit-bot: I haz the power, 2014-08-27 12:25:33]

Committed patchset #9 (320001) as 180969