Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10356042:
Fix presubmit js style nits. (Closed)
|
Created:
8 years, 7 months ago by kmadhusu Modified:
8 years, 7 months ago Reviewers:
mazda, Nikita (slow), Dan Beam, Ivan Korotkov, Dmitry Polukhin, xiyuan, zel, arv (Not doing code reviews), Tom Sepez, Tyler Breisacher (Chromium) CC:
chromium-reviews, arv (Not doing code reviews), nkostylev+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix presubmit js style nits.
BUG=122003
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=136782
Patch Set 1 #Patch Set 2 : '' #
Total comments: 2
Patch Set 3 : '' #
Total comments: 29
Patch Set 4 : Addressed review comments #Patch Set 5 : '' #Patch Set 6 : Reverted menu.js and network_menu.js changes #Patch Set 7 : Reverted document.getElementById changes in keyboard_overlay.js #
Total comments: 24
Patch Set 8 : Addressed review comments #
Messages
Total messages: 21 (0 generated)
Made code changes to fix presubmit js style errors. Most of them are style nits and others were related to function headers. Please read the function headers and let me know if you want add a detailed description about the function and its params. OWNERs CHECK dbeam@ image_burner.js tsepez@ menu.js wrench_menu.js oshima@chromium.org mobile_setup.js zelidrag@ network_menu.js xiyuan@ sim_unlock nkostylev@ user_images_grid ivankr@ keyboard_overlay mazda@ choose_media_network.js dpolukhin@ Thanks. http://codereview.chromium.org/10356042/diff/2001/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/network_menu.js (right): http://codereview.chromium.org/10356042/diff/2001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/network_menu.js:33: * @param {number} index Selected network index. xiyuan: Can you provide a detailed description about these params?
lgtm http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/user_images_grid.js (right): http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/user_images_grid.js:6: var ArrayDataModel = cr.ui.ArrayDataModel; Please use the "/** @const */ var ... = ..." style.
keyboard_overlay: lgtm Thank you for the fix!
wrench_menu.js should have been removed. I probably missed it when I removed the feature. LGTM for now and I'll remove it later.
network_menu.js LGTM http://codereview.chromium.org/10356042/diff/2001/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/network_menu.js (right): http://codereview.chromium.org/10356042/diff/2001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/network_menu.js:33: * @param {number} index Selected network index. On 2012/05/07 05:42:03, kmadhusu wrote: > xiyuan: Can you provide a detailed description about these params? index Index of the selected network menu item. passphrase Wifi network password for the selected network if applicable. For other networks, use an empty string. identify Wifi network SSID if applicable. For other networks, use an emtpy string. auto_connect Whether the network can be auto connected. Let me know if this would work.
http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/mobile_setup.js (left): http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/mobile_setup.js:13: MobileSetup.PLAN_ACTIVATION_PAGE_LOADING = -1; why removing spaces here? how's the making the code more readable?
http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/mobile_setup.js (left): http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/mobile_setup.js:13: MobileSetup.PLAN_ACTIVATION_PAGE_LOADING = -1; On 2012/05/07 16:41:32, zel wrote: > why removing spaces here? how's the making the code more readable? This is required by the linter that runs as part of the presubmit. Do you think we should turn off this rule, because of cases like this? http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/mobile_setup.js (right): http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/mobile_setup.js:263: console.log('Portal frame load error detected: ' + errorCode); It's often a good idea to change this kind of thing to: console.log('Portal frame load error detected: ', errorCode); because then the console can display errorCode in a more useful way, if errorCode is an object, or a DOM element or something. In this case, it's just a number (I would assume) so it doesn't really matter, but just fyi if you see other console.log calls you want to fix up...
FYI http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/mobile_setup.js (right): http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/mobile_setup.js:43: faked_transaction_: false, no underscores in JS names
If I'm not mistaken, the trailing underscore is still okay, right? So this should be fakedTransaction_?
On 2012/05/07 17:27:47, Tyler Breisacher wrote: > If I'm not mistaken, the trailing underscore is still okay, right? So this > should be fakedTransaction_? yes, private members - http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...
http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/menu.js (right): http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/menu.js:637: * downwards and -1 for upwards. nit: indent this continuation of @param +4\s http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/menu.js:640: * nit: remove empty comment line http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/menu.js:724: * See chromeos/views/native_menu_webui.cc for details. nit: remove extra \s or add 3\s, additionally there is a @see JSDoc directive, IIRC. http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/mobile_setup.js (left): http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/mobile_setup.js:13: MobileSetup.PLAN_ACTIVATION_PAGE_LOADING = -1; On 2012/05/07 16:41:32, zel wrote: > why removing spaces here? how's the making the code more readable? http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Code_fo... http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/network_menu.js (right): http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/network_menu.js:7: var statusConnected = 'connected'; /** @const */ var statusConnected = 'connected'; http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/network_menu.js:34: * @param {string} passphrase Password I'm not sure passphrase is quite the same as password, but if you could make this a full sentence (or somehow end it in a .) that'd be great. http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/network_menu.js:36: * @param {boolean} auto_connect True if the network can be auto connected. can you change auto_connect to autoConnect? http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/wrench_menu.js (right): http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/wrench_menu.js:153: * @extends {HTMLDivElement} it seems that this actually extends ButtonCommand, no? http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/wrench_menu.js:158: __proto__: ButtonCommand.prototype, here's the start of the prototype chain http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/wrench_menu.js:290: * @extends {HTMLDivElement} same here with this probably extending Menu.
http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/image_burner.js (right): http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/image_burner.js:91: * @this {State} These @this's shouldn't be necessary because we're inside the State.prototype object literal. There is a little bit of weirdness about when the linter flags this, so I'm just disabling it for now: http://codereview.chromium.org/10356042/
oshima@ pointed that wrench_menu.js, menu.js and network_menu.js are obselete files. Therefore, I reverted my changes in those files. Addressed all other review comments. PTAL. Thanks. http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/image_burner.js (right): http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/image_burner.js:91: * @this {State} On 2012/05/07 20:37:03, Tyler Breisacher wrote: > These @this's shouldn't be necessary because we're inside the State.prototype > object literal. There is a little bit of weirdness about when the linter flags > this, so I'm just disabling it for now: http://codereview.chromium.org/10356042/ Removed. http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/menu.js (right): http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/menu.js:637: * downwards and -1 for upwards. On 2012/05/07 17:43:53, Dan Beam wrote: > nit: indent this continuation of @param +4\s Done. http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/menu.js:640: * On 2012/05/07 17:43:53, Dan Beam wrote: > nit: remove empty comment line Done. http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/menu.js:724: * See chromeos/views/native_menu_webui.cc for details. On 2012/05/07 17:43:53, Dan Beam wrote: > nit: remove extra \s or add 3\s, additionally there is a @see JSDoc directive, > IIRC. Done. http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/mobile_setup.js (right): http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/mobile_setup.js:43: faked_transaction_: false, On 2012/05/07 17:26:03, arv wrote: > no underscores in JS names Done. http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/mobile_setup.js:263: console.log('Portal frame load error detected: ' + errorCode); On 2012/05/07 17:12:25, Tyler Breisacher wrote: > It's often a good idea to change this kind of thing to: > > console.log('Portal frame load error detected: ', errorCode); > > because then the console can display errorCode in a more useful way, if > errorCode is an object, or a DOM element or something. In this case, it's just a > number (I would assume) so it doesn't really matter, but just fyi if you see > other console.log calls you want to fix up... Done. http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/network_menu.js (right): http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/network_menu.js:7: var statusConnected = 'connected'; On 2012/05/07 17:43:53, Dan Beam wrote: > /** @const */ var statusConnected = 'connected'; Done. http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/network_menu.js:34: * @param {string} passphrase Password On 2012/05/07 17:43:53, Dan Beam wrote: > I'm not sure passphrase is quite the same as password, but if you could make > this a full sentence (or somehow end it in a .) that'd be great. Fixed. http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/network_menu.js:36: * @param {boolean} auto_connect True if the network can be auto connected. On 2012/05/07 17:43:53, Dan Beam wrote: > can you change auto_connect to autoConnect? Done. http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/user_images_grid.js (right): http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/user_images_grid.js:6: var ArrayDataModel = cr.ui.ArrayDataModel; On 2012/05/07 10:50:07, Ivan Korotkov wrote: > Please use the "/** @const */ var ... = ..." style. Done. http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/wrench_menu.js (right): http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/wrench_menu.js:153: * @extends {HTMLDivElement} On 2012/05/07 17:43:53, Dan Beam wrote: > it seems that this actually extends ButtonCommand, no? Reverted my changes. http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/wrench_menu.js:158: __proto__: ButtonCommand.prototype, On 2012/05/07 17:43:53, Dan Beam wrote: > here's the start of the prototype chain Reverted my changes. http://codereview.chromium.org/10356042/diff/4001/chrome/browser/resources/ch... chrome/browser/resources/chromeos/wrench_menu.js:290: * @extends {HTMLDivElement} On 2012/05/07 17:43:53, Dan Beam wrote: > same here with this probably extending Menu. Since we are going to remove this file in the near future, I reverted all my changes.
LGTM for choose_mobile_network.js
https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... File chrome/browser/resources/chromeos/image_burner.js (right): https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/image_burner.js:101: if (newState.warningText) { nit: no curlies https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/image_burner.js:118: if (deviceCount == 0) { nit: no curlies https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/image_burner.js:132: return (this.state == State.StatesEnum.DEVICE_NONE || nit: remove () https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/image_burner.js:166: * Returns the selected device count. Shouldn't this say: Shows the currently selected device. ? https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/image_burner.js:380: * @param {Object} update_signal Specifies the signal information. nit: updateSignal https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/image_burner.js:418: * @param {number} device_size Received device size. nit: deviceSize https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... File chrome/browser/resources/chromeos/keyboard_overlay.js (right): https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/keyboard_overlay.js:106: if (!e) { nit: no curlies https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/keyboard_overlay.js:122: if (modifierWithKeyCode) { nit: no curlies https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/keyboard_overlay.js:196: * @param {string} c Character A character to be checked. https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/keyboard_overlay.js:197: * @return {boolean} True if the character is a ASCII character. an ASCII https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/keyboard_overlay.js:256: var separatorStr = '<>'; in theory you'd still have /** @const */ here https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/keyboard_overlay.js:528: * @param {Event} e Mouse click events. event.
note to self: all other files looks good
dbeam@: Addressed review comments. PTAL. Thanks. https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... File chrome/browser/resources/chromeos/image_burner.js (right): https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/image_burner.js:101: if (newState.warningText) { On 2012/05/11 01:06:09, Dan Beam wrote: > nit: no curlies Done. https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/image_burner.js:118: if (deviceCount == 0) { On 2012/05/11 01:06:09, Dan Beam wrote: > nit: no curlies Done. https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/image_burner.js:132: return (this.state == State.StatesEnum.DEVICE_NONE || On 2012/05/11 01:06:09, Dan Beam wrote: > nit: remove () Done. https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/image_burner.js:166: * Returns the selected device count. On 2012/05/11 01:06:09, Dan Beam wrote: > Shouldn't this say: > > Shows the currently selected device. > > ? Done. https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/image_burner.js:380: * @param {Object} update_signal Specifies the signal information. On 2012/05/11 01:06:09, Dan Beam wrote: > nit: updateSignal Done. https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/image_burner.js:418: * @param {number} device_size Received device size. On 2012/05/11 01:06:09, Dan Beam wrote: > nit: deviceSize Done. https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... File chrome/browser/resources/chromeos/keyboard_overlay.js (right): https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/keyboard_overlay.js:106: if (!e) { On 2012/05/11 01:06:09, Dan Beam wrote: > nit: no curlies Done. https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/keyboard_overlay.js:122: if (modifierWithKeyCode) { On 2012/05/11 01:06:09, Dan Beam wrote: > nit: no curlies Done. https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/keyboard_overlay.js:196: * @param {string} c Character On 2012/05/11 01:06:09, Dan Beam wrote: > A character to be checked. Done. https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/keyboard_overlay.js:197: * @return {boolean} True if the character is a ASCII character. On 2012/05/11 01:06:09, Dan Beam wrote: > an ASCII Done. https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/keyboard_overlay.js:256: var separatorStr = '<>'; On 2012/05/11 01:06:09, Dan Beam wrote: > in theory you'd still have /** @const */ here Done. https://chromiumcodereview.appspot.com/10356042/diff/17001/chrome/browser/res... chrome/browser/resources/chromeos/keyboard_overlay.js:528: * @param {Event} e Mouse click events. On 2012/05/11 01:06:09, Dan Beam wrote: > event. Done.
lgtm
zel@, nkostylev@: ping
lgtm
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
