Allow declarative webrequest and webrequest in extensions.

Several things are done in this CL:

- Update _api_features.json and _permissions.json to allow the following features in extensions:
 - declarativeWebrequest,
 - events
 - webRequest
 - webViewRequest

- Move web_view_request_custom_bindings.js and web_view_request.json to extensions.

- Remove ExtensionsAPIClient::GetRuleRegistry() since RuleRegisteryService has been moved to extension.

- Clean up extensions/extensions.gyp.

TBR=sky@chromium.org
BUG=352293
Committed: https://crrev.com/4499156f5a5784eee9e0dc021f13b4ea0fd7d0d2
Cr-Commit-Position: refs/heads/master@{#297413}

Committed: https://crrev.com/c93416adae7930188e346f03d5764b49934568d3
Cr-Commit-Position: refs/heads/master@{#297532}

[Xi Han, 2014-09-23 21:59:08]

hanxi@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2014-09-24 14:32:53]

This is really exciting! LGTM!

[Xi Han, 2014-09-24 20:59:43]

Patchset #6 (id:90014) has been deleted

[Fady Samuel, 2014-09-25 23:07:09]

Xi, feel free to upload the changes we discussed here. Your work so far still
takes us in the right direction. Let's get that reviewed and landed while you
work on ChromeExtensionsNetworkDelegateImpl

[Xi Han, 2014-09-26 13:18:24]

Fady: I upload the changes that we discussed yesterday, PTAL. Thanks!

[Xi Han, 2014-09-26 13:28:57]

Patchset #10 (id:190001) has been deleted

[Xi Han, 2014-09-26 13:37:38]

Patchset #8 (id:150001) has been deleted

[Fady Samuel, 2014-09-26 14:39:39]

Awesome! Thanks Xi! Still LGTM! :-)

[Xi Han, 2014-09-26 14:50:01]

hanxi@chromium.org changed reviewers:
+ rockot@chromium.org

[Xi Han, 2014-09-26 14:50:01]

+rockot@chromium.org: Please review all changes. Thanks.

Hi Ken, this patch allows declareative-web-sample can be run in app-shell, but
it does nothing since shell_network_delegate doesn't have a similar
extension_delegate implementation like ChromeExtensionsNetworkDelegateImpl. The
remaining work will be in a following cl.

[Ken Rockot(use gerrit already), 2014-09-29 17:22:54]

lgtm with a few comments

https://codereview.chromium.org/596003002/diff/210001/extensions/browser/api/...
File extensions/browser/api/declarative/rules_registry_service.cc (right):

https://codereview.chromium.org/596003002/diff/210001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:98: if
(content_rules_registry.get()) {
I realize you're only preserving previous behavior, but I would feel more
comfortable if this just held onto a copy of the content registry's key
(content_rules_registry->webview_key()) rather than a direct weak pointer.

Then content_rules_registry() can perform the lookup in rule_registries_ and
safely return either a valid raw pointer or NULL

(or nullptr since that's now allowed in Chrome. if you're feeling adventurous):P

https://codereview.chromium.org/596003002/diff/210001/extensions/shell/common...
File extensions/shell/common/shell_extensions_client.cc (right):

https://codereview.chromium.org/596003002/diff/210001/extensions/shell/common...
extensions/shell/common/shell_extensions_client.cc:185:
api->RegisterSchemaResource("declarativeWebRequest",
You could also remove these from both ShellExtensionsClient and
ChromeExtensionsClient, and instead call them in
ExtensionAPI::InitDefaultConfiguration since it doesn't seem like this
implementation will ever change across extensions embedders.

[Xi Han, 2014-09-29 18:16:21]

sky@chromium.org: Please review changes in:

- chrome/common/extensions_api_resources.grd
- chrome/renderer/resources/renderer_resources.grd

Thanks!

https://codereview.chromium.org/596003002/diff/210001/extensions/browser/api/...
File extensions/browser/api/declarative/rules_registry_service.cc (right):

https://codereview.chromium.org/596003002/diff/210001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:98: if
(content_rules_registry.get()) {
Thank you Ken. I added this NULL check since I found in app_shell the function
CreateContentRulesRegistry(...) will return NULL. So using nullptr check might
be better here. The (content_rules_registry->webview_key()) will return an
object and it cannot pass compile:(. 


On 2014/09/29 17:22:53, Ken Rockot wrote:
> I realize you're only preserving previous behavior, but I would feel more
> comfortable if this just held onto a copy of the content registry's key
> (content_rules_registry->webview_key()) rather than a direct weak pointer.
> 
> Then content_rules_registry() can perform the lookup in rule_registries_ and
> safely return either a valid raw pointer or NULL
> 
> (or nullptr since that's now allowed in Chrome. if you're feeling
adventurous):P

https://codereview.chromium.org/596003002/diff/210001/extensions/shell/common...
File extensions/shell/common/shell_extensions_client.cc (right):

https://codereview.chromium.org/596003002/diff/210001/extensions/shell/common...
extensions/shell/common/shell_extensions_client.cc:185:
api->RegisterSchemaResource("declarativeWebRequest",
Great suggestion! I was considering how to remove this code duplication but
didn't find a nice way like this.


On 2014/09/29 17:22:53, Ken Rockot wrote:
> You could also remove these from both ShellExtensionsClient and
> ChromeExtensionsClient, and instead call them in
> ExtensionAPI::InitDefaultConfiguration since it doesn't seem like this
> implementation will ever change across extensions embedders.

[Ken Rockot(use gerrit already), 2014-09-29 19:31:42]

https://codereview.chromium.org/596003002/diff/210001/extensions/browser/api/...
File extensions/browser/api/declarative/rules_registry_service.cc (right):

https://codereview.chromium.org/596003002/diff/210001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:98: if
(content_rules_registry.get()) {
On 2014/09/29 18:16:21, hanxi wrote:
> Thank you Ken. I added this NULL check since I found in app_shell the function
> CreateContentRulesRegistry(...) will return NULL. So using nullptr check might
> be better here. The (content_rules_registry->webview_key()) will return an
> object and it cannot pass compile:(. 
> 
> 
> On 2014/09/29 17:22:53, Ken Rockot wrote:
> > I realize you're only preserving previous behavior, but I would feel more
> > comfortable if this just held onto a copy of the content registry's key
> > (content_rules_registry->webview_key()) rather than a direct weak pointer.
> > 
> > Then content_rules_registry() can perform the lookup in rule_registries_ and
> > safely return either a valid raw pointer or NULL
> > 
> > (or nullptr since that's now allowed in Chrome. if you're feeling
> adventurous):P
> 

This isn't what I meant; the condition was fine before. What I meant was that
content_rules_registry_ should not exist as a raw pointer, but rather maybe it
should be a string content_rules_registry_key_ (which may be empty); so then
this code would change to:

if (content_rules_registry.get()) {
  RegisterRulesRegistry(content_rules_registry)
  content_rules_registry_key_ = content_rules_registry->webview_key();
}


And then you replace the content_rules_registry() accessor with a
GetContentRulesRegistry() method, which has this implementation:

RulesRegistryService::GetContentRulesRegistry() {
  RulesRegistryMap::const_iterator i =
rule_registry_.find(content_rules_registry_key_);
  if (i != rule_registry_.end()) {
    return i->second.get();
  }
  return nullptr;
}


Does that make more sense? This is safer because it avoids holding onto a raw
pointer to the content registry.

[Fady Samuel, 2014-09-29 21:26:32]

https://codereview.chromium.org/596003002/diff/230001/extensions/browser/api/...
File extensions/browser/api/declarative/declarative_api.cc (right):

https://codereview.chromium.org/596003002/diff/230001/extensions/browser/api/...
extensions/browser/api/declarative/declarative_api.cc:158: GetRulesRegistry(key,
event_name);
Indentation is off.

[Fady Samuel, 2014-09-29 22:07:11]

Xi, could you please move the event listener removal code out of chrome and into
extensions as well?

https://codereview.chromium.org/596003002/diff/210001/extensions/browser/api/...
File extensions/browser/api/declarative/rules_registry_service.cc (right):

https://codereview.chromium.org/596003002/diff/210001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:98: if
(content_rules_registry.get()) {
On 2014/09/29 19:31:42, Ken Rockot wrote:
> On 2014/09/29 18:16:21, hanxi wrote:
> > Thank you Ken. I added this NULL check since I found in app_shell the
function
> > CreateContentRulesRegistry(...) will return NULL. So using nullptr check
might
> > be better here. The (content_rules_registry->webview_key()) will return an
> > object and it cannot pass compile:(. 
> > 
> > 
> > On 2014/09/29 17:22:53, Ken Rockot wrote:
> > > I realize you're only preserving previous behavior, but I would feel more
> > > comfortable if this just held onto a copy of the content registry's key
> > > (content_rules_registry->webview_key()) rather than a direct weak pointer.
> > > 
> > > Then content_rules_registry() can perform the lookup in rule_registries_
and
> > > safely return either a valid raw pointer or NULL
> > > 
> > > (or nullptr since that's now allowed in Chrome. if you're feeling
> > adventurous):P
> > 
> 
> This isn't what I meant; the condition was fine before. What I meant was that
> content_rules_registry_ should not exist as a raw pointer, but rather maybe it
> should be a string content_rules_registry_key_ (which may be empty); so then
> this code would change to:
> 
> if (content_rules_registry.get()) {
>   RegisterRulesRegistry(content_rules_registry)
>   content_rules_registry_key_ = content_rules_registry->webview_key();
> }
> 
> 
> And then you replace the content_rules_registry() accessor with a
> GetContentRulesRegistry() method, which has this implementation:
> 
> RulesRegistryService::GetContentRulesRegistry() {
>   RulesRegistryMap::const_iterator i =
> rule_registry_.find(content_rules_registry_key_);
>   if (i != rule_registry_.end()) {
>     return i->second.get();
>   }
>   return nullptr;
> }
> 
> 
> Does that make more sense? This is safer because it avoids holding onto a raw
> pointer to the content registry.

Hi Ken,

In AppShell, this code path will never execute. WebViewKey will always be
non-zero. 

EnsureDefaultRulesRegistriesRegistered(WebViewKey(0, 0)); on line 54 will create
a bunch of RulesRegistries that we will never use in AppShell because only the
Chrome Extension Declarative WebRequest API has a WebViewKey of 0,0.

I would like to suggest that we leave the code as is in this CL, and try to find
ways to clean up the code in a subsequent CL. WDYT? Thanks.

[Ken Rockot(use gerrit already), 2014-09-29 22:18:30]

OK. SGTM.

On Mon, Sep 29, 2014 at 3:07 PM, <fsamuel@chromium.org> wrote:

> Xi, could you please move the event listener removal code out of chrome
> and into
> extensions as well?
>
>
> https://codereview.chromium.org/596003002/diff/210001/
> extensions/browser/api/declarative/rules_registry_service.cc
> File extensions/browser/api/declarative/rules_registry_service.cc
> (right):
>
> https://codereview.chromium.org/596003002/diff/210001/
> extensions/browser/api/declarative/rules_registry_service.cc#newcode98
> extensions/browser/api/declarative/rules_registry_service.cc:98: if
> (content_rules_registry.get()) {
> On 2014/09/29 19:31:42, Ken Rockot wrote:
>
>> On 2014/09/29 18:16:21, hanxi wrote:
>> > Thank you Ken. I added this NULL check since I found in app_shell
>>
> the function
>
>> > CreateContentRulesRegistry(...) will return NULL. So using nullptr
>>
> check might
>
>> > be better here. The (content_rules_registry->webview_key()) will
>>
> return an
>
>> > object and it cannot pass compile:(.
>> >
>> >
>> > On 2014/09/29 17:22:53, Ken Rockot wrote:
>> > > I realize you're only preserving previous behavior, but I would
>>
> feel more
>
>> > > comfortable if this just held onto a copy of the content
>>
> registry's key
>
>> > > (content_rules_registry->webview_key()) rather than a direct weak
>>
> pointer.
>
>> > >
>> > > Then content_rules_registry() can perform the lookup in
>>
> rule_registries_ and
>
>> > > safely return either a valid raw pointer or NULL
>> > >
>> > > (or nullptr since that's now allowed in Chrome. if you're feeling
>> > adventurous):P
>> >
>>
>
>  This isn't what I meant; the condition was fine before. What I meant
>>
> was that
>
>> content_rules_registry_ should not exist as a raw pointer, but rather
>>
> maybe it
>
>> should be a string content_rules_registry_key_ (which may be empty);
>>
> so then
>
>> this code would change to:
>>
>
>  if (content_rules_registry.get()) {
>>    RegisterRulesRegistry(content_rules_registry)
>>    content_rules_registry_key_ = content_rules_registry->webview_key();
>> }
>>
>
>
>  And then you replace the content_rules_registry() accessor with a
>> GetContentRulesRegistry() method, which has this implementation:
>>
>
>  RulesRegistryService::GetContentRulesRegistry() {
>>    RulesRegistryMap::const_iterator i =
>> rule_registry_.find(content_rules_registry_key_);
>>    if (i != rule_registry_.end()) {
>>      return i->second.get();
>>    }
>>    return nullptr;
>> }
>>
>
>
>  Does that make more sense? This is safer because it avoids holding
>>
> onto a raw
>
>> pointer to the content registry.
>>
>
> Hi Ken,
>
> In AppShell, this code path will never execute. WebViewKey will always
> be non-zero.
>
> EnsureDefaultRulesRegistriesRegistered(WebViewKey(0, 0)); on line 54
> will create a bunch of RulesRegistries that we will never use in
> AppShell because only the Chrome Extension Declarative WebRequest API
> has a WebViewKey of 0,0.
>
> I would like to suggest that we leave the code as is in this CL, and try
> to find ways to clean up the code in a subsequent CL. WDYT? Thanks.
>
> https://codereview.chromium.org/596003002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Xi Han, 2014-09-29 22:22:22]

Fady: PTAL, thanks!

[Fady Samuel, 2014-09-29 22:24:39]

lgtm

[Fady Samuel, 2014-09-29 22:24:39]

lgtm

lgtm

[Xi Han, 2014-09-30 13:13:41]

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

[Xi Han, 2014-09-30 13:15:21]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-09-30 13:15:46]

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

[commit-bot: I haz the power, 2014-09-30 13:20:16]

Committed patchset #11 (id:250001) as 58af8acc86fc2999212b9e256fc0aa7856dd7dcc

[commit-bot: I haz the power, 2014-09-30 13:20:53]

Patchset 11 (id:??) landed as
https://crrev.com/4499156f5a5784eee9e0dc021f13b4ea0fd7d0d2
Cr-Commit-Position: refs/heads/master@{#297413}

[dcheng, 2014-09-30 19:02:41]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org, hanxi@chromium.org

[dcheng, 2014-09-30 19:02:42]

+hanxi@chromium.org as well (sorry for leaving you off the initial email)

[Xi Han, 2014-09-30 20:37:06]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-09-30 20:38:38]

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

[commit-bot: I haz the power, 2014-09-30 22:44:30]

Committed patchset #12 (id:270001) as 8ee32d30cfa3636fbce5676c12d3f7ee91290d0d

[commit-bot: I haz the power, 2014-09-30 22:45:17]

Patchset 12 (id:??) landed as
https://crrev.com/c93416adae7930188e346f03d5764b49934568d3
Cr-Commit-Position: refs/heads/master@{#297532}