Compile chrome://settings, part 3: 167 proper errors left

Compile chrome://settings, part 3: 167 proper errors left

R=dbeam@chromium.org
BUG=393873
TEST=GYP_GENERATORS=ninja gyp --depth . chrome/browser/resources/options/compiled_resources.gyp && ninja -C out/Default | grep ERROR | wc -l

Committed: https://crrev.com/44894ce48d3386bf9e2232d42cac14b59c787dd7
Cr-Commit-Position: refs/heads/master@{#294963}

[Dan Beam, 2014-09-09 03:10:29]

will get to this tonight or (more likely) tomorrow

[Dan Beam, 2014-09-11 02:53:09]

https://codereview.chromium.org/553573003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/autofill_edit_creditcard_overlay.js
(right):

https://codereview.chromium.org/553573003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/autofill_edit_creditcard_overlay.js:113: var
text;
var text = (i < 10 ? '0' : '') + i;

https://codereview.chromium.org/553573003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/autofill_edit_creditcard_overlay.js:121:
option.value = text;
option.text = options.value = text;

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/bluetooth_device_list.js (left):

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/bluetooth_device_list.js:253:
marginBotton: 0,
is this used?

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/bluetooth_device_list.js (right):

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/bluetooth_device_list.js:50: * @type
{{name: string,
nit: doesn't this need a nullable ?

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/bluetooth_device_list.js:59: *        
entered: (number|undefined)}}
@typedef this (maybe as BluetoohDevice?) and share with appendDevice

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/display_options.js (right):

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/display_options.js:25:
cr.exportPath('options');
why not options.{DisplayInfo,SecondaryDisplayLayout} with this exportPath()
higher up?

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/confirm_dialog.js (right):

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/confirm_dialog.js:21: * @param {string=}
opt_confirmed_pref A pref used to remember whether the
nit: opt_confirmedPref (confirmed_pref -> confirmedPref)

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/confirm_dialog.js:32: this.confirmed_pref =
opt_confirmed_pref;
nit: this.confirmed_pref -> this.confirmedPref_

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/controlled_setting.js (right):

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/controlled_setting.js:75: 'hasRecommendation' :
'';
nit: indent + 4\s more

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/supervised_user_list.js (right):

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/supervised_user_list.js:101: * @param {Object}
entry
^ why does this need a @param if it's @override?

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/sync_setup_overlay.js (right):

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
chrome/browser/resources/sync_setup_overlay.js:46: var SyncArgs;
nit: SyncConfig

[Vitaly Pavlenko, 2014-09-11 04:16:25]

https://codereview.chromium.org/553573003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/autofill_edit_creditcard_overlay.js
(right):

https://codereview.chromium.org/553573003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/autofill_edit_creditcard_overlay.js:113: var
text;
On 2014/09/11 02:53:08, Dan Beam wrote:
> var text = (i < 10 ? '0' : '') + i;

Done.

https://codereview.chromium.org/553573003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/autofill_edit_creditcard_overlay.js:121:
option.value = text;
On 2014/09/11 02:53:08, Dan Beam wrote:
> option.text = options.value = text;

Done.

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/bluetooth_device_list.js (left):

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/bluetooth_device_list.js:253:
marginBotton: 0,
On 2014/09/11 02:53:08, Dan Beam wrote:
> is this used?

Rarely:

https://code.google.com/p/chromium/codesearch#search/&q=measureItem%5C(%5C)&s...

Particularly, I see no point where marginBottom/marginRight is used.

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/bluetooth_device_list.js (right):

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/bluetooth_device_list.js:50: * @type
{{name: string,
On 2014/09/11 02:53:08, Dan Beam wrote:
> nit: doesn't this need a nullable ?

Done.

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/bluetooth_device_list.js:59: *        
entered: (number|undefined)}}
On 2014/09/11 02:53:08, Dan Beam wrote:
> @typedef this (maybe as BluetoohDevice?) and share with appendDevice

Done.

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/display_options.js (right):

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/display_options.js:25:
cr.exportPath('options');
On 2014/09/11 02:53:09, Dan Beam wrote:
> why not options.{DisplayInfo,SecondaryDisplayLayout} with this exportPath()
> higher up?

Done for SecondaryDisplayLayout.

I don't include @typedef's into namespaces so far. Should I?

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/confirm_dialog.js (right):

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/confirm_dialog.js:21: * @param {string=}
opt_confirmed_pref A pref used to remember whether the
On 2014/09/11 02:53:09, Dan Beam wrote:
> nit: opt_confirmedPref (confirmed_pref -> confirmedPref)

Done.

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/confirm_dialog.js:32: this.confirmed_pref =
opt_confirmed_pref;
On 2014/09/11 02:53:09, Dan Beam wrote:
> nit: this.confirmed_pref -> this.confirmedPref_

Done.

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/controlled_setting.js (right):

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/controlled_setting.js:75: 'hasRecommendation' :
'';
On 2014/09/11 02:53:09, Dan Beam wrote:
> nit: indent + 4\s more

This way?

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/supervised_user_list.js (right):

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/supervised_user_list.js:101: * @param {Object}
entry
On 2014/09/11 02:53:09, Dan Beam wrote:
> ^ why does this need a @param if it's @override?

Hm, at least it works. Btw, there's a lot of overloaded createItem's, and every
waits for its own type. So I either write this type inside @param or do a cast
in the function body. Which way is better?

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/sync_setup_overlay.js (right):

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
chrome/browser/resources/sync_setup_overlay.js:46: var SyncArgs;
On 2014/09/11 02:53:09, Dan Beam wrote:
> nit: SyncConfig

Done.

[Dan Beam, 2014-09-11 05:26:13]

lgtm w/nits

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/display_options.js (right):

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/display_options.js:25:
cr.exportPath('options');
On 2014/09/11 04:16:25, Vitaly Pavlenko wrote:
> On 2014/09/11 02:53:09, Dan Beam wrote:
> > why not options.{DisplayInfo,SecondaryDisplayLayout} with this exportPath()
> > higher up?
> 
> Done for SecondaryDisplayLayout.
> 
> I don't include @typedef's into namespaces so far. Should I?

if the @typedef's never used outside of the namespace, probably better to, yeah

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/supervised_user_list.js (right):

https://codereview.chromium.org/553573003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/supervised_user_list.js:101: * @param {Object}
entry
On 2014/09/11 04:16:25, Vitaly Pavlenko wrote:
> On 2014/09/11 02:53:09, Dan Beam wrote:
> > ^ why does this need a @param if it's @override?
> 
> Hm, at least it works. Btw, there's a lot of overloaded createItem's, and
every
> waits for its own type. So I either write this type inside @param or do a cast
> in the function body. Which way is better?

this is fine

https://codereview.chromium.org/553573003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/bluetooth_device_list.js (right):

https://codereview.chromium.org/553573003/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/bluetooth_device_list.js:279:
handleClick_: function(e) {
this is overriding a @private member in its base class :(, should probably make
protected or do something different here

https://codereview.chromium.org/553573003/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/bluetooth_device_list.js:283: var
target = /** @type {HTMLElement} */(e.target);
generally this is right because of where the base class attaches the listener

https://codereview.chromium.org/553573003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/confirm_dialog.js (right):

https://codereview.chromium.org/553573003/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/confirm_dialog.js:32: this.confirmedPref =
opt_confirmedPref;
why isn't this (and maybe other members) @private?

https://codereview.chromium.org/553573003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/website_settings.js (right):

https://codereview.chromium.org/553573003/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:91: this.allowedList_ =
allowedList;
did you mean to have an assertInstanceof() here?

[Vitaly Pavlenko, 2014-09-11 21:12:31]

https://chromiumcodereview.appspot.com/553573003/diff/40001/chrome/browser/re...
File chrome/browser/resources/options/chromeos/display_options.js (right):

https://chromiumcodereview.appspot.com/553573003/diff/40001/chrome/browser/re...
chrome/browser/resources/options/chromeos/display_options.js:25:
cr.exportPath('options');
On 2014/09/11 05:26:12, Dan Beam wrote:
> On 2014/09/11 04:16:25, Vitaly Pavlenko wrote:
> > On 2014/09/11 02:53:09, Dan Beam wrote:
> > > why not options.{DisplayInfo,SecondaryDisplayLayout} with this
exportPath()
> > > higher up?
> > 
> > Done for SecondaryDisplayLayout.
> > 
> > I don't include @typedef's into namespaces so far. Should I?
> 
> if the @typedef's never used outside of the namespace, probably better to,
yeah

Done.

https://chromiumcodereview.appspot.com/553573003/diff/40001/chrome/browser/re...
File chrome/browser/resources/options/supervised_user_list.js (right):

https://chromiumcodereview.appspot.com/553573003/diff/40001/chrome/browser/re...
chrome/browser/resources/options/supervised_user_list.js:101: * @param {Object}
entry
On 2014/09/11 05:26:12, Dan Beam wrote:
> On 2014/09/11 04:16:25, Vitaly Pavlenko wrote:
> > On 2014/09/11 02:53:09, Dan Beam wrote:
> > > ^ why does this need a @param if it's @override?
> > 
> > Hm, at least it works. Btw, there's a lot of overloaded createItem's, and
> every
> > waits for its own type. So I either write this type inside @param or do a
cast
> > in the function body. Which way is better?
> 
> this is fine

Acknowledged.

https://chromiumcodereview.appspot.com/553573003/diff/60001/chrome/browser/re...
File chrome/browser/resources/options/chromeos/bluetooth_device_list.js (right):

https://chromiumcodereview.appspot.com/553573003/diff/60001/chrome/browser/re...
chrome/browser/resources/options/chromeos/bluetooth_device_list.js:279:
handleClick_: function(e) {
On 2014/09/11 05:26:12, Dan Beam wrote:
> this is overriding a @private member in its base class :(, should probably
make
> protected or do something different here

Done.

https://chromiumcodereview.appspot.com/553573003/diff/60001/chrome/browser/re...
chrome/browser/resources/options/chromeos/bluetooth_device_list.js:283: var
target = /** @type {HTMLElement} */(e.target);
On 2014/09/11 05:26:12, Dan Beam wrote:
> generally this is right because of where the base class attaches the listener

Yes, I think it's right on specific methods like |handleClick|, where we can go
and check that the listener is attached to the element and cannot be fired on
Document/Window.

https://chromiumcodereview.appspot.com/553573003/diff/60001/chrome/browser/re...
File chrome/browser/resources/options/confirm_dialog.js (right):

https://chromiumcodereview.appspot.com/553573003/diff/60001/chrome/browser/re...
chrome/browser/resources/options/confirm_dialog.js:32: this.confirmedPref =
opt_confirmedPref;
On 2014/09/11 05:26:12, Dan Beam wrote:
> why isn't this (and maybe other members) @private?

I'm not sure for |metric| and |pref|. |confirmedPref| is used only in this file,
so I added underscore to it.

https://chromiumcodereview.appspot.com/553573003/diff/60001/chrome/browser/re...
File chrome/browser/resources/options/website_settings.js (right):

https://chromiumcodereview.appspot.com/553573003/diff/60001/chrome/browser/re...
chrome/browser/resources/options/website_settings.js:91: this.allowedList_ =
allowedList;
On 2014/09/11 05:26:12, Dan Beam wrote:
> did you mean to have an assertInstanceof() here?

Done.

[Dan Beam, 2014-09-11 22:51:09]

slgtm w/more nits ;)

https://chromiumcodereview.appspot.com/553573003/diff/60001/chrome/browser/re...
File chrome/browser/resources/options/confirm_dialog.js (right):

https://chromiumcodereview.appspot.com/553573003/diff/60001/chrome/browser/re...
chrome/browser/resources/options/confirm_dialog.js:32: this.confirmedPref =
opt_confirmedPref;
On 2014/09/11 21:12:31, Vitaly Pavlenko wrote:
> On 2014/09/11 05:26:12, Dan Beam wrote:
> > why isn't this (and maybe other members) @private?
> 
> I'm not sure for |metric| and |pref|. |confirmedPref| is used only in this
file,
> so I added underscore to it.

/** @protected */
this.pref = pref;

/** @protected */
this.metric = metric;

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

https://chromiumcodereview.appspot.com/553573003/diff/80001/chrome/browser/re...
File chrome/browser/resources/options/chromeos/bluetooth_device_list.js (right):

https://chromiumcodereview.appspot.com/553573003/diff/80001/chrome/browser/re...
chrome/browser/resources/options/chromeos/bluetooth_device_list.js:278: *
@protected
i think all of this can be /** @override */ more or less

https://chromiumcodereview.appspot.com/553573003/diff/80001/chrome/browser/re...
File chrome/browser/resources/options/confirm_dialog.js (right):

https://chromiumcodereview.appspot.com/553573003/diff/80001/chrome/browser/re...
chrome/browser/resources/options/confirm_dialog.js:31: this.metric = metric;
/** @private */

https://chromiumcodereview.appspot.com/553573003/diff/80001/chrome/browser/re...
File chrome/browser/resources/options/deletable_item_list.js (right):

https://chromiumcodereview.appspot.com/553573003/diff/80001/chrome/browser/re...
chrome/browser/resources/options/deletable_item_list.js:147: * @protected
@override

[Vitaly Pavlenko, 2014-09-11 23:13:09]

https://chromiumcodereview.appspot.com/553573003/diff/60001/chrome/browser/re...
File chrome/browser/resources/options/confirm_dialog.js (right):

https://chromiumcodereview.appspot.com/553573003/diff/60001/chrome/browser/re...
chrome/browser/resources/options/confirm_dialog.js:32: this.confirmedPref =
opt_confirmedPref;
On 2014/09/11 22:51:08, Dan Beam wrote:
> On 2014/09/11 21:12:31, Vitaly Pavlenko wrote:
> > On 2014/09/11 05:26:12, Dan Beam wrote:
> > > why isn't this (and maybe other members) @private?
> > 
> > I'm not sure for |metric| and |pref|. |confirmedPref| is used only in this
> file,
> > so I added underscore to it.
> 
> /** @protected */
> this.pref = pref;
> 
> /** @protected */
> this.metric = metric;
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

Doneish. Still remember that @private and @protected have no sense for our
current typechecking system.

https://chromiumcodereview.appspot.com/553573003/diff/80001/chrome/browser/re...
File chrome/browser/resources/options/chromeos/bluetooth_device_list.js (right):

https://chromiumcodereview.appspot.com/553573003/diff/80001/chrome/browser/re...
chrome/browser/resources/options/chromeos/bluetooth_device_list.js:278: *
@protected
On 2014/09/11 22:51:08, Dan Beam wrote:
> i think all of this can be /** @override */ more or less

Done.

https://chromiumcodereview.appspot.com/553573003/diff/80001/chrome/browser/re...
File chrome/browser/resources/options/confirm_dialog.js (right):

https://chromiumcodereview.appspot.com/553573003/diff/80001/chrome/browser/re...
chrome/browser/resources/options/confirm_dialog.js:31: this.metric = metric;
On 2014/09/11 22:51:08, Dan Beam wrote:
> /** @private */

Done.

https://chromiumcodereview.appspot.com/553573003/diff/80001/chrome/browser/re...
File chrome/browser/resources/options/deletable_item_list.js (right):

https://chromiumcodereview.appspot.com/553573003/diff/80001/chrome/browser/re...
chrome/browser/resources/options/deletable_item_list.js:147: * @protected
On 2014/09/11 22:51:08, Dan Beam wrote:
> @override

Done.

[Vitaly Pavlenko, 2014-09-12 23:26:17]

The CQ bit was checked by vitalyp@chromium.org

[Vitaly Pavlenko, 2014-09-12 23:26:24]

The CQ bit was unchecked by vitalyp@chromium.org

[Vitaly Pavlenko, 2014-09-12 23:26:28]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-12 23:46:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/553573003/100001

[commit-bot: I haz the power, 2014-09-13 01:31:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-13 01:31:07]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Vitaly Pavlenko, 2014-09-15 17:06:16]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-15 17:18:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/553573003/120001

[Vitaly Pavlenko, 2014-09-15 19:19:31]

The CQ bit was unchecked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-15 19:24:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-15 19:24:01]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Vitaly Pavlenko, 2014-09-15 22:25:01]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-15 22:39:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/553573003/140001

[Vitaly Pavlenko, 2014-09-15 22:45:51]

The CQ bit was unchecked by vitalyp@chromium.org

[Vitaly Pavlenko, 2014-09-15 23:40:23]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-16 00:01:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/553573003/160001

[commit-bot: I haz the power, 2014-09-16 01:33:00]

Committed patchset #9 (id:160001) as d7ffbcceda2c668229517c8659d553b5abbc22fa

[commit-bot: I haz the power, 2014-09-16 01:34:40]

Patchset 9 (id:??) landed as
https://crrev.com/44894ce48d3386bf9e2232d42cac14b59c787dd7
Cr-Commit-Position: refs/heads/master@{#294963}