Add the ability to pass arguments to runner.jar

Add the ability to pass arguments to runner.jar

This is used mainly so that the Chrome-specific compiler pass
can be enabled and disabled.

BUG=619091
R=aberent@chromium.org, jamiewalch@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/b16b05a3e4f0ef231f2b6a8df10d6ecff4284518

[Dan Beam, 2016-08-01 23:40:07]

Patchset #1 (id:1) has been deleted

[Dan Beam, 2016-08-02 05:21:01]

Description was changed from

==========
Pass --args to runner.jar

BUG=
==========

to

==========
Add the ability to pass arguments to runner.jar

BUG=619091
R=aberent@chromium.org
==========

[Dan Beam, 2016-08-02 05:21:01]

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

[Dan Beam, 2016-08-02 05:24:50]

Patchset #1 (id:20001) has been deleted

[Dan Beam, 2016-08-02 05:26:22]

Patchset #1 (id:40001) has been deleted

[Dan Beam, 2016-08-02 05:29:27]

Description was changed from

==========
Add the ability to pass arguments to runner.jar

BUG=619091
R=aberent@chromium.org
==========

to

==========
Add the ability to pass arguments to runner.jar

This is used mainly so Chrome-specific compiler passes
can be enabled and disabled.

BUG=619091
R=aberent@chromium.org
==========

[Dan Beam, 2016-08-02 06:06:12]

Description was changed from

==========
Add the ability to pass arguments to runner.jar

This is used mainly so Chrome-specific compiler passes
can be enabled and disabled.

BUG=619091
R=aberent@chromium.org
==========

to

==========
Add the ability to pass arguments to runner.jar

This is used mainly so that the Chrome-specific compiler pass
can be enabled and disabled.

BUG=619091
R=aberent@chromium.org
==========

[aberent, 2016-08-02 10:52:24]

https://codereview.chromium.org/2117653002/diff/60001/remoting/compile_js.gypi
File remoting/compile_js.gypi (right):

https://codereview.chromium.org/2117653002/diff/60001/remoting/compile_js.gyp...
remoting/compile_js.gypi:1: # Copyright 2015 The Chromium Authors. All rights
reserved.
Since GYP is no longer supported you probably need to urgently convert this to
GN.

https://codereview.chromium.org/2117653002/diff/60001/third_party/closure_com...
File third_party/closure_compiler/closure_args.gni (right):

https://codereview.chromium.org/2117653002/diff/60001/third_party/closure_com...
third_party/closure_compiler/closure_args.gni:6: default_closure_args = [
Would something like "checking_closure_args" be better? In my CL I split these
between those that are common to checking and minifying, and those that are
specific to one or the other.

https://codereview.chromium.org/2117653002/diff/60001/third_party/closure_com...
File third_party/closure_compiler/closure_args.gypi (right):

https://codereview.chromium.org/2117653002/diff/60001/third_party/closure_com...
third_party/closure_compiler/closure_args.gypi:7: 'default_closure_args': [
See my comments on the gni file.

https://codereview.chromium.org/2117653002/diff/60001/third_party/closure_com...
File third_party/closure_compiler/compile.py (right):

https://codereview.chromium.org/2117653002/diff/60001/third_party/closure_com...
third_party/closure_compiler/compile.py:224: runner_args = ["--%s" % arg for arg
in runner_args or []]
Using the same variable for both the processed and the unprocessed args here is
possibly confusing. Maybe do something like

processed_runner_args = ...

https://codereview.chromium.org/2117653002/diff/60001/third_party/closure_com...
third_party/closure_compiler/compile.py:339: parser.add_argument("-o",
"--out_file", dest="out_file",
Why the change? A '-' is conventional for multi-word arguments, at least in
Unix/Linux. I don't think any standard Linux tools use underscores.

If you do make this change, however, you could also take out the dest argument.

https://codereview.chromium.org/2117653002/diff/60001/third_party/closure_com...
third_party/closure_compiler/compile.py:344: parser.add_argument("-c",
"--closure_args", dest="closure_args",
Ditto.

https://codereview.chromium.org/2117653002/diff/60001/third_party/closure_com...
File third_party/closure_compiler/compile2.py (right):

https://codereview.chromium.org/2117653002/diff/60001/third_party/closure_com...
third_party/closure_compiler/compile2.py:268: runner_args = ["--%s" % arg for
arg in runner_args or []]
Using the same variable for both the processed and the unprocessed args here is
possibly confusing. Maybe do something like

processed_runner_args = ...

[Dan Beam, 2016-08-02 19:35:51]

Patchset #2 (id:80001) has been deleted

[Dan Beam, 2016-08-02 19:36:34]

https://codereview.chromium.org/2117653002/diff/60001/remoting/compile_js.gypi
File remoting/compile_js.gypi (right):

https://codereview.chromium.org/2117653002/diff/60001/remoting/compile_js.gyp...
remoting/compile_js.gypi:1: # Copyright 2015 The Chromium Authors. All rights
reserved.
On 2016/08/02 10:52:24, aberent wrote:
> Since GYP is no longer supported you probably need to urgently convert this to
> GN.

yep, brettw@ and dpranke@ agree

https://codereview.chromium.org/2117653002/diff/60001/third_party/closure_com...
File third_party/closure_compiler/closure_args.gni (right):

https://codereview.chromium.org/2117653002/diff/60001/third_party/closure_com...
third_party/closure_compiler/closure_args.gni:6: default_closure_args = [
On 2016/08/02 10:52:24, aberent wrote:
> Would something like "checking_closure_args" be better? In my CL I split these
> between those that are common to checking and minifying, and those that are
> specific to one or the other.

it makes sense to split this by "checking" or not in your CL because you
introduce the notion of "compressing".  this doesn't quite do that yet.

I have faith in your CL and your concept, but I'd like to let /your CL landing/
make this distinction in the rare case we end up going in a different direction,
this code can stay simpler.

notice that I haven't renamed "compile_js" to "typecheck_js" anywhere yet
either...

https://codereview.chromium.org/2117653002/diff/60001/third_party/closure_com...
File third_party/closure_compiler/closure_args.gypi (right):

https://codereview.chromium.org/2117653002/diff/60001/third_party/closure_com...
third_party/closure_compiler/closure_args.gypi:7: 'default_closure_args': [
On 2016/08/02 10:52:24, aberent wrote:
> See my comments on the gni file.

Acknowledged.

https://codereview.chromium.org/2117653002/diff/60001/third_party/closure_com...
File third_party/closure_compiler/compile.py (right):

https://codereview.chromium.org/2117653002/diff/60001/third_party/closure_com...
third_party/closure_compiler/compile.py:224: runner_args = ["--%s" % arg for arg
in runner_args or []]
On 2016/08/02 10:52:24, aberent wrote:
> Using the same variable for both the processed and the unprocessed args here
is
> possibly confusing. Maybe do something like
> 
> processed_runner_args = ...
> 

Done.

https://codereview.chromium.org/2117653002/diff/60001/third_party/closure_com...
third_party/closure_compiler/compile.py:339: parser.add_argument("-o",
"--out_file", dest="out_file",
On 2016/08/02 10:52:24, aberent wrote:
> Why the change? A '-' is conventional for multi-word arguments, at least in
> Unix/Linux. I don't think any standard Linux tools use underscores.

switching to _ consistently everywhere to:

a) be easier to use in variable names
b) remove dest=
c) be consistent with closure compiler

like we discussed

> 
> If you do make this change, however, you could also take out the dest
argument.

removed a few dest=

https://codereview.chromium.org/2117653002/diff/60001/third_party/closure_com...
third_party/closure_compiler/compile.py:344: parser.add_argument("-c",
"--closure_args", dest="closure_args",
On 2016/08/02 10:52:24, aberent wrote:
> Ditto.

Done.

https://codereview.chromium.org/2117653002/diff/60001/third_party/closure_com...
File third_party/closure_compiler/compile2.py (right):

https://codereview.chromium.org/2117653002/diff/60001/third_party/closure_com...
third_party/closure_compiler/compile2.py:268: runner_args = ["--%s" % arg for
arg in runner_args or []]
On 2016/08/02 10:52:24, aberent wrote:
> Using the same variable for both the processed and the unprocessed args here
is
> possibly confusing. Maybe do something like
> 
> processed_runner_args = ...
> 

Done.

[aberent, 2016-08-02 19:41:37]

lgtm

[Dan Beam, 2016-08-02 20:46:50]

ran both:

third_party/closure_compiler/run_compiler
third_party/closure_compiler/run_tests.py

locally

can't run try jobs via `git cl try` nor the UI on rietveld because this touches
runner.jar (afaik).

I can't land runner.jar separately because it'll break things.

tl;dr - landing manually and crossing fingers

[Dan Beam, 2016-08-02 21:06:53]

Description was changed from

==========
Add the ability to pass arguments to runner.jar

This is used mainly so that the Chrome-specific compiler pass
can be enabled and disabled.

BUG=619091
R=aberent@chromium.org
==========

to

==========
Add the ability to pass arguments to runner.jar

This is used mainly so that the Chrome-specific compiler pass
can be enabled and disabled.

BUG=619091
R=aberent@chromium.org,jamiewalch@chromium.org
==========

[Dan Beam, 2016-08-02 21:06:54]

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

[Dan Beam, 2016-08-02 21:07:05]

+jamiewalch@ for remoting/

[Jamie, 2016-08-02 21:13:14]

remoting/ lgtm

[commit-bot: I haz the power, 2016-08-02 21:47:00]

Description was changed from

==========
Add the ability to pass arguments to runner.jar

This is used mainly so that the Chrome-specific compiler pass
can be enabled and disabled.

BUG=619091
R=aberent@chromium.org,jamiewalch@chromium.org
==========

to

==========
Add the ability to pass arguments to runner.jar

This is used mainly so that the Chrome-specific compiler pass
can be enabled and disabled.

BUG=619091
R=aberent@chromium.org, jamiewalch@chromium.org

Committed: https://crrev.com/b16b05a3e4f0ef231f2b6a8df10d6ecff4284518
Cr-Commit-Position: refs/heads/master@{#409336}
==========

[commit-bot: I haz the power, 2016-08-02 21:47:02]

Patchset 4 (id:??) landed as
https://crrev.com/b16b05a3e4f0ef231f2b6a8df10d6ecff4284518
Cr-Commit-Position: refs/heads/master@{#409336}

[Dan Beam, 2016-08-02 21:48:08]

Description was changed from

==========
Add the ability to pass arguments to runner.jar

This is used mainly so that the Chrome-specific compiler pass
can be enabled and disabled.

BUG=619091
R=aberent@chromium.org, jamiewalch@chromium.org

Committed: https://crrev.com/b16b05a3e4f0ef231f2b6a8df10d6ecff4284518
Cr-Commit-Position: refs/heads/master@{#409336}
==========

to

==========
Add the ability to pass arguments to runner.jar

This is used mainly so that the Chrome-specific compiler pass
can be enabled and disabled.

BUG=619091
R=aberent@chromium.org, jamiewalch@chromium.org

Committed:
https://chromium.googlesource.com/chromium/src/+/b16b05a3e4f0ef231f2b6a8df10d...
==========

[Dan Beam, 2016-08-02 21:48:10]

Committed patchset #4 (id:140001) manually as
b16b05a3e4f0ef231f2b6a8df10d6ecff4284518 (presubmit successful).