Change profile menu button to avatar button

Change profile menu button to avatar button

This change doesn't add any new functionality. It simply changes the profile menu button so that it now draws an avatar icon.

Once this is checked in next steps will be to:
- associate avatar icons to profiles
- expand the profile menu

BUG=None
TEST=Ran on Windows and verified that things look ok.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88683
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88854

[Miranda Callahan, 2011-06-09 13:23:07]

Just a couple small things...

http://codereview.chromium.org/7003080/diff/1/chrome/app/theme/theme_resource...
File chrome/app/theme/theme_resources.grd (right):

http://codereview.chromium.org/7003080/diff/1/chrome/app/theme/theme_resource...
chrome/app/theme/theme_resources.grd:216: <include name="IDR_PROFILE_AVATAR_1"
file="avatar_cupcake.png" type="BINDATA" />
I'm just going to say that I love that you made the first avatar a cupcake. :-)

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

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.cc:39: ((icon.height() -
icon.height()) + 1) / 2, w, h,
Looks like you are calculating (x - x) here?  (Also, you stored icon.height() in
|h| above, but chose to take icon.height() again?)

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/b...
File chrome/browser/ui/views/frame/browser_view.h (right):

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/b...
chrome/browser/ui/views/frame/browser_view.h:168: // Returns true if the
non-client view should render and avatar icon.
s/and/an

[sky, 2011-06-09 15:46:46]

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

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.h:28: AvatarMenuButton(const
std::wstring& text, ui::MenuModel* menu_model);
Document AvatarMenuButton takes ownership of the model.

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/b...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/b...
chrome/browser/ui/views/frame/browser_view.cc:469: if (!IsBrowserTypeNormal())
nit: use a single return, eg:

return IsBrowserTypeNormal() && (IsOffTheRecord() || blah blah blah

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/g...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/g...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:85: ui::MenuModel*
menu_model = browser_view_->IsOffTheRecord() ?
Dcoument that AvatarMenuButton takes ownership of the model.

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/g...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:202: // See if the
point is within the avatar menu button.
Move this above 200.

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/o...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/o...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:204:
avatar_button_.reset(new AvatarMenuButton(std::wstring(), menu_model));
Document AvatarMenuButton takes ownership of menu_model.

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/o...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:349: if
(avatar_button_.get() && avatar_button_->IsVisible() &&
Move above 346

http://codereview.chromium.org/7003080/diff/1/views/controls/button/text_butt...
File views/controls/button/text_button.h (right):

http://codereview.chromium.org/7003080/diff/1/views/controls/button/text_butt...
views/controls/button/text_button.h:346: virtual SkBitmap GetImageToPaint()
const;
Can this return const SkBitmap& ?

[Peter Kasting, 2011-06-09 18:03:31]

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

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.cc:26: if (base::i18n::IsRTL()) {
You should not need to do this manually.  See
View::EnableCanvasFlippingForRTLUI() instead.

I think once that's done you can also remove the save and restore calls.

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.cc:39: ((icon.height() -
icon.height()) + 1) / 2, w, h,
On 2011/06/09 13:23:07, Miranda Callahan wrote:
> Looks like you are calculating (x - x) here?  (Also, you stored icon.height()
in
> |h| above, but chose to take icon.height() again?)

Yeah, I think one of these is supposed to be the bounds height and the other the
icon height.  Something got lost in translation when copying from the frame
code.

The original code was designed to grab the middle portion of the icon. Your code
now is probably doing something like grabbing the top portion.  Take a look at
how we clip the OTR icon in maximized versus restored mode in an existing build
to see what's supposed to happen.

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.cc:47: void
AvatarMenuButton::RunMenu(views::View* source, const gfx::Point &pt) {
Nit: While here, fix the position of '&'

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

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.h:32: virtual void
OnPaint(gfx::Canvas* canvas);
Nit: OVERRIDE?

May want to add "// views::MenuButton" above this line?

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.h:35: // Overridden from
views::ViewMenuDelegate:
Nit: Feel free to remove "Overridden from" and trailing colon

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/g...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/g...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:86: NULL : new
ProfileMenuModel;
FWIW, I think the OTR avatar should function as a button too.

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/g...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:442: // use the
incognito icon to layout the avatar button.
Why?

http://codereview.chromium.org/7003080/diff/1/views/controls/button/text_butt...
File views/controls/button/text_button.cc (right):

http://codereview.chromium.org/7003080/diff/1/views/controls/button/text_butt...
views/controls/button/text_button.cc:754: else if (has_pushed_icon_ && (state()
== BS_PUSHED))
Nit: No else after return

[sail, 2011-06-10 00:56:20]

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

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.cc:26: if (base::i18n::IsRTL()) {
On 2011/06/09 18:03:31, Peter Kasting wrote:
> You should not need to do this manually.  See
> View::EnableCanvasFlippingForRTLUI() instead.
> 
> I think once that's done you can also remove the save and restore calls.

Done.

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.cc:39: ((icon.height() -
icon.height()) + 1) / 2, w, h,
On 2011/06/09 18:03:31, Peter Kasting wrote:
> On 2011/06/09 13:23:07, Miranda Callahan wrote:
> > Looks like you are calculating (x - x) here?  (Also, you stored
icon.height()
> in
> > |h| above, but chose to take icon.height() again?)
> 
> Yeah, I think one of these is supposed to be the bounds height and the other
the
> icon height.  Something got lost in translation when copying from the frame
> code.
> 
> The original code was designed to grab the middle portion of the icon. Your
code
> now is probably doing something like grabbing the top portion.  Take a look at
> how we clip the OTR icon in maximized versus restored mode in an existing
build
> to see what's supposed to happen.

Fixed.
I changed the code to be a little more generic.
We now scale down the avatar image to fit the button.
If the image is bigger than the button we scale it to fit the width of the
button and then center vertically.

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.cc:47: void
AvatarMenuButton::RunMenu(views::View* source, const gfx::Point &pt) {
On 2011/06/09 18:03:31, Peter Kasting wrote:
> Nit: While here, fix the position of '&'

Done.

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

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.h:28: AvatarMenuButton(const
std::wstring& text, ui::MenuModel* menu_model);
On 2011/06/09 15:46:46, sky wrote:
> Document AvatarMenuButton takes ownership of the model.

Done.

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.h:32: virtual void
OnPaint(gfx::Canvas* canvas);
On 2011/06/09 18:03:31, Peter Kasting wrote:
> Nit: OVERRIDE?
> 
> May want to add "// views::MenuButton" above this line?

Done.

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/avatar_...
chrome/browser/ui/views/avatar_menu_button.h:35: // Overridden from
views::ViewMenuDelegate:
On 2011/06/09 18:03:31, Peter Kasting wrote:
> Nit: Feel free to remove "Overridden from" and trailing colon

Done.

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/b...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/b...
chrome/browser/ui/views/frame/browser_view.cc:469: if (!IsBrowserTypeNormal())
On 2011/06/09 15:46:46, sky wrote:
> nit: use a single return, eg:
> 
> return IsBrowserTypeNormal() && (IsOffTheRecord() || blah blah blah

Done.

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/b...
File chrome/browser/ui/views/frame/browser_view.h (right):

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/b...
chrome/browser/ui/views/frame/browser_view.h:168: // Returns true if the
non-client view should render and avatar icon.
On 2011/06/09 13:23:07, Miranda Callahan wrote:
> s/and/an

Done.

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/g...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/g...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:202: // See if the
point is within the avatar menu button.
On 2011/06/09 15:46:46, sky wrote:
> Move this above 200.

Done.

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/g...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:442: // use the
incognito icon to layout the avatar button.
On 2011/06/09 18:03:31, Peter Kasting wrote:
> Why?

Changed to hard code the button size.

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/o...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/o...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:204:
avatar_button_.reset(new AvatarMenuButton(std::wstring(), menu_model));
On 2011/06/09 15:46:46, sky wrote:
> Document AvatarMenuButton takes ownership of menu_model.

Done.

http://codereview.chromium.org/7003080/diff/1/chrome/browser/ui/views/frame/o...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:349: if
(avatar_button_.get() && avatar_button_->IsVisible() &&
On 2011/06/09 15:46:46, sky wrote:
> Move above 346

Done.

http://codereview.chromium.org/7003080/diff/1/views/controls/button/text_butt...
File views/controls/button/text_button.cc (right):

http://codereview.chromium.org/7003080/diff/1/views/controls/button/text_butt...
views/controls/button/text_button.cc:754: else if (has_pushed_icon_ && (state()
== BS_PUSHED))
On 2011/06/09 18:03:31, Peter Kasting wrote:
> Nit: No else after return

Done.

http://codereview.chromium.org/7003080/diff/1/views/controls/button/text_butt...
File views/controls/button/text_button.h (right):

http://codereview.chromium.org/7003080/diff/1/views/controls/button/text_butt...
views/controls/button/text_button.h:346: virtual SkBitmap GetImageToPaint()
const;
On 2011/06/09 15:46:46, sky wrote:
> Can this return const SkBitmap& ?

Done.

[sky, 2011-06-10 15:08:08]

http://codereview.chromium.org/7003080/diff/5002/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu_button.cc (right):

http://codereview.chromium.org/7003080/diff/5002/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_button.cc:38: float scale =
Don't you need to also worry about the height not fitting?

http://codereview.chromium.org/7003080/diff/5002/views/controls/button/text_b...
File views/controls/button/text_button.cc (right):

http://codereview.chromium.org/7003080/diff/5002/views/controls/button/text_b...
views/controls/button/text_button.cc:682: SkBitmap icon = GetImageToPaint();
const SkBitmap&

[sail, 2011-06-10 17:10:35]

http://codereview.chromium.org/7003080/diff/5002/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu_button.cc (right):

http://codereview.chromium.org/7003080/diff/5002/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_button.cc:38: float scale =
On 2011/06/10 15:08:08, sky wrote:
> Don't you need to also worry about the height not fitting?

If the height doesn't then it just gets cropped. This is what we do today with
the incognito avatar. I think it makes sense for profile avatars as well.

Added a comment explaining this.

http://codereview.chromium.org/7003080/diff/5002/views/controls/button/text_b...
File views/controls/button/text_button.cc (right):

http://codereview.chromium.org/7003080/diff/5002/views/controls/button/text_b...
views/controls/button/text_button.cc:682: SkBitmap icon = GetImageToPaint();
On 2011/06/10 15:08:08, sky wrote:
> const SkBitmap&

Done.

[sky, 2011-06-10 17:11:59]

LGTM

http://codereview.chromium.org/7003080/diff/1031/views/controls/button/text_b...
File views/controls/button/text_button.cc (right):

http://codereview.chromium.org/7003080/diff/1031/views/controls/button/text_b...
views/controls/button/text_button.cc:682: SkBitmap icon = GetImageToPaint();
const SkBitmap&

[sail, 2011-06-10 17:18:22]

http://codereview.chromium.org/7003080/diff/1031/views/controls/button/text_b...
File views/controls/button/text_button.cc (right):

http://codereview.chromium.org/7003080/diff/1031/views/controls/button/text_b...
views/controls/button/text_button.cc:682: SkBitmap icon = GetImageToPaint();
On 2011/06/10 17:11:59, sky wrote:
> const SkBitmap&

Oops, not awake yet. Fixed.

[Peter Kasting, 2011-06-10 20:37:21]

http://codereview.chromium.org/7003080/diff/5008/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu_button.cc (right):

http://codereview.chromium.org/7003080/diff/5008/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_button.cc:12: // offset size depends on
whether the frame is in glass or opaque mode.
This comment seems wrong.  I'm also confused as to what this 5 does.  It seems
like it would more accurately be computed based on existing constants in the
frame code?

http://codereview.chromium.org/7003080/diff/5008/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_button.cc:16: return
static_cast<int>(floor(x + 0.5));
Nit: "floor()" is unnecessary.

http://codereview.chromium.org/7003080/diff/5008/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_button.cc:42: int scaled_height =
Round(height() / scale);
You want "*", not "/".

http://codereview.chromium.org/7003080/diff/5008/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_button.cc:54: return gfx::Size(38, 31);
This seems kind of random.

I think your old way of getting the actual OTR icon was better, I just thought
you should have explained it more.

I also think if you're going to do this in this class you should override
GetPreferredSize() instead of making a new function.

http://codereview.chromium.org/7003080/diff/5008/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

http://codereview.chromium.org/7003080/diff/5008/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:85: ui::MenuModel*
menu_model = browser_view_->IsOffTheRecord() ?
I kinds think the avatar button should create the menu model itself as a local
scoped_refptr in the function where it runs the menu, rather than having it
passed in.

Besides, if you're going to show a menu all the time eventually anyway, this
will just be flat-out simpler.  I'd go ahead and make that change right now.

http://codereview.chromium.org/7003080/diff/5008/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:91: if
(browser_view_->IsOffTheRecord()) {
This is more code that seems like maybe it should be in the avatar menu button
itself?  The fact that both glass and opaque code contain copy-and-pasted code
seems like a tip-off.

http://codereview.chromium.org/7003080/diff/5008/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:201: // See if the
point is within the avatar menu button.
There's a problem with moving this code up like you've done, which is that now
the avatar takes precedence over the sysmenu region -- especially problematic in
a maximized window.

I think you need to move this back down to where it was (for both glass and
opaque code), and, if that leaves us with too much of the avatar unclickable,
move the avatar over a few pixels.

http://codereview.chromium.org/7003080/diff/5008/views/controls/button/text_b...
File views/controls/button/text_button.h (right):

http://codereview.chromium.org/7003080/diff/5008/views/controls/button/text_b...
views/controls/button/text_button.h:345: // Returns the image to paint. This is
invoked from paint.
Nit: I'd remove "This is invoked from paint" since this is invoked from multiple
places already.  Plus it's not really important to readers.

You could just remove the comment altogether, given that the comment says
exactly the same thing as the function name.

[sail, 2011-06-10 21:23:38]

This change is half checked in right now. I'll address Peter's suggestions in a
separate change.

http://codereview.chromium.org/7003080/diff/5008/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu_button.cc (right):

http://codereview.chromium.org/7003080/diff/5008/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_button.cc:42: int scaled_height =
Round(height() / scale);
On 2011/06/10 20:37:21, Peter Kasting wrote:
> You want "*", not "/".

scaled_height is for the image so using / works. * is used below for computing
the dst_height.

http://codereview.chromium.org/7003080/diff/5008/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_button.cc:54: return gfx::Size(38, 31);
On 2011/06/10 20:37:21, Peter Kasting wrote:
> This seems kind of random.
> 
> I think your old way of getting the actual OTR icon was better, I just thought
> you should have explained it more.
> 
> I also think if you're going to do this in this class you should override
> GetPreferredSize() instead of making a new function.

I had to make it static because the avatar bounds need to be calculate even if
the button doesn't exist. I'll change it back to using the OTR icon size though.

http://codereview.chromium.org/7003080/diff/5008/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right):

http://codereview.chromium.org/7003080/diff/5008/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/glass_browser_frame_view.cc:201: // See if the
point is within the avatar menu button.
On 2011/06/10 20:37:21, Peter Kasting wrote:
> There's a problem with moving this code up like you've done, which is that now
> the avatar takes precedence over the sysmenu region -- especially problematic
in
> a maximized window.
> 
> I think you need to move this back down to where it was (for both glass and
> opaque code), and, if that leaves us with too much of the avatar unclickable,
> move the avatar over a few pixels.

Will fix.
Regardless of where I put this check the avatar button always seems to get
precedence.  I talked to sky about this and he suggested I change the avatar hit
testing code to hit test against the opaque area of the avatar. Once I do this
it should interact better with the system menu.