Refactor handling of --show-app-list command line.

Refactor handling of --show-app-list command line.

AppListService's RecordStartupTimings is currently broken. To completely
fix, it needs to incorporate the Profile* that StartupBrowserCreatorImpl
wants to load, which is not currently passed in.

This change adds AppListService::HandleLaunchCommandLine, abstracting
away the details of initializing, showing, and recording UMA for the app
list. A follow-up will fix RecordStartupTimings.

BUG=382793
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281708

[tapted, 2014-07-01 02:51:00]

Sir Matt, PTAL

[Matt Giuca, 2014-07-02 04:28:42]

Hi, I find your commit description fairly mysterious. Before I look too closely
at this CL, could you update it to describe exactly what the "noise" is. i.e.,
what data are you not collecting which you previously collected. Also, judging
by "which resulted in only the noise being recorded", there is some data which
you are collecting which you weren't previously. Could you make it clearer what
new data is being collected?

Also could the fix for the DCHECK be separated out, because it seems orthogonal
to collecting a different set of data.

[tapted, 2014-07-02 06:25:38]

On 2014/07/02 04:28:42, Matt Giuca wrote:
> Hi, I find your commit description fairly mysterious. Before I look too
closely
> at this CL, could you update it to describe exactly what the "noise" is. i.e.,
> what data are you not collecting which you previously collected. Also, judging
> by "which resulted in only the noise being recorded", there is some data which
> you are collecting which you weren't previously. Could you make it clearer
what
> new data is being collected?

Done.

> Also could the fix for the DCHECK be separated out, because it seems
orthogonal
> to collecting a different set of data.

Done.

[Matt Giuca, 2014-07-02 07:35:18]

OK I think this looks good, but the amount of code moving around at the same
time as this is getting fixed is hurting my head. I think I'd like to see this
further split up so you do the nice refactor first, and then fix the UMA
reporting.

So CL #1 should have:
- Moving RecordShowTimings into private and renaming to RecordStartupTimings.
- Changes to startup_browser_creator_impl and the addition of
HandleLaunchCommandLine.

Then CL #2 should have:
- Addition of ProfileLoadState and g_profile_load_state.
- Passing Profile arguments in RecordStartupInfo and RecordStartupTimings, and
getting the current profile.
- The new logic in RecordFirstPaintTiming.

SG? I think both of those CLs would be fairly easy to read.

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

https://codereview.chromium.org/350883002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service.cc:93: if (g_profile_load_state ==
PROFILE_LOADED_NONE)
nit: Can this just be a DCHECK (avoiding the NOTREACHED)?

https://codereview.chromium.org/350883002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service.cc:116: void
RecordStartupTimings(AppListService* service,
nit: I think it would read nicer if this method was just above or below
RecordStartupInfo.

https://codereview.chromium.org/350883002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service.cc:117: const CommandLine&
command_line,
Your CL description mentions "RecordShowTimings"; since you've renamed this
method you should update the description.

[tapted, 2014-07-04 03:00:02]

forked into https://codereview.chromium.org/366233002/

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

https://codereview.chromium.org/350883002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service.cc:93: if (g_profile_load_state ==
PROFILE_LOADED_NONE)
On 2014/07/02 07:35:18, Matt Giuca wrote:
> nit: Can this just be a DCHECK (avoiding the NOTREACHED)?

Done.

https://codereview.chromium.org/350883002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service.cc:116: void
RecordStartupTimings(AppListService* service,
On 2014/07/02 07:35:18, Matt Giuca wrote:
> nit: I think it would read nicer if this method was just above or below
> RecordStartupInfo.

Done - I think it reads nice that way too, but note it required a prototype
declaration for anonymous::RecordFirstPaintTiming();

https://codereview.chromium.org/350883002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service.cc:117: const CommandLine&
command_line,
On 2014/07/02 07:35:18, Matt Giuca wrote:
> Your CL description mentions "RecordShowTimings"; since you've renamed this
> method you should update the description.

Done.

[Matt Giuca, 2014-07-04 04:37:15]

lgtm with optional nit, and ALSO, I don't like the CL description.

You shouldn't frame this as a "part 1 of 2" (because that implies you're leaving
a job half finished). Rather, frame this CL as a refactor, with a view to
refining UMA quality. You also refer to a bunch of stuff in the second CL which
is now irrelevant to this one. How about just this:

Refactor handling of --show-app-list command line.

Adds AppListService::HandleLaunchCommandLine, abstracting away the
details of starting up the app list.

BUG=382793

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

https://codereview.chromium.org/350883002/diff/120001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service.cc:116: void
RecordStartupTimings(AppListService* service,
Hmm that's not so nice. I'd be happy if you put it back where it was (to avoid a
forward declaration). Up to you!

https://codereview.chromium.org/350883002/diff/160001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service.cc (right):

https://codereview.chromium.org/350883002/diff/160001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service.cc:138: InitAll(launch_profile);
nit (optional): Upon consideration, I think you should take InitAll out of
HandleLaunchCommand, and call it from StartupBrowserCreatorImpl. I feel as
though initializing the AppListService and handling the command line are two
separate steps which you may wish to do either of without the other (and the
name HandleLaunchCommand doesn't imply it is also going to initialize
AppListService regardless of the launch command).

You should therefore DCHECK that |service| is not NULL after Getting it.

[tapted, 2014-07-07 07:52:35]

CL descriptions should have an element of "why the change is made" (see
http://www.chromium.org/developers/contributing-code#TOC-Change-List-Descript...
), so I've included a paragraph about that.

https://codereview.chromium.org/350883002/diff/160001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service.cc (right):

https://codereview.chromium.org/350883002/diff/160001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service.cc:138: InitAll(launch_profile);
On 2014/07/04 04:37:15, Matt Giuca wrote:
> nit (optional): Upon consideration, I think you should take InitAll out of
> HandleLaunchCommand, and call it from StartupBrowserCreatorImpl. I feel as
> though initializing the AppListService and handling the command line are two
> separate steps which you may wish to do either of without the other (and the
> name HandleLaunchCommand doesn't imply it is also going to initialize
> AppListService regardless of the launch command).

Ahhh, that's the thing - ideally, HandleLaunchCommand _shouldn't_ call Init
regardless. `Init` should only happen for each Service once. On Windows and
Linux it currently happens for each `Launch`, and that's partly why all this
misinterpretation and the original failed DCHECK started in the first place. I
think it's a bug, and is possibly making some of the other data screwy.

Encapsulating things in HandleLaunchCommand lets this be cleaned up, but
StartupBrowserCreatorImpl shouldn't care about how that happens.

> You should therefore DCHECK that |service| is not NULL after Getting it.

Done.

[tapted, 2014-07-07 23:56:05]

+msw for OWNERS (c/b/ui/startup/startup_browser_creator_impl.cc)

[msw, 2014-07-08 01:05:07]

https://codereview.chromium.org/350883002/diff/180001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service.cc (right):

https://codereview.chromium.org/350883002/diff/180001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service.cc:65: void
RecordFirstPaintTiming();
Remove this, reorder RecordStartupTimings after its actual definition.

https://codereview.chromium.org/350883002/diff/180001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service.cc:87: RecordStartupInfo(startup,
start_time);
nit: inline RecordStartupInfo and maybe use that more general name.

https://codereview.chromium.org/350883002/diff/180001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service.cc:144:
AppListService::Get(chrome::HOST_DESKTOP_TYPE_NATIVE);
nit: can you avoid explicitly qualifying the AppListService name?

[tapted, 2014-07-08 01:41:32]

https://codereview.chromium.org/350883002/diff/180001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service.cc (right):

https://codereview.chromium.org/350883002/diff/180001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service.cc:65: void
RecordFirstPaintTiming();
On 2014/07/08 01:05:07, msw wrote:
> Remove this, reorder RecordStartupTimings after its actual definition.

Done.

https://codereview.chromium.org/350883002/diff/180001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service.cc:87: RecordStartupInfo(startup,
start_time);
On 2014/07/08 01:05:07, msw wrote:
> nit: inline RecordStartupInfo and maybe use that more general name.

Done.

https://codereview.chromium.org/350883002/diff/180001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service.cc:144:
AppListService::Get(chrome::HOST_DESKTOP_TYPE_NATIVE);
On 2014/07/08 01:05:07, msw wrote:
> nit: can you avoid explicitly qualifying the AppListService name?

Of course! Done.

[msw, 2014-07-08 03:41:56]

LGTM

[tapted, 2014-07-08 03:58:12]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-07-08 04:00:06]

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

[commit-bot: I haz the power, 2014-07-08 06:01:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)

[commit-bot: I haz the power, 2014-07-08 06:10:52]

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

[commit-bot: I haz the power, 2014-07-08 06:10:52]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[tapted, 2014-07-08 06:58:11]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-07-08 06:58:40]

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

[commit-bot: I haz the power, 2014-07-08 10:29:39]

Change committed as 281708