Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10378027:
Fix presubmit js style nits. (Closed)
|
Created:
8 years, 7 months ago by kmadhusu Modified:
8 years, 7 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix presubmit js style nits.
BUG=122012
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=136077
Patch Set 1 #
Total comments: 19
Patch Set 2 : Addressed review comments #
Total comments: 2
Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 5
Patch Set 5 : Reverted gpu_internals.js #
Messages
Total messages: 23 (0 generated)
Made changes to fix presubmit js style nits. Please review and let me know your comments. Thanks.
LGTM but confused on the $ change http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... File chrome/browser/resources/gpu_internals/info_view.js (left): http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... chrome/browser/resources/gpu_internals/info_view.js:211: document.getElementById('log-messages')); Why are we introducing a dependency on $ as part of a style fix? Does presubmit insist on using $? That's a bit hinky, imo.... our util.js' $ function is pretty different than how, e.g. a jquery person would read $...
http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... File chrome/browser/resources/gpu_internals/info_view.js (left): http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... chrome/browser/resources/gpu_internals/info_view.js:211: document.getElementById('log-messages')); On 2012/05/07 16:30:32, nduca wrote: > Why are we introducing a dependency on $ as part of a style fix? Does presubmit > insist on using $? That's a bit hinky, imo.... our util.js' $ function is pretty > different than how, e.g. a jquery person would read $... nduca: http://dev.chromium.org/developers/web-development-style-guide#TOC-JavaScript Btw, there was $() before jQuery(), ;) (prototype, mootools, etc.), and jQuery() is insanely overloaded (with the default case of {string} falling back to Sizzle()).
Then I think our style guide is wrong.
On 2012/05/07 17:21:42, nduca wrote: > Then I think our style guide is wrong. If you already include util.js then there is no reason to type document.getElementById. If you don't include util.js feel free to type 22 extra characters.
On 2012/05/07 17:21:42, nduca wrote:
> Then I think our style guide is wrong.
And in particular, if its mandated in the style guide, then the $ function
really shouldn't be living in an ancillary util.js --- it should be more
obviously part of cr.js, imo.
I think my big objection here is the aliasing of $ to getelementbyid. Yes, its
overloaded. But usually $('foo') means querySelector('foo'), such that the style
mandates $('#foo') for picking by ID.
http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... File chrome/browser/resources/gpu_internals/info_view.js (left): http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... chrome/browser/resources/gpu_internals/info_view.js:211: document.getElementById('log-messages')); On 2012/05/07 16:30:32, nduca wrote: > Why are we introducing a dependency on $ as part of a style fix? Does presubmit > insist on using $? That's a bit hinky, imo.... our util.js' $ function is pretty > different than how, e.g. a jquery person would read $... You make a good point. I think it's good to have this in the style guide so that code is more uniform (i.e. we don't have some code using $ while other code uses getElementById). But there will be also places where getElementById makes sense. So maybe we should make this particular check a presubmit *message* instead of a warning or error. That way people will see it and may choose to switch to $, but if they want to leave it as getElementById, they can do that too and the CQ won't be blocked. What do you think?
http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... File chrome/browser/resources/gpu_internals/browser_bridge_tests.js (right): http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... chrome/browser/resources/gpu_internals/browser_bridge_tests.js:9: 'basic_info': [ I think our style guide prefers non-quoted object literal keys when the properties aren't called by name anywhere else (so they can be renamed in closure compiler's ADVANCED_OPTIMIZATIONS mode). I don't how useful that is to chrome code if we don't minify, but we do seem to follow this practice lots of other places in the code... http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... chrome/browser/resources/gpu_internals/browser_bridge_tests.js:10: { this should probably only be +2\s indented from the beginning of basic_info http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... chrome/browser/resources/gpu_internals/browser_bridge_tests.js:60: 'value': 'GL_ARB_color_buffer_float ' + nit: it'd be less chars and possibly less error prone to join an array of words together with ' ' as the "glue" http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... chrome/browser/resources/gpu_internals/browser_bridge_tests.js:228: 'GL_SUN_slice_accum '}], the ending }] are strange looking to me http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... chrome/browser/resources/gpu_internals/browser_bridge_tests.js:239: } something about this indent is wrong, there's only 1 \s http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... chrome/browser/resources/gpu_internals/browser_bridge_tests.js:327: document.querySelector('#debug-div').appendChild(controlEl, I hope this is being deferred until DOMContentLoaded... http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... File chrome/browser/resources/gpu_internals/gpu_internals.js (right): http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... chrome/browser/resources/gpu_internals/gpu_internals.js:5: <include src='browser_bridge.js'/> you don't need to " -> ' for grit
http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... File chrome/browser/resources/gpu_internals/gpu_internals.js (right): http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... chrome/browser/resources/gpu_internals/gpu_internals.js:5: <include src='browser_bridge.js'/> On 2012/05/07 18:11:57, Dan Beam wrote: > you don't need to " -> ' for grit The presubmit is not flagging this on my machine.
> You make a good point. I think it's good to have this in the style guide so that > code is more uniform (i.e. we don't have some code using $ while other code uses > getElementById). But there will be also places where getElementById makes sense. > So maybe we should make this particular check a presubmit *message* instead of a > warning or error. I'm fine with whatever, tbh. I had grumpy pants on. You guys have a lot more experience with me on this, so now that I've had my rant, go for whatever you think is right. And, of course, none of this affects this actual CL, which already looks good from my PoV. :)
Addressed review comments. PTAL. Thanks. http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... File chrome/browser/resources/gpu_internals/browser_bridge_tests.js (right): http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... chrome/browser/resources/gpu_internals/browser_bridge_tests.js:9: 'basic_info': [ On 2012/05/07 18:11:57, Dan Beam wrote: > I think our style guide prefers non-quoted object literal keys when the > properties aren't called by name anywhere else (so they can be renamed in > closure compiler's ADVANCED_OPTIMIZATIONS mode). I don't how useful that is to > chrome code if we don't minify, but we do seem to follow this practice lots of > other places in the code... Done. http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... chrome/browser/resources/gpu_internals/browser_bridge_tests.js:10: { On 2012/05/07 18:11:57, Dan Beam wrote: > this should probably only be +2\s indented from the beginning of basic_info Done. http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... chrome/browser/resources/gpu_internals/browser_bridge_tests.js:60: 'value': 'GL_ARB_color_buffer_float ' + On 2012/05/07 18:11:57, Dan Beam wrote: > nit: it'd be less chars and possibly less error prone to join an array of words > together with ' ' as the "glue" Done. http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... chrome/browser/resources/gpu_internals/browser_bridge_tests.js:228: 'GL_SUN_slice_accum '}], On 2012/05/07 18:11:57, Dan Beam wrote: > the ending }] are strange looking to me Fixed. http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... chrome/browser/resources/gpu_internals/browser_bridge_tests.js:239: } On 2012/05/07 18:11:57, Dan Beam wrote: > something about this indent is wrong, there's only 1 \s Fixed. http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... chrome/browser/resources/gpu_internals/browser_bridge_tests.js:327: document.querySelector('#debug-div').appendChild(controlEl, On 2012/05/07 18:11:57, Dan Beam wrote: > I hope this is being deferred until DOMContentLoaded... nduca@: This comment is associated with implementation details, Can you address this comment? http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... File chrome/browser/resources/gpu_internals/gpu_internals.js (right): http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... chrome/browser/resources/gpu_internals/gpu_internals.js:5: <include src='browser_bridge.js'/> On 2012/05/07 18:11:57, Dan Beam wrote: > you don't need to " -> ' for grit Reverted. http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... File chrome/browser/resources/gpu_internals/info_view.js (left): http://codereview.chromium.org/10378027/diff/1/chrome/browser/resources/gpu_i... chrome/browser/resources/gpu_internals/info_view.js:211: document.getElementById('log-messages')); On 2012/05/07 17:26:57, Tyler Breisacher wrote: > On 2012/05/07 16:30:32, nduca wrote: > > Why are we introducing a dependency on $ as part of a style fix? Does > presubmit > > insist on using $? That's a bit hinky, imo.... our util.js' $ function is > pretty > > different than how, e.g. a jquery person would read $... > > You make a good point. I think it's good to have this in the style guide so that > code is more uniform (i.e. we don't have some code using $ while other code uses > getElementById). But there will be also places where getElementById makes sense. > So maybe we should make this particular check a presubmit *message* instead of a > warning or error. That way people will see it and may choose to switch to $, but > if they want to leave it as getElementById, they can do that too and the CQ > won't be blocked. What do you think? Since util.js is already included, I am leaving this change as it is. http://codereview.chromium.org/10378027/diff/13001/chrome/browser/resources/g... File chrome/browser/resources/gpu_internals/browser_bridge_tests.js (right): http://codereview.chromium.org/10378027/diff/13001/chrome/browser/resources/g... chrome/browser/resources/gpu_internals/browser_bridge_tests.js:252: {'status': 'enabled', name: 'webgl'}, status is a reserved js keyword. Therefore I retained the single quotes.
lgtm w/nits http://codereview.chromium.org/10378027/diff/13002/chrome/browser/resources/g... File chrome/browser/resources/gpu_internals/browser_bridge_tests.js (right): http://codereview.chromium.org/10378027/diff/13002/chrome/browser/resources/g... chrome/browser/resources/gpu_internals/browser_bridge_tests.js:249: {'status': 'enabled', name: '2d_canvas'}, thank you for removing ' from around key names, but there are move if you'd like, ;) http://codereview.chromium.org/10378027/diff/13002/chrome/browser/resources/g... File chrome/browser/resources/gpu_internals/gpu_internals.js (right): http://codereview.chromium.org/10378027/diff/13002/chrome/browser/resources/g... chrome/browser/resources/gpu_internals/gpu_internals.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. revert this change unless the file actually was edited (before this) in 2012
http://codereview.chromium.org/10378027/diff/13002/chrome/browser/resources/g... File chrome/browser/resources/gpu_internals/browser_bridge_tests.js (right): http://codereview.chromium.org/10378027/diff/13002/chrome/browser/resources/g... chrome/browser/resources/gpu_internals/browser_bridge_tests.js:249: {'status': 'enabled', name: '2d_canvas'}, On 2012/05/08 18:03:27, Dan Beam wrote: > thank you for removing ' from around key names, but there are move if you'd > like, ;) s/move/more/
On 2012/05/08 17:59:35, kmadhusu wrote: > http://codereview.chromium.org/10378027/diff/13001/chrome/browser/resources/g... > chrome/browser/resources/gpu_internals/browser_bridge_tests.js:252: {'status': > 'enabled', name: 'webgl'}, > status is a reserved js keyword. Therefore I retained the single quotes. Are you sure? I don't see it on https://developer.mozilla.org/en/JavaScript/Reference/Reserved_Words Some text editors will highlight things as though they're keywords, even when they're not... Anyway, leave the single quotes or take them out, I don't care that much either way as long as we're moving in the general direction of more consistent code. lgtm
http://codereview.chromium.org/10378027/diff/13001/chrome/browser/resources/g... File chrome/browser/resources/gpu_internals/browser_bridge_tests.js (right): http://codereview.chromium.org/10378027/diff/13001/chrome/browser/resources/g... chrome/browser/resources/gpu_internals/browser_bridge_tests.js:252: {'status': 'enabled', name: 'webgl'}, On 2012/05/08 17:59:36, kmadhusu wrote: > status is a reserved js keyword. Therefore I retained the single quotes. static is, but status isn't AFAICT - http://es5.github.com/#x7.6.1.1
additionally, all reserved words can be key names in ES5, i.e.
(function(){ 'use strict'; return {if: 'then', else: 'foo'}; }());
http://codereview.chromium.org/10378027/diff/13002/chrome/browser/resources/g... File chrome/browser/resources/gpu_internals/browser_bridge_tests.js (right): http://codereview.chromium.org/10378027/diff/13002/chrome/browser/resources/g... chrome/browser/resources/gpu_internals/browser_bridge_tests.js:249: {'status': 'enabled', name: '2d_canvas'}, On 2012/05/08 18:03:27, Dan Beam wrote: > thank you for removing ' from around key names, but there are move if you'd > like, ;) As per http://www.quackit.com/javascript/javascript_reserved_words.cfm, 'status' is a reserved javascript keyword. Even my editor highlighted this keyword. Therefore, I am leaving this as it is. http://codereview.chromium.org/10378027/diff/13002/chrome/browser/resources/g... File chrome/browser/resources/gpu_internals/gpu_internals.js (right): http://codereview.chromium.org/10378027/diff/13002/chrome/browser/resources/g... chrome/browser/resources/gpu_internals/gpu_internals.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/05/08 18:03:27, Dan Beam wrote: > revert this change unless the file actually was edited (before this) in 2012 Done.
On 2012/05/08 20:27:03, kmadhusu wrote: > As per http://www.quackit.com/javascript/javascript_reserved_words.cfm, 'status' > is a reserved javascript keyword. The last table on that page is misleading at best. 'status' isn't a reserved word or a keyword, it's just that 'window.status' is already defined, just like window.alert or window.parseInt.
On 2012/05/08 20:36:30, Tyler Breisacher wrote: > On 2012/05/08 20:27:03, kmadhusu wrote: > > As per http://www.quackit.com/javascript/javascript_reserved_words.cfm, > 'status' > > is a reserved javascript keyword. > > The last table on that page is misleading at best. 'status' isn't a reserved > word or a keyword, it's just that 'window.status' is already defined, just like > window.alert or window.parseInt. Yeah after discussing with you and dbeam@ I realized that the table is misleading.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/10378027/14005
Try job failure for 10378027-14005 (retry) (retry) on win_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/10378027/14005
Change committed as 136077 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
