Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2113053002:
Make Chrome-specific enhancements optional in runner.jar (Closed)
|
Created:
4 years, 5 months ago by Dan Beam Modified:
4 years, 4 months ago CC:
chromium-reviews, vitalyp+closure_chromium.org, jlklein+watch-closure_chromium.org, dbeam+watch-closure_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake Chrome-specific enhancements optional in runner.jar
BUG=619091
R=tbreisacher@chromium.org
NOTRY=true
Committed: https://crrev.com/055b3f32db776858013eafe927c994a4d822376c
Cr-Commit-Position: refs/heads/master@{#409306}
Patch Set 1 : nits and executor change #
Total comments: 2
Patch Set 2 : setChecksOnly -> setContinueAfterErrors #
Total comments: 1
Patch Set 3 : re-add setChecksOnly() for speed #Patch Set 4 : update README #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 21 (11 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Make Chrome-specific enhancements optional in runner.jar BUG= ========== to ========== Make Chrome-specific enhancements optional in runner.jar BUG=619091 R=tbreisacher@chromium.org ==========
dbeam@chromium.org changed reviewers: + tbreisacher@chromium.org
Patchset #1 (id:20001) has been deleted
lgtm https://codereview.chromium.org/2113053002/diff/40001/third_party/closure_com... File third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java (right): https://codereview.chromium.org/2113053002/diff/40001/third_party/closure_com... third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java:189: options.setIdeMode(true); side note: This is deprecated. You likely want options.setChecksOnly(true);
https://codereview.chromium.org/2113053002/diff/40001/third_party/closure_com... File third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java (right): https://codereview.chromium.org/2113053002/diff/40001/third_party/closure_com... third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java:189: options.setIdeMode(true); On 2016/08/01 21:18:58, Tyler Breisacher (Chromium) wrote: > side note: This is deprecated. You likely want options.setChecksOnly(true); Done.
Patchset #2 (id:60001) has been deleted
aberent@chromium.org changed reviewers: + aberent@chromium.org
https://codereview.chromium.org/2113053002/diff/80001/third_party/closure_com... File third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java (right): https://codereview.chromium.org/2113053002/diff/80001/third_party/closure_com... third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java:278: usage = "Whether to add Chrome-specific compiler passes") --disable-chrome-pass would be easier for compatibility with existing uses. My minifying CL, which is the only thing that needs to disable the Chrome pass, hasn't yet landed and could easily set this option.
On 2016/08/02 16:26:52, aberent wrote: > https://codereview.chromium.org/2113053002/diff/80001/third_party/closure_com... > File > third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java > (right): > > https://codereview.chromium.org/2113053002/diff/80001/third_party/closure_com... > third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java:278: > usage = "Whether to add Chrome-specific compiler passes") > --disable-chrome-pass would be easier for compatibility with existing uses. My > minifying CL, which is the only thing that needs to disable the Chrome pass, > hasn't yet landed and could easily set this option. I get extremely confused when flags have negative names like "disable". IMO the flag name should be "enable chrome pass" or just "chrome pass" and then the default value of the flag can be set to whatever makes sense.
On 2016/08/02 16:29:17, Tyler Breisacher (Chromium) wrote: > On 2016/08/02 16:26:52, aberent wrote: > > > https://codereview.chromium.org/2113053002/diff/80001/third_party/closure_com... > > File > > > third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java > > (right): > > > > > https://codereview.chromium.org/2113053002/diff/80001/third_party/closure_com... > > > third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java:278: > > usage = "Whether to add Chrome-specific compiler passes") > > --disable-chrome-pass would be easier for compatibility with existing uses. My > > minifying CL, which is the only thing that needs to disable the Chrome pass, > > hasn't yet landed and could easily set this option. > > I get extremely confused when flags have negative names like "disable". IMO the > flag name should be "enable chrome pass" or just "chrome pass" and then the > default value of the flag can be set to whatever makes sense. Ok, so maybe we should make this a boolean flag, but with a default value of true. My point was that this would be easier to land if it didn't change the default behaviour.
https://codereview.chromium.org/2113053002/diff/120001/third_party/closure_co... File third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java (right): https://codereview.chromium.org/2113053002/diff/120001/third_party/closure_co... third_party/closure_compiler/runner/src/org/chromium/closure/compiler/Runner.java:190: options.setContinueAfterErrors(true); required for tests to pass
Description was changed from ========== Make Chrome-specific enhancements optional in runner.jar BUG=619091 R=tbreisacher@chromium.org ========== to ========== Make Chrome-specific enhancements optional in runner.jar BUG=619091 R=tbreisacher@chromium.org NOTRY=true ==========
The CQ bit was checked by dbeam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tbreisacher@chromium.org Link to the patchset: https://codereview.chromium.org/2113053002/#ps120001 (title: "update README")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make Chrome-specific enhancements optional in runner.jar BUG=619091 R=tbreisacher@chromium.org NOTRY=true ========== to ========== Make Chrome-specific enhancements optional in runner.jar BUG=619091 R=tbreisacher@chromium.org NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Make Chrome-specific enhancements optional in runner.jar BUG=619091 R=tbreisacher@chromium.org NOTRY=true ========== to ========== Make Chrome-specific enhancements optional in runner.jar BUG=619091 R=tbreisacher@chromium.org NOTRY=true Committed: https://crrev.com/055b3f32db776858013eafe927c994a4d822376c Cr-Commit-Position: refs/heads/master@{#409306} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/055b3f32db776858013eafe927c994a4d822376c Cr-Commit-Position: refs/heads/master@{#409306} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
