Switch platform apps from a declarative launch container to handling an onLaunched event.

Switch platform apps from a declarative launch container to handling an onLaunched event.

Platform apps must now have a background page. For the handling of launches,
the background page registers an onLaunched event handler, and creates UI
as appropriate via the chrome.windows.* API.

Updates all tests to use the new window model.

Also makes created shell window respect the bounds that were passed into the
chrome.windows.create() call.

Also fixes bug by resizing windows in PlatformAppBrowserTest.WindowsApi to a
size that matches the Aura grid.

BUG=119410

[Mihai Parparita -not on Chrome, 2012-04-05 02:28:12]

Reviewers:
Matt for extensions infrastructure bits
Mike for platform apps bits
Ben for browser/ui

http://codereview.chromium.org/9969087/diff/3001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/9969087/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.cc:572: // TODO(mihaip): It
might be less janky to pass in the desired bounds
Something like Aaron's Browser::CreateWithParams refactoring from
http://crrev.com/130696 would be ideal (then additional constraints, like
min/max size, which I temporarily ripped out, could be passed in). However, this
CL is big enough as it is, so I'll doing that in a follow up CL.

[Matt Perry, 2012-04-05 19:32:16]

http://codereview.chromium.org/9969087/diff/3001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/9969087/diff/3001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:717:
extensions::RuntimeEventRouter::DispatchOnLaunchedEvent(profile, extension);
It feels weird to have this be a special case of OpenApplication, especially
since call sites seem to know that they're launching a platform app. Would it be
better to have a separate OpenPlatformApplication?

http://codereview.chromium.org/9969087/diff/3001/chrome/common/extensions/doc...
File chrome/common/extensions/docs/examples/extensions/calculator/manifest.json
(right):

http://codereview.chromium.org/9969087/diff/3001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/calculator/manifest.json:7:
"scripts": ["main.js"]
should this be a transient background page?

http://codereview.chromium.org/9969087/diff/3001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/calculator/manifest.json:11:
"16": "icon_16.png",
indent

http://codereview.chromium.org/9969087/diff/8001/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/runtime/runtime_api.h (right):

http://codereview.chromium.org/9969087/diff/8001/chrome/browser/extensions/ap...
chrome/browser/extensions/api/runtime/runtime_api.h:21: static void
DispatchOnLaunchedEvent(Profile* profile,
This actually seems like it belongs in the chrome.app namespace, since it won't
apply to extensions, right?

[Mihai Parparita -not on Chrome, 2012-04-05 21:33:18]

http://codereview.chromium.org/9969087/diff/3001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/9969087/diff/3001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:717:
extensions::RuntimeEventRouter::DispatchOnLaunchedEvent(profile, extension);
On 2012/04/05 19:32:16, Matt Perry wrote:
> It feels weird to have this be a special case of OpenApplication, especially
> since call sites seem to know that they're launching a platform app. Would it
be
> better to have a separate OpenPlatformApplication?

I don't think most callsites are aware that they're launching a platform app
(e.g. the NTP and omnibox launching code just call Browser::OpenApplication, and
aren't aware that they're launching a platform app).

http://codereview.chromium.org/9969087/diff/3001/chrome/common/extensions/doc...
File chrome/common/extensions/docs/examples/extensions/calculator/manifest.json
(right):

http://codereview.chromium.org/9969087/diff/3001/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/extensions/calculator/manifest.json:7:
"scripts": ["main.js"]
On 2012/04/05 19:32:16, Matt Perry wrote:
> should this be a transient background page?

I ended up landing the calculator changes in a separate CL (since the .zip file
couldn't go through the tryservers/CQ), I'll fix both of these things in a
separate CL (https://chromiumcodereview.appspot.com/9949031).

http://codereview.chromium.org/9969087/diff/8001/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/runtime/runtime_api.h (right):

http://codereview.chromium.org/9969087/diff/8001/chrome/browser/extensions/ap...
chrome/browser/extensions/api/runtime/runtime_api.h:21: static void
DispatchOnLaunchedEvent(Profile* profile,
On 2012/04/05 19:32:16, Matt Perry wrote:
> This actually seems like it belongs in the chrome.app namespace, since it
won't
> apply to extensions, right?

Done.

[Matt Perry, 2012-04-05 21:38:00]

lgtm

[Ben Goodger (Google), 2012-04-06 01:13:35]

browser/ui lgtm

[miket_OOO, 2012-04-06 18:05:01]

lgtm

http://codereview.chromium.org/9969087/diff/19001/chrome/browser/extensions/c...
File chrome/browser/extensions/crx_installer_browsertest.cc (right):

http://codereview.chromium.org/9969087/diff/19001/chrome/browser/extensions/c...
chrome/browser/extensions/crx_installer_browsertest.cc:117:
test_data_dir_.AppendASCII("minimal_platform_app.crx"), 1));
Good.

http://codereview.chromium.org/9969087/diff/19001/chrome/browser/ui/gtk/exten...
File chrome/browser/ui/gtk/extensions/shell_window_gtk.cc (right):

http://codereview.chromium.org/9969087/diff/19001/chrome/browser/ui/gtk/exten...
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:22:
gtk_window_set_default_size(window_, 512, 384);
If these values are temporary, fine. But since I see them in more than one
place, would it make sense to give them constant names rather than using
immediate values?

http://codereview.chromium.org/9969087/diff/19001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (left):

http://codereview.chromium.org/9969087/diff/19001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:1480: }
Great!

http://codereview.chromium.org/9969087/diff/19001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/9969087/diff/19001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:1277: }
I wish there were a single point in this file where we could say that if you get
here, the extension is valid. Instead we have a lot of condition checking all
over the place, many of which can determine that the extension is fatally
flawed.

If you have an idea for centralizing these tests, this might be a nice time to
do it. But if not, let's do it some other time.

http://codereview.chromium.org/9969087/diff/19001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/identity/launch.html (left):

http://codereview.chromium.org/9969087/diff/19001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/identity/launch.html:1: <html></html>
Cleanup time! Great!

http://codereview.chromium.org/9969087/diff/19001/chrome/test/data/extensions...
File chrome/test/data/extensions/manifest_tests/init_invalid_platform_app_1.json
(right):

http://codereview.chromium.org/9969087/diff/19001/chrome/test/data/extensions...
chrome/test/data/extensions/manifest_tests/init_invalid_platform_app_1.json:11:
// Launch section is not allowed for platform apps.
Comments in JSON....

http://codereview.chromium.org/9969087/diff/19001/chrome/test/data/extensions...
File chrome/test/data/extensions/platform_apps/storage/test.js (right):

http://codereview.chromium.org/9969087/diff/19001/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/storage/test.js:10: 
I see no changes in this file (maybe this is a Rietveld thing).

http://codereview.chromium.org/9969087/diff/19001/chrome/test/data/extensions...
File chrome/test/data/extensions/platform_apps/windows_api/test.js (right):

http://codereview.chromium.org/9969087/diff/19001/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/windows_api/test.js:15:
function(window) { return window.type == 'shell'}).length);
Not a biggie, but whitespace inside { } isn't balanced.

[Aaron Boodman, 2012-04-06 18:59:01]

http://codereview.chromium.org/9969087/diff/19001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/9969087/diff/19001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:1277: }
On 2012/04/06 18:05:01, miket wrote:
> I wish there were a single point in this file where we could say that if you
get
> here, the extension is valid. Instead we have a lot of condition checking all
> over the place, many of which can determine that the extension is fatally
> flawed.
> 
> If you have an idea for centralizing these tests, this might be a nice time to
> do it. But if not, let's do it some other time.

It should be possible to extend the feature system to support subkeys ... I just
had hoped we wouldn't need it :-(.

http://codereview.chromium.org/9969087/diff/19001/chrome/test/data/extensions...
File chrome/test/data/extensions/manifest_tests/init_invalid_platform_app_1.json
(right):

http://codereview.chromium.org/9969087/diff/19001/chrome/test/data/extensions...
chrome/test/data/extensions/manifest_tests/init_invalid_platform_app_1.json:11:
// Launch section is not allowed for platform apps.
On 2012/04/06 18:05:01, miket wrote:
> Comments in JSON....

They're supported now! Woo!

[miket_OOO, 2012-04-06 19:03:50]

> > Comments in JSON....
> 
> They're supported now! Woo!

Great -- love it! Just wanted to make sure we were doing it intentionally rather
than by a thousand cuts.

[Aaron Boodman, 2012-04-06 19:10:04]

FYI: crbug.com/114233

On Fri, Apr 6, 2012 at 12:03 PM,  <miket@chromium.org> wrote:
>> > Comments in JSON....
>
>
>> They're supported now! Woo!
>
>
> Great -- love it! Just wanted to make sure we were doing it intentionally
> rather
> than by a thousand cuts.
>
> http://codereview.chromium.org/9969087/

[Mihai Parparita -not on Chrome, 2012-04-06 22:02:59]

http://codereview.chromium.org/9969087/diff/19001/chrome/browser/ui/gtk/exten...
File chrome/browser/ui/gtk/extensions/shell_window_gtk.cc (right):

http://codereview.chromium.org/9969087/diff/19001/chrome/browser/ui/gtk/exten...
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:22:
gtk_window_set_default_size(window_, 512, 384);
On 2012/04/06 18:05:01, miket wrote:
> If these values are temporary, fine. But since I see them in more than one
> place, would it make sense to give them constant names rather than using
> immediate values?

Moved to shared constants (defined in shell_window.h)

http://codereview.chromium.org/9969087/diff/19001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/9969087/diff/19001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:1277: }
On 2012/04/06 18:59:01, Aaron Boodman wrote:
> It should be possible to extend the feature system to support subkeys ... I
just
> had hoped we wouldn't need it :-(.

That seems pretty doable if we invert the loop in Manifest::ValidateManifest to
loop over the feature keys instead of over the manifest keys (then there could
be a separate app.launch key). I've added a TODO and filed
http://crbug.com/122459.

http://codereview.chromium.org/9969087/diff/19001/chrome/test/data/extensions...
File chrome/test/data/extensions/platform_apps/storage/test.js (right):

http://codereview.chromium.org/9969087/diff/19001/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/storage/test.js:10: 
On 2012/04/06 18:05:01, miket wrote:
> I see no changes in this file (maybe this is a Rietveld thing).

It got renamed from main.js (the patch file says this, not sure why Rietveld
doesn't).

http://codereview.chromium.org/9969087/diff/19001/chrome/test/data/extensions...
File chrome/test/data/extensions/platform_apps/windows_api/test.js (right):

http://codereview.chromium.org/9969087/diff/19001/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/windows_api/test.js:15:
function(window) { return window.type == 'shell'}).length);
On 2012/04/06 18:05:01, miket wrote:
> Not a biggie, but whitespace inside { } isn't balanced.

Fixed.

[commit-bot: I haz the power, 2012-04-06 22:03:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mihaip@chromium.org/9969087/11069

[commit-bot: I haz the power, 2012-04-06 22:03:24]

Can't process patch for file
chrome/test/data/extensions/platform_apps/context_menu/main.html.
Hunk header is incorrect: 9 vs 10

[Mihai Parparita -not on Chrome, 2012-04-06 22:08:38]

On Fri, Apr 6, 2012 at 3:03 PM, <commit-bot@chromium.org> wrote:

> Can't process patch for file
> chrome/test/data/extensions/**platform_apps/context_menu/**main.html.
> Hunk header is incorrect: 9 vs 10
>

I don't understand this. Will submit tryjobs and land via dcommit.

Mihai

[Mihai Parparita -not on Chrome, 2012-04-09 00:38:23]

I'm unexpectedly going on paternity leave two weeks earlier than expected, so I
won't be able to land this CL. Given that it's a pretty important Apps v2 model
change, I think it should be checked in sooner rather than later.

However, I wasn't able to land it on Friday due to a mysterious assert on the
win_rel tryserver when running PlatformAppBrowserTest.WindowsApi:

[2800:2624:0406/175636:3018015:FATAL:render_widget_host_view_win.cc(668)] Check
failed: parent_hwnd_. 

See
http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/18931/...
for the full log. That has some logging statements that I added, the assert
seems to occur around the setTimeout call in testUpdateWindowWidth.

Jeremy: given that you've looked at this CL in the past, and have the most
Windows/Apps v2 experience, could you take this over?

Thanks,
Mihai

[jeremya, 2012-04-10 18:48:39]

On 2012/04/09 00:38:23, Mihai Parparita wrote:
> I'm unexpectedly going on paternity leave two weeks earlier than expected, so
I
> won't be able to land this CL. Given that it's a pretty important Apps v2
model
> change, I think it should be checked in sooner rather than later.
> 
> However, I wasn't able to land it on Friday due to a mysterious assert on the
> win_rel tryserver when running PlatformAppBrowserTest.WindowsApi:
> 
> [2800:2624:0406/175636:3018015:FATAL:render_widget_host_view_win.cc(668)]
Check
> failed: parent_hwnd_. 
> 
> See
>
http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/18931/...
> for the full log. That has some logging statements that I added, the assert
> seems to occur around the setTimeout call in testUpdateWindowWidth.
> 
> Jeremy: given that you've looked at this CL in the past, and have the most
> Windows/Apps v2 experience, could you take this over?

Yep, I'll try to land this.

[commit-bot: I haz the power, 2012-04-23 05:22:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mihaip@chromium.org/9969087/11069

[commit-bot: I haz the power, 2012-04-23 05:23:04]

Can't process patch for file
chrome/test/data/extensions/platform_apps/context_menu/main.html.
Hunk header is incorrect: 9 vs 10