[Mac/multiprofile] "Customize User..." should edit the currently active profile

[Mac/multiprofile] "Customize User..." should edit the currently active profile

The NSMenu tag for the "Customize User..." menu item was always 0 before this patch.  Add a new selector to ProfileMenuController and use that for the menu item.

Also, move the code for determining the index of the active profile into the AvatarMenuModel.

BUG=99621
TEST=See bug


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

[jeremy, 2011-10-11 11:58:44]

Robert: Review plz
Mark: owner review, rubberstamp ok.

[Robert Sesek, 2011-10-11 14:23:13]

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

http://codereview.chromium.org/8230011/diff/1/chrome/browser/profiles/avatar_...
chrome/browser/profiles/avatar_menu_model_unittest.cc:85: ASSERT_EQ(0U,
model.GetActiveProfileIndex());
May also want to change the active profile and assert that the new index is
correct. I think the profile_menu_controller_unittest does something like this,
too.

http://codereview.chromium.org/8230011/diff/1/chrome/browser/ui/cocoa/profile...
File chrome/browser/ui/cocoa/profile_menu_controller.mm (right):

http://codereview.chromium.org/8230011/diff/1/chrome/browser/ui/cocoa/profile...
chrome/browser/ui/cocoa/profile_menu_controller.mm:86: -
(IBAction)editCurrentProfile:(id)sender {
I don't think you need a new selector. You can only edit the current profile.

http://codereview.chromium.org/8230011/diff/1/chrome/browser/ui/cocoa/profile...
chrome/browser/ui/cocoa/profile_menu_controller.mm:139: bool is_active =
active_profile_index == tag;
Use ObjC BOOL

[Mark Mentovai, 2011-10-11 14:36:18]

“noparent” is not in effect and my review is not necessary. Robert is more than
competent to give you an LG when this code is ready.

[Miranda Callahan, 2011-10-11 18:40:06]

On 2011/10/11 14:36:18, Mark Mentovai wrote:
> “noparent” is not in effect and my review is not necessary. Robert is more
than
> competent to give you an LG when this code is ready.

LGTM from me, but I defer to Robert as well -- this is his area.

[jeremy, 2011-10-19 13:15:23]

Ready for another look.

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

http://codereview.chromium.org/8230011/diff/1/chrome/browser/profiles/avatar_...
chrome/browser/profiles/avatar_menu_model_unittest.cc:85: ASSERT_EQ(0U,
model.GetActiveProfileIndex());
You're right but I'm a bit pressed for time with this change, added TODO.

http://codereview.chromium.org/8230011/diff/1/chrome/browser/ui/cocoa/profile...
File chrome/browser/ui/cocoa/profile_menu_controller.mm (right):

http://codereview.chromium.org/8230011/diff/1/chrome/browser/ui/cocoa/profile...
chrome/browser/ui/cocoa/profile_menu_controller.mm:86: -
(IBAction)editCurrentProfile:(id)sender {
On 2011/10/11 14:23:13, rsesek wrote:
> I don't think you need a new selector. You can only edit the current profile.

Done.

http://codereview.chromium.org/8230011/diff/1/chrome/browser/ui/cocoa/profile...
chrome/browser/ui/cocoa/profile_menu_controller.mm:139: bool is_active =
active_profile_index == tag;
On 2011/10/11 14:23:13, rsesek wrote:
> Use ObjC BOOL

Done.

[Robert Sesek, 2011-10-19 15:07:40]

http://codereview.chromium.org/8230011/diff/6001/chrome/browser/ui/cocoa/prof...
File chrome/browser/ui/cocoa/profile_menu_controller.mm (right):

http://codereview.chromium.org/8230011/diff/6001/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profile_menu_controller.mm:135: BOOL is_active =
(active_profile_index == tag) ? YES : NO;
Why pull this out into a separate variable? The original style is more concise.

[jeremy, 2011-10-19 18:24:08]

Lgtm with that part of the Cl reverted?
On Oct 19, 2011 5:07 PM, <rsesek@chromium.org> wrote:

>
> http://codereview.chromium.**org/8230011/diff/6001/chrome/**
>
browser/ui/cocoa/profile_menu_**controller.mm<http://codereview.chromium.org/8230011/diff/6001/chrome/browser/ui/cocoa/profile_menu_controller.mm>
> File
chrome/browser/ui/cocoa/profil**e_menu_controller.mm<http://profile_menu_controller.mm>(right):
>
> http://codereview.chromium.**org/8230011/diff/6001/chrome/**
>
browser/ui/cocoa/profile_menu_**controller.mm#newcode135<http://codereview.chromium.org/8230011/diff/6001/chrome/browser/ui/cocoa/profile_menu_controller.mm#newcode135>
>
chrome/browser/ui/cocoa/profil**e_menu_controller.mm:135<http://profile_menu_controller.mm:135>:
> BOOL is_active =
> (active_profile_index == tag) ? YES : NO;
> Why pull this out into a separate variable? The original style is more
> concise.
>
>
http://codereview.chromium.**org/8230011/<http://codereview.chromium.org/8230...
>

[Robert Sesek, 2011-10-19 18:50:13]

On 2011/10/19 18:24:08, jeremy wrote:
> Lgtm with that part of the Cl reverted?

yes. lgtm with that part reverted

> On Oct 19, 2011 5:07 PM, <mailto:rsesek@chromium.org> wrote:
> 
> >
> > http://codereview.chromium.**org/8230011/diff/6001/chrome/**
> >
>
browser/ui/cocoa/profile_menu_**controller.mm<http://codereview.chromium.org/8230011/diff/6001/chrome/browser/ui/cocoa/profile_menu_controller.mm>
> > File
>
chrome/browser/ui/cocoa/profil**e_menu_controller.mm<http://profile_menu_controller.mm>(right):
> >
> > http://codereview.chromium.**org/8230011/diff/6001/chrome/**
> >
>
browser/ui/cocoa/profile_menu_**controller.mm#newcode135<http://codereview.chromium.org/8230011/diff/6001/chrome/browser/ui/cocoa/profile_menu_controller.mm#newcode135>
> >
>
chrome/browser/ui/cocoa/profil**e_menu_controller.mm:135<http://profile_menu_controller.mm:135>:
> > BOOL is_active =
> > (active_profile_index == tag) ? YES : NO;
> > Why pull this out into a separate variable? The original style is more
> > concise.
> >
> >
>
http://codereview.chromium.**org/8230011/%3Chttp://codereview.chromium.org/82...>
> >

[commit-bot: I haz the power, 2011-10-23 12:18:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremy@chromium.org/8230011/12001

[commit-bot: I haz the power, 2011-10-23 12:18:02]

Failed to send try job 8230011-12001: 2

[commit-bot: I haz the power, 2011-10-25 08:27:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremy@chromium.org/8230011/12001

[commit-bot: I haz the power, 2011-10-25 09:40:29]

Change committed as 107089