Compile chrome://settings, part 6

Compile chrome://settings, part 6

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

[Dan Beam, 2014-09-12 03:25:20]

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
File chrome/browser/resources/chromeos/user_images_grid.js (right):

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
chrome/browser/resources/chromeos/user_images_grid.js:32: *     clickHandler:
(!Function|undefined)}. Contains user image URL,
why are you making this more loosely typed?

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
File chrome/browser/resources/options/browser_options.js (right):

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
chrome/browser/resources/options/browser_options.js:653:
document.body.addEventListener('click', function(e) {
var target = assertInstanceof(e.target, Node);

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
File chrome/browser/resources/options/chromeos/accounts_user_name_edit.js
(right):

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
chrome/browser/resources/options/chromeos/accounts_user_name_edit.js:111:
assert(e);
i'd just remove this.  i have no idea why this would ever be called with null
and it'd blow up on the next line.

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
File chrome/browser/resources/options/chromeos/display_options.js (right):

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
chrome/browser/resources/options/chromeos/display_options.js:276:
e.preventDefault();
var target = assertInstanceof(e.target, HTMLElement);

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
chrome/browser/resources/options/chromeos/display_options.js:295:
this.lastTouchLocation_ = {x: touch.pageX, y: touch.pageY};
var target = assertInstanceof(e.target, HTMLElement);

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
chrome/browser/resources/options/chromeos/internet_detail.js:358: *     {value:
*, controlledBy: *, recommendedValue: *}.
why?

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
File chrome/browser/resources/options/chromeos/network_list.js (right):

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
chrome/browser/resources/options/chromeos/network_list.js:912: entry));
indent off

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
File chrome/browser/resources/options/chromeos/onc_data.js (right):

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
chrome/browser/resources/options/chromeos/onc_data.js:79: * @return
{(Object|Array|string|undefined)} The property value or
@typedef

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
File chrome/browser/resources/options/chromeos/preferred_networks.js (right):

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
chrome/browser/resources/options/chromeos/preferred_networks.js:71: * @type
{?{Name: string, Type: string, servicePath: string}}
@typedef

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
File chrome/browser/resources/options/pref_ui.js (right):

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
chrome/browser/resources/options/pref_ui.js:332: this.metric);
Preferences.setIntegerPref(
    this.pref,
    this.mapPositionToPref(parseInt(this.value, 10)),
    !this.dialogPref,
    this.metric);

[Vitaly Pavlenko, 2014-09-12 19:16:23]

https://codereview.chromium.org/570503002/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/user_images_grid.js (right):

https://codereview.chromium.org/570503002/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/user_images_grid.js:32: *     clickHandler:
(!Function|undefined)}. Contains user image URL,
On 2014/09/12 03:25:19, Dan Beam wrote:
> why are you making this more loosely typed?

I previously had issues with line 127:
  
          this.itemConstructor = UserImagesGridItem;

Fixed now.

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

https://codereview.chromium.org/570503002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/browser_options.js:653:
document.body.addEventListener('click', function(e) {
On 2014/09/12 03:25:19, Dan Beam wrote:
> var target = assertInstanceof(e.target, Node);

Done.

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

https://codereview.chromium.org/570503002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/accounts_user_name_edit.js:111:
assert(e);
On 2014/09/12 03:25:19, Dan Beam wrote:
> i'd just remove this.  i have no idea why this would ever be called with null
> and it'd blow up on the next line.

Done.

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

https://codereview.chromium.org/570503002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/display_options.js:276:
e.preventDefault();
On 2014/09/12 03:25:19, Dan Beam wrote:
> var target = assertInstanceof(e.target, HTMLElement);

Done.

https://codereview.chromium.org/570503002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/display_options.js:295:
this.lastTouchLocation_ = {x: touch.pageX, y: touch.pageY};
On 2014/09/12 03:25:19, Dan Beam wrote:
> var target = assertInstanceof(e.target, HTMLElement);

Done.

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

https://codereview.chromium.org/570503002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/internet_detail.js:358: *     {value:
*, controlledBy: *, recommendedValue: *}.
On 2014/09/12 03:25:19, Dan Beam wrote:
> why?

Otherwise I need a cast on line 1447 (added), which adds extra code but doesn't
increase type safety.

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

https://codereview.chromium.org/570503002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/network_list.js:912: entry));
On 2014/09/12 03:25:19, Dan Beam wrote:
> indent off

This way?

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

https://codereview.chromium.org/570503002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/onc_data.js:79: * @return
{(Object|Array|string|undefined)} The property value or
On 2014/09/12 03:25:20, Dan Beam wrote:
> @typedef

Done.

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

https://codereview.chromium.org/570503002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/preferred_networks.js:71: * @type
{?{Name: string, Type: string, servicePath: string}}
On 2014/09/12 03:25:20, Dan Beam wrote:
> @typedef

Done.

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

https://codereview.chromium.org/570503002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/pref_ui.js:332: this.metric);
On 2014/09/12 03:25:20, Dan Beam wrote:
> Preferences.setIntegerPref(
>     this.pref,
>     this.mapPositionToPref(parseInt(this.value, 10)),
>     !this.dialogPref,
>     this.metric);

Done.

[Dan Beam, 2014-09-16 01:04:15]

lgtm w/whining

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
File chrome/browser/resources/options/chromeos/network_list.js (right):

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
chrome/browser/resources/options/chromeos/network_list.js:912: entry));
On 2014/09/12 19:16:23, Vitaly Pavlenko wrote:
> On 2014/09/12 03:25:19, Dan Beam wrote:
> > indent off
> 
> This way?

yes

https://chromiumcodereview.appspot.com/570503002/diff/20001/chrome/browser/re...
File chrome/browser/resources/chromeos/user_images_grid.js (right):

https://chromiumcodereview.appspot.com/570503002/diff/20001/chrome/browser/re...
chrome/browser/resources/chromeos/user_images_grid.js:129: /** @type
{function(new:cr.ui.ListItem, Object)} */(
so the compiler can't infer that a GridItem is-a ListItem?

[Vitaly Pavlenko, 2014-09-16 03:11:21]

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
File chrome/browser/resources/options/chromeos/network_list.js (right):

https://chromiumcodereview.appspot.com/570503002/diff/1/chrome/browser/resour...
chrome/browser/resources/options/chromeos/network_list.js:912: entry));
On 2014/09/16 01:04:14, Dan Beam wrote:
> On 2014/09/12 19:16:23, Vitaly Pavlenko wrote:
> > On 2014/09/12 03:25:19, Dan Beam wrote:
> > > indent off
> > 
> > This way?
> 
> yes

Acknowledged.

https://chromiumcodereview.appspot.com/570503002/diff/20001/chrome/browser/re...
File chrome/browser/resources/chromeos/user_images_grid.js (right):

https://chromiumcodereview.appspot.com/570503002/diff/20001/chrome/browser/re...
chrome/browser/resources/chromeos/user_images_grid.js:129: /** @type
{function(new:cr.ui.ListItem, Object)} */(
On 2014/09/16 01:04:14, Dan Beam wrote:
> so the compiler can't infer that a GridItem is-a ListItem?

No, the problem is with constructor argument specification:

##
/usr/local/google/home/vitalyp/src/chrome/browser/resources/chromeos/user_images_grid.js:128:
ERROR - assignment to property itemConstructor of options.UserImagesGrid
## found   : function (new:UserImagesGridItem, {clickHandler:
(Function|undefined), decorateFn: (Function|undefined), title:
(string|undefined), url: string, ...}): ?
## required: function (new:cr.ui.ListItem, (Object|null)): ?
##       this.itemConstructor = UserImagesGridItem;
##       ^

[Vitaly Pavlenko, 2014-09-18 22:06:07]

The CQ bit was checked by vitalyp@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/570503002/40001

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

Committed patchset #3 (id:40001) as de4ba78e5cdb381cfce94e6bac03ea9b6e3324b6

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

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

[stevenjb, 2014-11-05 20:30:14]

stevenjb@chromium.org changed reviewers:
+ stevenjb@chromium.org

[stevenjb, 2014-11-05 20:30:15]

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

https://codereview.chromium.org/570503002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:1574: propValue =
assertInstanceof(propValue, Object);
While I do appreciate this effort, this actually introduced a subtle bug causing
the details page not to load at all. If you could include me on the review of
any internet options related changes in the future I may be able to catch these
in advance. (FYI, fix is here: https://codereview.chromium.org/701243002/).
Thanks!