Update avatar button to MD

Update avatar button to MD

Summary of changes made [ see screenshots in https://bugs.chromium.org/p/chromium/issues/detail?id=635699&desc=2#c36 ]:

1) Show new MD button only on Windows 10 and only if not using a theme
2) Make MD button "cozy" (22px) when tabstrip is full and not maximised and text direction is LTR (RTL can be tested with --force-ui-direction=rtl)
3) Make "tall" (non-cozy) MD button the same height as caption bar buttons - it's not pixel-perfect (see issue 668278)
4) Change icon and highlighting for MD button (see screenshots attached)
5) Show "ink drop" animation when MD button is clicked, like for bookmarks bar and other menu buttons
6) Make menu appear on press, not on release (for all platforms, not just Windows 10)
7) Keep button in "pressed" state while the profile menu is shown (for all platforms)

BUG=635699

[emx, 2017-04-20 14:43:03]

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

[commit-bot: I haz the power, 2017-04-20 14:43:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emx, 2017-04-20 15:22:16]

Description was changed from

==========
Update avatar button to MD

BUG=635699
==========

to

==========
Update avatar button to MD

Summary of changes made [see screenshots in
https://bugs.chromium.org/p/chromium/issues/detail?id=635699&desc=2#c36]:

1) Show new MD button only on Windows 10 and only if not using a theme
2) Make MD button "cozy" (22px) when tabstrip is full and not maximised and not
text direction is not RTL (can be tested with --force-ui-direction=rtl)
3) Make "tall" (non-cozy) MD button the same height as caption bar buttons -
it's not pixel-perfect (see issue 668278)
4) Change icon and highlighting for MD button (see screenshots attached)
5) Show "ink drop" animation when MD button is clicked, like for bookmarks bar
and other menu buttons
6) Make menu appear on press, not on release (for all platforms, not just
Windows 10)
7) Keep button in "pressed" state while the profile menu is shown (for all
platforms)

BUG=635699
==========

[emx, 2017-04-20 15:28:03]

Description was changed from

==========
Update avatar button to MD

Summary of changes made [see screenshots in
https://bugs.chromium.org/p/chromium/issues/detail?id=635699&desc=2#c36]:

1) Show new MD button only on Windows 10 and only if not using a theme
2) Make MD button "cozy" (22px) when tabstrip is full and not maximised and not
text direction is not RTL (can be tested with --force-ui-direction=rtl)
3) Make "tall" (non-cozy) MD button the same height as caption bar buttons -
it's not pixel-perfect (see issue 668278)
4) Change icon and highlighting for MD button (see screenshots attached)
5) Show "ink drop" animation when MD button is clicked, like for bookmarks bar
and other menu buttons
6) Make menu appear on press, not on release (for all platforms, not just
Windows 10)
7) Keep button in "pressed" state while the profile menu is shown (for all
platforms)

BUG=635699
==========

to

==========
Update avatar button to MD

Summary of changes made [ see screenshots in
https://bugs.chromium.org/p/chromium/issues/detail?id=635699&desc=2#c36 ]:

1) Show new MD button only on Windows 10 and only if not using a theme
2) Make MD button "cozy" (22px) when tabstrip is full and not maximised and not
text direction is not RTL (can be tested with --force-ui-direction=rtl)
3) Make "tall" (non-cozy) MD button the same height as caption bar buttons -
it's not pixel-perfect (see issue 668278)
4) Change icon and highlighting for MD button (see screenshots attached)
5) Show "ink drop" animation when MD button is clicked, like for bookmarks bar
and other menu buttons
6) Make menu appear on press, not on release (for all platforms, not just
Windows 10)
7) Keep button in "pressed" state while the profile menu is shown (for all
platforms)

BUG=635699
==========

[commit-bot: I haz the power, 2017-04-20 15:30:55]

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

[commit-bot: I haz the power, 2017-04-20 15:30:56]

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

[emx, 2017-04-20 15:38:55]

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

[commit-bot: I haz the power, 2017-04-20 15:39:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emx, 2017-04-20 15:42:06]

emx@chromium.org changed reviewers:
+ msarda@chromium.org

[emx, 2017-04-20 15:42:07]

Initial review, please!

[commit-bot: I haz the power, 2017-04-20 16:43:19]

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

[commit-bot: I haz the power, 2017-04-20 16:43:20]

Dry run: This issue passed the CQ dry run.

[msarda, 2017-04-21 09:43:28]

The CL looks good overall. I have a bunch of nit comments.


I think for the ease of the review, it would have been very wise to extract the
minimize button height in a separate CL. Would it be possible to do that now,
before adding more reviewers? I think that will speed the review process and at
the end you'll have 2 committed CLs :)

CL description change: On line "2) Make MD button "cozy" (22px) when tabstrip is
full and not maximised and not text direction is not RTL", change " not text
direction is not RTL" to "text direction is LTR"

I think you also need to add assets for 200 percent.

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/avatar_button_manager.cc (right):

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/avatar_button_manager.cc:40: LOG_ASSERT(style ==
AvatarButtonStyle::NATIVE);
If this is an invariant, then it would be better to use DCHECK instead of
LOG_ASSERT. DCHECK crashes the application when built in debug mode and it
allows us to catch the cases when this assertion fails:
DCHECK_EQ(AvatarButtonStyle::NATIVE, style);

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:728: // May switch
between cozy and tall MD avatar button here
Add a "." at the end of the comment.

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:739: void
GlassBrowserFrameView::TabStripDeleted(TabStrip* tab_strip) {
Maybe check that the tab strip pointers match:
DCHECK_EQ(tab_strip, tab_strip_);

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:747: if
(!GetProfileSwitcherButton())
Instead of caling GetProfileSwitcherButton() 4 times, consider extracting it to
a local variable.

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:763: 
Revert the add of this new line.

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:772:
profile_switcher_.button()->UpdateButtonHeightForPosition(button_x,
Should this be: GetProfileSwitcherButton()?

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/glass_browser_frame_view.h (right):

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.h:201: // TabStrip, which
we need to watch for changes to resize the avatar button
It is not clear what "we" means in this context:
* is it the developper?
* is it this class?

Please rephrase. Maybe something like: 
// The tab strip, which needs to be observed for changes in order to resize the
avatar button.

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right):

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:205: // We want the
"tall" avatar button to be the same height as the caption
Who is "we"?

Also, it is not clear why the comment in this file references the avatar button
(what is the avatar button in this context?). Remove the reference to the avatar
button in this comment, but keep the rest of the comment as it is really good.

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:221: LOG(ERROR) <<
"GetSystemMetrics(SM_CYSIZE) error " << ::GetLastError();
If the user is in this case, then I think this line will write a log of logs
that are in general useless for the regular end user. I would argue that we
should only log this in debug, using DLOG.

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:222: return
GetDefaultButtonHeight();
From the name of the method, it looks like this function is supposed to return
the minimize button height for all Windows versions. However, in the
implementation, the default value is only correct for Windows 10.

I see 2 options:
* Change the method name to be Windows10 specific. And a DCHECK to check that
the OS version is at least 10 when this method is called.
* Have the right defaults for other versions. If 30 is the right default for all
windows versions, then change the constant name to kWinButtonHeight.

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:181: views::View*
button_view = GetProfileSwitcherButton();
Nit: This is just the button, not another view. Consider changing as:
s/button_view/button (maybe also change its type to views::MenuButton).

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/new_avatar_button.cc (right):

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/new_avatar_button.cc:146: 
Remove blank line.

[emx, 2017-04-21 11:26:20]

Description was changed from

==========
Update avatar button to MD

Summary of changes made [ see screenshots in
https://bugs.chromium.org/p/chromium/issues/detail?id=635699&desc=2#c36 ]:

1) Show new MD button only on Windows 10 and only if not using a theme
2) Make MD button "cozy" (22px) when tabstrip is full and not maximised and not
text direction is not RTL (can be tested with --force-ui-direction=rtl)
3) Make "tall" (non-cozy) MD button the same height as caption bar buttons -
it's not pixel-perfect (see issue 668278)
4) Change icon and highlighting for MD button (see screenshots attached)
5) Show "ink drop" animation when MD button is clicked, like for bookmarks bar
and other menu buttons
6) Make menu appear on press, not on release (for all platforms, not just
Windows 10)
7) Keep button in "pressed" state while the profile menu is shown (for all
platforms)

BUG=635699
==========

to

==========
Update avatar button to MD

Summary of changes made [ see screenshots in
https://bugs.chromium.org/p/chromium/issues/detail?id=635699&desc=2#c36 ]:

1) Show new MD button only on Windows 10 and only if not using a theme
2) Make MD button "cozy" (22px) when tabstrip is full and not maximised and text
direction is LTR (RTL can be tested with --force-ui-direction=rtl)
3) Make "tall" (non-cozy) MD button the same height as caption bar buttons -
it's not pixel-perfect (see issue 668278)
4) Change icon and highlighting for MD button (see screenshots attached)
5) Show "ink drop" animation when MD button is clicked, like for bookmarks bar
and other menu buttons
6) Make menu appear on press, not on release (for all platforms, not just
Windows 10)
7) Keep button in "pressed" state while the profile menu is shown (for all
platforms)

BUG=635699
==========

[emx, 2017-04-24 16:18:45]

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

[commit-bot: I haz the power, 2017-04-24 16:19:37]

Dry run: CQ is trying da patch.

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

[emx, 2017-04-24 16:23:10]

Mihai, I've addressed all your comments and also:
1) Fixed an issue where the cozy button was cut off at the top at 150% and 175%
scaling (but jumped down on hover!)
2) Removed the border images - they are unnecessary for Windows 10
3) Merged in changes from the menu_button review by sky

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/avatar_button_manager.cc (right):

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/avatar_button_manager.cc:40: LOG_ASSERT(style ==
AvatarButtonStyle::NATIVE);
On 2017/04/21 09:43:28, msarda wrote:
> If this is an invariant, then it would be better to use DCHECK instead of
> LOG_ASSERT. DCHECK crashes the application when built in debug mode and it
> allows us to catch the cases when this assertion fails:
> DCHECK_EQ(AvatarButtonStyle::NATIVE, style);

Done.

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:728: // May switch
between cozy and tall MD avatar button here
On 2017/04/21 09:43:28, msarda wrote:
> Add a "." at the end of the comment.

Done.

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:739: void
GlassBrowserFrameView::TabStripDeleted(TabStrip* tab_strip) {
On 2017/04/21 09:43:28, msarda wrote:
> Maybe check that the tab strip pointers match:
> DCHECK_EQ(tab_strip, tab_strip_);

Done.

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:747: if
(!GetProfileSwitcherButton())
On 2017/04/21 09:43:28, msarda wrote:
> Instead of caling GetProfileSwitcherButton() 4 times, consider extracting it
to
> a local variable.

Done.

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:763: 
On 2017/04/21 09:43:28, msarda wrote:
> Revert the add of this new line.

Done.

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:772:
profile_switcher_.button()->UpdateButtonHeightForPosition(button_x,
On 2017/04/21 09:43:28, msarda wrote:
> Should this be: GetProfileSwitcherButton()?

No, that returns MenuButton* and we need an AvatarButton* here, but I've changed
this method to have only one call to profile_switcher_.button().

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/glass_browser_frame_view.h (right):

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/glass_browser_frame_view.h:201: // TabStrip, which
we need to watch for changes to resize the avatar button
On 2017/04/21 09:43:28, msarda wrote:
> It is not clear what "we" means in this context:
> * is it the developper?
> * is it this class?
> 
> Please rephrase. Maybe something like: 
> // The tab strip, which needs to be observed for changes in order to resize
the
> avatar button.

Done.

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right):

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:205: // We want the
"tall" avatar button to be the same height as the caption
On 2017/04/21 09:43:28, msarda wrote:
> Who is "we"?
> 
> Also, it is not clear why the comment in this file references the avatar
button
> (what is the avatar button in this context?). Remove the reference to the
avatar
> button in this comment, but keep the rest of the comment as it is really good.

Done.

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:221: LOG(ERROR) <<
"GetSystemMetrics(SM_CYSIZE) error " << ::GetLastError();
On 2017/04/21 09:43:28, msarda wrote:
> If the user is in this case, then I think this line will write a log of logs
> that are in general useless for the regular end user. I would argue that we
> should only log this in debug, using DLOG.

Done.

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:222: return
GetDefaultButtonHeight();
On 2017/04/21 09:43:28, msarda wrote:
> From the name of the method, it looks like this function is supposed to return
> the minimize button height for all Windows versions. However, in the
> implementation, the default value is only correct for Windows 10.
> 
> I see 2 options:
> * Change the method name to be Windows10 specific. And a DCHECK to check that
> the OS version is at least 10 when this method is called.
> * Have the right defaults for other versions. If 30 is the right default for
all
> windows versions, then change the constant name to kWinButtonHeight.

Done.

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:181: views::View*
button_view = GetProfileSwitcherButton();
On 2017/04/21 09:43:28, msarda wrote:
> Nit: This is just the button, not another view. Consider changing as:
> s/button_view/button (maybe also change its type to views::MenuButton).

Done.

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/new_avatar_button.cc (right):

https://codereview.chromium.org/2832823002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/new_avatar_button.cc:146: 
On 2017/04/21 09:43:28, msarda wrote:
> Remove blank line.

Done.

[commit-bot: I haz the power, 2017-04-24 16:33:15]

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

[commit-bot: I haz the power, 2017-04-24 16:33:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emx, 2017-04-24 16:43:45]

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

[commit-bot: I haz the power, 2017-04-24 16:44:06]

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-04-24 18:15:27]

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

[commit-bot: I haz the power, 2017-04-24 18:15:29]

Dry run: This issue passed the CQ dry run.

[msarda, 2017-04-25 08:26:02]

LGTM with 2 nits.

https://codereview.chromium.org/2832823002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/new_avatar_button.cc (right):

https://codereview.chromium.org/2832823002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/new_avatar_button.cc:164: void
AvatarButton::SetButtonFromIdr(const int button_idr) {
Maybe: SetButtonAvatar(const int avatar_idr)?

https://codereview.chromium.org/2832823002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/new_avatar_button.h (right):

https://codereview.chromium.org/2832823002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/new_avatar_button.h:40: void
SetButtonFromIdr(const int button_idr);
It is not clear what this method does. May I ask you to add a short comment for
it?

[emx, 2017-04-25 12:41:59]

emx@chromium.org changed reviewers:
+ pkasting@chromium.org

[emx, 2017-04-25 12:42:00]

Hi Peter, this is the main CL for the avatar button change, which includes the
changes from 2833363002.

[emx, 2017-04-25 12:51:07]

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

[commit-bot: I haz the power, 2017-04-25 12:51:19]

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-04-25 13:35:12]

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

[commit-bot: I haz the power, 2017-04-25 13:35:13]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2017-04-26 02:09:53]

Only reviewed up through browser_non_client_frame_view_mus.cc due to lack of
time.

https://codereview.chromium.org/2832823002/diff/120001/chrome/app/theme/theme...
File chrome/app/theme/theme_resources.grd (right):

https://codereview.chromium.org/2832823002/diff/120001/chrome/app/theme/theme...
chrome/app/theme/theme_resources.grd:438: <structure type="chrome_scaled_image"
name="IDR_AVATAR_MD_BUTTON_AVATAR"
file="win/avatar_button/win10/sign_in_button_avatar.png" />
Please don't introduce new PNG resources; use vector resources (in .icon format)
instead.

You can easily create .icons from .svgs if the designers supply those, or
(ideally) from the original Material Design icons when they match -- looks like
this is https://material.io/icons/#ic_account_circle .

Check to make sure we don't already have this icon in our tree.

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/avatar_button_manager.cc (right):

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/avatar_button_manager.cc:7: #include
"base/win/windows_version.h"
Guard this with an #if OS_WIN block.

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/avatar_button_manager.cc:39:
ThemeServiceFactory::GetForProfile(profile)->UsingSystemTheme()) {
Nit: TODO about expanding this beyond Win 10 and system theme?

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/avatar_button_manager.cc:77: if (event &&
event->type() == ui::ET_GESTURE_LONG_PRESS)
This looks like we're showing the dropdown on right-click now, which seems
strange.  Why change this?

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/avatar_button_manager.h (right):

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/avatar_button_manager.h:9: #include
"chrome/browser/ui/views/profiles/avatar_button_style.h"
Nit: Seems like you could forward-declare AvatarButton instead.

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/avatar_button_manager.h:11: #include
"ui/views/controls/button/menu_button.h"
Nit: Seems like you shouldn't need this (we should have the necessary prototype
from our superclass).

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/avatar_button_manager.h:25: AvatarButton* button()
const { return button_; }
Const methods should not return non-const pointers; make method non-const or
pointer const.

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/avatar_button_manager.h:31: //
views::MenuButtonListener:
Nit: You don't directly inherit from AvatarButtonDelegate, so prefer to label
this as coming from that.

Per recent discussion on chromium-dev, overrides should match the visibility of
their parent, so this function should probably be public.

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_frame.h (right):

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_frame.h:14: #include
"ui/views/controls/button/menu_button.h"
Nit: Forward-declare rather than #include

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.h (right):

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.h:10: #include
"ui/views/controls/button/menu_button.h"
Nit: Forward-declare rather than #include

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc (right):

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc:255:
ConvertedHitTest(this, GetProfileSwitcherButton(), point)) {
Nit: Use a temp to capture the return value of CamelCaseFunctions() when you're
going to check them multiple times in one function (multiple places)

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.h (right):

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.h:15: #include
"ui/views/controls/button/menu_button.h"
Nit: Forward-declare rather than #include

[Evan Stade, 2017-04-27 01:30:53]

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

[Evan Stade, 2017-04-27 01:30:55]

Peter pinged me to take a look at this CL because I'm working on a similar patch
(updating the non-win10 button, essentially). I think you should go ahead with
this one and then I can rearrange my changes on top of it, but I have some
suggestions about how this can be made easier.

https://codereview.chromium.org/2832823002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/new_avatar_button.cc (right):

https://codereview.chromium.org/2832823002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/new_avatar_button.cc:204: gfx::Size ps =
views::MenuButton::GetPreferredSize();
please don't use abbreviations like this for variable names

https://codereview.chromium.org/2832823002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/new_avatar_button.h (right):

https://codereview.chromium.org/2832823002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/new_avatar_button.h:40: void
set_generic_avatar(gfx::ImageSkia generic_avatar) {
const ref

https://codereview.chromium.org/2832823002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/new_avatar_button.h:60: Profile* profile,
why does this take a profile param instead of using profile_? ditto
error_controller_

https://codereview.chromium.org/2832823002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/new_avatar_button.h:75: class NewAvatarButton :
public AvatarButton {
This should be called something like ThemedAvatarButton, and I believe it
deserves its own file.

(Note that I am also making changes to this class, i.e. avatar buttons for
opaque browser frame, and both this and the class below will follow the general
principles of MD, so "new" and "md" are not really apt descriptors.)

https://codereview.chromium.org/2832823002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/new_avatar_button.h:91: void
SetButtonAvatar(const int avatar_idr);
nit: no const on POD parameters

https://codereview.chromium.org/2832823002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/new_avatar_button.h:96: class
MaterialDesignAvatarButton : public AvatarButton {
This should be called something like Win10NativeAvatarButton rather than
MaterialDesignAvatarButton, and I think it deserves its own file.

https://codereview.chromium.org/2832823002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/2832823002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:645: void
ProfileChooserView::MakeCallingButtonPressedIfShowing(
it would be easier to review this patch if it didn't do so many things at once.
As far as fixing the pressed state, I believe we should deal with this in a
slightly different way, which works better for ripples and has less
inter-dependency (e.g. I don't think you'd need to make so many classes aware
that the avatar button is a MenuButton):
https://codereview.chromium.org/2832223003/

So my suggestion would be to remove this part of the change for now. After
landing this CL, I'll rebase my changes on top of it and we'll have the ripple
activated state when the bubble's open.

[emx, 2017-04-27 16:30:59]

https://codereview.chromium.org/2832823002/diff/120001/chrome/app/theme/theme...
File chrome/app/theme/theme_resources.grd (right):

https://codereview.chromium.org/2832823002/diff/120001/chrome/app/theme/theme...
chrome/app/theme/theme_resources.grd:438: <structure type="chrome_scaled_image"
name="IDR_AVATAR_MD_BUTTON_AVATAR"
file="win/avatar_button/win10/sign_in_button_avatar.png" />
On 2017/04/26 02:09:53, Peter Kasting wrote:
> Please don't introduce new PNG resources; use vector resources (in .icon
format)
> instead.
> 
> You can easily create .icons from .svgs if the designers supply those, or
> (ideally) from the original Material Design icons when they match -- looks
like
> this is https://material.io/icons/#ic_account_circle .
> 
> Check to make sure we don't already have this icon in our tree.

Done.

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/avatar_button_manager.cc (right):

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/avatar_button_manager.cc:7: #include
"base/win/windows_version.h"
On 2017/04/26 02:09:53, Peter Kasting wrote:
> Guard this with an #if OS_WIN block.

Done.

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/avatar_button_manager.cc:39:
ThemeServiceFactory::GetForProfile(profile)->UsingSystemTheme()) {
On 2017/04/26 02:09:53, Peter Kasting wrote:
> Nit: TODO about expanding this beyond Win 10 and system theme?

Done.

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/avatar_button_manager.cc:77: if (event &&
event->type() == ui::ET_GESTURE_LONG_PRESS)
On 2017/04/26 02:09:53, Peter Kasting wrote:
> This looks like we're showing the dropdown on right-click now, which seems
> strange.  Why change this?

We're not - OnMenuButtonClicked is simply not called for a right-click (unlike
ButtonPressed), so that check is no longer necessary.

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/avatar_button_manager.h (right):

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/avatar_button_manager.h:11: #include
"ui/views/controls/button/menu_button.h"
On 2017/04/26 02:09:53, Peter Kasting wrote:
> Nit: Seems like you shouldn't need this (we should have the necessary
prototype
> from our superclass).

Done.

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/avatar_button_manager.h:31: //
views::MenuButtonListener:
On 2017/04/26 02:09:53, Peter Kasting wrote:
> Nit: You don't directly inherit from AvatarButtonDelegate, so prefer to label
> this as coming from that.
> 
> Per recent discussion on chromium-dev, overrides should match the visibility
of
> their parent, so this function should probably be public.

Done.

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_frame.h (right):

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_frame.h:14: #include
"ui/views/controls/button/menu_button.h"
On 2017/04/26 02:09:53, Peter Kasting wrote:
> Nit: Forward-declare rather than #include

Done.

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.h (right):

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.h:10: #include
"ui/views/controls/button/menu_button.h"
On 2017/04/26 02:09:53, Peter Kasting wrote:
> Nit: Forward-declare rather than #include

Done.

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc (right):

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc:255:
ConvertedHitTest(this, GetProfileSwitcherButton(), point)) {
On 2017/04/26 02:09:53, Peter Kasting wrote:
> Nit: Use a temp to capture the return value of CamelCaseFunctions() when
you're
> going to check them multiple times in one function (multiple places)

Done.

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.h (right):

https://codereview.chromium.org/2832823002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.h:15: #include
"ui/views/controls/button/menu_button.h"
On 2017/04/26 02:09:53, Peter Kasting wrote:
> Nit: Forward-declare rather than #include

Done.

https://codereview.chromium.org/2832823002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/new_avatar_button.cc (right):

https://codereview.chromium.org/2832823002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/new_avatar_button.cc:204: gfx::Size ps =
views::MenuButton::GetPreferredSize();
On 2017/04/27 01:30:55, Evan Stade wrote:
> please don't use abbreviations like this for variable names

Done.

https://codereview.chromium.org/2832823002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/new_avatar_button.h (right):

https://codereview.chromium.org/2832823002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/new_avatar_button.h:40: void
set_generic_avatar(gfx::ImageSkia generic_avatar) {
On 2017/04/27 01:30:55, Evan Stade wrote:
> const ref

Done.

https://codereview.chromium.org/2832823002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/new_avatar_button.h:60: Profile* profile,
On 2017/04/27 01:30:55, Evan Stade wrote:
> why does this take a profile param instead of using profile_? ditto
> error_controller_

Mistakenly left over from an earlier implementation. Fixed.

https://codereview.chromium.org/2832823002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/new_avatar_button.h:75: class NewAvatarButton :
public AvatarButton {
On 2017/04/27 01:30:55, Evan Stade wrote:
> This should be called something like ThemedAvatarButton, and I believe it
> deserves its own file.
> 
> (Note that I am also making changes to this class, i.e. avatar buttons for
> opaque browser frame, and both this and the class below will follow the
general
> principles of MD, so "new" and "md" are not really apt descriptors.)

This isn't only used for themes, though, it's also used on platforms other than
Windows 10. What about "DefaultAvatarButton"? Any other ideas for a name?

https://codereview.chromium.org/2832823002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/new_avatar_button.h:91: void
SetButtonAvatar(const int avatar_idr);
On 2017/04/27 01:30:55, Evan Stade wrote:
> nit: no const on POD parameters

Done.

https://codereview.chromium.org/2832823002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/new_avatar_button.h:96: class
MaterialDesignAvatarButton : public AvatarButton {
On 2017/04/27 01:30:55, Evan Stade wrote:
> This should be called something like Win10NativeAvatarButton rather than
> MaterialDesignAvatarButton, and I think it deserves its own file.

"Win10NativeAvatarButton" sounds good to me. I'll change this together with the
other class, once we agree on the name.

https://codereview.chromium.org/2832823002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/2832823002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:645: void
ProfileChooserView::MakeCallingButtonPressedIfShowing(
On 2017/04/27 01:30:55, Evan Stade wrote:
> it would be easier to review this patch if it didn't do so many things at
once.
> As far as fixing the pressed state, I believe we should deal with this in a
> slightly different way, which works better for ripples and has less
> inter-dependency (e.g. I don't think you'd need to make so many classes aware
> that the avatar button is a MenuButton):
> https://codereview.chromium.org/2832223003/
> 
> So my suggestion would be to remove this part of the change for now. After
> landing this CL, I'll rebase my changes on top of it and we'll have the ripple
> activated state when the bubble's open.

OK, I'm creating a new CL without this.

[Evan Stade, 2017-04-27 17:03:30]

https://codereview.chromium.org/2832823002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/new_avatar_button.h (right):

https://codereview.chromium.org/2832823002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/new_avatar_button.h:75: class NewAvatarButton :
public AvatarButton {
On 2017/04/27 16:30:58, emx wrote:
> On 2017/04/27 01:30:55, Evan Stade wrote:
> > This should be called something like ThemedAvatarButton, and I believe it
> > deserves its own file.
> > 
> > (Note that I am also making changes to this class, i.e. avatar buttons for
> > opaque browser frame, and both this and the class below will follow the
> general
> > principles of MD, so "new" and "md" are not really apt descriptors.)
> 
> This isn't only used for themes, though, it's also used on platforms other
than
> Windows 10. What about "DefaultAvatarButton"? Any other ideas for a name?

You're right, it's always used on Linux. But using "theme" in the name matches
the naming convention of AvatarButtonStyle. The only other name that makes sense
I guess is "opaque" something, because it's used exclusively by the
OpaqueBrowserFrame, except that is also kinda weird because it's not necessarily
actually opaque. So I'd stick with "themed" for lack of a better word, unless
you can come up with a better word and want to also rename the AvatarButtonStyle
values.

[Peter Kasting, 2017-04-28 06:51:55]

Sorry, I didn't get to reviewing the new version of this before going on
vacation.

I agree with Evan's sentiment that this might have been easier to review if
split into multiple patches that made smaller changes.  Even if one of those
changes would have had most of the code change, just eliminating the other
behavior changes would mean I'd have less to worry about and watch out for
mentally while reading.

If you'd like to see progress on this while I'm gone, I suggest having bsep@
take a pass, as he is actively involved with Harmony and knows the frame code. 
Otherwise, I'll look when I get back in a week.

[Peter Kasting, 2017-05-05 23:59:27]

It looks like you split out part of this into
https://codereview.chromium.org/2851543002/ .  I'm going to hold off reviewing
this again until that lands and/or you tell me to re-review here.

[msarda, 2017-05-16 13:55:42]

msarda@chromium.org changed reviewers:
- msarda@chromium.org

[Peter Kasting, 2017-08-29 04:10:31]

pkasting@chromium.org changed reviewers:
+ bsep@chromium.org

[Peter Kasting, 2017-08-29 04:10:32]

Author no longer on the project; closing.  +CC bsep as feature owner in case
there's any value here (though it'd probably take a rebase to find out).