Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
958383003:
Output closure-compiled JavaScript files (Closed)
|
Created:
5 years, 9 months ago by Theresa Modified:
5 years, 9 months ago Reviewers:
Dan Beam CC:
chromium-reviews, vitalyp+closure_chromium.org, noyau+watch_chromium.org, arv+watch_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. |
DescriptionOutput closure-compiled JavaScript files
If generate_output is set, chcker.py adds compiler flags to actually
output the compiled JS. It will also output a source map if the build
is Debug.
BUG=393874
Committed: https://crrev.com/b7f849b027b42917a032dca4f2467db357fa26a4
Cr-Commit-Position: refs/heads/master@{#319637}
Patch Set 1 #Patch Set 2 : Little bit of clean up #
Total comments: 3
Patch Set 3 : Always generate source map if minified js is output #
Total comments: 21
Patch Set 4 : Always generate compiled output, revert changes to example files #
Total comments: 2
Patch Set 5 : Create output file if doesn't already exist #
Total comments: 1
Patch Set 6 : Add back checks for if(out_file) #
Total comments: 8
Patch Set 7 : Address issues from last review #Patch Set 8 : Fix a line wrap #
Total comments: 3
Patch Set 9 : Update out_file pydoc #Patch Set 10 : Fix line wrap #
Total comments: 1
Messages
Total messages: 32 (7 generated)
twellington@chromium.org changed reviewers: + dbeam@chromium.org
This is a first pass at outputting closure-compiled JS files. Ultimately, only checker.py and compile_js.gypi will be checked in with this CL. All of the other modified file are to exemplify how compiled files can be output and used; I'll revert those changes and pull them into a separate CL once this one is finished.
https://codereview.chromium.org/958383003/diff/20001/chrome/browser/resources... File chrome/browser/resources/enhanced_bookmarks/get_salient_image_url.js (right): https://codereview.chromium.org/958383003/diff/20001/chrome/browser/resources... chrome/browser/resources/enhanced_bookmarks/get_salient_image_url.js:21: var jsonEncoder = JSON['stringify']; is JSON.{encode,stringify} not in the externs? why is this necessary? https://codereview.chromium.org/958383003/diff/20001/third_party/closure_comp... File third_party/closure_compiler/checker.py (right): https://codereview.chromium.org/958383003/diff/20001/third_party/closure_comp... third_party/closure_compiler/checker.py:190: if create_source_map: i think we should always create source maps https://codereview.chromium.org/958383003/diff/40001/chrome/browser/resources... File chrome/browser/resources/enhanced_bookmarks/compiled_resources.gyp (right): https://codereview.chromium.org/958383003/diff/40001/chrome/browser/resources... chrome/browser/resources/enhanced_bookmarks/compiled_resources.gyp:9: 'generate_output' : 1, 'generate_output': (no space), also why not True? https://codereview.chromium.org/958383003/diff/40001/chrome/browser/resources... File chrome/browser/resources/enhanced_bookmarks/get_salient_image_url.js (right): https://codereview.chromium.org/958383003/diff/40001/chrome/browser/resources... chrome/browser/resources/enhanced_bookmarks/get_salient_image_url.js:21: var jsonEncoder = JSON['stringify']; why is this necessary? shouldn't externs preserve these? https://codereview.chromium.org/958383003/diff/40001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/958383003/diff/40001/chrome/chrome_browser.gy... chrome/chrome_browser.gypi:3388: 'browser/resources/enhanced_bookmarks/compiled_resources.gyp:*', this would force all chrome developers to install java https://codereview.chromium.org/958383003/diff/40001/chrome/chrome_browser.gy... chrome/chrome_browser.gypi:3389: remove extra \n https://codereview.chromium.org/958383003/diff/40001/third_party/closure_comp... File third_party/closure_compiler/checker.py (right): https://codereview.chromium.org/958383003/diff/40001/third_party/closure_comp... third_party/closure_compiler/checker.py:47: '--compilation_level=WHITESPACE_ONLY' ' -> " https://codereview.chromium.org/958383003/diff/40001/third_party/closure_comp... third_party/closure_compiler/checker.py:47: '--compilation_level=WHITESPACE_ONLY' nit: trailing , https://codereview.chromium.org/958383003/diff/40001/third_party/closure_comp... third_party/closure_compiler/checker.py:181: def run_js_check(self, sources, externs=None, out_file=None, i think if the code will be modified, there should be a source map, so couple these options together (i.e. just treat a non-empty out_file as a desire to generate output) https://codereview.chromium.org/958383003/diff/40001/third_party/closure_comp... third_party/closure_compiler/checker.py:311: generate_output = False if (opts.generate_output == '0') else True False if (condition) else True => can always be shortened to => (condition) https://codereview.chromium.org/958383003/diff/40001/third_party/closure_comp... third_party/closure_compiler/checker.py:313: # Clean up intermediate folders that get created by output.py or create the dirs if needed 80 col wrap https://codereview.chromium.org/958383003/diff/40001/third_party/closure_comp... third_party/closure_compiler/checker.py:313: # Clean up intermediate folders that get created by output.py or create the dirs if needed end with . https://codereview.chromium.org/958383003/diff/40001/third_party/closure_comp... third_party/closure_compiler/checker.py:316: if os.path.exists(out_dir) and os.listdir(out_dir) == []: why are we making empty directories? https://codereview.chromium.org/958383003/diff/40001/third_party/closure_comp... third_party/closure_compiler/checker.py:320: os.makedirs(out_dir) so closure doesn't do this automatically? make a path if it doesn't exist? https://codereview.chromium.org/958383003/diff/40001/third_party/closure_comp... third_party/closure_compiler/checker.py:330: out_file=opts.out_file, generate_output=generate_output) 80 col wrap
Thank you for the review :) a few comments inline, will work on getting this turned around Monday (I'm sheriffing tomorrow) and ping you when it's ready for another look. https://codereview.chromium.org/958383003/diff/20001/third_party/closure_comp... File third_party/closure_compiler/checker.py (right): https://codereview.chromium.org/958383003/diff/20001/third_party/closure_comp... third_party/closure_compiler/checker.py:190: if create_source_map: On 2015/03/06 02:57:28, Dan Beam wrote: > i think we should always create source maps Done. https://codereview.chromium.org/958383003/diff/40001/chrome/browser/resources... File chrome/browser/resources/enhanced_bookmarks/get_salient_image_url.js (right): https://codereview.chromium.org/958383003/diff/40001/chrome/browser/resources... chrome/browser/resources/enhanced_bookmarks/get_salient_image_url.js:21: var jsonEncoder = JSON['stringify']; On 2015/03/06 02:57:28, Dan Beam wrote: > why is this necessary? shouldn't externs preserve these? The compiler was complaining about not being able to find JSON.encode, so I did a hacky fix since the changes to these files are getting reverted anyway. https://codereview.chromium.org/958383003/diff/40001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/958383003/diff/40001/chrome/chrome_browser.gy... chrome/chrome_browser.gypi:3388: 'browser/resources/enhanced_bookmarks/compiled_resources.gyp:*', On 2015/03/06 02:57:28, Dan Beam wrote: > this would force all chrome developers to install java I'm going to revert changes to everything except compile_js.gypi and checker.py - this was just thrown in for testing/as an example. Definitely will include the dependency somewhere else in the future CL https://codereview.chromium.org/958383003/diff/40001/third_party/closure_comp... File third_party/closure_compiler/checker.py (right): https://codereview.chromium.org/958383003/diff/40001/third_party/closure_comp... third_party/closure_compiler/checker.py:181: def run_js_check(self, sources, externs=None, out_file=None, On 2015/03/06 02:57:29, Dan Beam wrote: > i think if the code will be modified, there should be a source map, so couple > these options together (i.e. just treat a non-empty out_file as a desire to > generate output) Right now, the out_file is never empty. I'll look into modifying compile_js to only specify an out_file if the corresponding compiled_resources.gyp specifies output should be generated. https://codereview.chromium.org/958383003/diff/40001/third_party/closure_comp... third_party/closure_compiler/checker.py:316: if os.path.exists(out_dir) and os.listdir(out_dir) == []: On 2015/03/06 02:57:29, Dan Beam wrote: > why are we making empty directories? I think output.py is creating them when it generates the path for the output file.. I'll change compile_js to be smarter about when it's running output.py. https://codereview.chromium.org/958383003/diff/40001/third_party/closure_comp... third_party/closure_compiler/checker.py:320: os.makedirs(out_dir) On 2015/03/06 02:57:28, Dan Beam wrote: > so closure doesn't do this automatically? make a path if it doesn't exist? It might. I'll try removing this. I had copied it from 312:315 below without checking if it was needed.
yeah, actually... i'd say just always write the output and let people use it if they care. it's generated anyways (and just ignored currently). https://codereview.chromium.org/958383003/diff/40001/third_party/closure_comp... File third_party/closure_compiler/checker.py (right): https://codereview.chromium.org/958383003/diff/40001/third_party/closure_comp... third_party/closure_compiler/checker.py:181: def run_js_check(self, sources, externs=None, out_file=None, On 2015/03/06 03:07:19, twellington wrote: > On 2015/03/06 02:57:29, Dan Beam wrote: > > i think if the code will be modified, there should be a source map, so couple > > these options together (i.e. just treat a non-empty out_file as a desire to > > generate output) > > Right now, the out_file is never empty. I'll look into modifying compile_js to > only specify an out_file if the corresponding compiled_resources.gyp specifies > output should be generated. why not just write the output for all files?
+1, that's certainly simpler :) On Thu, Mar 5, 2015, 7:41 PM null <dbeam@chromium.org> wrote: > yeah, actually... i'd say just always write the output and let people use > it if > they care. it's generated anyways (and just ignored currently). > > > https://codereview.chromium.org/958383003/diff/40001/ > third_party/closure_compiler/checker.py > File third_party/closure_compiler/checker.py (right): > > https://codereview.chromium.org/958383003/diff/40001/ > third_party/closure_compiler/checker.py#newcode181 > third_party/closure_compiler/checker.py:181 > <https://codereview.chromium.org/958383003/diff/40001/third_party/closure_comp...>: > def run_js_check(self, > sources, externs=None, out_file=None, > On 2015/03/06 03:07:19, twellington wrote: > > On 2015/03/06 02:57:29, Dan Beam wrote: > > > i think if the code will be modified, there should be a source map, > so couple > > > these options together (i.e. just treat a non-empty out_file as a > desire to > > > generate output) > > > Right now, the out_file is never empty. I'll look into modifying > compile_js to > > only specify an out_file if the corresponding compiled_resources.gyp > specifies > > output should be generated. > > why not just write the output for all files? > > https://codereview.chromium.org/958383003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
ptal had some time while the London sheriff was still on duty; compiled JS and source map is always created. Reverted changes to example files. https://codereview.chromium.org/958383003/diff/40001/third_party/closure_comp... File third_party/closure_compiler/checker.py (right): https://codereview.chromium.org/958383003/diff/40001/third_party/closure_comp... third_party/closure_compiler/checker.py:47: '--compilation_level=WHITESPACE_ONLY' On 2015/03/06 02:57:29, Dan Beam wrote: > nit: trailing , Done. https://codereview.chromium.org/958383003/diff/40001/third_party/closure_comp... third_party/closure_compiler/checker.py:47: '--compilation_level=WHITESPACE_ONLY' On 2015/03/06 02:57:29, Dan Beam wrote: > ' -> " Done.
https://codereview.chromium.org/958383003/diff/60001/third_party/closure_comp... File third_party/closure_compiler/checker.py (left): https://codereview.chromium.org/958383003/diff/60001/third_party/closure_comp... third_party/closure_compiler/checker.py:314: os.makedirs(out_dir) still need to do this somewhere $ echo 'alert( 1 );' | java -jar compiler.jar --js_output_file=i/totally/dont/exist.js java.io.FileNotFoundException: i/totally/dont/exist.js (No such file or directory)
ptal https://codereview.chromium.org/958383003/diff/60001/third_party/closure_comp... File third_party/closure_compiler/checker.py (left): https://codereview.chromium.org/958383003/diff/60001/third_party/closure_comp... third_party/closure_compiler/checker.py:314: os.makedirs(out_dir) On 2015/03/06 19:58:55, Dan Beam wrote: > still need to do this somewhere > > $ echo 'alert( 1 );' | java -jar compiler.jar > --js_output_file=i/totally/dont/exist.js > > java.io.FileNotFoundException: i/totally/dont/exist.js (No such file or > directory) Done.
https://codereview.chromium.org/958383003/diff/80001/third_party/closure_comp... File third_party/closure_compiler/checker.py (right): https://codereview.chromium.org/958383003/diff/80001/third_party/closure_comp... third_party/closure_compiler/checker.py:311: open(opts.out_file, "w").close() this last part isn't needed, just the directory
On 2015/03/06 20:36:26, Dan Beam wrote: > https://codereview.chromium.org/958383003/diff/80001/third_party/closure_comp... > File third_party/closure_compiler/checker.py (right): > > https://codereview.chromium.org/958383003/diff/80001/third_party/closure_comp... > third_party/closure_compiler/checker.py:311: open(opts.out_file, "w").close() > this last part isn't needed, just the directory K, I'll remove it. Do we want to make the out_file a required parameter? Either by marking it as such in add_argument above, or doing a check, printing a message and exiting if it's empty?
On 2015/03/06 20:40:01, twellington wrote: > On 2015/03/06 20:36:26, Dan Beam wrote: > > > https://codereview.chromium.org/958383003/diff/80001/third_party/closure_comp... > > File third_party/closure_compiler/checker.py (right): > > > > > https://codereview.chromium.org/958383003/diff/80001/third_party/closure_comp... > > third_party/closure_compiler/checker.py:311: open(opts.out_file, "w").close() > > this last part isn't needed, just the directory > > K, I'll remove it. Do we want to make the out_file a required parameter? Either > by marking it as such in add_argument above, or doing a check, printing a > message and exiting if it's empty? Or make it optional by doing checks to make sure it's set before using it?
ptal Decided to add back the checks for if(opt_file) (keeping the argument optional, since it's currently optional)
https://codereview.chromium.org/958383003/diff/100001/third_party/closure_com... File third_party/closure_compiler/checker.py (right): https://codereview.chromium.org/958383003/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:182: def run_js_check(self, sources, out_file=None, externs=None): can you make this private (i.e. _run_js_check)? https://codereview.chromium.org/958383003/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:218: externs: @extern files that inform the compiler about custom globals. update py doc https://codereview.chromium.org/958383003/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:251: externs) out_file=out_file, externs=externs https://codereview.chromium.org/958383003/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:307: out_file = opts.out_file or None is this needed? i don't remember why i did this for depends/externs other than it's probably advised by the google python styleguide and/or to simplify code given that they exist as a [] or set()
ptal https://codereview.chromium.org/958383003/diff/100001/third_party/closure_com... File third_party/closure_compiler/checker.py (right): https://codereview.chromium.org/958383003/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:182: def run_js_check(self, sources, out_file=None, externs=None): On 2015/03/07 00:43:54, Dan Beam wrote: > can you make this private (i.e. _run_js_check)? Done. https://codereview.chromium.org/958383003/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:218: externs: @extern files that inform the compiler about custom globals. On 2015/03/07 00:43:54, Dan Beam wrote: > update py doc Done. https://codereview.chromium.org/958383003/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:251: externs) On 2015/03/07 00:43:54, Dan Beam wrote: > out_file=out_file, externs=externs Done. https://codereview.chromium.org/958383003/diff/100001/third_party/closure_com... third_party/closure_compiler/checker.py:307: out_file = opts.out_file or None On 2015/03/07 00:43:54, Dan Beam wrote: > is this needed? > > i don't remember why i did this for depends/externs other than it's probably > advised by the google python styleguide and/or to simplify code given that they > exist as a [] or set() It's not needed, removed.
lgtm https://codereview.chromium.org/958383003/diff/140001/third_party/closure_com... File third_party/closure_compiler/checker.py (right): https://codereview.chromium.org/958383003/diff/140001/third_party/closure_com... third_party/closure_compiler/checker.py:20: class Checker(object): i guess i'll have to rename this to Compile eventually now that it does more than just check ;) https://codereview.chromium.org/958383003/diff/140001/third_party/closure_com... third_party/closure_compiler/checker.py:216: out_file: A file to output results. A file where the compiled output is written to.
https://codereview.chromium.org/958383003/diff/140001/third_party/closure_com... File third_party/closure_compiler/checker.py (right): https://codereview.chromium.org/958383003/diff/140001/third_party/closure_com... third_party/closure_compiler/checker.py:216: out_file: A file to output results. On 2015/03/07 01:55:26, Dan Beam wrote: > A file where the compiled output is written to. Done.
The CQ bit was checked by twellington@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/958383003/#ps180001 (title: "Fix line wrap")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/958383003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) Timed out jobs are not retried to avoid causing additional load on the builders with large pending queues.
The CQ bit was checked by twellington@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/958383003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) Timed out jobs are not retried to avoid causing additional load on the builders with large pending queues.
The CQ bit was checked by twellington@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/958383003/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/b7f849b027b42917a032dca4f2467db357fa26a4 Cr-Commit-Position: refs/heads/master@{#319637}
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/989253003/ by treib@chromium.org. The reason for reverting is: Likely broke the build, e.g. http://build.chromium.org/p/chromium/buildstatus?builder=Linux&number=59264 http://build.chromium.org/p/chromium.mac/buildstatus?builder=Mac%20Builder&nu... http://build.chromium.org/p/chromium.linux/buildstatus?builder=Linux%20Builde....
Message was sent while issue was closed.
https://codereview.chromium.org/958383003/diff/180001/third_party/closure_com... File third_party/closure_compiler/checker.py (left): https://codereview.chromium.org/958383003/diff/180001/third_party/closure_com... third_party/closure_compiler/checker.py:243: errors, stderr = self.run_js_check([self._expanded_file], externs) oh, guess this is used somewhere else... |
