Loading the Launcher with a Locked Profile launched UserManager.

Loading the Launcher with a Locked Profile launched UserManager.

On Mac, the --show-app-list flag causes the AppLauncher to start loading before the Profile is loaded. For this, the profile_path is sent to AppListService::InitAll(), so that this load can be aborted if it would point to a Locked Profile.

The AppListService will no longer permit loading a locked profile, and will direct to the UserManager instead. StartupBrowserCreator has a similar flow.

BUG=421308
TEST=Try to load the App Launcher for a locked profile in any possible way the Launcher can be launched ;) It should always direct to the UserManager, upon Profile Selection which then directs to the AppLauncher. Opening the AppLauncher for a non-locked profile should always work smoothly.

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

[Mike Lerman, 2015-01-16 19:48:10]

mlerman@chromium.org changed reviewers:
+ sky@chromium.org, tapted@chromium.org, xiyuan@chromium.org

[Mike Lerman, 2015-01-16 19:48:10]

Hey there, reviewers!

Feeling that post-OKRs antsy-ness? Looking for some code to review? Do I have
the CL for you!

sky - c/b/chrome_browser_main* & c/b/ui/startup/
tapted - c/b/ui/app_list/ and c/b/ui/views/app_list
xiyuan - c/b/ui/ash/app_list, user_manager_screen_handler, profile_window (and
optionally, c/b/ui/app_list)

Thanks!

[xiyuan, 2015-01-16 20:31:52]

Mostly good.

https://codereview.chromium.org/856733004/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service.h (right):

https://codereview.chromium.org/856733004/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service.h:58: const base::FilePath&
profile_path);
app_list_service_ash.cc and app_list_service_linux.cc need to be updated for
this too.

https://codereview.chromium.org/856733004/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service_mac.mm (right):

https://codereview.chromium.org/856733004/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_mac.mm:388: if
(!profile_info_cache.ProfileIsSigninRequiredAtIndex(profile_index))
Can we use IsProfileLocked() here?

[Mike Lerman, 2015-01-19 21:08:56]

https://codereview.chromium.org/856733004/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service.h (right):

https://codereview.chromium.org/856733004/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service.h:58: const base::FilePath&
profile_path);
On 2015/01/16 20:31:52, xiyuan wrote:
> app_list_service_ash.cc and app_list_service_linux.cc need to be updated for
> this too.

Ah, I did? Those files just aren't in the same subdirectory, but I modified the
InitAll() methods.

https://codereview.chromium.org/856733004/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_service_mac.mm (right):

https://codereview.chromium.org/856733004/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_service_mac.mm:388: if
(!profile_info_cache.ProfileIsSigninRequiredAtIndex(profile_index))
On 2015/01/16 20:31:52, xiyuan wrote:
> Can we use IsProfileLocked() here?

I could I suppose, if I had a ProfileStore. I don't think I have one here.
Beyond that, there isn't an IsProfileLocked function in the profiles code; this
is the code we use to determine if a Profile*'s locked, at least until the
ProfileInfoCache gets eventually refactored.

[tapted, 2015-01-19 22:25:51]

Sorry for the delay - had to page back in how all this asynchronous stuff goes
:/.

I think this really needs a test too - it seems like an easy thing to
accidentally break. Comments below might make testing a lot more
straightforward.

https://codereview.chromium.org/856733004/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_impl.cc (right):

https://codereview.chromium.org/856733004/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:98:
UserManager::Show(base::FilePath(),
WDYT about just calling DismissAppList() before this?

i.e. still start showing it in AppListServiceMac::Init(), but fade it out before
any icons are loaded. I think that will simplify things a lot, and the user
might see a progress spinner slightly sooner. The app list icon doesn't bounce
in the Dock (Chrome does), so it's good to get some UI feedback going.

Then, I don't think you'd need to pass the extra argument to InitAll, and the
logic will just be in one place.

(also I think this should be in ProfileLoader - not ProfileStore - see below)

https://codereview.chromium.org/856733004/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:135: return nullptr;
I'm a little wary of this - I don't think it's the right place. ProfileStoreImpl
is meant to be "dumb" since it's the part that isn't available to unit tests.

Can you modify ProfileLoader::LoadProfileInvalidatingOtherLoads() instead? I'm
pretty sure think that's the only place that uses ProfileLoader.

Then you can add `bool IsProfileLocked()` on ProfileLoader's interface, and have
it return a given value in tests.

[Mike Lerman, 2015-01-20 00:02:41]

On 2015/01/19 22:25:51, tapted wrote:
> Sorry for the delay - had to page back in how all this asynchronous stuff goes
> :/.
> 
> I think this really needs a test too - it seems like an easy thing to
> accidentally break. Comments below might make testing a lot more
> straightforward.
> 
>
https://codereview.chromium.org/856733004/diff/100001/chrome/browser/ui/app_l...
> File chrome/browser/ui/app_list/app_list_service_impl.cc (right):
> 
>
https://codereview.chromium.org/856733004/diff/100001/chrome/browser/ui/app_l...
> chrome/browser/ui/app_list/app_list_service_impl.cc:98:
> UserManager::Show(base::FilePath(),
> WDYT about just calling DismissAppList() before this?
> 
> i.e. still start showing it in AppListServiceMac::Init(), but fade it out
before
> any icons are loaded. I think that will simplify things a lot, and the user
> might see a progress spinner slightly sooner. The app list icon doesn't bounce
> in the Dock (Chrome does), so it's good to get some UI feedback going.
> 
> Then, I don't think you'd need to pass the extra argument to InitAll, and the
> logic will just be in one place.
> 
> (also I think this should be in ProfileLoader - not ProfileStore - see below)
> 
>
https://codereview.chromium.org/856733004/diff/100001/chrome/browser/ui/app_l...
> chrome/browser/ui/app_list/app_list_service_impl.cc:135: return nullptr;
> I'm a little wary of this - I don't think it's the right place.
ProfileStoreImpl
> is meant to be "dumb" since it's the part that isn't available to unit tests.
> 
> Can you modify ProfileLoader::LoadProfileInvalidatingOtherLoads() instead? I'm
> pretty sure think that's the only place that uses ProfileLoader.
> 
> Then you can add `bool IsProfileLocked()` on ProfileLoader's interface, and
have
> it return a given value in tests.

I had tried calling dismissAppList(), and it made for a really ugly user
experience, especially if the launcher is opening without a browser being open;
it opens a good few seconds before the User Manager.

[Mike Lerman, 2015-01-21 21:54:13]

https://codereview.chromium.org/856733004/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_impl.cc (right):

https://codereview.chromium.org/856733004/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:98:
UserManager::Show(base::FilePath(),
On 2015/01/19 22:25:50, tapted wrote:
> WDYT about just calling DismissAppList() before this?
> 
> i.e. still start showing it in AppListServiceMac::Init(), but fade it out
before
> any icons are loaded. I think that will simplify things a lot, and the user
> might see a progress spinner slightly sooner. The app list icon doesn't bounce
> in the Dock (Chrome does), so it's good to get some UI feedback going.
> 
> Then, I don't think you'd need to pass the extra argument to InitAll, and the
> logic will just be in one place.
> 
> (also I think this should be in ProfileLoader - not ProfileStore - see below)

We discussed offline - shall leave it as is. If the UserManager's too slow to
load, that's its own problem ;)

https://codereview.chromium.org/856733004/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:135: return nullptr;
On 2015/01/19 22:25:51, tapted wrote:
> I'm a little wary of this - I don't think it's the right place.
ProfileStoreImpl
> is meant to be "dumb" since it's the part that isn't available to unit tests.
> 
> Can you modify ProfileLoader::LoadProfileInvalidatingOtherLoads() instead? I'm
> pretty sure think that's the only place that uses ProfileLoader.
> 
> Then you can add `bool IsProfileLocked()` on ProfileLoader's interface, and
have
> it return a given value in tests.

Done.

[xiyuan, 2015-01-21 22:04:52]

LGTM my part

[tapted, 2015-01-21 22:47:21]

lgtm Thanks! nice test. And good choice for test harness - there is
(non-interactive) AppListServiceUnitTest, but the amount of plumbing required to
mock the UserManager::Show(..) call would probably make the test less
compelling.

https://codereview.chromium.org/856733004/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_impl.cc (right):

https://codereview.chromium.org/856733004/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:127: if
(IsProfileLocked(path))
nit: I'm pretty sure this can this be DCHECK(!IsProfileLocked(path)).

https://codereview.chromium.org/856733004/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc (right):

https://codereview.chromium.org/856733004/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc:131:
ASSERT_FALSE(service->IsAppListVisible());
nit: ASSERT -> EXPECT (here and all following)

I think the surrounding tests have ASSERT rather than EXPECT because the tests
could historically get in a weird state / timeout when they would unexpectedly
flake. But they're stable now and I think the preference would be to have this
ASSERT and all the ones following be EXPECT. (the ASSERTs above are good though
- null pointer dereferences tend not to give nice stackdumps in the bots)

https://codereview.chromium.org/856733004/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc:149:
ASSERT_TRUE(model);
note I think this would mean that the app launcher is still configured for the
initial/default profile.

It's subtle, so perhaps a check to that effect, like 

EXPECT_TRUE(service->GetCurrentAppListProfile());
EXPECT_EQ(browser->profile(), service->GetCurrentAppListProfile());


Maybe there are corner cases for locked profiles where this would be a problem,
but that's orthogonal to this CL (hint: It's tricky - but stuff in
AppListServiceImpl::OnProfileWillBeRemoved() is probably a lot like what you
might want).

[Mike Lerman, 2015-01-22 03:14:44]

sky@, over to you for review, please! chrome/browser and
chrome/browser/ui/startup.

tapted@, I'll address these.

[sky, 2015-01-22 18:26:15]

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

https://codereview.chromium.org/856733004/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:741: FROM_HERE,
base::Bind(&ShowAppList, browser, desktop_type_));
How do you know browser is still valid by the time ShowAPpList runs?

[Mike Lerman, 2015-01-23 16:58:33]

Thanks, sky@.

tapted@, can you take another look at the interactiveuitest? Thanks!

https://codereview.chromium.org/856733004/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_impl.cc (right):

https://codereview.chromium.org/856733004/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:127: if
(IsProfileLocked(path))
On 2015/01/21 22:47:20, tapted wrote:
> nit: I'm pretty sure this can this be DCHECK(!IsProfileLocked(path)).

Oop, thanks! I had meant to remove this completely, but a DCHECK makes sense too
:)

https://codereview.chromium.org/856733004/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc (right):

https://codereview.chromium.org/856733004/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc:131:
ASSERT_FALSE(service->IsAppListVisible());
On 2015/01/21 22:47:20, tapted wrote:
> nit: ASSERT -> EXPECT (here and all following)
> 
> I think the surrounding tests have ASSERT rather than EXPECT because the tests
> could historically get in a weird state / timeout when they would unexpectedly
> flake. But they're stable now and I think the preference would be to have this
> ASSERT and all the ones following be EXPECT. (the ASSERTs above are good
though
> - null pointer dereferences tend not to give nice stackdumps in the bots)

Done - all the NULL tests are ASSERTs and all the method / boolean checks are
EXPECTs.

https://codereview.chromium.org/856733004/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc:149:
ASSERT_TRUE(model);
On 2015/01/21 22:47:20, tapted wrote:
> note I think this would mean that the app launcher is still configured for the
> initial/default profile.
> 
> It's subtle, so perhaps a check to that effect, like 
> 
> EXPECT_TRUE(service->GetCurrentAppListProfile());
> EXPECT_EQ(browser->profile(), service->GetCurrentAppListProfile());
> 
> 
> Maybe there are corner cases for locked profiles where this would be a
problem,
> but that's orthogonal to this CL (hint: It's tricky - but stuff in
> AppListServiceImpl::OnProfileWillBeRemoved() is probably a lot like what you
> might want).

Ah, too much blind copy-pasting. Thanks!

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

https://codereview.chromium.org/856733004/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:741: FROM_HERE,
base::Bind(&ShowAppList, browser, desktop_type_));
On 2015/01/22 18:26:15, sky wrote:
> How do you know browser is still valid by the time ShowAPpList runs?

It just got created and there's nothing that should be deleting it? But the
point is valid. Since I only need a Profile* here, changed the parameter to a
Profile*.

[sky, 2015-01-23 17:55:04]

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

https://codereview.chromium.org/856733004/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:741: FROM_HERE,
base::Bind(&ShowAppList, browser, desktop_type_));
On 2015/01/23 16:58:33, Mike Lerman wrote:
> On 2015/01/22 18:26:15, sky wrote:
> > How do you know browser is still valid by the time ShowAPpList runs?
> 
> It just got created and there's nothing that should be deleting it? But the
> point is valid. Since I only need a Profile* here, changed the parameter to a
> Profile*.

Yes, the object just got got created (object being a browser or profile doesn't
matter). PostTask is delayed, how do you know the object is still valid when the
delayed task is run?

[Mike Lerman, 2015-01-23 18:21:03]

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

https://codereview.chromium.org/856733004/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:741: FROM_HERE,
base::Bind(&ShowAppList, browser, desktop_type_));
For the Profile, we should be safe, since the Profile* stays around until Chrome
shutdown.

For the Browsers in the above calls, I guess we don't know they're still valid,
except that that they've never been invalid (we'd be getting crash reports if it
did). I could harden the chrome::Show*() methods to DCHECK or return; if the
browser's null or otherwise invalid. I'd lean towards returning, and I'd also
lean towards doing that in another CL ;)

[sky, 2015-01-23 20:23:46]

It's not the null I'm worried about. It's about the browser/profile being
deleted by the time the callback runs. When you post the task you need to
wrap it in something that tracks when the browser is deleted. That way you
know if the browser is valid or not.

  -Scott

On Fri, Jan 23, 2015 at 10:21 AM, <mlerman@chromium.org> wrote:

>
> https://codereview.chromium.org/856733004/diff/120001/
> chrome/browser/ui/webui/signin/user_manager_screen_handler.cc
> File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc
> (right):
>
> https://codereview.chromium.org/856733004/diff/120001/
> chrome/browser/ui/webui/signin/user_manager_screen_handler.cc#newcode741
> chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:741:
> FROM_HERE, base::Bind(&ShowAppList, browser, desktop_type_));
> For the Profile, we should be safe, since the Profile* stays around
> until Chrome shutdown.
>
> For the Browsers in the above calls, I guess we don't know they're still
> valid, except that that they've never been invalid (we'd be getting
> crash reports if it did). I could harden the chrome::Show*() methods to
> DCHECK or return; if the browser's null or otherwise invalid. I'd lean
> towards returning, and I'd also lean towards doing that in another CL ;)
>
> https://codereview.chromium.org/856733004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mike Lerman, 2015-01-26 15:44:30]

I think the answer, then, is to use BrowserFinder on the Profile* (which never
gets deleted) to get a Browser* (potentially the same one) in the posted task.
Sound good?

I don't know of a mechanism that would be efficiently notified if the Browser in
question gets deleted by some other Task, so the best bet is to get the Browser
back from the browser_finder.

On 2015/01/23 20:23:46, sky wrote:
> It's not the null I'm worried about. It's about the browser/profile being
> deleted by the time the callback runs. When you post the task you need to
> wrap it in something that tracks when the browser is deleted. That way you
> know if the browser is valid or not.
> 
>   -Scott

[sky, 2015-01-26 16:56:50]

BrowserFinder isn't the right solution. Instead your object should listen
for browser to be destroyed (BrowserListObserver) and when destroyed null
out its fields.

  -Scott

On Mon, Jan 26, 2015 at 7:44 AM, <mlerman@chromium.org> wrote:

> I think the answer, then, is to use BrowserFinder on the Profile* (which
> never
> gets deleted) to get a Browser* (potentially the same one) in the posted
> task.
> Sound good?
>
> I don't know of a mechanism that would be efficiently notified if the
> Browser in
> question gets deleted by some other Task, so the best bet is to get the
> Browser
> back from the browser_finder.
>
> On 2015/01/23 20:23:46, sky wrote:
>
>> It's not the null I'm worried about. It's about the browser/profile being
>> deleted by the time the callback runs. When you post the task you need to
>> wrap it in something that tracks when the browser is deleted. That way you
>> know if the browser is valid or not.
>>
>
>     -Scott
>>
>
>
> https://codereview.chromium.org/856733004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tapted, 2015-01-27 02:52:55]

Sorry for the lag - it was a public holiday here on Monday. And I'm probably up
late tonight if you want to ping me during some timezone overlap.

On 2015/01/23 18:21:03, Mike Lerman wrote:
> For the Profile, we should be safe, since the Profile* stays around until
Chrome
> shutdown.

You could perhaps alleviate the concern by using a profile path data member,
rather than a raw pointer. The Profile* is deleted during shutdown -- there's
still opportunity for it to cause problems when task queues are flushed then.
Also, while the pointer-delete is delayed, it's not usually safe to actually
*do* anything with a removed Profile* since its user data dir will be [in the
process of being] deleted.

https://codereview.chromium.org/856733004/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc (right):

https://codereview.chromium.org/856733004/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc:149:
ASSERT_TRUE(model);
On 2015/01/23 16:58:33, Mike Lerman wrote:
> On 2015/01/21 22:47:20, tapted wrote:
> > note I think this would mean that the app launcher is still configured for
the
> > initial/default profile.
> > 
> > It's subtle, so perhaps a check to that effect, like 
> > 
> > EXPECT_TRUE(service->GetCurrentAppListProfile());
> > EXPECT_EQ(browser->profile(), service->GetCurrentAppListProfile());
> > 
> > 
> > Maybe there are corner cases for locked profiles where this would be a
> problem,
> > but that's orthogonal to this CL (hint: It's tricky - but stuff in
> > AppListServiceImpl::OnProfileWillBeRemoved() is probably a lot like what you
> > might want).
> 
> Ah, too much blind copy-pasting. Thanks!

Sorry, I meant that if that expectation is true it would mean that the app
launcher is still configured for the initial/default profile. (i.e. deleting the
expectation in the test won't change that).

My review comments actually first said "remove `app_list::AppListModel* model`
entirely since it's not really used". However, the fact that it is still
non-NULL here is "interesting", since it means the profile switch failed. I
think that's a valid test result, and it would be good to keep this in to check
for regressions. (but with the extra lines and a comment so it's clear to a
later reader what's going on).

It might be the case that, in a different context, having a non-NULL model here
is *not* a valid test result. e.g. locking the *current* profile rather than
switching *to* a locked profile. We can worry about that context in a follow-up
(if we need to at all), but it would be good to capture what's going on
currently.

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_impl.cc (right):

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:21: #include
"chrome/browser/ui/user_manager.h"
nit: unsued I think

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc (right):

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc:146: //
Pre-load the Guest Profile so the UserManager shows quickly.
why this change? If things were timing out it might not be enough to prevent
flakiness. Perhaps add to the comment here, like "If this is not done <something
something happens...>"

If it's the case that the profile creation is slow in the test, it might be
better to seed the guest profile with a dummy profile (which is kinda what the
InitSecondProfile() call at the start of the test here is doing), or just
disable the guest profile loading (if there's a nice way to do that).

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

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:148: class
UrlHashHelper : public chrome::BrowserListObserver {
add a brief comment, it should say something about the lifetime. (Also to me
`Hash` seems a but out of place and misleading. I guess it's historical here,
but I'd go maybe `KeyHander` or `IdHandler`)

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:151: Browser*
browser, chrome::HostDesktopType desktop_type, std::string hash);
const reference `hash`

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:164: };
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:170:
desktop_type_(desktop_type),
Note there's also browser->host_desktop_type(), which I'd say would be
marginally better than passing in |desktop_type| separately.

The question is raises would be whether OnBrowserWindowReady is ever called with
a browser on a different desktop type to
UserManagerScreenHandler::desktop_type_. On Mac, no. But on Windows 8 I can
imagine

 - Show App Launcher in Desktop Mode (appears in desktop mode regardless of
whether Chrome is in metro mode)
 - *locked or something* -> need to show UserManager
 - UserManager shows a Browser window
 - Browser windows are banned from desktop mode when user is in metro mode
 - Chrome opens Metro mode to show the browser window
 - ShowAppList....... but now it should show in Metro mode (since I'm pretty
sure we don't try to flip win8 out of metro mode when showing stuff on the
desktop [just the other way around]).

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:181: browser_ =
NULL;
nit: NULL -> nullptr.

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:185: if (browser_
== NULL)
NULL -> nullptr;

or, I'd probably go just `if (!browser_)`, but I'm not an OWNER here :)

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:201:
AppListService* app_list_service = AppListService::Get(desktop_type_);
note this desktop_type_ can be different to |browser_| now. Perhaps move this
whole `if` to the

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:205: delete this;
Using base::Owned(..) rather than base::Unretained(..) might be a better fit for
this, but I guess this works too.

base::Owned(..) would also have the benefit of being able to return early for
some nicer control flow here:
i.e.

if (app_launcher) {
  .. ShowForProfile(
  return;
}

if (!browser_)
  browser_ =

if (!browser_)
  return;

etc.

[tapted, 2015-01-27 02:56:36]

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

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:201:
AppListService* app_list_service = AppListService::Get(desktop_type_);
On 2015/01/27 02:52:55, tapted wrote:
> note this desktop_type_ can be different to |browser_| now. Perhaps move this
> whole `if` to the 

uh.. to the `top` that was going to say (but I thought I'd deleted this since
it's covered by the next comment)

[Mike Lerman, 2015-01-27 14:59:49]

Thanks Trent and Scott for your comments.

The solution below addresses what happens if a Browser* is deleted. Profile*
deletion can only happen in TearDown, after the MainMessageLoop has run out.
From what I understand, there's a longstanding assumption that a Profile*, once
valid, stays valid.

https://codereview.chromium.org/856733004/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc (right):

https://codereview.chromium.org/856733004/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc:149:
ASSERT_TRUE(model);
On 2015/01/27 02:52:54, tapted wrote:
> On 2015/01/23 16:58:33, Mike Lerman wrote:
> > On 2015/01/21 22:47:20, tapted wrote:
> > > note I think this would mean that the app launcher is still configured for
> the
> > > initial/default profile.
> > > 
> > > It's subtle, so perhaps a check to that effect, like 
> > > 
> > > EXPECT_TRUE(service->GetCurrentAppListProfile());
> > > EXPECT_EQ(browser->profile(), service->GetCurrentAppListProfile());
> > > 
> > > 
> > > Maybe there are corner cases for locked profiles where this would be a
> > problem,
> > > but that's orthogonal to this CL (hint: It's tricky - but stuff in
> > > AppListServiceImpl::OnProfileWillBeRemoved() is probably a lot like what
you
> > > might want).
> > 
> > Ah, too much blind copy-pasting. Thanks!
> 
> Sorry, I meant that if that expectation is true it would mean that the app
> launcher is still configured for the initial/default profile. (i.e. deleting
the
> expectation in the test won't change that).
> 
> My review comments actually first said "remove `app_list::AppListModel* model`
> entirely since it's not really used". However, the fact that it is still
> non-NULL here is "interesting", since it means the profile switch failed. I
> think that's a valid test result, and it would be good to keep this in to
check
> for regressions. (but with the extra lines and a comment so it's clear to a
> later reader what's going on).
> 
> It might be the case that, in a different context, having a non-NULL model
here
> is *not* a valid test result. e.g. locking the *current* profile rather than
> switching *to* a locked profile. We can worry about that context in a
follow-up
> (if we need to at all), but it would be good to capture what's going on
> currently.

You mention that the fact that it's not-NULL is "interesting", but I don't see
how it means that the ProfileSwitch failed. For an AppListService, there doesn't
seem to be a mechanism for removing the model, no matter what happens. Perhaps
that's worth noting, so have done so, but leave to your discretion if I should
change the comment, if I'm reading the code wrong.

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_impl.cc (right):

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl.cc:21: #include
"chrome/browser/ui/user_manager.h"
On 2015/01/27 02:52:55, tapted wrote:
> nit: unsued I think

Done.

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc (right):

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc:146: //
Pre-load the Guest Profile so the UserManager shows quickly.
When something calls UserManager::Show(), there's a potentially asynchronous
task to get the GuestProfile (become SystemProfile due to another CL which just
landed). If the GuestProfile doesn't already exist in memory, it's created from
disk, but if it already exists in memory then the function completes
synchronously. No Async call -> No flakiness.

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

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:148: class
UrlHashHelper : public chrome::BrowserListObserver {
On 2015/01/27 02:52:55, tapted wrote:
> add a brief comment, it should say something about the lifetime. (Also to me
> `Hash` seems a but out of place and misleading. I guess it's historical here,
> but I'd go maybe `KeyHander` or `IdHandler`)

Comment added.

It's called a "hash" because that's the variable used elsewhere in the
UserManagerScreenHandler to reference the parameter in the URL that follows the
#.

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:151: Browser*
browser, chrome::HostDesktopType desktop_type, std::string hash);
On 2015/01/27 02:52:55, tapted wrote:
> const reference `hash`

Done.

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:164: };
On 2015/01/27 02:52:55, tapted wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:170:
desktop_type_(desktop_type),
On 2015/01/27 02:52:55, tapted wrote:
> Note there's also browser->host_desktop_type(), which I'd say would be
> marginally better than passing in |desktop_type| separately.
> 
> The question is raises would be whether OnBrowserWindowReady is ever called
with
> a browser on a different desktop type to
> UserManagerScreenHandler::desktop_type_. On Mac, no. But on Windows 8 I can
> imagine
> 
>  - Show App Launcher in Desktop Mode (appears in desktop mode regardless of
> whether Chrome is in metro mode)
>  - *locked or something* -> need to show UserManager
>  - UserManager shows a Browser window
>  - Browser windows are banned from desktop mode when user is in metro mode
>  - Chrome opens Metro mode to show the browser window
>  - ShowAppList....... but now it should show in Metro mode (since I'm pretty
> sure we don't try to flip win8 out of metro mode when showing stuff on the
> desktop [just the other way around]).

SGTM. Done.

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:181: browser_ =
NULL;
On 2015/01/27 02:52:55, tapted wrote:
> nit: NULL -> nullptr. 

Done.

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:185: if (browser_
== NULL)
On 2015/01/27 02:52:55, tapted wrote:
> NULL -> nullptr;
> 
> or, I'd probably go just `if (!browser_)`, but I'm not an OWNER here :)

Done. I like that better, too :)

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:201:
AppListService* app_list_service = AppListService::Get(desktop_type_);
On 2015/01/27 02:56:36, tapted wrote:
> On 2015/01/27 02:52:55, tapted wrote:
> > note this desktop_type_ can be different to |browser_| now. Perhaps move
this
> > whole `if` to the 
> 
> uh.. to the `top` that was going to say (but I thought I'd deleted this since
> it's covered by the next comment)

Done.

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:201:
AppListService* app_list_service = AppListService::Get(desktop_type_);
On 2015/01/27 02:52:55, tapted wrote:
> note this desktop_type_ can be different to |browser_| now. Perhaps move this
> whole `if` to the 

Done.

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:205: delete this;
So base::Owned will take care of deleting the object once the posted task is
done execution? Sounds ideal for me! I can't find any documentation about that
class.

[sky, 2015-01-27 17:12:20]

On 2015/01/27 14:59:49, Mike Lerman wrote:
> Thanks Trent and Scott for your comments.
> 
> The solution below addresses what happens if a Browser* is deleted. Profile*
> deletion can only happen in TearDown, after the MainMessageLoop has run out.
> From what I understand, there's a longstanding assumption that a Profile*,
once
> valid, stays valid.

Really? That's news to me. Can you verify that with a profile owner.

> 
>
https://codereview.chromium.org/856733004/diff/120001/chrome/browser/ui/app_l...
> File chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc
(right):
> 
>
https://codereview.chromium.org/856733004/diff/120001/chrome/browser/ui/app_l...
> chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc:149:
> ASSERT_TRUE(model);
> On 2015/01/27 02:52:54, tapted wrote:
> > On 2015/01/23 16:58:33, Mike Lerman wrote:
> > > On 2015/01/21 22:47:20, tapted wrote:
> > > > note I think this would mean that the app launcher is still configured
for
> > the
> > > > initial/default profile.
> > > > 
> > > > It's subtle, so perhaps a check to that effect, like 
> > > > 
> > > > EXPECT_TRUE(service->GetCurrentAppListProfile());
> > > > EXPECT_EQ(browser->profile(), service->GetCurrentAppListProfile());
> > > > 
> > > > 
> > > > Maybe there are corner cases for locked profiles where this would be a
> > > problem,
> > > > but that's orthogonal to this CL (hint: It's tricky - but stuff in
> > > > AppListServiceImpl::OnProfileWillBeRemoved() is probably a lot like what
> you
> > > > might want).
> > > 
> > > Ah, too much blind copy-pasting. Thanks!
> > 
> > Sorry, I meant that if that expectation is true it would mean that the app
> > launcher is still configured for the initial/default profile. (i.e. deleting
> the
> > expectation in the test won't change that).
> > 
> > My review comments actually first said "remove `app_list::AppListModel*
model`
> > entirely since it's not really used". However, the fact that it is still
> > non-NULL here is "interesting", since it means the profile switch failed. I
> > think that's a valid test result, and it would be good to keep this in to
> check
> > for regressions. (but with the extra lines and a comment so it's clear to a
> > later reader what's going on).
> > 
> > It might be the case that, in a different context, having a non-NULL model
> here
> > is *not* a valid test result. e.g. locking the *current* profile rather than
> > switching *to* a locked profile. We can worry about that context in a
> follow-up
> > (if we need to at all), but it would be good to capture what's going on
> > currently.
> 
> You mention that the fact that it's not-NULL is "interesting", but I don't see
> how it means that the ProfileSwitch failed. For an AppListService, there
doesn't
> seem to be a mechanism for removing the model, no matter what happens. Perhaps
> that's worth noting, so have done so, but leave to your discretion if I should
> change the comment, if I'm reading the code wrong.
> 
>
https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/app_l...
> File chrome/browser/ui/app_list/app_list_service_impl.cc (right):
> 
>
https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/app_l...
> chrome/browser/ui/app_list/app_list_service_impl.cc:21: #include
> "chrome/browser/ui/user_manager.h"
> On 2015/01/27 02:52:55, tapted wrote:
> > nit: unsued I think
> 
> Done.
> 
>
https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/app_l...
> File chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc
(right):
> 
>
https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/app_l...
> chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc:146: //
> Pre-load the Guest Profile so the UserManager shows quickly.
> When something calls UserManager::Show(), there's a potentially asynchronous
> task to get the GuestProfile (become SystemProfile due to another CL which
just
> landed). If the GuestProfile doesn't already exist in memory, it's created
from
> disk, but if it already exists in memory then the function completes
> synchronously. No Async call -> No flakiness.
> 
>
https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):
> 
>
https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:148: class
> UrlHashHelper : public chrome::BrowserListObserver {
> On 2015/01/27 02:52:55, tapted wrote:
> > add a brief comment, it should say something about the lifetime. (Also to me
> > `Hash` seems a but out of place and misleading. I guess it's historical
here,
> > but I'd go maybe `KeyHander` or `IdHandler`)
> 
> Comment added.
> 
> It's called a "hash" because that's the variable used elsewhere in the
> UserManagerScreenHandler to reference the parameter in the URL that follows
the
> #.
> 
>
https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:151: Browser*
> browser, chrome::HostDesktopType desktop_type, std::string hash);
> On 2015/01/27 02:52:55, tapted wrote:
> > const reference `hash`
> 
> Done.
> 
>
https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:164: };
> On 2015/01/27 02:52:55, tapted wrote:
> > nit: DISALLOW_COPY_AND_ASSIGN
> 
> Done.
> 
>
https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:170:
> desktop_type_(desktop_type),
> On 2015/01/27 02:52:55, tapted wrote:
> > Note there's also browser->host_desktop_type(), which I'd say would be
> > marginally better than passing in |desktop_type| separately.
> > 
> > The question is raises would be whether OnBrowserWindowReady is ever called
> with
> > a browser on a different desktop type to
> > UserManagerScreenHandler::desktop_type_. On Mac, no. But on Windows 8 I can
> > imagine
> > 
> >  - Show App Launcher in Desktop Mode (appears in desktop mode regardless of
> > whether Chrome is in metro mode)
> >  - *locked or something* -> need to show UserManager
> >  - UserManager shows a Browser window
> >  - Browser windows are banned from desktop mode when user is in metro mode
> >  - Chrome opens Metro mode to show the browser window
> >  - ShowAppList....... but now it should show in Metro mode (since I'm pretty
> > sure we don't try to flip win8 out of metro mode when showing stuff on the
> > desktop [just the other way around]).
> 
> SGTM. Done.
> 
>
https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:181: browser_ =
> NULL;
> On 2015/01/27 02:52:55, tapted wrote:
> > nit: NULL -> nullptr. 
> 
> Done.
> 
>
https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:185: if
(browser_
> == NULL)
> On 2015/01/27 02:52:55, tapted wrote:
> > NULL -> nullptr;
> > 
> > or, I'd probably go just `if (!browser_)`, but I'm not an OWNER here :)
> 
> Done. I like that better, too :)
> 
>
https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:201:
> AppListService* app_list_service = AppListService::Get(desktop_type_);
> On 2015/01/27 02:56:36, tapted wrote:
> > On 2015/01/27 02:52:55, tapted wrote:
> > > note this desktop_type_ can be different to |browser_| now. Perhaps move
> this
> > > whole `if` to the 
> > 
> > uh.. to the `top` that was going to say (but I thought I'd deleted this
since
> > it's covered by the next comment)
> 
> Done.
> 
>
https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:201:
> AppListService* app_list_service = AppListService::Get(desktop_type_);
> On 2015/01/27 02:52:55, tapted wrote:
> > note this desktop_type_ can be different to |browser_| now. Perhaps move
this
> > whole `if` to the 
> 
> Done.
> 
>
https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:205: delete
this;
> So base::Owned will take care of deleting the object once the posted task is
> done execution? Sounds ideal for me! I can't find any documentation about that
> class.

[noms (inactive), 2015-01-27 17:44:50]

On 2015/01/27 17:12:20, sky wrote:
> On 2015/01/27 14:59:49, Mike Lerman wrote:
> > Thanks Trent and Scott for your comments.
> > 
> > The solution below addresses what happens if a Browser* is deleted. Profile*
> > deletion can only happen in TearDown, after the MainMessageLoop has run out.
> > From what I understand, there's a longstanding assumption that a Profile*,
> once
> > valid, stays valid.
> 
> Really? That's news to me. Can you verify that with a profile owner.

Yup, I'm afraid it's true. Even when a profile is deleted, the Profile* instance
stays around
(because we don't know how to deal with background apps and such). The info is 
deleted from the ProfileInfoCache, but the pointer stays around.

Here's a relevant bit in the code:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro....
The Profile* goes away when Chrome is shutting down and ready to delete the
profile directory.

[sky, 2015-01-27 17:54:31]

LGTM

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

https://codereview.chromium.org/856733004/diff/220001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:194: browser_ =
chrome::FindLastActiveWithProfile(profile_, desktop_type_);
nit: seems safer to introduce a local rather than muck with browser_

[Mike Lerman, 2015-01-27 18:26:11]

Thanks Monica and Scott.

Trent, any further comments, since I already have your LGTM? If no, into the CQ
it goes!

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

https://codereview.chromium.org/856733004/diff/220001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:194: browser_ =
chrome::FindLastActiveWithProfile(profile_, desktop_type_);
On 2015/01/27 17:54:31, sky wrote:
> nit: seems safer to introduce a local rather than muck with browser_

Really? This UrlHashHelper object will be deleted after the execution of the
method, so browser_ is temporary anyways. Which... may not have been obvious -
I'll add a comment.

[sky, 2015-01-27 21:54:28]

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

https://codereview.chromium.org/856733004/diff/220001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:194: browser_ =
chrome::FindLastActiveWithProfile(profile_, desktop_type_);
On 2015/01/27 18:26:11, Mike Lerman wrote:
> On 2015/01/27 17:54:31, sky wrote:
> > nit: seems safer to introduce a local rather than muck with browser_
> 
> Really? This UrlHashHelper object will be deleted after the execution of the
> method, so browser_ is temporary anyways. Which... may not have been obvious -
> I'll add a comment.

This object is temporary, but by changing browser_ you're changing the state of
URLHashHelper when you don't need to. Better to keep the changes to surrounding
state to a minimum. For example, what if the destructor assumed browser_ was the
object passed in?

[Mike Lerman, 2015-01-27 21:58:49]

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

https://codereview.chromium.org/856733004/diff/220001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:194: browser_ =
chrome::FindLastActiveWithProfile(profile_, desktop_type_);
On 2015/01/27 21:54:27, sky wrote:
> On 2015/01/27 18:26:11, Mike Lerman wrote:
> > On 2015/01/27 17:54:31, sky wrote:
> > > nit: seems safer to introduce a local rather than muck with browser_
> > 
> > Really? This UrlHashHelper object will be deleted after the execution of the
> > method, so browser_ is temporary anyways. Which... may not have been obvious
-
> > I'll add a comment.
> 
> This object is temporary, but by changing browser_ you're changing the state
of
> URLHashHelper when you don't need to. Better to keep the changes to
surrounding
> state to a minimum. For example, what if the destructor assumed browser_ was
the
> object passed in?

Okay. Done.

[sky, 2015-01-27 22:00:51]

LGTM

[tapted, 2015-01-27 22:40:26]

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc (right):

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc:146: //
Pre-load the Guest Profile so the UserManager shows quickly.
On 2015/01/27 14:59:49, Mike Lerman wrote:
> When something calls UserManager::Show(), there's a potentially asynchronous
> task to get the GuestProfile (become SystemProfile due to another CL which
just
> landed). If the GuestProfile doesn't already exist in memory, it's created
from
> disk, but if it already exists in memory then the function completes
> synchronously. No Async call -> No flakiness.

Can you capture this in a code comment? Something like "If this is not done the
system profile is created asynchronously by the user manager. Asking for its
Profile* here ensures it is created synchronously."

But TBH I would have thought a synchronous IO operation would still be banned by
thread_restrictions.cc for tests in Debug builds -- that seems to be what
ProfileImpl::CreateProfile will do in this case.. If the bots like it I guess
it's OK for a test.

https://codereview.chromium.org/856733004/diff/260001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc (right):

https://codereview.chromium.org/856733004/diff/260001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc:159:
ASSERT_TRUE(model);
Can you add

// Ensure the app list is still in a valid state, using the original profile.
EXPECT_TRUE(service->GetCurrentAppListProfile());
EXPECT_EQ(browser->profile(), service->GetCurrentAppListProfile());


i.e. this is just to re-affirm that the app list is in a sane state after a
failed show for a different profile that might not result in a profile unlock.
The flow in this test is as if someone selected a locked profile from the App
List's own profile switcher -- if they don't then unlock the profile, clicking
the app launcher again still needs to behave.

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

https://codereview.chromium.org/856733004/diff/260001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:149: // itself
after the action would be performed.
nit: now that it's base::Owned, the Closure will delete it. (i.e. no longer
deletes itself)

https://codereview.chromium.org/856733004/diff/260001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:164: const
std::string& hash_;
This can't be const-reference, just the constructor argument. Doing this results
in the string data still being owned by UserManagerScreenHandler, but there's
nothing that ties the lifetime of UrlHashHelper to UserManagerScreenHandler.

passing string arguments by const reference is talked about at
http://www.chromium.org/developers/chromium-string-usage (i.e. "For function
input parameters, prefer to pass a string by const reference instead of making a
new copy.")

[tapted, 2015-01-27 22:55:01]

.. and a couple of non-code things I missed.

https://codereview.chromium.org/856733004/diff/120001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc (right):

https://codereview.chromium.org/856733004/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc:149:
ASSERT_TRUE(model);
On 2015/01/27 14:59:49, Mike Lerman wrote:
> On 2015/01/27 02:52:54, tapted wrote:
> > On 2015/01/23 16:58:33, Mike Lerman wrote:
> > > On 2015/01/21 22:47:20, tapted wrote:
> > > > note I think this would mean that the app launcher is still configured
for
> > the
> > > > initial/default profile.
> > > > 
> > > > It's subtle, so perhaps a check to that effect, like 
> > > > 
> > > > EXPECT_TRUE(service->GetCurrentAppListProfile());
> > > > EXPECT_EQ(browser->profile(), service->GetCurrentAppListProfile());
> > > > 
> > > > 
> > > > Maybe there are corner cases for locked profiles where this would be a
> > > problem,
> > > > but that's orthogonal to this CL (hint: It's tricky - but stuff in
> > > > AppListServiceImpl::OnProfileWillBeRemoved() is probably a lot like what
> you
> > > > might want).
> > > 
> > > Ah, too much blind copy-pasting. Thanks!
> > 
> > Sorry, I meant that if that expectation is true it would mean that the app
> > launcher is still configured for the initial/default profile. (i.e. deleting
> the
> > expectation in the test won't change that).
> > 
> > My review comments actually first said "remove `app_list::AppListModel*
model`
> > entirely since it's not really used". However, the fact that it is still
> > non-NULL here is "interesting", since it means the profile switch failed. I
> > think that's a valid test result, and it would be good to keep this in to
> check
> > for regressions. (but with the extra lines and a comment so it's clear to a
> > later reader what's going on).
> > 
> > It might be the case that, in a different context, having a non-NULL model
> here
> > is *not* a valid test result. e.g. locking the *current* profile rather than
> > switching *to* a locked profile. We can worry about that context in a
> follow-up
> > (if we need to at all), but it would be good to capture what's going on
> > currently.
> 
> You mention that the fact that it's not-NULL is "interesting", but I don't see
> how it means that the ProfileSwitch failed. For an AppListService, there
doesn't
> seem to be a mechanism for removing the model, no matter what happens. Perhaps
> that's worth noting, so have done so, but leave to your discretion if I should
> change the comment, if I'm reading the code wrong.

`AppListServiceImpl::OnProfileWillBeRemoved()` has a mechanism by which the
model is removed, and all references to the Profile* are removed from
AppListFoo*

by "the profile switch failed" I mean it failed to load the locked profile. In
this test, it means the app list is a hidden window still loaded with the
original profile that InProcessBrowserTest made, and isn't locked. Since there
was no successful Show, the app list will still be configured to use that
profile if it is shown again by a mechanism like AppListService::Show() rather
than AppListService::ShowForProfile()

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

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:205: delete this;
On 2015/01/27 14:59:49, Mike Lerman wrote:
> So base::Owned will take care of deleting the object once the posted task is
> done execution? Sounds ideal for me! I can't find any documentation about that
> class.

btw this is documented in base/bind_helpers.h

but you're right that actually finding `Owned` is tricky since it's a 1-line
template function returning an OwnedWrapper

[tapted, 2015-01-28 10:58:48]

Fired off some bots to allay my IO-restrictions worries - they all check out, so
 slgtm with those 4 changes. (i.e.
 - async comment,
 - the extra profile checks,
 - ownership comment, and
 - |hash_| data member shouldn't be a reference.

[Mike Lerman, 2015-01-28 20:26:46]

All done. Thanks everyone!

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc (right):

https://codereview.chromium.org/856733004/diff/200001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc:146: //
Pre-load the Guest Profile so the UserManager shows quickly.
On 2015/01/27 22:40:26, tapted wrote:
> On 2015/01/27 14:59:49, Mike Lerman wrote:
> > When something calls UserManager::Show(), there's a potentially asynchronous
> > task to get the GuestProfile (become SystemProfile due to another CL which
> just
> > landed). If the GuestProfile doesn't already exist in memory, it's created
> from
> > disk, but if it already exists in memory then the function completes
> > synchronously. No Async call -> No flakiness.
> 
> Can you capture this in a code comment? Something like "If this is not done
the
> system profile is created asynchronously by the user manager. Asking for its
> Profile* here ensures it is created synchronously."
> 
> But TBH I would have thought a synchronous IO operation would still be banned
by
> thread_restrictions.cc for tests in Debug builds -- that seems to be what
> ProfileImpl::CreateProfile will do in this case.. If the bots like it I guess
> it's OK for a test.

Done.

https://codereview.chromium.org/856733004/diff/260001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc (right):

https://codereview.chromium.org/856733004/diff/260001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc:159:
ASSERT_TRUE(model);
On 2015/01/27 22:40:26, tapted wrote:
> Can you add
> 
> // Ensure the app list is still in a valid state, using the original profile.
> EXPECT_TRUE(service->GetCurrentAppListProfile());
> EXPECT_EQ(browser->profile(), service->GetCurrentAppListProfile());
> 
> 
> i.e. this is just to re-affirm that the app list is in a sane state after a
> failed show for a different profile that might not result in a profile unlock.
> The flow in this test is as if someone selected a locked profile from the App
> List's own profile switcher -- if they don't then unlock the profile, clicking
> the app launcher again still needs to behave.

Done.

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

https://codereview.chromium.org/856733004/diff/260001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:149: // itself
after the action would be performed.
On 2015/01/27 22:40:26, tapted wrote:
> nit: now that it's base::Owned, the Closure will delete it. (i.e. no longer
> deletes itself)

Done.

https://codereview.chromium.org/856733004/diff/260001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:164: const
std::string& hash_;
On 2015/01/27 22:40:26, tapted wrote:
> This can't be const-reference, just the constructor argument. Doing this
results
> in the string data still being owned by UserManagerScreenHandler, but there's
> nothing that ties the lifetime of UrlHashHelper to UserManagerScreenHandler.
> 
> passing string arguments by const reference is talked about at
> http://www.chromium.org/developers/chromium-string-usage (i.e. "For function
> input parameters, prefer to pass a string by const reference instead of making
a
> new copy.")

Ah that makes sense. Thanks!

[Mike Lerman, 2015-01-28 20:26:51]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2015-01-28 20:28:04]

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

[commit-bot: I haz the power, 2015-01-28 21:14:01]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-01-28 21:15:54]

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