Strip comments and whitespace from Javascript resources

Strip comments and whitespace from Javascript resources

This CL creates the infrastructure to strip comments and spaces from
Javascript resources, and uses this infrastructure on
the components and browser resources, hence reducing the size of
each pak file by around 20% (total reduction ~340KB)

The CL uses the Closure compiler to strip spaces and comments. Because
this may be slow, the CL uses ninja (via GN) to only run it on changed
Javascript resources. To do this the resources are processed in three
stages:
1) Strip <include..> and <if...> blocks. These are not part of Javascript
and can't be handled by the compiler.
2) Run the Closure compiler
3) Run grit, creating the resource file.

To enable this sequence this CL adds an argument to grit that tells it the
location of preprocessed (and hence moved) files.

At present this is only implemented on Android builds, since the builders
for other platforms may not have Java, which is required to run
the closure compiler.

BUG=619091

[aberent, 2016-06-27 14:53:53]

Description was changed from

==========
Strip comments and whitespace from Javascript resources

This CL creates the infrastructure to strip comments and spaces from
Javascript resources, and uses this infrastructure on
the components resources, hence reducing the size of
component_resources.pak by around 20%

BUG=619091
==========

to

==========
Strip comments and whitespace from Javascript resources

This CL creates the infrastructure to strip comments and spaces from
Javascript resources, and uses this infrastructure on
the components resources, hence reducing the size of
component_resources.pak by around 20%

The CL uses the Closure compiler to strip spaces and comments. Because
this may be slow, the CL uses ninja (via GN) to only run it on changed
Javascript resources. To do this the resources are processed in three
stages:
1) Strip <include..> and <if...> blocks. These are not part of Javascript
and can't be handled by the compiler.
2) Run the Closure compiler
3) Run grit, creating the resource file.

To enable this sequence this CL adds an argument to grit that tells it the
location of preprocessed (and hence moved) files.

BUG=619091
==========

[aberent, 2016-06-27 15:11:42]

aberent@chromium.org changed reviewers:
+ jochen@chromium.org, thestig@chromium.org

[aberent, 2016-06-27 15:11:42]

jochen@chromium.org: Please review changes in components/resources

thestig@chromium.org: Please review changes in tools/grit

This is an initial version; probably needs some new tests. Also I think this
should probably only be run for release builds (and possibly only on Android,
where we are really worried about PAK file size), since it could make debugging
Javascript problems more difficult. Please, however, comment on at least its
general direction. 

Adding brettw@ and dpranke@ to the CC list since this is my first complex use of
GN, and I would welcome comments on whether I am using it correctly. Also cc'ing
people who have commented on the bug.

[aberent, 2016-06-27 16:25:22]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-27 16:25:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-27 16:41:50]

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

[commit-bot: I haz the power, 2016-06-27 16:41:51]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[aberent, 2016-06-27 17:07:09]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-27 17:07:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-27 17:53:47]

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

[commit-bot: I haz the power, 2016-06-27 17:53:48]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Dirk Pranke, 2016-06-27 23:25:26]

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

[Dirk Pranke, 2016-06-27 23:25:27]

+dbeam

There's an ongoing effort to use the Closure compiler more broadly that dbeam@
has been working on. Have you talked to him about this?

Separate from that, I think that we only currently guarantee that java is
available on linux and on builders configured for ios; in particular, I don't
think java is installed on windows by default, which is why all of your windows
tryjobs failed.

So we'd probably need to decide that we're okay with requiring java on windows
to build before landing this ...

[Dan Beam, 2016-06-28 02:45:17]

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

[Dan Beam, 2016-06-28 02:45:18]

you should probably merge logic to handle <if> and <include> with
https://cs.chromium.org/chromium/src/third_party/closure_compiler/processor.p...
if it's helpful (but we didn't use grit directly).

also, as dpranke@ said: we do use closure in other places and that code has a
lot of stuff dialed in already (checks for java, sets up right flags). 
unfortunately, it's also all still GYP.

for what it's worth: we don't currently use the product of closure compilation
(the output), we just run it for typechecking but it's probably possible to make
it suit your needs.

lastly: yeah, the way I've implemented Closure in Chrome has never made Java
required to build Chrome (it's always been on bots or require locally for web
development in Chrome, i.e. a few teams, not the whole org).  You may hit
resistance to requiring Java to build Chrome.

[aberent, 2016-06-28 08:54:02]

On 2016/06/28 02:45:18, Dan Beam wrote:
> you should probably merge logic to handle <if> and <include> with
>
https://cs.chromium.org/chromium/src/third_party/closure_compiler/processor.p...
> if it's helpful (but we didn't use grit directly).

I will take a look at that. At a glance it does look as if it is solving the
same
problem.
> 
> also, as dpranke@ said: we do use closure in other places and that code has a
> lot of stuff dialed in already (checks for java, sets up right flags). 
> unfortunately, it's also all still GYP.

I did have a look for uses of the Closure compiler, but the bits I found looked
inappropriate for my use (they used heavy type checking, whereas I am trying to
do the minimum that will strip comments and whitespace). Also, as you say, they
were GYP, and, for Android (and, in future, for all platforms) this needs to be
in GN.

The only GN use I found of the compiler was IOS specific 
(https://cs.chromium.org/chromium/src/ios/web/js_compile.gni?q=js_com&sq=packa...),
and didn't look reusable without significant surgery.
 
> 
> for what it's worth: we don't currently use the product of closure compilation
> (the output), we just run it for typechecking but it's probably possible to
make
> it suit your needs.

How close is our Javascript to passing these checks? My worry about using this
was that I would end up having to solve all the typecheck failures before
landing
this CL. This could seriously increase the scope of this CL.

I was also slightly worried that it appears from https://crbug/78368 that no
significant work has been done on this for 18 months.
> 
> lastly: yeah, the way I've implemented Closure in Chrome has never made Java
> required to build Chrome (it's always been on bots or require locally for web
> development in Chrome, i.e. a few teams, not the whole org).  You may hit
> resistance to requiring Java to build Chrome.

As far as I know, the strongest case for doing this is actually on Android, 
where APK size is a  significant issue, and (for various good reasons) the
PAK files are stored uncompressed in the APK. Java is already required for
Android builds so we could, for now, simply restrict this to Android.

Longer term we probably do need to start a discussion about how important
PAK size is on desktop, and whether it justifies requiring Java. All my
work on Chrome has been in the Clank team, so I have no real feel for
this. What do you think?

[aberent, 2016-06-28 09:54:49]

On 2016/06/28 08:54:02, aberent wrote:
> On 2016/06/28 02:45:18, Dan Beam wrote:
> > you should probably merge logic to handle <if> and <include> with
> >
>
https://cs.chromium.org/chromium/src/third_party/closure_compiler/processor.p...
> > if it's helpful (but we didn't use grit directly).
> 

I don't think I can use this as it is now. In particular it seems it 
doesn't evaluate the  conditionals in the <if..> blocks, but simply 
removes them. For my use I need to evaluate them correctly, so that 
the right thing goes in the PAK files.

My current <include..> and <if..> remover is just a wrapper round 
html_inline.py 
(https://cs.chromium.org/chromium/src/tools/grit/grit/format/html_inline.py?q=...).
This is an existing component of grit, so merging the logic would mean
merging processor.py into grit. This seems out of scope for the current CL.

[jochen (gone - plz use gerrit), 2016-06-28 13:14:41]

will rubberstamp components once the main part of the CL is reviewed

[aberent, 2016-06-28 14:02:53]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-28 14:03:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 14:06:45]

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

[commit-bot: I haz the power, 2016-06-28 14:06:46]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[aberent, 2016-06-28 14:33:06]

The CQ bit was checked by aberent@chromium.org

[aberent, 2016-06-28 14:33:07]

The CQ bit was unchecked by aberent@chromium.org

[aberent, 2016-06-28 14:33:12]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-28 14:33:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 15:17:38]

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

[commit-bot: I haz the power, 2016-06-28 15:17:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Dan Beam, 2016-06-28 19:25:28]

On 2016/06/28 09:54:49, aberent wrote:
> On 2016/06/28 08:54:02, aberent wrote:
> > On 2016/06/28 02:45:18, Dan Beam wrote:
> > > you should probably merge logic to handle <if> and <include> with
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/closure_compiler/processor.p...
> > > if it's helpful (but we didn't use grit directly).
> > 
> 
> I don't think I can use this as it is now. In particular it seems it 
> doesn't evaluate the  conditionals in the <if..> blocks, but simply 
> removes them. For my use I need to evaluate them correctly, so that 
> the right thing goes in the PAK files.

yeah, you're right: in typechecking we want to ignore <if>, and <include> likely
isn't useful because we keep per-line info on the original file for mapping to
the source line.  sorry, forgot about these differences, cool to keep separate.

> 
> My current <include..> and <if..> remover is just a wrapper round 
> html_inline.py 
>
(https://cs.chromium.org/chromium/src/tools/grit/grit/format/html_inline.py?q=...).
> This is an existing component of grit, so merging the logic would mean
> merging processor.py into grit. This seems out of scope for the current CL.

[aberent, 2016-06-28 19:28:29]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[Dan Beam, 2016-06-28 19:28:44]

On 2016/06/28 08:54:02, aberent wrote:
> On 2016/06/28 02:45:18, Dan Beam wrote:
> > you should probably merge logic to handle <if> and <include> with
> >
>
https://cs.chromium.org/chromium/src/third_party/closure_compiler/processor.p...
> > if it's helpful (but we didn't use grit directly).
> 
> I will take a look at that. At a glance it does look as if it is solving the
> same
> problem.
> > 
> > also, as dpranke@ said: we do use closure in other places and that code has
a
> > lot of stuff dialed in already (checks for java, sets up right flags). 
> > unfortunately, it's also all still GYP.
> 
> I did have a look for uses of the Closure compiler, but the bits I found
looked
> inappropriate for my use (they used heavy type checking, whereas I am trying
to
> do the minimum that will strip comments and whitespace). Also, as you say,
they
> were GYP, and, for Android (and, in future, for all platforms) this needs to
be
> in GN.
> 
> The only GN use I found of the compiler was IOS specific 
>
(https://cs.chromium.org/chromium/src/ios/web/js_compile.gni?q=js_com&sq=packa...),
> and didn't look reusable without significant surgery.
>  
> > 
> > for what it's worth: we don't currently use the product of closure
compilation
> > (the output), we just run it for typechecking but it's probably possible to
> make
> > it suit your needs.
> 
> How close is our Javascript to passing these checks? My worry about using this
> was that I would end up having to solve all the typecheck failures before
> landing
> this CL. This could seriously increase the scope of this CL.

you could probably [fairly easily] override our closure_args
https://cs.chromium.org/chromium/src/third_party/closure_compiler/closure_arg...

^ we used to have a GN version but I think it's bitrotten now

> 
> I was also slightly worried that it appears from https://crbug/78368 that no
> significant work has been done on this for 18 months.

not the case at all ;)

> > 
> > lastly: yeah, the way I've implemented Closure in Chrome has never made Java
> > required to build Chrome (it's always been on bots or require locally for
web
> > development in Chrome, i.e. a few teams, not the whole org).  You may hit
> > resistance to requiring Java to build Chrome.
> 
> As far as I know, the strongest case for doing this is actually on Android, 
> where APK size is a  significant issue, and (for various good reasons) the
> PAK files are stored uncompressed in the APK. Java is already required for
> Android builds so we could, for now, simply restrict this to Android.

that's probably your best bet (don't add more build dependencies)

> 
> Longer term we probably do need to start a discussion about how important
> PAK size is on desktop, and whether it justifies requiring Java. All my
> work on Chrome has been in the Clank team, so I have no real feel for
> this. What do you think?

it's important-ish, but far less important than on Android as most installers
are compressed (Mac = xz, Linux = LZMA, Windows = ??? cuz binary updates).

[commit-bot: I haz the power, 2016-06-28 19:29:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aberent, 2016-06-28 19:31:35]

On 2016/06/28 19:25:28, Dan Beam wrote:
> On 2016/06/28 09:54:49, aberent wrote:
> > On 2016/06/28 08:54:02, aberent wrote:
> > > On 2016/06/28 02:45:18, Dan Beam wrote:
> > > > you should probably merge logic to handle <if> and <include> with
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/closure_compiler/processor.p...
> > > > if it's helpful (but we didn't use grit directly).
> > > 
> > 
> > I don't think I can use this as it is now. In particular it seems it 
> > doesn't evaluate the  conditionals in the <if..> blocks, but simply 
> > removes them. For my use I need to evaluate them correctly, so that 
> > the right thing goes in the PAK files.
> 
> yeah, you're right: in typechecking we want to ignore <if>, and <include>
likely
> isn't useful because we keep per-line info on the original file for mapping to
> the source line.  sorry, forgot about these differences, cool to keep
separate.
> 
> > 
> > My current <include..> and <if..> remover is just a wrapper round 
> > html_inline.py 
> >
>
(https://cs.chromium.org/chromium/src/tools/grit/grit/format/html_inline.py?q=...).
> > This is an existing component of grit, so merging the logic would mean
> > merging processor.py into grit. This seems out of scope for the current CL.

PTAL - ready for full review. I have restricted this to Android since we don't
know we have Java available elsewhere, but not to release builds since devtools
prettyprint does quite a good job of making the Javascript readable again.

[aberent, 2016-06-28 19:38:09]

> you could probably [fairly easily] override our closure_args
>
https://cs.chromium.org/chromium/src/third_party/closure_compiler/closure_arg...
> 
> ^ we used to have a GN version but I think it's bitrotten now

Our last comments came in simultaneously. I will take another look at this in
the morning.
> 
> > 
> > I was also slightly worried that it appears from https://crbug/78368 that no
> > significant work has been done on this for 18 months.
> 
> not the case at all ;)

Is there a current tracking bug?

[Dan Beam, 2016-06-28 20:37:45]

On 2016/06/28 19:38:09, aberent wrote:
> > you could probably [fairly easily] override our closure_args
> >
>
https://cs.chromium.org/chromium/src/third_party/closure_compiler/closure_arg...
> > 
> > ^ we used to have a GN version but I think it's bitrotten now
> 
> Our last comments came in simultaneously. I will take another look at this in
> the morning.
> > 
> > > 
> > > I was also slightly worried that it appears from https://crbug/78368 that
no
> > > significant work has been done on this for 18 months.
> > 
> > not the case at all ;)
> 
> Is there a current tracking bug?

ehhhh, not really, but we roll closure often and there's bots:
https://build.chromium.org/p/chromium.fyi/builders/Closure%20Compilation%20Linux
https://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compil...

[Dirk Pranke, 2016-06-28 20:53:33]

On 2016/06/28 20:37:45, Dan Beam wrote:
> On 2016/06/28 19:38:09, aberent wrote:
> > > you could probably [fairly easily] override our closure_args
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/closure_compiler/closure_arg...
> > > 
> > > ^ we used to have a GN version but I think it's bitrotten now
> > 
> > Our last comments came in simultaneously. I will take another look at this
in
> > the morning.
> > > 
> > > > 
> > > > I was also slightly worried that it appears from https://crbug/78368
that
> no
> > > > significant work has been done on this for 18 months.
> > > 
> > > not the case at all ;)
> > 
> > Is there a current tracking bug?
> 
> ehhhh, not really, but we roll closure often and there's bots:
>
https://build.chromium.org/p/chromium.fyi/builders/Closure%20Compilation%20Linux
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compil...

The GYP build (of the rest of Chromium) is going away Real Soon Now, and so we/I
will
be making sure this gets moved over to GN and (possibly) moved onto the main
waterfalls
fairly soon.

[commit-bot: I haz the power, 2016-06-28 21:17:24]

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

[commit-bot: I haz the power, 2016-06-28 21:17:25]

Dry run: This issue passed the CQ dry run.

[aberent, 2016-06-29 10:42:35]

On 2016/06/28 20:37:45, Dan Beam wrote:
> On 2016/06/28 19:38:09, aberent wrote:
> > > you could probably [fairly easily] override our closure_args
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/closure_compiler/closure_arg...
> > > 
> > > ^ we used to have a GN version but I think it's bitrotten now
> > 
> > Our last comments came in simultaneously. I will take another look at this
in
> > the morning.
> > > 
> > > > 
> > > > I was also slightly worried that it appears from https://crbug/78368
that
> no
> > > > significant work has been done on this for 18 months.
> > > 
> > > not the case at all ;)
> > 
> > Is there a current tracking bug?
> 
> ehhhh, not really, but we roll closure often and there's bots:
>
https://build.chromium.org/p/chromium.fyi/builders/Closure%20Compilation%20Linux
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compil...

I don't think using this would actually gain me anything. My current code 
for  running closure is only about 10 lines and invoking the python scripts 
would probably take as much or more. Since I am only doing whitespace 
stripping I don't need complex args, nor do I need to expand the file for
includes or ifs (I have already done that), and I don't think I want to
filter the error messages. As such, simply running the compiler directly 
as a Java program seems to work best.

This will, of course, change if we ever want to increase the level of
compression; but in the discussion on the bug at least some people were
worried about doing so because of the difficulties of debugging compressed
Javascript. It could also change if we wanted to do Javascript type checking
as part of our standard builds; but before we did that we would, I think,
need to sort out the requirement for Java on Windows.

[aberent, 2016-06-29 20:01:15]

Description was changed from

==========
Strip comments and whitespace from Javascript resources

This CL creates the infrastructure to strip comments and spaces from
Javascript resources, and uses this infrastructure on
the components resources, hence reducing the size of
component_resources.pak by around 20%

The CL uses the Closure compiler to strip spaces and comments. Because
this may be slow, the CL uses ninja (via GN) to only run it on changed
Javascript resources. To do this the resources are processed in three
stages:
1) Strip <include..> and <if...> blocks. These are not part of Javascript
and can't be handled by the compiler.
2) Run the Closure compiler
3) Run grit, creating the resource file.

To enable this sequence this CL adds an argument to grit that tells it the
location of preprocessed (and hence moved) files.

BUG=619091
==========

to

==========
Strip comments and whitespace from Javascript resources

This CL creates the infrastructure to strip comments and spaces from
Javascript resources, and uses this infrastructure on
the components and browser resources, hence reducing the size of
each pak file by around 20% (total reduction ~340KB)

The CL uses the Closure compiler to strip spaces and comments. Because
this may be slow, the CL uses ninja (via GN) to only run it on changed
Javascript resources. To do this the resources are processed in three
stages:
1) Strip <include..> and <if...> blocks. These are not part of Javascript
and can't be handled by the compiler.
2) Run the Closure compiler
3) Run grit, creating the resource file.

To enable this sequence this CL adds an argument to grit that tells it the
location of preprocessed (and hence moved) files.

At present this is only implemented on Android builds, since the builders
for other platforms may not have Java, which is required to run
the closure compiler.

BUG=619091
==========

[aberent, 2016-06-29 20:03:35]

PTAL. Note that I have added stripping browser resources to the CL. This saves a
further 220KB.

[Dan Beam, 2016-06-29 22:20:57]

On 2016/06/29 10:42:35, aberent wrote:
> On 2016/06/28 20:37:45, Dan Beam wrote:
> > On 2016/06/28 19:38:09, aberent wrote:
> > > > you could probably [fairly easily] override our closure_args
> > > > 
> > > 
> >
>
https://cs.chromium.org/chromium/src/third_party/closure_compiler/closure_arg...
> > > > 
> > > > ^ we used to have a GN version but I think it's bitrotten now 
> > > 
> > > Our last comments came in simultaneously. I will take another look at this
> in
> > > the morning.
> > > > 
> > > > > 
> > > > > I was also slightly worried that it appears from https://crbug/78368
> that
> > no
> > > > > significant work has been done on this for 18 months.
> > > > 
> > > > not the case at all ;)
> > > 
> > > Is there a current tracking bug?
> > 
> > ehhhh, not really, but we roll closure often and there's bots:
> >
>
https://build.chromium.org/p/chromium.fyi/builders/Closure%20Compilation%20Linux
> >
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compil...
> 
> I don't think using this would actually gain me anything. My current code 
> for  running closure is only about 10 lines and invoking the python scripts 
> would probably take as much or more. Since I am only doing whitespace 
> stripping I don't need complex args,

it's fairly easy to turn these off in the current framework or in the
[semi-unused] GN version

you'd just assign over the existing args, i.e.

  closure_args = ['--compilation_level=WHITESPACE_ONLY']

in GN (rather than closure_args += [...])

> nor do I need to expand the file for 
> includes or ifs (I have already done that), and I don't think I want to
> filter the error messages. As such, simply running the compiler directly 
> as a Java program seems to work best.

i still think it may make sense to just turn off the functionality you don't
need from third_party/closure_compiler but still share code with it

it's always been a goal to reduce the size and scope of the supporting python in
third_party/closure_compiler to make it basically like running compiler.jar
itself.  it's also a goal to convert to GN.  instead of helping either of those,
you're making your own version :(.

> 
> This will, of course, change if we ever want to increase the level of
> compression; but in the discussion on the bug at least some people were
> worried about doing so because of the difficulties of debugging compressed
> Javascript. It could also change if we wanted to do Javascript type checking
> as part of our standard builds; but before we did that we would, I think,
> need to sort out the requirement for Java on Windows.

i would highly recommend making source maps and archiving them, similar to
symbols, before just making all of our JS on Android into 30k char/1 line files

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit_rule.gni
File tools/grit/grit_rule.gni (right):

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit_rule.g...
tools/grit/grit_rule.gni:537: java_action_foreach(_target_name) {
does this run in an optimized way, i.e.

https://cs.chromium.org/chromium/src/third_party/closure_compiler/compile2.py...

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit_rule.g...
tools/grit/grit_rule.gni:549:
"$root_out_dir/{{source_gen_dir}}/{{source_name_part}}_${_target_name}.js"),
what about source maps?

third_party/closure_compiler/compile{,2}.py already do this

https://cs.chromium.org/chromium/src/third_party/closure_compiler/compile2.py...

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit_rule.g...
tools/grit/grit_rule.gni:552: "--language_out=ES5",
don't you want a --language_in as well?

[Lei Zhang, 2016-06-29 22:51:32]

https://codereview.chromium.org/2094193004/diff/120001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2094193004/diff/120001/chrome/browser/BUILD.g...
chrome/browser/BUILD.gn:1025: js_resource_files = [
So where does this list come from? How do we maintain it going forward?

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit/flatte...
File tools/grit/grit/flatten_resource_runner.py (right):

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit/flatte...
tools/grit/grit/flatten_resource_runner.py:42: return open(string, 'w')
Probably want 'wb' ?

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit/flatte...
tools/grit/grit/flatten_resource_runner.py:43: except (IOError , OSError) as e:
nit: no space before comma

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit/flatte...
tools/grit/grit/flatten_resource_runner.py:63: to the value of sys.platform. The
value provided via this
nit: I don't have Python readability, so what do I know, but this indentation
looks odd.

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit/flatte...
tools/grit/grit/flatten_resource_runner.py:84: with parsed.output as out:
Why not just parsed.output.write(...) ?

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit/tool/b...
File tools/grit/grit/tool/build.py (right):

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit/tool/b...
tools/grit/grit/tool/build.py:5: from grit.util import RAW_TEXT
Why is this on top?

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit/tool/b...
tools/grit/grit/tool/build.py:131: The argument is of the form 
<original-path>:<actual-path>.
nit: extra space, or s/  /: /

[brettw, 2016-07-01 20:22:31]

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

[brettw, 2016-07-01 20:22:32]

I haven't really thought about the details of this patch, so take this to be a
general comment looking at the activity rather than expressing specific opinions
about this patch...

Given the number of comments here and on the bug, I suspect this might be a good
thing for a design doc. The comments on these types of forums aren't necessarily
the best way to  hash out how the lists of JS files are maintained and what the
platform story is (it's not clear if this can even be done outside of Android).

This type of change is a great thing to think about but it does seem like there
are a lot of annoying implementation details in practice. I don't want to waste
your time tweaking a CL in response to the whims of all of the comments on the
CL.

I think this will be a good thing for the new design doc process in the email
that went out to the team yesterday. From there, I suspect this CL can be
rapidly resolved.

In particular, from skimming the various comments about this change, I would
suggest covering explicitly:

 - Platform story (will this stay Android only or is there a path for other
platforms?)
 - Debuggability for developers writing such JS code
 - How file lists are maintained (it seems this adds a lot of lists of JS files)

[aberent, 2016-07-05 14:59:36]

On 2016/07/01 20:22:32, brettw (plz ping after 24h) wrote:
> I haven't really thought about the details of this patch, so take this to be a
> general comment looking at the activity rather than expressing specific
opinions
> about this patch...
> 
> Given the number of comments here and on the bug, I suspect this might be a
good
> thing for a design doc. The comments on these types of forums aren't
necessarily
> the best way to  hash out how the lists of JS files are maintained and what
the
> platform story is (it's not clear if this can even be done outside of
Android).
> 
> This type of change is a great thing to think about but it does seem like
there
> are a lot of annoying implementation details in practice. I don't want to
waste
> your time tweaking a CL in response to the whims of all of the comments on the
> CL.
> 
> I think this will be a good thing for the new design doc process in the email
> that went out to the team yesterday. From there, I suspect this CL can be
> rapidly resolved.
> 
> In particular, from skimming the various comments about this change, I would
> suggest covering explicitly:
> 
>  - Platform story (will this stay Android only or is there a path for other
> platforms?)
>  - Debuggability for developers writing such JS code
>  - How file lists are maintained (it seems this adds a lot of lists of JS
files)

See
https://docs.google.com/document/d/1XUte4m_wkPwQ9I-MfVZfBhksoL0E2_7Eb7N-PkXQK...

[aberent, 2016-07-14 15:41:36]

Responded to most comments (although missed one from thestig@, will fix on next
pass). I haven't yet done anything about checking that the GN and grit file
lists match, but will do so (so there is, at least, one more patchset to come on
this CL).

I have added compile_js2.gni to run closure, based on the GYP equivalent, but
only implemented the parts of it I need, since the other parts are not yet used
from GN.

https://codereview.chromium.org/2094193004/diff/120001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2094193004/diff/120001/chrome/browser/BUILD.g...
chrome/browser/BUILD.gn:1025: js_resource_files = [
On 2016/06/29 22:51:32, Lei Zhang wrote:
> So where does this list come from? How do we maintain it going forward?

I got it originally by running grit buildinfo, but it will need to be maintained
manually. This is no different from any other source files; our GN files list
all our C++ and Java source and header files (and, in fact, for Java, as part of
the GN conversion, we removed the ability to find source files automatically).

There is a possible issue that we currently have no automatic check that this
list is complete, but this also true of C++ header files.

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit/flatte...
File tools/grit/grit/flatten_resource_runner.py (right):

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit/flatte...
tools/grit/grit/flatten_resource_runner.py:42: return open(string, 'w')
On 2016/06/29 22:51:32, Lei Zhang wrote:
> Probably want 'wb' ?

Done.

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit/flatte...
tools/grit/grit/flatten_resource_runner.py:43: except (IOError , OSError) as e:
On 2016/06/29 22:51:32, Lei Zhang wrote:
> nit: no space before comma

Done.

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit/flatte...
tools/grit/grit/flatten_resource_runner.py:63: to the value of sys.platform. The
value provided via this
On 2016/06/29 22:51:32, Lei Zhang wrote:
> nit: I don't have Python readability, so what do I know, but this indentation
> looks odd.

I think I hit an odd corner in Eclipse's python formatting. Done.

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit/flatte...
tools/grit/grit/flatten_resource_runner.py:84: with parsed.output as out:
On 2016/06/29 22:51:32, Lei Zhang wrote:
> Why not just parsed.output.write(...) ?

Done.

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit/tool/b...
File tools/grit/grit/tool/build.py (right):

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit/tool/b...
tools/grit/grit/tool/build.py:131: The argument is of the form 
<original-path>:<actual-path>.
On 2016/06/29 22:51:32, Lei Zhang wrote:
> nit: extra space, or s/  /: /

Comment was wrong in other ways as well. Updated.

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit_rule.gni
File tools/grit/grit_rule.gni (right):

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit_rule.g...
tools/grit/grit_rule.gni:537: java_action_foreach(_target_name) {
On 2016/06/29 22:20:57, Dan Beam wrote:
> does this run in an optimized way, i.e.
> 
>
https://cs.chromium.org/chromium/src/third_party/closure_compiler/compile2.py...

Now it does. See compile_js2.gni

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit_rule.g...
tools/grit/grit_rule.gni:549:
"$root_out_dir/{{source_gen_dir}}/{{source_name_part}}_${_target_name}.js"),
On 2016/06/29 22:20:57, Dan Beam wrote:
> what about source maps?
> 
> third_party/closure_compiler/compile{,2}.py already do this
> 
>
https://cs.chromium.org/chromium/src/third_party/closure_compiler/compile2.py...

Done.

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit_rule.g...
tools/grit/grit_rule.gni:552: "--language_out=ES5",
On 2016/06/29 22:20:57, Dan Beam wrote:
> don't you want a --language_in as well?

Done. See compile_js2.gni

[Dan Beam, 2016-07-14 17:47:59]

https://codereview.chromium.org/2094193004/diff/120001/components/resources/B...
File components/resources/BUILD.gn (right):

https://codereview.chromium.org/2094193004/diff/120001/components/resources/B...
components/resources/BUILD.gn:64: "//ui/webui/resources/js/util.js",
how are these components/ resources?

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit/format...
File tools/grit/grit/format/html_inline.py (right):

https://codereview.chromium.org/2094193004/diff/120001/tools/grit/grit/format...
tools/grit/grit/format/html_inline.py:142: evaluate_condition: a function to
evaluate boolean conditions
this isn't really too helpful of a variable name, maybe expr_predicate or
something?

https://codereview.chromium.org/2094193004/diff/140001/third_party/closure_co...
File third_party/closure_compiler/compile2.py (right):

https://codereview.chromium.org/2094193004/diff/140001/third_party/closure_co...
third_party/closure_compiler/compile2.py:56: return
remove

https://codereview.chromium.org/2094193004/diff/140001/tools/grit/grit/flatte...
File tools/grit/grit/flatten_resource_runner.py (right):

https://codereview.chromium.org/2094193004/diff/140001/tools/grit/grit/flatte...
tools/grit/grit/flatten_resource_runner.py:85: preprocess_only=True))
indent off

https://codereview.chromium.org/2094193004/diff/140001/tools/grit/grit/flatte...
File tools/grit/grit/flatten_resource_runner_unittest.py (right):

https://codereview.chromium.org/2094193004/diff/140001/tools/grit/grit/flatte...
tools/grit/grit/flatten_resource_runner_unittest.py:8: if __name__ ==
'__main__':
why do you need to do this only if __name__ == '__main__'?  isn't this for the
"from grit import flatten_resource_runner" line below?

https://codereview.chromium.org/2094193004/diff/140001/tools/grit/grit/flatte...
tools/grit/grit/flatten_resource_runner_unittest.py:9:
sys.path.append(os.path.join(os.path.dirname(__file__), '..'))
can you just do this in the other if __name__ == '__main__' block?

https://codereview.chromium.org/2094193004/diff/140001/tools/grit/grit/flatte...
tools/grit/grit/flatten_resource_runner_unittest.py:12: import tempfile
nit: alpha

https://codereview.chromium.org/2094193004/diff/140001/tools/grit/grit/flatte...
tools/grit/grit/flatten_resource_runner_unittest.py:22:
tempfile.NamedTemporaryFile() as output_file:
where do these get deleted?

https://codereview.chromium.org/2094193004/diff/140001/tools/grit/grit/flatte...
tools/grit/grit/flatten_resource_runner_unittest.py:25: input_file.seek(0)
what's this seek(0) for?  can we just close() instead?

https://codereview.chromium.org/2094193004/diff/140001/tools/grit/grit_rule.gni
File tools/grit/grit_rule.gni (right):

https://codereview.chromium.org/2094193004/diff/140001/tools/grit/grit_rule.g...
tools/grit/grit_rule.gni:563: # Generating the actual paths is messy, because,
to avoid name clashes both the
80 col wrap

https://codereview.chromium.org/2094193004/diff/140001/ui/login/screen.js
File ui/login/screen.js (right):

https://codereview.chromium.org/2094193004/diff/140001/ui/login/screen.js#new...
ui/login/screen.js:388: login[name] = api;  // exposes globally
you probably don't need to copy this comments, or if you want to: capitalize and
end with .

[Lei Zhang, 2016-07-15 00:13:09]

https://codereview.chromium.org/2094193004/diff/120001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2094193004/diff/120001/chrome/browser/BUILD.g...
chrome/browser/BUILD.gn:1025: js_resource_files = [
On 2016/07/14 15:41:36, aberent wrote:
> On 2016/06/29 22:51:32, Lei Zhang wrote:
> > So where does this list come from? How do we maintain it going forward?
> 
> I got it originally by running grit buildinfo, but it will need to be
maintained
> manually. This is no different from any other source files; our GN files list
> all our C++ and Java source and header files (and, in fact, for Java, as part
of
> the GN conversion, we removed the ability to find source files automatically).
> 
> There is a possible issue that we currently have no automatic check that this
> list is complete, but this also true of C++ header files.

The question of how to maintain this list going forward still stands. What's the
process for developers to follow here?

Also, what about chrome/browser/resources/certificate_viewer.js as an example,
where in the .grd file, it's behind an <if> condition? Do we just say meh and
process it anyway?

[brettw, 2016-07-15 17:05:14]

I still stand by my comments on the design doc that maintaining these lists
twice like this is not a good idea.

My suggestion, which I agree isn't perfect but which I still think is better
than this patch, is to have Grit filter .js files through a program you control
and run this only in official builds.

[Dirk Pranke, 2016-07-15 19:12:40]

For the record, I'm staying out of this as there are enough people looking at
this already. I defer to brettw@ here.

[aberent, 2016-07-18 14:55:21]

On 2016/07/15 17:05:14, brettw (ping after 24h) wrote:
> I still stand by my comments on the design doc that maintaining these lists
> twice like this is not a good idea.
> 
> My suggestion, which I agree isn't perfect but which I still think is better
> than this patch, is to have Grit filter .js files through a program you
control
> and run this only in official builds.

Ok, I will take another look at that option. I do, however, have a some concerns
about it:
1 - I am not sure exactly how much it will slow down official builds, but I
think it will slow them down significantly.
2 - Our bots only build official builds a few times a day, so if the process
somehow goes wrong and causes bugs or build errors (e.g. when a Javascript file
changes) then these will be discovered late.

An alternative CL will follow.

[aberent, 2016-07-25 17:01:45]

On 2016/07/18 14:55:21, aberent wrote:
> On 2016/07/15 17:05:14, brettw (ping after 24h) wrote:
> > I still stand by my comments on the design doc that maintaining these lists
> > twice like this is not a good idea.
> > 
> > My suggestion, which I agree isn't perfect but which I still think is better
> > than this patch, is to have Grit filter .js files through a program you
> control
> > and run this only in official builds.
> 
> Ok, I will take another look at that option. I do, however, have a some
concerns
> about it:
> 1 - I am not sure exactly how much it will slow down official builds, but I
> think it will slow them down significantly.
> 2 - Our bots only build official builds a few times a day, so if the process
> somehow goes wrong and causes bugs or build errors (e.g. when a Javascript
file
> changes) then these will be discovered late.
> 
> An alternative CL will follow.

The alternative CL is https://codereview.chromium.org/2179033002/