Partial interface should use original interface's cpp_name.

Partial interface should use original interface's cpp_name.

cpp_name in v8_utilities sees ImplementedAs, but ImplementedAs in partial interfaces just says nothing about cpp class name. It just says cpp/header file name.
Instead, we should use original interface's cpp_name for generated partial interface code.

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

[jsbell, 2015-04-01 18:23:45]

jsbell@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2015-04-01 18:23:46]

I can confirm that this CL fixes the problem I was seeing.

[jsbell, 2015-04-01 18:53:56]

I'm still confused about why this was occurring in the output for unrelated
partial interface. The fix seems adequate in that the [ImplementedAs] will be
ignored for partial interfaces in these cases, but why was it even relevant? Was
the [ImplementedAs] being merged across all partials or something?

[tasak, 2015-04-02 06:00:27]

On 2015/04/01 18:23:46, jsbell wrote:
> I can confirm that this CL fixes the problem I was seeing.

I've already confirmed that this CL fixes
https://codereview.chromium.org/1041183004.

[tasak, 2015-04-02 08:55:02]

On 2015/04/01 18:53:56, jsbell wrote:
> I'm still confused about why this was occurring in the output for unrelated
> partial interface. The fix seems adequate in that the [ImplementedAs] will be
> ignored for partial interfaces in these cases, but why was it even relevant?
Was
> the [ImplementedAs] being merged across all partials or something?

I've just finished investigating how code generator works (related to this
patch).

‘ImplementedAs’ in partial interfaces are copied to methods, i.e.
‘PartialInterfaceImplementedAs’ and ‘ImplementedAs’ should be never used for
generating partial interface code.

(1) code generator merges all partial interfaces into their original interface.
However, if partial interfaces are defined in modules and the original interface
is in core, partial interfaces are merged into 1 partial interface (for
modules).

   When compiling idl files,
   (a) 1 interface in core,
   (b) 1 interface in modules, or
   (c) 1 interface in core and 1 partial interface in modules,

   For example, when applying https://codereview.chromium.org/1041183004’s
patch, (c) matches. v8_interface.py obtains 1 partial interface
WorkerGlobalScope, i.e. GlobalC, for modules:
    The GlobalC interface has the following methods:
       fetch(...);
       webkitRequestFileSystem(...);
       webkitRequestFileSystemSync(...);
       webkitResolveLocalFileSystemURL(...);
       webkitResolveLocalFileSystemSyncURL(...);

(2) When merging partial interfaces, extended_attributes are also merged.

     This depends on the order of partial interfaces. The first one, i.e.
GlobalC, wins. Other partial interfaces are merged into the first one.

(3) In v8_interface.py and interface{_base}.cpp, GlobalC (i.e. ImpelementedAs
specified in partial interfaces) is never used. So {{cpp_name}} should be the
original interface's one. e.g. cpp_name should be WorkerGlobalScope, not
GlobalC.

(4) v8_method.py uses GlobalC to invoke GlobalC's static method. The information
is stored in each method’s extended_attribute, i.e.
PartialInterfaceImplementedAs.

    So IDL compiler can generate code like “GlobalC::someMethod();”.

[tasak, 2015-04-02 08:59:18]

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

[tasak, 2015-04-02 08:59:18]

+haraken, bashi

[haraken, 2015-04-02 09:30:58]

I'd delegate the review to bashi-san.

My proposal is just to remove [ImplementedAs] from partial interfaces (because
there would be no need to let a partial interface customize the implementation
name; if the implementation name is XXX.h, then we can just name the file
XXX.idl instead of using [ImplementedAs=XXX] in YYY.idl).

I'm fine with landing this CL as a short-term fix though.

[jsbell, 2015-04-02 16:39:46]

On 2015/04/02 06:00:27, tasak wrote:
> On 2015/04/01 18:23:46, jsbell wrote:
> > I can confirm that this CL fixes the problem I was seeing.
> 
> I've already confirmed that this CL fixes
> https://codereview.chromium.org/1041183004.

Poor wording on my part. I should have written "I *did* confirm..." (and with
the real CL I was working on). Thanks for taking this on, and your thorough
investigation!

On 2015/04/02 09:30:58, haraken wrote:
> My proposal is just to remove [ImplementedAs] from partial interfaces (because
> there would be no need to let a partial interface customize the implementation
> name; if the implementation name is XXX.h, then we can just name the file
> XXX.idl instead of using [ImplementedAs=XXX] in YYY.idl).

One of the patterns that horo-san introduced for modules/fetch (and I copied for
modules/cachestorage) is a single GlobalXXX.h/cpp that handles both
WindowXXX.idl and WorkerXXX.idl - hence the use of [ImplementedAs]

I haven't tried... is it possible to define multiple partial interfaces in
single file? (i.e. GlobalXXX.idl)

> I'm fine with landing this CL as a short-term fix though.

Agreed! Grep shows that there are many more [ImplementedAs] partials than I was
expecting.

[jsbell, 2015-04-02 17:49:29]

oh, and: lgtm

[bashi, 2015-04-02 23:15:27]

LGTM as a short-term fix.

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

https://codereview.chromium.org/1057443002/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/interface_dependency_resolver.py:188:
dependency_interface.extended_attributes.pop('ImplementedAs', None)
Without having comment, it's difficult to understand why we drop ImplementedAs.
Could you add a comment and put a TODO?

[haraken, 2015-04-02 23:32:50]

LGTM with adding the comment.

[tasak, 2015-04-03 05:22:53]

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

https://codereview.chromium.org/1057443002/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/interface_dependency_resolver.py:188:
dependency_interface.extended_attributes.pop('ImplementedAs', None)
On 2015/04/02 23:15:27, bashi1 wrote:
> Without having comment, it's difficult to understand why we drop
ImplementedAs.
> Could you add a comment and put a TODO?

Done.

[bashi, 2015-04-03 07:31:48]

On 2015/04/03 05:22:53, tasak wrote:
>
https://codereview.chromium.org/1057443002/diff/20001/Source/bindings/scripts...
> File Source/bindings/scripts/interface_dependency_resolver.py (right):
> 
>
https://codereview.chromium.org/1057443002/diff/20001/Source/bindings/scripts...
> Source/bindings/scripts/interface_dependency_resolver.py:188:
> dependency_interface.extended_attributes.pop('ImplementedAs', None)
> On 2015/04/02 23:15:27, bashi1 wrote:
> > Without having comment, it's difficult to understand why we drop
> ImplementedAs.
> > Could you add a comment and put a TODO?
> 
> Done.

LGTM. Chatted offline and we should refactor this code as a part of core/modules
componentization.

[tasak, 2015-04-03 07:32:26]

The CQ bit was checked by tasak@google.com

[tasak, 2015-04-03 07:32:26]

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

[tasak, 2015-04-03 07:32:37]

On 2015/04/03 07:31:48, bashi1 wrote:
> On 2015/04/03 05:22:53, tasak wrote:
> >
>
https://codereview.chromium.org/1057443002/diff/20001/Source/bindings/scripts...
> > File Source/bindings/scripts/interface_dependency_resolver.py (right):
> > 
> >
>
https://codereview.chromium.org/1057443002/diff/20001/Source/bindings/scripts...
> > Source/bindings/scripts/interface_dependency_resolver.py:188:
> > dependency_interface.extended_attributes.pop('ImplementedAs', None)
> > On 2015/04/02 23:15:27, bashi1 wrote:
> > > Without having comment, it's difficult to understand why we drop
> > ImplementedAs.
> > > Could you add a comment and put a TODO?
> > 
> > Done.
> 
> LGTM. Chatted offline and we should refactor this code as a part of
core/modules
> componentization.

Thank you.

[commit-bot: I haz the power, 2015-04-03 07:32:51]

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

[commit-bot: I haz the power, 2015-04-03 13:36:57]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193116