Avoid loading the last used browser profile in app_controller_mac when it's not needed.

Avoid loading the last used browser profile in app_controller_mac when it's not needed.

Currently -[AppController applicationDidFinishLaunching:] and
-[AppController windowLayeringDidChange:] can trigger a synchronous load
of the last used _browser_ profile, even when no browsers are being
opened, and that profile is not required. This causes delays when the
app launcher or app shims are launched for a profile that doesn't match
the last used browser profile, because both profiles must be loaded and
initialized.

This change moves ChromeBrowserMain's GetStartupProfilePath() from an
anonymous namespace to startup_browser_creator.h so that AppController
can share the logic that determines the profile loaded when the browser
process starts up.

The shim process is tweaked slightly to ensure it passes
`--show-app-list` for the app launcher shim, similar to the Windows app
launcher. This allows a newly created browser process to determine that
the first thing it will be doing is showing a launcher.

BUG=178260, 262848
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245199

[tapted, 2014-01-08 12:01:34]

Hi Nico, could you take a look? Interesting parts are some small logic changes
in app_controller_mac.mm, but maybe you know a more appropriate place for
GetStartupProfilePath(..) too.

(The regression test was fun.. hit a lot of dead-ends with TestingProfile,
JsonPrefStore, TestingBrowserProcess, etc. before settling on this.)

Note this is also inspired by crrev/117683002 (more launcher startup
improvements), which this CL split out from.

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

https://codereview.chromium.org/127343004/diff/210001/chrome/browser/app_cont...
chrome/browser/app_controller_mac.mm:571: } else if
(chrome::GetTotalBrowserCount() == 0) {
note: This was added in r111039 to handle deleting the current profile when
there are no other windows, which will close the browser window. (i.e., it
should only care about BrowserWindowController).

https://codereview.chromium.org/127343004/diff/210001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/127343004/diff/210001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.cc:799: base::FilePath
GetStartupProfilePath(const base::FilePath& user_data_dir,
note: this is verbatim, except the --show-app-list check now occurs before
--profile-directory (--show-app-list takes precedence and it ignores
--profile-directory).

[tapted, 2014-01-09 22:07:14]

thakis: ping?

[tapted, 2014-01-10 05:21:32]

thakis@google -> thakis@chromium (sorry!).

[Nico, 2014-01-14 02:06:34]

Looks harmless enough, lgtm. Thanks for including a test.

https://codereview.chromium.org/127343004/diff/470001/apps/app_shim/chrome_ma...
File apps/app_shim/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/127343004/diff/470001/apps/app_shim/chrome_ma...
apps/app_shim/chrome_main_app_mode_mac.mm:610:
command_line.AppendSwitch(switches::kShowAppList);
I didn't get why this is needed.

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

https://codereview.chromium.org/127343004/diff/470001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc:64: // the
browser instance is only ever associated with a single profile.
Is this still true? I thought cros had multiprofile these days.

https://codereview.chromium.org/127343004/diff/470001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc:202:
CHECK(second_profile_temp_dir_.Set(profile_path));
(something like this seems to be done in various tests; maybe there should be
some profile test helper)

[Nico, 2014-01-14 02:07:11]

(And sorry about the delay :-/)

[tapted, 2014-01-14 04:02:28]

Thanks Nico!

https://codereview.chromium.org/127343004/diff/470001/apps/app_shim/chrome_ma...
File apps/app_shim/chrome_main_app_mode_mac.mm (right):

https://codereview.chromium.org/127343004/diff/470001/apps/app_shim/chrome_ma...
apps/app_shim/chrome_main_app_mode_mac.mm:610:
command_line.AppendSwitch(switches::kShowAppList);
On 2014/01/14 02:06:34, Nico wrote:
> I didn't get why this is needed.

Added a comment. This is the "Chrome not running" branch: passing
--show-app-list allows browser startup codepaths to load the best profile.
Otherwise, it's not known until the UI runloop has spun up, and the shim has
connected to the IPC domain socket.

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

https://codereview.chromium.org/127343004/diff/470001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc:64: // the
browser instance is only ever associated with a single profile.
On 2014/01/14 02:06:34, Nico wrote:
> Is this still true? I thought cros had multiprofile these days.

A good point! It's still quite experimental (tracker: http://crbug.com/206815),
but I just had a play and it's come a long way. I've updated the comment and
filed http://crbug.com/334129 to track standing up more tests for
--multi-profiles on CrOS. The profile-switching code paths are different though.
The tests here focus on switching via the app launcher UI, so don't make sense
to stand up (at least in their current form) on CrOS.

https://codereview.chromium.org/127343004/diff/470001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_interactive_uitest.cc:202:
CHECK(second_profile_temp_dir_.Set(profile_path));
On 2014/01/14 02:06:34, Nico wrote:
> (something like this seems to be done in various tests; maybe there should be
> some profile test helper)

I think I would like that too - there is not much precedent for testing multi
profile stuff to make writing new tests easy. This one is a bit special though,
since it doesn't want to load the profile until later whereas, e.g.,
browser_close_browsertest does load the profile.

Except, looking at BrowserCloseTest, it looks like it has run into the same
problem that profile_browsertest.cc has gotten into: all of its tests are
disabled for being flaky, because it takes too long to fully construct two
browser profiles and run complicated tests on them. Here I think it's OK because
--show-app-list skips a bunch of browser initialization code and the durations
in the bot output are comparable with the surrounding tests.

However, performance_monitor_browsertest.cc manages OK with its
`TwoProfileUncleanExit` too, and could share some logic with this. And I think
adding to the test harness to encourage non-flaky multi-profile test coverage
would be a good thing. I've made http://crbug.com/334133 (since the refactor
mgiht change the behaviour of the other tests) and assigned it me to pick up
this milestone.

[commit-bot: I haz the power, 2014-01-14 07:22:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/127343004/580001

[commit-bot: I haz the power, 2014-01-14 08:35:06]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2014-01-15 22:49:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/127343004/680002

[commit-bot: I haz the power, 2014-01-16 05:45:23]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-16 07:06:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tapted@chromium.org/127343004/680002

[commit-bot: I haz the power, 2014-01-16 15:47:41]

Change committed as 245199