Remove minification spam

Remove minification spam

searchbox_api.js uses a Chrome extension to Javascript. This is not understood by the Closure compiler, so was giving errors when grit attempted to minify it. To prevent this don't minify this file.

BUG=644392
Committed: https://crrev.com/2dfa9b2034b9e982fa119f70cc8d324b0a0a4485
Cr-Commit-Position: refs/heads/master@{#427033}

[aberent, 2016-09-13 18:11:22]

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

[aberent, 2016-09-13 18:13:14]

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

[commit-bot: I haz the power, 2016-09-13 18:13:41]

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

[Lei Zhang, 2016-09-13 18:46:50]

thestig@chromium.org changed reviewers:
+ flackr@chromium.org, thakis@chromium.org

[Lei Zhang, 2016-09-13 18:46:50]

OOO -> deferring to other OWNERS.

[Nico, 2016-09-13 18:51:10]

That sounds like the Wrong Fix (tm) to me.

a) If minification fails, shouldn't that be a hard (exit code != 0) error?
b) Successful builds should be silent.

[commit-bot: I haz the power, 2016-09-13 19:00:49]

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

[commit-bot: I haz the power, 2016-09-13 19:00:50]

Dry run: This issue passed the CQ dry run.

[aberent, 2016-09-15 08:35:22]

On 2016/09/13 18:51:10, Nico wrote:
> That sounds like the Wrong Fix (tm) to me.
> 
> a) If minification fails, shouldn't that be a hard (exit code != 0) error?
There are a few JavaScript files for which we know that minification will
currently fail, because they use Chrome extensions to Javascript. These are not
supported by the Closure compiler, or, as far as I know, by any other JavaScript
minifier. There is an open bug
(https://bugs.chromium.org/p/chromium/issues/detail?id=631937) to fix this, but
it isn't currently being worked on and the work required is non-trivial.
Including them in Chrome unminified is a better solution than either not
minifying anything or causing all Android builds to fail.
> b) Successful builds should be silent.
There is certainly a case for this, however our Android builds (at least) are
currently not silent. We don't, for example, suppress deprecation warnings from
the Java compiler, but nor do we turn these into fatal build errors. Such
warnings remind us of things that should be fixed, but do not currently stop us
building Chrome.

[Nico, 2016-09-15 18:27:44]

On 2016/09/15 08:35:22, aberent wrote:
> On 2016/09/13 18:51:10, Nico wrote:
> > That sounds like the Wrong Fix (tm) to me.
> > 
> > a) If minification fails, shouldn't that be a hard (exit code != 0) error?
> There are a few JavaScript files for which we know that minification will
> currently fail, because they use Chrome extensions to Javascript. These are
not
> supported by the Closure compiler, or, as far as I know, by any other
JavaScript
> minifier. There is an open bug
> (https://bugs.chromium.org/p/chromium/issues/detail?id=631937) to fix this,
but
> it isn't currently being worked on and the work required is non-trivial.
> Including them in Chrome unminified is a better solution than either not
> minifying anything or causing all Android builds to fail.

Sure, but isn't the nicer fix to not run them through the compiler if we know
they're going to fail?

> > b) Successful builds should be silent.
> There is certainly a case for this, however our Android builds (at least) are
> currently not silent. We don't, for example, suppress deprecation warnings
from
> the Java compiler, but nor do we turn these into fatal build errors.

The reason for that is that javac doesn't have a toggle for that, last I looked.

> Such
> warnings remind us of things that should be fixed, but do not currently stop
us
> building Chrome.

[aberent, 2016-09-22 13:00:06]

On 2016/09/15 18:27:44, Nico wrote:
> On 2016/09/15 08:35:22, aberent wrote:
> > On 2016/09/13 18:51:10, Nico wrote:
> > > That sounds like the Wrong Fix (tm) to me.
> > > 
> > > a) If minification fails, shouldn't that be a hard (exit code != 0) error?
> > There are a few JavaScript files for which we know that minification will
> > currently fail, because they use Chrome extensions to Javascript. These are
> not
> > supported by the Closure compiler, or, as far as I know, by any other
> JavaScript
> > minifier. There is an open bug
> > (https://bugs.chromium.org/p/chromium/issues/detail?id=631937) to fix this,
> but
> > it isn't currently being worked on and the work required is non-trivial.
> > Including them in Chrome unminified is a better solution than either not
> > minifying anything or causing all Android builds to fail.
> 
> Sure, but isn't the nicer fix to not run them through the compiler if we know
> they're going to fail?

The issue here is that we don't have an explicit list of which Javascript files
to compile. I originally (see https://codereview.chromium.org/2094193004/ and
https://docs.google.com/document/d/1XUte4m_wkPwQ9I-MfVZfBhksoL0E2_7Eb7N-PkXQK...)
proposed that these should be listed in the GN build files, but after some 
discussion I was persuaded that it was more maintainable to run the compiler
from within grit, and to derive the list of files to be compiled from the 
.grd files, and from the <script> elements in the html files. This was
implemented in https://codereview.chromium.org/2179033002/.

We could have an exclusion list for files that should not be
compiled; but while, in practice, this would be short (two or three files I 
think) it would, in principle, have the same maintenance problems as having
an inclusion list.

> 
> > > b) Successful builds should be silent.
> > There is certainly a case for this, however our Android builds (at least)
are
> > currently not silent. We don't, for example, suppress deprecation warnings
> from
> > the Java compiler, but nor do we turn these into fatal build errors.
> 
> The reason for that is that javac doesn't have a toggle for that, last I
looked.
> 
> > Such
> > warnings remind us of things that should be fixed, but do not currently stop
> us
> > building Chrome.

[flackr, 2016-09-23 14:07:50]

On 2016/09/22 at 13:00:06, aberent wrote:
> On 2016/09/15 18:27:44, Nico wrote:
> > On 2016/09/15 08:35:22, aberent wrote:
> > > On 2016/09/13 18:51:10, Nico wrote:
> > > > That sounds like the Wrong Fix (tm) to me.
> > > > 
> > > > a) If minification fails, shouldn't that be a hard (exit code != 0)
error?
> > > There are a few JavaScript files for which we know that minification will
> > > currently fail, because they use Chrome extensions to Javascript. These
are
> > not
> > > supported by the Closure compiler, or, as far as I know, by any other
> > JavaScript
> > > minifier. There is an open bug
> > > (https://bugs.chromium.org/p/chromium/issues/detail?id=631937) to fix
this,
> > but
> > > it isn't currently being worked on and the work required is non-trivial.
> > > Including them in Chrome unminified is a better solution than either not
> > > minifying anything or causing all Android builds to fail.
> > 
> > Sure, but isn't the nicer fix to not run them through the compiler if we
know
> > they're going to fail?
> 
> The issue here is that we don't have an explicit list of which Javascript
files
> to compile. I originally (see https://codereview.chromium.org/2094193004/ and
>
https://docs.google.com/document/d/1XUte4m_wkPwQ9I-MfVZfBhksoL0E2_7Eb7N-PkXQK...)
> proposed that these should be listed in the GN build files, but after some 
> discussion I was persuaded that it was more maintainable to run the compiler
> from within grit, and to derive the list of files to be compiled from the 
> .grd files, and from the <script> elements in the html files. This was
> implemented in https://codereview.chromium.org/2179033002/.
> 
> We could have an exclusion list for files that should not be
> compiled; but while, in practice, this would be short (two or three files I 
> think) it would, in principle, have the same maintenance problems as having
> an inclusion list.

Nevertheless, I think an exclusion list is consistent with how we handle test
failures. I am in favor of having such a list.

Alternately, if you can cheaply detect files that will fail minification (You
mentioned they are files which use Chrome extensions to Javascript? Can we
search for this?) then we could skip attempting to minify them in grit.

> 
> > 
> > > > b) Successful builds should be silent.
> > > There is certainly a case for this, however our Android builds (at least)
are
> > > currently not silent. We don't, for example, suppress deprecation warnings
> > from
> > > the Java compiler, but nor do we turn these into fatal build errors.
> > 
> > The reason for that is that javac doesn't have a toggle for that, last I
looked.
> > 
> > > Such
> > > warnings remind us of things that should be fixed, but do not currently
stop
> > us
> > > building Chrome.

[aberent, 2016-09-26 10:47:48]

On 2016/09/23 14:07:50, flackr wrote:
> On 2016/09/22 at 13:00:06, aberent wrote:
> > On 2016/09/15 18:27:44, Nico wrote:
> > > On 2016/09/15 08:35:22, aberent wrote:
> > > > On 2016/09/13 18:51:10, Nico wrote:
> > > > > That sounds like the Wrong Fix (tm) to me.
> > > > > 
> > > > > a) If minification fails, shouldn't that be a hard (exit code != 0)
> error?
> > > > There are a few JavaScript files for which we know that minification
will
> > > > currently fail, because they use Chrome extensions to Javascript. These
> are
> > > not
> > > > supported by the Closure compiler, or, as far as I know, by any other
> > > JavaScript
> > > > minifier. There is an open bug
> > > > (https://bugs.chromium.org/p/chromium/issues/detail?id=631937) to fix
> this,
> > > but
> > > > it isn't currently being worked on and the work required is non-trivial.
> > > > Including them in Chrome unminified is a better solution than either not
> > > > minifying anything or causing all Android builds to fail.
> > > 
> > > Sure, but isn't the nicer fix to not run them through the compiler if we
> know
> > > they're going to fail?
> > 
> > The issue here is that we don't have an explicit list of which Javascript
> files
> > to compile. I originally (see https://codereview.chromium.org/2094193004/
and
> >
>
https://docs.google.com/document/d/1XUte4m_wkPwQ9I-MfVZfBhksoL0E2_7Eb7N-PkXQK...)
> > proposed that these should be listed in the GN build files, but after some 
> > discussion I was persuaded that it was more maintainable to run the compiler
> > from within grit, and to derive the list of files to be compiled from the 
> > .grd files, and from the <script> elements in the html files. This was
> > implemented in https://codereview.chromium.org/2179033002/.
> > 
> > We could have an exclusion list for files that should not be
> > compiled; but while, in practice, this would be short (two or three files I 
> > think) it would, in principle, have the same maintenance problems as having
> > an inclusion list.
> 
> Nevertheless, I think an exclusion list is consistent with how we handle test
> failures. I am in favor of having such a list.

OK, a new patch with an exclusion list will follow.
> 
> Alternately, if you can cheaply detect files that will fail minification (You
> mentioned they are files which use Chrome extensions to Javascript? Can we
> search for this?) then we could skip attempting to minify them in grit.

The simplest way is probably to run them through the compiler and to detect the
failures (effectively what we are doing now). There are no trivial markers in
the source code that something is an extension, so one would have to do
at least some level of parsing of the code.

> 
> > 
> > > 
> > > > > b) Successful builds should be silent.
> > > > There is certainly a case for this, however our Android builds (at
least)
> are
> > > > currently not silent. We don't, for example, suppress deprecation
warnings
> > > from
> > > > the Java compiler, but nor do we turn these into fatal build errors.
> > > 
> > > The reason for that is that javac doesn't have a toggle for that, last I
> looked.
> > > 
> > > > Such
> > > > warnings remind us of things that should be fixed, but do not currently
> stop
> > > us
> > > > building Chrome.

[aberent, 2016-09-26 17:36:50]

PTAL - since it turns out that only one file fails minification (and no others
should ever be added) I have simply named this file in minifier.py rather than
creating a separate list of bad files. Any new failure will cause the build to
fail.

[flackr, 2016-09-29 13:37:53]

https://codereview.chromium.org/2337253002/diff/20001/tools/grit/grit/format/...
File tools/grit/grit/format/minifier.py (right):

https://codereview.chromium.org/2337253002/diff/20001/tools/grit/grit/format/...
tools/grit/grit/format/minifier.py:23: # can't be minified. Remove this when
that is fixed.
nit: Indent comments to match "if"

https://codereview.chromium.org/2337253002/diff/20001/tools/grit/grit/format/...
tools/grit/grit/format/minifier.py:24: if
'chrome/renderer/resources/extensions/searchbox_api.js' in filename:
Can we pass in a normalized path (e.g. maybe relative to project root) and
compare equality to the blacklisted file path?

[aberent, 2016-09-29 18:54:55]

https://codereview.chromium.org/2337253002/diff/20001/tools/grit/grit/format/...
File tools/grit/grit/format/minifier.py (right):

https://codereview.chromium.org/2337253002/diff/20001/tools/grit/grit/format/...
tools/grit/grit/format/minifier.py:23: # can't be minified. Remove this when
that is fixed.
On 2016/09/29 13:37:53, flackr wrote:
> nit: Indent comments to match "if"

Done.

https://codereview.chromium.org/2337253002/diff/20001/tools/grit/grit/format/...
tools/grit/grit/format/minifier.py:24: if
'chrome/renderer/resources/extensions/searchbox_api.js' in filename:
On 2016/09/29 13:37:53, flackr wrote:
> Can we pass in a normalized path (e.g. maybe relative to project root) and
> compare equality to the blacklisted file path?

No, I don't think so. Grit, which this is part of, does everything relative to
the current directory or the grd file. I don't think it knows any absolute paths
of the project structure.

I have added code to convert the filename to an absolute path before testing it,
but I think I can still only do a substring test (actually now changed to
endswith()).

[aberent, 2016-10-03 14:32:01]

Description was changed from

==========
Remove minification spam

Minifier failure isn't a build failure, instead we simply use the
original JavaScript in this case. When, however, the minifier failed it
was printing the compiler's error messages. Since this contained
words such as "ERROR" it was making developers think that the
build was failing.

Supress this, and simply print a message saying that the original
source is being used.

BUG=644392
==========

to

==========
Remove minification spam

searchbox_api.js uses a Chrome extension to Javascript. This is not understood
by the Closure compiler, so was giving errors when grit attempted to minify it.
To prevent this don't minify this file.

BUG=644392
==========

[aberent, 2016-10-10 15:33:29]

On 2016/09/29 18:54:55, aberent wrote:
>
https://codereview.chromium.org/2337253002/diff/20001/tools/grit/grit/format/...
> File tools/grit/grit/format/minifier.py (right):
> 
>
https://codereview.chromium.org/2337253002/diff/20001/tools/grit/grit/format/...
> tools/grit/grit/format/minifier.py:23: # can't be minified. Remove this when
> that is fixed.
> On 2016/09/29 13:37:53, flackr wrote:
> > nit: Indent comments to match "if"
> 
> Done.
> 
>
https://codereview.chromium.org/2337253002/diff/20001/tools/grit/grit/format/...
> tools/grit/grit/format/minifier.py:24: if
> 'chrome/renderer/resources/extensions/searchbox_api.js' in filename:
> On 2016/09/29 13:37:53, flackr wrote:
> > Can we pass in a normalized path (e.g. maybe relative to project root) and
> > compare equality to the blacklisted file path?
> 
> No, I don't think so. Grit, which this is part of, does everything relative to
> the current directory or the grd file. I don't think it knows any absolute
paths
> of the project structure.
> 
> I have added code to convert the filename to an absolute path before testing
it,
> but I think I can still only do a substring test (actually now changed to
> endswith()).

PING. Still looking for comments or LGTM on this.

[aberent, 2016-10-10 15:35:24]

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

[commit-bot: I haz the power, 2016-10-10 15:35:36]

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-10-10 15:48:40]

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

[commit-bot: I haz the power, 2016-10-10 15:48:41]

Dry run: This issue passed the CQ dry run.

[flackr, 2016-10-21 17:45:41]

On 2016/10/10 at 15:33:29, aberent wrote:
> On 2016/09/29 18:54:55, aberent wrote:
> >
https://codereview.chromium.org/2337253002/diff/20001/tools/grit/grit/format/...
> > File tools/grit/grit/format/minifier.py (right):
> > 
> >
https://codereview.chromium.org/2337253002/diff/20001/tools/grit/grit/format/...
> > tools/grit/grit/format/minifier.py:23: # can't be minified. Remove this when
> > that is fixed.
> > On 2016/09/29 13:37:53, flackr wrote:
> > > nit: Indent comments to match "if"
> > 
> > Done.
> > 
> >
https://codereview.chromium.org/2337253002/diff/20001/tools/grit/grit/format/...
> > tools/grit/grit/format/minifier.py:24: if
> > 'chrome/renderer/resources/extensions/searchbox_api.js' in filename:
> > On 2016/09/29 13:37:53, flackr wrote:
> > > Can we pass in a normalized path (e.g. maybe relative to project root) and
> > > compare equality to the blacklisted file path?
> > 
> > No, I don't think so. Grit, which this is part of, does everything relative
to
> > the current directory or the grd file. I don't think it knows any absolute
paths
> > of the project structure.
> > 
> > I have added code to convert the filename to an absolute path before testing
it,
> > but I think I can still only do a substring test (actually now changed to
> > endswith()).
> 
> PING. Still looking for comments or LGTM on this.

Sorry for the delay! LGTM.

[aberent, 2016-10-24 09:21:09]

The CQ bit was checked by aberent@chromium.org

[commit-bot: I haz the power, 2016-10-24 09:21:30]

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-10-24 09:38:57]

Description was changed from

==========
Remove minification spam

searchbox_api.js uses a Chrome extension to Javascript. This is not understood
by the Closure compiler, so was giving errors when grit attempted to minify it.
To prevent this don't minify this file.

BUG=644392
==========

to

==========
Remove minification spam

searchbox_api.js uses a Chrome extension to Javascript. This is not understood
by the Closure compiler, so was giving errors when grit attempted to minify it.
To prevent this don't minify this file.

BUG=644392
==========

[commit-bot: I haz the power, 2016-10-24 09:38:59]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-24 09:55:20]

Description was changed from

==========
Remove minification spam

searchbox_api.js uses a Chrome extension to Javascript. This is not understood
by the Closure compiler, so was giving errors when grit attempted to minify it.
To prevent this don't minify this file.

BUG=644392
==========

to

==========
Remove minification spam

searchbox_api.js uses a Chrome extension to Javascript. This is not understood
by the Closure compiler, so was giving errors when grit attempted to minify it.
To prevent this don't minify this file.

BUG=644392

Committed: https://crrev.com/2dfa9b2034b9e982fa119f70cc8d324b0a0a4485
Cr-Commit-Position: refs/heads/master@{#427033}
==========

[commit-bot: I haz the power, 2016-10-24 09:55:21]

Patchset 3 (id:??) landed as
https://crrev.com/2dfa9b2034b9e982fa119f70cc8d324b0a0a4485
Cr-Commit-Position: refs/heads/master@{#427033}