Add load-app flag support to chrome

Add load-app flag support to chrome

BUG=432359
Committed: https://crrev.com/3ea6ffc52b1c0771295e0ab5949ba00d2335daea
Cr-Commit-Position: refs/heads/master@{#305688}

[Zachary Kuznia, 2014-11-14 22:34:46]

zork@chromium.org changed reviewers:
+ derat@chromium.org

[Zachary Kuznia, 2014-11-14 22:34:47]

Please take a look

[Daniel Erat, 2014-11-14 22:41:00]

lgtm

https://codereview.chromium.org/718133005/diff/1/chrome/browser/ui/startup/st...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/718133005/diff/1/chrome/browser/ui/startup/st...
chrome/browser/ui/startup/startup_browser_creator.cc:626: 
nit: delete extra blank line

https://codereview.chromium.org/718133005/diff/1/chrome/browser/ui/startup/st...
chrome/browser/ui/startup/startup_browser_creator.cc:631:
apps::AppLoadService::Get(last_used_profile)->Load(app_absolute_dir);
should we crash if this fails (and/or check that the dir exists before this
call)?

[James Cook, 2014-11-14 22:55:41]

jamescook@chromium.org changed reviewers:
+ jamescook@chromium.org

[James Cook, 2014-11-14 22:55:42]

https://codereview.chromium.org/718133005/diff/1/chrome/browser/ui/startup/st...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/718133005/diff/1/chrome/browser/ui/startup/st...
chrome/browser/ui/startup/startup_browser_creator.cc:632: }
drive-by comment: So --load-app is going to have different behavior in Chrome
and app_shell?  That seems annoying.  How hard would it be to make --load-app
launch the first loaded app?

[Zachary Kuznia, 2014-11-14 23:04:18]

https://codereview.chromium.org/718133005/diff/1/chrome/browser/ui/startup/st...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/718133005/diff/1/chrome/browser/ui/startup/st...
chrome/browser/ui/startup/startup_browser_creator.cc:626: 
On 2014/11/14 22:41:00, Daniel Erat wrote:
> nit: delete extra blank line

Done.

https://codereview.chromium.org/718133005/diff/1/chrome/browser/ui/startup/st...
chrome/browser/ui/startup/startup_browser_creator.cc:631:
apps::AppLoadService::Get(last_used_profile)->Load(app_absolute_dir);
On 2014/11/14 22:41:00, Daniel Erat wrote:
> should we crash if this fails (and/or check that the dir exists before this
> call)?

I'll change it to match the false return value used below on line 645.

[Zachary Kuznia, 2014-11-14 23:11:32]

https://codereview.chromium.org/718133005/diff/1/chrome/browser/ui/startup/st...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/718133005/diff/1/chrome/browser/ui/startup/st...
chrome/browser/ui/startup/startup_browser_creator.cc:632: }
On 2014/11/14 22:55:42, James Cook wrote:
> drive-by comment: So --load-app is going to have different behavior in Chrome
> and app_shell?  That seems annoying.  How hard would it be to make --load-app
> launch the first loaded app?

Done.

[Daniel Erat, 2014-11-14 23:13:58]

https://codereview.chromium.org/718133005/diff/20001/chrome/browser/ui/startu...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/718133005/diff/20001/chrome/browser/ui/startu...
chrome/browser/ui/startup/startup_browser_creator.cc:615: // Check for
--load-app.
nit: update to --load-apps (or delete the comment, since it doesn't seem to add
much on top of the if statement on the next line)

[Zachary Kuznia, 2014-11-14 23:20:41]

https://codereview.chromium.org/718133005/diff/20001/chrome/browser/ui/startu...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/718133005/diff/20001/chrome/browser/ui/startu...
chrome/browser/ui/startup/startup_browser_creator.cc:615: // Check for
--load-app.
On 2014/11/14 23:13:58, Daniel Erat wrote:
> nit: update to --load-apps (or delete the comment, since it doesn't seem to
add
> much on top of the if statement on the next line)

Done.

[Zachary Kuznia, 2014-11-14 23:44:14]

zork@chromium.org changed reviewers:
+ jhawkins@chromium.org, yoz@chromium.org

[Zachary Kuznia, 2014-11-14 23:44:15]

yoz: Could you do an OWNERS review of: apps/app_load_service.*

jhawkins: Could you do an OWNERS review of:
chrome/browser/ui/startup/startup_browser_creator.cc

Thanks,
-Zach

[Yoyo Zhou, 2014-11-14 23:50:47]

LGTM

https://codereview.chromium.org/718133005/diff/80001/apps/app_load_service.h
File apps/app_load_service.h (right):

https://codereview.chromium.org/718133005/diff/80001/apps/app_load_service.h#...
apps/app_load_service.h:65: // the extension has begun successfully.
the app

[Zachary Kuznia, 2014-11-14 23:54:12]

https://codereview.chromium.org/718133005/diff/80001/apps/app_load_service.h
File apps/app_load_service.h (right):

https://codereview.chromium.org/718133005/diff/80001/apps/app_load_service.h#...
apps/app_load_service.h:65: // the extension has begun successfully.
On 2014/11/14 23:50:47, Yoyo Zhou wrote:
> the app

Done.

[James Hawkins, 2014-11-18 00:10:47]

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

https://codereview.chromium.org/718133005/diff/100001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.cc:615: if
(command_line.HasSwitch(extensions::switches::kLoadApps) &&
This method is ~300 lines long.  Please break this out into a separate method.

[Zachary Kuznia, 2014-11-18 01:17:52]

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

https://codereview.chromium.org/718133005/diff/100001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.cc:615: if
(command_line.HasSwitch(extensions::switches::kLoadApps) &&
On 2014/11/18 00:10:47, James Hawkins wrote:
> This method is ~300 lines long.  Please break this out into a separate method.

Done.

[James Hawkins, 2014-11-18 01:24:34]

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

https://codereview.chromium.org/718133005/diff/110001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.cc:621: // Return early here
since we don't want to open a browser window.
Optional nit: Consider rewording to remove ambiguous pronoun 'we'.

https://codereview.chromium.org/718133005/diff/110001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.cc:777: bool
StartupBrowserCreator::ProcessLoadApps(const CommandLine& command_line,
nit: Document method and parameters.

https://codereview.chromium.org/718133005/diff/110001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.cc:787: bool first_app = true;
Why not just manually call tokenizer.getNext() outside the while loop and
unconditionally do the code at 794?  Then the while loop can be simplified to
the code at 799.  Unless I'm misreading this.

https://codereview.chromium.org/718133005/diff/110001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.cc:794: if
(!apps::AppLoadService::Get(last_used_profile)->LoadAndLaunch(
This is somewhat confusing.  We're launching the first app?  I thought this flag
just loads the apps?

[Zachary Kuznia, 2014-11-18 23:32:22]

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

https://codereview.chromium.org/718133005/diff/110001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.cc:621: // Return early here
since we don't want to open a browser window.
On 2014/11/18 01:24:33, James Hawkins wrote:
> Optional nit: Consider rewording to remove ambiguous pronoun 'we'.

Done.

https://codereview.chromium.org/718133005/diff/110001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.cc:777: bool
StartupBrowserCreator::ProcessLoadApps(const CommandLine& command_line,
On 2014/11/18 01:24:34, James Hawkins wrote:
> nit: Document method and parameters.

Documentation added to header file.

https://codereview.chromium.org/718133005/diff/110001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.cc:787: bool first_app = true;
On 2014/11/18 01:24:33, James Hawkins wrote:
> Why not just manually call tokenizer.getNext() outside the while loop and
> unconditionally do the code at 794?  Then the while loop can be simplified to
> the code at 799.  Unless I'm misreading this.

Done.

https://codereview.chromium.org/718133005/diff/110001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator.cc:794: if
(!apps::AppLoadService::Get(last_used_profile)->LoadAndLaunch(
On 2014/11/18 01:24:33, James Hawkins wrote:
> This is somewhat confusing.  We're launching the first app?  I thought this
flag
> just loads the apps?

The first app is loaded so that app_shell and chrome handle this flag the same
way.  On app_shell, we always want to launch an app, so the first app is
launched by convention.

[Zachary Kuznia, 2014-11-24 19:53:06]

Ping.

[James Cook, 2014-11-24 20:32:08]

lgtm

[James Hawkins, 2014-11-25 18:33:01]

lgtm

[Zachary Kuznia, 2014-11-25 19:32:08]

The CQ bit was checked by zork@chromium.org

[commit-bot: I haz the power, 2014-11-25 19:33:07]

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

[commit-bot: I haz the power, 2014-11-25 20:32:07]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2014-11-25 20:32:49]

Patchset 8 (id:??) landed as
https://crrev.com/3ea6ffc52b1c0771295e0ab5949ba00d2335daea
Cr-Commit-Position: refs/heads/master@{#305688}