Introduces AppActivity and handler of chrome.shell API.

Introduces AppActivity and handler of chrome.shell API.

Now, clicking the app item just emits onLaunched event, and the
app will create an app window on the event handler. The API handler
creates a ShellAppWindow for app_shell, but it should create
an activity on athena.

BUG=380421
R=jamescook@chromium.org, oshima@chromium.org
TEST=manually

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

[James Cook, 2014-06-16 16:12:31]

Looking good, just a few small comments.

https://codereview.chromium.org/335003003/diff/30001/apps/shell/browser/defau...
File apps/shell/browser/default_shell_app_window_controller.h (right):

https://codereview.chromium.org/335003003/diff/30001/apps/shell/browser/defau...
apps/shell/browser/default_shell_app_window_controller.h:17: // app_window.
Nice that you explained it handles a single window.

https://codereview.chromium.org/335003003/diff/30001/apps/shell/browser/defau...
apps/shell/browser/default_shell_app_window_controller.h:20:
DefaultShellAppWindowController(
explicit

https://codereview.chromium.org/335003003/diff/30001/apps/shell/browser/defau...
apps/shell/browser/default_shell_app_window_controller.h:24: // Overridden from
ShellAppWindowController:
nit: The extensions code prefers "ShellAppWindowController implementation."  I
don't like it, but for app_shell I'm trying to use it.

https://codereview.chromium.org/335003003/diff/30001/athena/activity/public/a...
File athena/activity/public/activity_factory.h (right):

https://codereview.chromium.org/335003003/diff/30001/athena/activity/public/a...
athena/activity/public/activity_factory.h:37: virtual Activity*
CreateAppActivity(content::WebContents* web_contents) = 0;
Not for this CL:  Will Athena ever support multi-profile? If so you might want
to make the ActivityFactory a KeyedService (on the BrowserContext).

https://codereview.chromium.org/335003003/diff/30001/athena/main/athena_app_w...
File athena/main/athena_app_window_controller.cc (right):

https://codereview.chromium.org/335003003/diff/30001/athena/main/athena_app_w...
athena/main/athena_app_window_controller.cc:17: class AppWindowOwner : public
content::WebContentsObserver {
Could you document why the WebContents needs to own the app window? That seems a
little backwards -- I would expect the app to own the contents.  I guess this is
because the activity manager owns the WebContents?

https://codereview.chromium.org/335003003/diff/30001/athena/main/athena_app_w...
athena/main/athena_app_window_controller.cc:32:
DISALLOW_COPY_AND_ASSIGN(AppWindowOwner);
nit: blank line above

https://codereview.chromium.org/335003003/diff/30001/athena/main/athena_app_w...
File athena/main/athena_app_window_controller.h (right):

https://codereview.chromium.org/335003003/diff/30001/athena/main/athena_app_w...
athena/main/athena_app_window_controller.h:13: // The shell app window
controller for athena.
nit: Could you describe briefly what this class is expected to do?  Open apps in
fullscreen?  Just associate app windows with the window manager?

[Jun Mukai, 2014-06-16 17:46:52]

https://codereview.chromium.org/335003003/diff/30001/apps/shell/browser/defau...
File apps/shell/browser/default_shell_app_window_controller.h (right):

https://codereview.chromium.org/335003003/diff/30001/apps/shell/browser/defau...
apps/shell/browser/default_shell_app_window_controller.h:20:
DefaultShellAppWindowController(
On 2014/06/16 16:12:30, James Cook wrote:
> explicit

Done.

https://codereview.chromium.org/335003003/diff/30001/apps/shell/browser/defau...
apps/shell/browser/default_shell_app_window_controller.h:24: // Overridden from
ShellAppWindowController:
On 2014/06/16 16:12:30, James Cook wrote:
> nit: The extensions code prefers "ShellAppWindowController implementation."  I
> don't like it, but for app_shell I'm trying to use it.

Done.

https://codereview.chromium.org/335003003/diff/30001/athena/activity/public/a...
File athena/activity/public/activity_factory.h (right):

https://codereview.chromium.org/335003003/diff/30001/athena/activity/public/a...
athena/activity/public/activity_factory.h:37: virtual Activity*
CreateAppActivity(content::WebContents* web_contents) = 0;
On 2014/06/16 16:12:30, James Cook wrote:
> Not for this CL:  Will Athena ever support multi-profile? If so you might want
> to make the ActivityFactory a KeyedService (on the BrowserContext).
> 

I think eventually it will, but not now.

https://codereview.chromium.org/335003003/diff/30001/athena/main/athena_app_w...
File athena/main/athena_app_window_controller.cc (right):

https://codereview.chromium.org/335003003/diff/30001/athena/main/athena_app_w...
athena/main/athena_app_window_controller.cc:17: class AppWindowOwner : public
content::WebContentsObserver {
On 2014/06/16 16:12:30, James Cook wrote:
> Could you document why the WebContents needs to own the app window? That seems
a
> little backwards -- I would expect the app to own the contents.  I guess this
is
> because the activity manager owns the WebContents?

Actually WebContents doesn't own the app window, the app window still owns
WebContents (and the WebContents is attached to an AppActivity), and the
AppActivity is owned by the activity manager but the AppActivity cannot own the
app window for the dependency reason (possibly).

That's why I've added an owner class, which owns app window and removes the app
window when the AppActivity is closed.

This is really weird, and possibly a bad design though...

https://codereview.chromium.org/335003003/diff/30001/athena/main/athena_app_w...
athena/main/athena_app_window_controller.cc:32:
DISALLOW_COPY_AND_ASSIGN(AppWindowOwner);
On 2014/06/16 16:12:31, James Cook wrote:
> nit: blank line above

Done.

https://codereview.chromium.org/335003003/diff/30001/athena/main/athena_app_w...
File athena/main/athena_app_window_controller.h (right):

https://codereview.chromium.org/335003003/diff/30001/athena/main/athena_app_w...
athena/main/athena_app_window_controller.h:13: // The shell app window
controller for athena.
On 2014/06/16 16:12:31, James Cook wrote:
> nit: Could you describe briefly what this class is expected to do?  Open apps
in
> fullscreen?  Just associate app windows with the window manager?

The web contents are embedded into an athena's activity; added comments.

[Jun Mukai, 2014-06-16 17:58:11]

https://codereview.chromium.org/335003003/diff/30001/athena/main/athena_app_w...
File athena/main/athena_app_window_controller.cc (right):

https://codereview.chromium.org/335003003/diff/30001/athena/main/athena_app_w...
athena/main/athena_app_window_controller.cc:17: class AppWindowOwner : public
content::WebContentsObserver {
On 2014/06/16 17:46:52, Jun Mukai wrote:
> On 2014/06/16 16:12:30, James Cook wrote:
> > Could you document why the WebContents needs to own the app window? That
seems
> a
> > little backwards -- I would expect the app to own the contents.  I guess
this
> is
> > because the activity manager owns the WebContents?
> 
> Actually WebContents doesn't own the app window, the app window still owns
> WebContents (and the WebContents is attached to an AppActivity), and the
> AppActivity is owned by the activity manager but the AppActivity cannot own
the
> app window for the dependency reason (possibly).
> 
> That's why I've added an owner class, which owns app window and removes the
app
> window when the AppActivity is closed.
> 
> This is really weird, and possibly a bad design though...

Thinking about this, and possibly better to make AppActivity own AppWindow.
Changed the code.

[oshima, 2014-06-16 18:40:08]

The design is ok for now, but I think we need two changes in near future.

1) Use app_window instead of app_shell_window. We need the features app_window
provides, unlike app_shell.
2) Update the activity framework so that ActivityView gets created upon some
events (like view ready, shown etc).

https://codereview.chromium.org/335003003/diff/90001/apps/shell/browser/shell...
File apps/shell/browser/shell_desktop_controller.cc (right):

https://codereview.chromium.org/335003003/diff/90001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.cc:194: return
app_window_controller_
then you can remove NULL check?

https://codereview.chromium.org/335003003/diff/90001/apps/shell/browser/shell...
File apps/shell/browser/shell_desktop_controller.h (right):

https://codereview.chromium.org/335003003/diff/90001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.h:76: void
SetAppWindowController(ShellAppWindowController* app_window_controller);
looks like you can just pass it in ctor?

https://codereview.chromium.org/335003003/diff/90001/athena/content/app_activ...
File athena/content/app_activity.cc (right):

https://codereview.chromium.org/335003003/diff/90001/athena/content/app_activ...
athena/content/app_activity.cc:14: // AppActivity is similar to WebActivity but
doesn't have accelerators.
Apps does not support accelerator because there was some technical issues, not
really by design. (jamescook tried it before but didn't work well) Just FYI.

[Jun Mukai, 2014-06-16 18:48:15]

https://codereview.chromium.org/335003003/diff/90001/apps/shell/browser/shell...
File apps/shell/browser/shell_desktop_controller.cc (right):

https://codereview.chromium.org/335003003/diff/90001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.cc:194: return
app_window_controller_
On 2014/06/16 18:40:08, oshima wrote:
> then you can remove NULL check?

Removed, anyways app_window_controller_ needs to be specified beforehand.

https://codereview.chromium.org/335003003/diff/90001/apps/shell/browser/shell...
File apps/shell/browser/shell_desktop_controller.h (right):

https://codereview.chromium.org/335003003/diff/90001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.h:76: void
SetAppWindowController(ShellAppWindowController* app_window_controller);
On 2014/06/16 18:40:08, oshima wrote:
> looks like you can just pass it in ctor?

The DefaultShellAppWindowController (of app_shell) requires
ShellDesktopController's reference -- actually it needs the pointer to the root
window to create the actual window correctly (on the other hand, this is not
required at all in Athena).

After considering about this, I think it is better to create
ShellAppWindowController properly *after* ShellDesktopController is created, and
then set it. See app_shell's default_shell_browser_main_delegate.cc FYI.

https://codereview.chromium.org/335003003/diff/90001/athena/content/app_activ...
File athena/content/app_activity.cc (right):

https://codereview.chromium.org/335003003/diff/90001/athena/content/app_activ...
athena/content/app_activity.cc:14: // AppActivity is similar to WebActivity but
doesn't have accelerators.
On 2014/06/16 18:40:08, oshima wrote:
> Apps does not support accelerator because there was some technical issues, not
> really by design. (jamescook tried it before but didn't work well) Just FYI.

Removed this comment, but do you mean eventually we should add the same
accelerator handlers to AppWindow?  Then I should put a TODO here.

[oshima, 2014-06-16 18:53:16]

lgtm

https://codereview.chromium.org/335003003/diff/90001/apps/shell/browser/shell...
File apps/shell/browser/shell_desktop_controller.h (right):

https://codereview.chromium.org/335003003/diff/90001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.h:76: void
SetAppWindowController(ShellAppWindowController* app_window_controller);
On 2014/06/16 18:48:15, Jun Mukai wrote:
> On 2014/06/16 18:40:08, oshima wrote:
> > looks like you can just pass it in ctor?
> 
> The DefaultShellAppWindowController (of app_shell) requires
> ShellDesktopController's reference -- actually it needs the pointer to the
root
> window to create the actual window correctly (on the other hand, this is not
> required at all in Athena).
> 
> After considering about this, I think it is better to create
> ShellAppWindowController properly *after* ShellDesktopController is created,
and
> then set it. See app_shell's default_shell_browser_main_delegate.cc FYI.

I think this part needs ovehaul later on, so I'm fine if this is necessary for
now.

https://codereview.chromium.org/335003003/diff/90001/athena/content/app_activ...
File athena/content/app_activity.cc (right):

https://codereview.chromium.org/335003003/diff/90001/athena/content/app_activ...
athena/content/app_activity.cc:14: // AppActivity is similar to WebActivity but
doesn't have accelerators.
On 2014/06/16 18:48:15, Jun Mukai wrote:
> On 2014/06/16 18:40:08, oshima wrote:
> > Apps does not support accelerator because there was some technical issues,
not
> > really by design. (jamescook tried it before but didn't work well) Just FYI.
> 
> Removed this comment, but do you mean eventually we should add the same
> accelerator handlers to AppWindow?  Then I should put a TODO here.
yes please.

[Jun Mukai, 2014-06-16 18:59:33]

https://codereview.chromium.org/335003003/diff/90001/athena/content/app_activ...
File athena/content/app_activity.cc (right):

https://codereview.chromium.org/335003003/diff/90001/athena/content/app_activ...
athena/content/app_activity.cc:14: // AppActivity is similar to WebActivity but
doesn't have accelerators.
On 2014/06/16 18:53:16, oshima wrote:
> On 2014/06/16 18:48:15, Jun Mukai wrote:
> > On 2014/06/16 18:40:08, oshima wrote:
> > > Apps does not support accelerator because there was some technical issues,
> not
> > > really by design. (jamescook tried it before but didn't work well) Just
FYI.
> > 
> > Removed this comment, but do you mean eventually we should add the same
> > accelerator handlers to AppWindow?  Then I should put a TODO here.
> yes please.

Done.

[Jun Mukai, 2014-06-16 18:59:39]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-06-16 19:00:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mukai@chromium.org/335003003/130001

[James Cook, 2014-06-16 20:20:32]

lgtm

https://codereview.chromium.org/335003003/diff/130001/apps/shell/browser/shel...
File apps/shell/browser/shell_desktop_controller.h (right):

https://codereview.chromium.org/335003003/diff/130001/apps/shell/browser/shel...
apps/shell/browser/shell_desktop_controller.h:76: void
SetAppWindowController(ShellAppWindowController* app_window_controller);
nit: Comment that it takes ownership.

[Jun Mukai, 2014-06-16 20:29:17]

The CQ bit was unchecked by mukai@chromium.org

[Jun Mukai, 2014-06-16 20:35:06]

On 2014/06/16 20:20:32, James Cook wrote:
> lgtm
> 
>
https://codereview.chromium.org/335003003/diff/130001/apps/shell/browser/shel...
> File apps/shell/browser/shell_desktop_controller.h (right):
> 
>
https://codereview.chromium.org/335003003/diff/130001/apps/shell/browser/shel...
> apps/shell/browser/shell_desktop_controller.h:76: void
> SetAppWindowController(ShellAppWindowController* app_window_controller);
> nit: Comment that it takes ownership.

Also noticed that clicking an item in app-list will cause a crash unless --app
flag is specified.
Created a CL https://codereview.chromium.org/336263002/, let me proceed that
first and then land this.

[Jun Mukai, 2014-06-16 20:44:21]

https://codereview.chromium.org/335003003/diff/130001/apps/shell/browser/shel...
File apps/shell/browser/shell_desktop_controller.h (right):

https://codereview.chromium.org/335003003/diff/130001/apps/shell/browser/shel...
apps/shell/browser/shell_desktop_controller.h:76: void
SetAppWindowController(ShellAppWindowController* app_window_controller);
On 2014/06/16 20:20:32, James Cook wrote:
> nit: Comment that it takes ownership.

Done.

[Jun Mukai, 2014-06-17 16:54:45]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-06-17 16:57:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mukai@chromium.org/335003003/190001

[commit-bot: I haz the power, 2014-06-17 20:07:16]

Change committed as 277841