Move app_shell specific impl from athena_lib to athena_shell_lib

Move app_shell specific impl from athena_lib to athena_shell_lib
* AppActivity is now base class. There will be one for chrome
* Moved content_activity_factory.h from public to impl as ther eis no need to expose header.
* Added CreateAppActivity that takes apps::Window. Chrome
 impl will use this to create an activity. This simply returns NULL for app_shell version.

BUG=397167
R=mukai@chromium.org
TBR=jamescook@chromium.org
TEST=no functional change. all tests must pass.

Committed: https://crrev.com/025e4673571604d9bb7cbb7359030a076e8611ce
Cr-Commit-Position: refs/heads/master@{#292038}

[oshima, 2014-08-26 18:11:08]

Patchset #1 (id:1) has been deleted

[oshima, 2014-08-26 18:11:42]

Patchset #1 (id:20001) has been deleted

[oshima, 2014-08-26 20:08:51]

Patchset #2 (id:60001) has been deleted

[Jun Mukai, 2014-08-26 21:28:23]

+hashimoto

Still confused. Once hashimoto's task has been finished, I think
ShellAppActivity will be gone... then, there remains single AppActivity class,
right?  Or do we still have two AppActivity subclasses then?

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

https://codereview.chromium.org/505273002/diff/40001/athena/activity/public/a...
athena/activity/public/activity_factory.h:44: // The returned activity should
own |app_window|.
Please comment that this method will be removed once crbug.com/403726 has been
finished.

https://codereview.chromium.org/505273002/diff/40001/athena/content/app_activ...
File athena/content/app_activity_unittest.cc (right):

https://codereview.chromium.org/505273002/diff/40001/athena/content/app_activ...
athena/content/app_activity_unittest.cc:31: class TestAppActivity : public
ShellAppActivity {
Instead of depending ShellAppActivity, why not:
- inherit from AppActivity
- override GetWebContents() which returns NULL
?

[Jun Mukai, 2014-08-26 21:34:00]

https://codereview.chromium.org/505273002/diff/40001/athena/main/athena_launc...
File athena/main/athena_launcher.cc (right):

https://codereview.chromium.org/505273002/diff/40001/athena/main/athena_launc...
athena/main/athena_launcher.cc:133:
StartAthenaSession(athena::CreateContentActivityFactory(),
Thinking about this: I would say ContentAppModelBuilder and
ContentActivityFactory are essentially browser-context-keyed-services, and they
should be created through BCKS factory framework (I don't like factory-factory
patterns though... :(

That sounds a big change, I'm not saying you have to do so. But would you mind
recording a TODO for this refactoring?  (or file a bug?) and assign to me.

[oshima, 2014-08-26 21:43:19]

On 2014/08/26 21:28:23, Jun Mukai wrote:
> +hashimoto
> 
> Still confused. Once hashimoto's task has been finished, I think
> ShellAppActivity will be gone... then, there remains single AppActivity class,
> right?  Or do we still have two AppActivity subclasses then?

Yes, it'll be removed. This is so that we can move in parallel. This (and other
changes that follow) change should also makes his work a bit easier as I'm
adding the code that uses apps::AppWindow, AppsClient for chrome.

> 
>
https://codereview.chromium.org/505273002/diff/40001/athena/activity/public/a...
> File athena/activity/public/activity_factory.h (right):
> 
>
https://codereview.chromium.org/505273002/diff/40001/athena/activity/public/a...
> athena/activity/public/activity_factory.h:44: // The returned activity should
> own |app_window|.
> Please comment that this method will be removed once crbug.com/403726 has been
> finished.
> 
>
https://codereview.chromium.org/505273002/diff/40001/athena/content/app_activ...
> File athena/content/app_activity_unittest.cc (right):
> 
>
https://codereview.chromium.org/505273002/diff/40001/athena/content/app_activ...
> athena/content/app_activity_unittest.cc:31: class TestAppActivity : public
> ShellAppActivity {
> Instead of depending ShellAppActivity, why not:
> - inherit from AppActivity
> - override GetWebContents() which returns NULL
> ?

[oshima, 2014-08-26 21:43:27]

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

https://codereview.chromium.org/505273002/diff/40001/athena/activity/public/a...
athena/activity/public/activity_factory.h:44: // The returned activity should
own |app_window|.
On 2014/08/26 21:28:23, Jun Mukai wrote:
> Please comment that this method will be removed once crbug.com/403726 has been
> finished.

Done.

https://codereview.chromium.org/505273002/diff/40001/athena/content/app_activ...
File athena/content/app_activity_unittest.cc (right):

https://codereview.chromium.org/505273002/diff/40001/athena/content/app_activ...
athena/content/app_activity_unittest.cc:31: class TestAppActivity : public
ShellAppActivity {
On 2014/08/26 21:28:23, Jun Mukai wrote:
> Instead of depending ShellAppActivity, why not:
> - inherit from AppActivity
> - override GetWebContents() which returns NULL
> ?

That's better, thanks. Done.

[Jun Mukai, 2014-08-26 21:46:36]

lgtm

looked at crbug.com/403726 and it still looks a long way to go. I'm okay to
proceed with this for now.

[oshima, 2014-08-26 21:56:55]

https://codereview.chromium.org/505273002/diff/40001/athena/main/athena_launc...
File athena/main/athena_launcher.cc (right):

https://codereview.chromium.org/505273002/diff/40001/athena/main/athena_launc...
athena/main/athena_launcher.cc:133:
StartAthenaSession(athena::CreateContentActivityFactory(),
On 2014/08/26 21:33:59, Jun Mukai wrote:
> Thinking about this: I would say ContentAppModelBuilder and
> ContentActivityFactory are essentially browser-context-keyed-services, and
they
> should be created through BCKS factory framework (I don't like factory-factory
> patterns though... :(
> 
> That sounds a big change, I'm not saying you have to do so. But would you mind
> recording a TODO for this refactoring?  (or file a bug?) and assign to me.

Good point. We don't need it now, but will be necessary to support incognito
windows and multi-profile. I'll file a bug and work on it
separately.

[oshima, 2014-08-26 22:02:41]

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

[oshima, 2014-08-26 22:02:41]

TBR'ing jamescook for moving "+extensions/shell/browser/shell_app_window.h" deps
from athena/content/DEPS to athena/content/shell/DEPS

[oshima, 2014-08-26 22:02:48]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-08-26 22:04:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/505273002/80001

[commit-bot: I haz the power, 2014-08-26 23:52:05]

Committed patchset #2 (80001) as daef0890f7735a4cb4464231c149b8088d120a8b

[commit-bot: I haz the power, 2014-09-10 02:47:36]

Patchset 2 (id:??) landed as
https://crrev.com/025e4673571604d9bb7cbb7359030a076e8611ce
Cr-Commit-Position: refs/heads/master@{#292038}