[bindings] partial interfaces should not violate componentization

[bindings] partial interfaces should not violate componentization

Design doc:
https://docs.google.com/a/google.com/document/d/1TpqG_bWJfHFo6swv5zbqfjxHl_K31C307NvdyF1NAcU/edit

https://docs.google.com/a/google.com/document/d/1TLOCAG1DrW7UdI_DVIclHeNn8rdHzDzK52gPPY8Uo5I/edit
(idea A: 
 core_idl_files: no partial interfaces in modules,
 core_idl_with_modules_dependency_files: partial interfaces in modules,
 webcore_testing_idl_files,
 webcore_testing_idl_with_modules_dependency_files: ditto)

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

[tasak, 2014-10-02 07:59:12]

Patchset #1 (id:1) has been deleted

[tasak, 2014-10-02 08:36:33]

Patchset #1 (id:2) has been deleted

[tasak, 2014-10-02 08:45:33]

tasak@google.com changed reviewers:
+ bashi@chromium.org, haraken@chromium.org, nbarth@chromium.org

[tasak, 2014-10-02 08:45:34]

Would you review this CL?

[haraken, 2014-10-02 11:12:23]

Thanks for the CL!

May I ask you to add some comments (using the Rietvled comments) to help review?
I need some guide to understand which part is doing what.

[tasak, 2014-10-02 13:48:19]

On 2014/10/02 11:12:23, haraken wrote:
> Thanks for the CL!
> 
> May I ask you to add some comments (using the Rietvled comments) to help
review?
> I need some guide to understand which part is doing what.

Sure.

[haraken, 2014-10-09 02:00:38]

On 2014/10/02 13:48:19, tasak wrote:
> On 2014/10/02 11:12:23, haraken wrote:
> > Thanks for the CL!
> > 
> > May I ask you to add some comments (using the Rietvled comments) to help
> review?
> > I need some guide to understand which part is doing what.
> 
> Sure.

Sorry, the design document is clear enough to understand what the CL is doing.
I'll take a closer look at the CL.

[tasak, 2014-10-09 02:29:45]

On 2014/10/09 02:00:38, haraken wrote:

> Sorry, the design document is clear enough to understand what the CL is doing.
> I'll take a closer look at the CL.

I found that current run-bindings-tests cannot test partial interface
modularization.

I'm now trying to fix the issue.

[haraken, 2014-10-09 04:24:02]

Overall, the approach looks good.

The only part we might want to reduce complexity is the 'visible' handling for
method overloading. I have no concrete idea but it would be great if we could
make it simpler.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/core/idl...
File Source/bindings/core/idl.gypi (right):

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/core/idl...
Source/bindings/core/idl.gypi:38: # Write lists of core IDL files which has
dependency IDL files in modules.

lists => a list
has => have

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Binding.h:1025: typedef void
(*InstallTemplateFunction)(v8::Handle<v8::FunctionTemplate>, v8::Isolate*);

I'd add this to V8DOMConfiguration.h.

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/generated.gyp:113: '--component-dir',

Do we have a plan to remove the --component-dir option?

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/modules/...
File Source/bindings/modules/generated.gyp (right):

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/modules/...
Source/bindings/modules/generated.gyp:147:
'<@(core_modules_dependency_idl_files)',

Just help me understand: Why does this take core_idl_files, not
modules_idl_files?

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/modules/...
File Source/bindings/modules/v8/generated.gypi (right):

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/modules/...
Source/bindings/modules/v8/generated.gypi:16:
'bindings_modules_v8_generated_partial_aggregate_files': [

Can we write this outside the 'OS=="win" and buildtype=="Official"' branch?

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/modules/...
Source/bindings/modules/v8/generated.gypi:41:
'bindings_modules_v8_generated_partial_aggregate_files': [

Then you can remove this, I think.

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/generate_init_partial_interfaces.py:35: void
ModuleBindingsInitializer::initPartialInterface()

It's a bit hacky to define ModuleBindingsInitializer's method here.

Shall we:

- define ModuleBindingsInitializer::initPartialInterface() in
ModuleBindingsInitializer.cpp
- define initializeModulesPartialInterfaces() in the aggregated cpp
- call initializeModulesPartialInterfaces() from
ModuleBindingsInitializer::initPartialInterface()

?

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/generate_init_partial_interfaces.py:47:
parser.add_option('--cache-directory',

Is --cache-directory used?

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/generate_init_partial_interfaces.py:91: conditional_code
= '#if %s' % format_conditional(conditional)

Do you need to support conditional? Conditional is deprecated.

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/idl_definitions.py:278: self.original_interface = None

Is "original interface" a term of Web IDL? I'm not sure of the best name.
original interface, base interface, dependent_interface, etc.

I'm fine with "original interface" if Web IDL doesn't have a speced term.

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/interface_dependency_resolver.py:158: if not
is_valid_component_dependency(component, dependency_component) and
dependency_interface.is_partial:

Would you elaborate on what this is doing?

Why do we allow a case where is_valid_component_dependency returns false?

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/utilities.py:34: if not basename:

When can this happen?

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:80: if not interface.is_partial:

Avoid 'if not ... else ...'. Use 'if ... else ...'.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:114: if 'Iterable' in
extended_attributes and not interface.is_partial:

Add a FIXME to support Iterable in partial interfaces?

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:204: if not interface.is_partial:

Use if ... else ....

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:412: def
update_partial_interface_constructor_context(interface, context):

Why can we share the code with update_interface_constructor_context ?

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:413: # [Constructor], [NamedConstructor]
must not be specified on partial interface

Can we raise an exception if this happens?

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/attributes.cpp:193:
{{cpp_class}}PartialV8Internal::{{cpp_class}}ConstructorGetter{{world_suffix}}(property,
info);

Is it possible that a partial interface has a constructor?

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/attributes.cpp:402: {% set empty_or_partial = '' if
not is_partial else 'Partial' %}

As commented in another place, we want to avoid creating the namespace name
repeatedly in templates. Can we create the namespace name once in the code
generator and pass it to the templates?

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/interface.cpp:732: {% set installTemplateFunction =
'install%sTemplate' % v8_class if not has_partial_interface else
'%s::install%sTemplateFunction' % (v8_class, v8_class) %}

Instead of using 'if not ... else ...', use 'if ... else ...'.

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/interface.h:173: {% if method.overloads and
method.overloads.partial_overloads %}

partial_overloads => has_partial_overloads

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/interface_base.cpp:29: {% set wrapper_type_info_type =
'const ' if not has_partial_interface else '' %}

Avoid 'if not ... else ...'. Use 'if ... else ...'.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/interface_base.cpp:141: {% endif %}

You can land this change separately.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/interface_base.cpp:165: {% if method_visible or
overloads_callback_visible_in_partial or overloads_callback_visible_in_original
%}

Add a comment about what these variables mean.

(Also it would be great if we could decrease the number of these |visible|
variables. It looks complicated.)

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/interface_base.cpp:274:
functionTemplate->ReadOnlyPrototype();

Why don't you need to call ReadOnlyPrototype if not is_partial ?

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/methods.cpp:400: {# #}

Add a comment.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/methods.cpp:622: '%s%sV8Internal::%sMethodCallback' %
(cpp_class, 'Partial' if is_partial else '', method.name) %}

We're creating the namespace name repeatedly in templates. Can we avoid this by
passing the namespace name from the code generator?

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.cpp:3:
{##############################################################################}

Nit: I'd drop {######} just to list up outlined blocks.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.cpp:28: {% from 'methods.cpp' import
union_type_method_call_and_set_return_value %}

Is this needed?

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.cpp:120: {# install per-context
enabled properties defined in master interface. #}

master => original

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.cpp:136: {# invoke
installConditionallyEnabledProperties defined in master #}

master => original interface

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.cpp:148: {# Define per-context
enabled operations defined in master interfaces #}

master => original

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.cpp:153: ExecutionContext* context =
toExecutionContext(prototypeObject->CreationContext());

Where is prototypeObject defined?

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.cpp:158: {% filter
exposed(method.exposed_test) %}

Shall we use 'for ... if ...' instead of filters, for consistency with
installConditionallyEnabledProperties ?

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.cpp:167: {# invoke
installConditionallyEnabledMethods defined in master #}

master => original interface

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.cpp:190: {% block create_wrapper %}

I don't fully understand why we need createWrapper(). According to the document
(https://docs.google.com/a/google.com/document/d/1TpqG_bWJfHFo6swv5zbqfjxHl_K3...),
createWrapper() is not needed in the generated code for partial interfaces, is
it?

Other than createWrapper(), I'm not sure if we need to duplicate so many things
(e.g., toActiveDOMObject etc) in the generated code for partial interfaces.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.cpp:207: {% if not
method.overload_index or method.overloads %}

I don't understand this condition. This if branch should be always true because
method.overloads is true here.

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.h:10: {% filter
conditional(conditional_string) %}

Do we need to support conditional? Conditional is deprecated.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.h:22: {% filter
conditional(method.conditional_string) %}

Ditto.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.h:38: {% if
has_custom_legacy_call_as_function %}

[LegacyCallAsFunction] is also deprecated. Do we need to support it?

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/tests/re...
File Source/bindings/tests/results/core/V8TestInterface.cpp (right):

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/V8TestInterface.cpp:1414: if
(partial2VoidMethodMethodForPartialInterface) {

Is it possible that partial2VoidMethodMethodForPartialInterface is 0 here?

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/V8TestInterface.cpp:1422: if
(partial2VoidMethodMethodForPartialInterface) {

Ditto.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/V8TestInterface.cpp:1866: return
V8DOMConfiguration::domClassTemplate(isolate,
const_cast<WrapperTypeInfo*>(&wrapperTypeInfo),
V8TestInterface::installV8TestInterfaceTemplateFunction);

Can we add an assert to verify that
V8TestInterface::installV8TestInterfaceTemplateFunction is NOT equal to
&V8TestInterface::installV8TestInterfaceTemplate ?

I want to make sure that updateWrapperTypeInfo is called before domTemplate is
called.

https://codereview.chromium.org/618373003/diff/40001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/618373003/diff/40001/Source/core/core.gypi#ne...
Source/core/core.gypi:449: # core defines, and modules updates with 'partial
interface'

# core IDL files that have partial interfaces in modules.

https://codereview.chromium.org/618373003/diff/40001/Source/core/core.gypi#ne...
Source/core/core.gypi:450: 'core_modules_dependency_idl_files': [

core_idl_files_with_modules_dependency ?

What happens if we write "an IDL file that should be in
core_idl_files_with_modules_dependency" in core_idl_files? Will it lead to a
compile error?

I'm fine with the approach of statically listing IDL files in GYP files, but we
should have a way to detect an error if a developer writes an IDL file in a
wrong target.

https://codereview.chromium.org/618373003/diff/40001/Source/core/core.gypi#ne...
Source/core/core.gypi:3448: # core defines, and modules updates with 'partial
interface'

# testing IDL files that have partial interfaces in modules.

https://codereview.chromium.org/618373003/diff/40001/Source/modules/modules.gyp
File Source/modules/modules.gyp (right):

https://codereview.chromium.org/618373003/diff/40001/Source/modules/modules.g...
Source/modules/modules.gyp:57:
'<(bindings_modules_v8_output_dir)/InitModulesForPartialInterface.cpp',

InitModulesForPartialInterface.cpp => InitModulesForPartialInterfaces.cpp

https://codereview.chromium.org/618373003/diff/40001/public/platform/WebRect.h
File public/platform/WebRect.h (right):

https://codereview.chromium.org/618373003/diff/40001/public/platform/WebRect....
public/platform/WebRect.h:41: #undef ceil

Add a FIXME about this.

[tasak, 2014-10-10 07:39:06]

Patchset #5 (id:440002) has been deleted

[tasak, 2014-10-10 07:39:29]

Patchset #5 (id:440002) has been deleted

[tasak, 2014-10-10 07:42:15]

Patchset #4 (id:120001) has been deleted

[tasak, 2014-10-10 07:52:25]

Thank you for reviewing.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/core/idl...
File Source/bindings/core/idl.gypi (right):

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/core/idl...
Source/bindings/core/idl.gypi:38: # Write lists of core IDL files which has
dependency IDL files in modules.
On 2014/10/09 04:24:00, haraken wrote:
> 
> lists => a list
> has => have

Done.

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/generated.gyp:113: '--component-dir',
On 2014/10/09 04:24:00, haraken wrote:
> 
> Do we have a plan to remove the --component-dir option?

Currently, no, because we have no information about where we need to generate
code, i.e. gen/blink/bindings/core/v8 or gen/blink/bindings/modules/v8, for
core_idl_with_modules_dependency_files.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/modules/...
File Source/bindings/modules/generated.gyp (right):

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/modules/...
Source/bindings/modules/generated.gyp:147:
'<@(core_modules_dependency_idl_files)',
On 2014/10/09 04:24:00, haraken wrote:
> 
> Just help me understand: Why does this take core_idl_files, not
> modules_idl_files?

Because:
[before] core_idl_files ==> [now] core_idl_files +
core_idl_with_modules_dependency_files.

When removing core_idl_with_modules_dependency_files, http/tests/serviceworker
fails because of missing some constructors, e.g. URL.

FAIL Verify access control of fetch() in a Service Worker assert_equals:
Username must match. url:
http://127.0.0.1:8000/serviceworker/resources/fetch-access-control.php?method...
expected "undefined" but got "username1"

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/modules/...
File Source/bindings/modules/v8/generated.gypi (right):

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/modules/...
Source/bindings/modules/v8/generated.gypi:16:
'bindings_modules_v8_generated_partial_aggregate_files': [
On 2014/10/09 04:24:00, haraken wrote:
> 
> Can we write this outside the 'OS=="win" and buildtype=="Official"' branch?

Done.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/modules/...
Source/bindings/modules/v8/generated.gypi:41:
'bindings_modules_v8_generated_partial_aggregate_files': [
On 2014/10/09 04:24:00, haraken wrote:
> 
> Then you can remove this, I think.

Done.

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/generate_init_partial_interfaces.py:35: void
ModuleBindingsInitializer::initPartialInterface()
On 2014/10/09 04:24:00, haraken wrote:
> 
> It's a bit hacky to define ModuleBindingsInitializer's method here.
> 
> Shall we:
> 
> - define ModuleBindingsInitializer::initPartialInterface() in
> ModuleBindingsInitializer.cpp
> - define initializeModulesPartialInterfaces() in the aggregated cpp
> - call initializeModulesPartialInterfaces() from
> ModuleBindingsInitializer::initPartialInterface()
> 
> ?

Done.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/generate_init_partial_interfaces.py:47:
parser.add_option('--cache-directory',
On 2014/10/09 04:24:00, haraken wrote:
> 
> Is --cache-directory used?

IdlReader uses.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/generate_init_partial_interfaces.py:91: conditional_code
= '#if %s' % format_conditional(conditional)
On 2014/10/09 04:24:00, haraken wrote:
> 
> Do you need to support conditional? Conditional is deprecated.

I think, it would be better to remove all conditional from
Source/bindings/scripts/*.py in another CL.

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/idl_definitions.py:278: self.original_interface = None
On 2014/10/09 04:24:00, haraken wrote:
> 
> Is "original interface" a term of Web IDL? I'm not sure of the best name.
> original interface, base interface, dependent_interface, etc.
> 
> I'm fine with "original interface" if Web IDL doesn't have a speced term.

I could not find any special term in WebIDL spec. However, I found:

Note
A partial interface definition cannot specify that the interface inherits from
another interface. Inheritance must be specified on the original interface 
definition.

or

4.3.5 Exposed
...
- partial interface
  The exposure set of the partial interface is the exposure set of the original
interface definition.

http://heycam.github.io/webidl/

So I think, we can use "original".

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/interface_dependency_resolver.py:158: if not
is_valid_component_dependency(component, dependency_component) and
dependency_interface.is_partial:
On 2014/10/09 04:24:00, haraken wrote:
> 
> Would you elaborate on what this is doing?
> 
> Why do we allow a case where is_valid_component_dependency returns false?

Done.

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/utilities.py:34: if not basename:
On 2014/10/09 04:24:00, haraken wrote:
> 
> When can this happen?

I found that os.path.split('/') returns '/', None.

So this "while" doesn't stop because path = '/'.

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:80: if not interface.is_partial:
On 2014/10/09 04:24:00, haraken wrote:
> 
> Avoid 'if not ... else ...'. Use 'if ... else ...'.

Done.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:114: if 'Iterable' in
extended_attributes and not interface.is_partial:
On 2014/10/09 04:24:00, haraken wrote:
> 
> Add a FIXME to support Iterable in partial interfaces?

Done.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:204: if not interface.is_partial:
On 2014/10/09 04:24:00, haraken wrote:
> 
> Use if ... else ....

Done.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:412: def
update_partial_interface_constructor_context(interface, context):
On 2014/10/09 04:24:00, haraken wrote:
> 
> Why can we share the code with update_interface_constructor_context ?

Done.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:413: # [Constructor], [NamedConstructor]
must not be specified on partial interface
On 2014/10/09 04:24:00, haraken wrote:
> 
> Can we raise an exception if this happens?

Done.

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/attributes.cpp:193:
{{cpp_class}}PartialV8Internal::{{cpp_class}}ConstructorGetter{{world_suffix}}(property,
info);
On 2014/10/09 04:24:00, haraken wrote:
> 
> Is it possible that a partial interface has a constructor?

This code is used for ServiceWorker, i.e.:

partial interface ServiceWorkerGlobalScope {
    [RuntimeEnabled=EncodingAPI] attribute TextDecoderConstructor TextDecoder;
    [RuntimeEnabled=EncodingAPI] attribute TextEncoderConstructor TextEncoder;
...
};

c.f. gen/blink/bindings/modules/ServiceWorkerGlobalScopeModulesConstructors.idl

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/attributes.cpp:402: {% set empty_or_partial = '' if
not is_partial else 'Partial' %}
On 2014/10/09 04:24:00, haraken wrote:
> 
> As commented in another place, we want to avoid creating the namespace name
> repeatedly in templates. Can we create the namespace name once in the code
> generator and pass it to the templates?

Done.
Added "acutal_cpp_class" and "actual_v8_class" for
"{{cpp_class}}{{empty_or_partial}}" and "{{v8_class}}{{empty_or_partial}}".

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/interface.cpp:732: {% set installTemplateFunction =
'install%sTemplate' % v8_class if not has_partial_interface else
'%s::install%sTemplateFunction' % (v8_class, v8_class) %}
On 2014/10/09 04:24:01, haraken wrote:
> 
> Instead of using 'if not ... else ...', use 'if ... else ...'.

Done.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/interface.cpp:766: {% if has_conditional_attributes %}
This code is now in interface_base.cpp.

I created a patch for this move:
https://codereview.chromium.org/645833002

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/interface.h:173: {% if method.overloads and
method.overloads.partial_overloads %}
On 2014/10/09 04:24:01, haraken wrote:
> 
> partial_overloads => has_partial_overloads

Done.

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/interface_base.cpp:29: {% set wrapper_type_info_type =
'const ' if not has_partial_interface else '' %}
On 2014/10/09 04:24:01, haraken wrote:
> 
> Avoid 'if not ... else ...'. Use 'if ... else ...'.

Done.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/interface_base.cpp:141: {% endif %}
On 2014/10/09 04:24:01, haraken wrote:
> 
> You can land this change separately.

Done.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/interface_base.cpp:165: {% if method_visible or
overloads_callback_visible_in_partial or overloads_callback_visible_in_original
%}
On 2014/10/09 04:24:01, haraken wrote:
> 
> Add a comment about what these variables mean.
> 
> (Also it would be great if we could decrease the number of these |visible|
> variables. It looks complicated.)

Done.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/interface_base.cpp:274:
functionTemplate->ReadOnlyPrototype();
On 2014/10/09 04:24:01, haraken wrote:
> 
> Why don't you need to call ReadOnlyPrototype if not is_partial ?

When partial interface, ReadOnlyPrototype will be invoked in the original
interface's install{{vy_class}}Template.

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/methods.cpp:400: {# #}
On 2014/10/09 04:24:01, haraken wrote:
> 
> Add a comment.

Done.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/methods.cpp:622: '%s%sV8Internal::%sMethodCallback' %
(cpp_class, 'Partial' if is_partial else '', method.name) %}
On 2014/10/09 04:24:01, haraken wrote:
> 
> We're creating the namespace name repeatedly in templates. Can we avoid this
by
> passing the namespace name from the code generator?

Done.
Added actual_cpp_class and actual_v8_class for this purpose.

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.cpp:3:
{##############################################################################}
On 2014/10/09 04:24:01, haraken wrote:
> 
> Nit: I'd drop {######} just to list up outlined blocks.

Done.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.cpp:28: {% from 'methods.cpp' import
union_type_method_call_and_set_return_value %}
On 2014/10/09 04:24:01, haraken wrote:
> 
> Is this needed?

Done.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.cpp:120: {# install per-context
enabled properties defined in master interface. #}
On 2014/10/09 04:24:01, haraken wrote:
> 
> master => original

Done.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.cpp:136: {# invoke
installConditionallyEnabledProperties defined in master #}
On 2014/10/09 04:24:01, haraken wrote:
> 
> master => original interface

Done.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.cpp:148: {# Define per-context
enabled operations defined in master interfaces #}
On 2014/10/09 04:24:01, haraken wrote:
> 
> master => original

Done.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.cpp:153: ExecutionContext* context =
toExecutionContext(prototypeObject->CreationContext());
On 2014/10/09 04:24:01, haraken wrote:
> 
> Where is prototypeObject defined?

This is a bug when I copied code from interface.cpp.

So I moved installConditionallyEnabled{Methods|Properties} in interface.cpp into
interface_base.cpp.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.cpp:158: {% filter
exposed(method.exposed_test) %}
On 2014/10/09 04:24:01, haraken wrote:
> 
> Shall we use 'for ... if ...' instead of filters, for consistency with
> installConditionallyEnabledProperties ?

Looking at the newest code, installConditionallyEnabledProperties uses filters?

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.cpp:167: {# invoke
installConditionallyEnabledMethods defined in master #}
On 2014/10/09 04:24:01, haraken wrote:
> 
> master => original interface

Done.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.cpp:190: {% block create_wrapper %}
Yeah, we don't need createWrapper for partial interfaces.

So this is just for explicitly making createWrapper block empty.

Since interface_base.cpp's create_wrapper block is empty, we can remove this
block.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.cpp:207: {% if not
method.overload_index or method.overloads %}
On 2014/10/09 04:24:01, haraken wrote:
> 
> I don't understand this condition. This if branch should be always true
because
> method.overloads is true here.

This should be "method.overloads and method.overloads.partial_overloads".
I removed "if not method.overload_index or method.overloads".

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

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.h:10: {% filter
conditional(conditional_string) %}
On 2014/10/09 04:24:01, haraken wrote:
> 
> Do we need to support conditional? Conditional is deprecated.

I think, this is another issue, because I found several
conditional(conditional_string):
Source/bindings/templates/callback_interface.cpp:{% filter
conditional(conditional_string) %}
Source/bindings/templates/interface_base.cpp:{% filter
conditional(conditional_string) %}
...

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.h:20: static
v8::Handle<v8::FunctionTemplate> domTemplate(v8::Isolate*);
We don't need to generate domTemplate for partial interfaces.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/template...
Source/bindings/templates/partial_interface.h:38: {% if
has_custom_legacy_call_as_function %}
On 2014/10/09 04:24:02, haraken wrote:
> 
> [LegacyCallAsFunction] is also deprecated. Do we need to support it?

I think, we should remove this in another CL.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/tests/re...
File Source/bindings/tests/results/core/V8TestInterface.cpp (right):

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/V8TestInterface.cpp:1414: if
(partial2VoidMethodMethodForPartialInterface) {
On 2014/10/09 04:24:02, haraken wrote:
> 
> Is it possible that partial2VoidMethodMethodForPartialInterface is 0 here?

Done.
If we want downloadable modules, we will initialize this lazily.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/V8TestInterface.cpp:1422: if
(partial2VoidMethodMethodForPartialInterface) {
On 2014/10/09 04:24:02, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/618373003/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/V8TestInterface.cpp:1866: return
V8DOMConfiguration::domClassTemplate(isolate,
const_cast<WrapperTypeInfo*>(&wrapperTypeInfo),
V8TestInterface::installV8TestInterfaceTemplateFunction);
On 2014/10/09 04:24:02, haraken wrote:
> 
> Can we add an assert to verify that
> V8TestInterface::installV8TestInterfaceTemplateFunction is NOT equal to
> &V8TestInterface::installV8TestInterfaceTemplate ?
> 
> I want to make sure that updateWrapperTypeInfo is called before domTemplate is
> called.

Done.

https://codereview.chromium.org/618373003/diff/40001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/618373003/diff/40001/Source/core/core.gypi#ne...
Source/core/core.gypi:449: # core defines, and modules updates with 'partial
interface'
On 2014/10/09 04:24:02, haraken wrote:
> 
> # core IDL files that have partial interfaces in modules.

Done.

https://codereview.chromium.org/618373003/diff/40001/Source/core/core.gypi#ne...
Source/core/core.gypi:450: 'core_modules_dependency_idl_files': [
On 2014/10/09 04:24:02, haraken wrote:
> 
> core_idl_files_with_modules_dependency ?
> 
> What happens if we write "an IDL file that should be in
> core_idl_files_with_modules_dependency" in core_idl_files? Will it lead to a
> compile error?
> 
> I'm fine with the approach of statically listing IDL files in GYP files, but
we
> should have a way to detect an error if a developer writes an IDL file in a
> wrong target.

If we add an idl file with modules dependency to code_idl_files, we will see no
compile error and will see layout test regression (e.g. undefined XXX).

I think, presubmit check or bindings tests will help us to detect such errors.

https://codereview.chromium.org/618373003/diff/40001/Source/core/core.gypi#ne...
Source/core/core.gypi:3448: # core defines, and modules updates with 'partial
interface'
On 2014/10/09 04:24:02, haraken wrote:
> 
> # testing IDL files that have partial interfaces in modules.

Done.

https://codereview.chromium.org/618373003/diff/40001/Source/modules/modules.gyp
File Source/modules/modules.gyp (right):

https://codereview.chromium.org/618373003/diff/40001/Source/modules/modules.g...
Source/modules/modules.gyp:57:
'<(bindings_modules_v8_output_dir)/InitModulesForPartialInterface.cpp',
On 2014/10/09 04:24:02, haraken wrote:
> 
> InitModulesForPartialInterface.cpp => InitModulesForPartialInterfaces.cpp

Done.

https://codereview.chromium.org/618373003/diff/40001/public/platform/WebRect.h
File public/platform/WebRect.h (right):

https://codereview.chromium.org/618373003/diff/40001/public/platform/WebRect....
public/platform/WebRect.h:41: #undef ceil
On 2014/10/09 04:24:02, haraken wrote:
> 
> Add a FIXME about this.

Done.

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/tests/i...
File Source/bindings/tests/idls/core/TestInterface.idl (right):

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/tests/i...
Source/bindings/tests/idls/core/TestInterface.idl:94: static void
staticVoidMethodPartialOverload();
Typo. Partail => Partial

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/V8TestInterface.h (right):

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/tests/r...
Source/bindings/tests/results/core/V8TestInterface.h:56: static void
registerStaticVoidMethodPartialOverloadMethodForPartialInterface(void (*)(const
v8::FunctionCallbackInfo<v8::Value>&));
Newly added, because I fixed typo,

https://codereview.chromium.org/618373003/diff/840001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/bindings/main.py (right):

https://codereview.chromium.org/618373003/diff/840001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/bindings/main.py:236: partial_interface_output_dir =
os.path.join(output_directory,
Added to test partial interface code, i.e. V8{{cpp_class}}Partial.{h,cpp}.

[bashi, 2014-10-10 09:48:11]

I just glanced at the first half and haven't took a close look yet. will do next
week. Sorry for slow review..

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/modules...
File Source/bindings/modules/v8/generated.gyp (right):

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/modules...
Source/bindings/modules/v8/generated.gyp:211: # Update that regex if command
line changes (other than changing flags)
I'm not sure, but you might need to take a look at
src/build/sanitize-win-build-log.sed in chromium repository.

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/modules...
Source/bindings/modules/v8/generated.gyp:252: '1',
It's difficult to understand what '1' means here.

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/modules...
Source/bindings/modules/v8/generated.gyp:302:
'bindings_modules_v8_generated_init_partial',
nit: alphabetical order

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
File Source/bindings/scripts/aggregate_generated_bindings.py (right):

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/aggregate_generated_bindings.py:105:
parser.add_option('--partial', type='int', help='if true, aggregates partial
interface code, e.g. V8XXXPartial.cpp')
--partial -> --aggregate-partial-interfaces

How about using a boolean flag option instead int?

parser.add_option('--partial', help='...', action='store_true', default=False)

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/aggregate_generated_bindings.py:124: parser.error('Must
specify exactly more than 1 output file as argument, but %d given.' % len(args))
'Must specify one or more output files...' ?

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/aggregate_generated_bindings.py:214: component =
idl_filename_to_component(idl_filename)
Can |component| be different from options.component_dir?

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/aggregate_generated_bindings.py:216: if
len(definitions.keys()) == 1 and definitions[component]:
Could you elaborate on this condition?

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

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/code_generator_v8.py:157: for method in
definitions.interfaces.values():
Could you explain why this change is needed?

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
File Source/bindings/scripts/generate_init_partial_interfaces.py (right):

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/generate_init_partial_interfaces.py:53:
parser.disable_interspersed_args()
It seems that we have a lot of copy-and-paste code like this. I understand there
are subtle differences but it would be great if we could merge them.

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/generate_init_partial_interfaces.py:77: header_files =
[]
unused?

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/generate_init_partial_interfaces.py:78: initialize_calls
= []
remove? (the same as L86)

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

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/idl_compiler.py:55:
parser.add_option('--generate-partial', action='store_true')
default=False

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/idl_compiler.py:119: if output_code_list is None:
When does this happen?

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/idl_compiler.py:122: if output_code and output_path:
When does this happen?

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
File Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/idl_definitions.py:278: self.original_interface = None
This looks strange to me.. My understanding is that idl definitions only have
information which come from idl parser. All other properties are set in this
initializer, but |original_interface| and |partial_interfaces| are not. I think
this implies that there would be a better place to hold such information.

[tasak, 2014-10-10 11:40:03]

Patchset #5 (id:1100001) has been deleted

[tasak, 2014-10-10 11:43:40]

Thank you for reviewing.

I removed codes which depend on old design (i.e. no
core_idl_with_modules_dependency_files).

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

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/core/v8...
Source/bindings/core/v8/generated.gyp:17:
'../../../bindings/modules/generated.gypi',
Removed, because of unused.

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/modules...
File Source/bindings/modules/v8/generated.gyp (right):

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/modules...
Source/bindings/modules/v8/generated.gyp:211: # Update that regex if command
line changes (other than changing flags)
On 2014/10/10 09:48:11, bashi1 wrote:
> I'm not sure, but you might need to take a look at
> src/build/sanitize-win-build-log.sed in chromium repository.

Done.

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/modules...
Source/bindings/modules/v8/generated.gyp:252: '1',
On 2014/10/10 09:48:11, bashi1 wrote:
> It's difficult to understand what '1' means here.

Done.

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/modules...
Source/bindings/modules/v8/generated.gyp:302:
'bindings_modules_v8_generated_init_partial',
On 2014/10/10 09:48:11, bashi1 wrote:
> nit: alphabetical order

Done.

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
File Source/bindings/scripts/aggregate_generated_bindings.py (right):

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/aggregate_generated_bindings.py:105:
parser.add_option('--partial', type='int', help='if true, aggregates partial
interface code, e.g. V8XXXPartial.cpp')
On 2014/10/10 09:48:11, bashi1 wrote:
> --partial -> --aggregate-partial-interfaces
> 
> How about using a boolean flag option instead int?
> 
> parser.add_option('--partial', help='...', action='store_true', default=False)

Removed this flag.

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/aggregate_generated_bindings.py:124: parser.error('Must
specify exactly more than 1 output file as argument, but %d given.' % len(args))
On 2014/10/10 09:48:11, bashi1 wrote:
> 'Must specify one or more output files...' ?

Done.

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/aggregate_generated_bindings.py:214: component =
idl_filename_to_component(idl_filename)
On 2014/10/10 09:48:11, bashi1 wrote:
> Can |component| be different from options.component_dir?

Yes.
To aggregate partial interface code, idl_filename is defined in
core_idl_with_modules_dependency_files, but component_dir = modules.

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/aggregate_generated_bindings.py:216: if
len(definitions.keys()) == 1 and definitions[component]:
On 2014/10/10 09:48:11, bashi1 wrote:
> Could you elaborate on this condition?

Removed.
Instead, added "raise exception" when a given interface has no partial
interfaces.

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

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/code_generator_v8.py:157: for method in
definitions.interfaces.values():
On 2014/10/10 09:48:11, bashi1 wrote:
> Could you explain why this change is needed?

This is for avoiding exceptions when --generate-partial is enabled for
core_idl_files without modules dependency.
Now we are using core_idl_with_modules_dependency_files. So we don't need this.

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
File Source/bindings/scripts/generate_init_partial_interfaces.py (right):

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/generate_init_partial_interfaces.py:53:
parser.disable_interspersed_args()
On 2014/10/10 09:48:11, bashi1 wrote:
> It seems that we have a lot of copy-and-paste code like this. I understand
there
> are subtle differences but it would be great if we could merge them.

Removed cache-directory, interfaces-info.

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/generate_init_partial_interfaces.py:77: header_files =
[]
On 2014/10/10 09:48:11, bashi1 wrote:
> unused?

Done.

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/generate_init_partial_interfaces.py:78: initialize_calls
= []
On 2014/10/10 09:48:11, bashi1 wrote:
> remove? (the same as L86)

Done.

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

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/idl_compiler.py:55:
parser.add_option('--generate-partial', action='store_true')
On 2014/10/10 09:48:11, bashi1 wrote:
> default=False

Done.

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/idl_compiler.py:119: if output_code_list is None:
On 2014/10/10 09:48:11, bashi1 wrote:
> When does this happen?

Now we are using core_idl_with_modules_dependency_files. So we don't need this.
I removed.

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/idl_compiler.py:122: if output_code and output_path:
On 2014/10/10 09:48:11, bashi1 wrote:
> When does this happen?

Done.

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
File Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/idl_definitions.py:278: self.original_interface = None
On 2014/10/10 09:48:11, bashi1 wrote:
> This looks strange to me.. My understanding is that idl definitions only have
> information which come from idl parser. All other properties are set in this
> initializer, but |original_interface| and |partial_interfaces| are not. I
think
> this implies that there would be a better place to hold such information.

So interface_dependency_resolver updates idl definitions. This looks also
strange?

[tasak, 2014-10-14 05:24:54]

Patchset #6 (id:1420001) has been deleted

[tasak, 2014-10-14 05:28:21]

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/core/v...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/core/v...
Source/bindings/core/v8/V8Binding.h:1024: typedef void
(*InstallTemplateFunction)(v8::Handle<v8::FunctionTemplate>, v8::Isolate*);
If moving this typedef to V8DOMConfiguration.h, we need to change V8Window.h to
include V8DOMConfiguration.h instead of V8Binding.h.

So I would like to keep this typdef in V8Binding.h.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
File Source/bindings/scripts/aggregate_generated_bindings.py (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/aggregate_generated_bindings.py:180: raise
Exception('Expected at least 5 arguments.')
(diff patchset 5)
Should be 5, not 4.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
File Source/bindings/scripts/generate_init_partial_interfaces.py (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/generate_init_partial_interfaces.py:75: for
interface_name in interface_names]
(diff patchset 5)
Removed conditional, because no partial interfaces use conditional.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
File Source/bindings/scripts/idl_compiler.py (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/idl_compiler.py:57: 'component of input idl file')
Now specifying target component instead of --generate-partial.
For example, to generate V8URLPartial.{h,cpp}, a given idl file(URL.idl)'s
component is 'core', and target-component is 'modules'.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/templa...
File Source/bindings/templates/partial_interface.cpp (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/templa...
Source/bindings/templates/partial_interface.cpp:2: 
Removed empty blocks.
(These blocks are also empty in interface_base.cpp)

[bashi, 2014-10-15 05:29:24]

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
File Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/idl_definitions.py:278: self.original_interface = None
On 2014/10/10 11:43:40, tasak wrote:
> On 2014/10/10 09:48:11, bashi1 wrote:
> > This looks strange to me.. My understanding is that idl definitions only
have
> > information which come from idl parser. All other properties are set in this
> > initializer, but |original_interface| and |partial_interfaces| are not. I
> think
> > this implies that there would be a better place to hold such information.
> 
> So interface_dependency_resolver updates idl definitions. This looks also
> strange?

That means we have two paths; the first path reads an idl file and create
definitions, the second path updates (or additionary creates) definitions based
on dependencies. I feel that it might be clear that if we could explicitly treat
these information separately (information come from each path). But I'm not sure
it's worth doing, so I'm ok with this.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/core/v...
File Source/bindings/core/v8/generated.gyp (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/core/v...
Source/bindings/core/v8/generated.gyp:17:
#'../../../bindings/modules/generated.gypi',
Remove?

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
File Source/bindings/scripts/interface_dependency_resolver.py (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/interface_dependency_resolver.py:144: No return:
modifies target_interface in place.
Could you update the comment? This method returns a value. Maybe just refer
resolve_dependencies()?

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/interface_dependency_resolver.py:177:
resolved_definitions[dependency_component].update(dependency_definitions)
Don't we need to update |original_interface| and |partial_interfaces|?

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/interface_dependency_resolver.py:183:
dependency_interface.extended_attributes.update(extended_attributes)
Can't we do below?

dependency_interface.extended_attributes.update(target_interface.extended_attributes)

This doesn't work if there is a conflict, but in such cases, shouldn't we raise
an exception?

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
File Source/bindings/scripts/utilities.py (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/utilities.py:34: if not basename:
Why is this required? As far as I tested,

os.path.split('') -> ('', '')
os.path.split('a/') -> ('a', '')

and ''.lower() is valid (returning '').

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:84: parent_interface = None
nit: you can omit this.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:85: is_audio_buffer = False
unused?

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:87: is_event_target = False
nit: you can omit assigning False.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:206: 'actual_v8_class': v8_class_name +
'Partial' if interface.is_partial else v8_class_name,
nit: alphabetical order. (I'd like to call this |v8_partial_class| though)

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:264: method =
v8_methods.method_context(interface, operation)
How about adding an optional argument |is_visible|, of which default value is
True, to method_context()?
This way, we can write this like below:

methods.extends([v8_methods.method_context(interface, method, is_visible=False)
    for method in interface.original_interface.operations])

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:371: def
update_interface_constructor_context(interface, context):
Question: is there any change except for factoring out as a method? It's
somewhat difficult to check what has been changed without diff..

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:488: # partial overloads breaks this
assumption.
Should we remove or change the assert then?

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:529: overloads_visibles =
set([method.get('visible', True) for method in overloads])
Use method['visible'] instead of get()? It seems that |method| always has
'visible'.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/templa...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/templa...
Source/bindings/templates/interface.cpp:925: {% for method in methods %}
nit: you can merge "for" and "if" like above.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/templa...
File Source/bindings/templates/interface.h (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/templa...
Source/bindings/templates/interface.h:172: {% for method in methods %}
nit: you can merge "for" and "if" block.

{% for method in methods if method.overloads and
method.overloads.has_partial_overloads %}

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/templa...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/templa...
Source/bindings/templates/interface_base.cpp:42: {% for method in methods %}
nit: you can merge "for" and "if".

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/templa...
Source/bindings/templates/interface_base.cpp:161: {% if ( method.overloads and
method.overloads.visible and
nit: remove space after/before open/close parenthesis?

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/templa...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/templa...
Source/bindings/templates/methods.cpp:404: {% if not is_partial and
overloads.has_partial_overloads %}
Question: just checking has_partial_overloads isn't enough?

https://codereview.chromium.org/618373003/diff/1440001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/bindings/main.py (right):

https://codereview.chromium.org/618373003/diff/1440001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/bindings/main.py:246: partial_interface_output_dir = None
Remove? It seems that you don't use |partial_interface_output_dir|.

https://codereview.chromium.org/618373003/diff/1440001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/bindings/main.py:247: idl_partial_interface_compiler =
None
Question: Does this mean that we don't test partial interfaces in modules?

[tasak, 2014-10-15 10:48:13]

Patchset #7 (id:1460001) has been deleted

[tasak, 2014-10-15 11:24:19]

Thank you for reviewing.

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
File Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/618373003/diff/840001/Source/bindings/scripts...
Source/bindings/scripts/idl_definitions.py:278: self.original_interface = None
On 2014/10/15 05:29:22, bashi1 wrote:
> On 2014/10/10 11:43:40, tasak wrote:
> > On 2014/10/10 09:48:11, bashi1 wrote:
> > > This looks strange to me.. My understanding is that idl definitions only
> have
> > > information which come from idl parser. All other properties are set in
this
> > > initializer, but |original_interface| and |partial_interfaces| are not. I
> > think
> > > this implies that there would be a better place to hold such information.
> > 
> > So interface_dependency_resolver updates idl definitions. This looks also
> > strange?
> 
> That means we have two paths; the first path reads an idl file and create
> definitions, the second path updates (or additionary creates) definitions
based
> on dependencies. I feel that it might be clear that if we could explicitly
treat
> these information separately (information come from each path). But I'm not
sure
> it's worth doing, so I'm ok with this.

As far as I know, we can obtain valid IdlDefinition by using only idl_reader,
not idl_parser.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/core/v...
File Source/bindings/core/v8/generated.gyp (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/core/v...
Source/bindings/core/v8/generated.gyp:17:
#'../../../bindings/modules/generated.gypi',
On 2014/10/15 05:29:22, bashi1 wrote:
> Remove?

Done.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
File Source/bindings/scripts/interface_dependency_resolver.py (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/interface_dependency_resolver.py:144: No return:
modifies target_interface in place.
On 2014/10/15 05:29:23, bashi1 wrote:
> Could you update the comment? This method returns a value. Maybe just refer
> resolve_dependencies()?

Done.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/interface_dependency_resolver.py:177:
resolved_definitions[dependency_component].update(dependency_definitions)
On 2014/10/15 05:29:23, bashi1 wrote:
> Don't we need to update |original_interface| and |partial_interfaces|?

In this case, partial interfaces and the original interfaces are defined in the
same component (or allowed components).
So we don't need to generate any special code. We can just merge the interfaces.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/interface_dependency_resolver.py:183:
dependency_interface.extended_attributes.update(extended_attributes)
On 2014/10/15 05:29:22, bashi1 wrote:
> Can't we do below?
> 
>
dependency_interface.extended_attributes.update(target_interface.extended_attributes)
> 
> This doesn't work if there is a conflict, but in such cases, shouldn't we
raise
> an exception?

Done.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
File Source/bindings/scripts/utilities.py (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/utilities.py:34: if not basename:
On 2014/10/15 05:29:23, bashi1 wrote:
> Why is this required? As far as I tested,
> 
> os.path.split('') -> ('', '')
> os.path.split('a/') -> ('a', '')
> 
> and ''.lower() is valid (returning '').

os.path.split('/') => ('/', '')
A given path is "realpath". So it always starts with '/' (linux and macosx).

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:85: is_audio_buffer = False
On 2014/10/15 05:29:23, bashi1 wrote:
> unused?

Done.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:87: is_event_target = False
On 2014/10/15 05:29:23, bashi1 wrote:
> nit: you can omit assigning False.

I saw the following errors:
    'is_document': is_document,
UnboundLocalError: local variable 'is_document' referenced before assignment
    'is_event_target': is_event_target,
UnboundLocalError: local variable 'is_event_target' referenced before assignment
    'parent_interface': parent_interface,
UnboundLocalError: local variable 'parent_interface' referenced before
assignment

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:206: 'actual_v8_class': v8_class_name +
'Partial' if interface.is_partial else v8_class_name,
On 2014/10/15 05:29:23, bashi1 wrote:
> nit: alphabetical order. (I'd like to call this |v8_partial_class| though)

Done.
I would like to call. However, if a given interface is not partial, we can use
|v8_partial_class| in jinja templates?

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:264: method =
v8_methods.method_context(interface, operation)
On 2014/10/15 05:29:23, bashi1 wrote:
> How about adding an optional argument |is_visible|, of which default value is
> True, to method_context()?
> This way, we can write this like below:
> 
> methods.extends([v8_methods.method_context(interface, method,
is_visible=False)
>     for method in interface.original_interface.operations])

Done.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:371: def
update_interface_constructor_context(interface, context):
On 2014/10/15 05:29:23, bashi1 wrote:
> Question: is there any change except for factoring out as a method? It's
> somewhat difficult to check what has been changed without diff..

Done.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:488: # partial overloads breaks this
assumption.
On 2014/10/15 05:29:23, bashi1 wrote:
> Should we remove or change the assert then?

Removed the comment. This assert is correct now.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:529: overloads_visibles =
set([method.get('visible', True) for method in overloads])
On 2014/10/15 05:29:23, bashi1 wrote:
> Use method['visible'] instead of get()? It seems that |method| always has
> 'visible'.

|method| always has. However, |constructor| never has.

constructors = [constructor_context ...]
if len(constructors) > 1:
        context['constructor_overloads'] = overloads_context(interface,
constructors)

So we should modify constructor_context?

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/templa...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/templa...
Source/bindings/templates/interface.cpp:925: {% for method in methods %}
On 2014/10/15 05:29:23, bashi1 wrote:
> nit: you can merge "for" and "if" like above.

Done.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/templa...
File Source/bindings/templates/interface.h (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/templa...
Source/bindings/templates/interface.h:172: {% for method in methods %}
On 2014/10/15 05:29:23, bashi1 wrote:
> nit: you can merge "for" and "if" block.
> 
> {% for method in methods if method.overloads and
> method.overloads.has_partial_overloads %}

Done.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/templa...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/templa...
Source/bindings/templates/interface_base.cpp:42: {% for method in methods %}
On 2014/10/15 05:29:23, bashi1 wrote:
> nit: you can merge "for" and "if".

Done.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/templa...
Source/bindings/templates/interface_base.cpp:161: {% if ( method.overloads and
method.overloads.visible and
On 2014/10/15 05:29:23, bashi1 wrote:
> nit: remove space after/before open/close parenthesis? 

Done.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/templa...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/templa...
Source/bindings/templates/methods.cpp:404: {% if not is_partial and
overloads.has_partial_overloads %}
On 2014/10/15 05:29:23, bashi1 wrote:
> Question: just checking has_partial_overloads isn't enough?

Partial interfaces also need to know whether a given method is overloaded
between the interfaces and the original interfaces or not.
So has_partial_overloads can be true when is_partial is true.

https://codereview.chromium.org/618373003/diff/1440001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/bindings/main.py (right):

https://codereview.chromium.org/618373003/diff/1440001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/bindings/main.py:246: partial_interface_output_dir = None
On 2014/10/15 05:29:23, bashi1 wrote:
> Remove? It seems that you don't use |partial_interface_output_dir|.

Done.

https://codereview.chromium.org/618373003/diff/1440001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/bindings/main.py:247: idl_partial_interface_compiler =
None
On 2014/10/15 05:29:23, bashi1 wrote:
> Question: Does this mean that we don't test partial interfaces in modules?

If partial interfaces are defined in modules and the original interfaces are
also defined in modules, the partial interfaces are merged into the original
interfaces.
So we don't see any partial interfaces. We can do this because of no
core/modules violation.

[haraken, 2014-10-16 04:24:09]

LGTM

The following comments are just nits or things to be addressed in follow-up CLs.

https://codereview.chromium.org/618373003/diff/40001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/618373003/diff/40001/Source/core/core.gypi#ne...
Source/core/core.gypi:450: 'core_modules_dependency_idl_files': [
On 2014/10/10 07:52:24, tasak wrote:
> On 2014/10/09 04:24:02, haraken wrote:
> > 
> > core_idl_files_with_modules_dependency ?
> > 
> > What happens if we write "an IDL file that should be in
> > core_idl_files_with_modules_dependency" in core_idl_files? Will it lead to a
> > compile error?
> > 
> > I'm fine with the approach of statically listing IDL files in GYP files, but
> we
> > should have a way to detect an error if a developer writes an IDL file in a
> > wrong target.
> 
> If we add an idl file with modules dependency to code_idl_files, we will see
no
> compile error and will see layout test regression (e.g. undefined XXX).
> 
> I think, presubmit check or bindings tests will help us to detect such errors.
> 
> 

I think we need to have a verification for this. Probably the IDL compiler can
do the verification because the IDL compiler knows what interface is depended by
what interfaces. You can do it in a follow-up.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/core/i...
File Source/bindings/core/idl.gypi (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/core/i...
Source/bindings/core/idl.gypi:22:
'<@(webcore_testing_idl_with_modules_dependency_files)',

Nit: Let's rename webcore to core in a follow-up.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/module...
File Source/bindings/modules/v8/ModuleBindingsInitializer.cpp (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/module...
Source/bindings/modules/v8/ModuleBindingsInitializer.cpp:17: void
initModulesPartialInterfaces();

initModulesPartialInterfaces() => initModulesForPartialInterfaces() (since the
file name is InitModulesForPartialInterfaces.cpp)

Slightly better: initPartialInterfacesInModules

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
File Source/bindings/scripts/code_generator_v8.py (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/code_generator_v8.py:95: if not
is_valid_component_dependency(component, dependency):

Shouldn't we raise an exception in this case?

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/code_generator_v8.py:177: definition_name =
''.join([interface_name, 'Partial'])

I'd just overwrite interface_name (instead of introducing definition_name), just
like you're overwriting component below.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
File Source/bindings/scripts/compute_interfaces_info_overall.py (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/compute_interfaces_info_overall.py:74: paths to
dependencies caused by partial (for merging into main)

partial => partial interfaces

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/compute_interfaces_info_overall.py:76: paths for use in
C++ #include directives for partial interface

partial interface => partial interfaces

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/compute_interfaces_info_overall.py:291:
partial_interface_dependencies_full_paths.append(full_path)

This naming is a bit confusing because some partial interface dependencies are
stored into dependencies_full_paths.

Shall we rename them to:

- full_paths
- other_component_full_paths

?

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/compute_interfaces_info_overall.py:298:
partial_interface_dependencies_include_paths.append(include_path)

Ditto.

- include_paths
- other_component_include_paths

?

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
File Source/bindings/scripts/generate_init_partial_interfaces.py (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/generate_init_partial_interfaces.py:6: """Generate
initialize .cpp file for modules' partial interfaces of core interfaces.

Generate initPartialInterfacesInModules(), which registers partial interfaces in
modules to core interfaces.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/generate_init_partial_interfaces.py:20: _COPYRIGHT =
"""// Copyright 2014 The Chromium Authors. All rights reserved.

Consider using a jinja template for this. You can do it in a follow-up.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/generate_init_partial_interfaces.py:48: # ensure output
comes last, so command line easy to parse via regexes

Is this comment still valid?

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/generate_init_partial_interfaces.py:55:
parser.error('Must specify idl files list file using --idl-files-list.')

Must specify a list of IDL files using --idl-files-list.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
File Source/bindings/scripts/idl_compiler.py (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/idl_compiler.py:55:
parser.add_option('--target-component',

We should always explicitly specify --target-component and remove the default
behavior. Shall we add a FIXME and address it in a follow-up?

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
File Source/bindings/scripts/interface_dependency_resolver.py (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/interface_dependency_resolver.py:127: for
referenced_component in referenced_definitions:

I'd rename all "referenced" to "dependent", but you can do this in a follow-up.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/interface_dependency_resolver.py:213: if
'NoInterfaceObject' not in dependency_interface.extended_attributes:

You can merge the two if branches.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
File Source/bindings/scripts/utilities.py (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/utilities.py:42: # See whether "component" can depend on
"referred_component" or not:

referred => dependency ?

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
File Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/v8_attributes.py:440:
namespace_name.append('V8Internal')

Can we move actual_cpp_class and remove this code?

(Probably you need to define a helper method to get actual_cpp_class in
utilities.py.)

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:167: 'actual_v8_class': v8_class_name +
'Partial' if interface.is_partial else v8_class_name,

"actual_" wouldn't be a good name...

I don't have a better idea, but "maybe_partial_cpp_class" seems slightly better?

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:523: has_partial_overloads = False

Discussed offline and I understood what this code is doing.

Let's have a comment and make the code better.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
Source/bindings/templates/attributes.cpp:440: void
{{actual_v8_class}}::installConditionallyEnabledProperties(v8::Handle<v8::Object>
instanceObject, v8::Isolate* isolate)

BTW, do we need to support installConditionallyEnabledProperties for partial
interfaces? I think installConditionallyEnabledProperties is just for custom
elements and won't be needed in partial interfaces. If that's the case, we can
remove the support from this CL. (It's important to reduce the complexity.)

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
Source/bindings/templates/interface_base.cpp:28: {% set wrapper_type_info_type =
'' if has_partial_interface else 'const ' %}

wrapper_type_info_type => wrapper_type_info_const ?

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
Source/bindings/templates/interface_base.cpp:155: {# If
overloads_callback_visible_in_partial is True, method callback #}

What is overloads_callback_visible_in_partial?

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
Source/bindings/templates/interface_base.cpp:161: (not method.overloads and
method.visible) %}

Discussed offline and I finally understood what this condition means. Let's add
more comments. In particular, I'd like to have a very good comment about when
method.visible, method.overloads.visible, method.overloads.has_partial_overloads
etc become true.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
Source/bindings/templates/interface_base.cpp:413: {% if not is_partial %}

Do we need this check?

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
Source/bindings/templates/interface_base.cpp:443: {% if is_partial or
has_conditional_attributes %}

Not related to your CL, |has_conditional_attributes| should be renamed to
|conditionally_enabled_properties| (for consistency with
|conditionally_enabled_methods|).

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
File Source/bindings/templates/partial_interface.h (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
Source/bindings/templates/partial_interface.h:10: {% filter
conditional(conditional_string) %}

I think you can just drop this (You don't need to introduce new [Conditional]
support).

The same comment for the 'filter conditional' in this file. Let's keep the IDL
compiler as simple as possible.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
Source/bindings/templates/partial_interface.h:37: {% if
has_custom_legacy_call_as_function %}

And remove this. We won't add new [LegacyCallCustom] in the future.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/tests/...
File Source/bindings/tests/results/core/V8TestInterface.cpp (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/tests/...
Source/bindings/tests/results/core/V8TestInterface.cpp:1425: default:

Do we need this default block?

I'm fine with having the default block if it simplifies the IDL compiler.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/tests/...
Source/bindings/tests/results/core/V8TestInterface.cpp:1429:
(voidMethodPartialOverloadMethodForPartialInterface)(info);

Can we add:

  ASSERT(voidMethodPartialOverloadMethodForPartialInterface);

[bashi, 2014-10-17 02:23:24]

code generator stuff LGTM. (I didn't take closer look at overload resolution and
domTemplate stuff, as I'm not familiar with them)

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
File Source/bindings/scripts/interface_dependency_resolver.py (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/interface_dependency_resolver.py:177:
resolved_definitions[dependency_component].update(dependency_definitions)
On 2014/10/15 11:24:18, tasak wrote:
> On 2014/10/15 05:29:23, bashi1 wrote:
> > Don't we need to update |original_interface| and |partial_interfaces|?
> 
> In this case, partial interfaces and the original interfaces are defined in
the
> same component (or allowed components).
> So we don't need to generate any special code. We can just merge the
interfaces.

Acknowledged.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
File Source/bindings/scripts/utilities.py (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/utilities.py:34: if not basename:
On 2014/10/15 11:24:18, tasak wrote:
> On 2014/10/15 05:29:23, bashi1 wrote:
> > Why is this required? As far as I tested,
> > 
> > os.path.split('') -> ('', '')
> > os.path.split('a/') -> ('a', '')
> > 
> > and ''.lower() is valid (returning '').
> 
> os.path.split('/') => ('/', '')
> A given path is "realpath". So it always starts with '/' (linux and macosx).
> 

Then, "while path:" looks wrong to me... But it's a subtle issue and I'm OK with
this.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:87: is_event_target = False
On 2014/10/15 11:24:18, tasak wrote:
> On 2014/10/15 05:29:23, bashi1 wrote:
> > nit: you can omit assigning False.
> 
> I saw the following errors:
>     'is_document': is_document,
> UnboundLocalError: local variable 'is_document' referenced before assignment
>     'is_event_target': is_event_target,
> UnboundLocalError: local variable 'is_event_target' referenced before
assignment
>     'parent_interface': parent_interface,
> UnboundLocalError: local variable 'parent_interface' referenced before
> assignment

BTW, shouldn't we raise an exception rather than assigning false?

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:206: 'actual_v8_class': v8_class_name +
'Partial' if interface.is_partial else v8_class_name,
On 2014/10/15 11:24:18, tasak wrote:
> On 2014/10/15 05:29:23, bashi1 wrote:
> > nit: alphabetical order. (I'd like to call this |v8_partial_class| though)
> 
> Done.
> I would like to call. However, if a given interface is not partial, we can use
> |v8_partial_class| in jinja templates?

Acknowledged.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:529: overloads_visibles =
set([method.get('visible', True) for method in overloads])
On 2014/10/15 11:24:19, tasak wrote:
> On 2014/10/15 05:29:23, bashi1 wrote:
> > Use method['visible'] instead of get()? It seems that |method| always has
> > 'visible'.
> 
> |method| always has. However, |constructor| never has.
> 
> constructors = [constructor_context ...]
> if len(constructors) > 1:
>         context['constructor_overloads'] = overloads_context(interface,
> constructors)
> 
> So we should modify constructor_context?

It will help make code simple, IMO.

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/templa...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/618373003/diff/1440001/Source/bindings/templa...
Source/bindings/templates/methods.cpp:404: {% if not is_partial and
overloads.has_partial_overloads %}
On 2014/10/15 11:24:19, tasak wrote:
> On 2014/10/15 05:29:23, bashi1 wrote:
> > Question: just checking has_partial_overloads isn't enough?
> 
> Partial interfaces also need to know whether a given method is overloaded
> between the interfaces and the original interfaces or not.
> So has_partial_overloads can be true when is_partial is true.

Acknowledged.

[bashi, 2014-10-17 02:25:09]

I think you need to write GN targets.

[tasak, 2014-10-17 07:24:18]

Patchset #8 (id:1500001) has been deleted

[tasak, 2014-10-17 07:38:19]

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/module...
File Source/bindings/modules/v8/ModuleBindingsInitializer.cpp (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/module...
Source/bindings/modules/v8/ModuleBindingsInitializer.cpp:17: void
initModulesPartialInterfaces();
On 2014/10/16 04:24:07, haraken wrote:
> 
> initModulesPartialInterfaces() => initModulesForPartialInterfaces() (since the
> file name is InitModulesForPartialInterfaces.cpp)
> 
> Slightly better: initPartialInterfacesInModules

Done.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/module...
File Source/bindings/modules/v8/generated.gyp (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/module...
Source/bindings/modules/v8/generated.gyp:267:
'<(bindings_modules_v8_output_dir)/InitModulesForPartialInterfaces.cpp',
Renamed.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
File Source/bindings/scripts/code_generator_v8.py (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/code_generator_v8.py:95: if not
is_valid_component_dependency(component, dependency):
On 2014/10/16 04:24:07, haraken wrote:
> 
> Shouldn't we raise an exception in this case?

Changed. Now assert, because compute_interfaces_info_overall considers
dependency.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/code_generator_v8.py:177: definition_name =
''.join([interface_name, 'Partial'])
On 2014/10/16 04:24:07, haraken wrote:
> 
> I'd just overwrite interface_name (instead of introducing definition_name),
just
> like you're overwriting component below.

Done.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
File Source/bindings/scripts/compute_interfaces_info_overall.py (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/compute_interfaces_info_overall.py:69:
'implemented_interfaces_full_paths':
Need to fix this comment.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/compute_interfaces_info_overall.py:71:
'implemented_interfaces_include_paths':
Ditto.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/compute_interfaces_info_overall.py:74: paths to
dependencies caused by partial (for merging into main)
On 2014/10/16 04:24:07, haraken wrote:
> 
> partial => partial interfaces

Done.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/compute_interfaces_info_overall.py:76: paths for use in
C++ #include directives for partial interface
On 2014/10/16 04:24:07, haraken wrote:
> 
> partial interface => partial interfaces

Done.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/compute_interfaces_info_overall.py:291:
partial_interface_dependencies_full_paths.append(full_path)
On 2014/10/16 04:24:07, haraken wrote:
> 
> This naming is a bit confusing because some partial interface dependencies are
> stored into dependencies_full_paths.
> 
> Shall we rename them to:
> 
> - full_paths
> - other_component_full_paths
> 
> ?

Done.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/compute_interfaces_info_overall.py:298:
partial_interface_dependencies_include_paths.append(include_path)
On 2014/10/16 04:24:07, haraken wrote:
> 
> Ditto.
> 
> - include_paths
> - other_component_include_paths
> 
> ?

Done.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
File Source/bindings/scripts/generate_init_partial_interfaces.py (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/generate_init_partial_interfaces.py:6: """Generate
initialize .cpp file for modules' partial interfaces of core interfaces.
On 2014/10/16 04:24:08, haraken wrote:
> 
> Generate initPartialInterfacesInModules(), which registers partial interfaces
in
> modules to core interfaces.

Done.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/generate_init_partial_interfaces.py:20: _COPYRIGHT =
"""// Copyright 2014 The Chromium Authors. All rights reserved.
On 2014/10/16 04:24:08, haraken wrote:
> 
> Consider using a jinja template for this. You can do it in a follow-up.

I will try this in another CL.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/generate_init_partial_interfaces.py:48: # ensure output
comes last, so command line easy to parse via regexes
On 2014/10/16 04:24:08, haraken wrote:
> 
> Is this comment still valid?

Done.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/generate_init_partial_interfaces.py:55:
parser.error('Must specify idl files list file using --idl-files-list.')
On 2014/10/16 04:24:07, haraken wrote:
> 
> Must specify a list of IDL files using --idl-files-list.

Done.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
File Source/bindings/scripts/idl_compiler.py (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/idl_compiler.py:55:
parser.add_option('--target-component',
On 2014/10/16 04:24:08, haraken wrote:
> 
> We should always explicitly specify --target-component and remove the default
> behavior. Shall we add a FIXME and address it in a follow-up?

Done.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
File Source/bindings/scripts/interface_dependency_resolver.py (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/interface_dependency_resolver.py:127: for
referenced_component in referenced_definitions:
On 2014/10/16 04:24:08, haraken wrote:
> 
> I'd rename all "referenced" to "dependent", but you can do this in a
follow-up.

I will do this in a follow-up.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/interface_dependency_resolver.py:213: if
'NoInterfaceObject' not in dependency_interface.extended_attributes:
On 2014/10/16 04:24:08, haraken wrote:
> 
> You can merge the two if branches.

Done.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
File Source/bindings/scripts/utilities.py (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/utilities.py:42: # See whether "component" can depend on
"referred_component" or not:
On 2014/10/16 04:24:08, haraken wrote:
> 
> referred => dependency ?

Done.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
File Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/v8_attributes.py:440:
namespace_name.append('V8Internal')
On 2014/10/16 04:24:08, haraken wrote:
> 
> Can we move actual_cpp_class and remove this code?
> 
> (Probably you need to define a helper method to get actual_cpp_class in
> utilities.py.)

Done.
Added cpp_name_or_partial().

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:167: 'actual_v8_class': v8_class_name +
'Partial' if interface.is_partial else v8_class_name,
On 2014/10/16 04:24:08, haraken wrote:
> 
> "actual_" wouldn't be a good name...
> 
> I don't have a better idea, but "maybe_partial_cpp_class" seems slightly
better?

actual_ => _or_partial

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:523: has_partial_overloads = False
On 2014/10/16 04:24:08, haraken wrote:
> 
> Discussed offline and I understood what this code is doing.
> 
> Let's have a comment and make the code better.
> 

Done.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
Source/bindings/templates/attributes.cpp:440: void
{{actual_v8_class}}::installConditionallyEnabledProperties(v8::Handle<v8::Object>
instanceObject, v8::Isolate* isolate)
On 2014/10/16 04:24:08, haraken wrote:
> 
> BTW, do we need to support installConditionallyEnabledProperties for partial
> interfaces? I think installConditionallyEnabledProperties is just for custom
> elements and won't be needed in partial interfaces. If that's the case, we can
> remove the support from this CL. (It's important to reduce the complexity.)

Done.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
Source/bindings/templates/interface_base.cpp:28: {% set wrapper_type_info_type =
'' if has_partial_interface else 'const ' %}
On 2014/10/16 04:24:08, haraken wrote:
> 
> wrapper_type_info_type => wrapper_type_info_const ?

Done.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
Source/bindings/templates/interface_base.cpp:155: {# If
overloads_callback_visible_in_partial is True, method callback #}
On 2014/10/16 04:24:08, haraken wrote:
> 
> What is overloads_callback_visible_in_partial?

Removed.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
Source/bindings/templates/interface_base.cpp:161: (not method.overloads and
method.visible) %}
On 2014/10/16 04:24:08, haraken wrote:
> 
> Discussed offline and I finally understood what this condition means. Let's
add
> more comments. In particular, I'd like to have a very good comment about when
> method.visible, method.overloads.visible,
method.overloads.has_partial_overloads
> etc become true.
> 

https://docs.google.com/document/d/1qBC7Therp437Jbt_QYAtNYMZs6zQ_7_tnMkNUG_AC...

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
Source/bindings/templates/interface_base.cpp:413: {% if not is_partial %}
On 2014/10/16 04:24:08, haraken wrote:
> 
> Do we need this check?

Yes, because interface_name is 'Window' when generating V8WindowPartial.cpp.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
Source/bindings/templates/interface_base.cpp:443: {% if is_partial or
has_conditional_attributes %}
On 2014/10/16 04:24:08, haraken wrote:
> 
> Not related to your CL, |has_conditional_attributes| should be renamed to
> |conditionally_enabled_properties| (for consistency with
> |conditionally_enabled_methods|).

I will do in another CL.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
File Source/bindings/templates/partial_interface.h (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
Source/bindings/templates/partial_interface.h:10: {% filter
conditional(conditional_string) %}
On 2014/10/16 04:24:08, haraken wrote:
> 
> I think you can just drop this (You don't need to introduce new [Conditional]
> support).
> 
> The same comment for the 'filter conditional' in this file. Let's keep the IDL
> compiler as simple as possible.

Done.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
Source/bindings/templates/partial_interface.h:37: {% if
has_custom_legacy_call_as_function %}
On 2014/10/16 04:24:08, haraken wrote:
> 
> And remove this. We won't add new [LegacyCallCustom] in the future.

Done.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/templa...
Source/bindings/templates/partial_interface.h:41: static void
installConditionallyEnabledProperties(v8::Handle<v8::Object>, v8::Isolate*);
Removed installConditionallyEnabledProperties.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/tests/...
File Source/bindings/tests/idls/modules/TestPartialInterface3.idl (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/tests/...
Source/bindings/tests/idls/modules/TestPartialInterface3.idl:38: static
attribute long partial3StaticLongAttribute;
Removed, because we don't allow conditionally enabled properties in
V8TestInterfacePartial.{h,cpp}.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/tests/...
File Source/bindings/tests/results/core/V8TestInterface.cpp (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/tests/...
Source/bindings/tests/results/core/V8TestInterface.cpp:1425: default:
On 2014/10/16 04:24:09, haraken wrote:
> 
> Do we need this default block?
> 
> I'm fine with having the default block if it simplifies the IDL compiler.

Done.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/tests/...
Source/bindings/tests/results/core/V8TestInterface.cpp:1429:
(voidMethodPartialOverloadMethodForPartialInterface)(info);
On 2014/10/16 04:24:09, haraken wrote:
> 
> Can we add:
> 
>   ASSERT(voidMethodPartialOverloadMethodForPartialInterface);

Done.

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/tests/...
File Source/bindings/tests/results/modules/V8TestInterfacePartial.h (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/bindings/tests/...
Source/bindings/tests/results/modules/V8TestInterfacePartial.h:10: #if
ENABLE(CONDITION)
Removed, because of removing "Conditional" from partial_interface.h.

https://codereview.chromium.org/618373003/diff/1480001/Source/modules/modules...
File Source/modules/modules.gyp (right):

https://codereview.chromium.org/618373003/diff/1480001/Source/modules/modules...
Source/modules/modules.gyp:57:
'<(bindings_modules_v8_output_dir)/InitModulesForPartialInterfaces.cpp',
Renamed: InitModulesForPartialInterfaces => initPartialInterfacesInModules

https://codereview.chromium.org/618373003/diff/1520001/Source/bindings/script...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/618373003/diff/1520001/Source/bindings/script...
Source/bindings/scripts/v8_interface.py:288: if has_conditional_attributes and
interface.is_partial:
Now raise an exception when partial code has conditionally enabled properties.

[tasak, 2014-10-20 10:37:34]

tasak@google.com changed reviewers:
+ brettw@chromium.org

[tasak, 2014-10-20 10:37:35]

brettw, would you review GN-related changes in this CL?

[brettw, 2014-10-20 16:46:25]

I don't see anything weird in the .gn files. rubberstamp LGTM

[tasak, 2014-10-21 05:10:43]

tasak@google.com changed reviewers:
+ tkent@chromium.org

[tasak, 2014-10-21 05:10:44]

tkent, would you review public/ changes?

[tasak, 2014-10-21 05:11:13]

On 2014/10/20 16:46:25, brettw wrote:
> I don't see anything weird in the .gn files. rubberstamp LGTM

Thank you.

[tasak, 2014-10-21 07:22:14]

On 2014/10/21 05:10:44, tasak wrote:
> tkent, would you review public/ changes?

Thank you for comments.

So mac_gpu passes without #undef ceil.
You are right.

We don't need "#undef ceil" in WebRect.h.

[tasak, 2014-10-21 08:13:20]

The CQ bit was checked by tasak@google.com

[commit-bot: I haz the power, 2014-10-21 08:14:01]

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

[commit-bot: I haz the power, 2014-10-21 09:32:10]

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

[commit-bot: I haz the power, 2014-10-21 09:32:11]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/32765)

[tasak, 2014-10-21 09:37:34]

The CQ bit was checked by tasak@google.com

[commit-bot: I haz the power, 2014-10-21 09:38:48]

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

[commit-bot: I haz the power, 2014-10-21 10:26:26]

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

[commit-bot: I haz the power, 2014-10-21 10:26:27]

Failed to apply patch for Source/bindings/templates/interface.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/bindings/templates/interface.h
  Hunk #1 FAILED at 54.
  1 out of 2 hunks FAILED -- saving rejects to file
Source/bindings/templates/interface.h.rej

Patch:       Source/bindings/templates/interface.h
Index: Source/bindings/templates/interface.h
diff --git a/Source/bindings/templates/interface.h
b/Source/bindings/templates/interface.h
index
7f4ecbad4c273b36cf89f2b382eb6bde8d6e23db..ddd64c7a84aa0a65296aa2694dd7aa50f89e873f
100644
--- a/Source/bindings/templates/interface.h
+++ b/Source/bindings/templates/interface.h
@@ -54,7 +54,11 @@ public:
     }
     {% endif %}
     static {{cpp_class}}* toImplWithTypeCheck(v8::Isolate*,
v8::Handle<v8::Value>);
+    {% if has_partial_interface %}
+    static WrapperTypeInfo wrapperTypeInfo;
+    {% else %}
     static const WrapperTypeInfo wrapperTypeInfo;
+    {% endif %}
     static void refObject(ScriptWrappableBase* internalPointer);
     static void derefObject(ScriptWrappableBase* internalPointer);
     static void trace(Visitor* visitor, ScriptWrappableBase* internalPointer)
@@ -163,12 +167,24 @@ public:
     static void installConditionallyEnabledMethods(v8::Handle<v8::Object>,
v8::Isolate*){% if conditionally_enabled_methods %};
     {% else %} { }
     {% endif %}
+    {% if has_partial_interface %}
+    static void updateWrapperTypeInfo(InstallTemplateFunction,
InstallConditionallyEnabledMethodsFunction);
+    static void install{{v8_class}}Template(v8::Handle<v8::FunctionTemplate>,
v8::Isolate*);
+    {% for method in methods if method.overloads and
method.overloads.has_partial_overloads %}
+    static void register{{method.name |
blink_capitalize}}MethodForPartialInterface(void (*)(const
v8::FunctionCallbackInfo<v8::Value>&));
+    {% endfor %}
+    {% endif %}
     {% if not has_custom_to_v8 and not is_script_wrappable %}
 
 private:
     friend v8::Handle<v8::Object> wrap({{cpp_class}}*, v8::Handle<v8::Object>
creationContext, v8::Isolate*);
     static v8::Handle<v8::Object> createWrapper({{pass_cpp_type}},
v8::Handle<v8::Object> creationContext, v8::Isolate*);
     {% endif %}
+    {% if has_partial_interface %}
+
+private:
+    static InstallTemplateFunction install{{v8_class}}TemplateFunction;
+    {% endif %}
 };
 
 {% if has_custom_to_v8 %}

[commit-bot: I haz the power, 2014-10-21 10:27:02]

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

[tasak, 2014-10-21 10:40:43]

The CQ bit was checked by tasak@google.com

[commit-bot: I haz the power, 2014-10-21 10:41:45]

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

[commit-bot: I haz the power, 2014-10-21 13:01:57]

Committed patchset #12 (id:1600001) as 184083