Move modules-dependent IDL statements out of core by supporting 'implements' in RHS interface

IDLs in core have 'implements' statements that depend on interfaces in modules, i.e.:
CoreFoo implements ModulesBar;

To avoid layer violation, these statements are moved to the implemented (RHS) IDL files.
The code generator is updated to support this.

BUG=358074
BUG=360435
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173599

[Inactive, 2014-05-07 20:00:27]

I'd like haraken to give his opinion on this as I seem to remember he was the
one who asked me to have the 'implements' statements in the destination idls.
However, for the case of module idls, I agree that your change makes sense and I
hope others agree.

https://codereview.chromium.org/270573005/diff/1/Source/bindings/scripts/util...
File Source/bindings/scripts/utilities.py (right):

https://codereview.chromium.org/270573005/diff/1/Source/bindings/scripts/util...
Source/bindings/scripts/utilities.py:82: implements_re = (r'^\s*'
The code duplication with get_implemented_interfaces_from_idl() is not nice,
please try to reduce the existing function instead.

[Inactive, 2014-05-07 20:08:06]

https://codereview.chromium.org/270573005/diff/1/Source/bindings/scripts/comp...
File Source/bindings/scripts/compute_interfaces_info.py (right):

https://codereview.chromium.org/270573005/diff/1/Source/bindings/scripts/comp...
Source/bindings/scripts/compute_interfaces_info.py:194:
source_implements_interfaces = get_source_interfaces_from_idl(idl_file_contents,
interface_name)
Please add a comment to explain why we need to do this.

https://codereview.chromium.org/270573005/diff/1/Source/modules/imagebitmap/I...
File Source/modules/imagebitmap/ImageBitmapFactories.idl (right):

https://codereview.chromium.org/270573005/diff/1/Source/modules/imagebitmap/I...
Source/modules/imagebitmap/ImageBitmapFactories.idl:59: Window implements
ImageBitmapFactories;
We might want to move *all* the 'implements' statements to their source IDL, for
consistency and to avoid handling both cases in the bindings generator.

Don't do this until haraken and others have agreed to this though.

The original reason why we put the 'implements' statements in the destination
IDLs, was because it made it more clear when looking at the IDL of the
destination interface that it was being 'extended'.

However, with partial interfaces, it is already not obvious when you look at
Window.idl (for example) what API is actually supports.
Since 'implements' statements are very similar to partial interfaces (but with
several destination interfaces), I don't think it is an issue to have them in
the source IDL.

[c.shu, 2014-05-07 20:14:29]

https://codereview.chromium.org/270573005/diff/1/Source/modules/imagebitmap/I...
File Source/modules/imagebitmap/ImageBitmapFactories.idl (right):

https://codereview.chromium.org/270573005/diff/1/Source/modules/imagebitmap/I...
Source/modules/imagebitmap/ImageBitmapFactories.idl:59: Window implements
ImageBitmapFactories;
We may have to support both as there could be cases where the source IDL is in
core and destination IDL is in modules. Like partial interface, it's unfortunate
we have to look at least two IDLs to determine the complete interface of a
class. Right?

On 2014/05/07 20:08:07, Chris Dumez wrote:
> We might want to move *all* the 'implements' statements to their source IDL,
for
> consistency and to avoid handling both cases in the bindings generator.
> 
> Don't do this until haraken and others have agreed to this though.
> 
> The original reason why we put the 'implements' statements in the destination
> IDLs, was because it made it more clear when looking at the IDL of the
> destination interface that it was being 'extended'.
> 
> However, with partial interfaces, it is already not obvious when you look at
> Window.idl (for example) what API is actually supports.
> Since 'implements' statements are very similar to partial interfaces (but with
> several destination interfaces), I don't think it is an issue to have them in
> the source IDL.

[Inactive, 2014-05-07 20:21:09]

https://codereview.chromium.org/270573005/diff/1/Source/modules/imagebitmap/I...
File Source/modules/imagebitmap/ImageBitmapFactories.idl (right):

https://codereview.chromium.org/270573005/diff/1/Source/modules/imagebitmap/I...
Source/modules/imagebitmap/ImageBitmapFactories.idl:59: Window implements
ImageBitmapFactories;
On 2014/05/07 20:14:29, c.shu wrote:
> We may have to support both as there could be cases where the source IDL is in
> core and destination IDL is in modules. Like partial interface, it's
unfortunate
> we have to look at least two IDLs to determine the complete interface of a
> class. Right?
> 
> On 2014/05/07 20:08:07, Chris Dumez wrote:
> > We might want to move *all* the 'implements' statements to their source IDL,
> for
> > consistency and to avoid handling both cases in the bindings generator.
> > 
> > Don't do this until haraken and others have agreed to this though.
> > 
> > The original reason why we put the 'implements' statements in the
destination
> > IDLs, was because it made it more clear when looking at the IDL of the
> > destination interface that it was being 'extended'.
> > 
> > However, with partial interfaces, it is already not obvious when you look at
> > Window.idl (for example) what API is actually supports.
> > Since 'implements' statements are very similar to partial interfaces (but
with
> > several destination interfaces), I don't think it is an issue to have them
in
> > the source IDL.
> 

Right, I looked out of curiosity and we have at least one such case:
Source/modules/serviceworkers/ServiceWorker.idl:ServiceWorker implements
AbstractWorker;

[c.shu, 2014-05-07 21:01:49]

Merged two functions and added explanations.

[Inactive, 2014-05-07 21:10:09]

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

https://codereview.chromium.org/270573005/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/compute_interfaces_info.py:175: 
how about something like:
implements_statements = get_implements_from_idl(idl_file_content,
interface_name)
// implements_statements could be a list of 2-element tuples.

https://codereview.chromium.org/270573005/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/compute_interfaces_info.py:179: 'implements_interfaces':
get_right_or_left_interfaces_of_implements_from_idl(idl_file_contents,
interface_name, True),
[v[1] for v in implements_statements]

https://codereview.chromium.org/270573005/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/compute_interfaces_info.py:194:
left_implements_interfaces =
get_right_or_left_interfaces_of_implements_from_idl(idl_file_contents,
interface_name, False)
Gosh, terribly long function name and an obscure boolean argument :(

left_implements_interfaces = [v[0] for v in implements_statements] ?

[c.shu, 2014-05-07 21:42:00]

Addressed reviewer comments.

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

https://codereview.chromium.org/270573005/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/compute_interfaces_info.py:175: 
On 2014/05/07 21:10:09, Chris Dumez wrote:
> how about something like:
> implements_statements = get_implements_from_idl(idl_file_content,
> interface_name)
> // implements_statements could be a list of 2-element tuples.

Done.

https://codereview.chromium.org/270573005/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/compute_interfaces_info.py:179: 'implements_interfaces':
get_right_or_left_interfaces_of_implements_from_idl(idl_file_contents,
interface_name, True),
On 2014/05/07 21:10:09, Chris Dumez wrote:
> [v[1] for v in implements_statements]

Done.

https://codereview.chromium.org/270573005/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/compute_interfaces_info.py:194:
left_implements_interfaces =
get_right_or_left_interfaces_of_implements_from_idl(idl_file_contents,
interface_name, False)
On 2014/05/07 21:10:09, Chris Dumez wrote:
> Gosh, terribly long function name and an obscure boolean argument :(
> 
> left_implements_interfaces = [v[0] for v in implements_statements] ?

Done.

[Inactive, 2014-05-07 21:45:05]

https://codereview.chromium.org/270573005/diff/30001/Source/bindings/scripts/...
File Source/bindings/scripts/compute_interfaces_info.py (right):

https://codereview.chromium.org/270573005/diff/30001/Source/bindings/scripts/...
Source/bindings/scripts/compute_interfaces_info.py:176: [left_interfaces,
right_interfaces] = get_implements_from_idl(idl_file_contents, interface_name)
left_interfaces, right_interfaces = ...

No need for the square brackets AFAIK.

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

https://codereview.chromium.org/270573005/diff/30001/Source/bindings/scripts/...
Source/bindings/scripts/utilities.py:82: return [A_interfaces, B_interfaces]
You should return a tuple here:
return (A_interfaces, B_interfaces)

[c.shu, 2014-05-07 21:52:40]

https://codereview.chromium.org/270573005/diff/30001/Source/bindings/scripts/...
File Source/bindings/scripts/compute_interfaces_info.py (right):

https://codereview.chromium.org/270573005/diff/30001/Source/bindings/scripts/...
Source/bindings/scripts/compute_interfaces_info.py:176: [left_interfaces,
right_interfaces] = get_implements_from_idl(idl_file_contents, interface_name)
On 2014/05/07 21:45:05, Chris Dumez wrote:
> left_interfaces, right_interfaces = ...
> 
> No need for the square brackets AFAIK.

Done.

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

https://codereview.chromium.org/270573005/diff/30001/Source/bindings/scripts/...
Source/bindings/scripts/utilities.py:82: return [A_interfaces, B_interfaces]
On 2014/05/07 21:45:05, Chris Dumez wrote:
> You should return a tuple here:
> return (A_interfaces, B_interfaces)

Done.

[Inactive, 2014-05-08 00:31:26]

lgtm but please let haraken and Nils review as well.

[haraken, 2014-05-08 00:58:32]

LGTM but nbarth@ should check.

[Inactive, 2014-05-08 01:13:01]

cshu: By the way, what does 'RHS' mean? I am not a big fan of abbreviations
because I never understand them :)

[Nils Barth (inactive), 2014-05-08 01:37:35]

On 2014/05/08 01:13:01, Chris Dumez wrote:
> cshu: By the way, what does 'RHS' mean? I am not a big fan of abbreviations
> because I never understand them :)

lhs/rhs (left hand side, right hand side) are pretty standard (if informal)
acronyms:
http://en.wikipedia.org/wiki/Sides_of_an_equation

[Inactive, 2014-05-08 01:41:36]

On 2014/05/08 01:37:35, Nils Barth wrote:
> On 2014/05/08 01:13:01, Chris Dumez wrote:
> > cshu: By the way, what does 'RHS' mean? I am not a big fan of abbreviations
> > because I never understand them :)
> 
> lhs/rhs (left hand side, right hand side) are pretty standard (if informal)
> acronyms:
> http://en.wikipedia.org/wiki/Sides_of_an_equation

Thanks for the explanation Nils.

[c.shu, 2014-05-08 02:00:12]

On 2014/05/08 01:41:36, Chris Dumez wrote:
> On 2014/05/08 01:37:35, Nils Barth wrote:
> > On 2014/05/08 01:13:01, Chris Dumez wrote:
> > > cshu: By the way, what does 'RHS' mean? I am not a big fan of
abbreviations
> > > because I never understand them :)
> > 
> > lhs/rhs (left hand side, right hand side) are pretty standard (if informal)
> > acronyms:
> > http://en.wikipedia.org/wiki/Sides_of_an_equation
> 
> Thanks for the explanation Nils.

yes, it's for right hand side. Maybe a better wording is needed. any
suggestions?

[Inactive, 2014-05-08 02:09:14]

On 2014/05/08 02:00:12, c.shu wrote:
> On 2014/05/08 01:41:36, Chris Dumez wrote:
> > On 2014/05/08 01:37:35, Nils Barth wrote:
> > > On 2014/05/08 01:13:01, Chris Dumez wrote:
> > > > cshu: By the way, what does 'RHS' mean? I am not a big fan of
> abbreviations
> > > > because I never understand them :)
> > > 
> > > lhs/rhs (left hand side, right hand side) are pretty standard (if
informal)
> > > acronyms:
> > > http://en.wikipedia.org/wiki/Sides_of_an_equation
> > 
> > Thanks for the explanation Nils.
> 
> yes, it's for right hand side. Maybe a better wording is needed. any
> suggestions?

I personally think right-hand side is clear. I just wasn't aware of the
abbreviation.  Maybe we could use the non abbreviated form in the description
for folks like me?

[c.shu, 2014-05-08 02:16:16]

On 2014/05/08 02:09:14, Chris Dumez wrote:
> On 2014/05/08 02:00:12, c.shu wrote:
> > On 2014/05/08 01:41:36, Chris Dumez wrote:
> > > On 2014/05/08 01:37:35, Nils Barth wrote:
> > > > On 2014/05/08 01:13:01, Chris Dumez wrote:
> > > > > cshu: By the way, what does 'RHS' mean? I am not a big fan of
> > abbreviations
> > > > > because I never understand them :)
> > > > 
> > > > lhs/rhs (left hand side, right hand side) are pretty standard (if
> informal)
> > > > acronyms:
> > > > http://en.wikipedia.org/wiki/Sides_of_an_equation
> > > 
> > > Thanks for the explanation Nils.
> > 
> > yes, it's for right hand side. Maybe a better wording is needed. any
> > suggestions?
> 
> I personally think right-hand side is clear. I just wasn't aware of the
> abbreviation.  Maybe we could use the non abbreviated form in the description
> for folks like me?

As you wish.

[Nils Barth (inactive), 2014-05-08 02:37:39]

Thanks so much for doing this Chang!

This is correct and necessary for componentization to be reflected in the IDL
files.
We already have support in C++ to not violate layering (via the [TreatAsPartial]
extended attribute), and this will allow us to ensure it at the IDL level.

I'll review and comment in more detail, but approach is fine.

For background:
(Offline chat with haraken.)
The issue of subtyping (inheritance, implements, and partial interfaces) is
*very* confusing.

I've written it up in detail at:
http://www.chromium.org/developers/web-idl-interfaces#TOC-Subtyping

Summary:
* implements is fundamentally *multiple (interface) inheritance*, and
implemented methods SHOULD be implemented on impl (on the C++ object of the
implementing class).
This is important for some interfaces, like WebGL, and also avoids boilerplate.

I've been fixing this in:
Web IDL: make 'implements' use (object) methods, not static member functions
https://code.google.com/p/chromium/issues/detail?id=360435

* partial interfaces are *type extension*, and implemented methods are
implemented as static methods on an unrelated class.
This is to ensure layering when necessary, but adds boilerplate.

* interfaces in core SHOULD NOT implement interfaces in modules: this is a
layering violation, and is because the core/module distinction is a Blink
implementation detail, not something from the spec.
However, this exists and it seems we need to allow it (can't just move these to
core), so in these cases ONLY we have static member functions (to preserve
layering), specified by  the extended attribute [TreatAsPartial].

Bottom line:
We need both behaviors for implements:
should actually implement by default, but
allow static member functions if necessary for layering.

[Nils Barth (inactive), 2014-05-08 02:46:46]

LGTM with nits.

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

https://codereview.chromium.org/270573005/diff/50001/Source/bindings/scripts/...
Source/bindings/scripts/utilities.py:64: # Returns two lists of interfaces that
contain identifier-As and identifier-Bs.
Could you wrap at 80 cols?

Actually, could you make this into a docstring?
"""Returns lists of implementing and implemented interfaces.

Rule is: ...
"""

https://codereview.chromium.org/270573005/diff/50001/Source/bindings/scripts/...
Source/bindings/scripts/utilities.py:74: for match in implements_matches]
Could you add a check that *one* of the interfaces is included in the implements
statement?
foreign_implements = [(left, right) for left, right in implements_pairs
                      if interface_name not in (left, right)]
if foreign_implements:
    raise ...

https://codereview.chromium.org/270573005/diff/50001/Source/bindings/scripts/...
Source/bindings/scripts/utilities.py:75: A_interfaces = []
List comprehensions please:
A_interfaces = [left for left, right in implements_pairs
                if right == interface_name]
B_interfaces = [right for left, right in implements_pairs
                if left == interface_name]

https://codereview.chromium.org/270573005/diff/50001/Source/bindings/scripts/...
Source/bindings/scripts/utilities.py:82: return (A_interfaces, B_interfaces)
No parens.

https://codereview.chromium.org/270573005/diff/50001/Source/modules/imagebitm...
File Source/modules/imagebitmap/ImageBitmapFactories.idl (right):

https://codereview.chromium.org/270573005/diff/50001/Source/modules/imagebitm...
Source/modules/imagebitmap/ImageBitmapFactories.idl:61: 
No trailing blank line

[c.shu, 2014-05-08 02:48:34]

On 2014/05/08 02:37:39, Nils Barth wrote:
> Thanks so much for doing this Chang!
> 
> This is correct and necessary for componentization to be reflected in the IDL
> files.
> We already have support in C++ to not violate layering (via the
[TreatAsPartial]
> extended attribute), and this will allow us to ensure it at the IDL level.
> 
> I'll review and comment in more detail, but approach is fine.
> 
> For background:
> (Offline chat with haraken.)
> The issue of subtyping (inheritance, implements, and partial interfaces) is
> *very* confusing.
> 
> I've written it up in detail at:
> http://www.chromium.org/developers/web-idl-interfaces#TOC-Subtyping
> 
> Summary:
> * implements is fundamentally *multiple (interface) inheritance*, and
> implemented methods SHOULD be implemented on impl (on the C++ object of the
> implementing class).
> This is important for some interfaces, like WebGL, and also avoids
boilerplate.
> 
> I've been fixing this in:
> Web IDL: make 'implements' use (object) methods, not static member functions
> https://code.google.com/p/chromium/issues/detail?id=360435
> 
> * partial interfaces are *type extension*, and implemented methods are
> implemented as static methods on an unrelated class.
> This is to ensure layering when necessary, but adds boilerplate.
> 
> * interfaces in core SHOULD NOT implement interfaces in modules: this is a
> layering violation, and is because the core/module distinction is a Blink
> implementation detail, not something from the spec.
> However, this exists and it seems we need to allow it (can't just move these
to
> core), so in these cases ONLY we have static member functions (to preserve
> layering), specified by  the extended attribute [TreatAsPartial].
> 
> Bottom line:
> We need both behaviors for implements:
> should actually implement by default, but
> allow static member functions if necessary for layering.

Excellent background summary, Nils. I will read them in details. Thanks!

[Nils Barth (inactive), 2014-05-08 02:54:22]

A note on *where* to put the "implements" statement.
Specs vary, and use both.

Option 1: Put on implement*ing* interface (current policy), *unless* necessary
to fix layering (as here).
This means it normally looks and behaves like inheritance, but if necessary
looks and behaves as type extension.
(Could enforce if desired.)

Option 2: Follow specs.
Option 3: Use our judgment case-by-case.

Option 1 is clearest implementation-wise.
Option 2 is most spec-correct, but v. minor.
Option 3 is potentially confusing.

WDYT?

[c.shu, 2014-05-08 02:57:20]

The CQ bit was checked by c.shu@samsung.com

[commit-bot: I haz the power, 2014-05-08 02:58:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/c.shu@samsung.com/270573005/50001

[commit-bot: I haz the power, 2014-05-08 04:08:52]

Change committed as 173599

[Nils Barth (inactive), 2014-05-08 06:37:03]

On 2014/05/08 02:57:20, c.shu wrote:
> The CQ bit was checked by mailto:c.shu@samsung.com

BTW Chang, could you make sure to fix last nits before committing?
"LGTM with nits" is quite common in these parts ;)
(In this case I fixed in a followup.)

[Nils Barth (inactive), 2014-05-08 06:40:25]

This caused a run-bindings-test failure, and the logic was fragile:
we need to handle these RHS implements *afterward* processing individual files,
which requires some changes to the logic.

Presumably wasn't caught by PRESUBMIT because depends on file ordering;
not a big deal, and easily fixed.

Also, this should have had a test case; also fixed in followup.
(Meaning: don't worry about it Chang!)

Fix "implements in RHS file"
https://codereview.chromium.org/270823002/

[c.shu, 2014-05-08 14:26:51]

On 2014/05/08 06:37:03, Nils Barth wrote:
> On 2014/05/08 02:57:20, c.shu wrote:
> > The CQ bit was checked by mailto:c.shu@samsung.com
> 
> BTW Chang, could you make sure to fix last nits before committing?
> "LGTM with nits" is quite common in these parts ;)
> (In this case I fixed in a followup.)

Really sorry about this. I didn't realize there were comments in your post. I
made a post almost at the same time and your comment was collapsed. I just saw
the green line. :)

[Nils Barth (inactive), 2014-05-09 00:40:50]

On 2014/05/08 14:26:51, c.shu wrote:
> Really sorry about this. I didn't realize there were comments in your post. I
> made a post almost at the same time and your comment was collapsed. I just saw
> the green line. :)

N/p -- I was just a bit puzzled (^.-)b