Enable file_handlers and chrome.app.runtime for QuickOffice.

Enable file_handlers and chrome.app.runtime for QuickOffice.

This will enable QuickOffice to use the onLaunched event and
file_handlers to implement launching file Files.App and later from the app launcher.

To make chrome.app.runtime available to whitelisted extensions, the chrome.app namespace needs to also be removed. This has been enabled by making extension feature files understand blacklisting.

The PDF viewer may also use this in future.

TBR=sky@chromium.org
BUG=329587
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268845

[benwells, 2014-04-30 07:16:45]

kalman, please take a first look.

As discussed doing this the other way (move chrome.app.runtime to
chrome.runtime) is more major and will introduce documentation headaches. I'd
prefer to do it like this.

I think the problem we discussed about app / app.runtime fighting for inclusion
is ok as it will always work currently. If the bindings code is changed it might
stop working. If this patch looks ok I'll add a test to make sure QO keeps
access to app.runtime.

[not at google - send to devlin, 2014-04-30 15:52:54]

The problem isn't that the APIs will fight over being defined. The problem is
that "chrome.app" will be defined, then "chrome.app.runtime", and in traversing
the object graph to set up the "runtime" property "chrome.app" will get
realised. Usually it's lazy. In turn, that will cause app_custom_bindings.js to
be evaluated and all its dependencies, *before* the context is fully set up.
This leads to undefined behaviour.

Indeed, you're right that in future iterations of bindings it may even just
break. I had actually planned to add a hard check somewhere in bindings to
prevent the above undefined behaviour from every occurring.

(incidentally, the non-lazy setup of bindings code does lead to a performance
hit on context creation, perf regression tests have shown that.. 10s of
milliseconds only, but enough to trigger a revert. there are no app performance
tests).

https://codereview.chromium.org/265503003/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/265503003/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/_api_features.json:58: "noparent": true,
nit: attempt alphabetic or at least parallel the ordering of the common case.

https://codereview.chromium.org/265503003/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/_api_features.json:62: "whitelist": [
I have an idea: how about implementing a "blacklist" property on features? It
shouldn't be much harder than implementing "whitelist". Then you can use that on
the "app" API and I will feel comfortable about this change.

https://codereview.chromium.org/265503003/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/_api_features.json:63:
"gbkeegbaiigmenfmjfclcdgdpimamgkj",  // QuickOffice
we are supposed to be using ID hashes these days. There is a comment about that
somewhere. it should be in all the feature files.

[not at google - send to devlin, 2014-04-30 15:54:25]

P.S. maybe the REAL answer here is that apps should be launchable in a browser
tab if the user desires :)

[benwells, 2014-05-01 01:17:34]

On 2014/04/30 15:54:25, kalman wrote:
> P.S. maybe the REAL answer here is that apps should be launchable in a browser
> tab if the user desires :)

Not sure if you're entirely serious, but I was thinking something like this
yesterday, but with a subtle difference - that it should be if the *app*
desires.

The practical side of this is that we should make whatever APIs QO needs to do
it stuff available to v2 apps (including opening their tabs), and then make QO
into a packaged app.

To get to that kind of nirvana I think we need more thought. I'd like to lose
such a clear distinction between apps and extensions, and between the different
types of apps. E.g. maybe instead of infering 'platform' app from having a
background thing in your app thing, we don't infer anything like that and let
manifests say what capabilities they want. Then you could have an appstension
that was both an extension and a platform app.

[benwells, 2014-05-01 01:17:46]

https://codereview.chromium.org/265503003/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/265503003/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/_api_features.json:62: "whitelist": [
On 2014/04/30 15:52:55, kalman wrote:
> I have an idea: how about implementing a "blacklist" property on features? It
> shouldn't be much harder than implementing "whitelist". Then you can use that
on
> the "app" API and I will feel comfortable about this change.

OK I'll have a look at that.

https://codereview.chromium.org/265503003/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/_api_features.json:63:
"gbkeegbaiigmenfmjfclcdgdpimamgkj",  // QuickOffice
On 2014/04/30 15:52:55, kalman wrote:
> we are supposed to be using ID hashes these days. There is a comment about
that
> somewhere. it should be in all the feature files.

Ah that dangers of copy paste...

[benwells, 2014-05-01 08:19:21]

https://codereview.chromium.org/265503003/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/265503003/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/_api_features.json:58: "noparent": true,
On 2014/04/30 15:52:55, kalman wrote:
> nit: attempt alphabetic or at least parallel the ordering of the common case.

Done.

https://codereview.chromium.org/265503003/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/_api_features.json:62: "whitelist": [
On 2014/05/01 01:17:47, benwells wrote:
> On 2014/04/30 15:52:55, kalman wrote:
> > I have an idea: how about implementing a "blacklist" property on features?
It
> > shouldn't be much harder than implementing "whitelist". Then you can use
that
> on
> > the "app" API and I will feel comfortable about this change.
> 
> OK I'll have a look at that.

Done.

https://codereview.chromium.org/265503003/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/_api_features.json:63:
"gbkeegbaiigmenfmjfclcdgdpimamgkj",  // QuickOffice
On 2014/05/01 01:17:47, benwells wrote:
> On 2014/04/30 15:52:55, kalman wrote:
> > we are supposed to be using ID hashes these days. There is a comment about
> that
> > somewhere. it should be in all the feature files.
> 
> Ah that dangers of copy paste...

Done.

[not at google - send to devlin, 2014-05-01 20:55:06]

lgtm, and also mention the blacklist thing in the CL description.

https://codereview.chromium.org/265503003/diff/40001/chrome/browser/ui/extens...
File chrome/browser/ui/extensions/application_launch.h (right):

https://codereview.chromium.org/265503003/diff/40001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/application_launch.h:99: bool
LaunchesViaEvent(const extensions::Extension* extension);
comment pls.

"CanLaunchViaEvent" would be a slightly better name, since "LaunchesViaEvent"
implies exclusivity to me.

https://codereview.chromium.org/265503003/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/265503003/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/_api_features.json:44: "blacklist": [
thank you!

https://codereview.chromium.org/265503003/diff/40001/chrome/test/data/extensi...
File
chrome/test/data/extensions/platform_apps/launch_whitelisted_ext_with_file/test.js
(right):

https://codereview.chromium.org/265503003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/launch_whitelisted_ext_with_file/test.js:10:
chrome.test.assertTrue(chrome.app.getIsInstalled === undefined);
chrome.test.assertEq(undefined, chrome.app.getIsInstalled)

https://codereview.chromium.org/265503003/diff/40001/extensions/common/featur...
File extensions/common/features/complex_feature.cc (right):

https://codereview.chromium.org/265503003/diff/40001/extensions/common/featur...
extensions/common/features/complex_feature.cc:12: no_parent_ =
features_[0]->no_parent();
nice catch

https://codereview.chromium.org/265503003/diff/40001/extensions/common/featur...
File extensions/common/features/simple_feature.cc (right):

https://codereview.chromium.org/265503003/diff/40001/extensions/common/featur...
extensions/common/features/simple_feature.cc:296: if (!blacklist_.empty()) {
the !blacklist_.empty() check isn't really necessary.

[benwells, 2014-05-02 00:58:52]

https://codereview.chromium.org/265503003/diff/40001/chrome/browser/ui/extens...
File chrome/browser/ui/extensions/application_launch.h (right):

https://codereview.chromium.org/265503003/diff/40001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/application_launch.h:99: bool
LaunchesViaEvent(const extensions::Extension* extension);
On 2014/05/01 20:55:06, kalman wrote:
> comment pls.
> 
> "CanLaunchViaEvent" would be a slightly better name, since "LaunchesViaEvent"
> implies exclusivity to me.

Done.

https://codereview.chromium.org/265503003/diff/40001/chrome/test/data/extensi...
File
chrome/test/data/extensions/platform_apps/launch_whitelisted_ext_with_file/test.js
(right):

https://codereview.chromium.org/265503003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/launch_whitelisted_ext_with_file/test.js:10:
chrome.test.assertTrue(chrome.app.getIsInstalled === undefined);
On 2014/05/01 20:55:06, kalman wrote:
> chrome.test.assertEq(undefined, chrome.app.getIsInstalled)

Done.

https://codereview.chromium.org/265503003/diff/40001/extensions/common/featur...
File extensions/common/features/simple_feature.cc (right):

https://codereview.chromium.org/265503003/diff/40001/extensions/common/featur...
extensions/common/features/simple_feature.cc:296: if (!blacklist_.empty()) {
On 2014/05/01 20:55:06, kalman wrote:
> the !blacklist_.empty() check isn't really necessary.

Done.

[benwells, 2014-05-02 01:00:26]

+tbr sky for refactoring fallout

[benwells, 2014-05-02 01:09:20]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2014-05-02 01:09:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/benwells@chromium.org/265503003/50001

[commit-bot: I haz the power, 2014-05-02 01:13:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-02 01:13:03]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[benwells, 2014-05-02 01:31:04]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2014-05-02 01:31:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/benwells@chromium.org/265503003/50001

[commit-bot: I haz the power, 2014-05-02 02:02:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-02 02:02:31]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium

[benwells, 2014-05-02 02:48:16]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2014-05-02 02:48:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/benwells@chromium.org/265503003/50001

[commit-bot: I haz the power, 2014-05-02 02:59:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-02 02:59:06]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[benwells, 2014-05-02 06:30:23]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2014-05-02 06:30:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/benwells@chromium.org/265503003/50001

[commit-bot: I haz the power, 2014-05-02 06:53:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-02 06:53:19]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium

[benwells, 2014-05-02 06:59:30]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2014-05-02 06:59:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/benwells@chromium.org/265503003/50001

[commit-bot: I haz the power, 2014-05-02 07:04:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-02 07:04:05]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium

[benwells, 2014-05-05 00:10:59]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2014-05-05 00:11:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/benwells@chromium.org/265503003/50001

[commit-bot: I haz the power, 2014-05-05 00:57:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-05 00:57:01]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium

[benwells, 2014-05-06 12:58:48]

kalman - as you can see from some earlier try jobs the approach we discussed of
moving that bit of code caused lots of failures. Looks like lots of component
extensions rely on getting auto whitelisted.

So ... how do you like the last patch set as a temporary hack? I think we can
use this to gradually whitelist component extensions api by api, which I'm happy
to do for M37.

[not at google - send to devlin, 2014-05-06 23:04:02]

lgtm

https://codereview.chromium.org/265503003/diff/100001/extensions/common/featu...
File extensions/common/features/simple_feature.cc (right):

https://codereview.chromium.org/265503003/diff/100001/extensions/common/featu...
extensions/common/features/simple_feature.cc:300: // See http://crbug.com/370375
for more details.
this is pretty hacky. I've been working on trying to cut out the component auto
whitelist again but it's tricky, so, let's just do this for now. It's not too
invasive.

https://codereview.chromium.org/265503003/diff/100001/extensions/common/featu...
extensions/common/features/simple_feature.cc:321: // TODO(kalman/asargent):
Should this match EXTERNAL_COMPONENT too?
hey could you change this TODO to be instead:

NOTE: Deliberately does not match EXTERNAL_COMPONENT.

[benwells, 2014-05-07 06:33:31]

On 2014/05/06 23:04:02, kalman wrote:
> lgtm
> 
>
https://codereview.chromium.org/265503003/diff/100001/extensions/common/featu...
> File extensions/common/features/simple_feature.cc (right):
> 
>
https://codereview.chromium.org/265503003/diff/100001/extensions/common/featu...
> extensions/common/features/simple_feature.cc:300: // See
http://crbug.com/370375
> for more details.
> this is pretty hacky. I've been working on trying to cut out the component
auto
> whitelist again but it's tricky, so, let's just do this for now. It's not too
> invasive.
> 
>
https://codereview.chromium.org/265503003/diff/100001/extensions/common/featu...
> extensions/common/features/simple_feature.cc:321: // TODO(kalman/asargent):
> Should this match EXTERNAL_COMPONENT too?
> hey could you change this TODO to be instead:
> 
> NOTE: Deliberately does not match EXTERNAL_COMPONENT.

All done. I also made it less hacky as I now understand we want to get rid of
auto granting, not auto whitelisting, so we don't need the second auto grant for
component apps.

BTW I'm quite happy to remove the auto grant - I logged that other bug and will
get to it in m37.

[benwells, 2014-05-07 06:33:39]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2014-05-07 06:34:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/benwells@chromium.org/265503003/120001

[commit-bot: I haz the power, 2014-05-07 12:40:58]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium

[not at google - send to devlin, 2014-05-07 15:35:29]

The CQ bit was unchecked by kalman@chromium.org

[not at google - send to devlin, 2014-05-07 15:35:36]

The CQ bit was checked by kalman@chromium.org

[not at google - send to devlin, 2014-05-07 15:36:08]

oops

lgtm

[commit-bot: I haz the power, 2014-05-07 15:40:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/benwells@chromium.org/265503003/120001

[commit-bot: I haz the power, 2014-05-07 17:47:52]

Change committed as 268845