Simplify some old avatar menu button code.

Simplify some old avatar menu button code.

The view is still used for:
- the incognito icon
- the CrOS multi-profile teleported window case. To see this, sign into
  a second user, open a window, and click "send to XXX's desktop".

It no longer needs to have a menu attached ever.

BUG=534850, 591586
TBR=achuith@chromium.org

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

[Evan Stade, 2016-05-12 19:07:47]

Description was changed from

==========
Simplify some old avatar menu button code.

The view is still used for:
- the incognito icon
- the CrOS multi-profile teleported window case. To see this, sign into
  a second user, open a window, and click "send to XXX's desktop".

It no longer needs to have a menu attached ever.

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

to

==========
Simplify some old avatar menu button code.

The view is still used for:
- the incognito icon
- the CrOS multi-profile teleported window case. To see this, sign into
  a second user, open a window, and click "send to XXX's desktop".

It no longer needs to have a menu attached ever.

BUG=534850
==========

[Evan Stade, 2016-05-12 19:07:47]

estade@chromium.org changed reviewers:
+ anthonyvd@chromium.org, pkasting@chromium.org

[Evan Stade, 2016-05-12 19:07:54]

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

[commit-bot: I haz the power, 2016-05-12 19:08:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1972033002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1972033002/120001

[Evan Stade, 2016-05-12 19:09:17]

Description was changed from

==========
Simplify some old avatar menu button code.

The view is still used for:
- the incognito icon
- the CrOS multi-profile teleported window case. To see this, sign into
  a second user, open a window, and click "send to XXX's desktop".

It no longer needs to have a menu attached ever.

BUG=534850
==========

to

==========
Simplify some old avatar menu button code.

The view is still used for:
- the incognito icon
- the CrOS multi-profile teleported window case. To see this, sign into
  a second user, open a window, and click "send to XXX's desktop".

It no longer needs to have a menu attached ever.

BUG=534850,591586
==========

[Evan Stade, 2016-05-12 19:09:21]

"avatar_button", "new avatar button", "old avatar button" was confusing me so
thoroughly I had to fix it before I could even attempt to update the profile
switcher menu button (aka "new avatar button").

[commit-bot: I haz the power, 2016-05-12 19:22:39]

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

[commit-bot: I haz the power, 2016-05-12 19:22:40]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2016-05-16 22:53:33]

ping

[Peter Kasting, 2016-05-17 03:51:43]

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/profile...
File chrome/browser/profiles/avatar_menu_desktop.cc (left):

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/profile...
chrome/browser/profiles/avatar_menu_desktop.cc:24: NOTREACHED();
So can this actually be reached, maybe for disk corruption or something?  Might
be nice to add a comment about what case results in this.

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/view_ids.h (right):

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/view_ids.h:25: VIEW_ID_PROFILE_INDICATOR_ICON,
Nit: I'm not in love with "profile indicator icon" as it seems verbose and
redundant yet vague, but I don't have a great idea myself.  I wish we could just
call it "incognito icon" but the teleported window case throws wrenches at that.
 Maybe just "profile icon" is as descriptive as this (i.e. not very) but at
least less verbose?

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/view_ids.h:26: VIEW_ID_NEW_AVATAR_BUTTON,
Nit: Maybe just AVATAR_BUTTON now?

(Maybe we should get rid of "avatar" altogether since we use labels now and just
call it the "profile button" or something, but that's probably bigger than this
CL.  Although I guess even the phrasing "new avatar button" may be too big to
change in this CL since it encompasses view IDs, function names, variable names,
etc...)

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

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/avatar_button_manager.cc:25: bool show_avatar =
Nit: I'd just inline this into the next statement.

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:173:
AvatarMenu::GetImageForMenuButton(profile->GetPath(), &icon, &unused);
Nit: Does this need to be conditionally compiled in only for ChromeOS?  At a
glance it looks like it would link without the ifdef, in which case, why not
just do that?

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:235: // valid
before using.
Nit: Comment might be clearer as "Tests may not have a browser process or
profile manager, so null-check them before using."

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:240:
chrome::DrawTaskbarDecoration(frame_->GetNativeWindow(), nullptr);
If you're going to make this whole block ifdefed for Windows only, I think we no
longer need the cross-platform declaration of this function and empty definition
of it.  It can be purely Windows-specific, and pulled in to this file via an
ifdefed #include.

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:247: // pinned
the badge-less Chrome shortcut which will cause windows to ignore
Nit: While here: windows -> Windows ?

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:253: bool unused;
Nit: This is the second place in this file that doesn't want to use the third
arg.  Might be nice to allow this function to take nullptr there (maybe even as
a default argument).

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

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.h:84: // Update the
profile switcher button if one should exist. Otherwise, update
Nit: While here: Update -> Updates (2 places)

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.h:91: views::View*
profile_indicator() const { return profile_indicator_; }
Const accessors shouldn't return non-const pointers; either make this non-const
or have it return a const pointer.

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.h:118: // On
desktop, this is used to show an incognito indicator. On CrOS, it's also
Nit: indicator -> icon?

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.h:120:
ProfileIndicatorIcon* profile_indicator_ = nullptr;
Nit: I would make the type name and member name match exactly, whatever you
choose.

It's also a bit weird to me to use an initializer here in the declaration for
this variable but not the other members... would kinda prefer changing them all
(or else none)

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h (right):

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h:117: // Layout
the image that indicates incognito or teleported windows.
Nit: Seems like we ought not to need to explain again what this is here since we
did so in BrowserNonClientFrameView.  Maybe if it's not clear we should just
explicitly tell readers to look there?

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc
(right):

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc:345:
views::MenuButton* new_avatar_button_;
Nit: Again, maybe this should just be |avatar_button_| now

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_indicator_icon.cc (right):

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_indicator_icon.cc:1: // Copyright 2014
The Chromium Authors. All rights reserved.
Nit: 2016

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_indicator_icon.cc:31: #include
"ui/views/widget/widget.h"
Surely we don't need all these #includes?

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_indicator_icon.cc:33: static inline int
Round(double x) {
Use std::round() instead.

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_indicator_icon.cc:39: // In RTL mode,
the avatar icon should be looking the opposite direction.
Nit: avatar -> profile (or nothing)

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_indicator_icon.cc:72: int dst_y =
Round((height() - dst_height) / 2.0);
Nit: 2.0f?

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_indicator_icon.h (right):

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_indicator_icon.h:1: // Copyright 2014
The Chromium Authors. All rights reserved.
Nit: 2016

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_indicator_icon.h:44: 
Nit: Unnecessary blank line

[Evan Stade, 2016-05-17 18:13:09]

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/profile...
File chrome/browser/profiles/avatar_menu_desktop.cc (left):

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/profile...
chrome/browser/profiles/avatar_menu_desktop.cc:24: NOTREACHED();
On 2016/05/17 03:51:42, Peter Kasting wrote:
> So can this actually be reached, maybe for disk corruption or something? 
Might
> be nice to add a comment about what case results in this.

I made this change because we already had a lot of checks and early returns for
this condition in other parts of the code, so it's not exactly a situation that
can't occur. It's simpler IMO to just call GetImageForMenuButton unconditionally
than to check this condition first. I believe it's explained by this
pre-existing comment, which I'll copy here.

// This can happen if the user deletes the current profile.

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/view_ids.h (right):

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/view_ids.h:25: VIEW_ID_PROFILE_INDICATOR_ICON,
On 2016/05/17 03:51:42, Peter Kasting wrote:
> Nit: I'm not in love with "profile indicator icon" as it seems verbose and
> redundant yet vague, but I don't have a great idea myself.  I wish we could
just
> call it "incognito icon" but the teleported window case throws wrenches at
that.
>  Maybe just "profile icon" is as descriptive as this (i.e. not very) but at
> least less verbose?

I think profile icon and profile indicator icon are equally descriptive and the
former is more concise. But I prefer the latter because it's a more distinctive
name. The overuse and reuse of "avatar" was really confusing, hopefully using
"profile indicator" everywhere will be make this hard-to-describe piece of UI
more grep-able and more recognizable.

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/view_ids.h:26: VIEW_ID_NEW_AVATAR_BUTTON,
On 2016/05/17 03:51:42, Peter Kasting wrote:
> Nit: Maybe just AVATAR_BUTTON now?
> 
> (Maybe we should get rid of "avatar" altogether since we use labels now and
just
> call it the "profile button" or something, but that's probably bigger than
this
> CL.  Although I guess even the phrasing "new avatar button" may be too big to
> change in this CL since it encompasses view IDs, function names, variable
names,
> etc...)

Done.

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

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/avatar_button_manager.cc:25: bool show_avatar =
On 2016/05/17 03:51:42, Peter Kasting wrote:
> Nit: I'd just inline this into the next statement.

Done.

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:173:
AvatarMenu::GetImageForMenuButton(profile->GetPath(), &icon, &unused);
On 2016/05/17 03:51:42, Peter Kasting wrote:
> Nit: Does this need to be conditionally compiled in only for ChromeOS?  At a
> glance it looks like it would link without the ifdef, in which case, why not
> just do that?

I wanted to add the NOTREACHED to document that this path shouldn't be hit
except on cros (which is otherwise not obvious).

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:235: // valid
before using.
On 2016/05/17 03:51:42, Peter Kasting wrote:
> Nit: Comment might be clearer as "Tests may not have a browser process or
> profile manager, so null-check them before using."

Done.

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:240:
chrome::DrawTaskbarDecoration(frame_->GetNativeWindow(), nullptr);
On 2016/05/17 03:51:42, Peter Kasting wrote:
> If you're going to make this whole block ifdefed for Windows only, I think we
no
> longer need the cross-platform declaration of this function and empty
definition
> of it.  It can be purely Windows-specific, and pulled in to this file via an
> ifdefed #include.

Done.

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:247: // pinned
the badge-less Chrome shortcut which will cause windows to ignore
On 2016/05/17 03:51:42, Peter Kasting wrote:
> Nit: While here: windows -> Windows ?

Done.

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:253: bool unused;
On 2016/05/17 03:51:42, Peter Kasting wrote:
> Nit: This is the second place in this file that doesn't want to use the third
> arg.  Might be nice to allow this function to take nullptr there (maybe even
as
> a default argument).

turns out it's not used anywhere any more. Removed altogether.

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

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.h:84: // Update the
profile switcher button if one should exist. Otherwise, update
On 2016/05/17 03:51:42, Peter Kasting wrote:
> Nit: While here: Update -> Updates (2 places)

Done.

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.h:91: views::View*
profile_indicator() const { return profile_indicator_; }
On 2016/05/17 03:51:42, Peter Kasting wrote:
> Const accessors shouldn't return non-const pointers; either make this
non-const
> or have it return a const pointer.

Done.

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.h:118: // On
desktop, this is used to show an incognito indicator. On CrOS, it's also
On 2016/05/17 03:51:42, Peter Kasting wrote:
> Nit: indicator -> icon?

Done.

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.h:120:
ProfileIndicatorIcon* profile_indicator_ = nullptr;
On 2016/05/17 03:51:42, Peter Kasting wrote:
> Nit: I would make the type name and member name match exactly, whatever you
> choose.

done

> 
> It's also a bit weird to me to use an initializer here in the declaration for
> this variable but not the other members... would kinda prefer changing them
all
> (or else none)

well, I did change none :). But I'll move this one to the initializer list.

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h (right):

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h:117: // Layout
the image that indicates incognito or teleported windows.
On 2016/05/17 03:51:42, Peter Kasting wrote:
> Nit: Seems like we ought not to need to explain again what this is here since
we
> did so in BrowserNonClientFrameView.  Maybe if it's not clear we should just
> explicitly tell readers to look there?

removed. This is an example where "ProfileIndicator" makes it easier to
guess/figure out what this is referring to (as opposed to the more generic
"ProfileIcon").

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc
(right):

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc:345:
views::MenuButton* new_avatar_button_;
On 2016/05/17 03:51:42, Peter Kasting wrote:
> Nit: Again, maybe this should just be |avatar_button_| now

Done.

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_indicator_icon.cc (right):

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_indicator_icon.cc:1: // Copyright 2014
The Chromium Authors. All rights reserved.
On 2016/05/17 03:51:43, Peter Kasting wrote:
> Nit: 2016

this is just a trimmed down version of avatar_menu_button.cc but if git doesn't
recognize it as such (without messing with similarity) then I guess we can treat
it as a new file.

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_indicator_icon.cc:31: #include
"ui/views/widget/widget.h"
On 2016/05/17 03:51:43, Peter Kasting wrote:
> Surely we don't need all these #includes?

Done.

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_indicator_icon.cc:33: static inline int
Round(double x) {
On 2016/05/17 03:51:43, Peter Kasting wrote:
> Use std::round() instead.

Done.

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_indicator_icon.cc:39: // In RTL mode,
the avatar icon should be looking the opposite direction.
On 2016/05/17 03:51:43, Peter Kasting wrote:
> Nit: avatar -> profile (or nothing)

removed --- I don't think we want this anywhere except pre-MD incognito, so it's
moved to BrowserNonClientFrameView

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_indicator_icon.cc:72: int dst_y =
Round((height() - dst_height) / 2.0);
On 2016/05/17 03:51:43, Peter Kasting wrote:
> Nit: 2.0f?

Done.

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_indicator_icon.h (right):

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_indicator_icon.h:1: // Copyright 2014
The Chromium Authors. All rights reserved.
On 2016/05/17 03:51:43, Peter Kasting wrote:
> Nit: 2016

Done.

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_indicator_icon.h:44: 
On 2016/05/17 03:51:43, Peter Kasting wrote:
> Nit: Unnecessary blank line

Done.

[Evan Stade, 2016-05-17 18:13:20]

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

[Evan Stade, 2016-05-17 18:21:26]

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

[commit-bot: I haz the power, 2016-05-17 18:22:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1972033002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1972033002/180001

[commit-bot: I haz the power, 2016-05-17 18:29:42]

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

[commit-bot: I haz the power, 2016-05-17 18:29:43]

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

[anthonyvd, 2016-05-17 18:30:07]

c/b/profiles lgtm. Great cleanup!

[Evan Stade, 2016-05-18 00:17:22]

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

[commit-bot: I haz the power, 2016-05-18 00:18:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1972033002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1972033002/200001

[commit-bot: I haz the power, 2016-05-18 00:33:07]

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

[commit-bot: I haz the power, 2016-05-18 00:33:09]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Kasting, 2016-05-18 02:44:01]

LGTM

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/view_ids.h (right):

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/view_ids.h:25: VIEW_ID_PROFILE_INDICATOR_ICON,
On 2016/05/17 18:13:08, Evan Stade wrote:
> On 2016/05/17 03:51:42, Peter Kasting wrote:
> > Nit: I'm not in love with "profile indicator icon" as it seems verbose and
> > redundant yet vague, but I don't have a great idea myself.  I wish we could
> just
> > call it "incognito icon" but the teleported window case throws wrenches at
> that.
> >  Maybe just "profile icon" is as descriptive as this (i.e. not very) but at
> > least less verbose?
> 
> I think profile icon and profile indicator icon are equally descriptive and
the
> former is more concise. But I prefer the latter because it's a more
distinctive
> name. The overuse and reuse of "avatar" was really confusing, hopefully using
> "profile indicator" everywhere will be make this hard-to-describe piece of UI
> more grep-able and more recognizable.

OK... I'm not strongly opposed, it just feels kind of like we're saying "hey,
like's call it the profile icon icon, because that's a more unique phrase than
profile icon".  It reads really oddly to me.

If you think it's better go with it.

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:173:
AvatarMenu::GetImageForMenuButton(profile->GetPath(), &icon, &unused);
On 2016/05/17 18:13:08, Evan Stade wrote:
> On 2016/05/17 03:51:42, Peter Kasting wrote:
> > Nit: Does this need to be conditionally compiled in only for ChromeOS?  At a
> > glance it looks like it would link without the ifdef, in which case, why not
> > just do that?
> 
> I wanted to add the NOTREACHED to document that this path shouldn't be hit
> except on cros (which is otherwise not obvious).

Can we just have a comment that says "only reached on CrOS"?  I'm mainly trying
to avoid the ifdef, but I also kind of have an allergy to "if (foo) { ... } else
NOTREACHED" because it's technically kind of a violation of the "don't handle
DCHECK failure" policy.

https://codereview.chromium.org/1972033002/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h (right):

https://codereview.chromium.org/1972033002/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h:117: void
LayoutProfileIndicator();
Nit: Should this be LayoutProfileIndicatorIcon()?

https://codereview.chromium.org/1972033002/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/taskbar_decorator_win.h (left):

https://codereview.chromium.org/1972033002/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/taskbar_decorator_win.h:20: 
Nit: If you're removing this blank line I'd remove the one above the function
comment too, or else keep this

[Evan Stade, 2016-05-20 19:58:01]

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

[commit-bot: I haz the power, 2016-05-20 19:58:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1972033002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1972033002/260001

[commit-bot: I haz the power, 2016-05-20 20:16:44]

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

[commit-bot: I haz the power, 2016-05-20 20:16:45]

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

[Evan Stade, 2016-05-20 22:04:26]

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

[commit-bot: I haz the power, 2016-05-20 22:04:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1972033002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1972033002/280001

[Evan Stade, 2016-05-20 22:04:43]

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:173:
AvatarMenu::GetImageForMenuButton(profile->GetPath(), &icon, &unused);
On 2016/05/18 02:44:01, Peter Kasting wrote:
> On 2016/05/17 18:13:08, Evan Stade wrote:
> > On 2016/05/17 03:51:42, Peter Kasting wrote:
> > > Nit: Does this need to be conditionally compiled in only for ChromeOS?  At
a
> > > glance it looks like it would link without the ifdef, in which case, why
not
> > > just do that?
> > 
> > I wanted to add the NOTREACHED to document that this path shouldn't be hit
> > except on cros (which is otherwise not obvious).
> 
> Can we just have a comment that says "only reached on CrOS"?  I'm mainly
trying
> to avoid the ifdef, but I also kind of have an allergy to "if (foo) { ... }
else
> NOTREACHED" because it's technically kind of a violation of the "don't handle
> DCHECK failure" policy.

I personally don't have a problem with ifdefs. Sure, nested and/or complicated
ifdefs get hard to read, but I don't think this qualifies. I would prefer to
enforce a comment via NOTREACHED than to leave it up to the reader to notice. I
could make it

  } else {
#if !defined(OS_CHROMEOS)
    notreached
#endif
    dostuff
  }

if that made you feel better about not handling dchecks, but it seems a little
bit like going out of our way to avoid handling the dcheck (if the ifdef's
already there, why not use it).

https://codereview.chromium.org/1972033002/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h (right):

https://codereview.chromium.org/1972033002/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h:117: void
LayoutProfileIndicator();
On 2016/05/18 02:44:01, Peter Kasting wrote:
> Nit: Should this be LayoutProfileIndicatorIcon()?

Done.

https://codereview.chromium.org/1972033002/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/taskbar_decorator_win.h (left):

https://codereview.chromium.org/1972033002/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/taskbar_decorator_win.h:20: 
On 2016/05/18 02:44:01, Peter Kasting wrote:
> Nit: If you're removing this blank line I'd remove the one above the function
> comment too, or else keep this

this is the output of git cl format (I agree it's odd)

[Peter Kasting, 2016-05-20 22:24:22]

Still LGTM

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/1972033002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:173:
AvatarMenu::GetImageForMenuButton(profile->GetPath(), &icon, &unused);
On 2016/05/20 22:04:42, Evan Stade wrote:
> On 2016/05/18 02:44:01, Peter Kasting wrote:
> > On 2016/05/17 18:13:08, Evan Stade wrote:
> > > On 2016/05/17 03:51:42, Peter Kasting wrote:
> > > > Nit: Does this need to be conditionally compiled in only for ChromeOS? 
At
> a
> > > > glance it looks like it would link without the ifdef, in which case, why
> not
> > > > just do that?
> > > 
> > > I wanted to add the NOTREACHED to document that this path shouldn't be hit
> > > except on cros (which is otherwise not obvious).
> > 
> > Can we just have a comment that says "only reached on CrOS"?  I'm mainly
> trying
> > to avoid the ifdef, but I also kind of have an allergy to "if (foo) { ... }
> else
> > NOTREACHED" because it's technically kind of a violation of the "don't
handle
> > DCHECK failure" policy.
> 
> I personally don't have a problem with ifdefs. Sure, nested and/or complicated
> ifdefs get hard to read, but I don't think this qualifies. I would prefer to
> enforce a comment via NOTREACHED than to leave it up to the reader to notice.
I
> could make it
> 
>   } else {
> #if !defined(OS_CHROMEOS)
>     notreached
> #endif
>     dostuff
>   }
> 
> if that made you feel better about not handling dchecks, but it seems a little
> bit like going out of our way to avoid handling the dcheck (if the ifdef's
> already there, why not use it).

How you have it is fine.

[Evan Stade, 2016-05-20 22:35:04]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2016-05-20 22:35:16]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-05-20 22:35:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/1972033002/#ps280001
(title: "reinstate ash browser test fix")

[commit-bot: I haz the power, 2016-05-20 22:36:04]

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

[commit-bot: I haz the power, 2016-05-20 22:44:59]

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

[commit-bot: I haz the power, 2016-05-20 22:45:00]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Evan Stade, 2016-05-23 18:48:11]

Description was changed from

==========
Simplify some old avatar menu button code.

The view is still used for:
- the incognito icon
- the CrOS multi-profile teleported window case. To see this, sign into
  a second user, open a window, and click "send to XXX's desktop".

It no longer needs to have a menu attached ever.

BUG=534850,591586
==========

to

==========
Simplify some old avatar menu button code.

The view is still used for:
- the incognito icon
- the CrOS multi-profile teleported window case. To see this, sign into
  a second user, open a window, and click "send to XXX's desktop".

It no longer needs to have a menu attached ever.

BUG=534850,591586
TBR=achuith@chromium.org
==========

[Evan Stade, 2016-05-23 18:48:11]

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

[Evan Stade, 2016-05-23 18:48:45]

TBR achuith for chrome/browser/chromeos/profiles/avatar_menu_chromeos.cc

[Evan Stade, 2016-05-23 18:48:53]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-05-23 18:49:24]

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

[commit-bot: I haz the power, 2016-05-23 20:03:52]

Description was changed from

==========
Simplify some old avatar menu button code.

The view is still used for:
- the incognito icon
- the CrOS multi-profile teleported window case. To see this, sign into
  a second user, open a window, and click "send to XXX's desktop".

It no longer needs to have a menu attached ever.

BUG=534850,591586
TBR=achuith@chromium.org
==========

to

==========
Simplify some old avatar menu button code.

The view is still used for:
- the incognito icon
- the CrOS multi-profile teleported window case. To see this, sign into
  a second user, open a window, and click "send to XXX's desktop".

It no longer needs to have a menu attached ever.

BUG=534850,591586
TBR=achuith@chromium.org
==========

[commit-bot: I haz the power, 2016-05-23 20:03:53]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-05-23 20:10:05]

Description was changed from

==========
Simplify some old avatar menu button code.

The view is still used for:
- the incognito icon
- the CrOS multi-profile teleported window case. To see this, sign into
  a second user, open a window, and click "send to XXX's desktop".

It no longer needs to have a menu attached ever.

BUG=534850,591586
TBR=achuith@chromium.org
==========

to

==========
Simplify some old avatar menu button code.

The view is still used for:
- the incognito icon
- the CrOS multi-profile teleported window case. To see this, sign into
  a second user, open a window, and click "send to XXX's desktop".

It no longer needs to have a menu attached ever.

BUG=534850,591586
TBR=achuith@chromium.org

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

[commit-bot: I haz the power, 2016-05-23 20:10:07]

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