Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2921143002:
Reverse accidental enabling of 'use strict' in closure compiler (Closed)
DescriptionReverse accidental enabling of 'use strict' in closure compiler
In grit, JavaScript files are minified using closure compiler. The output
is in ES6 without "use strict", until a roll in
https://codereview.chromium.org/2918683002, which adds "use strict" by default
for ES6 (now called ES2015).
Having "use strict" caused failures in DOM distiller and probably other things,
so we need to fix them first.
BUG=728655, 729689
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2921143002
Cr-Commit-Position: refs/heads/master@{#477192}
Committed: https://chromium.googlesource.com/chromium/src/+/14fb5d1f03c1d705a5c0e1d33904df5da7153221
Patch Set 1 #Patch Set 2 : emit_use_strict=false #
Total comments: 11
Patch Set 3 : add todo #
Depends on Patchset: Messages
Total messages: 39 (22 generated)
The CQ bit was checked by wychen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix 'use strict' issue of closure_compiler BUG=728655 ========== to ========== Fix 'use strict' issue of closure_compiler BUG=728655 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by wychen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
wychen@chromium.org changed reviewers: + dbeam@chromium.org, flackr@chromium.org
dbeam@chromium.org: Please review changes in closure_args.gni flackr@chromium.org: Please review changes in grit_rule.gni
dbeam@chromium.org: Please review changes in closure_args.gni flackr@chromium.org: Please review changes in grit_rule.gni
Description was changed from ========== Fix 'use strict' issue of closure_compiler BUG=728655 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix 'use strict' issue of closure_compiler BUG=728655 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dbeam@chromium.org changed reviewers: + dpapad@chromium.org - dbeam@chromium.org
-dbeam, +dpapad
https://codereview.chromium.org/2921143002/diff/20001/third_party/closure_com... File third_party/closure_compiler/closure_args.gni (right): https://codereview.chromium.org/2921143002/diff/20001/third_party/closure_com... third_party/closure_compiler/closure_args.gni:56: "emit_use_strict=false", Can you elaborate a bit on what is the exact problem caused by "use strict"? In general "use strict" makes JS usage more sane, so I would prefer - keeping it - If we can't keep it, figure out exactly why. If some code fails in strict mode, this usually indicaten some other problems that can be fixed alternatively. Specifically see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode, section "Converting mistakes into errors"
https://codereview.chromium.org/2921143002/diff/20001/third_party/closure_com... File third_party/closure_compiler/closure_args.gni (right): https://codereview.chromium.org/2921143002/diff/20001/third_party/closure_com... third_party/closure_compiler/closure_args.gni:56: "emit_use_strict=false", On 2017/06/05 17:31:31, dpapad wrote: > Can you elaborate a bit on what is the exact problem caused by "use strict"? In > general "use strict" makes JS usage more sane, so I would prefer > - keeping it > - If we can't keep it, figure out exactly why. > > If some code fails in strict mode, this usually indicaten some other problems > that can be fixed alternatively. > > Specifically see > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode, > section "Converting mistakes into errors" I agree enabling "use strict" is the right direction for us. However, I believe we've turned this on by accident and caused regressions. See crbug.com/728655. We can turn it on after fixing the known issues.
https://codereview.chromium.org/2921143002/diff/20001/tools/grit/grit_rule.gni File tools/grit/grit_rule.gni (right): https://codereview.chromium.org/2921143002/diff/20001/tools/grit/grit_rule.gn... tools/grit/grit_rule.gni:407: "//third_party/closure_compiler/compiler/compiler.jar", I'm not sure I follow this change, is this invalidating compiled resources on the compiler changing? Does the compiler change when you change the arguments set in closure_arguments.gni?
https://codereview.chromium.org/2921143002/diff/20001/tools/grit/grit_rule.gni File tools/grit/grit_rule.gni (right): https://codereview.chromium.org/2921143002/diff/20001/tools/grit/grit_rule.gn... tools/grit/grit_rule.gni:407: "//third_party/closure_compiler/compiler/compiler.jar", On 2017/06/05 17:39:46, flackr wrote: > I'm not sure I follow this change, is this invalidating compiled resources on > the compiler changing? Does the compiler change when you change the arguments > set in closure_arguments.gni? The dependency was underspecified, so that when closure rolled in, the CQ and waterfall didn't reliably repack the resources, and made bisecting difficult for crbug.com/728655.
https://codereview.chromium.org/2921143002/diff/20001/third_party/closure_com... File third_party/closure_compiler/closure_args.gni (right): https://codereview.chromium.org/2921143002/diff/20001/third_party/closure_com... third_party/closure_compiler/closure_args.gni:56: "emit_use_strict=false", > We can turn it on after fixing the known issues. I see. Can you file a bug listing the known issues and add a TODO pointing to this bug here? I am hoping that this would increase the possibility of this issue being addressed in the future (FWIW the phrasing in the bug makes it sound that it won't be fixed anytime soon). Also, what is the difference between "emit_use_strict" and "strict_mode_input"? The commit at https://github.com/google/closure-compiler/commit/5279eb46b805f4a8c8d066a0b6d... suggests that strict_mode_input should be used to opt out of the new default behavior.
https://codereview.chromium.org/2921143002/diff/20001/tools/grit/grit_rule.gni File tools/grit/grit_rule.gni (right): https://codereview.chromium.org/2921143002/diff/20001/tools/grit/grit_rule.gn... tools/grit/grit_rule.gni:407: "//third_party/closure_compiler/compiler/compiler.jar", On 2017/06/05 17:46:25, wychen wrote: > On 2017/06/05 17:39:46, flackr wrote: > > I'm not sure I follow this change, is this invalidating compiled resources on > > the compiler changing? Does the compiler change when you change the arguments > > set in closure_arguments.gni? > > The dependency was underspecified, so that when closure rolled in, the CQ and > waterfall didn't reliably repack the resources, and made bisecting difficult for > crbug.com/728655. Ah I see, this makes sense to do as a separate commit. You may also want to depend on the arguments file right?
https://codereview.chromium.org/2921143002/diff/20001/third_party/closure_com... File third_party/closure_compiler/closure_args.gni (right): https://codereview.chromium.org/2921143002/diff/20001/third_party/closure_com... third_party/closure_compiler/closure_args.gni:56: "emit_use_strict=false", On 2017/06/05 17:51:23, dpapad wrote: > > We can turn it on after fixing the known issues. > > I see. Can you file a bug listing the known issues and add a TODO pointing to > this bug here? I am hoping that this would increase the possibility of this > issue being addressed in the future (FWIW the phrasing in the bug makes it sound > that it won't be fixed anytime soon). I'll add a TODO. The thing I worry the most is the testing coverage. "use strict" only changes the runtime behavior, and we might not have exercised them enough in our automated tests. Many of the JS is used in the web UI, and that's not where our coverage is best. The only known issue is documented in that bug (dom distiller). This might be a bit ironic since some of the "use strict" violations come from code generated by (older version of) closure_compiler. > Also, what is the difference between "emit_use_strict" and "strict_mode_input"? > The commit at > https://github.com/google/closure-compiler/commit/5279eb46b805f4a8c8d066a0b6d... > suggests that strict_mode_input should be used to opt out of the new default > behavior. I tried strict_mode_input=false but somehow it didn't remove the "use strict" output. https://codereview.chromium.org/2921143002/diff/20001/tools/grit/grit_rule.gni File tools/grit/grit_rule.gni (right): https://codereview.chromium.org/2921143002/diff/20001/tools/grit/grit_rule.gn... tools/grit/grit_rule.gni:407: "//third_party/closure_compiler/compiler/compiler.jar", On 2017/06/05 17:54:54, flackr wrote: > On 2017/06/05 17:46:25, wychen wrote: > > On 2017/06/05 17:39:46, flackr wrote: > > > I'm not sure I follow this change, is this invalidating compiled resources > on > > > the compiler changing? Does the compiler change when you change the > arguments > > > set in closure_arguments.gni? > > > > The dependency was underspecified, so that when closure rolled in, the CQ and > > waterfall didn't reliably repack the resources, and made bisecting difficult > for > > crbug.com/728655. > > Ah I see, this makes sense to do as a separate commit. You may also want to > depend on the arguments file right? I'll create a separate CL. What do you mean by argument file? It looks like this js minifier use command line arguments, and I think it'll get rebuilt if the arguments change. There are several python scripts that's underspecified as well. Not sure if it makes sense to add them all.
https://codereview.chromium.org/2921143002/diff/20001/tools/grit/grit_rule.gni File tools/grit/grit_rule.gni (right): https://codereview.chromium.org/2921143002/diff/20001/tools/grit/grit_rule.gn... tools/grit/grit_rule.gni:407: "//third_party/closure_compiler/compiler/compiler.jar", On 2017/06/05 18:25:56, wychen wrote: > On 2017/06/05 17:54:54, flackr wrote: > > On 2017/06/05 17:46:25, wychen wrote: > > > On 2017/06/05 17:39:46, flackr wrote: > > > > I'm not sure I follow this change, is this invalidating compiled resources > > on > > > > the compiler changing? Does the compiler change when you change the > > arguments > > > > set in closure_arguments.gni? > > > > > > The dependency was underspecified, so that when closure rolled in, the CQ > and > > > waterfall didn't reliably repack the resources, and made bisecting difficult > > for > > > crbug.com/728655. > > > > Ah I see, this makes sense to do as a separate commit. You may also want to > > depend on the arguments file right? > > I'll create a separate CL. > What do you mean by argument file? It looks like this js minifier use command > line arguments, and I think it'll get rebuilt if the arguments change. > > There are several python scripts that's underspecified as well. Not sure if it > makes sense to add them all. Ah okay, as long as argument changes in closure_args.gni trigger a rebuild then it should be fine. I'm not sure, about adding other dependencies, I would stick to just the ones which are known to be problematic.
https://codereview.chromium.org/2921143002/diff/20001/tools/grit/grit_rule.gni File tools/grit/grit_rule.gni (right): https://codereview.chromium.org/2921143002/diff/20001/tools/grit/grit_rule.gn... tools/grit/grit_rule.gni:407: "//third_party/closure_compiler/compiler/compiler.jar", On 2017/06/05 18:46:40, flackr wrote: > On 2017/06/05 18:25:56, wychen wrote: > > On 2017/06/05 17:54:54, flackr wrote: > > > On 2017/06/05 17:46:25, wychen wrote: > > > > On 2017/06/05 17:39:46, flackr wrote: > > > > > I'm not sure I follow this change, is this invalidating compiled > resources > > > on > > > > > the compiler changing? Does the compiler change when you change the > > > arguments > > > > > set in closure_arguments.gni? > > > > > > > > The dependency was underspecified, so that when closure rolled in, the CQ > > and > > > > waterfall didn't reliably repack the resources, and made bisecting > difficult > > > for > > > > crbug.com/728655. > > > > > > Ah I see, this makes sense to do as a separate commit. You may also want to > > > depend on the arguments file right? > > > > I'll create a separate CL. > > What do you mean by argument file? It looks like this js minifier use command > > line arguments, and I think it'll get rebuilt if the arguments change. > > > > There are several python scripts that's underspecified as well. Not sure if it > > makes sense to add them all. > > Ah okay, as long as argument changes in closure_args.gni trigger a rebuild then > it should be fine. I'm not sure, about adding other dependencies, I would stick > to just the ones which are known to be problematic. Split out here: https://codereview.chromium.org/2924653002/
Description was changed from ========== Fix 'use strict' issue of closure_compiler BUG=728655 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix 'use strict' issue of closure_compiler BUG=728655, 729689 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Fix 'use strict' issue of closure_compiler BUG=728655, 729689 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Reverse accidental enabling of 'use strict' in closure compiler In grit, JavaScript files are minified using closure compiler. The output is in ES6 without "use strict", until a roll in https://codereview.chromium.org/2918683002, which adds "use strict" by default for ES6 (now called ES2015). Having "use strict" caused failures in DOM distiller and probably other things, so we need to fix them first. BUG=728655, 729689 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2921143002/diff/20001/third_party/closure_com... File third_party/closure_compiler/closure_args.gni (right): https://codereview.chromium.org/2921143002/diff/20001/third_party/closure_com... third_party/closure_compiler/closure_args.gni:56: "emit_use_strict=false", On 2017/06/05 18:25:56, wychen wrote: > On 2017/06/05 17:51:23, dpapad wrote: > > > We can turn it on after fixing the known issues. > > > > I see. Can you file a bug listing the known issues and add a TODO pointing to > > this bug here? I am hoping that this would increase the possibility of this > > issue being addressed in the future (FWIW the phrasing in the bug makes it > sound > > that it won't be fixed anytime soon). > > I'll add a TODO. Done.
wychen@chromium.org changed reviewers: - flackr@chromium.org
LGTM
The CQ bit was checked by wychen@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2924653002 Patch 1). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by wychen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wychen@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496722485359230,
"parent_rev": "07fadbc84d1cfaac39addc39c2a64b4eafa89030", "commit_rev":
"14fb5d1f03c1d705a5c0e1d33904df5da7153221"}
Message was sent while issue was closed.
Description was changed from ========== Reverse accidental enabling of 'use strict' in closure compiler In grit, JavaScript files are minified using closure compiler. The output is in ES6 without "use strict", until a roll in https://codereview.chromium.org/2918683002, which adds "use strict" by default for ES6 (now called ES2015). Having "use strict" caused failures in DOM distiller and probably other things, so we need to fix them first. BUG=728655, 729689 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Reverse accidental enabling of 'use strict' in closure compiler In grit, JavaScript files are minified using closure compiler. The output is in ES6 without "use strict", until a roll in https://codereview.chromium.org/2918683002, which adds "use strict" by default for ES6 (now called ES2015). Having "use strict" caused failures in DOM distiller and probably other things, so we need to fix them first. BUG=728655, 729689 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2921143002 Cr-Commit-Position: refs/heads/master@{#477192} Committed: https://chromium.googlesource.com/chromium/src/+/14fb5d1f03c1d705a5c0e1d33904... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/14fb5d1f03c1d705a5c0e1d33904... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
