MD WebUI: Strip comments from vulcanized JS files

MD WebUI: Strip comments from vulcanized JS files

This adds a simple JS processor to the vulcanize script which strips
most comments from the javascript output. In the interest of simplicity
and correctness, it deliberately does not attempt to strip every
comments. However, the result is still enough to reduce the size of
resources.pak by almost 300Kb.

BUG=629406
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

[tsergeant, 2016-08-17 04:09:17]

Description was changed from

==========
MD WebUI: Strip comments from vulcanized JS files

This adds a simple JS processor to the vulcanize script which strips
most comments from the javascript output. In the interest of simplicity
and correctness, it deliberately does not attempt to strip every
comments. However, the result is still enough to reduce the size of
resources.pak by almost 300Kb.

BUG=629406
==========

to

==========
MD WebUI: Strip comments from vulcanized JS files

This adds a simple JS processor to the vulcanize script which strips
most comments from the javascript output. In the interest of simplicity
and correctness, it deliberately does not attempt to strip every
comments. However, the result is still enough to reduce the size of
resources.pak by almost 300Kb.

BUG=629406
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tsergeant, 2016-08-17 04:25:47]

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

[commit-bot: I haz the power, 2016-08-17 04:26:01]

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-08-17 05:15:24]

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

[commit-bot: I haz the power, 2016-08-17 05:15:26]

Dry run: This issue passed the CQ dry run.

[tsergeant, 2016-08-17 05:18:18]

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

[tsergeant, 2016-08-17 05:18:19]

PTAL.

Doing this manually works almost as well as uglify (which saves an extra 40K or
so), but since we're not reformatting the entire file it's much nicer on git and
easier to reason about what is changing.

[michaelpg, 2016-08-17 07:39:55]

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

[michaelpg, 2016-08-17 07:39:56]

I think it's extremely... brave... to add a hand-rolled comment parser to a
manually-run script...

https://codereview.chromium.org/2252963002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/vulcanize.py (right):

https://codereview.chromium.org/2252963002/diff/40001/chrome/browser/resource...
chrome/browser/resources/vulcanize.py:57: keep_comment =
re.compile(r'Copyright|license|LICENSE|\<\/?if')
ugh, grit works inside comments? what about
    // <include

[tsergeant, 2016-08-17 07:57:52]

Surely a manually-run script is better for this than an automatic one? It's easy
to eyeball the vulcanize output during code review. The parser also deliberately
avoids any cases which feel likely to be error prone.

It's not great that I've had to hand-roll a comment parser, but considering the
benefits and alternatives, I think it's a reasonable trade-off.

https://codereview.chromium.org/2252963002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/vulcanize.py (right):

https://codereview.chromium.org/2252963002/diff/40001/chrome/browser/resource...
chrome/browser/resources/vulcanize.py:57: keep_comment =
re.compile(r'Copyright|license|LICENSE|\<\/?if')
On 2016/08/17 07:39:56, michaelpg wrote:
> ugh, grit works inside comments? what about
>     // <include

The commented-if is used in one place in util.js. You can check with dbeam for
why it is commented out.

Includes have already been explicitly disabled by this stage so they don't need
to be handled here.

[Dan Beam, 2016-08-17 17:42:58]

wait, why not uglify?  because it munges too hard?

[tsergeant, 2016-08-18 00:07:36]

On 2016/08/17 17:42:58, Dan Beam wrote:
> wait, why not uglify?  because it munges too hard?

Yeah, uglify completely rewrites the file according to its own style rules. I
was worried that this was going to cause a lot of churn in the crisper file,
where small changes in input could cause big changes in output because of uglify
deciding to move things around. Manually stripping comments should be more
reproducible and closer to the source input.

In practice, there's probably not much difference and uglify gives slightly
better minification, so ¯\_(ツ)_/¯

[Dan Beam, 2016-08-18 01:46:40]

On 2016/08/18 00:07:36, tsergeant wrote:
> On 2016/08/17 17:42:58, Dan Beam wrote:
> > wait, why not uglify?  because it munges too hard?
> 
> Yeah, uglify completely rewrites the file according to its own style rules. I
> was worried that this was going to cause a lot of churn in the crisper file,
> where small changes in input could cause big changes in output because of
uglify
> deciding to move things around. Manually stripping comments should be more
> reproducible and closer to the source input.
> 
> In practice, there's probably not much difference and uglify gives slightly
> better minification, so ¯\_(ツ)_/¯

can we just use uglify --source-map and maybe check in the source map?

[tsergeant, 2016-08-18 05:23:15]

On 2016/08/18 01:46:40, Dan Beam wrote:
> On 2016/08/18 00:07:36, tsergeant wrote:
> > On 2016/08/17 17:42:58, Dan Beam wrote:
> > > wait, why not uglify?  because it munges too hard?
> > 
> > Yeah, uglify completely rewrites the file according to its own style rules.
I
> > was worried that this was going to cause a lot of churn in the crisper file,
> > where small changes in input could cause big changes in output because of
> uglify
> > deciding to move things around. Manually stripping comments should be more
> > reproducible and closer to the source input.
> > 
> > In practice, there's probably not much difference and uglify gives slightly
> > better minification, so ¯\_(ツ)_/¯
> 
> can we just use uglify --source-map and maybe check in the source map?

Chatting to Chris, we've decided to go with Uglify. I don't think there's any
particular need for source maps (the output is still human-readable), but that
might be something useful to think about in the future.

I'll send a separate CL for uglify.