Multi-Profiles: Change avatar menu to bubble view

Multi-Profiles: Change avatar menu to bubble view

BUG=
TEST=Ran on views and verified that things looked ok.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96306

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96311

[sail, 2011-08-05 00:10:04]

erg: profiles OWNER
sky/pkasting: ui/views OWNER
rsesek: everything else

[Elliot Glaysher, 2011-08-05 00:14:31]

LGTM

http://codereview.chromium.org/7566050/diff/1/chrome/browser/profiles/avatar_...
File chrome/browser/profiles/avatar_menu_model.cc (right):

http://codereview.chromium.org/7566050/diff/1/chrome/browser/profiles/avatar_...
chrome/browser/profiles/avatar_menu_model.cc:73: void
AvatarMenuModel::SwitchToProfile(size_t index) {
Haha. I have to review a two line spelling fix.

[Robert Sesek, 2011-08-05 00:44:02]

http://codereview.chromium.org/7566050/diff/1/chrome/browser/profiles/avatar_...
File chrome/browser/profiles/avatar_menu_model.cc (right):

http://codereview.chromium.org/7566050/diff/1/chrome/browser/profiles/avatar_...
chrome/browser/profiles/avatar_menu_model.cc:73: void
AvatarMenuModel::SwitchToProfile(size_t index) {
On 2011/08/05 00:14:31, Elliot Glaysher wrote:
> Haha. I have to review a two line spelling fix.

Do'h.

http://codereview.chromium.org/7566050/diff/1/chrome/browser/ui/views/avatar_...
File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right):

http://codereview.chromium.org/7566050/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:22: const int ITEM_WIDTH =
210;
Shouldn't constants be in kCamelCase? That's what we do in Cocoa, anyways.

http://codereview.chromium.org/7566050/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:59: :
views::ImageButton(listener) {
nit: indent 4

http://codereview.chromium.org/7566050/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:100: SkBitmap check_icon =
*rb.GetBitmapNamed(IDR_PROFILE_SELECTED);
Use GetImageNamed()

http://codereview.chromium.org/7566050/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:137: size_t GetProfileIndex()
{
profile_index()

http://codereview.chromium.org/7566050/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:153: size_t
active_profile_index = 0;
Move this into OnAvatarMenuModelChanged(). That will be called after the class
is instantiated.

http://codereview.chromium.org/7566050/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:170:
rb.GetBitmapNamed(IDR_PROFILE_EDIT));
GetImageNamed() here and throughout

http://codereview.chromium.org/7566050/diff/1/chrome/browser/ui/views/avatar_...
File chrome/browser/ui/views/avatar_menu_button.cc (right):

http://codereview.chromium.org/7566050/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.cc:74: AvatarMenuBubbleView*
bubble_view = new AvatarMenuBubbleView(browser_);
Comment about ownership?

[sail, 2011-08-05 01:28:21]

http://codereview.chromium.org/7566050/diff/1/chrome/browser/ui/views/avatar_...
File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right):

http://codereview.chromium.org/7566050/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:22: const int ITEM_WIDTH =
210;
On 2011/08/05 00:44:03, rsesek wrote:
> Shouldn't constants be in kCamelCase? That's what we do in Cocoa, anyways.

Done.

http://codereview.chromium.org/7566050/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:59: :
views::ImageButton(listener) {
On 2011/08/05 00:44:03, rsesek wrote:
> nit: indent 4

Done.

http://codereview.chromium.org/7566050/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:100: SkBitmap check_icon =
*rb.GetBitmapNamed(IDR_PROFILE_SELECTED);
On 2011/08/05 00:44:03, rsesek wrote:
> Use GetImageNamed()

Done.

http://codereview.chromium.org/7566050/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:137: size_t GetProfileIndex()
{
On 2011/08/05 00:44:03, rsesek wrote:
> profile_index()

Done.

http://codereview.chromium.org/7566050/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:153: size_t
active_profile_index = 0;
On 2011/08/05 00:44:03, rsesek wrote:
> Move this into OnAvatarMenuModelChanged(). That will be called after the class
> is instantiated.

Done.

http://codereview.chromium.org/7566050/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:153: size_t
active_profile_index = 0;
On 2011/08/05 00:44:03, rsesek wrote:
> Move this into OnAvatarMenuModelChanged(). That will be called after the class
> is instantiated.

Done.

http://codereview.chromium.org/7566050/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:170:
rb.GetBitmapNamed(IDR_PROFILE_EDIT));
On 2011/08/05 00:44:03, rsesek wrote:
> GetImageNamed() here and throughout

Done.

http://codereview.chromium.org/7566050/diff/1/chrome/browser/ui/views/avatar_...
File chrome/browser/ui/views/avatar_menu_button.cc (right):

http://codereview.chromium.org/7566050/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.cc:74: AvatarMenuBubbleView*
bubble_view = new AvatarMenuBubbleView(browser_);
On 2011/08/05 00:44:03, rsesek wrote:
> Comment about ownership?

Done.

[Robert Sesek, 2011-08-05 01:34:21]

http://codereview.chromium.org/7566050/diff/7001/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right):

http://codereview.chromium.org/7566050/diff/7001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:38: int scaled_width;
Two-space indents

http://codereview.chromium.org/7566050/diff/7001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:152:
OnAvatarMenuModelChanged();
The MenuModel should call this for you.

http://codereview.chromium.org/7566050/diff/7001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:209: // TODO(sail): This
function can get called before our menu model reference
Make the model pass itself as the arg to this function?

[Robert Sesek, 2011-08-05 01:41:21]

http://codereview.chromium.org/7566050/diff/7001/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right):

http://codereview.chromium.org/7566050/diff/7001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:209: // TODO(sail): This
function can get called before our menu model reference
On 2011/08/05 01:34:21, rsesek wrote:
> Make the model pass itself as the arg to this function?

Alternatively, make the model not call this in the ctor and have it only happen
when it observers the profile changed notification.

[sail, 2011-08-05 01:50:24]

http://codereview.chromium.org/7566050/diff/7001/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right):

http://codereview.chromium.org/7566050/diff/7001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:38: int scaled_width;
On 2011/08/05 01:34:21, rsesek wrote:
> Two-space indents

Done.

http://codereview.chromium.org/7566050/diff/7001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:152:
OnAvatarMenuModelChanged();
On 2011/08/05 01:34:21, rsesek wrote:
> The MenuModel should call this for you.

Done.

http://codereview.chromium.org/7566050/diff/7001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:209: // TODO(sail): This
function can get called before our menu model reference
On 2011/08/05 01:34:21, rsesek wrote:
> Make the model pass itself as the arg to this function?

Done.

[Robert Sesek, 2011-08-05 01:51:30]

LGTM. I'm not a views expert, though.

[sail, 2011-08-05 01:54:32]

On 2011/08/05 01:51:30, rsesek wrote:
> LGTM. I'm not a views expert, though.

Thanks for the quick review! I'll ping sky and pkasting to review the views
code.

[Peter Kasting, 2011-08-05 18:08:33]

http://codereview.chromium.org/7566050/diff/5005/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right):

http://codereview.chromium.org/7566050/diff/5005/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:56: class AddUserButton :
public views::ImageButton {
You should use a Link object for this, it will do what you want.

http://codereview.chromium.org/7566050/diff/5005/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:121: else
Nit: No else after return

http://codereview.chromium.org/7566050/diff/5005/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:158: return
gfx::Size(kItemWidth, (kItemHeight + kItemMarginY) *
How come we hardcode the size and never check the size of the profile name
string?

It seems like instead we should be summing up the max name size + icon widths +
padding and using that as a size, possibly with a minimum and/or maximum clamp.

http://codereview.chromium.org/7566050/diff/5005/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:162: void
AvatarMenuBubbleView::Layout() {
I think you can get rid of most of this code if you use a layout manager like
GridLayout.

http://codereview.chromium.org/7566050/diff/5005/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu_bubble_view.h (right):

http://codereview.chromium.org/7566050/diff/5005/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.h:33: // views::ButtonListener
implementation.
Nit: List these groups in the same order as you have them above.

[sky, 2011-08-08 15:39:15]

Since Peter is reviewing this I'm removing myself.

  -Scott

[sail, 2011-08-09 00:47:47]

http://codereview.chromium.org/7566050/diff/5005/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right):

http://codereview.chromium.org/7566050/diff/5005/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:56: class AddUserButton :
public views::ImageButton {
On 2011/08/05 18:08:33, Peter Kasting wrote:
> You should use a Link object for this, it will do what you want.

Done.

http://codereview.chromium.org/7566050/diff/5005/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:121: else
On 2011/08/05 18:08:33, Peter Kasting wrote:
> Nit: No else after return

Done.

http://codereview.chromium.org/7566050/diff/5005/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:158: return
gfx::Size(kItemWidth, (kItemHeight + kItemMarginY) *
On 2011/08/05 18:08:33, Peter Kasting wrote:
> How come we hardcode the size and never check the size of the profile name
> string?
> 
> It seems like instead we should be summing up the max name size + icon widths
+
> padding and using that as a size, possibly with a minimum and/or maximum
clamp.

Done.

http://codereview.chromium.org/7566050/diff/5005/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:162: void
AvatarMenuBubbleView::Layout() {
On 2011/08/05 18:08:33, Peter Kasting wrote:
> I think you can get rid of most of this code if you use a layout manager like
> GridLayout.

I spent half a day on this and I couldn't get a correct layout with a layout
manager. Doing the layout by hand ended up being much simpler code and a better
looking layout.

http://codereview.chromium.org/7566050/diff/5005/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu_bubble_view.h (right):

http://codereview.chromium.org/7566050/diff/5005/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.h:33: // views::ButtonListener
implementation.
On 2011/08/05 18:08:33, Peter Kasting wrote:
> Nit: List these groups in the same order as you have them above.

Done.

[Peter Kasting, 2011-08-09 21:49:05]

LGTM except for one pretty important issue

http://codereview.chromium.org/7566050/diff/12002/chrome/browser/ui/views/ava...
File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right):

http://codereview.chromium.org/7566050/diff/12002/chrome/browser/ui/views/ava...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:108: return normal ?
SkColorSetRGB(30, 30, 30) : SkColorSetRGB(0, 0, 0);
All uses of colors in this file seem to assume a white background.  This isn't
guaranteed.  You should take a look at how the star bubble switches between
foreground colors for different items based on what contrasts with the
background color, so that users of e.g. high-contrast white-on-black mode can
still read everything.

http://codereview.chromium.org/7566050/diff/12002/chrome/browser/ui/views/ava...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:250:
views::CustomButton::BS_PUSHED,
Nit: This can go on the previous line

[sail, 2011-08-09 23:10:51]

http://codereview.chromium.org/7566050/diff/12002/chrome/browser/ui/views/ava...
File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right):

http://codereview.chromium.org/7566050/diff/12002/chrome/browser/ui/views/ava...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:108: return normal ?
SkColorSetRGB(30, 30, 30) : SkColorSetRGB(0, 0, 0);
On 2011/08/09 21:49:05, Peter Kasting wrote:
> All uses of colors in this file seem to assume a white background.  This isn't
> guaranteed.  You should take a look at how the star bubble switches between
> foreground colors for different items based on what contrasts with the
> background color, so that users of e.g. high-contrast white-on-black mode can
> still read everything.

Ahh good point, if you don't mind I'll fix this in a second CL.

http://codereview.chromium.org/7566050/diff/12002/chrome/browser/ui/views/ava...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:250:
views::CustomButton::BS_PUSHED,
On 2011/08/09 21:49:05, Peter Kasting wrote:
> Nit: This can go on the previous line

Done.

[Peter Kasting, 2011-08-09 23:14:07]

On 2011/08/09 23:10:51, sail wrote:
> Ahh good point, if you don't mind I'll fix this in a second CL.

That's fine.