Add the ability to load multiple apps into app_shell.

Add the ability to load multiple apps into app_shell.

  * To load the apps you can specify a comma-separated list in the command-line.
    For example: 'app_shell --app-shell-app-path=path/to/app1/,path/to/app2'

  * For now there's no API to control the focus of the apps, so the only way
    to switch apps is to either close the app_window (which will switch focus to
    the next app in the hierarchy) or using the aura::Window API directly and
    calling Focus() on the windows.

BUG=422618
Committed: https://crrev.com/45eeb4a8886278206f4ebd9e8335314d042fb987
Cr-Commit-Position: refs/heads/master@{#301135}

[lfg, 2014-10-23 20:45:37]

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

[lfg, 2014-10-23 20:45:37]

First pass, PTAL.

[James Cook, 2014-10-23 21:29:53]

Looks like the right direction to me.

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/def...
File extensions/shell/browser/default_shell_browser_main_delegate.cc (right):

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/def...
extensions/shell/browser/default_shell_browser_main_delegate.cc:34: t(path_list,
FILE_PATH_LITERAL(","));
Give this a more descriptive name than "t"

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/def...
extensions/shell/browser/default_shell_browser_main_delegate.cc:35:
extension_system->Init();
I would move this above the tokenizer declaration since it is related to the
extension system more than the looping.

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/des...
File extensions/shell/browser/desktop_controller.h (right):

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/des...
extensions/shell/browser/desktop_controller.h:50: virtual void
RemoveAppWindow(AppWindow* window) = 0;
You'll need a stub implementation of this for AthenaDesktopController. Make sure
you can build athena_main.

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/she...
File extensions/shell/browser/shell_desktop_controller.cc (right):

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/she...
extensions/shell/browser/shell_desktop_controller.cc:206: auto iter =
std::find(app_windows_.begin(), app_windows_.end(), window);
hooray for auto!

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/she...
File extensions/shell/browser/shell_extension_system.cc (right):

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/she...
extensions/shell/browser/shell_extension_system.cc:47: scoped_refptr<Extension>
extension_refptr = file_util::LoadExtension(
nit: I find it awkward to have both extension_refptr and extension. Consider
just using scoped_refptr<Extension> extension and using extension.get().

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/she...
extensions/shell/browser/shell_extension_system.cc:63:
extensions_.Insert(extension_refptr);
Do you need extensions_ at all? Can you just get the list out of
ExtensionRegistry::Get(context)->enabled_extensions()?

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/she...
File extensions/shell/browser/shell_extension_system.h (right):

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/she...
extensions/shell/browser/shell_extension_system.h:46: // Initializes the
extension system.
Document the meaning of the return value.

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/she...
extensions/shell/browser/shell_extension_system.h:49: // Launch the currently
loaded app.
Update comment please.

https://codereview.chromium.org/642383003/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_desktop_controller.cc (right):

https://codereview.chromium.org/642383003/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_desktop_controller.cc:200: void
ShellDesktopController::AddAppWindow(aura::Window* window) {
Aside (not for this CL): The create/add/remove cycle for AppWindow vs.
NativeAppWindow vs. aura::Window is very confusing. I should clean this up some
day.

https://codereview.chromium.org/642383003/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_desktop_controller.cc:217:
app_window->GetBaseWindow()->Close();  // Close() deletes |app_window_|.
|app_window_| -> |app_window|

[lfg, 2014-10-23 22:04:05]

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/def...
File extensions/shell/browser/default_shell_browser_main_delegate.cc (right):

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/def...
extensions/shell/browser/default_shell_browser_main_delegate.cc:34: t(path_list,
FILE_PATH_LITERAL(","));
On 2014/10/23 21:29:52, James Cook wrote:
> Give this a more descriptive name than "t"

Done.

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/def...
extensions/shell/browser/default_shell_browser_main_delegate.cc:35:
extension_system->Init();
On 2014/10/23 21:29:52, James Cook wrote:
> I would move this above the tokenizer declaration since it is related to the
> extension system more than the looping.

Done.

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/des...
File extensions/shell/browser/desktop_controller.h (right):

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/des...
extensions/shell/browser/desktop_controller.h:50: virtual void
RemoveAppWindow(AppWindow* window) = 0;
On 2014/10/23 21:29:52, James Cook wrote:
> You'll need a stub implementation of this for AthenaDesktopController. Make
sure
> you can build athena_main.

Done.

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/she...
File extensions/shell/browser/shell_extension_system.cc (right):

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/she...
extensions/shell/browser/shell_extension_system.cc:47: scoped_refptr<Extension>
extension_refptr = file_util::LoadExtension(
On 2014/10/23 21:29:52, James Cook wrote:
> nit: I find it awkward to have both extension_refptr and extension. Consider
> just using scoped_refptr<Extension> extension and using extension.get().

Done.

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/she...
extensions/shell/browser/shell_extension_system.cc:63:
extensions_.Insert(extension_refptr);
On 2014/10/23 21:29:53, James Cook wrote:
> Do you need extensions_ at all? Can you just get the list out of
> ExtensionRegistry::Get(context)->enabled_extensions()?

Indeed, that is not necessary. Removed.

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/she...
File extensions/shell/browser/shell_extension_system.h (right):

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/she...
extensions/shell/browser/shell_extension_system.h:46: // Initializes the
extension system.
On 2014/10/23 21:29:53, James Cook wrote:
> Document the meaning of the return value.

Switched to a void function, since the return value is not used.

https://codereview.chromium.org/642383003/diff/1/extensions/shell/browser/she...
extensions/shell/browser/shell_extension_system.h:49: // Launch the currently
loaded app.
On 2014/10/23 21:29:53, James Cook wrote:
> Update comment please.

Done.

https://codereview.chromium.org/642383003/diff/20001/extensions/shell/browser...
File extensions/shell/browser/shell_desktop_controller.cc (right):

https://codereview.chromium.org/642383003/diff/20001/extensions/shell/browser...
extensions/shell/browser/shell_desktop_controller.cc:217:
app_window->GetBaseWindow()->Close();  // Close() deletes |app_window_|.
On 2014/10/23 21:29:53, James Cook wrote:
> |app_window_| -> |app_window|

Done.

[James Cook, 2014-10-24 00:26:50]

LGTM with nits.

I suggest oshima as a reviewer for the Athena bits.

https://codereview.chromium.org/642383003/diff/80001/extensions/shell/browser...
File extensions/shell/browser/default_shell_browser_main_delegate.cc (right):

https://codereview.chromium.org/642383003/diff/80001/extensions/shell/browser...
extensions/shell/browser/default_shell_browser_main_delegate.cc:31:
CommandLine::StringType path_list =
btw, it's nice that you used StringType here... yoz is trying to keep app_shell
compatible with Windows.

https://codereview.chromium.org/642383003/diff/80001/extensions/shell/browser...
File extensions/shell/browser/shell_extension_system.h (right):

https://codereview.chromium.org/642383003/diff/80001/extensions/shell/browser...
extensions/shell/browser/shell_extension_system.h:12: #include
"extensions/common/extension_set.h"
I don't think you need this header any more.

https://codereview.chromium.org/642383003/diff/80001/extensions/shell/browser...
extensions/shell/browser/shell_extension_system.h:50: void LaunchApp(const
ExtensionId& extension_id);
optional: A lot of code in extensions (most?) just uses std::string for
extension IDs. I wouldn't feel obligated to use ExtensionId here. If you decided
to keep it you should explicitly include extension.h, where it comes from.

[lfg, 2014-10-24 15:23:15]

https://codereview.chromium.org/642383003/diff/80001/extensions/shell/browser...
File extensions/shell/browser/shell_extension_system.h (right):

https://codereview.chromium.org/642383003/diff/80001/extensions/shell/browser...
extensions/shell/browser/shell_extension_system.h:12: #include
"extensions/common/extension_set.h"
On 2014/10/24 00:26:49, James Cook wrote:
> I don't think you need this header any more.

Done.

https://codereview.chromium.org/642383003/diff/80001/extensions/shell/browser...
extensions/shell/browser/shell_extension_system.h:50: void LaunchApp(const
ExtensionId& extension_id);
On 2014/10/24 00:26:49, James Cook wrote:
> optional: A lot of code in extensions (most?) just uses std::string for
> extension IDs. I wouldn't feel obligated to use ExtensionId here. If you
decided
> to keep it you should explicitly include extension.h, where it comes from.

Done.

[lfg, 2014-10-24 15:23:57]

lfg@chromium.org changed reviewers:
+ lazyboy@chromium.org, oshima@chromium.org

[lfg, 2014-10-24 15:23:57]

lazyboy@chromium.org: Please review changes in 

web_view

oshima@chromium.org: Please review changes in 

athena

[oshima, 2014-10-24 15:33:14]

lgtm

nice to see that app shell can run multiple apps now!

[lazyboy, 2014-10-24 16:15:32]

web_view LGTM

[lfg, 2014-10-24 16:19:59]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2014-10-24 16:20:41]

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

[commit-bot: I haz the power, 2014-10-24 17:34:19]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-10-24 17:35:07]

Patchset 6 (id:??) landed as
https://crrev.com/45eeb4a8886278206f4ebd9e8335314d042fb987
Cr-Commit-Position: refs/heads/master@{#301135}