Fixes to make scripts generate includes with paths.

Fixes to make scripts generate includes with paths.

Adds a hacky idl->path table and equally hacky script
changes to use that script.

BUG=

[eseidel, 2013-04-25 07:03:39]

Haraken is definitely your best reviewer here.  abarth might have thoughts too.

https://codereview.chromium.org/14456006/diff/1/Source/bindings/scripts/idlto...
File Source/bindings/scripts/idltopathgenerator.py (right):

https://codereview.chromium.org/14456006/diff/1/Source/bindings/scripts/idlto...
Source/bindings/scripts/idltopathgenerator.py:95: output.append("# GENERATED BY
idltopath.py, DO NOT MODIFY BY HAND.")
Woh.  Using python to generate perl, eh?

I think you might want to use a multi-line string for this. :)
http://www.python.org/dev/peps/pep-0295/

[eseidel, 2013-04-25 07:03:55]

CCing nico for good measure.

[Daniel Bratell, 2013-04-25 08:48:28]

On 2013/04/25 07:03:39, Eric Seidel (Google) wrote:
> Woh.  Using python to generate perl, eh?
Doh!

That was not supposed to have been included. Dead code from an earlier solution
that wasn't fully integrated in the build system.

[haraken, 2013-04-25 13:58:14]

Nico: Would you first check if the proposed patch is OK in term of build
dependency/performance? I'm happy to review script implementation.

[abarth-chromium, 2013-04-25 14:14:24]

I don't think this CL is better than the alternative of just adding the needed
include_dirs when building the generated files.  This CL adds a lot of
complexity and makes the overall system more brittle and harder to change.

https://codereview.chromium.org/14456006/diff/5001/Source/bindings/derived_so...
File Source/bindings/derived_sources.gyp (right):

https://codereview.chromium.org/14456006/diff/5001/Source/bindings/derived_so...
Source/bindings/derived_sources.gyp:123: 'action_name':
'generateIDL2PathMapping',
Maybe rather than calling this an IDL2PathMap, we should just call it a
HeaderMap.  It's not really specific to IDLs.

https://codereview.chromium.org/14456006/diff/5001/Source/bindings/scripts/Co...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://codereview.chromium.org/14456006/diff/5001/Source/bindings/scripts/Co...
Source/bindings/scripts/CodeGeneratorV8.pm:218:
$implIncludes{"bindings/v8/V8GCController.h"} = 1;
Why not use the header map for these headers as well?

https://codereview.chromium.org/14456006/diff/5001/Source/bindings/scripts/ge...
File Source/bindings/scripts/generate-bindings.pl (right):

https://codereview.chromium.org/14456006/diff/5001/Source/bindings/scripts/ge...
Source/bindings/scripts/generate-bindings.pl:73: $ENV{"IDLTOPATHFILE"} =
$idlToPathFile;
We should find a way to avoid messing with the environment.  Explicit data flow
is much better than implicit data flow via global variables.

https://codereview.chromium.org/14456006/diff/5001/Source/bindings/scripts/id...
File Source/bindings/scripts/idltopathgenerator.py (right):

https://codereview.chromium.org/14456006/diff/5001/Source/bindings/scripts/id...
Source/bindings/scripts/idltopathgenerator.py:16:
parser.add_argument("--blink-source-directory", required=True,
Please don't use the word "blink" in source code.  We haven't decided whether
the code is going to know that it's part of a project called blink yet.

https://codereview.chromium.org/14456006/diff/5001/Source/bindings/scripts/id...
Source/bindings/scripts/idltopathgenerator.py:29: # Find the WebCore directory
There is no such thing as WebCore.

https://codereview.chromium.org/14456006/diff/5001/Source/bindings/scripts/id...
Source/bindings/scripts/idltopathgenerator.py:30: webkit_source =
os.path.abspath(args.blink_source_directory)
Please don't use the word "webkit" in new source code.  We're going to remove
the word from the codebase at some point.

https://codereview.chromium.org/14456006/diff/5001/Source/bindings/scripts/id...
Source/bindings/scripts/idltopathgenerator.py:33: core_dir =
os.path.join(webkit_source, "core")
I don't think we should hard-code information about the structure of the source
directory into these scripts.  We're likely to change it in the future and this
sort of thing just makes the system brittle.

https://codereview.chromium.org/14456006/diff/5001/Source/bindings/scripts/id...
Source/bindings/scripts/idltopathgenerator.py:57: ("InternalSettingsGenerated",
".."),
This doesn't look very maintainable.  It would be better to fix the bugs in the
code that require these special cases.

https://codereview.chromium.org/14456006/diff/5001/Source/core/scripts/InFile...
File Source/core/scripts/InFilesCompiler.pm (right):

https://codereview.chromium.org/14456006/diff/5001/Source/core/scripts/InFile...
Source/core/scripts/InFilesCompiler.pm:41: use lib
"$FindBin::Bin/../../../../../third_party/WebKit/Source/bindings/scripts";  #
use the binding generator directory.
Woah there.  The code shouldn't know that it's contained in a
"third_party/WebKit" directory.  That's likely to change in the future.  We
don't want to hard-code that information into the code.  That makes the system
brittle and harder to change in the future.

[Daniel Bratell, 2013-04-25 17:54:09]

On 2013/04/25 14:14:24, abarth wrote:
> I don't think this CL is better than the alternative of just adding the needed
> include_dirs when building the generated files.

I don't know how much time we're talking about or exactly what files, but is it
only a verly limited number of cc files, then you might be right. On the other
hand if it's also header files then not having paths in the includes can spread
to other parts of the code.

I was happy to read that it's already improved 10%, but that is still only a
quarter of the easily reachable improvement in Visual Studio so somewhere there
is more to do. Maybe this is not it, but I'm trying to upload the changes I did
one part at a time (after cleaning them up) so that we can see.

[Nico, 2013-04-25 19:26:16]

I think there are two parts here:

1.) Should generated header files have absolute paths? I think the answer is
"yes, if feasible". Without this, ever target that includes a generated header
needs to list a bunch of additional include directories.

2.) What's the best way to give generated files absolute paths? I agree that the
method in this CL is fairly heavy-weight.
https://codereview.chromium.org/13877023 might be a simpler approach (it's not
quite done yet though, so who knows).

[Daniel Bratell, 2013-04-25 19:59:59]

On 2013/04/25 19:26:16, Nico wrote:
> I think there are two parts here:
> 
> 1.) Should generated header files have absolute paths? I think the answer is
> "yes, if feasible". Without this, ever target that includes a generated header
> needs to list a bunch of additional include directories.
> 
> 2.) What's the best way to give generated files absolute paths? I agree that
the
> method in this CL is fairly heavy-weight.
> https://codereview.chromium.org/13877023 might be a simpler approach (it's not
> quite done yet though, so who knows).

It looks like a very similar solution idea. This whole patch depends on the fact
that idl files and their header files are in the same folders.

The problem was to know in what folder an idl file Foo is. I didn't know of a
better way than building a map of idl -> path, and that made something that
would have been small and simple into something more complex. Since I didn't
want to add slowness scanning the disk for them seemed to be wrong so I added
this one-time step of building a file, which takes about 100ms to do on my aging
and overloaded computer.

[Daniel Bratell, 2013-04-25 21:24:26]

https://codereview.chromium.org/14456006/diff/5001/Source/bindings/derived_so...
File Source/bindings/derived_sources.gyp (right):

https://codereview.chromium.org/14456006/diff/5001/Source/bindings/derived_so...
Source/bindings/derived_sources.gyp:123: 'action_name':
'generateIDL2PathMapping',
On 2013/04/25 14:14:24, abarth wrote:
> Maybe rather than calling this an IDL2PathMap, we should just call it a
> HeaderMap.  It's not really specific to IDLs.

It kind of is. The binding scripts work on IDL interface names and this map says
where the corresponding header file to include is. It's not a list of all header
files.

It would be possible to have a full header map too, and I had such a thing in
one of the other scripts I used to convert all the source, but since this needs
to be kept up to date with every change, I think it's best for build performance
and for dependency tracking that it's restricted to IDL interface names.

Also, there are (or were?) headers with the same name in the WebKit tree. If
that is the case, or becomes the case, a full header map would become non
deterministic.

https://codereview.chromium.org/14456006/diff/5001/Source/bindings/scripts/Co...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://codereview.chromium.org/14456006/diff/5001/Source/bindings/scripts/Co...
Source/bindings/scripts/CodeGeneratorV8.pm:218:
$implIncludes{"bindings/v8/V8GCController.h"} = 1;
On 2013/04/25 14:14:24, abarth wrote:
> Why not use the header map for these headers as well?

This is not an IDL interface name so these are not in that file. Also, since the
header file name is constant we don't need to calculate the position runtime.

https://codereview.chromium.org/14456006/diff/5001/Source/bindings/scripts/ge...
File Source/bindings/scripts/generate-bindings.pl (right):

https://codereview.chromium.org/14456006/diff/5001/Source/bindings/scripts/ge...
Source/bindings/scripts/generate-bindings.pl:73: $ENV{"IDLTOPATHFILE"} =
$idlToPathFile;
On 2013/04/25 14:14:24, abarth wrote:
> We should find a way to avoid messing with the environment.  Explicit data
flow
> is much better than implicit data flow via global variables.

This is the part where I just couldn't find a good solution. Maybe because I
don't know Perl well enough. But I can keep digging.

https://codereview.chromium.org/14456006/diff/5001/Source/bindings/scripts/id...
File Source/bindings/scripts/idltopathgenerator.py (right):

https://codereview.chromium.org/14456006/diff/5001/Source/bindings/scripts/id...
Source/bindings/scripts/idltopathgenerator.py:57: ("InternalSettingsGenerated",
".."),
On 2013/04/25 14:14:24, abarth wrote:
> This doesn't look very maintainable.  It would be better to fix the bugs in
the
> code that require these special cases.

And I've even found a few more interfaces that don't have corresponding idl
files.

But I agree that it's far from optimal, though it's also far from unique.
CodeGeneratorV8 and the other generator files are full of special cases for
various interfaces.

https://codereview.chromium.org/14456006/diff/5001/Source/core/scripts/InFile...
File Source/core/scripts/InFilesCompiler.pm (right):

https://codereview.chromium.org/14456006/diff/5001/Source/core/scripts/InFile...
Source/core/scripts/InFilesCompiler.pm:41: use lib
"$FindBin::Bin/../../../../../third_party/WebKit/Source/bindings/scripts";  #
use the binding generator directory.
On 2013/04/25 14:14:24, abarth wrote:
> Woah there.  The code shouldn't know that it's contained in a
> "third_party/WebKit" directory.  That's likely to change in the future.  We
> don't want to hard-code that information into the code.  That makes the system
> brittle and harder to change in the future.

Someone will need to know how to find this piece of code, but I'll find a better
way.

[Daniel Bratell, 2013-04-25 21:26:40]

https://codereview.chromium.org/14456006/diff/5001/Source/bindings/scripts/ge...
File Source/bindings/scripts/generate-bindings.pl (right):

https://codereview.chromium.org/14456006/diff/5001/Source/bindings/scripts/ge...
Source/bindings/scripts/generate-bindings.pl:73: $ENV{"IDLTOPATHFILE"} =
$idlToPathFile;
On 2013/04/25 21:24:26, bratell wrote:
> On 2013/04/25 14:14:24, abarth wrote:
> > We should find a way to avoid messing with the environment.  Explicit data
> flow
> > is much better than implicit data flow via global variables.
> 
> This is the part where I just couldn't find a good solution. Maybe because I
> don't know Perl well enough. But I can keep digging.

I did change it to use an explicit init function. Seems to work and is much less
magic.

https://codereview.chromium.org/14456006/diff/5001/Source/core/scripts/InFile...
File Source/core/scripts/InFilesCompiler.pm (right):

https://codereview.chromium.org/14456006/diff/5001/Source/core/scripts/InFile...
Source/core/scripts/InFilesCompiler.pm:41: use lib
"$FindBin::Bin/../../../../../third_party/WebKit/Source/bindings/scripts";  #
use the binding generator directory.
On 2013/04/25 21:24:26, bratell wrote:
> On 2013/04/25 14:14:24, abarth wrote:
> > Woah there.  The code shouldn't know that it's contained in a
> > "third_party/WebKit" directory.  That's likely to change in the future.  We
> > don't want to hard-code that information into the code.  That makes the
system
> > brittle and harder to change in the future.
> 
> Someone will need to know how to find this piece of code, but I'll find a
better
> way.

-I flags to the perl executable was a better way.

[Daniel Bratell, 2013-05-03 14:19:49]

I've done some measurements of my own and at best this saves 0.5% compilation
time of the compilation of webcore_derived, at worse it has no effect at all.

Which means I need to keep looking for what part of my old 5000 file patch saved
those last 10-20% compilation time.

The current effect of this patch now is to add some complexity in the build
system and remove some FIXMEs so not very important things.

[Daniel Bratell, 2013-08-21 13:41:17]

On 2013/05/03 14:19:49, bratell wrote:
> I've done some measurements of my own and at best this saves 0.5% compilation
> time of the compilation of webcore_derived, at worse it has no effect at all.
> 
> Which means I need to keep looking for what part of my old 5000 file patch
saved
> those last 10-20% compilation time.
> 
> The current effect of this patch now is to add some complexity in the build
> system and remove some FIXMEs so not very important things.

I think this is good enough now and with the conversion to the python hopefully
as good as it should be. Closing.