Make management.launchApp() work for hosted apps on Athena

Make management.launchApp() work for hosted apps on Athena

BUG=422729
TEST=Manual, see bug

Committed: https://crrev.com/2da57686386e1a046ea3db8600b2548a8ee6df34
Cr-Commit-Position: refs/heads/master@{#300497}

[pkotwicz, 2014-10-12 00:35:26]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2014-10-12 00:46:39]

Patchset #1 (id:130001) has been deleted

[pkotwicz, 2014-10-12 01:36:14]

Patchset #1 (id:180001) has been deleted

[pkotwicz, 2014-10-12 01:36:20]

Patchset #1 (id:240001) has been deleted

[pkotwicz, 2014-10-12 01:36:25]

Patchset #1 (id:360001) has been deleted

[pkotwicz, 2014-10-12 01:37:36]

Patchset #1 (id:380001) has been deleted

[pkotwicz, 2014-10-12 01:39:21]

Patchset #1 (id:430001) has been deleted

[pkotwicz, 2014-10-12 01:40:42]

Patchset #1 (id:460001) has been deleted

[pkotwicz, 2014-10-12 01:41:44]

Patchset #1 (id:480001) has been deleted

[pkotwicz, 2014-10-12 01:45:00]

Patchset #1 (id:500001) has been deleted

[pkotwicz, 2014-10-12 01:46:57]

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

[pkotwicz, 2014-10-12 01:46:58]

Oshima, can you please take a look?

I wasn't too sure how to split out the athena-specific parts

[oshima, 2014-10-13 17:53:27]

can you get review from extensions team first?
I wonder if we can componentize the app launch API at least.

https://codereview.chromium.org/635233008/diff/520001/chrome/browser/ui/athen...
File chrome/browser/ui/athena/extensions/application_launch_hosted_app_athena.cc
(right):

https://codereview.chromium.org/635233008/diff/520001/chrome/browser/ui/athen...
chrome/browser/ui/athena/extensions/application_launch_hosted_app_athena.cc:3:
// found in the LICENSE file.
We'd like to keep the athena code under /chrome minimum.
Can you inject the code from athena instead?

[pkotwicz, 2014-10-13 20:43:27]

pkotwicz@chromium.org changed reviewers:
+ asargent@chromium.org

[pkotwicz, 2014-10-13 20:43:27]

asargent@ can you please take a look?

[asargent_no_longer_on_chrome, 2014-10-14 16:35:58]

Are there tests which will ensure that we don't regress this in the future?

Also note that I am not in chrome/browser/ui/extensions/OWNERS so you'll need to
find another reviewer.

[pkotwicz, 2014-10-14 16:50:45]

None of the browser tests currently run on Athena. Enabling a subset of the API
tests and extension tests on Athena is an upcoming challenge. I do not yet have
a plan on how to tackle tests yet.

I want to first fix things sufficiently that the designers can try apps out on
Athena, then finish fixing crashes in installing apps, then look into enabling
extension api tests and browser tests on Athena

[oshima, 2014-10-14 17:30:06]

On 2014/10/14 16:50:45, pkotwicz wrote:
> None of the browser tests currently run on Athena.

We do have browser tests now, although we don't have bots yet. (We're getting
push back unfortunately)

> Enabling a subset of the API
> tests and extension tests on Athena is an upcoming challenge. I do not yet
have
> a plan on how to tackle tests yet.
> 
> I want to first fix things sufficiently that the designers can try apps out on
> Athena, then finish fixing crashes in installing apps, then look into enabling
> extension api tests and browser tests on Athena

[pkotwicz, 2014-10-14 17:47:37]

Oshima, you are right we have browser_tests on Athena. We have disabled all of
the existing browser_tests on Athena though

In particular:
['use_athena==1', {
    'sources!': [
        '<@(chrome_browser_tests_sources)',
    ],
]
in chrome_tests.gypi

[oshima, 2014-10-14 20:23:38]

On 2014/10/14 17:47:37, pkotwicz wrote:
> Oshima, you are right we have browser_tests on Athena. We have disabled all of
> the existing browser_tests on Athena though
> 
> In particular:
> ['use_athena==1', {
>     'sources!': [
>         '<@(chrome_browser_tests_sources)',
>     ],
> ]
> in chrome_tests.gypi

We disabled them first because we don't know which tests make sense on athena.
You may re-enable tests that are necessary/makes sense on Athena.

[asargent_no_longer_on_chrome, 2014-10-14 20:47:29]

asargent@chromium.org changed reviewers:
- asargent@chromium.org

[pkotwicz, 2014-10-14 22:30:02]

pkotwicz@chromium.org changed reviewers:
+ benwells@chromium.org

[pkotwicz, 2014-10-14 22:30:02]

benwells@ can you please take a look?

After a discussion with Oshima, we agreed that it made sense to investigate in
bringing up relevant extension API tests after this CL lands

[benwells, 2014-10-16 03:43:02]

Was the discussion about testing offline? Why did it make sense to add the tests
after landing this?

https://codereview.chromium.org/635233008/diff/520001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/application_launch.cc (right):

https://codereview.chromium.org/635233008/diff/520001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/application_launch.cc:39: #if !defined(USE_ATHENA)
I'm a bit ignorant about Athena. I thought it wasn't going to include any chrome
code - has that changed?

https://codereview.chromium.org/635233008/diff/520001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/application_launch.cc:175: tab =
OpenHostedAppWindow(params, url);
This name is inaccurate - this code will be hit for legacy packaged apps as well
as hosted apps.

I think putting the previous name back is probably better.

https://codereview.chromium.org/635233008/diff/520001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/application_launch.cc:178: tab =
OpenHostedAppTab(params, url);
Same here.

[pkotwicz, 2014-10-16 15:59:03]

https://codereview.chromium.org/635233008/diff/520001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/application_launch.cc (right):

https://codereview.chromium.org/635233008/diff/520001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/application_launch.cc:39: #if !defined(USE_ATHENA)
Initially we intended on making componentizing all of Chrome a prerequisite for
Athena. We have realized that this goal was overly ambitious. Athena will depend
on Chrome to the same extent as Android does. In particular, Athena will not
compile chrome/browser/ui/browser.cc Athena does want to use most parts of
Chrome though (apps, web store, history, safe browsing, sync, popup blocker,
...)

Yes, I discussed making the extension API tests (e.g.
webstore_private_apitest.cc) work on Athena offline with Oshima. The decision on
making these work on Athena as a follow on CL is that making these tests work on
Athena is quite a bit of work. In order to make webstore_private_apitest.cc work
on Athena we have to make the test not rely on the existance of a Browser
object. e.g. ui_test_utils::NavigateToURL() currently relies on the existance of
a Browser object.

https://codereview.chromium.org/635233008/diff/520001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/application_launch.cc:175: tab =
OpenHostedAppWindow(params, url);
Thanks for the comment! I wasn't aware of legacy packaged apps.

What do you suggest renaming application_launch_hosted_app.cc to?

[oshima, 2014-10-16 16:20:59]

https://codereview.chromium.org/635233008/diff/520001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/application_launch.cc (right):

https://codereview.chromium.org/635233008/diff/520001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/application_launch.cc:39: #if !defined(USE_ATHENA)
On 2014/10/16 15:59:03, pkotwicz wrote:
> Initially we intended on making componentizing all of Chrome a prerequisite
for
> Athena. We have realized that this goal was overly ambitious. Athena will
depend
> on Chrome to the same extent as Android does. In particular, Athena will not
> compile chrome/browser/ui/browser.cc Athena does want to use most parts of
> Chrome though (apps, web store, history, safe browsing, sync, popup blocker,
> ...)

Note that we haven't given up athena w/o chrome. Our long term goal is still
athena w/o chrome, but it will take years. We decided to build athena+chrome
first, and decrease chrome dependencies step by step instead.


> 
> Yes, I discussed making the extension API tests (e.g.
> webstore_private_apitest.cc) work on Athena offline with Oshima. The decision
on
> making these work on Athena as a follow on CL is that making these tests work
on
> Athena is quite a bit of work. In order to make webstore_private_apitest.cc
work
> on Athena we have to make the test not rely on the existance of a Browser
> object. e.g. ui_test_utils::NavigateToURL() currently relies on the existance
of
> a Browser object.

[benwells, 2014-10-16 20:47:55]

https://codereview.chromium.org/635233008/diff/520001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/application_launch.cc (right):

https://codereview.chromium.org/635233008/diff/520001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/application_launch.cc:39: #if !defined(USE_ATHENA)
On 2014/10/16 16:20:59, oshima wrote:
> On 2014/10/16 15:59:03, pkotwicz wrote:
> > Initially we intended on making componentizing all of Chrome a prerequisite
> for
> > Athena. We have realized that this goal was overly ambitious. Athena will
> depend
> > on Chrome to the same extent as Android does. In particular, Athena will not
> > compile chrome/browser/ui/browser.cc Athena does want to use most parts of
> > Chrome though (apps, web store, history, safe browsing, sync, popup blocker,
> > ...)
> 
> Note that we haven't given up athena w/o chrome. Our long term goal is still
> athena w/o chrome, but it will take years. We decided to build athena+chrome
> first, and decrease chrome dependencies step by step instead.
> 
> 
> > 
> > Yes, I discussed making the extension API tests (e.g.
> > webstore_private_apitest.cc) work on Athena offline with Oshima. The
decision
> on
> > making these work on Athena as a follow on CL is that making these tests
work
> on
> > Athena is quite a bit of work. In order to make webstore_private_apitest.cc
> work
> > on Athena we have to make the test not rely on the existance of a Browser
> > object. e.g. ui_test_utils::NavigateToURL() currently relies on the
existance
> of
> > a Browser object.
> 

OK that all makes sense, thanks for the update.

https://codereview.chromium.org/635233008/diff/520001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/application_launch.cc:175: tab =
OpenHostedAppWindow(params, url);
On 2014/10/16 15:59:03, pkotwicz wrote:
> Thanks for the comment! I wasn't aware of legacy packaged apps.
> 
> What do you suggest renaming application_launch_hosted_app.cc to?

how about changing hosted_app to web_app? It is not specifically tied to hosted
apps but covers hosted apps, bookmark apps (which are special types of hosted
apps which will hopefully not be deprecated soon) and if you squint it covers
legacy packaged apps.

BTW if you want the file could just be launch_web_app, but if you like the
common application_launch prefix I'm ok with that too.

[pkotwicz, 2014-10-16 22:04:08]

Patchset #2 (id:600001) has been deleted

[pkotwicz, 2014-10-16 22:07:43]

Ben, can you please take another look?

[benwells, 2014-10-16 22:39:37]

lgtm

[pkotwicz, 2014-10-16 22:45:21]

Oshima, can you please take a look at the Athena related code?

[oshima, 2014-10-17 19:49:02]

lgtm with nits

https://codereview.chromium.org/635233008/diff/620001/chrome/browser/ui/athen...
File chrome/browser/ui/athena/extensions/DEPS (right):

https://codereview.chromium.org/635233008/diff/620001/chrome/browser/ui/athen...
chrome/browser/ui/athena/extensions/DEPS:3: ]
nit: aybe we should use specific_include_rules?

https://codereview.chromium.org/635233008/diff/620001/chrome/browser/ui/athen...
File chrome/browser/ui/athena/extensions/application_launch_web_app_athena.cc
(right):

https://codereview.chromium.org/635233008/diff/620001/chrome/browser/ui/athen...
chrome/browser/ui/athena/extensions/application_launch_web_app_athena.cc:42: // 
              focus explicitly.
just curious. is this comment still valid?

https://codereview.chromium.org/635233008/diff/620001/chrome/browser/ui/athen...
chrome/browser/ui/athena/extensions/application_launch_web_app_athena.cc:49:
const GURL& url) {
nit: indent

[pkotwicz, 2014-10-20 19:35:10]

Patchset #3 (id:640001) has been deleted

[pkotwicz, 2014-10-20 19:37:42]

pkotwicz@chromium.org changed reviewers:
+ sky@chromium.org

[pkotwicz, 2014-10-20 19:37:43]

sky@ for chrome/browser/ui/athena OWNERS

https://codereview.chromium.org/635233008/diff/620001/chrome/browser/ui/athen...
File chrome/browser/ui/athena/extensions/DEPS (right):

https://codereview.chromium.org/635233008/diff/620001/chrome/browser/ui/athen...
chrome/browser/ui/athena/extensions/DEPS:3: ]
I do not think that it is worth it.
https://codereview.chromium.org/634313004/ introduces DEPS here too

https://codereview.chromium.org/635233008/diff/620001/chrome/browser/ui/athen...
File chrome/browser/ui/athena/extensions/application_launch_web_app_athena.cc
(right):

https://codereview.chromium.org/635233008/diff/620001/chrome/browser/ui/athen...
chrome/browser/ui/athena/extensions/application_launch_web_app_athena.cc:42: // 
              focus explicitly.
I am not sure if the comment is still valid. I checked and calling
WebContents::SetInitialFocus() is not a no-op

[sky, 2014-10-20 21:38:51]

LGTM

https://codereview.chromium.org/635233008/diff/660001/chrome/browser/ui/athen...
File chrome/browser/ui/athena/extensions/application_launch_web_app_athena.cc
(right):

https://codereview.chromium.org/635233008/diff/660001/chrome/browser/ui/athen...
chrome/browser/ui/athena/extensions/application_launch_web_app_athena.cc:41: //
TODO(jcampan): http://crbug.com/8123 we should not need to set the initial
Jay isn't going to fix this. Update the todo appropriately.

[pkotwicz, 2014-10-21 15:17:22]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-10-21 15:18:44]

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

[commit-bot: I haz the power, 2014-10-21 16:34:42]

Committed patchset #5 (id:700001)

[commit-bot: I haz the power, 2014-10-21 16:36:00]

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