Split bindings/v8/custom into core and modules.

Split bindings/v8/custom into core and modules.

- Custom bindings for modules are moved into bindings/modules/v8/custom, and
- Custom bindings for core are moved into bindings/core/v8/custom.
- Update gn and gypi files.

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

[tasak, 2014-06-17 05:31:21]

Would you review this CL?

[Nils Barth (inactive), 2014-06-17 06:43:57]

Few nits, otherwise ok.

https://codereview.chromium.org/331373002/diff/20001/Source/bindings/modules/...
File Source/bindings/modules/v8/custom/custom.gypi (right):

https://codereview.chromium.org/331373002/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/custom/custom.gypi:7:
'bindings_v8_modules_custom_dir': '.',
You're not using this variable, so there's no need for it.
Currently bindings_v8_custom_dir is used in core/core.gyp and core/BUILD.gn
as an include_dir, but this is no longer necessary
(since includes of v8/custom are qualified correctly),
so I'm removing it in another CL:
core build: Remove unnecessary include_dirs (bindings cruft)
https://codereview.chromium.org/340473002/

https://codereview.chromium.org/331373002/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/custom/custom.gypi:8:
'bindings_v8_modules_custom_files': [
v8 and modules are backwards; should be (matching dir order):
'bindings_modules_v8_custom_files': ...

https://codereview.chromium.org/331373002/diff/20001/Source/bindings/v8/v8.gypi
File Source/bindings/v8/v8.gypi (right):

https://codereview.chromium.org/331373002/diff/20001/Source/bindings/v8/v8.gy...
Source/bindings/v8/v8.gypi:8: '../modules/v8/custom/custom.gypi',
BTW, eventually we need to eliminate this file
(once this directory is empty).

However, it's fine to keep it short-term so we don't have
direct references from core to bindings_modules,
so this addition is ok.

[Nils Barth (inactive), 2014-06-17 06:44:56]

Also, could you use more specific titles?

Rather than "cleanup", this is "move files" or
"split off modules from bindings/v8/custom".

Thanks!

[tasak, 2014-06-24 02:43:41]

Thank you for reviewing.

I'm now trying to all files under Source/bindings/v8/custom.

https://codereview.chromium.org/331373002/diff/20001/Source/bindings/modules/...
File Source/bindings/modules/v8/custom/custom.gypi (right):

https://codereview.chromium.org/331373002/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/custom/custom.gypi:7:
'bindings_v8_modules_custom_dir': '.',
On 2014/06/17 06:43:57, Nils Barth wrote:
> You're not using this variable, so there's no need for it.
> Currently bindings_v8_custom_dir is used in core/core.gyp and core/BUILD.gn
> as an include_dir, but this is no longer necessary
> (since includes of v8/custom are qualified correctly),
> so I'm removing it in another CL:
> core build: Remove unnecessary include_dirs (bindings cruft)
> https://codereview.chromium.org/340473002/

Done.

https://codereview.chromium.org/331373002/diff/20001/Source/bindings/modules/...
Source/bindings/modules/v8/custom/custom.gypi:8:
'bindings_v8_modules_custom_files': [
On 2014/06/17 06:43:57, Nils Barth wrote:
> v8 and modules are backwards; should be (matching dir order):
> 'bindings_modules_v8_custom_files': ...

I see.
Done.

https://codereview.chromium.org/331373002/diff/20001/Source/bindings/v8/v8.gypi
File Source/bindings/v8/v8.gypi (right):

https://codereview.chromium.org/331373002/diff/20001/Source/bindings/v8/v8.gy...
Source/bindings/v8/v8.gypi:8: '../modules/v8/custom/custom.gypi',
On 2014/06/17 06:43:57, Nils Barth wrote:
> BTW, eventually we need to eliminate this file
> (once this directory is empty).
> 
> However, it's fine to keep it short-term so we don't have
> direct references from core to bindings_modules,
> so this addition is ok.

Yeah, we should remove the file.

However, currently I found that we need the file because Source/core/core.gyp
uses "bindings_modules_v8_output_dir".

So now the file is '../modules/v8/generated.gypi'.

[Nils Barth (inactive), 2014-06-24 03:13:39]

On 2014/06/24 02:43:41, tasak wrote:
> I'm now trying to all files under Source/bindings/v8/custom.

Could you *update* the CL description when you change its content?
(I've done so here.)

[Nils Barth (inactive), 2014-06-24 03:14:03]

LGTM with nits, thanks!

https://codereview.chromium.org/331373002/diff/20001/Source/bindings/v8/v8.gypi
File Source/bindings/v8/v8.gypi (right):

https://codereview.chromium.org/331373002/diff/20001/Source/bindings/v8/v8.gy...
Source/bindings/v8/v8.gypi:8: '../modules/v8/custom/custom.gypi',
On 2014/06/24 02:43:41, tasak wrote:
> Yeah, we should remove the file.
> 
> However, currently I found that we need the file because Source/core/core.gyp
> uses "bindings_modules_v8_output_dir".
> 
> So now the file is '../modules/v8/generated.gypi'.

Exactly!

https://codereview.chromium.org/331373002/diff/60001/Source/bindings/modules/...
File Source/bindings/modules/modules.gypi (right):

https://codereview.chromium.org/331373002/diff/60001/Source/bindings/modules/...
Source/bindings/modules/modules.gypi:7: 'v8/v8.gypi',
alpha

https://codereview.chromium.org/331373002/diff/60001/Source/bindings/modules/...
File Source/bindings/modules/v8/v8.gypi (right):

https://codereview.chromium.org/331373002/diff/60001/Source/bindings/modules/...
Source/bindings/modules/v8/v8.gypi:10: 'bindings_modules_v8_dir': '.',
This isn't being used, is it?
Can you remove it?

https://codereview.chromium.org/331373002/diff/60001/Source/bindings/modules/...
Source/bindings/modules/v8/v8.gypi:11: 'bindings_modules_v8_files': [
You're not using this variable, but you should
(in Source/modules/modules.gyp)

https://codereview.chromium.org/331373002/diff/60001/Source/core/core.gyp
File Source/core/core.gyp (right):

https://codereview.chromium.org/331373002/diff/60001/Source/core/core.gyp#new...
Source/core/core.gyp:48: '<(bindings_modules_v8_output_dir)', # e.g.
EventTargetCustom requires.
You mean EventHeaders.h (gen/blink/core),
not V8EventTargetCustom.cpp (Source/bindings/[core/]v8);
it's *core* build scripts that are the problems:
bindings all have correctly qualified includes.

Could you fix this and also move it up so it's above the dir lines?

https://codereview.chromium.org/331373002/diff/60001/Source/modules/modules.gyp
File Source/modules/modules.gyp (right):

https://codereview.chromium.org/331373002/diff/60001/Source/modules/modules.g...
Source/modules/modules.gyp:54: '<@(bindings_modules_v8_custom_files)',
Could you remove the _custom and just make this:
'<@(bindings_modules_v8_files)',
(...that way it also works if we don't have only custom files)

[tasak, 2014-06-24 10:55:45]

Thank you for reviewing.

https://codereview.chromium.org/331373002/diff/60001/Source/bindings/modules/...
File Source/bindings/modules/modules.gypi (right):

https://codereview.chromium.org/331373002/diff/60001/Source/bindings/modules/...
Source/bindings/modules/modules.gypi:7: 'v8/v8.gypi',
On 2014/06/24 03:14:02, Nils Barth wrote:
> alpha

Done.

https://codereview.chromium.org/331373002/diff/60001/Source/bindings/modules/...
File Source/bindings/modules/v8/v8.gypi (right):

https://codereview.chromium.org/331373002/diff/60001/Source/bindings/modules/...
Source/bindings/modules/v8/v8.gypi:10: 'bindings_modules_v8_dir': '.',
On 2014/06/24 03:14:02, Nils Barth wrote:
> This isn't being used, is it?
> Can you remove it?

Done.

https://codereview.chromium.org/331373002/diff/60001/Source/bindings/modules/...
Source/bindings/modules/v8/v8.gypi:11: 'bindings_modules_v8_files': [
On 2014/06/24 03:14:02, Nils Barth wrote:
> You're not using this variable, but you should
> (in Source/modules/modules.gyp)

I see. Done.

https://codereview.chromium.org/331373002/diff/60001/Source/core/core.gyp
File Source/core/core.gyp (right):

https://codereview.chromium.org/331373002/diff/60001/Source/core/core.gyp#new...
Source/core/core.gyp:48: '<(bindings_modules_v8_output_dir)', # e.g.
EventTargetCustom requires.
On 2014/06/24 03:14:02, Nils Barth wrote:
> You mean EventHeaders.h (gen/blink/core),
> not V8EventTargetCustom.cpp (Source/bindings/[core/]v8);
> it's *core* build scripts that are the problems:
> bindings all have correctly qualified includes.

I mean, EventTargetCustom.cpp's toV8 depends on
modules/EventTargetModulesHeaders because of 
EVENT_TARGET_MODULES_INTERFACES_FOR_EACH(TRY_TO_WRAP_WITH_INTERFACE).

I think, this means, core's toV8 depends on some modules.

I have to split the toV8 into core version and modules' version.

> Could you fix this and also move it up so it's above the dir lines?

As far as I understand, core can depend on bindings' core. So
bindings_core_v8_output_dir looks correct, but bindings_modules_v8_output_dir
looks wrong.
I moved up bindings_core_v8_output_dir line and updated the comment.

https://codereview.chromium.org/331373002/diff/60001/Source/modules/modules.gyp
File Source/modules/modules.gyp (right):

https://codereview.chromium.org/331373002/diff/60001/Source/modules/modules.g...
Source/modules/modules.gyp:54: '<@(bindings_modules_v8_custom_files)',
On 2014/06/24 03:14:02, Nils Barth wrote:
> Could you remove the _custom and just make this:
> '<@(bindings_modules_v8_files)',
> (...that way it also works if we don't have only custom files)

Done.

[Nils Barth (inactive), 2014-06-25 01:37:40]

One clarification, explaining what's going on with include dirs.
Again, could you fix the comment in core/core.gyp ?
Thanks!

https://codereview.chromium.org/331373002/diff/60001/Source/core/core.gyp
File Source/core/core.gyp (right):

https://codereview.chromium.org/331373002/diff/60001/Source/core/core.gyp#new...
Source/core/core.gyp:48: '<(bindings_modules_v8_output_dir)', # e.g.
EventTargetCustom requires.
On 2014/06/24 10:55:45, tasak wrote:
> On 2014/06/24 03:14:02, Nils Barth wrote:
> > You mean EventHeaders.h (gen/blink/core),
> > not V8EventTargetCustom.cpp (Source/bindings/[core/]v8);
> > it's *core* build scripts that are the problems:
> > bindings all have correctly qualified includes.
> 
> I mean, EventTargetCustom.cpp's toV8 depends on
> modules/EventTargetModulesHeaders because of 
> EVENT_TARGET_MODULES_INTERFACES_FOR_EACH(TRY_TO_WRAP_WITH_INTERFACE).
> 
> I think, this means, core's toV8 depends on some modules.

*V8*EventTargetCustom.cpp.
That's a good point, and something that needs fixing,
but that's not what's happening here.

This is "include dirs", and these lines are only necessary
due to *unqualified* includes.
(We're qualifying includes exactly so we can track dependencies,
as you're noticing.)

For example, the include lines for
V8EventTargetCustom.cpp
are:
#include "core/EventTargetHeaders.h"
#include "core/EventTargetInterfaces.h"
#include "modules/EventTargetModulesHeaders.h"
#include "modules/EventTargetModulesInterfaces.h"

All the includes are qualified (relative to Source or gen/blink),
so V8EventTargetCustom.cpp does *not* need
gen/blink/bindings/core/v8
gen/blink/bindings/modules/v8
in its include search path.

However, EventHeaders.h starts with:
#include "core/events/KeyboardEvent.h"
#include "V8KeyboardEvent.h"
...and the second line is an *unqualified* include, which should read:
#include "bindings/core/v8/V8KeyboardEvent.h"

That's why these include_dir lines are there, and that's why
I was correcting your comment.

This is what Chang was doing in:
Add sub-directories to the include lines of generated files.
https://code.google.com/p/chromium/issues/detail?id=381876

> > Could you fix this and also move it up so it's above the dir lines?
> 
> As far as I understand, core can depend on bindings' core.

*bindings_core*

> So bindings_core_v8_output_dir looks correct, but
> bindings_modules_v8_output_dir looks wrong.

You're right about dependencies, but *include dirs* should not
be used.
That ensures that include paths are properly qualified,
and thus we can use checkdeps to validate *dependencies*
(which is what we really care about).
https://code.google.com/p/chromium/issues/detail?id=365190


So here's the idea:
1. Make core properly qualify include paths.
2. Remove these include_dir lines (this guarantees that include paths are
properly qualified).
3. Run checkdeps on gen/blink.
4. It fails because
gen/blink/bindings/core/v8/V8EventTargetCustom.cpp
has
#include "modules/EventTargetModulesHeaders.h"
...and bindings_core is not allowed to depend on modules.

[haraken, 2014-06-25 05:22:52]

LGTM.

I believe this CL is going on a right way, so I think we can land this CL and
iterate from there.

[tasak, 2014-06-25 12:33:35]

https://codereview.chromium.org/331373002/diff/60001/Source/core/core.gyp
File Source/core/core.gyp (right):

https://codereview.chromium.org/331373002/diff/60001/Source/core/core.gyp#new...
Source/core/core.gyp:48: '<(bindings_modules_v8_output_dir)', # e.g.
EventTargetCustom requires.
On 2014/06/25 01:37:40, Nils Barth wrote:
> On 2014/06/24 10:55:45, tasak wrote:
> > On 2014/06/24 03:14:02, Nils Barth wrote:
> > > You mean EventHeaders.h (gen/blink/core),
> > > not V8EventTargetCustom.cpp (Source/bindings/[core/]v8);
> > > it's *core* build scripts that are the problems:
> > > bindings all have correctly qualified includes.
> > 
> > I mean, EventTargetCustom.cpp's toV8 depends on
> > modules/EventTargetModulesHeaders because of 
> > EVENT_TARGET_MODULES_INTERFACES_FOR_EACH(TRY_TO_WRAP_WITH_INTERFACE).
> > 
> > I think, this means, core's toV8 depends on some modules.
> 
> *V8*EventTargetCustom.cpp.
> That's a good point, and something that needs fixing,
> but that's not what's happening here.
> 
> This is "include dirs", and these lines are only necessary
> due to *unqualified* includes.
> (We're qualifying includes exactly so we can track dependencies,
> as you're noticing.)
> 
> For example, the include lines for
> V8EventTargetCustom.cpp
> are:
> #include "core/EventTargetHeaders.h"
> #include "core/EventTargetInterfaces.h"
> #include "modules/EventTargetModulesHeaders.h"
> #include "modules/EventTargetModulesInterfaces.h"
> 
> All the includes are qualified (relative to Source or gen/blink),
> so V8EventTargetCustom.cpp does *not* need
> gen/blink/bindings/core/v8
> gen/blink/bindings/modules/v8
> in its include search path.
> 
> However, EventHeaders.h starts with:
> #include "core/events/KeyboardEvent.h"
> #include "V8KeyboardEvent.h"
> ...and the second line is an *unqualified* include, which should read:
> #include "bindings/core/v8/V8KeyboardEvent.h"
> 
> That's why these include_dir lines are there, and that's why
> I was correcting your comment.
> 
> This is what Chang was doing in:
> Add sub-directories to the include lines of generated files.
> https://code.google.com/p/chromium/issues/detail?id=381876
> 
> > > Could you fix this and also move it up so it's above the dir lines?
> > 
> > As far as I understand, core can depend on bindings' core.
> 
> *bindings_core*
> 
> > So bindings_core_v8_output_dir looks correct, but
> > bindings_modules_v8_output_dir looks wrong.
> 
> You're right about dependencies, but *include dirs* should not
> be used.
> That ensures that include paths are properly qualified,
> and thus we can use checkdeps to validate *dependencies*
> (which is what we really care about).
> https://code.google.com/p/chromium/issues/detail?id=365190
> 
> 
> So here's the idea:
> 1. Make core properly qualify include paths.
> 2. Remove these include_dir lines (this guarantees that include paths are
> properly qualified).
> 3. Run checkdeps on gen/blink.
> 4. It fails because
> gen/blink/bindings/core/v8/V8EventTargetCustom.cpp
> has
> #include "modules/EventTargetModulesHeaders.h"
> ...and bindings_core is not allowed to depend on modules.

Thank you. I see. I probably understand what are issues. The original comment is
correct.
I've just reverted my change.

[tasak, 2014-06-26 02:40:57]

The CQ bit was checked by tasak@google.com

[commit-bot: I haz the power, 2014-06-26 02:41:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tasak@google.com/331373002/140001

[commit-bot: I haz the power, 2014-06-26 03:52:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/8553)

[commit-bot: I haz the power, 2014-06-26 03:56:00]

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

[commit-bot: I haz the power, 2014-06-26 03:56:04]

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

[tasak, 2014-06-26 04:57:07]

jochen, would you review this CL?

I need Source/web owner's LGTM.

[jochen (gone - plz use gerrit), 2014-06-26 14:52:39]

lgtm

[tasak, 2014-06-27 03:43:48]

On 2014/06/26 14:52:39, jochen wrote:
> lgtm

Thank you.

[tasak, 2014-06-27 03:43:57]

The CQ bit was checked by tasak@google.com

[commit-bot: I haz the power, 2014-06-27 03:46:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tasak@google.com/331373002/140001

[commit-bot: I haz the power, 2014-06-27 04:28:25]

Change committed as 177062