MD Settings: Stop using IronMenuBehavior from cr-profile-avatar-selector-grid.

MD Settings: Stop using IronMenuBehavior from cr-profile-avatar-selector-grid.

This both simplifies the code and fixes the selection bug. The original
code relied on multiple inherited Polymer behaviors which stomped on each other
causing what seems like a race condition per findings at
https://bugs.chromium.org/p/chromium/issues/detail?id=734789#c2.

Specifically: 
 - Simplify logic within cr-profile-avatar-selector-grid, no more inherited
   behaviors.
 - Update both settings/ and md_user_manager/ client code.
 - Stop relying on bubbling 'iron-activate' event in manage_profile.html.
   Instead use a Polymer binding + observer.

Regarding the race condition: The previous code, was seemingly attempting to
highlight the grid with the currently selected avatar icon on creation, but
unsuccessfully because this was intentionally broken within the C++ side at
crbug.com/710660. As a side-effect, the 'profile-info-changed' event
propagating back from C++ after every user-initiated change, was causing the
selection of the user to be immediately discarded.

It would make little sense to re-implement JS functionality that is already not
working. Instead I am removing such tests, since they are meaningless until/if
the C++ side is fixed.

BUG=734789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2950943002
Cr-Commit-Position: refs/heads/master@{#481776}
Committed: https://chromium.googlesource.com/chromium/src/+/e67dbad97952415cabff9444ed85f543579240e6

[dpapad, 2017-06-20 20:41:44]

Description was changed from

==========
cr-profile-avatar drop IronMenuBehavior, WIP.

BUG=
==========

to

==========
cr-profile-avatar drop IronMenuBehavior, WIP.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-06-21 00:46:29]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 00:46:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-06-21 00:50:36]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[dpapad, 2017-06-21 00:50:46]

Patchset #5 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-06-21 00:50:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-06-21 01:16:03]

Description was changed from

==========
cr-profile-avatar drop IronMenuBehavior, WIP.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Stop using IronMenuBehavior from cr-profile-avatar-selector-grid.

 - Simplify logic within cr-profile-avatar-selector, no more inherited
behaviors.
 - Update both settings/ and md_user_manager/ client code.
 - Stop relying on 
This fixes both simplifies the code and fixes the selection bug.

BUG=734789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-06-21 01:20:47]

Description was changed from

==========
MD Settings: Stop using IronMenuBehavior from cr-profile-avatar-selector-grid.

 - Simplify logic within cr-profile-avatar-selector, no more inherited
behaviors.
 - Update both settings/ and md_user_manager/ client code.
 - Stop relying on 
This fixes both simplifies the code and fixes the selection bug.

BUG=734789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Stop using IronMenuBehavior from cr-profile-avatar-selector-grid.

 - Simplify logic within cr-profile-avatar-selector, no more inherited
behaviors.
 - Update both settings/ and md_user_manager/ client code.
 - Stop relying on bubbling 'on-iron-activate' event in manage_profile.html.
   Instead use a Polymer binding + observer.

Also the previous code, was seemingly attempting to highlight the grid with
the currently selected avatar icon on creation, but unsuccessfully. If this is
desired it should be fixed in a separate CL.

This both simplifies the code and fixes the selection bug. The original
code relied on multiple inherited Polymer behaviors which stomped on each other
causing what seems like a race condition per findings at
https://bugs.chromium.org/p/chromium/issues/detail?id=734789#c2.

BUG=734789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-06-21 01:53:44]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 01:54:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 03:10:43]

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

[commit-bot: I haz the power, 2017-06-21 03:10:44]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dpapad, 2017-06-21 18:12:32]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 18:12:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-06-21 18:29:50]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 18:30:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-06-21 18:33:20]

Description was changed from

==========
MD Settings: Stop using IronMenuBehavior from cr-profile-avatar-selector-grid.

 - Simplify logic within cr-profile-avatar-selector, no more inherited
behaviors.
 - Update both settings/ and md_user_manager/ client code.
 - Stop relying on bubbling 'on-iron-activate' event in manage_profile.html.
   Instead use a Polymer binding + observer.

Also the previous code, was seemingly attempting to highlight the grid with
the currently selected avatar icon on creation, but unsuccessfully. If this is
desired it should be fixed in a separate CL.

This both simplifies the code and fixes the selection bug. The original
code relied on multiple inherited Polymer behaviors which stomped on each other
causing what seems like a race condition per findings at
https://bugs.chromium.org/p/chromium/issues/detail?id=734789#c2.

BUG=734789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Stop using IronMenuBehavior from cr-profile-avatar-selector-grid.

This both simplifies the code and fixes the selection bug. The original
code relied on multiple inherited Polymer behaviors which stomped on each other
causing what seems like a race condition per findings at
https://bugs.chromium.org/p/chromium/issues/detail?id=734789#c2.

Specifically: 
 - Simplify logic within cr-profile-avatar-selector, no more inherited
behaviors.
 - Update both settings/ and md_user_manager/ client code.
 - Stop relying on bubbling 'on-iron-activate' event in manage_profile.html.
   Instead use a Polymer binding + observer.

Also the previous code, was seemingly attempting to highlight the grid with
the currently selected avatar icon on creation, but unsuccessfully because this
was intentionally broken at crbug.com/710660. It would make little sense to
re-implement JS functionality that is already not working because of the C++
changes.
Instead I am removing such tests, since they are meaningless until the C++ side
is
fixed.

BUG=734789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-06-21 19:22:52]

Description was changed from

==========
MD Settings: Stop using IronMenuBehavior from cr-profile-avatar-selector-grid.

This both simplifies the code and fixes the selection bug. The original
code relied on multiple inherited Polymer behaviors which stomped on each other
causing what seems like a race condition per findings at
https://bugs.chromium.org/p/chromium/issues/detail?id=734789#c2.

Specifically: 
 - Simplify logic within cr-profile-avatar-selector, no more inherited
behaviors.
 - Update both settings/ and md_user_manager/ client code.
 - Stop relying on bubbling 'on-iron-activate' event in manage_profile.html.
   Instead use a Polymer binding + observer.

Also the previous code, was seemingly attempting to highlight the grid with
the currently selected avatar icon on creation, but unsuccessfully because this
was intentionally broken at crbug.com/710660. It would make little sense to
re-implement JS functionality that is already not working because of the C++
changes.
Instead I am removing such tests, since they are meaningless until the C++ side
is
fixed.

BUG=734789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Stop using IronMenuBehavior from cr-profile-avatar-selector-grid.

This both simplifies the code and fixes the selection bug. The original
code relied on multiple inherited Polymer behaviors which stomped on each other
causing what seems like a race condition per findings at
https://bugs.chromium.org/p/chromium/issues/detail?id=734789#c2.

Specifically: 
 - Simplify logic within cr-profile-avatar-selector, no more inherited
behaviors.
 - Update both settings/ and md_user_manager/ client code.
 - Stop relying on bubbling 'on-iron-activate' event in manage_profile.html.
   Instead use a Polymer binding + observer.

Also the previous code, was seemingly attempting to highlight the grid with
the currently selected avatar icon on creation, but unsuccessfully because this
was intentionally broken within the C++ side at crbug.com/710660. It would make
little sense to re-implement JS functionality that is already not working.
Instead I am removing such tests, since they are meaningless until/if the C++
side is fixed.

BUG=734789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-06-21 19:54:00]

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

[dpapad, 2017-06-21 19:54:01]

Greet tests in patch#8.

@tommycli: Sending to you since you have familiarity with the people_page/
related stuff.

cc'ing dbeam@ mostly as FIY, because of familiarity with crbug.com/710660 (see
CL description on how it relates to this CL).

The core of this CL is the change inside cr_profile_avatar_selector_grid.js.
Thanks

[dpapad, 2017-06-21 20:40:32]

Description was changed from

==========
MD Settings: Stop using IronMenuBehavior from cr-profile-avatar-selector-grid.

This both simplifies the code and fixes the selection bug. The original
code relied on multiple inherited Polymer behaviors which stomped on each other
causing what seems like a race condition per findings at
https://bugs.chromium.org/p/chromium/issues/detail?id=734789#c2.

Specifically: 
 - Simplify logic within cr-profile-avatar-selector, no more inherited
behaviors.
 - Update both settings/ and md_user_manager/ client code.
 - Stop relying on bubbling 'on-iron-activate' event in manage_profile.html.
   Instead use a Polymer binding + observer.

Also the previous code, was seemingly attempting to highlight the grid with
the currently selected avatar icon on creation, but unsuccessfully because this
was intentionally broken within the C++ side at crbug.com/710660. It would make
little sense to re-implement JS functionality that is already not working.
Instead I am removing such tests, since they are meaningless until/if the C++
side is fixed.

BUG=734789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Stop using IronMenuBehavior from cr-profile-avatar-selector-grid.

This both simplifies the code and fixes the selection bug. The original
code relied on multiple inherited Polymer behaviors which stomped on each other
causing what seems like a race condition per findings at
https://bugs.chromium.org/p/chromium/issues/detail?id=734789#c2.

Specifically: 
 - Simplify logic within cr-profile-avatar-selectorsgrid, no more inherited
   behaviors.
 - Update both settings/ and md_user_manager/ client code.
 - Stop relying on bubbling 'on-iron-activate' event in manage_profile.html.
   Instead use a Polymer binding + observer.

Also the previous code, was seemingly attempting to highlight the grid with
the currently selected avatar icon on creation, but unsuccessfully because this
was intentionally broken within the C++ side at crbug.com/710660. It would make
little sense to re-implement JS functionality that is already not working.
Instead I am removing such tests, since they are meaningless until/if the C++
side is fixed.

BUG=734789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-06-22 19:02:41]

Description was changed from

==========
MD Settings: Stop using IronMenuBehavior from cr-profile-avatar-selector-grid.

This both simplifies the code and fixes the selection bug. The original
code relied on multiple inherited Polymer behaviors which stomped on each other
causing what seems like a race condition per findings at
https://bugs.chromium.org/p/chromium/issues/detail?id=734789#c2.

Specifically: 
 - Simplify logic within cr-profile-avatar-selectorsgrid, no more inherited
   behaviors.
 - Update both settings/ and md_user_manager/ client code.
 - Stop relying on bubbling 'on-iron-activate' event in manage_profile.html.
   Instead use a Polymer binding + observer.

Also the previous code, was seemingly attempting to highlight the grid with
the currently selected avatar icon on creation, but unsuccessfully because this
was intentionally broken within the C++ side at crbug.com/710660. It would make
little sense to re-implement JS functionality that is already not working.
Instead I am removing such tests, since they are meaningless until/if the C++
side is fixed.

BUG=734789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Stop using IronMenuBehavior from cr-profile-avatar-selector-grid.

This both simplifies the code and fixes the selection bug. The original
code relied on multiple inherited Polymer behaviors which stomped on each other
causing what seems like a race condition per findings at
https://bugs.chromium.org/p/chromium/issues/detail?id=734789#c2.

Specifically: 
 - Simplify logic within cr-profile-avatar-selector-grid, no more inherited
   behaviors.
 - Update both settings/ and md_user_manager/ client code.
 - Stop relying on bubbling 'on-iron-activate' event in manage_profile.html.
   Instead use a Polymer binding + observer.

Also the previous code, was seemingly attempting to highlight the grid with
the currently selected avatar icon on creation, but unsuccessfully because this
was intentionally broken within the C++ side at crbug.com/710660. It would make
little sense to re-implement JS functionality that is already not working.
Instead I am removing such tests, since they are meaningless until/if the C++
side is fixed.

BUG=734789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-06-22 19:11:18]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 19:11:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-06-22 19:36:58]

Friendly ping.

[commit-bot: I haz the power, 2017-06-22 20:43:47]

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

[commit-bot: I haz the power, 2017-06-22 20:43:48]

Dry run: This issue passed the CQ dry run.

[dpapad, 2017-06-22 21:06:39]

Description was changed from

==========
MD Settings: Stop using IronMenuBehavior from cr-profile-avatar-selector-grid.

This both simplifies the code and fixes the selection bug. The original
code relied on multiple inherited Polymer behaviors which stomped on each other
causing what seems like a race condition per findings at
https://bugs.chromium.org/p/chromium/issues/detail?id=734789#c2.

Specifically: 
 - Simplify logic within cr-profile-avatar-selector-grid, no more inherited
   behaviors.
 - Update both settings/ and md_user_manager/ client code.
 - Stop relying on bubbling 'on-iron-activate' event in manage_profile.html.
   Instead use a Polymer binding + observer.

Also the previous code, was seemingly attempting to highlight the grid with
the currently selected avatar icon on creation, but unsuccessfully because this
was intentionally broken within the C++ side at crbug.com/710660. It would make
little sense to re-implement JS functionality that is already not working.
Instead I am removing such tests, since they are meaningless until/if the C++
side is fixed.

BUG=734789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Stop using IronMenuBehavior from cr-profile-avatar-selector-grid.

This both simplifies the code and fixes the selection bug. The original
code relied on multiple inherited Polymer behaviors which stomped on each other
causing what seems like a race condition per findings at
https://bugs.chromium.org/p/chromium/issues/detail?id=734789#c2.

Specifically: 
 - Simplify logic within cr-profile-avatar-selector-grid, no more inherited
   behaviors.
 - Update both settings/ and md_user_manager/ client code.
 - Stop relying on bubbling 'iron-activate' event in manage_profile.html.
   Instead use a Polymer binding + observer.

Also the previous code, was seemingly attempting to highlight the grid with
the currently selected avatar icon on creation, but unsuccessfully because this
was intentionally broken within the C++ side at crbug.com/710660. It would make
little sense to re-implement JS functionality that is already not working.
Instead I am removing such tests, since they are meaningless until/if the C++
side is fixed.

BUG=734789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-06-22 21:38:56]

Description was changed from

==========
MD Settings: Stop using IronMenuBehavior from cr-profile-avatar-selector-grid.

This both simplifies the code and fixes the selection bug. The original
code relied on multiple inherited Polymer behaviors which stomped on each other
causing what seems like a race condition per findings at
https://bugs.chromium.org/p/chromium/issues/detail?id=734789#c2.

Specifically: 
 - Simplify logic within cr-profile-avatar-selector-grid, no more inherited
   behaviors.
 - Update both settings/ and md_user_manager/ client code.
 - Stop relying on bubbling 'iron-activate' event in manage_profile.html.
   Instead use a Polymer binding + observer.

Also the previous code, was seemingly attempting to highlight the grid with
the currently selected avatar icon on creation, but unsuccessfully because this
was intentionally broken within the C++ side at crbug.com/710660. It would make
little sense to re-implement JS functionality that is already not working.
Instead I am removing such tests, since they are meaningless until/if the C++
side is fixed.

BUG=734789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Stop using IronMenuBehavior from cr-profile-avatar-selector-grid.

This both simplifies the code and fixes the selection bug. The original
code relied on multiple inherited Polymer behaviors which stomped on each other
causing what seems like a race condition per findings at
https://bugs.chromium.org/p/chromium/issues/detail?id=734789#c2.

Specifically: 
 - Simplify logic within cr-profile-avatar-selector-grid, no more inherited
   behaviors.
 - Update both settings/ and md_user_manager/ client code.
 - Stop relying on bubbling 'iron-activate' event in manage_profile.html.
   Instead use a Polymer binding + observer.

Regarding the race condition: The previous code, was seemingly attempting to
highlight the grid with the currently selected avatar icon on creation, but
unsuccessfully because this was intentionally broken within the C++ side at
crbug.com/710660. As a side-effect, the 'profile-info-changed' event propagating
back from C++ after every user-initiated change, was causing the selection of
the
user to be immediately discarded.

It would make little sense to re-implement JS functionality that is already not
working.
Instead I am removing such tests, since they are meaningless until/if the C++
side is fixed.

BUG=734789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-06-22 21:39:25]

Description was changed from

==========
MD Settings: Stop using IronMenuBehavior from cr-profile-avatar-selector-grid.

This both simplifies the code and fixes the selection bug. The original
code relied on multiple inherited Polymer behaviors which stomped on each other
causing what seems like a race condition per findings at
https://bugs.chromium.org/p/chromium/issues/detail?id=734789#c2.

Specifically: 
 - Simplify logic within cr-profile-avatar-selector-grid, no more inherited
   behaviors.
 - Update both settings/ and md_user_manager/ client code.
 - Stop relying on bubbling 'iron-activate' event in manage_profile.html.
   Instead use a Polymer binding + observer.

Regarding the race condition: The previous code, was seemingly attempting to
highlight the grid with the currently selected avatar icon on creation, but
unsuccessfully because this was intentionally broken within the C++ side at
crbug.com/710660. As a side-effect, the 'profile-info-changed' event propagating
back from C++ after every user-initiated change, was causing the selection of
the
user to be immediately discarded.

It would make little sense to re-implement JS functionality that is already not
working.
Instead I am removing such tests, since they are meaningless until/if the C++
side is fixed.

BUG=734789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Stop using IronMenuBehavior from cr-profile-avatar-selector-grid.

This both simplifies the code and fixes the selection bug. The original
code relied on multiple inherited Polymer behaviors which stomped on each other
causing what seems like a race condition per findings at
https://bugs.chromium.org/p/chromium/issues/detail?id=734789#c2.

Specifically: 
 - Simplify logic within cr-profile-avatar-selector-grid, no more inherited
   behaviors.
 - Update both settings/ and md_user_manager/ client code.
 - Stop relying on bubbling 'iron-activate' event in manage_profile.html.
   Instead use a Polymer binding + observer.

Regarding the race condition: The previous code, was seemingly attempting to
highlight the grid with the currently selected avatar icon on creation, but
unsuccessfully because this was intentionally broken within the C++ side at
crbug.com/710660. As a side-effect, the 'profile-info-changed' event
propagating back from C++ after every user-initiated change, was causing the
selection of the user to be immediately discarded.

It would make little sense to re-implement JS functionality that is already not
working. Instead I am removing such tests, since they are meaningless until/if
the C++ side is fixed.

BUG=734789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tommycli, 2017-06-22 21:45:33]

some questions

https://codereview.chromium.org/2950943002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/manage_profile.js (right):

https://codereview.chromium.org/2950943002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/manage_profile.js:21:
profileAvatar: {
Since this property is no longer changed externally (except from a child), we
should make this profileAvatar_ so a parent never tries to bind to it.

https://codereview.chromium.org/2950943002/diff/220001/ui/webui/resources/cr_...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html
(right):

https://codereview.chromium.org/2950943002/diff/220001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:1:
<link rel="import" href="chrome://resources/html/polymer.html">
Is this change intentional?

https://codereview.chromium.org/2950943002/diff/220001/ui/webui/resources/cr_...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.html
(right):

https://codereview.chromium.org/2950943002/diff/220001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.html:15:
<content select=".avatar"></content>
Why do you need to select only .avatar class children?

https://codereview.chromium.org/2950943002/diff/220001/ui/webui/resources/cr_...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js
(right):

https://codereview.chromium.org/2950943002/diff/220001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:77:
break;
Hmm... how can nextItem be null or undefined in this case, isn't it an element
of an array returned by querySelectorAll?

[dpapad, 2017-06-22 23:22:49]

https://codereview.chromium.org/2950943002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/manage_profile.js (right):

https://codereview.chromium.org/2950943002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/manage_profile.js:21:
profileAvatar: {
On 2017/06/22 at 21:45:31, tommycli wrote:
> Since this property is no longer changed externally (except from a child), we
should make this profileAvatar_ so a parent never tries to bind to it.

Done.

https://codereview.chromium.org/2950943002/diff/220001/ui/webui/resources/cr_...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html
(right):

https://codereview.chromium.org/2950943002/diff/220001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector.html:1:
<link rel="import" href="chrome://resources/html/polymer.html">
On 2017/06/22 at 21:45:31, tommycli wrote:
> Is this change intentional?

Yes. This was discussed at
https://groups.google.com/a/google.com/d/msg/chrome-settings/1v1_5twj4X4/4Ca1...,
where we decided to always have polymer.html as the 1st import followed by a
blank line. Bulk change happened at https://codereview.chromium.org/2889333002,
but somehow not all files follow this convention.

https://codereview.chromium.org/2950943002/diff/220001/ui/webui/resources/cr_...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.html
(right):

https://codereview.chromium.org/2950943002/diff/220001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.html:15:
<content select=".avatar"></content>
On 2017/06/22 at 21:45:31, tommycli wrote:
> Why do you need to select only .avatar class children?

At cr_profile_avatar_selector.html the only thing passed to the grid looks as
follows:
<paper-button class="avatar">...</paper-button>

Given that now there is a call to querySelectorAll() in
cr_profile_avatar_selector_grid.js, I figured that using the 'avatar' class
instead of 'paper-button' was better since paper-button is just an
implementation detail, whereas 'avatar' has some semantic meaning.

I can revert this and change the selector to querySelectorAll('paper-button') if
you think is better.

https://codereview.chromium.org/2950943002/diff/220001/ui/webui/resources/cr_...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js
(right):

https://codereview.chromium.org/2950943002/diff/220001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:77:
break;
On 2017/06/22 at 21:45:31, tommycli wrote:
> Hmm... how can nextItem be null or undefined in this case, isn't it an element
of an array returned by querySelectorAll?

It can happen because the gridSize can be larger than the items.length. See
example at http://imgur.com/a/dFWiE where there is an empty spot in the grid.
Pressing the down arrow while the soccer ball is focused triggers this codepath
(in order to wrap around to the top).

[tommycli, 2017-06-22 23:25:32]

LGTM (just one requested comment add below)

https://codereview.chromium.org/2950943002/diff/220001/ui/webui/resources/cr_...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.html
(right):

https://codereview.chromium.org/2950943002/diff/220001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.html:15:
<content select=".avatar"></content>
On 2017/06/22 23:22:48, dpapad wrote:
> On 2017/06/22 at 21:45:31, tommycli wrote:
> > Why do you need to select only .avatar class children?
> 
> At cr_profile_avatar_selector.html the only thing passed to the grid looks as
> follows:
> <paper-button class="avatar">...</paper-button>
> 
> Given that now there is a call to querySelectorAll() in
> cr_profile_avatar_selector_grid.js, I figured that using the 'avatar' class
> instead of 'paper-button' was better since paper-button is just an
> implementation detail, whereas 'avatar' has some semantic meaning.
> 
> I can revert this and change the selector to querySelectorAll('paper-button')
if
> you think is better.

No that's good. I just was curious as to reasoning.

https://codereview.chromium.org/2950943002/diff/220001/ui/webui/resources/cr_...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js
(right):

https://codereview.chromium.org/2950943002/diff/220001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:77:
break;
On 2017/06/22 23:22:48, dpapad wrote:
> On 2017/06/22 at 21:45:31, tommycli wrote:
> > Hmm... how can nextItem be null or undefined in this case, isn't it an
element
> of an array returned by querySelectorAll?
> 
> It can happen because the gridSize can be larger than the items.length. See
> example at http://imgur.com/a/dFWiE where there is an empty spot in the grid.
> Pressing the down arrow while the soccer ball is focused triggers this
codepath
> (in order to wrap around to the top).

Ah gotcha. Could be worth adding an explanation comment since it wasn't very
obvious to the reader.

[dpapad, 2017-06-22 23:47:55]

Thanks.

https://codereview.chromium.org/2950943002/diff/220001/ui/webui/resources/cr_...
File
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js
(right):

https://codereview.chromium.org/2950943002/diff/220001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_profile_avatar_selector/cr_profile_avatar_selector_grid.js:77:
break;
> Ah gotcha. Could be worth adding an explanation comment since it wasn't very
obvious to the reader.

Done.

[dpapad, 2017-06-22 23:49:41]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2017-06-22 23:49:42]

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/2950943002/#ps260001
(title: "Added comment.")

[commit-bot: I haz the power, 2017-06-22 23:50:03]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-23 01:54:33]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1498175381764520,
"parent_rev": "ddcb2490139405d4cafae70b3adfefd6b3b11a2f", "commit_rev":
"e67dbad97952415cabff9444ed85f543579240e6"}

[commit-bot: I haz the power, 2017-06-23 01:54:47]

Description was changed from

==========
MD Settings: Stop using IronMenuBehavior from cr-profile-avatar-selector-grid.

This both simplifies the code and fixes the selection bug. The original
code relied on multiple inherited Polymer behaviors which stomped on each other
causing what seems like a race condition per findings at
https://bugs.chromium.org/p/chromium/issues/detail?id=734789#c2.

Specifically: 
 - Simplify logic within cr-profile-avatar-selector-grid, no more inherited
   behaviors.
 - Update both settings/ and md_user_manager/ client code.
 - Stop relying on bubbling 'iron-activate' event in manage_profile.html.
   Instead use a Polymer binding + observer.

Regarding the race condition: The previous code, was seemingly attempting to
highlight the grid with the currently selected avatar icon on creation, but
unsuccessfully because this was intentionally broken within the C++ side at
crbug.com/710660. As a side-effect, the 'profile-info-changed' event
propagating back from C++ after every user-initiated change, was causing the
selection of the user to be immediately discarded.

It would make little sense to re-implement JS functionality that is already not
working. Instead I am removing such tests, since they are meaningless until/if
the C++ side is fixed.

BUG=734789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Stop using IronMenuBehavior from cr-profile-avatar-selector-grid.

This both simplifies the code and fixes the selection bug. The original
code relied on multiple inherited Polymer behaviors which stomped on each other
causing what seems like a race condition per findings at
https://bugs.chromium.org/p/chromium/issues/detail?id=734789#c2.

Specifically: 
 - Simplify logic within cr-profile-avatar-selector-grid, no more inherited
   behaviors.
 - Update both settings/ and md_user_manager/ client code.
 - Stop relying on bubbling 'iron-activate' event in manage_profile.html.
   Instead use a Polymer binding + observer.

Regarding the race condition: The previous code, was seemingly attempting to
highlight the grid with the currently selected avatar icon on creation, but
unsuccessfully because this was intentionally broken within the C++ side at
crbug.com/710660. As a side-effect, the 'profile-info-changed' event
propagating back from C++ after every user-initiated change, was causing the
selection of the user to be immediately discarded.

It would make little sense to re-implement JS functionality that is already not
working. Instead I am removing such tests, since they are meaningless until/if
the C++ side is fixed.

BUG=734789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2950943002
Cr-Commit-Position: refs/heads/master@{#481776}
Committed:
https://chromium.googlesource.com/chromium/src/+/e67dbad97952415cabff9444ed85...
==========

[commit-bot: I haz the power, 2017-06-23 01:54:49]

Committed patchset #13 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/e67dbad97952415cabff9444ed85...