Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(161)

Issue 2179033002: Strip comments and whitespace from javascript resources (Closed)

Created:
4 years, 5 months ago by aberent
Modified:
4 years, 4 months ago
CC:
chromium-reviews, vitalyp+closure_chromium.org, jlklein+watch-closure_chromium.org, dbeam+watch-closure_chromium.org, brettw, Lei Zhang, jochen (gone - plz use gerrit), agrieve, Bernhard Bauer, Torne
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Strip comments and whitespace from javascript resources On Android strip comments and whitespace from javascript resources. Add an option to grit build that provides a command that strips comments and whitespace, and modify grit so that this is applied to all javascript resource and scripts within HTML resources. Add a python script that wraps the closure compiler to implement this command. Pull it all together with GN. BUG=619091 Committed: https://crrev.com/8dfa12f1b8845bc912728471d19d9915119456d7 Cr-Commit-Position: refs/heads/master@{#409498}

Patch Set 1 #

Patch Set 2 : One minor fix #

Total comments: 24

Patch Set 3 : Respond to comments, handle errors, avoid double minification #

Patch Set 4 : Fix naming of GN variables, and rebase. #

Total comments: 6

Patch Set 5 : Disable extra Chrome specific processing in compiler. #

Patch Set 6 : Remove all compiler options that cause code tranformation #

Total comments: 19

Patch Set 7 : Change upstream CL, and fix nits #

Patch Set 8 : Change upstream CL, and fix nits #

Patch Set 9 : Revert changes to screen.js (no longer needed for this CL) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+166 lines, -17 lines) Patch
M third_party/closure_compiler/closure_args.gni View 1 2 3 4 5 6 3 chunks +19 lines, -3 lines 0 comments Download
M third_party/closure_compiler/compile2.py View 1 2 3 4 5 6 3 chunks +4 lines, -3 lines 0 comments Download
A third_party/closure_compiler/js_minify.py View 1 2 3 4 5 6 1 chunk +50 lines, -0 lines 0 comments Download
M third_party/closure_compiler/runner/runner.jar View 1 2 3 4 Binary file 0 comments Download
M tools/grit/grit/format/html_inline.py View 1 2 11 chunks +22 lines, -9 lines 0 comments Download
A tools/grit/grit/format/minifier.py View 1 2 3 4 5 6 1 chunk +30 lines, -0 lines 0 comments Download
M tools/grit/grit/gather/chrome_html.py View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M tools/grit/grit/node/include.py View 1 2 chunks +5 lines, -2 lines 0 comments Download
M tools/grit/grit/tool/build.py View 1 2 3 4 5 6 6 chunks +14 lines, -0 lines 0 comments Download
M tools/grit/grit_rule.gni View 1 2 3 4 5 3 chunks +21 lines, -0 lines 0 comments Download

Messages

Total messages: 51 (33 generated)
aberent
dbeam@chromium.org: Please review changes in closure_compiler dpranke@chromium.org: Please review changes in grit and features.gni This ...
4 years, 5 months ago (2016-07-25 17:00:11 UTC) #2
Dan Beam
https://codereview.chromium.org/2179033002/diff/20001/third_party/closure_compiler/js_minify.py File third_party/closure_compiler/js_minify.py (right): https://codereview.chromium.org/2179033002/diff/20001/third_party/closure_compiler/js_minify.py#newcode11 third_party/closure_compiler/js_minify.py:11: closure_args = [ can we just compose these rather ...
4 years, 5 months ago (2016-07-25 17:29:16 UTC) #5
Dirk Pranke
This is mostly okay, just a few more minor questions. https://codereview.chromium.org/2179033002/diff/20001/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/2179033002/diff/20001/build/config/features.gni#newcode184 ...
4 years, 5 months ago (2016-07-25 21:12:37 UTC) #8
brettw
https://codereview.chromium.org/2179033002/diff/20001/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/2179033002/diff/20001/build/config/features.gni#newcode184 build/config/features.gni:184: strip_resource_files = is_android On 2016/07/25 21:12:36, Dirk Pranke wrote: ...
4 years, 4 months ago (2016-07-26 21:01:01 UTC) #10
aberent
https://codereview.chromium.org/2179033002/diff/20001/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/2179033002/diff/20001/build/config/features.gni#newcode184 build/config/features.gni:184: strip_resource_files = is_android On 2016/07/25 21:12:36, Dirk Pranke wrote: ...
4 years, 4 months ago (2016-07-27 09:46:36 UTC) #11
Dan Beam
lgtm https://codereview.chromium.org/2179033002/diff/60001/third_party/closure_compiler/js_minify.py File third_party/closure_compiler/js_minify.py (right): https://codereview.chromium.org/2179033002/diff/60001/third_party/closure_compiler/js_minify.py#newcode17 third_party/closure_compiler/js_minify.py:17: from compile2 import Checker nit: \n\n between top-level ...
4 years, 4 months ago (2016-07-28 16:27:05 UTC) #21
aberent
https://codereview.chromium.org/2179033002/diff/60001/third_party/closure_compiler/js_minify.py File third_party/closure_compiler/js_minify.py (right): https://codereview.chromium.org/2179033002/diff/60001/third_party/closure_compiler/js_minify.py#newcode17 third_party/closure_compiler/js_minify.py:17: from compile2 import Checker On 2016/07/28 16:27:05, Dan Beam ...
4 years, 4 months ago (2016-08-02 11:43:30 UTC) #26
aberent
Dirk - PTAL, I think everything in the grit area should now be stable. Dan ...
4 years, 4 months ago (2016-08-02 12:54:56 UTC) #31
Lei Zhang
https://codereview.chromium.org/2179033002/diff/90001/tools/grit/grit/format/html_inline.py File tools/grit/grit/format/html_inline.py (right): https://codereview.chromium.org/2179033002/diff/90001/tools/grit/grit/format/html_inline.py#newcode21 tools/grit/grit/format/html_inline.py:21: from grit.format import minifier Alphabetical order. https://codereview.chromium.org/2179033002/diff/90001/tools/grit/grit/format/minifier.py File tools/grit/grit/format/minifier.py ...
4 years, 4 months ago (2016-08-02 16:47:48 UTC) #33
aberent
Note:- This should be dependent on https://codereview.chromium.org/2117653002/#ps60001, but it appears that something has gone wrong ...
4 years, 4 months ago (2016-08-02 20:27:42 UTC) #36
Lei Zhang
grit lgtm, but there are red bots... https://codereview.chromium.org/2179033002/diff/90001/tools/grit/grit/format/html_inline.py File tools/grit/grit/format/html_inline.py (right): https://codereview.chromium.org/2179033002/diff/90001/tools/grit/grit/format/html_inline.py#newcode21 tools/grit/grit/format/html_inline.py:21: from grit.format ...
4 years, 4 months ago (2016-08-02 20:43:32 UTC) #39
aberent
On 2016/08/02 20:43:32, Lei Zhang wrote: > grit lgtm, but there are red bots... > ...
4 years, 4 months ago (2016-08-02 21:03:57 UTC) #40
Dirk Pranke
lgtm
4 years, 4 months ago (2016-08-03 00:45:26 UTC) #41
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2179033002/150001
4 years, 4 months ago (2016-08-03 09:10:05 UTC) #44
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_compile_dbg_ng/builds/231978) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, ...
4 years, 4 months ago (2016-08-03 09:22:55 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2179033002/150001
4 years, 4 months ago (2016-08-03 11:24:05 UTC) #48
commit-bot: I haz the power
Committed patchset #9 (id:150001)
4 years, 4 months ago (2016-08-03 12:43:50 UTC) #49
commit-bot: I haz the power
4 years, 4 months ago (2016-08-03 12:46:07 UTC) #51
Message was sent while issue was closed.
Patchset 9 (id:??) landed as
https://crrev.com/8dfa12f1b8845bc912728471d19d9915119456d7
Cr-Commit-Position: refs/heads/master@{#409498}

Powered by Google App Engine
This is Rietveld 408576698