[Mac] Redesign the new avatar button.

[Mac] Redesign the new avatar button.

Screenshots: https://drive.google.com/open?id=0B1B1Up4p2NRMRXpYbGJ6emtlX0U&authuser=1

TL; DR:
- never show a drop down arrow in the button.
- this means that we no longer need to do the auth error drawing magic, as we can use the
button's image. Also cleaned up ALL of that insane code.
- if there is one local, non-signed in profile, show a tiny button with a generic avatar
- in all other cases, show the avatar button with the profile name
- increased the button height by 1px so that it's perfectly aligned with the new tab button

BUG=410946
Committed: https://crrev.com/c86c1fcf7beec49a7b362208da99904e5aee13fd
Cr-Commit-Position: refs/heads/master@{#297255}

[noms (inactive), 2014-09-25 18:16:35]

Patchset #1 (id:1) has been deleted

[noms (inactive), 2014-09-25 21:00:47]

Patchset #1 (id:20001) has been deleted

[noms (inactive), 2014-09-25 21:00:54]

Patchset #1 (id:40001) has been deleted

[noms (inactive), 2014-09-25 21:05:58]

noms@chromium.org changed reviewers:
+ groby@chromium.org

[noms (inactive), 2014-09-25 21:06:00]

Soooo I think I made moar better all the weird padding code by accident! YAY! 
( ＾◡＾)っ✂❤
(that is the emoticon for refactoring, obviously)

[groby-ooo-7-16, 2014-09-25 22:03:30]

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

https://codereview.chromium.org/605803002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:164: [cell
setHasError:hasError_ withTitle:[button_ title]];
1) Isn't [button title] empty at this point? It just got allocated?

2) Don't you want to call this even if you don't have an errorController, just
with hasError_ = false?

https://codereview.chromium.org/605803002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:227:
g_browser_process->profile_manager()->GetProfileInfoCache();
I suppose it's the only way to get the cache :( I'll just sit here and quietly
grumble about globals.

https://codereview.chromium.org/605803002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:240:
HoverImageButton* button = base::mac::ObjCCast<HoverImageButton>(button_);
Why not just make button_ a HoverImageButton? That way, you don't need to cast -
and it's not like it can be a different type.

https://codereview.chromium.org/605803002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:249: // button is
initially sized incorrectly unless a default image is provided.
Can you point to crbug?

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

https://codereview.chromium.org/605803002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller_unittest.mm:67: 
Can you add a test for the error case, please?

[noms (inactive), 2014-09-26 15:49:11]

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

https://codereview.chromium.org/605803002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:164: [cell
setHasError:hasError_ withTitle:[button_ title]];
On 2014/09/25 22:03:30, groby wrote:
> 1) Isn't [button title] empty at this point? It just got allocated?
> 
> 2) Don't you want to call this even if you don't have an errorController, just
> with hasError_ = false?

Done.

https://codereview.chromium.org/605803002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:227:
g_browser_process->profile_manager()->GetProfileInfoCache();
Yes ma'am. Chrome is peppered with this sad line, often times not making the
cache a const ref because of bad past choices.

On 2014/09/25 22:03:30, groby wrote:
> I suppose it's the only way to get the cache :( I'll just sit here and quietly
> grumble about globals.

https://codereview.chromium.org/605803002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:240:
HoverImageButton* button = base::mac::ObjCCast<HoverImageButton>(button_);
button_ is defined in AvatarBaseController. AvatarIconController also inherits
from that, and its button_ is just an NSButton. I guess, in theory I could make
that a HoverImageButton, though it wouldn't use any of the hover bits. Should I?

On 2014/09/25 22:03:30, groby wrote:
> Why not just make button_ a HoverImageButton? That way, you don't need to cast
-
> and it's not like it can be a different type.

https://codereview.chromium.org/605803002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:249: // button is
initially sized incorrectly unless a default image is provided.
On 2014/09/25 22:03:30, groby wrote:
> Can you point to crbug? 

Done.

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

https://codereview.chromium.org/605803002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller_unittest.mm:67: 
On 2014/09/25 22:03:30, groby wrote:
> Can you add a test for the error case, please?

Done.

[noms (inactive), 2014-09-29 13:55:12]

ping :)

[groby-ooo-7-16, 2014-09-29 17:55:44]

LGTM w/ a few nits.

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

https://codereview.chromium.org/605803002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:240:
HoverImageButton* button = base::mac::ObjCCast<HoverImageButton>(button_);
On 2014/09/26 15:49:10, Monica Dinculescu wrote:
> button_ is defined in AvatarBaseController. AvatarIconController also inherits
> from that, and its button_ is just an NSButton. I guess, in theory I could
make
> that a HoverImageButton, though it wouldn't use any of the hover bits. Should
I?

I keep missing that fact, don't I? No, you probably shouldn't. You should
probably make this ObjCCastStrict, though - it always should be set here, and it
always should be a HoverImageButton, right? 

> 
> On 2014/09/25 22:03:30, groby wrote:
> > Why not just make button_ a HoverImageButton? That way, you don't need to
cast
> -
> > and it's not like it can be a different type.
>

https://codereview.chromium.org/605803002/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/avatar_button_controller.h (right):

https://codereview.chromium.org/605803002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.h:29: @interface
AvatarButtonController (ExposedForTesting)
I wouldn't expose it in the header - just do it in the testing file. (With a
comment that it's already defined in implementation)

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

https://codereview.chromium.org/605803002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:31: // of 8px.
Would you terribly mind writing "3" as "8 - 5", so those of us who are
undercaffeinated can correlate the comment and the value?

https://codereview.chromium.org/605803002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:225: // If we have
a single local profile, then use the generic avatar
Tiny nit: "If there is a single...".  "we" is somewhat ambiguous. (Yes, it's
super-nitty, but it fits with Chrome's overall style)

https://codereview.chromium.org/605803002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:230:
cache.GetUserNameOfProfileAtIndex(0).empty());
Question: Does it make sense to move this entire logic onto the Profile class?
It's at least more logical for the ProfileInfoCache to be accessed from there,
and Profile is the source of GuestSessionStatus.

https://codereview.chromium.org/605803002/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/avatar_button_controller_unittest.mm
(right):

https://codereview.chromium.org/605803002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_button_controller_unittest.mm:56: // We
only have one local profile, which means we are displaying the
Nit: un-"we", if possible?

https://codereview.chromium.org/605803002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_button_controller_unittest.mm:58:
EXPECT_EQ(base::string16(), base::SysNSStringToUTF16([button() title]));
EXPECT_NSEQ(@"", [button() title]);

https://codereview.chromium.org/605803002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_button_controller_unittest.mm:65:
EXPECT_FALSE([view() isHidden]);
Why is that an expect? Should that be ASSERT_FALSE? (I.e. it's not part of
what's tested, it's part of the precondition for hte test, right?) Here and
elsewhere, if that assumption is correct.

https://codereview.chromium.org/605803002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_button_controller_unittest.mm:66:
EXPECT_EQ(base::ASCIIToUTF16("Person 1"),
EXPECT_NSEQ - here and elsewhere

https://codereview.chromium.org/605803002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_button_controller_unittest.mm:84:
EXPECT_LT(0, [button() image].size.width);
Isn't that encapsulated in the EXPECT_EQ?

[noms (inactive), 2014-09-29 19:21:57]

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

https://codereview.chromium.org/605803002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:240:
HoverImageButton* button = base::mac::ObjCCast<HoverImageButton>(button_);
On 2014/09/29 17:55:43, groby wrote:
> On 2014/09/26 15:49:10, Monica Dinculescu wrote:
> > button_ is defined in AvatarBaseController. AvatarIconController also
inherits
> > from that, and its button_ is just an NSButton. I guess, in theory I could
> make
> > that a HoverImageButton, though it wouldn't use any of the hover bits.
Should
> I?
> 
> I keep missing that fact, don't I? No, you probably shouldn't. You should
> probably make this ObjCCastStrict, though - it always should be set here, and
it
> always should be a HoverImageButton, right? 
> 
> > 
> > On 2014/09/25 22:03:30, groby wrote:
> > > Why not just make button_ a HoverImageButton? That way, you don't need to
> cast
> > -
> > > and it's not like it can be a different type.
> > 
> 

Done.

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

https://codereview.chromium.org/605803002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:31: // of 8px.
On 2014/09/29 17:55:43, groby wrote:
> Would you terribly mind writing "3" as "8 - 5", so those of us who are
> undercaffeinated can correlate the comment and the value?

Done.

https://codereview.chromium.org/605803002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:225: // If we have
a single local profile, then use the generic avatar
On 2014/09/29 17:55:43, groby wrote:
> Tiny nit: "If there is a single...".  "we" is somewhat ambiguous. (Yes, it's
> super-nitty, but it fits with Chrome's overall style)

Done.

https://codereview.chromium.org/605803002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:230:
cache.GetUserNameOfProfileAtIndex(0).empty());
Eeeek, it doesn't. Profile doesn't really know/speak to the ProfileInfoCache,
and I'm not sure a specific profile should know about what all the other
profiles are doing.

It's really common for UI code to call into the ProfileInfoCache, which is the
source of all information about profiles...(especially if they're not loaded)

On 2014/09/29 17:55:43, groby wrote:
> Question: Does it make sense to move this entire logic onto the Profile class?
> It's at least more logical for the ProfileInfoCache to be accessed from there,
> and Profile is the source of GuestSessionStatus.

https://codereview.chromium.org/605803002/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/avatar_button_controller_unittest.mm
(right):

https://codereview.chromium.org/605803002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_button_controller_unittest.mm:56: // We
only have one local profile, which means we are displaying the
On 2014/09/29 17:55:43, groby wrote:
> Nit: un-"we", if possible?

Done.

https://codereview.chromium.org/605803002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_button_controller_unittest.mm:58:
EXPECT_EQ(base::string16(), base::SysNSStringToUTF16([button() title]));
Oh, cool! Done.
On 2014/09/29 17:55:43, groby wrote:
> EXPECT_NSEQ(@"", [button() title]);

https://codereview.chromium.org/605803002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_button_controller_unittest.mm:65:
EXPECT_FALSE([view() isHidden]);
On 2014/09/29 17:55:43, groby wrote:
> Why is that an expect? Should that be ASSERT_FALSE? (I.e. it's not part of
> what's tested, it's part of the precondition for hte test, right?) Here and
> elsewhere, if that assumption is correct.

Done.

https://codereview.chromium.org/605803002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_button_controller_unittest.mm:66:
EXPECT_EQ(base::ASCIIToUTF16("Person 1"),
On 2014/09/29 17:55:43, groby wrote:
> EXPECT_NSEQ - here and elsewhere

Done.

https://codereview.chromium.org/605803002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_button_controller_unittest.mm:84:
EXPECT_LT(0, [button() image].size.width);
I was really paranoid that maybe IDR_ICON.. would be of 0 size. But that's
insane. Removed.
On 2014/09/29 17:55:43, groby wrote:
> Isn't that encapsulated in the EXPECT_EQ?

[groby-ooo-7-16, 2014-09-29 20:29:41]

lgtm

[noms (inactive), 2014-09-29 20:33:50]

noms@chromium.org changed reviewers:
+ oshima@chromium.org

[noms (inactive), 2014-09-29 20:33:51]

+ oshima for theme resources

[oshima, 2014-09-29 20:41:41]

ui/resources lgtm

[noms (inactive), 2014-09-29 20:45:21]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-09-29 20:47:28]

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

[commit-bot: I haz the power, 2014-09-29 21:00:49]

Committed patchset #4 (id:120001) as 769a9608a50a637cdcd013fd5fad994721e2f814

[commit-bot: I haz the power, 2014-09-29 21:01:31]

Patchset 4 (id:??) landed as
https://crrev.com/c86c1fcf7beec49a7b362208da99904e5aee13fd
Cr-Commit-Position: refs/heads/master@{#297255}