extensions: Register 'app' and 'webstore' bindings only if they are available.

extensions: Register 'app' and 'webstore' bindings only if they are available.

When running from outside chrome (e.g. in athena), 'app' and 'webstore' api
implementations are not available. So check to see if the API is available
before registering the corresponding bindings.

BUG=391478
R=kalman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287935

[not at google - send to devlin, 2014-08-05 22:43:39]

https://codereview.chromium.org/443723003/diff/1/extensions/renderer/dispatch...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/443723003/diff/1/extensions/renderer/dispatch...
extensions/renderer/dispatcher.cc:1006: if (IsApiAvailable(std::string("app"),
context))
it's easier to do

if (context->GetAvailability("app").is_available())
  ...

and below.

https://codereview.chromium.org/443723003/diff/1/extensions/renderer/dispatch...
extensions/renderer/dispatcher.cc:1009: if
(IsApiAvailable(std::string("webstore"), context))
also, no need to explicitly construct a std::string here.

https://codereview.chromium.org/443723003/diff/1/extensions/renderer/dispatch...
extensions/renderer/dispatcher.cc:1010: RegisterBinding("webstore", context);
indentation

[sadrul, 2014-08-05 22:51:02]

https://codereview.chromium.org/443723003/diff/1/extensions/renderer/dispatch...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/443723003/diff/1/extensions/renderer/dispatch...
extensions/renderer/dispatcher.cc:1006: if (IsApiAvailable(std::string("app"),
context))
On 2014/08/05 22:43:39, kalman wrote:
> it's easier to do
> 
> if (context->GetAvailability("app").is_available())
>   ...
> 
> and below.

Nice! Done.

https://codereview.chromium.org/443723003/diff/1/extensions/renderer/dispatch...
extensions/renderer/dispatcher.cc:1009: if
(IsApiAvailable(std::string("webstore"), context))
On 2014/08/05 22:43:39, kalman wrote:
> also, no need to explicitly construct a std::string here.

Acknowledged.

https://codereview.chromium.org/443723003/diff/1/extensions/renderer/dispatch...
extensions/renderer/dispatcher.cc:1010: RegisterBinding("webstore", context);
On 2014/08/05 22:43:39, kalman wrote:
> indentation

Done.

[not at google - send to devlin, 2014-08-05 23:13:04]

https://codereview.chromium.org/443723003/diff/20001/extensions/renderer/disp...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/443723003/diff/20001/extensions/renderer/disp...
extensions/renderer/dispatcher.cc:1009: if
(context->GetAvailability("webstore").is_available())
this is a *very slight* change in the current behaviour. Before, it would always
register. Now, it will take into account the extra restrictions in
_api_features.json.

for "app" the difference is in the "matches". this current behaviour is
basically <all_urls>, it's going to change to specific URLs. we should change it
to <all_urls> there and add a comment.

the same applies to "webstore". change to <all_urls>. however, there is also no
extension_types there... you will need to specify "all" there instead of no
extension type, then update the comment.

[sadrul, 2014-08-06 00:19:24]

https://codereview.chromium.org/443723003/diff/20001/extensions/renderer/disp...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/443723003/diff/20001/extensions/renderer/disp...
extensions/renderer/dispatcher.cc:1009: if
(context->GetAvailability("webstore").is_available())
On 2014/08/05 23:13:04, kalman wrote:
> this is a *very slight* change in the current behaviour. Before, it would
always
> register. Now, it will take into account the extra restrictions in
> _api_features.json.
> 
> for "app" the difference is in the "matches". this current behaviour is
> basically <all_urls>, it's going to change to specific URLs. we should change
it
> to <all_urls> there and add a comment.
> 
> the same applies to "webstore". change to <all_urls>. however, there is also
no
> extension_types there... you will need to specify "all" there instead of no
> extension type, then update the comment.

Done (to clarify: the change for extension_types is just for 'webstore', and not
for 'app', right?).

[not at google - send to devlin, 2014-08-06 00:21:22]

lgtm

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

https://codereview.chromium.org/443723003/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/_api_features.json:767: // displayed on
/extensions/webstore and /apps/webstore.
Also mention: the "contexts" restriction effectively restricts this to hosted
apps and webpages.

[sadrul, 2014-08-06 02:00:13]

I had to make a change in a test failing in the trybots to allow app in
about:flags and chrome://flags Mind taking another look?

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

https://codereview.chromium.org/443723003/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/_api_features.json:767: // displayed on
/extensions/webstore and /apps/webstore.
On 2014/08/06 00:21:22, kalman wrote:
> Also mention: the "contexts" restriction effectively restricts this to hosted
> apps and webpages.

Done.

[not at google - send to devlin, 2014-08-06 17:26:18]

sorry about the back-and-forth :(

https://codereview.chromium.org/443723003/diff/80001/chrome/common/extensions...
File chrome/common/extensions/api/extension_api_unittest.cc (right):

https://codereview.chromium.org/443723003/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/extension_api_unittest.cc:678:
EXPECT_TRUE(MatchesURL(api.get(), "app", "chrome://flags"));
Ah I think you caught an actual bug here. we should be validating that
<all_urls> doesn't include chrome: and about: URLs.

you may need to check PermisisonsData::IsRestrictedUrl in simple_feature.cc:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...

[sadrul, 2014-08-06 18:51:51]

https://codereview.chromium.org/443723003/diff/80001/chrome/common/extensions...
File chrome/common/extensions/api/extension_api_unittest.cc (right):

https://codereview.chromium.org/443723003/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/extension_api_unittest.cc:678:
EXPECT_TRUE(MatchesURL(api.get(), "app", "chrome://flags"));
On 2014/08/06 17:26:18, kalman wrote:
> Ah I think you caught an actual bug here. we should be validating that
> <all_urls> doesn't include chrome: and about: URLs.
> 
> you may need to check PermisisonsData::IsRestrictedUrl in simple_feature.cc:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...

Done.

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

https://codereview.chromium.org/443723003/diff/140001/extensions/common/featu...
extensions/common/features/simple_feature.cc:411: (url.is_valid() &&
The check for url.is_valid() is here because
ExtensionAPI::IsAvailableInUntrustedContext() uses an empty URL, which fails in
PermissionsData::IsRestrictedUrl(), causing failure in some tests.

https://codereview.chromium.org/443723003/diff/140001/extensions/common/permi...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/443723003/diff/140001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:80: if (extension &&
CanExecuteScriptEverywhere(extension))
The null checks here are necessary since some tests send in a null extension to
IsAvailableToContext()

[not at google - send to devlin, 2014-08-06 22:10:28]

lgtm with basically that previous patchset which changed the test.

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

https://codereview.chromium.org/443723003/diff/140001/extensions/common/featu...
extensions/common/features/simple_feature.cc:411: (url.is_valid() &&
On 2014/08/06 18:51:51, sadrul wrote:
> The check for url.is_valid() is here because
> ExtensionAPI::IsAvailableInUntrustedContext() uses an empty URL, which fails
in
> PermissionsData::IsRestrictedUrl(), causing failure in some tests.

damn, it looks like this check is breaking the webui tests. oh well, let's just
forget about it and change the test expectation after all.

https://codereview.chromium.org/443723003/diff/140001/extensions/common/permi...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/443723003/diff/140001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:80: if (extension &&
CanExecuteScriptEverywhere(extension))
On 2014/08/06 18:51:51, sadrul wrote:
> The null checks here are necessary since some tests send in a null extension
to
> IsAvailableToContext()

yeah this change is correct no matter what the context.

[sadrul, 2014-08-06 23:09:09]

Went back to the previous patchset that changed the test expectation to allow
'app' from chrome://flags and about:flags.

[sadrul, 2014-08-07 02:32:08]

Committed patchset #8 manually as 287935 (presubmit successful).