Desktop Menu UMA for Mac

Desktop Menu UMA for Mac, copying the Views(Win/Linux) code.

BUG=357693
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274443

[Mike Lerman, 2014-05-29 13:40:32]

Hi Scott,

Can I get a code review please? I'm porting these two CLs over to Mac:
https://codereview.chromium.org/288493008/
https://codereview.chromium.org/302603013/

Thanks!

[Scott Hess - ex-Googler, 2014-05-30 04:09:27]

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

https://codereview.chromium.org/308433002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.h:45:
withServiceType:(signin::GAIAServiceType)GAIAServiceType ;
Excess space before ;.  Also, I think repeating the type name in the variable
name is redundant, unless everyone else uses that convention.

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

https://codereview.chromium.org/308433002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.h:63:
signin::GAIAServiceType GAIAServiceType_;
Hmm, and also I think it would be gaiaServiceType_.  I guess that's another
argument for serviceType_ ?

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

https://codereview.chromium.org/308433002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:745: -
(void)postActionPerformed:(ProfileMetrics::ProfileDesktopMenu)action;
I think it's weird to have this down here though it's already been used
elsewhere in the file.  You should arrange to have it higher up somewhere in a
private interface or something.  I see lots of inline interfaces and
implementations, so I'm not going to decode what all is going on in here, but
I'm sure there is something earlier which makes sense.

That said, I'm not 100% certain about having it be a helper method versus some
sort of anonymous-namespace helper.  I guess resetting the instance variable is
valuable, and a c++-level helper wouldn't have access to that without
contortions.  So I'm talking myself right out of that.

[Mike Lerman, 2014-05-30 17:46:24]

Thanks for the input, Scott.

Can you review the changes I've made within profile_chooser_controller.mm to
pass around the reference to the Controller so I can call postActionPerformed on
it from the subViews? Thanks!

Mike

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

https://codereview.chromium.org/308433002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.h:45:
withServiceType:(signin::GAIAServiceType)GAIAServiceType ;
On 2014/05/30 04:09:27, shess wrote:
> Excess space before ;.  Also, I think repeating the type name in the variable
> name is redundant, unless everyone else uses that convention.

Done.

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

https://codereview.chromium.org/308433002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.h:63:
signin::GAIAServiceType GAIAServiceType_;
On 2014/05/30 04:09:27, shess wrote:
> Hmm, and also I think it would be gaiaServiceType_.  I guess that's another
> argument for serviceType_ ?

Done.

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

https://codereview.chromium.org/308433002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:745: -
(void)postActionPerformed:(ProfileMetrics::ProfileDesktopMenu)action;
On 2014/05/30 04:09:27, shess wrote:
> I think it's weird to have this down here though it's already been used
> elsewhere in the file.  You should arrange to have it higher up somewhere in a
> private interface or something.  I see lots of inline interfaces and
> implementations, so I'm not going to decode what all is going on in here, but
> I'm sure there is something earlier which makes sense.
> 
> That said, I'm not 100% certain about having it be a helper method versus some
> sort of anonymous-namespace helper.  I guess resetting the instance variable
is
> valuable, and a c++-level helper wouldn't have access to that without
> contortions.  So I'm talking myself right out of that.

Done... I guess... since there's nothing for me to do?

I passed the ProfileChooserController to the subViews so I can call
postActionPerformed. Can you review that part?

[Scott Hess - ex-Googler, 2014-05-30 23:35:45]

On 2014/05/30 17:46:24, Mike Lerman wrote:
> Can you review the changes I've made within profile_chooser_controller.mm to
> pass around the reference to the Controller so I can call postActionPerformed
on
> it from the subViews? Thanks!

LGTM ... did the previous code not work, then?  I guess I didn't even notice
that it was on a different object, for some reason I thought it was that the
implementation was broken up into pieces.

[Mike Lerman, 2014-06-02 16:45:51]

On 2014/05/30 23:35:45, shess wrote:
> On 2014/05/30 17:46:24, Mike Lerman wrote:
> > Can you review the changes I've made within profile_chooser_controller.mm to
> > pass around the reference to the Controller so I can call
postActionPerformed
> on
> > it from the subViews? Thanks!
> 
> LGTM ... did the previous code not work, then?  I guess I didn't even notice
> that it was on a different object, for some reason I thought it was that the
> implementation was broken up into pieces.

No, the previous code didn't actually work. It does now.

Thanks, Scott!

[Mike Lerman, 2014-06-02 21:10:03]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-06-02 21:11:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlerman@chromium.org/308433002/40001

[commit-bot: I haz the power, 2014-06-03 02:29:05]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-03 09:06:47]

Change committed as 274443