Add platform_app flag to manifest.

Add platform_app flag to manifest.

Enforce constraint that a platform app must declare a panel as its
launch container.

BUG=none
TEST=unit tests added.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108885

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108936

[miket_OOO, 2011-10-31 22:28:15]

I think that the histogram functionality doesn't need me to do anything explicit
if I'm just adding a new extension type enum. Please confirm.

[Aaron Boodman, 2011-11-01 01:00:05]

This needs to be protected by the experimental permission so that we don't end
up with data in the wild using it yet.

Also, what's a platform app?

[miket_OOO, 2011-11-01 17:00:46]

On 2011/11/01 01:00:05, Aaron Boodman wrote:
> This needs to be protected by the experimental permission so that we don't end
> up with data in the wild using it yet.

Done.

> Also, what's a platform app?

The experimental flag now documents it, briefly. As for the exact capabilities,
there isn't anything to document yet, but we'll add it as we go.

[miket_OOO, 2011-11-03 15:38:22]

Updated reviewer to aa. Aaron, can you approve?

[Mihai Parparita -not on Chrome, 2011-11-03 21:37:12]

Aaron's on vacation today, so you're stuck with me as the reviewer.

http://codereview.chromium.org/8429023/diff/7001/chrome/common/extensions/ext...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/8429023/diff/7001/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:906: if
(!CommandLine::ForCurrentProcess()->HasSwitch(
Maybe I'm missing something, but it seems like the ! is not needed; you only
want to set the platform app bit if the switch is present.

http://codereview.chromium.org/8429023/diff/7001/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:1523: if (launch_container() !=
extension_misc::LAUNCH_PANEL) {
Rather than forcing panel to be specified, it might be cleaner for the default
to be panel, and to not allow the container to be specified in the manifest at
all.

[Aaron Boodman, 2011-11-04 05:45:16]

Muah, you can't escape me.

http://codereview.chromium.org/8429023/diff/7001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/8429023/diff/7001/chrome/browser/about_flags.c...
chrome/browser/about_flags.cc:446: {
Keep in mind that once you put something in about:flags, users will start
clicking it and filing bugs. This happened to Tessa's recent work on lazy
background pages.

It seems like this isn't to the level yet where that would be useful, so I
recommend skipping about:flags for now, and just having the command-line flag.

http://codereview.chromium.org/8429023/diff/7001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_manifests_unittest.cc (right):

http://codereview.chromium.org/8429023/diff/7001/chrome/common/extensions/ext...
chrome/common/extensions/extension_manifests_unittest.cc:243:
LoadAndExpectSuccess("init_valid_platform_app.json");
There's probably going to be more of these app manifest tests, so I recommend
creating a new test fixture for them (TEST_F(ExtensionManifestTest,
PlatformApps))

[miket_OOO, 2011-11-04 22:39:21]

OK, changes made.

[miket_OOO, 2011-11-04 22:39:45]

Please re-review, both of you. I added a new test helper class; let me know what
you think.

http://codereview.chromium.org/8429023/diff/7001/chrome/common/extensions/ext...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/8429023/diff/7001/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:906: if
(!CommandLine::ForCurrentProcess()->HasSwitch(
Overly subtle manual testing that didn't get cleaned up.

http://codereview.chromium.org/8429023/diff/7001/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:1523: if (launch_container() !=
extension_misc::LAUNCH_PANEL) {
On 2011/11/03 21:37:12, Mihai Parparita wrote:
> Rather than forcing panel to be specified, it might be cleaner for the default
> to be panel, and to not allow the container to be specified in the manifest at
> all.

I thought about this, but the panel dimensions are still specified in the
manifest, so it'd be weird to have one without the other.

Eventually I envision many panel attributes to be controllable via JS, and then
it would make sense to remove the dimensions from the manifest. But not today.

[Aaron Boodman, 2011-11-05 16:10:21]

lgtm

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

http://codereview.chromium.org/8429023/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:1523: if (launch_container() !=
extension_misc::LAUNCH_PANEL) {
Nit: why not put this in the LoadLaunchContainer() method?

[commit-bot: I haz the power, 2011-11-07 17:07:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miket@chromium.org/8429023/15001

[miket_OOO, 2011-11-07 17:07:46]

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

http://codereview.chromium.org/8429023/diff/15001/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:1523: if (launch_container() !=
extension_misc::LAUNCH_PANEL) {
On 2011/11/05 16:10:21, Aaron Boodman wrote:
> Nit: why not put this in the LoadLaunchContainer() method?

Doing so would contribute to a problem I see with this class in general: that
certain methods have implicit dependencies on other methods being called before
them. I wanted to keep the new ordering dependency (that LoadLaunchContainer be
called after LoadIsApp) contained within the InitFromValue method.

[commit-bot: I haz the power, 2011-11-07 17:07:47]

Can't apply patch for file chrome/chrome_tests.gypi.
While running patch -p1 --forward --force;
patching file chrome/chrome_tests.gypi
Hunk #1 FAILED at 196.
1 out of 1 hunk FAILED -- saving rejects to file chrome/chrome_tests.gypi.rej

[commit-bot: I haz the power, 2011-11-07 17:31:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miket@chromium.org/8429023/25014

[commit-bot: I haz the power, 2011-11-07 18:39:19]

Change committed as 108885

[miket_OOO, 2011-11-07 21:19:36]

Reverted because of an apparently spurious link failure on Windows. Ran trybots,
now trying again.

[commit-bot: I haz the power, 2011-11-07 21:19:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miket@chromium.org/8429023/25014

[commit-bot: I haz the power, 2011-11-07 23:33:20]

Change committed as 108936