Add right-click user switching tutorial bubble.

Add right-click user switching tutorial bubble.

This design is based on this mock: https://drive.google.com/a/chromium.org/file/d/0B-DVbqI3huZGZGhIcTFtRWRTSTA/view?usp=sharing

BUG=458664
TEST=
1. Launch chrome and make sure there is more than one available profile.
2. Left-click on the Avatar Button.
3. A tutorial bubble should be displayed informing the user they can
   right-click the button to see other users.
4. Close the bubble without dismissing the tutorial, for example by
   clicking in the content area.
5. Reopening the Avatar Menu should still display the tutorial until it
   is explicitly dismissed (clicking the X or the "Ok, got it!" button)
   or until the user right clicks the button.
6. Once dismissed, the tutorial should never be shown again.

Committed: https://crrev.com/b7bdcdec6cbab0f4b395f35100954e4a47820194
Cr-Commit-Position: refs/heads/master@{#330565}

[anthonyvd, 2015-05-04 20:49:11]

anthonyvd@chromium.org changed reviewers:
+ groby@chromium.org, mlerman@chromium.org, sky@chromium.org

[anthonyvd, 2015-05-04 20:49:12]

Hello everyone! This changelist that adds a message informing the user of the
existence of right-click user switching on the Avatar Button. The feature has
been enabled since M42 but we want to tell users about it before the alternative
is removed (the #enable-fast-user-switching flag).

Could you please take a look at:

mlerman@: chrome/browser/profiles/*
groby@: chrome/browser/ui/cocoa/*
sky@: chrome/browser/ui/*

Thanks a lot, it's much appreciated :)

[Mike Lerman, 2015-05-05 01:31:45]

Hi Anthony,

Excited to see this tutorial coming to fruition!

https://codereview.chromium.org/1120013003/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1120013003/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:12077: +        Right click on the button
above to view other people.
Is there a design doc or folio that provides the strings and design? If so, can
you please link it either in the CL Description or as a comment in the bug?

https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.cc:57: registry->RegisterBooleanPref(
Let's register this near where the other tutorial prefs are - in profile.cc,
line 149. Where prefs::kProfileAvatarTutorialShown, is registered.

https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.cc:243: void
SetFastUserSwitchingTutorialDismissedState(bool dismissed) {
We usually just directly access this type of preference directly. Any reason we
need to wrap that access in these two functions?

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

https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1281: if
(isFastProfileChooser) {
nit: inline the condition.

https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/ui/profi...
File chrome/browser/ui/profile_chooser_constants.h (right):

https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/ui/profi...
chrome/browser/ui/profile_chooser_constants.h:41:
TUTORIAL_MODE_RIGHT_CLICK_SWITCHING,
Hmm. We should have UMA stats around this tutorial: When it's Shown and when
people Acknowledge it. Similar to Profile.NewAvatarMenu.NotYou/Upgrade/Signin

https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:923: if
(!tutorial_view)
I understand what you're doing here, but the control flow doesn't seem obvious. 

As one example, this would be much more readable if there were a function that
determined which tutorial to display, and then a function that displayed the
tutorial. There may be other flows that work, too.

[anthonyvd, 2015-05-05 16:19:17]

Thanks for your feedback Mike!

https://codereview.chromium.org/1120013003/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1120013003/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:12077: +        Right click on the button
above to view other people.
On 2015/05/05 01:31:44, Mike Lerman wrote:
> Is there a design doc or folio that provides the strings and design? If so,
can
> you please link it either in the CL Description or as a comment in the bug? 

John emailed an image to me. Can I put it on drive and go/ link to it?

https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.cc:57: registry->RegisterBooleanPref(
On 2015/05/05 01:31:45, Mike Lerman wrote:
> Let's register this near where the other tutorial prefs are - in profile.cc,
> line 149. Where prefs::kProfileAvatarTutorialShown, is registered.

I put it here because the tutorial prefs in profile.cc are profile-specific
where this one has to be consistent across all profiles and this is where we
seem to register those.

https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.cc:243: void
SetFastUserSwitchingTutorialDismissedState(bool dismissed) {
On 2015/05/05 01:31:45, Mike Lerman wrote:
> We usually just directly access this type of preference directly. Any reason
we
> need to wrap that access in these two functions?

I guess it just... happened. There no need really, so they're now accessed
directly!

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

https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1281: if
(isFastProfileChooser) {
On 2015/05/05 01:31:45, Mike Lerman wrote:
> nit: inline the condition.

It's used again a little further down.

https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/ui/profi...
File chrome/browser/ui/profile_chooser_constants.h (right):

https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/ui/profi...
chrome/browser/ui/profile_chooser_constants.h:41:
TUTORIAL_MODE_RIGHT_CLICK_SWITCHING,
On 2015/05/05 01:31:45, Mike Lerman wrote:
> Hmm. We should have UMA stats around this tutorial: When it's Shown and when
> people Acknowledge it. Similar to Profile.NewAvatarMenu.NotYou/Upgrade/Signin

Not that I'm against that but how actionable/insight-providing would those stats
be?

We've already decided to require that the user dismiss the tutorial manually so
the data would show #times shown/#times dismissed (basically the amount of times
people see it before they get that they need to dismiss). That would probably be
interesting insight but I'm wondering if it's worth the additional metric
considering it doesn't lead to any follow up. Do you have anything else in mind
that I'm not thinking about?

https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:923: if
(!tutorial_view)
On 2015/05/05 01:31:45, Mike Lerman wrote:
> I understand what you're doing here, but the control flow doesn't seem
obvious. 
> 
> As one example, this would be much more readable if there were a function that
> determined which tutorial to display, and then a function that displayed the
> tutorial. There may be other flows that work, too.

Very good point, here's a small refactor :) Let me know what you think!

[Mike Lerman, 2015-05-05 17:18:12]

Thanks! profiles lgtm.

https://codereview.chromium.org/1120013003/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1120013003/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:12077: +        Right click on the button
above to view other people.
On 2015/05/05 16:19:17, anthonyvd wrote:
> On 2015/05/05 01:31:44, Mike Lerman wrote:
> > Is there a design doc or folio that provides the strings and design? If so,
> can
> > you please link it either in the CL Description or as a comment in the bug? 
> 
> John emailed an image to me. Can I put it on drive and go/ link to it?

Don't even need a go-link. Just a link to drive would be useful for
verification. Thanks!

https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.cc:57: registry->RegisterBooleanPref(
On 2015/05/05 16:19:17, anthonyvd wrote:
> On 2015/05/05 01:31:45, Mike Lerman wrote:
> > Let's register this near where the other tutorial prefs are - in profile.cc,
> > line 149. Where prefs::kProfileAvatarTutorialShown, is registered.
> 
> I put it here because the tutorial prefs in profile.cc are profile-specific
> where this one has to be consistent across all profiles and this is where we
> seem to register those. 

So we show it once per Chrome, not one per Profile? Wouldn't each profile
(meat-person) want to see the tutorial? Although I guess this is targeting power
users... so I can see how it makes sense to be per-Chrome.

https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:923: if
(!tutorial_view)
On 2015/05/05 16:19:17, anthonyvd wrote:
> On 2015/05/05 01:31:45, Mike Lerman wrote:
> > I understand what you're doing here, but the control flow doesn't seem
> obvious. 
> > 
> > As one example, this would be much more readable if there were a function
that
> > determined which tutorial to display, and then a function that displayed the
> > tutorial. There may be other flows that work, too.
> 
> Very good point, here's a small refactor :) Let me know what you think!

I love it! Thanks!

https://codereview.chromium.org/1120013003/diff/80001/chrome/browser/profiles...
File chrome/browser/profiles/profiles_state.h (right):

https://codereview.chromium.org/1120013003/diff/80001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.h:96: void
SetFastUserSwitchingTutorialDismissedState(bool dismissed);
You can remove these calls now.

[groby-ooo-7-16, 2015-05-06 17:25:04]

On 2015/05/05 17:18:12, Mike Lerman wrote:
> Thanks! profiles lgtm.
> 
>
https://codereview.chromium.org/1120013003/diff/40001/chrome/app/generated_re...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/1120013003/diff/40001/chrome/app/generated_re...
> chrome/app/generated_resources.grd:12077: +        Right click on the button
> above to view other people.
> On 2015/05/05 16:19:17, anthonyvd wrote:
> > On 2015/05/05 01:31:44, Mike Lerman wrote:
> > > Is there a design doc or folio that provides the strings and design? If
so,
> > can
> > > you please link it either in the CL Description or as a comment in the
bug? 
> > 
> > John emailed an image to me. Can I put it on drive and go/ link to it?
> 
> Don't even need a go-link. Just a link to drive would be useful for
> verification. Thanks!

Please put it in a place that's readable for chromium.org contributors in
general

> 
>
https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/profiles...
> File chrome/browser/profiles/profiles_state.cc (right):
> 
>
https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/profiles...
> chrome/browser/profiles/profiles_state.cc:57: registry->RegisterBooleanPref(
> On 2015/05/05 16:19:17, anthonyvd wrote:
> > On 2015/05/05 01:31:45, Mike Lerman wrote:
> > > Let's register this near where the other tutorial prefs are - in
profile.cc,
> > > line 149. Where prefs::kProfileAvatarTutorialShown, is registered.
> > 
> > I put it here because the tutorial prefs in profile.cc are profile-specific
> > where this one has to be consistent across all profiles and this is where we
> > seem to register those. 
> 
> So we show it once per Chrome, not one per Profile? Wouldn't each profile
> (meat-person) want to see the tutorial? Although I guess this is targeting
power
> users... so I can see how it makes sense to be per-Chrome.

Can we have a comment to that effect?

> 
>
https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/ui/views...
> File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):
> 
>
https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/ui/views...
> chrome/browser/ui/views/profiles/profile_chooser_view.cc:923: if
> (!tutorial_view)
> On 2015/05/05 16:19:17, anthonyvd wrote:
> > On 2015/05/05 01:31:45, Mike Lerman wrote:
> > > I understand what you're doing here, but the control flow doesn't seem
> > obvious. 
> > > 
> > > As one example, this would be much more readable if there were a function
> that
> > > determined which tutorial to display, and then a function that displayed
the
> > > tutorial. There may be other flows that work, too.
> > 
> > Very good point, here's a small refactor :) Let me know what you think!
> 
> I love it! Thanks!
> 
>
https://codereview.chromium.org/1120013003/diff/80001/chrome/browser/profiles...
> File chrome/browser/profiles/profiles_state.h (right):
> 
>
https://codereview.chromium.org/1120013003/diff/80001/chrome/browser/profiles...
> chrome/browser/profiles/profiles_state.h:96: void
> SetFastUserSwitchingTutorialDismissedState(bool dismissed);
> You can remove these calls now.

[anthonyvd, 2015-05-06 17:40:16]

On 2015/05/06 17:25:04, groby wrote:
> On 2015/05/05 17:18:12, Mike Lerman wrote:
> > Thanks! profiles lgtm.
> > 
> >
>
https://codereview.chromium.org/1120013003/diff/40001/chrome/app/generated_re...
> > File chrome/app/generated_resources.grd (right):
> > 
> >
>
https://codereview.chromium.org/1120013003/diff/40001/chrome/app/generated_re...
> > chrome/app/generated_resources.grd:12077: +        Right click on the button
> > above to view other people.
> > On 2015/05/05 16:19:17, anthonyvd wrote:
> > > On 2015/05/05 01:31:44, Mike Lerman wrote:
> > > > Is there a design doc or folio that provides the strings and design? If
> so,
> > > can
> > > > you please link it either in the CL Description or as a comment in the
> bug? 
> > > 
> > > John emailed an image to me. Can I put it on drive and go/ link to it?
> > 
> > Don't even need a go-link. Just a link to drive would be useful for
> > verification. Thanks!
> 
> Please put it in a place that's readable for http://chromium.org contributors
in
> general
> 
> > 
> >
>
https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/profiles...
> > File chrome/browser/profiles/profiles_state.cc (right):
> > 
> >
>
https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/profiles...
> > chrome/browser/profiles/profiles_state.cc:57: registry->RegisterBooleanPref(
> > On 2015/05/05 16:19:17, anthonyvd wrote:
> > > On 2015/05/05 01:31:45, Mike Lerman wrote:
> > > > Let's register this near where the other tutorial prefs are - in
> profile.cc,
> > > > line 149. Where prefs::kProfileAvatarTutorialShown, is registered.
> > > 
> > > I put it here because the tutorial prefs in profile.cc are
profile-specific
> > > where this one has to be consistent across all profiles and this is where
we
> > > seem to register those. 
> > 
> > So we show it once per Chrome, not one per Profile? Wouldn't each profile
> > (meat-person) want to see the tutorial? Although I guess this is targeting
> power
> > users... so I can see how it makes sense to be per-Chrome.
> 
> Can we have a comment to that effect?
> 
> > 
> >
>
https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/ui/views...
> > File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):
> > 
> >
>
https://codereview.chromium.org/1120013003/diff/40001/chrome/browser/ui/views...
> > chrome/browser/ui/views/profiles/profile_chooser_view.cc:923: if
> > (!tutorial_view)
> > On 2015/05/05 16:19:17, anthonyvd wrote:
> > > On 2015/05/05 01:31:45, Mike Lerman wrote:
> > > > I understand what you're doing here, but the control flow doesn't seem
> > > obvious. 
> > > > 
> > > > As one example, this would be much more readable if there were a
function
> > that
> > > > determined which tutorial to display, and then a function that displayed
> the
> > > > tutorial. There may be other flows that work, too.
> > > 
> > > Very good point, here's a small refactor :) Let me know what you think!
> > 
> > I love it! Thanks!
> > 
> >
>
https://codereview.chromium.org/1120013003/diff/80001/chrome/browser/profiles...
> > File chrome/browser/profiles/profiles_state.h (right):
> > 
> >
>
https://codereview.chromium.org/1120013003/diff/80001/chrome/browser/profiles...
> > chrome/browser/profiles/profiles_state.h:96: void
> > SetFastUserSwitchingTutorialDismissedState(bool dismissed);
> > You can remove these calls now.

The mock is now in the CL description. The X was changed to an "Ok, got it"
button at the request of John, our PM: "re: ok, got it- checked w/ Alan, and he
agrees, but also says we shouldn't have duplicate targets.  We should nix the
"x" and keep "okay, got it!"". The rationale is that we're forcing an action
(the prompt is shown until dismissed) so it should be more obvious than jsut the
X.

Re: Showing per-chrome instead of per-profile: It was initially set up to be
per-profile but it felt spammy since we're forcing the user to dismiss it. It
was changed to once per-chrome at John's request as well.

[groby-ooo-7-16, 2015-05-07 01:59:02]

Mostly there, but can I please get some tests?

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

https://codereview.chromium.org/1120013003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1452: return
tutorialMode_ == profiles::TUTORIAL_MODE_WELCOME_UPGRADE ||
That's returning a bool, not BOOL.  Since you never compare the return value to
YES/NO, I'd suggest just using bool as return type.

https://codereview.chromium.org/1120013003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1520: if ([self
shouldShowWelcomeUpgradeTutorial]) {
If I read this correctly, this code only updates the counter if we're not
already in welcomeUpgradeTutorial mode, but it always builds the view.

I'm not quite clear how you could end up in that situation - could you
elaborate?

[anthonyvd, 2015-05-12 16:09:11]

Comments addressed, tests added!

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

https://codereview.chromium.org/1120013003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1452: return
tutorialMode_ == profiles::TUTORIAL_MODE_WELCOME_UPGRADE ||
On 2015/05/07 01:59:01, groby wrote:
> That's returning a bool, not BOOL.  Since you never compare the return value
to
> YES/NO, I'd suggest just using bool as return type.

Thanks! Done.

https://codereview.chromium.org/1120013003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1520: if ([self
shouldShowWelcomeUpgradeTutorial]) {
On 2015/05/07 01:59:01, groby wrote:
> If I read this correctly, this code only updates the counter if we're not
> already in welcomeUpgradeTutorial mode, but it always builds the view.
> 
> I'm not quite clear how you could end up in that situation - could you
> elaborate?

I moved this code as-is from line 1439 of the previous version but my
understanding is that building the tutorial view sets tutorialMode_ to
profiles::TUTORIAL_MODE_WELCOME_UPGRADE. Other code can rebuild the menu with
the current tutorial mode without user interaction (for example to refresh it)
and that shouldn't affect the "shown" counter so it's only incremented when we
actually take the decision to go into that mode.

[groby-ooo-7-16, 2015-05-12 19:27:58]

Thank you for the tests!

c/b/ui/cocoa LGTM with one nit question.

https://codereview.chromium.org/1120013003/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm
(right):

https://codereview.chromium.org/1120013003/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:112:
NSTextField* tutorialTitle = base::mac::ObjCCast<NSTextField>(
Should those be ObjCCastStrict?

[sky, 2015-05-14 17:34:56]

https://codereview.chromium.org/1120013003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/1120013003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.h:171: bool
ShouldShowWelcomeUpgradeTutorial();
Are these ShouldShowFoo functions really platform specific? Should they be
promoted to a more common place?

[Mike Lerman, 2015-05-14 17:39:58]

https://codereview.chromium.org/1120013003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/1120013003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.h:171: bool
ShouldShowWelcomeUpgradeTutorial();
On 2015/05/14 17:34:56, sky wrote:
> Are these ShouldShowFoo functions really platform specific? Should they be
> promoted to a more common place?

Oooh I like! That more common place would be profile_window.h/cc.

[anthonyvd, 2015-05-14 20:24:17]

https://codereview.chromium.org/1120013003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/1120013003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.h:171: bool
ShouldShowWelcomeUpgradeTutorial();
On 2015/05/14 17:34:56, sky wrote:
> Are these ShouldShowFoo functions really platform specific? Should they be
> promoted to a more common place?

Good point, done!

[Mike Lerman, 2015-05-14 20:29:43]

Profiles Still LGTM.

https://codereview.chromium.org/1120013003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/1120013003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:1599: bool
ProfileChooserView::ShouldShowWelcomeUpgradeTutorial() {
You can remove these method definitions from the header file. (this function,
and below, from the View and the Controller)

[sky, 2015-05-14 21:22:03]

https://codereview.chromium.org/1120013003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/1120013003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.h:171: bool
ShouldShowWelcomeUpgradeTutorial();
These two aren't needed anymore, right?

[anthonyvd, 2015-05-15 13:26:57]

Comments addressed, thanks.

https://codereview.chromium.org/1120013003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/1120013003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:1599: bool
ProfileChooserView::ShouldShowWelcomeUpgradeTutorial() {
On 2015/05/14 20:29:43, Mike Lerman wrote:
> You can remove these method definitions from the header file. (this function,
> and below, from the View and the Controller)

Done.

https://codereview.chromium.org/1120013003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/1120013003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.h:171: bool
ShouldShowWelcomeUpgradeTutorial();
On 2015/05/14 21:22:03, sky wrote:
> These two aren't needed anymore, right?

Done.

[groby-ooo-7-16, 2015-05-15 18:49:33]

ping re: nit

[anthonyvd, 2015-05-15 18:52:31]

On 2015/05/15 18:49:33, groby wrote:
> ping re: nit

Oops sorry about that I totally forgot to reply to your comment! Yes, I imagine
they should be. There's no reason for those controls to be any other types and
we'd probably want the test to DCHECK if they're not. I'll make the change.

[groby-ooo-7-16, 2015-05-15 19:01:39]

Awesome, thank you. (And of course, c/b/ui/cocoa still LGTM)

[anthonyvd, 2015-05-15 19:25:27]

Thanks to you!

https://codereview.chromium.org/1120013003/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm
(right):

https://codereview.chromium.org/1120013003/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:112:
NSTextField* tutorialTitle = base::mac::ObjCCast<NSTextField>(
On 2015/05/12 19:27:57, groby wrote:
> Should those be ObjCCastStrict?

Done.

[anthonyvd, 2015-05-15 19:49:15]

The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run

[anthonyvd, 2015-05-15 19:49:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlerman@chromium.org, groby@chromium.org
Link to the patchset: https://codereview.chromium.org/1120013003/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2015-05-15 19:52:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1120013003/180001

[commit-bot: I haz the power, 2015-05-15 21:03:19]

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

[commit-bot: I haz the power, 2015-05-15 21:03:20]

Dry run: This issue passed the CQ dry run.

[sky, 2015-05-18 17:07:06]

LGTM

[anthonyvd, 2015-05-19 17:29:02]

The CQ bit was checked by anthonyvd@chromium.org

[commit-bot: I haz the power, 2015-05-19 17:30:08]

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

[commit-bot: I haz the power, 2015-05-19 18:31:38]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-05-19 18:32:30]

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