Settings People Revamp: Switch Change Picture to use iron-selector.

Settings People Revamp: Switch Change Picture to use iron-selector.

This reduces the lines of code and makes everything a lot clearer.

BUG=563721
Committed: https://crrev.com/e8a54f9640bda9da78d9ce6f7280669c311609ef
Cr-Commit-Position: refs/heads/master@{#371068}

[tommycli, 2016-01-22 01:18:38]

tommycli@chromium.org changed reviewers:
+ dpapad@chromium.org

[tommycli, 2016-01-22 01:18:38]

dpapad: PTAL. Apologize for the code churn... but I think this model is actually
better.

Thanks,

Tommy

[dpapad, 2016-01-22 02:35:32]

https://codereview.chromium.org/1615093003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1615093003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:45: * @private
{!string}
Nit, "!" is implied with string, number and boolean types. See
https://developers.google.com/closure/compiler/docs/js-for-compiler?hl=en,
specifically this part

"Functions and all value types (boolean, number, and string) are non-nullable by
default whether or not they are declared with the Non-nullable operator."

https://codereview.chromium.org/1615093003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:111: var images
= this.$.selector.items;
Nit (optional): You can also do it as follows (I think is less work).

var index = this.$.selector.items.findIndex(
    function(image) {
      return image.dataset.type == 'default' && image.src == imageUrl;
    });
assertTrue(index != -1, .....);
this.$.selector.select(index);

https://codereview.chromium.org/1615093003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:131:
this.$.selector.select(this.$.selector.indexOf(this.$.oldImage));
iron-selector#indexOf returns a number, but iron-selector#select is documented
as expecting a string. Is the documentation not accurate or is there an implicit
conversion happening here?

https://codereview.chromium.org/1615093003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:195: return
!oldImageUrl;
This can only return false if oldImageUrl is the empty string (null/undefined
are ruled out by the type annotation). So I suggest making the check more
explicit here to avoid implicit string to bool conversion.

return oldImageUrl.length == 0;

https://codereview.chromium.org/1615093003/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/change_picture_browsertest_chromeos.js
(right):

https://codereview.chromium.org/1615093003/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/change_picture_browsertest_chromeos.js:59: *
@param {!function()} action is executed after the listener is set up.
@return missing

[tommycli, 2016-01-22 19:24:41]

dpapad: thanks for the super fast review

https://codereview.chromium.org/1615093003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1615093003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:45: * @private
{!string}
On 2016/01/22 02:35:32, dpapad wrote:
> Nit, "!" is implied with string, number and boolean types. See
> https://developers.google.com/closure/compiler/docs/js-for-compiler?hl=en,
> specifically this part
> 
> "Functions and all value types (boolean, number, and string) are non-nullable
by
> default whether or not they are declared with the Non-nullable operator."

Done. Oops! Thanks!

https://codereview.chromium.org/1615093003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:111: var images
= this.$.selector.items;
On 2016/01/22 02:35:32, dpapad wrote:
> Nit (optional): You can also do it as follows (I think is less work).
> 
> var index = this.$.selector.items.findIndex(
>     function(image) {
>       return image.dataset.type == 'default' && image.src == imageUrl;
>     });
> assertTrue(index != -1, .....);
> this.$.selector.select(index);

Done.

https://codereview.chromium.org/1615093003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:131:
this.$.selector.select(this.$.selector.indexOf(this.$.oldImage));
On 2016/01/22 02:35:32, dpapad wrote:
> iron-selector#indexOf returns a number, but iron-selector#select is documented
> as expecting a string. Is the documentation not accurate or is there an
implicit
> conversion happening here?

Yes... exactly. The selection key can be a string if you attach it to a custom
attribute.

However, the default is to use the index.

https://codereview.chromium.org/1615093003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:195: return
!oldImageUrl;
On 2016/01/22 02:35:32, dpapad wrote:
> This can only return false if oldImageUrl is the empty string (null/undefined
> are ruled out by the type annotation). So I suggest making the check more
> explicit here to avoid implicit string to bool conversion.
> 
> return oldImageUrl.length == 0;

Done.

https://codereview.chromium.org/1615093003/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/change_picture_browsertest_chromeos.js
(right):

https://codereview.chromium.org/1615093003/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/change_picture_browsertest_chromeos.js:59: *
@param {!function()} action is executed after the listener is set up.
On 2016/01/22 02:35:32, dpapad wrote:
> @return missing

Done.

[dpapad, 2016-01-22 20:12:26]

lgtm

Nits.

https://codereview.chromium.org/1615093003/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/change_picture_browsertest_chromeos.js
(right):

https://codereview.chromium.org/1615093003/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/change_picture_browsertest_chromeos.js:59: *
@param {!function()} action is executed after the listener is set up.
On 2016/01/22 at 19:24:40, tommycli wrote:
> On 2016/01/22 02:35:32, dpapad wrote:
> > @return missing
> 
> Done.

Nit: "function" is also implied non-null (unless you do "?function()"), so no
need for "!".

https://codereview.chromium.org/1615093003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1615093003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/change_picture.js:187: * @param
{string} oldImageUrl
@return missing.

https://codereview.chromium.org/1615093003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/change_picture.js:197: * @private
@return missing.

[tommycli, 2016-01-22 21:37:54]

dpapad: thank you sir

https://codereview.chromium.org/1615093003/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/change_picture_browsertest_chromeos.js
(right):

https://codereview.chromium.org/1615093003/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/change_picture_browsertest_chromeos.js:59: *
@param {!function()} action is executed after the listener is set up.
On 2016/01/22 20:12:25, dpapad wrote:
> On 2016/01/22 at 19:24:40, tommycli wrote:
> > On 2016/01/22 02:35:32, dpapad wrote:
> > > @return missing
> > 
> > Done.
> 
> Nit: "function" is also implied non-null (unless you do "?function()"), so no
> need for "!".

Done.

https://codereview.chromium.org/1615093003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1615093003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/change_picture.js:187: * @param
{string} oldImageUrl
On 2016/01/22 20:12:25, dpapad wrote:
> @return missing.

Done.

https://codereview.chromium.org/1615093003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/change_picture.js:197: * @private
On 2016/01/22 20:12:25, dpapad wrote:
> @return missing.

Done.

[tommycli, 2016-01-22 21:38:07]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-01-22 21:38:08]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/1615093003/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-01-22 21:38:54]

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

[commit-bot: I haz the power, 2016-01-22 23:08:49]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-22 23:10:17]

Description was changed from

==========
Settings People Revamp: Switch Change Picture to use iron-selector.

This reduces the lines of code and makes everything a lot clearer.

BUG=563721
==========

to

==========
Settings People Revamp: Switch Change Picture to use iron-selector.

This reduces the lines of code and makes everything a lot clearer.

BUG=563721

Committed: https://crrev.com/e8a54f9640bda9da78d9ce6f7280669c311609ef
Cr-Commit-Position: refs/heads/master@{#371068}
==========

[commit-bot: I haz the power, 2016-01-22 23:10:18]

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