Multi-Profiles: Add icon chooser to profiles menu

Multi-Profiles: Add icon chooser to profiles menu

This change adds an icon chooser grid to the profiles menu. This is only implemented for toolkit views for now.

Screenshots: http://www.dropmocks.com/mXGba

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

[sky, 2011-07-08 21:01:43]

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/profile_me...
File chrome/browser/ui/profile_menu_model.h (right):

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/profile_me...
chrome/browser/ui/profile_menu_model.h:27: enum {
enums should be first in a section.

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu.cc (right):

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu.cc:31: const int CELL_WIDTH = 32;
kCellWidth. Same for all these constants.

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu.cc:62: void SetAvatarIconToIndex(int
icon_index);
Description?

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu.cc:108:
ProfileInfoCache::GetDefaultAvatarIconResourceIDAtIndex(i);
indent by 4.

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu.cc:150: break;
Do you want to close the menu when the user clicks on a button?

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu.cc:153: }
NOTREACHED

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu.h (right):

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu.h:22: explicit AvatarMenu(Browser* browser);
It seems like we only need Profile for this class. Is there a reason you're
taking Browser?

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu.h:29: virtual bool IsItemChecked(int id)
const OVERRIDE;
Code for building a MenuItemView exists in MenuModelAdapter (in
views/controls/menu). Can you use it instead of duplicating the functionality?

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

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_button.cc:62: avatar_menu->RunMenu(this);
Add a comment that AvatarMenu deletes itself.

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu_button.h (right):

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_button.h:30: AvatarMenuButton(const
std::wstring& text, ui::MenuModel* menu_model,
When you can't fill all params on the same line, then one per line.

http://codereview.chromium.org/7331017/diff/2001/views/controls/menu/menu_ite...
File views/controls/menu/menu_item_view.cc (right):

http://codereview.chromium.org/7331017/diff/2001/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view.cc:750: } else {
no else (since you returned early).

http://codereview.chromium.org/7331017/diff/2001/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view.cc:757: return gfx::Size(width, 0);
Document why its ok to return a height of 0 here.

[sail, 2011-07-08 21:53:57]

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/profile_me...
File chrome/browser/ui/profile_menu_model.h (right):

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/profile_me...
chrome/browser/ui/profile_menu_model.h:27: enum {
On 2011/07/08 21:01:44, sky wrote:
> enums should be first in a section.

Done.

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu.cc (right):

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu.cc:31: const int CELL_WIDTH = 32;
On 2011/07/08 21:01:44, sky wrote:
> kCellWidth. Same for all these constants.

Done.

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu.cc:62: void SetAvatarIconToIndex(int
icon_index);
On 2011/07/08 21:01:44, sky wrote:
> Description?

Done.

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu.cc:108:
ProfileInfoCache::GetDefaultAvatarIconResourceIDAtIndex(i);
On 2011/07/08 21:01:44, sky wrote:
> indent by 4.

Done.

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu.cc:150: break;
On 2011/07/08 21:01:44, sky wrote:
> Do you want to close the menu when the user clicks on a button?

Ahh, that would be a good idea. Off the top of your head do you know what method
does that?

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu.cc:153: }
On 2011/07/08 21:01:44, sky wrote:
> NOTREACHED

Done.

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu.h (right):

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu.h:22: explicit AvatarMenu(Browser* browser);
On 2011/07/08 21:01:44, sky wrote:
> It seems like we only need Profile for this class. Is there a reason you're
> taking Browser?

Done.

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu.h:29: virtual bool IsItemChecked(int id)
const OVERRIDE;
On 2011/07/08 21:01:44, sky wrote:
> Code for building a MenuItemView exists in MenuModelAdapter (in
> views/controls/menu). Can you use it instead of duplicating the functionality?

Done.

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

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_button.cc:62: avatar_menu->RunMenu(this);
On 2011/07/08 21:01:44, sky wrote:
> Add a comment that AvatarMenu deletes itself.

Done.

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu_button.h (right):

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_button.h:30: AvatarMenuButton(const
std::wstring& text, ui::MenuModel* menu_model,
On 2011/07/08 21:01:44, sky wrote:
> When you can't fill all params on the same line, then one per line.

Cleaned up the constructor.

http://codereview.chromium.org/7331017/diff/2001/views/controls/menu/menu_ite...
File views/controls/menu/menu_item_view.cc (right):

http://codereview.chromium.org/7331017/diff/2001/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view.cc:750: } else {
On 2011/07/08 21:01:44, sky wrote:
> no else (since you returned early).

Done.

http://codereview.chromium.org/7331017/diff/2001/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view.cc:757: return gfx::Size(width, 0);
On 2011/07/08 21:01:44, sky wrote:
> Document why its ok to return a height of 0 here.

Done.

[sky, 2011-07-08 22:31:25]

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu.cc (right):

http://codereview.chromium.org/7331017/diff/2001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu.cc:150: break;
On 2011/07/08 21:53:57, sail wrote:
> On 2011/07/08 21:01:44, sky wrote:
> > Do you want to close the menu when the user clicks on a button?
> 
> Ahh, that would be a good idea. Off the top of your head do you know what
method
> does that?

CAncel will close the menu.

[sail, 2011-07-08 22:51:34]

> CAncel will close the menu.
Done, thanks!

[sky, 2011-07-08 23:03:32]

LGTM

http://codereview.chromium.org/7331017/diff/14/chrome/browser/ui/views/avatar...
File chrome/browser/ui/views/avatar_menu.cc (right):

http://codereview.chromium.org/7331017/diff/14/chrome/browser/ui/views/avatar...
chrome/browser/ui/views/avatar_menu.cc:54: explicit AvatarIconGridView(Profile*
profile, views::MenuItemView* menu);
no explicit

http://codereview.chromium.org/7331017/diff/14/chrome/browser/ui/views/avatar...
File chrome/browser/ui/views/avatar_menu_button.h (right):

http://codereview.chromium.org/7331017/diff/14/chrome/browser/ui/views/avatar...
chrome/browser/ui/views/avatar_menu_button.h:30: AvatarMenuButton(Browser*
browser, bool has_menu);
Document has_menu. And make this take a Profile instead of Browser.

[sail, 2011-07-08 23:07:55]

http://codereview.chromium.org/7331017/diff/14/chrome/browser/ui/views/avatar...
File chrome/browser/ui/views/avatar_menu.cc (right):

http://codereview.chromium.org/7331017/diff/14/chrome/browser/ui/views/avatar...
chrome/browser/ui/views/avatar_menu.cc:54: explicit AvatarIconGridView(Profile*
profile, views::MenuItemView* menu);
On 2011/07/08 23:03:35, sky wrote:
> no explicit

Done.

http://codereview.chromium.org/7331017/diff/14/chrome/browser/ui/views/avatar...
File chrome/browser/ui/views/avatar_menu_button.h (right):

http://codereview.chromium.org/7331017/diff/14/chrome/browser/ui/views/avatar...
chrome/browser/ui/views/avatar_menu_button.h:30: AvatarMenuButton(Browser*
browser, bool has_menu);
On 2011/07/08 23:03:35, sky wrote:
> Document has_menu. And make this take a Profile instead of Browser.

Added documentation. I still need the browser parameter though. It's used to
create the ProfileMenuModel object.

[sky, 2011-07-08 23:08:46]

On Fri, Jul 8, 2011 at 4:07 PM,  <sail@chromium.org> wrote:
>
>
http://codereview.chromium.org/7331017/diff/14/chrome/browser/ui/views/avatar...
> File chrome/browser/ui/views/avatar_menu.cc (right):
>
>
http://codereview.chromium.org/7331017/diff/14/chrome/browser/ui/views/avatar...
> chrome/browser/ui/views/avatar_menu.cc:54: explicit
> AvatarIconGridView(Profile* profile, views::MenuItemView* menu);
> On 2011/07/08 23:03:35, sky wrote:
>>
>> no explicit
>
> Done.
>
>
http://codereview.chromium.org/7331017/diff/14/chrome/browser/ui/views/avatar...
> File chrome/browser/ui/views/avatar_menu_button.h (right):
>
>
http://codereview.chromium.org/7331017/diff/14/chrome/browser/ui/views/avatar...
> chrome/browser/ui/views/avatar_menu_button.h:30:
> AvatarMenuButton(Browser* browser, bool has_menu);
> On 2011/07/08 23:03:35, sky wrote:
>>
>> Document has_menu. And make this take a Profile instead of Browser.
>
> Added documentation. I still need the browser parameter though. It's
> used to create the ProfileMenuModel object.

Fair enough.

  -Scott

[Miranda Callahan, 2011-07-09 13:24:25]

Code looks great -- just a question and a nit; but I have a concern about the
mock -- looking at the mock-up, I really don't know what I'm supposed to do in
this menu.  Why is "Default Profile" grayed out? Is that the profile I'm in at
the moment?  Am I supposed to choose from these icons?  Why isn't the icon
chooser under the "customize profile..." dialog?

I would vote for the icon chooser to be under "customize profile..." -- I don't
really get why it isn't, logically. If it stays here, there should at least be
some indication that all these icons are appearing because this is a chooser,
because that's not apparent.  Something like "Change profile icon:".  

If you want to get this in and iterate on the UI (which is probably the right
solution for now), could you at least add this explanatory sentence? Otherwise
I'm afraid this menu is going to be confusing (it is for me).

http://codereview.chromium.org/7331017/diff/6005/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu.cc (right):

http://codereview.chromium.org/7331017/diff/6005/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu.cc:90: dst_width = std::min(width(),
icon.width());
Do you need a std::max of the dimension here (and in the else clause)?

http://codereview.chromium.org/7331017/diff/6005/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu_button.h (right):

http://codereview.chromium.org/7331017/diff/6005/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_button.h:29: // Creates a new button. If
|has_menu| is truen then clicking on the button
s/truen/true

[sail, 2011-07-09 17:58:47]

> I would vote for the icon chooser to be under "customize profile..." -- I
don't
Hey Miranda, I agree with you that the current UI is confusing. I originally had
the icon chooser inside a submenu. See: http://www.dropmocks.com/mXG99

I thought moving to a top level would simply things but that doesn't seem to
have worked.

The first item in the menu is supposed to be the profile name. I think it's
really helpful to have it there. Do you think there's a way to make it clearer?
Maybe something like "Profile: Name of profile"

Also, I'll check in the UI as it is right now and send out a separate CL to fix
up the UI. Thanks.

http://codereview.chromium.org/7331017/diff/6005/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu.cc (right):

http://codereview.chromium.org/7331017/diff/6005/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu.cc:90: dst_width = std::min(width(),
icon.width());
On 2011/07/09 13:24:26, Miranda Callahan wrote:
> Do you need a std::max of the dimension here (and in the else clause)?

the destination width can't be bigger than the width of the view. That's why
this has to be a minimum of the icon width or the view width.

http://codereview.chromium.org/7331017/diff/6005/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu_button.h (right):

http://codereview.chromium.org/7331017/diff/6005/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_button.h:29: // Creates a new button. If
|has_menu| is truen then clicking on the button
On 2011/07/09 13:24:26, Miranda Callahan wrote:
> s/truen/true

Done.