Redesign of the avatar menu button.

Redesign of the avatar menu button.

Screenshots: https://drive.google.com/folderview?id=0B1B1Up4p2NRMYm8zRzFhQkpCTUE&usp=sharing
This is behind the --new-profile-management flag, and shows the avatar
switcher as a menu button in the caption area. The button shows the user's
name, rather than an image.

There will be an upcoming CL to ensure that the correct button is displayed for Metro (Win8) browsers (this CL would display the Aero button for Win8 as well).

BUG=287883
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228293

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229082

[noms (inactive), 2013-09-26 14:42:57]

Hi Scott,

Here's a first try at moving the avatar menu button from the status quo to a
button in the caption area. Please take a look.

[sky, 2013-09-26 20:21:39]

+erg for opaque_browser_frame_view_layout.h

And how about a test in opaque_browser_frame_view_unittest?

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:50:
UpdateNewStyleAvatarInfo();
Why do we need this here and not at init time?

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:155: bool
is_incognito = browser_view->IsOffTheRecord() &&
You seem to have this check in a couple of places, and it isn't really
incognito. Is there some better term to use here? And can a method be used for
this rather than same code here, 434 and other files?

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/ne...
File chrome/browser/ui/views/new_avatar_button.cc (right):

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/ne...
chrome/browser/ui/views/new_avatar_button.cc:8: #include
"chrome/browser/profiles/profile_metrics.h"
I think this code would be cleaner if you keep the dependencies on chrome out of
this class. Instead have another class that knows about NewAvatarButton and
other chrome things. NewAvatarButton notifies said class by way of
ButtonListener. That way this code just worries about painting and your other
class does the binding.

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/ne...
chrome/browser/ui/views/new_avatar_button.cc:81: 
nit: remove newline.

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/ne...
File chrome/browser/ui/views/new_avatar_button.h (right):

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/ne...
chrome/browser/ui/views/new_avatar_button.h:31: void ShowAvatarBubble();
Description?

https://codereview.chromium.org/24647003/diff/6001/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/24647003/diff/6001/chrome/chrome_browser_ui.g...
chrome/chrome_browser_ui.gypi:1592: 'browser/ui/views/new_avatar_button.cc',
sort.

https://codereview.chromium.org/24647003/diff/6001/ui/views/controls/button/m...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/24647003/diff/6001/ui/views/controls/button/m...
ui/views/controls/button/menu_button.cc:137: gfx::Insets insets = GetInsets();
const

https://codereview.chromium.org/24647003/diff/6001/ui/views/controls/button/m...
ui/views/controls/button/menu_button.cc:143: gfx::Rect arrow_bounds(width() -
insets.right() -
const

https://codereview.chromium.org/24647003/diff/6001/ui/views/controls/button/m...
File ui/views/controls/button/menu_button.h (right):

https://codereview.chromium.org/24647003/diff/6001/ui/views/controls/button/m...
ui/views/controls/button/menu_button.h:55: void PaintMenuMarker(gfx::Canvas*
canvas);
Why is this public?

[Elliot Glaysher, 2013-09-26 21:05:43]

Tests?

Looking this over, I suspect this is all badly busted on Linux.

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc (right):

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:140:
available_width -= new_avatar_button_->width();
This shouldn't be calculated here; rather, it should indirectly be calculated by
working with either the existing minimum size calculations, or the existing
leading/trailing calculations.

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:367: void
OpaqueBrowserFrameViewLayout::LayoutNewStyleAvatar() {
Since the new avatar menu looks like a caption button, please plumb
"ButtonAlignment alignment" here and act on it.

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:374: caption_y
+ kCaptionButtonHeightWithPadding);
This position calculation is wrong. You can't position things in terms of the
minimize button because it's not always on the right side of the screen. (And
when it is, it's not guaranteed to be the leftmost button on the right side.
"close:maximize,minimize" is valid button layout here.)

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:393:
avatar_button_->SetBoundsRect(avatar_bounds_);
If you're pulling out the code block after this (not adjusting
leading_button_start_ or minimum_size_for_buttons_), the incognito avatar
probably doesn't take up any space in the tabstrip; I suspect the tabs can be
placed on top of the avatar.

[noms (inactive), 2013-10-01 17:42:21]

I think I have addressed all but one of the review comments. I've added a test
and hopefully fixed all the opaque window layout sins I made -- they were many
:(

Please take a look! Thanks.

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:50:
UpdateNewStyleAvatarInfo();
I don't fully understand the comment about DrawTaskBasDecoration, but I've
changed it to have the same functionality as before -- only get called if the
old avatar icon should be displayed. 

On 2013/09/26 20:21:39, sky wrote:
> Why do we need this here and not at init time?

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:155: bool
is_incognito = browser_view->IsOffTheRecord() &&
On 2013/09/26 20:21:39, sky wrote:
> You seem to have this check in a couple of places, and it isn't really
> incognito. Is there some better term to use here? And can a method be used for
> this rather than same code here, 434 and other files?

Done.

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc (right):

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:140:
available_width -= new_avatar_button_->width();
On 2013/09/26 21:05:44, Elliot Glaysher wrote:
> This shouldn't be calculated here; rather, it should indirectly be calculated
by
> working with either the existing minimum size calculations, or the existing
> leading/trailing calculations.

Done.

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:367: void
OpaqueBrowserFrameViewLayout::LayoutNewStyleAvatar() {
I've used the has_trailing_buttons_ boolean that gets set after
LayoutWindowControls runs. I don't know what should happen if both that and
has_leading_buttons_ are set to true -- could this realistically ever happen?

On 2013/09/26 21:05:44, Elliot Glaysher wrote:
> Since the new avatar menu looks like a caption button, please plumb
> "ButtonAlignment alignment" here and act on it.

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:374: caption_y
+ kCaptionButtonHeightWithPadding);
On 2013/09/26 21:05:44, Elliot Glaysher wrote:
> This position calculation is wrong. You can't position things in terms of the
> minimize button because it's not always on the right side of the screen. (And
> when it is, it's not guaranteed to be the leftmost button on the right side.
> "close:maximize,minimize" is valid button layout here.)

Done.

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:393:
avatar_button_->SetBoundsRect(avatar_bounds_);
I don't actually know what I was thinking when I split this code up. :| 
Reverted.
On 2013/09/26 21:05:44, Elliot Glaysher wrote:
> If you're pulling out the code block after this (not adjusting
> leading_button_start_ or minimum_size_for_buttons_), the incognito avatar
> probably doesn't take up any space in the tabstrip; I suspect the tabs can be
> placed on top of the avatar.

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/ne...
File chrome/browser/ui/views/new_avatar_button.cc (right):

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/ne...
chrome/browser/ui/views/new_avatar_button.cc:81: 
On 2013/09/26 20:21:39, sky wrote:
> nit: remove newline.

Done.

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/ne...
File chrome/browser/ui/views/new_avatar_button.h (right):

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/ne...
chrome/browser/ui/views/new_avatar_button.h:31: void ShowAvatarBubble();
On 2013/09/26 20:21:39, sky wrote:
> Description?

Done.

https://codereview.chromium.org/24647003/diff/6001/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/24647003/diff/6001/chrome/chrome_browser_ui.g...
chrome/chrome_browser_ui.gypi:1592: 'browser/ui/views/new_avatar_button.cc',
On 2013/09/26 20:21:39, sky wrote:
> sort.

Done.

https://codereview.chromium.org/24647003/diff/6001/ui/views/controls/button/m...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/24647003/diff/6001/ui/views/controls/button/m...
ui/views/controls/button/menu_button.cc:137: gfx::Insets insets = GetInsets();
On 2013/09/26 20:21:39, sky wrote:
> const

Done.

https://codereview.chromium.org/24647003/diff/6001/ui/views/controls/button/m...
ui/views/controls/button/menu_button.cc:143: gfx::Rect arrow_bounds(width() -
insets.right() -
I don't think arrow_bounds can be const, as it gets updated on line 148.
On 2013/09/26 20:21:39, sky wrote:
> const

https://codereview.chromium.org/24647003/diff/6001/ui/views/controls/button/m...
File ui/views/controls/button/menu_button.h (right):

https://codereview.chromium.org/24647003/diff/6001/ui/views/controls/button/m...
ui/views/controls/button/menu_button.h:55: void PaintMenuMarker(gfx::Canvas*
canvas);
I changed it to protected.
 
On 2013/09/26 20:21:39, sky wrote:
> Why is this public?

[Elliot Glaysher, 2013-10-01 18:12:56]

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc (right):

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:367: void
OpaqueBrowserFrameViewLayout::LayoutNewStyleAvatar() {
On 2013/10/01 17:42:21, Monica Dinculescu wrote:
> I've used the has_trailing_buttons_ boolean that gets set after
> LayoutWindowControls runs. I don't know what should happen if both that and
> has_leading_buttons_ are set to true -- could this realistically ever happen?

Yeap! "close:minimize" is the default button layout on some window managers (no
maximize button!) and we deal with that in the gtk port today. (Also, since
button layout is under user control, we have to handle it anyway...)

In such cases, the GTK port defaults to putting the avatar selector on whichever
side has the least buttons, biased to the right in case of tie, which is the
policy I'd prefer to see going forward. (Currently, the views code here just
*always* puts things to the left, but that's only because I'm swamped and have
much higher priority things to do right now to get linux_aura out the door.)

https://codereview.chromium.org/24647003/diff/50001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc (right):

https://codereview.chromium.org/24647003/diff/50001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:54: // How far
the new avatar button is from the left of the minimize button.
Change comment: how far it is from the closest caption button

[noms (inactive), 2013-10-03 19:30:14]

Scott: ping!

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc (right):

https://codereview.chromium.org/24647003/diff/6001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:367: void
OpaqueBrowserFrameViewLayout::LayoutNewStyleAvatar() {
After offline conversation, I've made it so that the button always gets added to
the trailing buttons. For now this is only supported on Windows, where the
button should go to the right, with the trailing buttons. On Linux/Mac we don't
have UI specs yet, so it's unclear where the button _should_ go, but it's likely
it will go as a trailing button as not to clutter the area before the first tab
too much. 

This will get revisited as soon as there are Linux UI specs, I promise :)

On 2013/10/01 18:12:56, Elliot Glaysher wrote:
> On 2013/10/01 17:42:21, Monica Dinculescu wrote:
> > I've used the has_trailing_buttons_ boolean that gets set after
> > LayoutWindowControls runs. I don't know what should happen if both that and
> > has_leading_buttons_ are set to true -- could this realistically ever
happen?
> 
> Yeap! "close:minimize" is the default button layout on some window managers
(no
> maximize button!) and we deal with that in the gtk port today. (Also, since
> button layout is under user control, we have to handle it anyway...)
> 
> In such cases, the GTK port defaults to putting the avatar selector on
whichever
> side has the least buttons, biased to the right in case of tie, which is the
> policy I'd prefer to see going forward. (Currently, the views code here just
> *always* puts things to the left, but that's only because I'm swamped and have
> much higher priority things to do right now to get linux_aura out the door.)

https://codereview.chromium.org/24647003/diff/50001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc (right):

https://codereview.chromium.org/24647003/diff/50001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:54: // How far
the new avatar button is from the left of the minimize button.
On 2013/10/01 18:12:56, Elliot Glaysher wrote:
> Change comment: how far it is from the closest caption button

Done.

[Elliot Glaysher, 2013-10-03 21:52:52]

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc
(right):

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc:333:
switches::kNewProfileManagement);
Isn't this permanently changing the command line? If not, where does this get
reset? base::TestSuite::~TestSuite() runs CommandLine::Reset(), but that looks
like its once at shutdown, not per test...

[sky, 2013-10-03 23:23:11]

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:129: if
(browser_view_->IsGuestSession()) {
Can this making of profile to name be moved else where? Presumably it'll be
useful on other platforms.

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:145:
RemoveChildView(new_avatar_button_);
Not needed, you can delete.

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.h (right):

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_non_client_frame_view.h:38:
NewAvatarButton* new_avatar_button() const { return new_avatar_button_; }
This is a bit pattern. Return value here should match const, or don't make it
const. Eg one (or both) of:

const NewAvatarButton* new_avatar_button() const
NewAvatarButton* new_avatar_button()

Otherwise you can easily circumvent constness.

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_non_client_frame_view.h:67: // Updates the
title of the avatar button displayed in the caption area.
nit: newline betwten 66/67.

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_view.h (right):

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view.h:197: bool IsGuestSession() const;
I believe you can nuke this once you address my other comment.

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.h (right):

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.h:20: 
remove newline.

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/n...
File chrome/browser/ui/views/new_avatar_button.cc (right):

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/n...
chrome/browser/ui/views/new_avatar_button.cc:24: const int kInset = 10;
Description?

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/n...
chrome/browser/ui/views/new_avatar_button.cc:86: if
(browser_view->frame()->ShouldUseNativeFrame()) {
I think this code is cleaner if you remove the browserview dependency. The
places that create this can configure it.

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/n...
chrome/browser/ui/views/new_avatar_button.cc:130: ShowAvatarBubble();
How come you're showing on a press and not release?

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/n...
chrome/browser/ui/views/new_avatar_button.cc:134: void
NewAvatarButton::ShowAvatarBubble() {
Can this be done via a listener and not here?

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/n...
File chrome/browser/ui/views/new_avatar_menu_button_browsertest.cc (right):

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/n...
chrome/browser/ui/views/new_avatar_menu_button_browsertest.cc:92:
StartAvatarMenu();
Any time you call a function that potentially asserts you need to wrap it in
ASSERT_NO_FATAL_FAILURE.

https://codereview.chromium.org/24647003/diff/81001/ui/views/controls/button/...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/24647003/diff/81001/ui/views/controls/button/...
ui/views/controls/button/menu_button.cc:136: void
MenuButton::PaintMenuMarker(gfx::Canvas* canvas) {
In general smaller patches are preferable. They make everything go smoother. Can
you split the menu_button change into its own patch?

Also, make the position match the header.

[noms (inactive), 2013-10-07 21:18:15]

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.cc (right):

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:129: if
(browser_view_->IsGuestSession()) {
On 2013/10/03 23:23:11, sky wrote:
> Can this making of profile to name be moved else where? Presumably it'll be
> useful on other platforms.

Done.

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_non_client_frame_view.cc:145:
RemoveChildView(new_avatar_button_);
On 2013/10/03 23:23:11, sky wrote:
> Not needed, you can delete.

Done.

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.h (right):

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_non_client_frame_view.h:38:
NewAvatarButton* new_avatar_button() const { return new_avatar_button_; }
Eek, I'm not sure I can. This seems to be a pattern used everywhere in this
class. If I make it return a const NewAvatarButton*, then
GlassBrowserFrameView::LayoutNewStyleAvatar is grumpy because it needs to call
SetBounds on the button.

If I drop the const on the function, then all the const functions in
GlassBrowserFrameView calling new_avatar_button() don't like it because it's not
const. :(

I think that fixing this instance might require a lot of adding consts or
dropping consts to GlassBrowserFrame. Should I make a bug out of it?

On 2013/10/03 23:23:11, sky wrote:
> This is a bit pattern. Return value here should match const, or don't make it
> const. Eg one (or both) of:
> 
> const NewAvatarButton* new_avatar_button() const
> NewAvatarButton* new_avatar_button()
> 
> Otherwise you can easily circumvent constness.

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_non_client_frame_view.h:67: // Updates the
title of the avatar button displayed in the caption area.
On 2013/10/03 23:23:11, sky wrote:
> nit: newline betwten 66/67.

Done.

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_view.h (right):

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view.h:197: bool IsGuestSession() const;
It's still needed by the function that determines which avatar icon to show
(line 84). I tried pulling that code out into profiles_state.cc, where I moved
the code that determines the display name of the profile, but it would need to
include browser_view (for the OTRIconResouceID), which is not allowed from under
profiles.

On 2013/10/03 23:23:11, sky wrote:
> I believe you can nuke this once you address my other comment.

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.h (right):

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.h:20: 
On 2013/10/03 23:23:11, sky wrote:
> remove newline.

Done.

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc
(right):

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc:333:
switches::kNewProfileManagement);
The TestClientInitializer has two functions, OnTestStart and OnTestEnd which
backup and restore the command line for the current process. This means that
whatever command line changes you do in a test, they get reverted at the end of
the test.

On 2013/10/03 21:52:53, Elliot Glaysher wrote:
> Isn't this permanently changing the command line? If not, where does this get
> reset? base::TestSuite::~TestSuite() runs CommandLine::Reset(), but that looks
> like its once at shutdown, not per test...

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/n...
File chrome/browser/ui/views/new_avatar_button.cc (right):

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/n...
chrome/browser/ui/views/new_avatar_button.cc:24: const int kInset = 10;
On 2013/10/03 23:23:11, sky wrote:
> Description?

Done.

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/n...
chrome/browser/ui/views/new_avatar_button.cc:130: ShowAvatarBubble();
On 2013/10/03 23:23:11, sky wrote:
> How come you're showing on a press and not release?

Done.

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/n...
chrome/browser/ui/views/new_avatar_button.cc:134: void
NewAvatarButton::ShowAvatarBubble() {
I'm not sure what you mean by this. I've moved the code directly in
OnMousePressed, but I'm fairly sure that's not what you meant.

On 2013/10/03 23:23:11, sky wrote:
> Can this be done via a listener and not here?

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/n...
File chrome/browser/ui/views/new_avatar_menu_button_browsertest.cc (right):

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/n...
chrome/browser/ui/views/new_avatar_menu_button_browsertest.cc:92:
StartAvatarMenu();
On 2013/10/03 23:23:11, sky wrote:
> Any time you call a function that potentially asserts you need to wrap it in
> ASSERT_NO_FATAL_FAILURE.

Done.

https://codereview.chromium.org/24647003/diff/81001/ui/views/controls/button/...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/24647003/diff/81001/ui/views/controls/button/...
ui/views/controls/button/menu_button.cc:136: void
MenuButton::PaintMenuMarker(gfx::Canvas* canvas) {
On 2013/10/03 23:23:11, sky wrote:
> In general smaller patches are preferable. They make everything go smoother.
Can
> you split the menu_button change into its own patch?
> 
> Also, make the position match the header.

Done.

[sky, 2013-10-08 01:56:04]

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.h (right):

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_non_client_frame_view.h:38:
NewAvatarButton* new_avatar_button() const { return new_avatar_button_; }
On 2013/10/07 21:18:15, Monica Dinculescu wrote:
> Eek, I'm not sure I can. This seems to be a pattern used everywhere in this
> class. If I make it return a const NewAvatarButton*, then
> GlassBrowserFrameView::LayoutNewStyleAvatar is grumpy because it needs to call
> SetBounds on the button.
> 
> If I drop the const on the function, then all the const functions in
> GlassBrowserFrameView calling new_avatar_button() don't like it because it's
not
> const. :(
> 
> I think that fixing this instance might require a lot of adding consts or
> dropping consts to GlassBrowserFrame. Should I make a bug out of it?
> 
> On 2013/10/03 23:23:11, sky wrote:
> > This is a bit pattern. Return value here should match const, or don't make
it
> > const. Eg one (or both) of:
> > 
> > const NewAvatarButton* new_avatar_button() const
> > NewAvatarButton* new_avatar_button()
> > 
> > Otherwise you can easily circumvent constness.
> 

Yes, make a bug for it.

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/n...
File chrome/browser/ui/views/new_avatar_button.cc (right):

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/n...
chrome/browser/ui/views/new_avatar_button.cc:134: void
NewAvatarButton::ShowAvatarBubble() {
On 2013/10/07 21:18:15, Monica Dinculescu wrote:
> I'm not sure what you mean by this. I've moved the code directly in
> OnMousePressed, but I'm fairly sure that's not what you meant.
> 
> On 2013/10/03 23:23:11, sky wrote:
> > Can this be done via a listener and not here?
> 

I'm suggesting you make someone implement ButtonListener and not bake this
implementation in here. Less depdendencies.

[noms (inactive), 2013-10-08 18:21:14]

Addressed all the comments. Please take a look.

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.h (right):

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_non_client_frame_view.h:38:
NewAvatarButton* new_avatar_button() const { return new_avatar_button_; }
On 2013/10/08 01:56:04, sky wrote:
> On 2013/10/07 21:18:15, Monica Dinculescu wrote:
> > Eek, I'm not sure I can. This seems to be a pattern used everywhere in this
> > class. If I make it return a const NewAvatarButton*, then
> > GlassBrowserFrameView::LayoutNewStyleAvatar is grumpy because it needs to
call
> > SetBounds on the button.
> > 
> > If I drop the const on the function, then all the const functions in
> > GlassBrowserFrameView calling new_avatar_button() don't like it because it's
> not
> > const. :(
> > 
> > I think that fixing this instance might require a lot of adding consts or
> > dropping consts to GlassBrowserFrame. Should I make a bug out of it?
> > 
> > On 2013/10/03 23:23:11, sky wrote:
> > > This is a bit pattern. Return value here should match const, or don't make
> it
> > > const. Eg one (or both) of:
> > > 
> > > const NewAvatarButton* new_avatar_button() const
> > > NewAvatarButton* new_avatar_button()
> > > 
> > > Otherwise you can easily circumvent constness.
> > 
> 
> Yes, make a bug for it.

Done.

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/n...
File chrome/browser/ui/views/new_avatar_button.cc (right):

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/n...
chrome/browser/ui/views/new_avatar_button.cc:86: if
(browser_view->frame()->ShouldUseNativeFrame()) {
On 2013/10/03 23:23:11, sky wrote:
> I think this code is cleaner if you remove the browserview dependency. The
> places that create this can configure it.

Done.

https://codereview.chromium.org/24647003/diff/81001/chrome/browser/ui/views/n...
chrome/browser/ui/views/new_avatar_button.cc:134: void
NewAvatarButton::ShowAvatarBubble() {
Sorry, I had a completely half-a-brain moment there. Done. The glass/opaque
browser windows are now the listeners.

On 2013/10/08 01:56:04, sky wrote:
> On 2013/10/07 21:18:15, Monica Dinculescu wrote:
> > I'm not sure what you mean by this. I've moved the code directly in
> > OnMousePressed, but I'm fairly sure that's not what you meant.
> > 
> > On 2013/10/03 23:23:11, sky wrote:
> > > Can this be done via a listener and not here?
> > 
> 
> I'm suggesting you make someone implement ButtonListener and not bake this
> implementation in here. Less depdendencies.

[sky, 2013-10-08 21:29:25]

https://codereview.chromium.org/24647003/diff/150001/chrome/browser/ui/views/...
File chrome/browser/ui/views/new_avatar_button.cc (right):

https://codereview.chromium.org/24647003/diff/150001/chrome/browser/ui/views/...
chrome/browser/ui/views/new_avatar_button.cc:23: class AvatarGlassButtonBorder :
public views::TextButtonDefaultBorder {
Can you make the set_**painter buttons public in TextButtonBorder and not
subclass?

https://codereview.chromium.org/24647003/diff/150001/chrome/browser/ui/views/...
chrome/browser/ui/views/new_avatar_button.cc:34: };
nit: no ; here, 38, 57 and 61.

https://codereview.chromium.org/24647003/diff/150001/chrome/browser/ui/views/...
chrome/browser/ui/views/new_avatar_button.cc:74: : MenuButton(listener,
profile_name, NULL, true) {
nit: indent 2 more.

[noms (inactive), 2013-10-09 15:12:46]

https://codereview.chromium.org/24647003/diff/150001/chrome/browser/ui/views/...
File chrome/browser/ui/views/new_avatar_button.cc (right):

https://codereview.chromium.org/24647003/diff/150001/chrome/browser/ui/views/...
chrome/browser/ui/views/new_avatar_button.cc:23: class AvatarGlassButtonBorder :
public views::TextButtonDefaultBorder {
Do you mean move them from TextButtonDefaultBorder to TextButtonBorder? Then I'm
not sure we'd need both classes (there would be nothing left in
TextButtonDefaultBorder)

On 2013/10/08 21:29:25, sky wrote:
> Can you make the set_**painter buttons public in TextButtonBorder and not
> subclass?

https://codereview.chromium.org/24647003/diff/150001/chrome/browser/ui/views/...
chrome/browser/ui/views/new_avatar_button.cc:34: };
On 2013/10/08 21:29:25, sky wrote:
> nit: no ; here, 38, 57 and 61.

Done.

https://codereview.chromium.org/24647003/diff/150001/chrome/browser/ui/views/...
chrome/browser/ui/views/new_avatar_button.cc:74: : MenuButton(listener,
profile_name, NULL, true) {
On 2013/10/08 21:29:25, sky wrote:
> nit: indent 2 more.

Done.

[Elliot Glaysher, 2013-10-09 17:43:50]

frame layout lgtm

https://codereview.chromium.org/24647003/diff/158001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc (right):

https://codereview.chromium.org/24647003/diff/158001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:371: button_x
= host->width() - trailing_button_start_ - button_size_with_offset;
move this so that it is int buton_x = host->width() ...

[noms (inactive), 2013-10-09 17:50:47]

Elliot, would you mind looking over chrome/browser/profiles/* for an OWNERS
stamp as well, please? Thanks!

On 2013/10/09 17:43:50, Elliot Glaysher wrote:
> frame layout lgtm
> 
>
https://codereview.chromium.org/24647003/diff/158001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc
(right):
> 
>
https://codereview.chromium.org/24647003/diff/158001/chrome/browser/ui/views/...
> chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:371:
button_x
> = host->width() - trailing_button_start_ - button_size_with_offset;
> move this so that it is int buton_x = host->width() ...

[Elliot Glaysher, 2013-10-09 17:56:33]

lgtm

https://codereview.chromium.org/24647003/diff/158001/chrome/browser/profiles/...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/24647003/diff/158001/chrome/browser/profiles/...
chrome/browser/profiles/profiles_state.cc:72: size_t index =
cache.GetIndexOfProfileWithPath(profile->GetPath());
nit: this api feels weird. If this method can presumably fail, why doesn't it
return bool and pass index back as a parameter? Why std::string::npos in a
method that doesn't actual accept or vend strings?

This isn't a comment about your code; I checked and you didn't make this API.
Likewise, there are way too many callsites for me to just ask you to fix this
(used maybe 70 times in chrome). Just wanted to point this out and maybe plant
an idea in your mind.

[noms (inactive), 2013-10-09 18:17:51]

+ oshima for OWNERS stamp on chrome/app/theme/theme_resources.grd

https://codereview.chromium.org/24647003/diff/158001/chrome/browser/profiles/...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/24647003/diff/158001/chrome/browser/profiles/...
chrome/browser/profiles/profiles_state.cc:72: size_t index =
cache.GetIndexOfProfileWithPath(profile->GetPath());
Agreed. I've opened crbug.com/305720 for this.

On 2013/10/09 17:56:34, Elliot Glaysher wrote:
> nit: this api feels weird. If this method can presumably fail, why doesn't it
> return bool and pass index back as a parameter? Why std::string::npos in a
> method that doesn't actual accept or vend strings?
> 
> This isn't a comment about your code; I checked and you didn't make this API.
> Likewise, there are way too many callsites for me to just ask you to fix this
> (used maybe 70 times in chrome). Just wanted to point this out and maybe plant
> an idea in your mind.

https://codereview.chromium.org/24647003/diff/158001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc (right):

https://codereview.chromium.org/24647003/diff/158001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:371: button_x
= host->width() - trailing_button_start_ - button_size_with_offset;
On 2013/10/09 17:43:50, Elliot Glaysher wrote:
> move this so that it is int buton_x = host->width() ... 

Done.

[oshima, 2013-10-09 18:38:59]

chrome/app/theme lgtm

[sky, 2013-10-09 20:22:48]

https://codereview.chromium.org/24647003/diff/150001/chrome/browser/ui/views/...
File chrome/browser/ui/views/new_avatar_button.cc (right):

https://codereview.chromium.org/24647003/diff/150001/chrome/browser/ui/views/...
chrome/browser/ui/views/new_avatar_button.cc:23: class AvatarGlassButtonBorder :
public views::TextButtonDefaultBorder {
I mean rather than subclassing create a TextButtonDefaultBorder and invoke
set_XXX_painter.

On 2013/10/09 15:12:47, Monica Dinculescu wrote:
> Do you mean move them from TextButtonDefaultBorder to TextButtonBorder? Then
I'm
> not sure we'd need both classes (there would be nothing left in
> TextButtonDefaultBorder)
> 
> On 2013/10/08 21:29:25, sky wrote:
> > Can you make the set_**painter buttons public in TextButtonBorder and not
> > subclass?
>

[noms (inactive), 2013-10-09 21:26:31]

https://codereview.chromium.org/24647003/diff/150001/chrome/browser/ui/views/...
File chrome/browser/ui/views/new_avatar_button.cc (right):

https://codereview.chromium.org/24647003/diff/150001/chrome/browser/ui/views/...
chrome/browser/ui/views/new_avatar_button.cc:23: class AvatarGlassButtonBorder :
public views::TextButtonDefaultBorder {
Oh that is way nicer. Thanks! Done.

On 2013/10/09 20:22:49, sky wrote:
> I mean rather than subclassing create a TextButtonDefaultBorder and invoke
> set_XXX_painter.
> 
> On 2013/10/09 15:12:47, Monica Dinculescu wrote:
> > Do you mean move them from TextButtonDefaultBorder to TextButtonBorder? Then
> I'm
> > not sure we'd need both classes (there would be nothing left in
> > TextButtonDefaultBorder)
> > 
> > On 2013/10/08 21:29:25, sky wrote:
> > > Can you make the set_**painter buttons public in TextButtonBorder and not
> > > subclass?
> > 
>

[sky, 2013-10-09 21:56:06]

LGTM

[commit-bot: I haz the power, 2013-10-10 01:05:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/24647003/167030

[commit-bot: I haz the power, 2013-10-10 01:06:10]

Failed to request the patch to try. Please note that binary files are still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 500: 
<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered an error and could not complete your request.<p>If
the problem persists, please <A
HREF="http://code.google.com/appengine/community.html">report</A> your problem
and mention this error message and the query that caused it.</h2>
<h2></h2>
</body></html>

[commit-bot: I haz the power, 2013-10-10 01:29:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/24647003/167030

[commit-bot: I haz the power, 2013-10-10 09:56:47]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-10 13:33:14]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[commit-bot: I haz the power, 2013-10-10 13:34:23]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[commit-bot: I haz the power, 2013-10-11 13:36:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/24647003/197001

[commit-bot: I haz the power, 2013-10-11 16:36:23]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-10-11 18:06:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/24647003/203001

[commit-bot: I haz the power, 2013-10-11 18:17:06]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-10-11 20:26:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/24647003/203001

[commit-bot: I haz the power, 2013-10-12 00:46:16]

Change committed as 228293

[noms (inactive), 2013-10-16 17:02:24]

Hi Elliot or Scott,

I fixed the crash from crbug.com/307111, which happened because we were trying
to layout the avatar when we weren't supposed to (in the case of app or popup
windows). Is this still good to go?

Thanks!

[sky, 2013-10-16 20:03:54]

What should I diff between to see the changes?

[noms, 2013-10-16 20:08:05]

I would diff with 19, as that's where I rebased.
The tl;dr is that I added:
- an "if (browser_view()->ShouldShowAvatar())" check in
GlassBrowserFrameView::Layout() (so that we don't layout unless we're supposed
to)
- a null pointer check in GlassBrowserFrameView::LayoutNewStyleAvatar() to be
consistent with LayoutAvatar()

[Elliot Glaysher, 2013-10-16 20:11:03]

I will opine that you don't need my lgtm since you aren't touching the OBFVL.

[sky, 2013-10-16 22:03:48]

LGTM

[commit-bot: I haz the power, 2013-10-17 01:28:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/24647003/266001

[commit-bot: I haz the power, 2013-10-17 02:46:54]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-10-17 04:33:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/24647003/266001

[commit-bot: I haz the power, 2013-10-17 08:56:45]

Change committed as 229082