Bindings: inherit enums and callbacks from partial interface files.

Bindings: inherit enums and callbacks from partial interface files.

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

[haraken, 2014-03-19 16:01:09]

LGTM, but I want to have nbarth@ take another look.

[Nils Barth (inactive), 2014-03-20 01:01:46]

Could you hold off for a minute?
I'll quickly do a bit of refactoring that will make this CL
easier and cleaner.
Thanks for doing this!

https://codereview.chromium.org/203193015/diff/1/Source/bindings/scripts/idl_...
File Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/203193015/diff/1/Source/bindings/scripts/idl_...
Source/bindings/scripts/idl_definitions.py:151: def merge(self, other):
This is a useful method!
Could you:
1. Move the attribute, constant, and operation appending to this method?
This requires just having a conditional:
* if interface doesn't exist, add it;
* if it does exist, append members (by calling merge() on IdlInterface)

2. Add a docstring, like:
"""Merge in another set of IdlDefinitions (e.g., partial interface)."""

[Nils Barth (inactive), 2014-03-20 02:11:10]

Thanks Mounir!

I've done some refactoring based on this CL in a separate CL:
Refactor interface dependency resolution
https://codereview.chromium.org/205873003/

(I had a few comments and saw ways the existing code could
be improved, so I did that myself.)

Once that lands, could you update this CL
(which will then be quite simple) and it will be ok?

Thanks!

https://codereview.chromium.org/203193015/diff/1/Source/bindings/scripts/idl_...
File Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/203193015/diff/1/Source/bindings/scripts/idl_...
Source/bindings/scripts/idl_definitions.py:151: def merge(self, other):
This method is a great idea, so I've implemented it in a separate CL
(calling it update()):
Refactor interface dependency resolution
https://codereview.chromium.org/205873003/

Once that lands, you'll just need to add these two lines to the method,
and then add your tests, and this CL will be good to go!

[Nils Barth (inactive), 2014-03-20 05:46:12]

Hi Mounir,
The refactoring CL has landed;
if you could rebase and repost, I'll be happy to ok it!
(shouldn't need any interface_dependency_resolver.py changes,
just idl_definitions and tests).

[mlamouri (slow - plz ping), 2014-03-20 10:36:50]

All comments applied and rebased on top of the refactor.

nbarth@, PTAL.

[mlamouri (slow - plz ping), 2014-03-21 16:42:52]

On 2014/03/20 10:36:50, Mounir Lamouri wrote:
> All comments applied and rebased on top of the refactor.
> 
> nbarth@, PTAL.

Nils, just a friendly reminder: I am waiting for your review to land this CL and
it is blocking a couple of other CLs I have.

[haraken, 2014-03-21 16:44:27]

On 2014/03/21 16:42:52, Mounir Lamouri wrote:
> On 2014/03/20 10:36:50, Mounir Lamouri wrote:
> > All comments applied and rebased on top of the refactor.
> > 
> > nbarth@, PTAL.
> 
> Nils, just a friendly reminder: I am waiting for your review to land this CL
and
> it is blocking a couple of other CLs I have.

Japan is already in the weekend. I think it's OK to land this CL now. LGTM.

Let's give Nils a chance to take a look on Monday.

[mlamouri (slow - plz ping), 2014-03-21 16:46:48]

On 2014/03/21 16:44:27, haraken wrote:
> On 2014/03/21 16:42:52, Mounir Lamouri wrote:
> > On 2014/03/20 10:36:50, Mounir Lamouri wrote:
> > > All comments applied and rebased on top of the refactor.
> > > 
> > > nbarth@, PTAL.
> > 
> > Nils, just a friendly reminder: I am waiting for your review to land this CL
> and
> > it is blocking a couple of other CLs I have.
> 
> Japan is already in the weekend. I think it's OK to land this CL now. LGTM.
> 
> Let's give Nils a chance to take a look on Monday.

I realized that it was nearly morning in Japan but didn't realize it was nearly
*Saturday* morning ;)

I'm going to land that then.

[mlamouri (slow - plz ping), 2014-03-21 16:46:55]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-03-21 16:47:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/203193015/20001

[commit-bot: I haz the power, 2014-03-21 17:47:21]

Change committed as 169749

[Nils Barth (inactive), 2014-03-24 01:50:54]

LGTM; thanks for patience and for implementing this!

[Nils Barth (inactive), 2014-03-24 01:53:54]

On 2014/03/21 16:42:52, Mounir Lamouri wrote:
> Nils, just a friendly reminder: I am waiting for your review to land this CL
and
> it is blocking a couple of other CLs I have.

Thanks for checking and waiting (we had a long weekend in Japan);
in general I'm ok with your going ahead and landing
if you think you've addressed concerns (and any remaining concerns
would just be nits), particularly if it's already been ok'ed by
someone else.
...so feel free to use your judgment and I won't get upset!