Access to Chrome via the System Tray should go through the User Manager.

Access to Chrome via the System Tray should go through the User Manager.

If the most recent profile is locked and the user uses the Windows
system tray to access the Task Manager or About Chrome, the user will
now first be presented with the User Manager, and only after selecting
or unlocking a profile will they be brought automatically to their
original destination.

BUG=409030
TEST=Enable --new-profile-management. Have at least one background app
running under your profile (e.g. Hangouts). Close Chrome. In the
system tray, Chrome ->Task Manager or About Chrome. Only after unlocking
your profile or opening a new profile, you'll see the original selection.

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

[Mike Lerman, 2014-09-11 15:06:54]

mlerman@chromium.org changed reviewers:
+ atwilson@chromium.org, estade@chromium.org, noms@chromium.org,
pkasting@chromium.org

[Mike Lerman, 2014-09-11 15:06:55]

Kind and wonderful reviewers,

Please cast your eyes upon this CL. May it be found worthy for your
consideration.

atwilson@chromium.org: c/b/background
noms@chromium.org: c/b/profiles and c/b/ui/webui/signin/
pkasting@chromium.org: c/b/ui, c/b/ui/views/profiles
estade@chromium.org: c/b/resources/ and also your opinions on
c/b/ui/webui/signin

Thanks!

[Evan Stade, 2014-09-11 17:02:51]

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/user_manager/user_manager.js (right):

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/resources...
chrome/browser/resources/user_manager/user_manager.js:146:
chrome.send('userManagerInitialize');
chrome.send('userManagerInitialize', [window.location.hash]);

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:675:
base::Unretained(this)),
I don't think unretained is right here. Are you guaranteed that this
UserManagerScreenHandler will outlive whatever is doing the profile switching? I
think not (unless this callback is run synchronously)... you probably need a
weak ptr.

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:693: // destruct
the UserManagerScreenHandler as well.
I don't quite understand. HideUserManager might destroy |this|, but it's
delayed. So you should be able to finish out this function before being
destroyed, hence you don't need to post on L702 or L710.

I don't even know why you're posting HideUserManager. Why not just put it at the
end of this function?

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:703: FROM_HERE,
base::Bind(&chrome::ShowTaskManager, browser));
I'm worried that |browser| might be invalid by the time ShowTaskManager is
called.

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:705: url_hash_ ==
move to same line as if

[Mike Lerman, 2014-09-11 18:49:30]

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/user_manager/user_manager.js (right):

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/resources...
chrome/browser/resources/user_manager/user_manager.js:146:
chrome.send('userManagerInitialize');
On 2014/09/11 17:02:50, Evan Stade wrote:
> chrome.send('userManagerInitialize', [window.location.hash]);

Done.

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:675:
base::Unretained(this)),
On 2014/09/11 17:02:51, Evan Stade wrote:
> I don't think unretained is right here. Are you guaranteed that this
> UserManagerScreenHandler will outlive whatever is doing the profile switching?
I
> think not (unless this callback is run synchronously)... you probably need a
> weak ptr.

The User Manager will certainly always wait around until the profiles:: code
executes (synchronously or otherwise). If not, Chrome would shut itself down
(especially on Windows) as there'd be no active Profiles or Browsers. 

noms@ can confirm this.

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:693: // destruct
the UserManagerScreenHandler as well.
On 2014/09/11 17:02:50, Evan Stade wrote:
> I don't quite understand. HideUserManager might destroy |this|, but it's
> delayed. So you should be able to finish out this function before being
> destroyed, hence you don't need to post on L702 or L710.
> 
> I don't even know why you're posting HideUserManager. Why not just put it at
the
> end of this function?

More in depth explanation is here, where the code was implemented this way:
https://codereview.chromium.org/149883004

This method (OnSwitchToProfileComplete) can be called synchronously (see line
380 above as an example) within this object, meaning this object needs to stick
around a bit longer.

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:703: FROM_HERE,
base::Bind(&chrome::ShowTaskManager, browser));
On 2014/09/11 17:02:51, Evan Stade wrote:
> I'm worried that |browser| might be invalid by the time ShowTaskManager is
> called.

This is for the new browser object that's created for the just-opening profile.
Unless I call ShowTaskManager synchronously, this would always be a concern.

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:705: url_hash_ ==
On 2014/09/11 17:02:51, Evan Stade wrote:
> move to same line as if

Done.

[Evan Stade, 2014-09-11 19:02:52]

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:675:
base::Unretained(this)),
On 2014/09/11 18:49:29, Mike Lerman wrote:
> On 2014/09/11 17:02:51, Evan Stade wrote:
> > I don't think unretained is right here. Are you guaranteed that this
> > UserManagerScreenHandler will outlive whatever is doing the profile
switching?
> I
> > think not (unless this callback is run synchronously)... you probably need a
> > weak ptr.
> 
> The User Manager will certainly always wait around until the profiles:: code
> executes (synchronously or otherwise). If not, Chrome would shut itself down
> (especially on Windows) as there'd be no active Profiles or Browsers. 

During shutdown is the time I'd be most worried about this. Anyway, this isn't
really a question of "expected behavior", it's a question of object ownership.
If |this| mysteriously goes away, this callback needs to know about it. You need
a weak ptr.

> 
> noms@ can confirm this.

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:693: // destruct
the UserManagerScreenHandler as well.
On 2014/09/11 18:49:29, Mike Lerman wrote:
> On 2014/09/11 17:02:50, Evan Stade wrote:
> > I don't quite understand. HideUserManager might destroy |this|, but it's
> > delayed. So you should be able to finish out this function before being
> > destroyed, hence you don't need to post on L702 or L710.
> > 
> > I don't even know why you're posting HideUserManager. Why not just put it at
> the
> > end of this function?
> 
> More in depth explanation is here, where the code was implemented this way:
> https://codereview.chromium.org/149883004
> 
> This method (OnSwitchToProfileComplete) can be called synchronously (see line
> 380 above as an example) within this object, meaning this object needs to
stick
> around a bit longer.

I don't see how L380 or any use is synchronous?

Even if |this| is destroyed, you just have to make sure not to reference |this|
afterwards. i.e. calling OnSwitchToProfileComplete synchronously would be OK as
long as it's the last thing you do in that method.

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:703: FROM_HERE,
base::Bind(&chrome::ShowTaskManager, browser));
On 2014/09/11 18:49:29, Mike Lerman wrote:
> On 2014/09/11 17:02:51, Evan Stade wrote:
> > I'm worried that |browser| might be invalid by the time ShowTaskManager is
> > called.
> 
> This is for the new browser object that's created for the just-opening
profile.
> Unless I call ShowTaskManager synchronously, this would always be a concern.

I do think you should call ShowTaskManager synchronously. But if you had to do
this async, you could still post a task which first gets the browser then
synchronously calls showTaskManager with it.

[noms (inactive), 2014-09-11 19:05:41]

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:675:
base::Unretained(this)),
I think we could get in a weird situation where you can start switching to a
profile and provided that takes super long, or you are super fast, you could
then manually close the UserManager. That would maybe open a bit of a hellmouth.
Just to be on the safe side, I would agree with Evan and make this a weak ptr.

On 2014/09/11 18:49:29, Mike Lerman wrote:
> On 2014/09/11 17:02:51, Evan Stade wrote:
> > I don't think unretained is right here. Are you guaranteed that this
> > UserManagerScreenHandler will outlive whatever is doing the profile
switching?
> I
> > think not (unless this callback is run synchronously)... you probably need a
> > weak ptr.
> 
> The User Manager will certainly always wait around until the profiles:: code
> executes (synchronously or otherwise). If not, Chrome would shut itself down
> (especially on Windows) as there'd be no active Profiles or Browsers. 
> 
> noms@ can confirm this.

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:703: FROM_HERE,
base::Bind(&chrome::ShowTaskManager, browser));
I'm not sure why we need to PostTask this one. What happens if you don't, and
just call ShowTaskManager?

On 2014/09/11 18:49:29, Mike Lerman wrote:
> On 2014/09/11 17:02:51, Evan Stade wrote:
> > I'm worried that |browser| might be invalid by the time ShowTaskManager is
> > called.
> 
> This is for the new browser object that's created for the just-opening
profile.
> Unless I call ShowTaskManager synchronously, this would always be a concern.

https://codereview.chromium.org/564453003/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/564453003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile.h:112: CREATE_STATUS_ALREADY_CREATED,
Hmm, why can't you reuse CREATE_STATUS_INITIALIZED? That one is used when you're
trying to create a profile that's already created as well, I think.

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

https://codereview.chromium.org/564453003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_window.cc:156: page += "#tutorial";
nit: maybe make this into a constant next to the other two as well?

[Peter Kasting, 2014-09-11 20:35:02]

https://codereview.chromium.org/564453003/diff/40001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_dialogs.h (right):

https://codereview.chromium.org/564453003/diff/40001/chrome/browser/ui/browse...
chrome/browser/ui/browser_dialogs.h:112: void ShowUserManagerThenAboutChrome();
In general, I think this whole header was a mistake.  We decided a while back to
get rid of the chrome:: namespace, and these are basically global functions at
that point.  So I'd try to avoid adding to it.

I'd prefer to just expose public static methods in relevant classes.  For
example, you could perhaps replace UserManagerView::Show() with these two
methods.  Or you could have callers just call Show() directly with the right two
arguments.

[Mike Lerman, 2014-09-11 20:51:51]

Peter, I just saw your comment and will address it soon. In brief, I believe the
Chrome:: header is necessary because the calls in browser_dialogs.h are
implemented in both UserManagerMac and UserManagerViews, and I need a common
place to call from. There's already a class called user_manager_ui in
c/b/ui/webui/signin, but could add another class with static UserManager methods
in c/b/ui. 

On the flip side, you helped me realize that I haven't added the necessary calls
to UserManagerMac. I'll do so tomorrow morning. Thanks!

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:675:
base::Unretained(this)),
On 2014/09/11 19:05:41, Monica Dinculescu wrote:
> I think we could get in a weird situation where you can start switching to a
> profile and provided that takes super long, or you are super fast, you could
> then manually close the UserManager. That would maybe open a bit of a
hellmouth.
> Just to be on the safe side, I would agree with Evan and make this a weak ptr.
> 
> On 2014/09/11 18:49:29, Mike Lerman wrote:
> > On 2014/09/11 17:02:51, Evan Stade wrote:
> > > I don't think unretained is right here. Are you guaranteed that this
> > > UserManagerScreenHandler will outlive whatever is doing the profile
> switching?
> > I
> > > think not (unless this callback is run synchronously)... you probably need
a
> > > weak ptr.
> > 
> > The User Manager will certainly always wait around until the profiles:: code
> > executes (synchronously or otherwise). If not, Chrome would shut itself down
> > (especially on Windows) as there'd be no active Profiles or Browsers. 
> > 
> > noms@ can confirm this.
> 

Done.

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:693: // destruct
the UserManagerScreenHandler as well.
On 2014/09/11 19:02:52, Evan Stade wrote:
> On 2014/09/11 18:49:29, Mike Lerman wrote:
> > On 2014/09/11 17:02:50, Evan Stade wrote:
> > > I don't quite understand. HideUserManager might destroy |this|, but it's
> > > delayed. So you should be able to finish out this function before being
> > > destroyed, hence you don't need to post on L702 or L710.
> > > 
> > > I don't even know why you're posting HideUserManager. Why not just put it
at
> > the
> > > end of this function?
> > 
> > More in depth explanation is here, where the code was implemented this way:
> > https://codereview.chromium.org/149883004
> > 
> > This method (OnSwitchToProfileComplete) can be called synchronously (see
line
> > 380 above as an example) within this object, meaning this object needs to
> stick
> > around a bit longer.
> 
> I don't see how L380 or any use is synchronous?
> 
> Even if |this| is destroyed, you just have to make sure not to reference
|this|
> afterwards. i.e. calling OnSwitchToProfileComplete synchronously would be OK
as
> long as it's the last thing you do in that method.
> 

L380 can execute the following chain of calls synchronously:
SwitchToGuestProfile() -> CreateProfileAsync() -> If the ProfileInfo exists
(profile already existed), execute the Callback directly.

HideUserManager needs to be posted because the new browser's window hasn't been
created yet, and Chrome (racily) can close if there are no windows open.

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:703: FROM_HERE,
base::Bind(&chrome::ShowTaskManager, browser));
On 2014/09/11 19:05:41, Monica Dinculescu wrote:
> I'm not sure why we need to PostTask this one. What happens if you don't, and
> just call ShowTaskManager?
> 
> On 2014/09/11 18:49:29, Mike Lerman wrote:
> > On 2014/09/11 17:02:51, Evan Stade wrote:
> > > I'm worried that |browser| might be invalid by the time ShowTaskManager is
> > > called.
> > 
> > This is for the new browser object that's created for the just-opening
> profile.
> > Unless I call ShowTaskManager synchronously, this would always be a concern.
> 

The TaskManager requires the browser's window to be created. At the time of
killing OnSwitchToProfileComplete, I always have a Browser pointer, but don't
always have a Window. Thus, I post. (updated the comment).

https://codereview.chromium.org/564453003/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/564453003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile.h:112: CREATE_STATUS_ALREADY_CREATED,
On 2014/09/11 19:05:41, Monica Dinculescu wrote:
> Hmm, why can't you reuse CREATE_STATUS_INITIALIZED? That one is used when
you're
> trying to create a profile that's already created as well, I think.

okay.

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

https://codereview.chromium.org/564453003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_window.cc:156: page += "#tutorial";
On 2014/09/11 19:05:41, Monica Dinculescu wrote:
> nit: maybe make this into a constant next to the other two as well?

Sure.

[Evan Stade, 2014-09-11 21:30:00]

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:703: FROM_HERE,
base::Bind(&chrome::ShowTaskManager, browser));
On 2014/09/11 20:51:51, Mike Lerman wrote:
> On 2014/09/11 19:05:41, Monica Dinculescu wrote:
> > I'm not sure why we need to PostTask this one. What happens if you don't,
and
> > just call ShowTaskManager?
> > 
> > On 2014/09/11 18:49:29, Mike Lerman wrote:
> > > On 2014/09/11 17:02:51, Evan Stade wrote:
> > > > I'm worried that |browser| might be invalid by the time ShowTaskManager
is
> > > > called.
> > > 
> > > This is for the new browser object that's created for the just-opening
> > profile.
> > > Unless I call ShowTaskManager synchronously, this would always be a
concern.
> > 
> 
> The TaskManager requires the browser's window to be created. At the time of
> killing OnSwitchToProfileComplete, I always have a Browser pointer, but don't
> always have a Window.

Did you consider listening for NOTIFICATION_BROWSER_WINDOW_READY?

> Thus, I post. (updated the comment).

Passing |browser| like this is still not safe.

[Mike Lerman, 2014-09-12 14:26:42]

Peter a follow-up question for you.

Evan, your comment's addressed now.

Thanks!

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/564453003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:703: FROM_HERE,
base::Bind(&chrome::ShowTaskManager, browser));
On 2014/09/11 21:30:00, Evan Stade wrote:
> On 2014/09/11 20:51:51, Mike Lerman wrote:
> > On 2014/09/11 19:05:41, Monica Dinculescu wrote:
> > > I'm not sure why we need to PostTask this one. What happens if you don't,
> and
> > > just call ShowTaskManager?
> > > 
> > > On 2014/09/11 18:49:29, Mike Lerman wrote:
> > > > On 2014/09/11 17:02:51, Evan Stade wrote:
> > > > > I'm worried that |browser| might be invalid by the time
ShowTaskManager
> is
> > > > > called.
> > > > 
> > > > This is for the new browser object that's created for the just-opening
> > > profile.
> > > > Unless I call ShowTaskManager synchronously, this would always be a
> concern.
> > > 
> > 
> > The TaskManager requires the browser's window to be created. At the time of
> > killing OnSwitchToProfileComplete, I always have a Browser pointer, but
don't
> > always have a Window.
> 
> Did you consider listening for NOTIFICATION_BROWSER_WINDOW_READY?
> 
> > Thus, I post. (updated the comment).
> 
> Passing |browser| like this is still not safe.

New implementation! Huzzah.

I now look at the browser. If it already has a window that means it was open
previously, and I will call the necessary tasks synchronously (which should be
safe, no?). If not, then I listed to the browser window notification and then
synchronously execute all tasks.

What say you?

https://codereview.chromium.org/564453003/diff/40001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_dialogs.h (right):

https://codereview.chromium.org/564453003/diff/40001/chrome/browser/ui/browse...
chrome/browser/ui/browser_dialogs.h:112: void ShowUserManagerThenAboutChrome();
On 2014/09/11 20:35:02, Peter Kasting wrote:
> In general, I think this whole header was a mistake.  We decided a while back
to
> get rid of the chrome:: namespace, and these are basically global functions at
> that point.  So I'd try to avoid adding to it.
> 
> I'd prefer to just expose public static methods in relevant classes.  For
> example, you could perhaps replace UserManagerView::Show() with these two
> methods.  Or you could have callers just call Show() directly with the right
two
> arguments.

I addressed most of this in the CL reply. This is differentiated because there
is also UserManagerMac::Show(), and we need a point that can direct both ways. I
can create a UserManagerBase class in chrome/browser/ui with static methods and
#ifdef's implementation that calls UserManagerMac::Show or UserManagerView::Show
as appropriate. Is that the direction you're thinking this should go?

[noms (inactive), 2014-09-12 14:44:16]

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

https://codereview.chromium.org/564453003/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:42:
UserManagerMac::Show(base::FilePath(),
Peter's comment aside. I would combine these two into something like
ShowUserManagerThenTakeAction, at least.

https://codereview.chromium.org/564453003/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:53: void
ShowUserManagerThenTaskManager
is this a typo?

https://codereview.chromium.org/564453003/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/564453003/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:693: if (url_hash_
==
Why don't you just save the hash as an std::string the first time?

[Evan Stade, 2014-09-12 21:33:58]

cool

https://codereview.chromium.org/564453003/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/564453003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:711: if (browser)
{
just DCHECK(browser) at the top of the function, no need to handle it here

https://codereview.chromium.org/564453003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:726:
OnBrowserWindowReady(content::Source<Browser>(source).ptr());
how are you sure this is the right browser? I think you need to check
BrowserMatches, or something. (What creates the browser in the browser == NULL
case in OnSwitchToProfileComplete?)

[Peter Kasting, 2014-09-12 22:23:02]

On 2014/09/12 14:26:42, Mike Lerman wrote:
> I addressed most of this in the CL reply. This is differentiated because there
> is also UserManagerMac::Show(), and we need a point that can direct both ways.
I
> can create a UserManagerBase class in chrome/browser/ui with static methods
and
> #ifdef's implementation that calls UserManagerMac::Show or
UserManagerView::Show
> as appropriate. Is that the direction you're thinking this should go?

What about something like this in your caller:

void BackgroundModeManager::ExecuteCommand(int command_id, int event_flags) {
#if defined(OS_MACOSX)
  typedef UserManagerMac UserManager;
#elif defined(TOOLKIT_VIEWS)
  typedef UserManagerView UserManager;
#endif

  ...
  case IDC_ABOUT:
    if (bmd) {
      chrome::ShowAboutChrome(bmd->GetBrowserWindow());
    } else {
      UserManager::Show(base::FilePath(),
                        profiles::USER_MANAGER_NO_TUTORIAL,
                        profiles::USER_MANAGER_SELECT_PROFILE_ABOUT_CHROME);
    }
    break;
  ...

If there is more than one function that calls the Show() variants, though, then
I would probably create a cross-platform header declaring UserManager class with
public static Show(), Hide(), and IsShowing() functions, then have e.g.
user_manager_view.cc implement those functions as well as a UserManagerView
class (doesn't have to even be a subclass if you don't need it to be, nor does
it need to have the class declaration in a header unless other code needs to
access the UserManagerView class specifically).

In either case, I'd eliminate the various (still-existing) ShowXYZ() variant
functions and just make the three-arg Show() the lone public access point to
that functionality.

[Mike Lerman, 2014-09-15 13:44:04]

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

https://codereview.chromium.org/564453003/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:42:
UserManagerMac::Show(base::FilePath(),
On 2014/09/12 14:44:16, Monica Dinculescu wrote:
> Peter's comment aside. I would combine these two into something like
> ShowUserManagerThenTakeAction, at least.

I'll wait on Peter's comments before I start refactoring these methods, but at
the least I'll combine these two into one method.

https://codereview.chromium.org/564453003/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:53: void
ShowUserManagerThenTaskManager
On 2014/09/12 14:44:16, Monica Dinculescu wrote:
> is this a typo?

The best kind!

https://codereview.chromium.org/564453003/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/564453003/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:693: if (url_hash_
==
On 2014/09/12 14:44:16, Monica Dinculescu wrote:
> Why don't you just save the hash as an std::string the first time?

Because that's too easy!

Done.

https://codereview.chromium.org/564453003/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/564453003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:711: if (browser)
{
On 2014/09/12 21:33:58, Evan Stade wrote:
> just DCHECK(browser) at the top of the function, no need to handle it here

Done.

https://codereview.chromium.org/564453003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:726:
OnBrowserWindowReady(content::Source<Browser>(source).ptr());
On 2014/09/12 21:33:58, Evan Stade wrote:
> how are you sure this is the right browser? I think you need to check
> BrowserMatches, or something. (What creates the browser in the browser == NULL
> case in OnSwitchToProfileComplete?)

BrowserMatches is sadly private (within an anonymous namespace). It doesn't
truly matter if it's the right browser, the Task Manager or About Chrome can be
launched in any browser that's about to open. The launching profile is about the
become the "Last Active Profile", which is where we aim to open these windows
anyways.
It's easy enough to check if browser->profile() == profile_, although I'm not
convinced it's desired.

If browser==NULL within OnSwitchToProfileComplete, OnSwitchToProfileComplete is
always the callback to Profile::SwitchToProfile (or a variant there-of) where
all call OpenBrowserWindowForProfile (within profile_window.cc).

[Andrew T Wilson (Slow), 2014-09-15 13:59:21]

https://codereview.chromium.org/564453003/diff/140001/chrome/browser/backgrou...
File chrome/browser/background/background_mode_manager.cc (right):

https://codereview.chromium.org/564453003/diff/140001/chrome/browser/backgrou...
chrome/browser/background/background_mode_manager.cc:542:
bmd->ExecuteCommand(command_id, event_flags);
What happens here (i.e. if you click on a background app while that profile is
locked)? Won't this crash because |bmd| is null?

[Mike Lerman, 2014-09-15 16:43:11]

Hi Peter,

I've implemented the refactoring you suggested. Please review.

https://codereview.chromium.org/564453003/diff/140001/chrome/browser/backgrou...
File chrome/browser/background/background_mode_manager.cc (right):

https://codereview.chromium.org/564453003/diff/140001/chrome/browser/backgrou...
chrome/browser/background/background_mode_manager.cc:542:
bmd->ExecuteCommand(command_id, event_flags);
On 2014/09/15 13:59:21, Andrew T Wilson wrote:
> What happens here (i.e. if you click on a background app while that profile is
> locked)? Won't this crash because |bmd| is null?

It will! Excellent point, good sir.

[Peter Kasting, 2014-09-15 20:17:28]

The UserManagerView comments below probably also apply to e.g. Cocoa.

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/user_...
File chrome/browser/ui/user_manager.h (right):

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/user_...
chrome/browser/ui/user_manager.h:18: // profile given by
|profile_path_to_focus|. Based on the value of
Since various callers pass an empty FilePath, you should probably note that
that's legal and what it does.

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/user_...
chrome/browser/ui/user_manager.h:20: // |profile_path_to_focus| is ignored.
After a profile is opened, execute the
Nit: execute -> executes

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/user_...
chrome/browser/ui/user_manager.h:26: // Hide the User Manager.
Nit: Hide -> Hides

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/user_...
chrome/browser/ui/user_manager.h:29: // Returns whether or not the User Manager
is showing.
Nit: Remove "or not"

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/user_manager_view.h (right):

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/user_manager_view.h:27: static void Show(const
base::FilePath& profile_path_to_focus,
Nit: I'd eliminate these three public statics and have the UserManager function
implementations in the .cc file just do the necessary bits.  This will require
making more things in UserManagerView public or adding accessors (or,
alternately, declaring UserManager a friend), but it eliminates boilerplate and
avoids the possibility of the UserManager and UserManagerView APIs getting out
of sync.

[Evan Stade, 2014-09-15 21:07:22]

my files lgtm

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:292: if
(args->GetString(0, &url_hash))
  args->GetString(0, &url_hash_)

should be sufficient

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:709: FROM_HERE,
base::Bind(&chrome::ShowTaskManager, browser));
this indent is too big

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:712: FROM_HERE,
base::Bind(&chrome::ShowAboutChrome, browser));
this indent is too little

[Mike Lerman, 2014-09-17 18:31:46]

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/user_...
File chrome/browser/ui/user_manager.h (right):

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/user_...
chrome/browser/ui/user_manager.h:18: // profile given by
|profile_path_to_focus|. Based on the value of
On 2014/09/15 20:17:28, Peter Kasting wrote:
> Since various callers pass an empty FilePath, you should probably note that
> that's legal and what it does.

Done.

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/user_...
chrome/browser/ui/user_manager.h:20: // |profile_path_to_focus| is ignored.
After a profile is opened, execute the
On 2014/09/15 20:17:28, Peter Kasting wrote:
> Nit: execute -> executes

Done.

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/user_...
chrome/browser/ui/user_manager.h:26: // Hide the User Manager.
On 2014/09/15 20:17:28, Peter Kasting wrote:
> Nit: Hide -> Hides

Done.

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/user_...
chrome/browser/ui/user_manager.h:29: // Returns whether or not the User Manager
is showing.
On 2014/09/15 20:17:27, Peter Kasting wrote:
> Nit: Remove "or not"

Done.

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/user_manager_view.h (right):

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/user_manager_view.h:27: static void Show(const
base::FilePath& profile_path_to_focus,
On 2014/09/15 20:17:28, Peter Kasting wrote:
> Nit: I'd eliminate these three public statics and have the UserManager
function
> implementations in the .cc file just do the necessary bits.  This will require
> making more things in UserManagerView public or adding accessors (or,
> alternately, declaring UserManager a friend), but it eliminates boilerplate
and
> avoids the possibility of the UserManager and UserManagerView APIs getting out
> of sync.

In the interest of keeping private things private (like instance_) what do you
think of having UserManagerView and UserManagerMac inherit from
c/b/ui/UserManager, and then I can bring the implementations of the *View and
*Mac methods inside the static methods without having to change the visibility
of the private members? You had implied you preferred to avoid explicit
references from UMView and UMMac to the UserManager, but I think this provides a
good reason to create that dependency.

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:292: if
(args->GetString(0, &url_hash))
On 2014/09/15 21:07:21, Evan Stade wrote:
>   args->GetString(0, &url_hash_)
> 
> should be sufficient

Done.

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:709: FROM_HERE,
base::Bind(&chrome::ShowTaskManager, browser));
On 2014/09/15 21:07:21, Evan Stade wrote:
> this indent is too big

Done.

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:712: FROM_HERE,
base::Bind(&chrome::ShowAboutChrome, browser));
On 2014/09/15 21:07:21, Evan Stade wrote:
> this indent is too little

And this indent is juuust right, goldilocks!

Done ;)

[Peter Kasting, 2014-09-17 20:14:13]

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/user_manager_view.h (right):

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/user_manager_view.h:27: static void Show(const
base::FilePath& profile_path_to_focus,
On 2014/09/17 18:31:46, Mike Lerman wrote:
> On 2014/09/15 20:17:28, Peter Kasting wrote:
> > Nit: I'd eliminate these three public statics and have the UserManager
> function
> > implementations in the .cc file just do the necessary bits.  This will
require
> > making more things in UserManagerView public or adding accessors (or,
> > alternately, declaring UserManager a friend), but it eliminates boilerplate
> and
> > avoids the possibility of the UserManager and UserManagerView APIs getting
out
> > of sync.
> 
> In the interest of keeping private things private (like instance_) what do you
> think of having UserManagerView and UserManagerMac inherit from
> c/b/ui/UserManager, and then I can bring the implementations of the *View and
> *Mac methods inside the static methods without having to change the visibility
> of the private members? You had implied you preferred to avoid explicit
> references from UMView and UMMac to the UserManager, but I think this provides
a
> good reason to create that dependency.

I don't know if I understand your plan.  Perhaps you could partially implement
it (e.g. implement the views side) so I could see what it consists of?

[Mike Lerman, 2014-09-18 18:32:45]

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/user_manager_view.h (right):

https://codereview.chromium.org/564453003/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/user_manager_view.h:27: static void Show(const
base::FilePath& profile_path_to_focus,
On 2014/09/17 20:14:13, Peter Kasting wrote:
> On 2014/09/17 18:31:46, Mike Lerman wrote:
> > On 2014/09/15 20:17:28, Peter Kasting wrote:
> > > Nit: I'd eliminate these three public statics and have the UserManager
> > function
> > > implementations in the .cc file just do the necessary bits.  This will
> require
> > > making more things in UserManagerView public or adding accessors (or,
> > > alternately, declaring UserManager a friend), but it eliminates
boilerplate
> > and
> > > avoids the possibility of the UserManager and UserManagerView APIs getting
> out
> > > of sync.
> > 
> > In the interest of keeping private things private (like instance_) what do
you
> > think of having UserManagerView and UserManagerMac inherit from
> > c/b/ui/UserManager, and then I can bring the implementations of the *View
and
> > *Mac methods inside the static methods without having to change the
visibility
> > of the private members? You had implied you preferred to avoid explicit
> > references from UMView and UMMac to the UserManager, but I think this
provides
> a
> > good reason to create that dependency.
> 
> I don't know if I understand your plan.  Perhaps you could partially implement
> it (e.g. implement the views side) so I could see what it consists of?

Couldn't find a great approach. I eliminated these three public statics and had
the UserManager implementations do the necessary bits, as you had suggested.

[Peter Kasting, 2014-09-18 18:55:05]

LGTM

https://codereview.chromium.org/564453003/diff/220001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/user_manager_view.cc (right):

https://codereview.chromium.org/564453003/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/user_manager_view.cc:42: static
UserManagerView* instance_ = NULL;
This shouldn't be static as it's already in an anonymous namespace.

https://codereview.chromium.org/564453003/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/user_manager_view.cc:46: void
UserManager::Show(
Optional: Add divider lines ("// UserManager -----------------...") above each
separate class' functions so readers see that we're implementing two different
classes here.  See e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...
as a sample of using these dividers.

https://codereview.chromium.org/564453003/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/user_manager_view.cc:63:
base::Bind(&UserManagerView::OnGuestProfileCreated,
Somehow it seems weird to me that this is a static function instead of a member
that we'll call on the new instance we create in the next line.  I guess it
doesn't really hurt anything, though.

https://codereview.chromium.org/564453003/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/user_manager_view.cc:91: instance_ =
instance.release();  // |instance_| takes over ownership.
Should this check that |instance_| was previously null?

[Mike Lerman, 2014-09-18 19:08:25]

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

[Mike Lerman, 2014-09-18 19:08:26]

Hi Alexei,

Can you review my cocoa changes, please? c/b/ui/cocoa changes? Thanks.

[Alexei Svitkine (slow), 2014-09-18 20:54:04]

https://codereview.chromium.org/564453003/diff/240001/chrome/browser/app_cont...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/564453003/diff/240001/chrome/browser/app_cont...
chrome/browser/app_controller_mac.mm:1255: profiles::USER_MANAGER_NO_TUTORIAL,
Nit: Align.

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

https://codereview.chromium.org/564453003/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:908:
profiles::USER_MANAGER_NO_TUTORIAL,
Nit: Align.

https://codereview.chromium.org/564453003/diff/240001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/user_manager_mac.h (right):

https://codereview.chromium.org/564453003/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/user_manager_mac.h:31: static void
OnGuestProfileCreated(Profile* guest_profile,
This is a very strange name for a method when looking at this class definition
by itself.

If it were just a method in the anon namespace used within a single .cc file,
I'd be okay with it, but in a .h file, I think it should have a better name
(i.e. one that matches the comment above it).

However, I'm also somewhat confused by this whole set up. If this is the only
way to create this class, then how does it work for non-guest profiles? (Since
the constructor is private.)

Maybe add a comment at the top of the class to better explain the class (e.g.
its singleton-ish nature and so forth).

https://codereview.chromium.org/564453003/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/user_manager_mac.h:34:
UserManagerWindowController* get_window_controller() {
Remove get_ prefixed, it's used for non-trivial methods (e.g.
GetWindowController), while trivial ones should omit it.

https://codereview.chromium.org/564453003/diff/240001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right):

https://codereview.chromium.org/564453003/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:29: UserManagerMac*
instance_ = NULL;  // Weak.
Why not make it a static member of the class?

[Peter Kasting, 2014-09-18 21:33:47]

https://codereview.chromium.org/564453003/diff/240001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/user_manager_mac.mm (right):

https://codereview.chromium.org/564453003/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/user_manager_mac.mm:29: UserManagerMac*
instance_ = NULL;  // Weak.
On 2014/09/18 20:54:04, Alexei Svitkine wrote:
> Why not make it a static member of the class?

Being file-scoped in the .mm file is even more local than being static to the
class -- completely hides this implementation detail from readers of the header.

[Mike Lerman, 2014-09-19 15:49:30]

Drew, any further comments?

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

https://codereview.chromium.org/564453003/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:908:
profiles::USER_MANAGER_NO_TUTORIAL,
On 2014/09/18 20:54:04, Alexei Svitkine wrote:
> Nit: Align.

Done.

https://codereview.chromium.org/564453003/diff/240001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/user_manager_mac.h (right):

https://codereview.chromium.org/564453003/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/user_manager_mac.h:31: static void
OnGuestProfileCreated(Profile* guest_profile,
On 2014/09/18 20:54:04, Alexei Svitkine wrote:
> This is a very strange name for a method when looking at this class definition
> by itself.
> 
> If it were just a method in the anon namespace used within a single .cc file,
> I'd be okay with it, but in a .h file, I think it should have a better name
> (i.e. one that matches the comment above it).
> 
> However, I'm also somewhat confused by this whole set up. If this is the only
> way to create this class, then how does it work for non-guest profiles? (Since
> the constructor is private.)
> 
> Maybe add a comment at the top of the class to better explain the class (e.g.
> its singleton-ish nature and so forth).'

Comment added.

The UserManager always operates as the Guest Profile, the profile from which you
launched it becomes Last Active.

https://codereview.chromium.org/564453003/diff/240001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/user_manager_mac.h:34:
UserManagerWindowController* get_window_controller() {
On 2014/09/18 20:54:04, Alexei Svitkine wrote:
> Remove get_ prefixed, it's used for non-trivial methods (e.g.
> GetWindowController), while trivial ones should omit it.

Done.

[Andrew T Wilson (Slow), 2014-09-19 15:53:55]

lgtm

[Alexei Svitkine (slow), 2014-09-19 15:54:17]

lgtm

[Mike Lerman, 2014-09-19 16:54:15]

mlerman@chromium.org changed reviewers:
+ sky@chromium.org

[Mike Lerman, 2014-09-19 16:54:16]

Sky,

Can I get a review of my change to chrome/browser please? Thanks!

Mike

[noms (inactive), 2014-09-19 17:31:42]

profiles lgtm % nit

https://codereview.chromium.org/564453003/diff/280001/chrome/browser/ui/user_...
File chrome/browser/ui/user_manager.h (right):

https://codereview.chromium.org/564453003/diff/280001/chrome/browser/ui/user_...
chrome/browser/ui/user_manager.h:30: static bool IsShowing();
needs a DISALLOW_COPY_AND_ASSIGN(UserManager), right?

[sky, 2014-09-19 21:11:16]

On 2014/09/19 16:54:16, Mike Lerman wrote:
> Sky,
> 
> Can I get a review of my change to chrome/browser please? Thanks!
> 
> Mike

All of chrome/browser? Is there a specific set you need me to review?

[Mike Lerman, 2014-09-20 23:59:55]

On 2014/09/19 21:11:16, sky wrote:
> On 2014/09/19 16:54:16, Mike Lerman wrote:
> > Sky,
> > 
> > Can I get a review of my change to chrome/browser please? Thanks!
> > 
> > Mike
> 
> All of chrome/browser? Is there a specific set you need me to review?

Just the files directly in chrome/browser, that is app_controller_mac[_unittest]
and browser_process_impl. All subdirectories have reviews from owners, sorry for
not being more specific.

[Mike Lerman, 2014-09-24 15:11:09]

Ping sky@

[sky, 2014-09-24 15:37:09]

Said files LGTM

[Mike Lerman, 2014-09-24 16:40:23]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-09-24 16:41:27]

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

[commit-bot: I haz the power, 2014-09-24 16:45:09]

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

[commit-bot: I haz the power, 2014-09-24 16:45:11]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/60112)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Mike Lerman, 2014-09-24 19:24:34]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-09-24 19:25:26]

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

[commit-bot: I haz the power, 2014-09-24 19:31:33]

Committed patchset #22 (id:420001) as 4ef9352212bc8570f6ab503f1940d3a8a17fe3e1

[commit-bot: I haz the power, 2014-09-24 19:32:16]

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

[Mike Lerman, 2014-11-25 14:06:36]

Patchset #23 (id:440001) has been deleted