Refactors profile avatar selector into a Polymer element to use in md-settings & md-user-manager

Refactors profile avatar selector into a Polymer element to use in md-settings & md-user-manager
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/cdf2b2c2820eac70d13e2315bd8159dbf1be6227
Cr-Commit-Position: refs/heads/master@{#401025}

[Moe, 2016-06-14 17:37:21]

Description was changed from

==========
Refactors profile avatar selector into a Polymer element to use in md-settings &
md-user-manager
==========

to

==========
Refactors profile avatar selector into a Polymer element to use in md-settings &
md-user-manager
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Moe, 2016-06-14 17:43:44]

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

[Moe, 2016-06-14 17:43:45]

Hi Tommy, Please review this CL.

[tommycli, 2016-06-14 23:51:00]

This is an excellent patch, see comments below

https://codereview.chromium.org/2068713003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/manage_profile.html (right):

https://codereview.chromium.org/2068713003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/manage_profile.html:27:
selected-avatar-url="[[profileIconUrl]]"
Perhaps the tests are failing because this needs to be {{profileIconUrl}}?

https://codereview.chromium.org/2068713003/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/settings_manage_profile_handler.cc
(right):

https://codereview.chromium.org/2068713003/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/settings_manage_profile_handler.cc:101:
std::unique_ptr<base::ListValue> ManageProfileHandler::GetAvailableIcons() {
I noticed that you're updating this to be very similar to
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/signin/signin_cr...

Can we dedupe the code a bit / share logic? Could we even move the
getAvailableIcons mechanism to be within the selector element? That could be in
a followup patch if necessary.

https://codereview.chromium.org/2068713003/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/settings_manage_profile_handler.cc:116:
gaia_picture_info->SetString("label", std::string());
It seems like maybe it's an empty string?
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/manage_p...

However, Ideally I think it should be a localized version of "Profile Image".

Do we already have such a localized string we could set it as?

https://codereview.chromium.org/2068713003/diff/1/chrome/test/data/webui/cr_e...
File chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js
(right):

https://codereview.chromium.org/2068713003/diff/1/chrome/test/data/webui/cr_e...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:6:
cr.define('cr_profile_avatar_selector', function() {
It's awesome that you're adding a test to something that was previously
untested.

https://codereview.chromium.org/2068713003/diff/1/chrome/test/data/webui/cr_e...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:9: /**
@type {!CrProfileavatarSelector} */
Is this typename capitalized correctly? ProfileAvatar?

https://codereview.chromium.org/2068713003/diff/1/chrome/test/data/webui/cr_e...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:10: var
avatarSelector;
initialized to null normally right?

https://codereview.chromium.org/2068713003/diff/1/chrome/test/data/webui/cr_e...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:38:
test('No avatar is initially selected', function() {
Could we add a test to verify that an avatar IS initially selected if the
element is created with an initially selected icon? This will be the use-case
that Manage Profile uses.

I think you will have to recreate the element within the test body.

something like:

avatarSelector = document.createElement...
avatarSelector.avatars = [ ... ]
avatarSelector.selectedAvatarUrl = ...
document.body.appendChild(avatarSelector);

https://codereview.chromium.org/2068713003/diff/1/chrome/test/data/webui/cr_e...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:54:
test('Fires an event when selected avatar changes', function(done) {
These tests excellent.

https://codereview.chromium.org/2068713003/diff/1/chrome/test/data/webui/cr_e...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:60:
assertEquals(event.detail.value, 'chrome://avatar2.png');
indent is off. (2 spaces)

https://codereview.chromium.org/2068713003/diff/1/ui/webui/resources/cr_eleme...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html
(right):

https://codereview.chromium.org/2068713003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:14:
--avatar-spacing: 24px;
I think these four variables can be eliminated.

https://codereview.chromium.org/2068713003/diff/1/ui/webui/resources/cr_eleme...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js
(right):

https://codereview.chromium.org/2068713003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js:18:
* The currently selected profile avatar Url. May be a data URI.
UURL should be capitalized.

https://codereview.chromium.org/2068713003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js:27:
* The list of available profile avatar Urls and labels.
same here

[Moe, 2016-06-15 19:35:01]

https://codereview.chromium.org/2068713003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/manage_profile.html (right):

https://codereview.chromium.org/2068713003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/manage_profile.html:27:
selected-avatar-url="[[profileIconUrl]]"
On 2016/06/14 23:50:59, tommycli wrote:
> Perhaps the tests are failing because this needs to be {{profileIconUrl}}?

[[profileIconUrl]] should probably change to {{profileIconUrl}} so that we don't
bet on profileIconUrl having been changed externally when onProfileNameChanged_
is called.
Apart from dom changes, the test were mostly failing b/c I was using
'selected-avatar-url-changed' to update the profile avatar instead of
'iron-activate'. the former get called every time profileIconUrl changes.
Ideally we would want it to be called when the user explicitly taps the avatar.

https://codereview.chromium.org/2068713003/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/settings_manage_profile_handler.cc
(right):

https://codereview.chromium.org/2068713003/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/settings_manage_profile_handler.cc:101:
std::unique_ptr<base::ListValue> ManageProfileHandler::GetAvailableIcons() {
On 2016/06/14 23:50:59, tommycli wrote:
> I noticed that you're updating this to be very similar to
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/signin/signin_cr...
> 
> Can we dedupe the code a bit / share logic? Could we even move the
> getAvailableIcons mechanism to be within the selector element? That could be
in
> a followup patch if necessary.

i think we should refactor getting the default avatar icons and labels. and the
maybe you can only add gaia profile picture to the beginning of the list if it's
available. Let's do that in a follow up CL.
As for moving the getAvailableIcons mechanism to the element itself, I don't
know how that can be combined with the browser proxy approach. We can discuss
that though.

https://codereview.chromium.org/2068713003/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/settings_manage_profile_handler.cc:116:
gaia_picture_info->SetString("label", std::string());
On 2016/06/14 23:50:59, tommycli wrote:
> It seems like maybe it's an empty string?
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/manage_p...
> 
> However, Ideally I think it should be a localized version of "Profile Image".
> 
> Do we already have such a localized string we could set it as? 

There's already a localized string for changing the profile picture in cros:
IDS_SETTINGS_CHANGE_PICTURE_PROFILE_PHOTO
I went ahead and reused that here. It says "Google Profile photo". I chatted
with ewald@ and he's on board with that.

https://codereview.chromium.org/2068713003/diff/1/chrome/test/data/webui/cr_e...
File chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js
(right):

https://codereview.chromium.org/2068713003/diff/1/chrome/test/data/webui/cr_e...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:9: /**
@type {!CrProfileavatarSelector} */
On 2016/06/14 23:50:59, tommycli wrote:
> Is this typename capitalized correctly? ProfileAvatar?

Done.

https://codereview.chromium.org/2068713003/diff/1/chrome/test/data/webui/cr_e...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:10: var
avatarSelector;
On 2016/06/14 23:50:59, tommycli wrote:
> initialized to null normally right?

Done.

https://codereview.chromium.org/2068713003/diff/1/chrome/test/data/webui/cr_e...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:38:
test('No avatar is initially selected', function() {
On 2016/06/14 23:50:59, tommycli wrote:
> Could we add a test to verify that an avatar IS initially selected if the
> element is created with an initially selected icon? This will be the use-case
> that Manage Profile uses.
> 
> I think you will have to recreate the element within the test body.
> 
> something like:
> 
> avatarSelector = document.createElement...
> avatarSelector.avatars = [ ... ]
> avatarSelector.selectedAvatarUrl = ...
> document.body.appendChild(avatarSelector);

Done.

https://codereview.chromium.org/2068713003/diff/1/chrome/test/data/webui/cr_e...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:60:
assertEquals(event.detail.value, 'chrome://avatar2.png');
On 2016/06/14 23:50:59, tommycli wrote:
> indent is off. (2 spaces)

Done.

https://codereview.chromium.org/2068713003/diff/1/ui/webui/resources/cr_eleme...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html
(right):

https://codereview.chromium.org/2068713003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:14:
--avatar-spacing: 24px;
On 2016/06/14 23:50:59, tommycli wrote:
> I think these four variables can be eliminated.

I chatted bettes@ and he mentioned the spacing for the avatars in the settings
might be a bit different from that of the user manager. Let's keep that one.
I'll eliminate the rest in favor of the --avatar-selector-avatar mixin.

https://codereview.chromium.org/2068713003/diff/1/ui/webui/resources/cr_eleme...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js
(right):

https://codereview.chromium.org/2068713003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js:18:
* The currently selected profile avatar Url. May be a data URI.
On 2016/06/14 23:51:00, tommycli wrote:
> UURL should be capitalized.

Done. I vaguely remember dbeam@ was against capitalizing 'URL' though.

https://codereview.chromium.org/2068713003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js:27:
* The list of available profile avatar Urls and labels.
On 2016/06/14 23:51:00, tommycli wrote:
> same here

Done.

[tommycli, 2016-06-15 19:59:47]

https://codereview.chromium.org/2068713003/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/settings_manage_profile_handler.cc
(right):

https://codereview.chromium.org/2068713003/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/settings_manage_profile_handler.cc:101:
std::unique_ptr<base::ListValue> ManageProfileHandler::GetAvailableIcons() {
On 2016/06/15 19:35:00, Moe wrote:
> On 2016/06/14 23:50:59, tommycli wrote:
> > I noticed that you're updating this to be very similar to
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/signin/signin_cr...
> > 
> > Can we dedupe the code a bit / share logic? Could we even move the
> > getAvailableIcons mechanism to be within the selector element? That could be
> in
> > a followup patch if necessary.
> 
> i think we should refactor getting the default avatar icons and labels. and
the
> maybe you can only add gaia profile picture to the beginning of the list if
it's
> available. Let's do that in a follow up CL.
> As for moving the getAvailableIcons mechanism to the element itself, I don't
> know how that can be combined with the browser proxy approach. We can discuss
> that though. 

Sounds good I'm okay with doing that refactor in a followup.

https://codereview.chromium.org/2068713003/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/settings_manage_profile_handler.cc:116:
gaia_picture_info->SetString("label", std::string());
On 2016/06/15 19:35:00, Moe wrote:
> On 2016/06/14 23:50:59, tommycli wrote:
> > It seems like maybe it's an empty string?
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/manage_p...
> > 
> > However, Ideally I think it should be a localized version of "Profile
Image".
> > 
> > Do we already have such a localized string we could set it as? 
> 
> There's already a localized string for changing the profile picture in cros:
> IDS_SETTINGS_CHANGE_PICTURE_PROFILE_PHOTO
> I went ahead and reused that here. It says "Google Profile photo". I chatted
> with ewald@ and he's on board with that.

Excellent. Thanks for looking into it.

https://codereview.chromium.org/2068713003/diff/1/ui/webui/resources/cr_eleme...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js
(right):

https://codereview.chromium.org/2068713003/diff/1/ui/webui/resources/cr_eleme...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js:18:
* The currently selected profile avatar Url. May be a data URI.
On 2016/06/15 19:35:01, Moe wrote:
> On 2016/06/14 23:51:00, tommycli wrote:
> > UURL should be capitalized.
> 
> Done. I vaguely remember dbeam@ was against capitalizing 'URL' though.

I think only in variable names. I think he said it should be selectedUrl instead
of selectedURL. I can't imagine he would want them uncapitalized in a comment
though.

https://codereview.chromium.org/2068713003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js
(right):

https://codereview.chromium.org/2068713003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:44:
avatarSelector.pop('avatars');
I've never seen such a usage of a pop method on a Polymer element before.

Does avatarSelector.avatars.pop() work?

https://codereview.chromium.org/2068713003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:85:
test('Fires iron-active event when an avatar is activated',
"Fires an iron-activate event"?

https://codereview.chromium.org/2068713003/diff/20001/ui/webui/resources/cr_e...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js
(right):

https://codereview.chromium.org/2068713003/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js:39:
items: {
I don't think it's worth keeping this property in here just for testing.

Instead, could you do a DOM selector for '.avatar' elements, and then use the
resulting array in your tests?

[tommycli, 2016-06-15 20:03:02]

https://codereview.chromium.org/2068713003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js
(right):

https://codereview.chromium.org/2068713003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:44:
avatarSelector.pop('avatars');
On 2016/06/15 19:59:47, tommycli wrote:
> I've never seen such a usage of a pop method on a Polymer element before.
> 
> Does avatarSelector.avatars.pop() work?

I understand now. You need to use element.pop to trigger the Polymer observer
updates too I assume?

[Moe, 2016-06-15 20:29:17]

https://codereview.chromium.org/2068713003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js
(right):

https://codereview.chromium.org/2068713003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:44:
avatarSelector.pop('avatars');
On 2016/06/15 20:03:02, tommycli wrote:
> On 2016/06/15 19:59:47, tommycli wrote:
> > I've never seen such a usage of a pop method on a Polymer element before.
> > 
> > Does avatarSelector.avatars.pop() work?
> 
> I understand now. You need to use element.pop to trigger the Polymer observer
> updates too I assume?

Exactly. We need array mutations if there are observers, computed properties, or
data bindings for an array:
https://www.polymer-project.org/1.0/docs/devguide/properties#array-mutation

https://codereview.chromium.org/2068713003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:85:
test('Fires iron-active event when an avatar is activated',
On 2016/06/15 19:59:47, tommycli wrote:
> "Fires an iron-activate event"?

Done.

https://codereview.chromium.org/2068713003/diff/20001/ui/webui/resources/cr_e...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js
(right):

https://codereview.chromium.org/2068713003/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js:39:
items: {
On 2016/06/15 19:59:47, tommycli wrote:
> I don't think it's worth keeping this property in here just for testing.
> 
> Instead, could you do a DOM selector for '.avatar' elements, and then use the
> resulting array in your tests?

I was initially using a DOM selector for '.avatar' elements in my tests. but
then I saw your tests use the items property of iron-selector. I think it's a
good idea to have an API to access the underlying avatar elements, even if it is
only used in tests right now. Its immediate benefit is that the tests won't
break if we change class names etc in the elements. and the work is already done
;)

[tommycli, 2016-06-15 20:44:51]

https://codereview.chromium.org/2068713003/diff/20001/ui/webui/resources/cr_e...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js
(right):

https://codereview.chromium.org/2068713003/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js:39:
items: {
On 2016/06/15 20:29:16, Moe wrote:
> On 2016/06/15 19:59:47, tommycli wrote:
> > I don't think it's worth keeping this property in here just for testing.
> > 
> > Instead, could you do a DOM selector for '.avatar' elements, and then use
the
> > resulting array in your tests?
> 
> I was initially using a DOM selector for '.avatar' elements in my tests. but
> then I saw your tests use the items property of iron-selector. I think it's a
> good idea to have an API to access the underlying avatar elements, even if it
is
> only used in tests right now. Its immediate benefit is that the tests won't
> break if we change class names etc in the elements. and the work is already
done
> ;)

Another thing that could work in the tests would be to do:

avatarSelector.$.selector.items[0] etc.

Then we would not need to expose a separate items property and do the extra
book-keeping.

[Moe, 2016-06-15 21:10:40]

https://codereview.chromium.org/2068713003/diff/20001/ui/webui/resources/cr_e...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js
(right):

https://codereview.chromium.org/2068713003/diff/20001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.js:39:
items: {
On 2016/06/15 20:44:51, tommycli wrote:
> On 2016/06/15 20:29:16, Moe wrote:
> > On 2016/06/15 19:59:47, tommycli wrote:
> > > I don't think it's worth keeping this property in here just for testing.
> > > 
> > > Instead, could you do a DOM selector for '.avatar' elements, and then use
> the
> > > resulting array in your tests?
> > 
> > I was initially using a DOM selector for '.avatar' elements in my tests. but
> > then I saw your tests use the items property of iron-selector. I think it's
a
> > good idea to have an API to access the underlying avatar elements, even if
it
> is
> > only used in tests right now. Its immediate benefit is that the tests won't
> > break if we change class names etc in the elements. and the work is already
> done
> > ;)
> 
> Another thing that could work in the tests would be to do:
> 
> avatarSelector.$.selector.items[0] etc.
> 
> Then we would not need to expose a separate items property and do the extra
> book-keeping.

Fair enough. Done.

[tommycli, 2016-06-15 21:17:38]

lgtm sans below nit. Thanks!

https://codereview.chromium.org/2068713003/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js
(right):

https://codereview.chromium.org/2068713003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:95:
MockInteractions.tap(selector.items[1]);
nit: No need to create selector variable in this case.

[Moe, 2016-06-16 12:55:39]

mahmadi@chromium.org changed reviewers:
+ michaelpg@chromium.org

[Moe, 2016-06-16 12:55:40]

Thanks Tommy. Michael, could you please review this CL?

https://codereview.chromium.org/2068713003/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js
(right):

https://codereview.chromium.org/2068713003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:95:
MockInteractions.tap(selector.items[1]);
On 2016/06/15 21:17:38, tommycli wrote:
> nit: No need to create selector variable in this case.

Done.

[michaelpg, 2016-06-20 17:01:11]

https://codereview.chromium.org/2068713003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/manage_profile.html (right):

https://codereview.chromium.org/2068713003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/manage_profile.html:16:
--avatar-selector-avatar-hovered: {
nit: put variable/mixin definitions at top of block

https://codereview.chromium.org/2068713003/diff/80001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/settings/settings_manage_profile_handler_unittest.cc
(right):

https://codereview.chromium.org/2068713003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_manage_profile_handler_unittest.cc:52:
// Expect the list of icon URLs to be a non-empty list of non-empty strings.
update/remove comment

https://codereview.chromium.org/2068713003/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_elements_browsertest.js (right):

https://codereview.chromium.org/2068713003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_elements_browsertest.js:26:
'cr_profile_avatar_selector_tests.js',
please add a TODO(michaelpg) to split these up rather than loading resources
monolithically; see https://codereview.chromium.org/2050053002/

https://codereview.chromium.org/2068713003/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js
(right):

https://codereview.chromium.org/2068713003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:9: /**
@type {!CrProfileAvatarSelectorElement} */
nit: remove !

https://codereview.chromium.org/2068713003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:12: /**
@type {!CrProfileAvatarSelectorElement} */
@type => @return

https://codereview.chromium.org/2068713003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:60: var
avatarSelector = createElement();
remove line (avatarSelector already created in setup, right?)

https://codereview.chromium.org/2068713003/diff/80001/ui/webui/resources/cr_e...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html
(right):

https://codereview.chromium.org/2068713003/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:2:
<link rel="import"
href="chrome://resources/polymer/v1_0/iron-flex-layout/iron-flex-layout.html">
remove, just use native CSS

https://codereview.chromium.org/2068713003/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:10:
@apply(--layout-inline);
display: inline-flex;

https://codereview.chromium.org/2068713003/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:12:
@apply(--avatar-selector);
move to top

https://codereview.chromium.org/2068713003/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:28:
@apply(--layout);
display: flex;

https://codereview.chromium.org/2068713003/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:29:
@apply(--layout-center-center);
align-items: center;
justify-content: center;

https://codereview.chromium.org/2068713003/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:37:
@apply(--avatar-selector-avatar);
move to 28

https://codereview.chromium.org/2068713003/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:46:
@apply(--avatar-selector-avatar-selected);
move to top

[Moe, 2016-06-20 18:48:56]

https://codereview.chromium.org/2068713003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/manage_profile.html (right):

https://codereview.chromium.org/2068713003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/manage_profile.html:16:
--avatar-selector-avatar-hovered: {
On 2016/06/20 17:01:10, michaelpg wrote:
> nit: put variable/mixin definitions at top of block

Done.

https://codereview.chromium.org/2068713003/diff/80001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/settings/settings_manage_profile_handler_unittest.cc
(right):

https://codereview.chromium.org/2068713003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_manage_profile_handler_unittest.cc:52:
// Expect the list of icon URLs to be a non-empty list of non-empty strings.
On 2016/06/20 17:01:10, michaelpg wrote:
> update/remove comment

Done.

https://codereview.chromium.org/2068713003/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_elements_browsertest.js (right):

https://codereview.chromium.org/2068713003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_elements_browsertest.js:26:
'cr_profile_avatar_selector_tests.js',
On 2016/06/20 17:01:10, michaelpg wrote:
> please add a TODO(michaelpg) to split these up rather than loading resources
> monolithically; see https://codereview.chromium.org/2050053002/

i'll just do it for you ;)

https://codereview.chromium.org/2068713003/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js
(right):

https://codereview.chromium.org/2068713003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:9: /**
@type {!CrProfileAvatarSelectorElement} */
On 2016/06/20 17:01:10, michaelpg wrote:
> nit: remove !

Done.

https://codereview.chromium.org/2068713003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:12: /**
@type {!CrProfileAvatarSelectorElement} */
On 2016/06/20 17:01:10, michaelpg wrote:
> @type => @return

Done.

https://codereview.chromium.org/2068713003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_profile_avatar_selector_tests.js:60: var
avatarSelector = createElement();
On 2016/06/20 17:01:10, michaelpg wrote:
> remove line (avatarSelector already created in setup, right?)

Creating a second avatarSelector is intended in this test. We would wanna know
whether an avatar is initially selected if selectedAvatarUrl is set before the
element is attached. I changed the variable name so there won't be a confusion.

https://codereview.chromium.org/2068713003/diff/80001/ui/webui/resources/cr_e...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html
(right):

https://codereview.chromium.org/2068713003/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:2:
<link rel="import"
href="chrome://resources/polymer/v1_0/iron-flex-layout/iron-flex-layout.html">
On 2016/06/20 17:01:10, michaelpg wrote:
> remove, just use native CSS

Done.

https://codereview.chromium.org/2068713003/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:10:
@apply(--layout-inline);
On 2016/06/20 17:01:10, michaelpg wrote:
> display: inline-flex;

Done.

https://codereview.chromium.org/2068713003/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:12:
@apply(--avatar-selector);
On 2016/06/20 17:01:10, michaelpg wrote:
> move to top

I'm applying a mixin here not defining it. Shouldn't it be the last statement in
the block in order to override previously set properties? Polymer source seems
to apply mixins this way.

https://codereview.chromium.org/2068713003/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:28:
@apply(--layout);
On 2016/06/20 17:01:10, michaelpg wrote:
> display: flex;

Done.

https://codereview.chromium.org/2068713003/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:29:
@apply(--layout-center-center);
On 2016/06/20 17:01:10, michaelpg wrote:
> align-items: center;
> justify-content: center;

Done.

https://codereview.chromium.org/2068713003/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:37:
@apply(--avatar-selector-avatar);
On 2016/06/20 17:01:10, michaelpg wrote:
> move to 28

ditto

https://codereview.chromium.org/2068713003/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:46:
@apply(--avatar-selector-avatar-selected);
On 2016/06/20 17:01:10, michaelpg wrote:
> move to top

ditto.

[michaelpg, 2016-06-21 00:48:09]

lgtm

https://codereview.chromium.org/2068713003/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_elements_browsertest.js (right):

https://codereview.chromium.org/2068713003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_elements_browsertest.js:26:
'cr_profile_avatar_selector_tests.js',
On 2016/06/20 18:48:56, Moe wrote:
> On 2016/06/20 17:01:10, michaelpg wrote:
> > please add a TODO(michaelpg) to split these up rather than loading resources
> > monolithically; see https://codereview.chromium.org/2050053002/
> 
> i'll just do it for you ;)

You're awesome!

https://codereview.chromium.org/2068713003/diff/80001/ui/webui/resources/cr_e...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html
(right):

https://codereview.chromium.org/2068713003/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:12:
@apply(--avatar-selector);
On 2016/06/20 18:48:56, Moe wrote:
> On 2016/06/20 17:01:10, michaelpg wrote:
> > move to top
> 
> I'm applying a mixin here not defining it. Shouldn't it be the last statement
in
> the block in order to override previously set properties? Polymer source seems
> to apply mixins this way.

Acknowledged.

[Dan Beam, 2016-06-21 00:56:00]

https://codereview.chromium.org/2068713003/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/cr_elements/cr_elements_browsertest.js (right):

https://codereview.chromium.org/2068713003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/cr_elements/cr_elements_browsertest.js:26:
'cr_profile_avatar_selector_tests.js',
On 2016/06/21 00:48:09, michaelpg wrote:
> On 2016/06/20 18:48:56, Moe wrote:
> > On 2016/06/20 17:01:10, michaelpg wrote:
> > > please add a TODO(michaelpg) to split these up rather than loading
resources
> > > monolithically; see https://codereview.chromium.org/2050053002/
> > 
> > i'll just do it for you ;)
> 
> You're awesome!

+1

[Moe, 2016-06-21 14:02:48]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2016-06-21 14:02:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/2068713003/#ps100001
(title: "Addressed comments")

[commit-bot: I haz the power, 2016-06-21 14:03:02]

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

[commit-bot: I haz the power, 2016-06-21 14:04:59]

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

[commit-bot: I haz the power, 2016-06-21 14:05:01]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[Moe, 2016-06-21 17:23:31]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2016-06-21 17:23:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2068713003/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-21 17:24:11]

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

[commit-bot: I haz the power, 2016-06-21 17:33:29]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-21 17:49:39]

Description was changed from

==========
Refactors profile avatar selector into a Polymer element to use in md-settings &
md-user-manager
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Refactors profile avatar selector into a Polymer element to use in md-settings &
md-user-manager
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2016-06-21 17:49:40]

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

[Moe, 2016-06-21 17:56:17]

On 2016/06/21 17:49:40, commit-bot: I haz the power wrote:
> Patchset 7 (id:??) landed as
> https://crrev.com/cdf2b2c2820eac70d13e2315bd8159dbf1be6227
> Cr-Commit-Position: refs/heads/master@{#401025}

Thank you!