Compile chrome://settings, part 2: reduce from 950 to 400 errors

Compile chrome://settings, part 2: reduce from 950 to 400 errors

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/e49301cf668c7f5ef8f2fac348a6eb2e981c3b61
Cr-Commit-Position: refs/heads/master@{#294690}

[Dan Beam, 2014-09-06 02:22:39]

some good finds...

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:1559: * @param {{value:
number, disabled: boolean, controlledBy: string}} pref
might be worth a typedef between here and L1538

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/accounts_user_name_edit.js
(right):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/accounts_user_name_edit.js:70: *
@return {?{name: string, email: string}} User info parsed from the
isn't this the default?

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:265: var params =
parseQueryParams(window.location);
^ can you just put this into showNetworkDetails_ now instead of passing a param?

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:272: * is included
in the URL.
"@param {Object} params" missing

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:332: var activeApn
= {};
did this work before?

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:718:
handleAutoConfigProxy_: function(opt_e) {
can you just make these methods not take a parameter if they're unused anyways?

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:933: /** @type
{HTMLInputElement} */($('prefer-network-wifi')));
assertInstanceof, IMO

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:1448:
$('buyplan-details').hidden = true;
it doesn't seem like this button is ever used (nor its C++ handling code).  can
you try to remove?

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/content_settings_exceptions_area.js:230: } else
{
nit:

  if (!this.isEmbeddingRule())
    return this.dataItem.origin;

  return loadTimeData.getStringF('embeddedOnHost',
                                 this.dataItem.embeddingOrigin);

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/font_settings.js (right):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/font_settings.js:185: value = items[i][0];
nit: I'd say just put var instead here, i.e.

  var value = items[i][0];

it's not really making a block-level variable, but it is the first rule in the
style guide:
https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#var

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/manage_profile_overlay.js (left):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/manage_profile_overlay.js:402: };
it'd probably be better just to remove the ; at the end

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/options_page.js (right):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:31: while (tab &&
!tab.classList.contains('subpages-nav-tabs') &&
maybe add a check for tab.classList here?

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/password_manager.js (right):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/password_manager.js:142: * @param {!cu.ui.List}
list The list to toggle visilibility for.
uh oh

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/website_settings.js (left):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/website_settings.js:43: var target =
event.target;
hmmmm, wonder if this worked before...

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
File ui/webui/resources/js/cr/ui/autocomplete_list.js (right):

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/autocomplete_list.js:171: false);
can you just make this param optional?  positionPopupAroundRect already is.

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
File ui/webui/resources/js/cr/ui/bubble_button.js (right):

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/bubble_button.js:87: toggleBubble_: undefined
nit: ,

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/bubble_button.js:87: toggleBubble_: undefined
@protected methods aren't suffixed with _, only @private

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/bubble_button.js:87: toggleBubble_: undefined
nit: maybe instead:

  toggleBubble: assertNotReached,

?

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
File ui/webui/resources/js/cr/ui/grid.js (left):

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/grid.js:40: ListItem.prototype.decorate.call(this,
arguments);
what's wrong with this?

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
File ui/webui/resources/js/cr/ui/repeating_button.js (right):

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/repeating_button.js:47: intervalCallbackID_:
undefined,
nit: ID -> Id in both cases

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/repeating_button.js:100: var armRepeaterCallback_ =
function() {
nit: remove _

[Vitaly Pavlenko, 2014-09-06 22:54:09]

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:1559: * @param {{value:
number, disabled: boolean, controlledBy: string}} pref
On 2014/09/06 02:22:36, Dan Beam wrote:
> might be worth a typedef between here and L1538

I don't think so. These are formed and called by two different places in
.cc-source:

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

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/accounts_user_name_edit.js
(right):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/accounts_user_name_edit.js:70: *
@return {?{name: string, email: string}} User info parsed from the
On 2014/09/06 02:22:36, Dan Beam wrote:
> isn't this the default?

No.
https://gist.github.com/vpavlenko/45226ac2e3876f87b094

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:265: var params =
parseQueryParams(window.location);
On 2014/09/06 02:22:36, Dan Beam wrote:
> ^ can you just put this into showNetworkDetails_ now instead of passing a
param?

Done.

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:272: * is included
in the URL.
On 2014/09/06 02:22:36, Dan Beam wrote:
> "@param {Object} params" missing

Done.

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:332: var activeApn
= {};
On 2014/09/06 02:22:36, Dan Beam wrote:
> did this work before?

Yes, and I don't know how. Should I contact the code owner?

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:718:
handleAutoConfigProxy_: function(opt_e) {
On 2014/09/06 02:22:36, Dan Beam wrote:
> can you just make these methods not take a parameter if they're unused
anyways?

Done.

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:933: /** @type
{HTMLInputElement} */($('prefer-network-wifi')));
On 2014/09/06 02:22:36, Dan Beam wrote:
> assertInstanceof, IMO

Done.

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:1448:
$('buyplan-details').hidden = true;
On 2014/09/06 02:22:36, Dan Beam wrote:
> it doesn't seem like this button is ever used (nor its C++ handling code). 
can
> you try to remove?

Do you know how can I open this internet_detail.html screen?
The button is still in UI:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/content_settings_exceptions_area.js
(right):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/content_settings_exceptions_area.js:230: } else
{
On 2014/09/06 02:22:36, Dan Beam wrote:
> nit:
> 
>   if (!this.isEmbeddingRule())
>     return this.dataItem.origin;
> 
>   return loadTimeData.getStringF('embeddedOnHost',
>                                  this.dataItem.embeddingOrigin);

Done.

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/font_settings.js (right):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/font_settings.js:185: value = items[i][0];
On 2014/09/06 02:22:36, Dan Beam wrote:
> nit: I'd say just put var instead here, i.e.
> 
>   var value = items[i][0];
> 
> it's not really making a block-level variable, but it is the first rule in the
> style guide:
> https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#var

Done.

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/manage_profile_overlay.js (left):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/manage_profile_overlay.js:402: };
On 2014/09/06 02:22:36, Dan Beam wrote:
> it'd probably be better just to remove the ; at the end

##
/usr/local/google/home/vitalyp/src/chrome/browser/resources/options/manage_profile_overlay.js:390:
ERROR - functions can only be declared at top level or immediately within
another function in ES5 strict mode
##           function getImportHandler(supervisedUser, nameIsUnique) {
##           ^
##

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/options_page.js (right):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:31: while (tab &&
!tab.classList.contains('subpages-nav-tabs') &&
On 2014/09/06 02:22:36, Dan Beam wrote:
> maybe add a check for tab.classList here?

Why do we need to? I think classList is present in any Element, isn't it?
https://developer.mozilla.org/en-US/docs/Web/API/Element.classList

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/password_manager.js (right):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/password_manager.js:142: * @param {!cu.ui.List}
list The list to toggle visilibility for.
On 2014/09/06 02:22:38, Dan Beam wrote:
> uh oh

Done.

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
File ui/webui/resources/js/cr/ui/autocomplete_list.js (right):

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/autocomplete_list.js:171: false);
On 2014/09/06 02:22:38, Dan Beam wrote:
> can you just make this param optional?  positionPopupAroundRect already is.

Done.

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
File ui/webui/resources/js/cr/ui/bubble_button.js (right):

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/bubble_button.js:87: toggleBubble_: undefined
On 2014/09/06 02:22:39, Dan Beam wrote:
> @protected methods aren't suffixed with _, only @private

Done.

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/bubble_button.js:87: toggleBubble_: undefined
On 2014/09/06 02:22:39, Dan Beam wrote:
> nit: maybe instead:
> 
>   toggleBubble: assertNotReached,
> 
> ?

Done.

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/bubble_button.js:87: toggleBubble_: undefined
On 2014/09/06 02:22:39, Dan Beam wrote:
> nit: ,

Done.

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
File ui/webui/resources/js/cr/ui/grid.js (left):

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/grid.js:40: ListItem.prototype.decorate.call(this,
arguments);
On 2014/09/06 02:22:39, Dan Beam wrote:
> what's wrong with this?

1) ListItem.prototype.decorate() needs no arguments:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...

2) Function.prototype.call() takes arguments one by one, not the array of
arguments:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...

3) Nevertheless, I don't understand why Closure Compiler whines the words it did
on this line:
## /usr/local/google/home/vitalyp/src/ui/webui/resources/js/cr/ui/grid.js:40:
ERROR - Function ListItem.prototype.decorate.call: called with 2 argument(s).
Function requires at least 0 argument(s) and no more than 1 argument(s).
##       ListItem.prototype.decorate.call(this, arguments);
##       ^

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
File ui/webui/resources/js/cr/ui/repeating_button.js (right):

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/repeating_button.js:47: intervalCallbackID_:
undefined,
On 2014/09/06 02:22:39, Dan Beam wrote:
> nit: ID -> Id in both cases

Done.

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/repeating_button.js:100: var armRepeaterCallback_ =
function() {
On 2014/09/06 02:22:39, Dan Beam wrote:
> nit: remove _

Done.

[Dan Beam, 2014-09-09 02:59:10]

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/accounts_user_name_edit.js
(right):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/accounts_user_name_edit.js:70: *
@return {?{name: string, email: string}} User info parsed from the
On 2014/09/06 22:54:07, Vitaly Pavlenko wrote:
> On 2014/09/06 02:22:36, Dan Beam wrote:
> > isn't this the default?
> 
> No.
> https://gist.github.com/vpavlenko/45226ac2e3876f87b094

the literal notation ({key: type}) is non-nullable while Object is nullable
(e.g. /** @type {Object} */ var a = null;  // succeeds).  weird.

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:265: var params =
parseQueryParams(window.location);
On 2014/09/06 22:54:08, Vitaly Pavlenko wrote:
> On 2014/09/06 02:22:36, Dan Beam wrote:
> > ^ can you just put this into showNetworkDetails_ now instead of passing a
> param?
> 
> Done.

i meant call parseQueryParams() inside showNetworkDetails_

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:332: var activeApn
= {};
On 2014/09/06 22:54:07, Vitaly Pavlenko wrote:
> On 2014/09/06 02:22:36, Dan Beam wrote:
> > did this work before?
> 
> Yes, and I don't know how. Should I contact the code owner?

stevenjb@ said he's fixing this

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:1448:
$('buyplan-details').hidden = true;
On 2014/09/06 22:54:07, Vitaly Pavlenko wrote:
> On 2014/09/06 02:22:36, Dan Beam wrote:
> > it doesn't seem like this button is ever used (nor its C++ handling code). 
> can
> > you try to remove?
> 
> Do you know how can I open this internet_detail.html screen?
> The button is still in UI:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

stevenjb@ is removing this, I think

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/manage_profile_overlay.js (left):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/manage_profile_overlay.js:402: };
On 2014/09/06 22:54:08, Vitaly Pavlenko wrote:
> On 2014/09/06 02:22:36, Dan Beam wrote:
> > it'd probably be better just to remove the ; at the end
> 
> ##
>
/usr/local/google/home/vitalyp/src/chrome/browser/resources/options/manage_profile_overlay.js:390:
> ERROR - functions can only be declared at top level or immediately within
> another function in ES5 strict mode
> ##           function getImportHandler(supervisedUser, nameIsUnique) {
> ##           ^
> ## 

probably still better to put at the top of the function instead

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/options_page.js (right):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:31: while (tab &&
!tab.classList.contains('subpages-nav-tabs') &&
On 2014/09/06 22:54:08, Vitaly Pavlenko wrote:
> On 2014/09/06 02:22:36, Dan Beam wrote:
> > maybe add a check for tab.classList here?
> 
> Why do we need to? I think classList is present in any Element, isn't it?
> https://developer.mozilla.org/en-US/docs/Web/API/Element.classList

document.documentElement.parentNode.classList is undefined because
document.documentElement (<html>)'s parent is the document (a Node, not Element)

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
File ui/webui/resources/js/cr/ui/grid.js (left):

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/grid.js:40: ListItem.prototype.decorate.call(this,
arguments);
On 2014/09/06 22:54:09, Vitaly Pavlenko wrote:
> On 2014/09/06 02:22:39, Dan Beam wrote:
> > what's wrong with this?
> 
> 1) ListItem.prototype.decorate() needs no arguments:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...
> 
> 2) Function.prototype.call() takes arguments one by one, not the array of
> arguments:
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...
> 
> 3) Nevertheless, I don't understand why Closure Compiler whines the words it
did
> on this line:
> ## /usr/local/google/home/vitalyp/src/ui/webui/resources/js/cr/ui/grid.js:40:
> ERROR - Function ListItem.prototype.decorate.call: called with 2 argument(s).
> Function requires at least 0 argument(s) and no more than 1 argument(s).
> ##       ListItem.prototype.decorate.call(this, arguments);
> ##       ^

oh, that should be .apply(), not .call().  it's fine as is if the compiler
recognizes the arg count.

https://codereview.chromium.org/543493002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/543493002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:252:
showNetworkDetails_: function(params) {
remove |params| and instead:

var params = parseQueryParams(window.location);

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

https://codereview.chromium.org/543493002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/options_page.js:31: while (tab &&
!tab.classList.contains('subpages-nav-tabs') &&
i don't know how this code works if it doesn't find a .tab or .subpages-nav-tabs
element (seems like it'd blow up)

https://codereview.chromium.org/543493002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/options_page.js:33: tab = /** @type {Element}
*/(tab.parentNode);
this cast is wrong

https://codereview.chromium.org/543493002/diff/20001/ui/webui/resources/js/cr...
File ui/webui/resources/js/cr/ui/bubble_button.js (right):

https://codereview.chromium.org/543493002/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/bubble_button.js:85: * @private
this can't be private [1], keep @protected just remove _

[1] https://code.google.com/p/chromium/codesearch#search/&q=toggleBubble

[Vitaly Pavlenko, 2014-09-09 17:54:52]

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/accounts_user_name_edit.js
(right):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/accounts_user_name_edit.js:70: *
@return {?{name: string, email: string}} User info parsed from the
On 2014/09/09 02:59:09, Dan Beam wrote:
> On 2014/09/06 22:54:07, Vitaly Pavlenko wrote:
> > On 2014/09/06 02:22:36, Dan Beam wrote:
> > > isn't this the default?
> > 
> > No.
> > https://gist.github.com/vpavlenko/45226ac2e3876f87b094
> 
> the literal notation ({key: type}) is non-nullable while Object is nullable
> (e.g. /** @type {Object} */ var a = null;  // succeeds).  weird.

Acknowledged.

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:265: var params =
parseQueryParams(window.location);
On 2014/09/09 02:59:09, Dan Beam wrote:
> On 2014/09/06 22:54:08, Vitaly Pavlenko wrote:
> > On 2014/09/06 02:22:36, Dan Beam wrote:
> > > ^ can you just put this into showNetworkDetails_ now instead of passing a
> > param?
> > 
> > Done.
> 
> i meant call parseQueryParams() inside showNetworkDetails_

Done.

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:332: var activeApn
= {};
On 2014/09/09 02:59:09, Dan Beam wrote:
> On 2014/09/06 22:54:07, Vitaly Pavlenko wrote:
> > On 2014/09/06 02:22:36, Dan Beam wrote:
> > > did this work before?
> > 
> > Yes, and I don't know how. Should I contact the code owner?
> 
> stevenjb@ said he's fixing this

Acknowledged.

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:1448:
$('buyplan-details').hidden = true;
On 2014/09/09 02:59:09, Dan Beam wrote:
> On 2014/09/06 22:54:07, Vitaly Pavlenko wrote:
> > On 2014/09/06 02:22:36, Dan Beam wrote:
> > > it doesn't seem like this button is ever used (nor its C++ handling code).

> > can
> > > you try to remove?
> > 
> > Do you know how can I open this internet_detail.html screen?
> > The button is still in UI:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> 
> stevenjb@ is removing this, I think

Acknowledged.

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/manage_profile_overlay.js (left):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/manage_profile_overlay.js:402: };
On 2014/09/09 02:59:09, Dan Beam wrote:
> On 2014/09/06 22:54:08, Vitaly Pavlenko wrote:
> > On 2014/09/06 02:22:36, Dan Beam wrote:
> > > it'd probably be better just to remove the ; at the end
> > 
> > ##
> >
>
/usr/local/google/home/vitalyp/src/chrome/browser/resources/options/manage_profile_overlay.js:390:
> > ERROR - functions can only be declared at top level or immediately within
> > another function in ES5 strict mode
> > ##           function getImportHandler(supervisedUser, nameIsUnique) {
> > ##           ^
> > ## 
> 
> probably still better to put at the top of the function instead

Not sure, if I'm doing it right. Please, have a look.

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/options_page.js (right):

https://codereview.chromium.org/543493002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:31: while (tab &&
!tab.classList.contains('subpages-nav-tabs') &&
On 2014/09/09 02:59:09, Dan Beam wrote:
> On 2014/09/06 22:54:08, Vitaly Pavlenko wrote:
> > On 2014/09/06 02:22:36, Dan Beam wrote:
> > > maybe add a check for tab.classList here?
> > 
> > Why do we need to? I think classList is present in any Element, isn't it?
> > https://developer.mozilla.org/en-US/docs/Web/API/Element.classList
> 
> document.documentElement.parentNode.classList is undefined because
> document.documentElement (<html>)'s parent is the document (a Node, not
Element)

Done.

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
File ui/webui/resources/js/cr/ui/grid.js (left):

https://codereview.chromium.org/543493002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/grid.js:40: ListItem.prototype.decorate.call(this,
arguments);
On 2014/09/09 02:59:09, Dan Beam wrote:
> On 2014/09/06 22:54:09, Vitaly Pavlenko wrote:
> > On 2014/09/06 02:22:39, Dan Beam wrote:
> > > what's wrong with this?
> > 
> > 1) ListItem.prototype.decorate() needs no arguments:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...
> > 
> > 2) Function.prototype.call() takes arguments one by one, not the array of
> > arguments:
> >
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...
> > 
> > 3) Nevertheless, I don't understand why Closure Compiler whines the words it
> did
> > on this line:
> > ##
/usr/local/google/home/vitalyp/src/ui/webui/resources/js/cr/ui/grid.js:40:
> > ERROR - Function ListItem.prototype.decorate.call: called with 2
argument(s).
> > Function requires at least 0 argument(s) and no more than 1 argument(s).
> > ##       ListItem.prototype.decorate.call(this, arguments);
> > ##       ^
> 
> oh, that should be .apply(), not .call().  it's fine as is if the compiler
> recognizes the arg count.

Done.

https://codereview.chromium.org/543493002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/543493002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:252:
showNetworkDetails_: function(params) {
On 2014/09/09 02:59:09, Dan Beam wrote:
> remove |params| and instead:
> 
> var params = parseQueryParams(window.location);

Even "var params" can be inlined, is it ok?

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

https://codereview.chromium.org/543493002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/options_page.js:31: while (tab &&
!tab.classList.contains('subpages-nav-tabs') &&
On 2014/09/09 02:59:09, Dan Beam wrote:
> i don't know how this code works if it doesn't find a .tab or
.subpages-nav-tabs
> element (seems like it'd blow up)

I hope I've rewritten it so it doesn't blow up. Should we do something else?

https://codereview.chromium.org/543493002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/options_page.js:33: tab = /** @type {Element}
*/(tab.parentNode);
On 2014/09/09 02:59:09, Dan Beam wrote:
> this cast is wrong

Done.

https://codereview.chromium.org/543493002/diff/20001/ui/webui/resources/js/cr...
File ui/webui/resources/js/cr/ui/bubble_button.js (right):

https://codereview.chromium.org/543493002/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/bubble_button.js:85: * @private
On 2014/09/09 02:59:09, Dan Beam wrote:
> this can't be private [1], keep @protected just remove _
> 
> [1] https://code.google.com/p/chromium/codesearch#search/&q=toggleBubble

Done, in all files where it's used.

[Dan Beam, 2014-09-09 19:05:48]

lgtm

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

https://codereview.chromium.org/543493002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/manage_profile_overlay.js:369: }
}.bind(this);

https://codereview.chromium.org/543493002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/manage_profile_overlay.js:409: .bind(this);
move bind() to getImportHandler_();

[Vitaly Pavlenko, 2014-09-09 21:26:05]

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

https://codereview.chromium.org/543493002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/manage_profile_overlay.js:409: .bind(this);
On 2014/09/09 19:05:48, Dan Beam wrote:
> move bind() to getImportHandler_();

Done.

[Vitaly Pavlenko, 2014-09-09 21:26:11]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-09 21:31:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vitalyp@chromium.org/543493002/60001

[Vitaly Pavlenko, 2014-09-09 21:42:49]

The CQ bit was unchecked by vitalyp@chromium.org

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

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-09 22:06:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vitalyp@chromium.org/543493002/80001

[commit-bot: I haz the power, 2014-09-09 22:19:22]

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

[commit-bot: I haz the power, 2014-09-09 22:19:23]

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

[Vitaly Pavlenko, 2014-09-10 22:58:39]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-10 23:17:57]

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

[commit-bot: I haz the power, 2014-09-11 00:02:47]

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

[commit-bot: I haz the power, 2014-09-11 00:02:49]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Dan Beam, 2014-09-11 04:18:35]

i'm committing https://codereview.chromium.org/561873004/ to unblock the
presubmit error

[Dan Beam, 2014-09-11 16:57:34]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2014-09-11 16:58:54]

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

[commit-bot: I haz the power, 2014-09-11 17:29:46]

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

[commit-bot: I haz the power, 2014-09-11 17:30:01]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  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-11 18:16:38]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-11 18:19:47]

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

[commit-bot: I haz the power, 2014-09-11 18:38:47]

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

[commit-bot: I haz the power, 2014-09-11 18:38:49]

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

[Vitaly Pavlenko, 2014-09-11 18:49:19]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-11 18:55:43]

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

[commit-bot: I haz the power, 2014-09-12 00:58:12]

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

[commit-bot: I haz the power, 2014-09-12 00:58:14]

Exceeded time limit waiting for builds to trigger.

[Vitaly Pavlenko, 2014-09-12 00:59:38]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-12 01:03:12]

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

[commit-bot: I haz the power, 2014-09-12 02:39:21]

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

[commit-bot: I haz the power, 2014-09-12 02:39:23]

Try jobs failed on following builders:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Vitaly Pavlenko, 2014-09-12 02:50:34]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-12 02:52:01]

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

[commit-bot: I haz the power, 2014-09-12 04:07:06]

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

[commit-bot: I haz the power, 2014-09-12 04:07:08]

Try jobs failed on following builders:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Vitaly Pavlenko, 2014-09-12 18:15:55]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-12 18:19:19]

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

[commit-bot: I haz the power, 2014-09-12 20:34:30]

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

[commit-bot: I haz the power, 2014-09-12 20:34:32]

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-12 22:28:51]

The CQ bit was checked by vitalyp@chromium.org

[commit-bot: I haz the power, 2014-09-12 22:29:24]

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

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

Committed patchset #11 (id:190001) as 4785e1d37334b8c5c2b700d3ccc3a6ae1a551ff2

[commit-bot: I haz the power, 2014-09-12 23:18:10]

Patchset 11 (id:??) landed as
https://crrev.com/e49301cf668c7f5ef8f2fac348a6eb2e981c3b61
Cr-Commit-Position: refs/heads/master@{#294690}