Bring up fast user switcher on right-click of the avatar menu on Mac.

Bring up fast user switcher on right-click of the avatar menu on Mac.

Change the behavior of fast user switching on Mac from Command click to right
click to be consistent with Windows and Linux.

BUG=458755
TEST=
1. Disable #enable-fast-user-switcher and enable #new-avatar-menu flags in chrome://flags
2. Relaunch Chrome
3. Right click on the Avatar Button, the fast user switcher should be shown
4. Command+Click on the Avatar Button, nothing should happen

Committed: https://crrev.com/95b89c2672ce7b0ed1a6803b59acee369b232a3c
Cr-Commit-Position: refs/heads/master@{#317631}

[anthonyvd, 2015-02-14 05:58:35]

anthonyvd@chromium.org changed reviewers:
+ asvitkine@chromium.org

[anthonyvd, 2015-02-14 05:58:36]

Hi asvitkine@, can you please take a look at those 4 files under
chrome/browser/ui/cocoa/profiles?

Thanks a lot!

[Alexei Svitkine (slow), 2015-02-17 16:43:25]

asvitkine@chromium.org changed reviewers:
+ rsesek@chromium.org

[Alexei Svitkine (slow), 2015-02-17 16:43:27]

Please add a TEST= line explaining how the UI changes can be tested.

+rsesek to also review the Cocoa nested message loop code, in case I miss
something subtle.

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
File chrome/browser/ui/cocoa/profiles/avatar_button.h (right):

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/avatar_button.h:14: UI_BASE_EXPORT
This isn't in ui base, so shouldn't need this.

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/avatar_button.h:16: SEL rightAction;
Add @private.

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/avatar_button.h:19: -
(void)setRightAction:(SEL)selector;
Needs a comment.

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
File chrome/browser/ui/cocoa/profiles/avatar_button.mm (right):

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/avatar_button.mm:18: -
(void)rightMouseDown:(NSEvent*)theEvent {
Nit: theEvent -> event

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/avatar_button.mm:19: NSEvent *newEvent =
theEvent;
Nit: * should be left of the space.

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/avatar_button.mm:41: -
(void)performRightClick:(id)sender {
Why have a sender param if you're passing in nil on line 37?

[Robert Sesek, 2015-02-17 18:40:46]

AvatarButton should also have a unittest.

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
File chrome/browser/ui/cocoa/profiles/avatar_button.h (right):

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/avatar_button.h:12: // Subclassing
HoverImageButton for the sole purpose of letting the Avatar
Use descriptive comments
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm...).

E.g., "A subclass of HoverImageButton that sends a target-action on right
click."

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
File chrome/browser/ui/cocoa/profiles/avatar_button.mm (right):

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/avatar_button.mm:16: // Override rightMouseDown
and implement a custom mouse tracking loop
nit: punctuation

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/avatar_button.mm:17: // See:
http://stackoverflow.com/a/5819695
Remove.

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/avatar_button.mm:19: NSEvent *newEvent =
theEvent;
naming: nextEvent

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/avatar_button.mm:30: NSRightMouseUpMask];
nit: align with the 'N'

[anthonyvd, 2015-02-18 00:02:24]

I also added a unit test.  I'm having issues testing the actual mouse tracking. 
Is there an easy way to fake mouse movement/buttons?

Thanks for your time!

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
File chrome/browser/ui/cocoa/profiles/avatar_button.h (right):

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/avatar_button.h:12: // Subclassing
HoverImageButton for the sole purpose of letting the Avatar
On 2015/02/17 18:40:46, Robert Sesek wrote:
> Use descriptive comments
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm...).
> 
> E.g., "A subclass of HoverImageButton that sends a target-action on right
> click."

Done.

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/avatar_button.h:14: UI_BASE_EXPORT
On 2015/02/17 16:43:26, Alexei Svitkine wrote:
> This isn't in ui base, so shouldn't need this.

Done.

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/avatar_button.h:16: SEL rightAction;
On 2015/02/17 16:43:26, Alexei Svitkine wrote:
> Add @private.

Done.

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/avatar_button.h:19: -
(void)setRightAction:(SEL)selector;
On 2015/02/17 16:43:26, Alexei Svitkine wrote:
> Needs a comment.

Done.

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
File chrome/browser/ui/cocoa/profiles/avatar_button.mm (right):

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/avatar_button.mm:16: // Override rightMouseDown
and implement a custom mouse tracking loop
On 2015/02/17 18:40:46, Robert Sesek wrote:
> nit: punctuation

Done.

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/avatar_button.mm:17: // See:
http://stackoverflow.com/a/5819695
On 2015/02/17 18:40:46, Robert Sesek wrote:
> Remove.

Done.

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/avatar_button.mm:18: -
(void)rightMouseDown:(NSEvent*)theEvent {
On 2015/02/17 16:43:26, Alexei Svitkine wrote:
> Nit: theEvent -> event

Done.

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/avatar_button.mm:19: NSEvent *newEvent =
theEvent;
On 2015/02/17 16:43:26, Alexei Svitkine wrote:
> Nit: * should be left of the space.

Done.

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/avatar_button.mm:19: NSEvent *newEvent =
theEvent;
On 2015/02/17 18:40:46, Robert Sesek wrote:
> naming: nextEvent

Done.

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/avatar_button.mm:30: NSRightMouseUpMask];
On 2015/02/17 18:40:46, Robert Sesek wrote:
> nit: align with the 'N'

Done.

https://codereview.chromium.org/916523003/diff/1/chrome/browser/ui/cocoa/prof...
chrome/browser/ui/cocoa/profiles/avatar_button.mm:41: -
(void)performRightClick:(id)sender {
On 2015/02/17 16:43:26, Alexei Svitkine wrote:
> Why have a sender param if you're passing in nil on line 37?

Done.

[Robert Sesek, 2015-02-18 00:12:10]

On 2015/02/18 00:02:24, anthonyvd wrote:
> I also added a unit test.  I'm having issues testing the actual mouse
tracking. 
> Is there an easy way to fake mouse movement/buttons?
> 
> Thanks for your time!

I don't have time to do a full round right now, but to answer this question:

https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/test/coc...

[Robert Sesek, 2015-02-18 18:51:37]

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/avatar_button.h (right):

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button.h:14: @private
nit: indent +1 space

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button.h:15: SEL rightAction;
naming: rightAction_

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/avatar_button_unittest.mm (right):

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_unittest.mm:21: @synthesize
clicked;
clicked = clicked_;

[Alexei Svitkine (slow), 2015-02-18 19:58:09]

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/avatar_button_unittest.mm (right):

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_unittest.mm:1: // Copyright (c)
2015 The Chromium Authors. All rights reserved.
No (c)

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_unittest.mm:14: @interface
AvatarButtonTestObserver: NSObject
Nit: Space before :

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_unittest.mm:34: public:
Nit: Indent 1 more

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_unittest.mm:47:
AvatarButtonTestObserver* obs = [[AvatarButtonTestObserver alloc] init];
Is this leaked?  Probably should be scoped_nsobject.

Also, use full names -> observer

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_unittest.mm:54: }  // namespace
Usually, the TEST_F() code isn't inside an anon namespace...

[anthonyvd, 2015-02-19 03:11:02]

anthonyvd@chromium.org changed reviewers:
+ sadrul@chromium.org

[anthonyvd, 2015-02-19 03:11:02]

Hi sadrul@, could you please take a look at those cocoa_test_event_utils
changes?  Thank you!

[anthonyvd, 2015-02-19 03:12:52]

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/avatar_button.h (right):

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button.h:14: @private
On 2015/02/18 18:51:37, Robert Sesek wrote:
> nit: indent +1 space

Done.

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button.h:15: SEL rightAction;
On 2015/02/18 18:51:37, Robert Sesek wrote:
> naming: rightAction_

Done.

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/avatar_button_unittest.mm (right):

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_unittest.mm:1: // Copyright (c)
2015 The Chromium Authors. All rights reserved.
On 2015/02/18 19:58:09, Alexei Svitkine wrote:
> No (c)

Done.

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_unittest.mm:14: @interface
AvatarButtonTestObserver: NSObject
On 2015/02/18 19:58:09, Alexei Svitkine wrote:
> Nit: Space before :

Done.

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_unittest.mm:21: @synthesize
clicked;
On 2015/02/18 18:51:37, Robert Sesek wrote:
> clicked = clicked_;

Done.

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_unittest.mm:34: public:
On 2015/02/18 19:58:09, Alexei Svitkine wrote:
> Nit: Indent 1 more

Done.

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_unittest.mm:47:
AvatarButtonTestObserver* obs = [[AvatarButtonTestObserver alloc] init];
On 2015/02/18 19:58:09, Alexei Svitkine wrote:
> Is this leaked?  Probably should be scoped_nsobject.
> 
> Also, use full names -> observer

Done.

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_unittest.mm:54: }  // namespace
On 2015/02/18 19:58:09, Alexei Svitkine wrote:
> Usually, the TEST_F() code isn't inside an anon namespace...

Done.

[Alexei Svitkine (slow), 2015-02-19 16:40:45]

lgtm % comments, but please also wait for rsesek's review.

Thanks!

https://codereview.chromium.org/916523003/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/avatar_button.mm (right):

https://codereview.chromium.org/916523003/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button.mm:16: // Override rightMouseDown
and implement a custom mouse tracking loop.
Nit: Change verb tense - "Overrides" and "implements".

Also, rightMouseDown -> -rightMouseDown (with a - in front).

https://codereview.chromium.org/916523003/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right):

https://codereview.chromium.org/916523003/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:149: AvatarButton*
hoverButton =
Nit: hoverButton -> avatarButton

https://codereview.chromium.org/916523003/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:171: [hoverButton
setRightAction:@selector(buttonRightClicked:)];
Nit: this function uses button_ instead of the local var for the other calls -
be consistent.

[Robert Sesek, 2015-02-19 21:54:21]

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/avatar_button_unittest.mm (right):

https://codereview.chromium.org/916523003/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_unittest.mm:21: @synthesize
clicked;
On 2015/02/18 18:51:37, Robert Sesek wrote:
> clicked = clicked_;

Sorry if this comment was confusing. The @property should still be:

@property(nonatomc) BOOL clicked;

But the synthesize should make an ivar named clicked_:

@synthesize clicked = clicked_;

https://codereview.chromium.org/916523003/diff/40001/ui/events/test/cocoa_tes...
File ui/events/test/cocoa_test_event_utils.h (right):

https://codereview.chromium.org/916523003/diff/40001/ui/events/test/cocoa_tes...
ui/events/test/cocoa_test_event_utils.h:21: NSEvent*
MouseEventAtPointInWindow(NSPoint point,
Header order should match impl.

[anthonyvd, 2015-02-20 18:47:40]

New patchsets have been uploaded after l-g-t-m from asvitkine@chromium.org

[anthonyvd, 2015-02-20 18:50:48]

https://codereview.chromium.org/916523003/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/avatar_button.mm (right):

https://codereview.chromium.org/916523003/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button.mm:16: // Override rightMouseDown
and implement a custom mouse tracking loop.
On 2015/02/19 16:40:44, Alexei Svitkine wrote:
> Nit: Change verb tense - "Overrides" and "implements".
> 
> Also, rightMouseDown -> -rightMouseDown (with a - in front).

Done.

https://codereview.chromium.org/916523003/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right):

https://codereview.chromium.org/916523003/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:149: AvatarButton*
hoverButton =
On 2015/02/19 16:40:44, Alexei Svitkine wrote:
> Nit: hoverButton -> avatarButton

Done.

https://codereview.chromium.org/916523003/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:171: [hoverButton
setRightAction:@selector(buttonRightClicked:)];
On 2015/02/19 16:40:44, Alexei Svitkine wrote:
> Nit: this function uses button_ instead of the local var for the other calls -
> be consistent.

Done.

https://codereview.chromium.org/916523003/diff/40001/ui/events/test/cocoa_tes...
File ui/events/test/cocoa_test_event_utils.h (right):

https://codereview.chromium.org/916523003/diff/40001/ui/events/test/cocoa_tes...
ui/events/test/cocoa_test_event_utils.h:21: NSEvent*
MouseEventAtPointInWindow(NSPoint point,
On 2015/02/19 21:54:21, Robert Sesek wrote:
> Header order should match impl.

Done.

[anthonyvd, 2015-02-20 21:52:44]

Hi guys,

Sorry to be pushy like this but I just wanted to notify you that this just
became a Beta Blocker. I'll do my best to address anymore feedback you have
swiftly and I really appreciate you taking this time.

Thanks a lot!

[Robert Sesek, 2015-02-20 21:56:07]

Last comment!

https://codereview.chromium.org/916523003/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/avatar_button_unittest.mm (right):

https://codereview.chromium.org/916523003/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_unittest.mm:25: - (id)init {
-alloc will 0 all instance variables, so you can delete this, and line 18.

[sadrul, 2015-02-23 17:04:49]

rubber-stamp lgtm after addressing rsesek@'s comments.

Can //ui/events/cocoa/ and //ui/events/test/cocoa* have owners?

[anthonyvd, 2015-02-23 17:20:38]

New patchsets have been uploaded after l-g-t-m from sadrul@chromium.org

[anthonyvd, 2015-02-23 17:21:53]

rsesek@, addressed your last comment.

Thanks everyone!

https://codereview.chromium.org/916523003/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/avatar_button_unittest.mm (right):

https://codereview.chromium.org/916523003/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_unittest.mm:25: - (id)init {
On 2015/02/20 21:56:06, Robert Sesek wrote:
> -alloc will 0 all instance variables, so you can delete this, and line 18.

Done.

[Robert Sesek, 2015-02-23 17:34:31]

lgtm

[anthonyvd, 2015-02-23 17:34:51]

The CQ bit was checked by anthonyvd@chromium.org

[commit-bot: I haz the power, 2015-02-23 17:35:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/916523003/80001

[commit-bot: I haz the power, 2015-02-23 17:41:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-23 17:41:25]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[anthonyvd, 2015-02-23 19:54:51]

The CQ bit was checked by anthonyvd@chromium.org

[commit-bot: I haz the power, 2015-02-23 19:55:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/916523003/80001

[commit-bot: I haz the power, 2015-02-23 19:56:45]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-23 19:57:39]

Patchset 5 (id:??) landed as
https://crrev.com/95b89c2672ce7b0ed1a6803b59acee369b232a3c
Cr-Commit-Position: refs/heads/master@{#317631}