<webview>: Implement declarativeWebRequest API

<webview>: Implement declarativeWebRequest API

This CL exposes the declarative WebRequest API to <webview>s.

This CL isolates rules registries on a per-<profile, embedder_process_id, webview_instance_id> tuple. For extensions, the pair <embedder_process_id, webview_instance_id> == <0, 0>.

Rules registries are now created on-demand rather than on creation of the RulesRegistryService. This is so that we only create rules registries for webviews if the webview adds rules. This also allows rules to be installed prior to initial navigation of a webview.

Sample code:
var webview = document.querySelector('webview');
var rule = {
  conditions: [
   new chrome.webViewRequest.RequestMatcher({ url: { hostSuffix: 'slashdot.org' } })
  ],
  actions: [
   new chrome.webViewRequest.CancelRequest()
  ]
};
webview.request.onRequest.addRules([rule])

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

[Fady Samuel, 2013-10-23 14:48:58]

This CL is still a work-in-progress. Things seem to work fine now though.
Extensions and webviews are isolated from one another. I'd like some feedback on
what I have so far as I continue to cleanup and polish this experimental
implementation. Thanks!

+rockot@, mpcomplete@: for API feedback. I'm not a fan of attaching the
Actions/Conditions to webview.request. I would prefer exposing a
chrome.webview.request.*. Perhaps a chrome.webViewRequest instead? Thoughts on
the API would be greatly appreciated.

mpcomplete@: I've done a number of changes to RulesRegistryService, and various
RulesRegistry classes to enable per-webview isolation of these rules. I'd like
your thoughts on these changes.

[Fady Samuel, 2013-10-23 14:53:22]

On 2013/10/23 14:48:58, Fady Samuel wrote:
> This CL is still a work-in-progress. Things seem to work fine now though.
> Extensions and webviews are isolated from one another. I'd like some feedback
on
> what I have so far as I continue to cleanup and polish this experimental
> implementation. Thanks!
> 
> +rockot@, mpcomplete@: for API feedback. I'm not a fan of attaching the
> Actions/Conditions to webview.request. I would prefer exposing a
> chrome.webview.request.*. Perhaps a chrome.webViewRequest instead? Thoughts on
> the API would be greatly appreciated.
> 
> mpcomplete@: I've done a number of changes to RulesRegistryService, and
various
> RulesRegistry classes to enable per-webview isolation of these rules. I'd like
> your thoughts on these changes.

Also, an implementation question for mpcomplete@. What exactly is
RulesRegistryWithCache doing? Are these rules cached on disk? I haven't followed
all the code to really understand what it's doing. If they're cached on disk,
why?

[Fady Samuel, 2013-10-23 15:03:16]

Note: This CL is also potentially a prerequisite for other declarative <webview>
APIs such as a declarative ExecuteScript API as it isolates rules registries on
a per-webview basis.

[Fady Samuel, 2013-10-25 16:51:35]

I feel that this CL is now officially ready for review. I've done quite a bit of
testing locally and everything seems to work from adding rules, removing rules,
querying rules, tpo rules isolation, etc. All of it works great. I've added a
couple of <webview> tests. I may add more here, but I'm hoping to get a review
focused on the refactoring and tweaking of the RulesRegistryService, and the
rules registries, and WebRequestEvent and so on. Thanks!

[vabr (Chromium), 2013-10-25 20:40:23]

Hi Fady,
I took a look at the parts touching (Declarative) Web Request (DWR in my
comments below).

If you want to hear useful comments, you might also want to speak to jyasskin
for the changes in rules registry -- he did reviews for my code-changes there
and has always been very helpful and insightful.

Cheers,
Vaclav

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/declarative_api.cc (right):

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_api.cc:36: if
(web_request_event_name.find(kWebView) != std::string::npos)
[This is optional, and probably questionable, feel free to ignore.]
Could |kWebView| appear anywhere, or as a prefix, or even as a dot-separated
word? The more restrictive this test is, the less danger of matching more than
intended. In case this gets changed, it might be a good idea to create a
separate testing helper function, and replace with it also the instances of
.find(kWebView) below.

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_api.cc:85: // If are not
operating on a particular <webview> then the key is (0, 0).
grammar: remove "are"?

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry_service.cc
(right):

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:78: // Only
create a ContentRulesRegistry for regular pages and not webviews.
Is this a permanent design? In that case I find it confusing that
GetContentRulesRegistry has the |webview_key| argument. And even if there are
plans to add the content rules registry per-webview later, for now we should be
consistent with content_rules_registry_ being a single pointer.

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:142:
RegisterDefaultRulesRegistriesIfNotAvailable(webview_key);
If somebody calls this with a webview_key other than <0,0>, no
content_rules_registry will be returned, by the corresponding webrequest
registry will be created. Is this expected? If yes, it should be documented at
the declaration of GetContentRulesRegistry.

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:142:
RegisterDefaultRulesRegistriesIfNotAvailable(webview_key);
Also, is GetContentRulesRegistry called often? I'm concerned that the time
RegisterDefaultRulesRegistriesIfNotAvailable takes to realise that the content
registry has already been created is significantly longer than a simple check
whether content_rules_registry_ is NULL.

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:165:
registries_to_delete.insert(key);
Is there a reason to first build the set of registries to delete, and then
RemoveRulesRegistry them? (Also, delete vs. remove may be confusing.) I think we
should RemoveRulesRegistry(key) directly, to avoid the overhead of creating a
set. Am I missing something?

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry_service.h (right):

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry_service.h:65: // has
been registered.
Is this comment still up-to-date? It looks like it should mention the
registration on demand.

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
File
chrome/browser/extensions/api/declarative/rules_registry_service_unittest.cc
(right):

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry_service_unittest.cc:68:
const RulesRegistry::WebViewKey key(0, 0);
I think we also need to test adding and removing registries for <webviews>. In
particular, we should test that if the webview renderer process terminates, the
corresponding registries are removed.

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry_service_unittest.cc:111:

Could you also please test that GetContentRulesRegistry works? Since it's no
more a simple getter, but involves creation on demand, we should make sure that
that creation on demand works.

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc
(right):

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc:69:
event_name.c_str(), webview_key.first, webview_key.second);
Is this stable across browser restarts?
As far as I understand, webview_key.first is the process ID of the webview
renderer. It looks to me like what is the process ID of the webview of app A,
can be a process ID of the webview of app B after browser restart. If that's
correct, then we are storing wrong information, which can lead to registered
rules not being restored on browser restart.

In case of changing this, please also update
RulesRegistryWithCache::RuleStorageOnUI::GetRulesStoredKey.

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_api.cc:605:
std::pair<void*, RulesRegistryService::WebViewKey> key(profile, webview_key);
Use the already defined typedef RulesRegistryKey instead?

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_api.cc:1864: bool is_guest
= ExtensionRendererState::GetInstance()->
What does "guest" mean in this context?

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_api.cc:1894:
rules_registries_.find(rules_key) != rules_registries_.end()) {
|cross_profile| is not contained in rules_key -- is that what you wanted here
and on line 1896 below?
If so, you can at least cache the test on this line (the exactly same test is
also on line 1887.

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
File chrome/browser/extensions/api/web_request/web_request_api.h (right):

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_api.h:429: // Maps each
profile (and OTRProfile) to its respective rules registry.
Please also update this comment. Now it's matching a profile and a webview key
to a registry.

https://codereview.chromium.org/28273006/diff/510001/chrome/common/extensions...
File chrome/common/extensions/api/webview_request.json (right):

https://codereview.chromium.org/28273006/diff/510001/chrome/common/extensions...
chrome/common/extensions/api/webview_request.json:8: "description": "Use the
<code>chrome.webViewRequest</code> API to intercept, block, or modify requests
in-flight. It is significantly faster than the <a
href='webRequest.html'><code>chrome.webRequest</code> API</a> because you can
register rules that are evaluated in the browser rather than the JavaScript
engine with reduces roundtrip latencies and allows higher efficiency."
Maybe we should be careful with saying "significantly faster", unless we have
benchmarks to prove it. While speed was one of the main motivations for
introducing DWR, and it saves the round-trips, the difference might not be that
significant after all, due to recent improvements in V8 and other parts of
Chrome critical for WebRequest.

There is still a good incentive to use DWR as opposed to the old WR: declarative
programming in general is a good practice to avoid boilerplate in the
extensions, content scripts, and improve security. The security win is prominent
outside of webview, where certain more dangerous parts of DWR (redirecting as
opposed to just blocking) can be separated by higher permissions. This is
probably not so relevant for webview.

[Fady Samuel, 2013-10-25 22:48:08]

Addressed most comments except two. I'm looking into those now. vabr@, if I
don't have them addressed by Monday, let's chat about them. :-)

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/declarative_api.cc (right):

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_api.cc:36: if
(web_request_event_name.find(kWebView) != std::string::npos)
On 2013/10/25 20:40:24, vabr (Chromium) wrote:
> [This is optional, and probably questionable, feel free to ignore.]
> Could |kWebView| appear anywhere, or as a prefix, or even as a dot-separated
> word? The more restrictive this test is, the less danger of matching more than
> intended. In case this gets changed, it might be a good idea to create a
> separate testing helper function, and replace with it also the instances of
> .find(kWebView) below.

It doesn't hurt to be more restrictive. Done.

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/declarative_api.cc:85: // If are not
operating on a particular <webview> then the key is (0, 0).
On 2013/10/25 20:40:24, vabr (Chromium) wrote:
> grammar: remove "are"?

Added "we"

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry_service.cc
(right):

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:78: // Only
create a ContentRulesRegistry for regular pages and not webviews.
On 2013/10/25 20:40:24, vabr (Chromium) wrote:
> Is this a permanent design? In that case I find it confusing that
> GetContentRulesRegistry has the |webview_key| argument. And even if there are
> plans to add the content rules registry per-webview later, for now we should
be
> consistent with content_rules_registry_ being a single pointer.

There are no immediate plans to support content rules in webview. I'm not even
sure what that would mean. <webview> doesn't have page actions. Done.

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:142:
RegisterDefaultRulesRegistriesIfNotAvailable(webview_key);
On 2013/10/25 20:40:24, vabr (Chromium) wrote:
> If somebody calls this with a webview_key other than <0,0>, no
> content_rules_registry will be returned, by the corresponding webrequest
> registry will be created. Is this expected? If yes, it should be documented at
> the declaration of GetContentRulesRegistry.

Done.

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:142:
RegisterDefaultRulesRegistriesIfNotAvailable(webview_key);
On 2013/10/25 20:40:24, vabr (Chromium) wrote:
> Also, is GetContentRulesRegistry called often? I'm concerned that the time
> RegisterDefaultRulesRegistriesIfNotAvailable takes to realise that the content
> registry has already been created is significantly longer than a simple check
> whether content_rules_registry_ is NULL.

Done.

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:165:
registries_to_delete.insert(key);
On 2013/10/25 20:40:24, vabr (Chromium) wrote:
> Is there a reason to first build the set of registries to delete, and then
> RemoveRulesRegistry them? (Also, delete vs. remove may be confusing.) I think
we
> should RemoveRulesRegistry(key) directly, to avoid the overhead of creating a
> set. Am I missing something?

Yes, I am iterating over a map |rules_registries_|. RemoveRulesRegistry modifies
that map. Modifying a map while iterating produces undefined behavior. Thus, we
find out which keys we need to delete, store them in a separate set and then
modify the original map.

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry_service.h (right):

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry_service.h:65: // has
been registered.
On 2013/10/25 20:40:24, vabr (Chromium) wrote:
> Is this comment still up-to-date? It looks like it should mention the
> registration on demand.

Done.

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
File
chrome/browser/extensions/api/declarative/rules_registry_service_unittest.cc
(right):

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry_service_unittest.cc:68:
const RulesRegistry::WebViewKey key(0, 0);
On 2013/10/25 20:40:24, vabr (Chromium) wrote:
> I think we also need to test adding and removing registries for <webviews>. In
> particular, we should test that if the webview renderer process terminates,
the
> corresponding registries are removed.

Agreed, but I'm not sure how to do that in a unit test. I'll chat with you about
this early morning (afternoon for you) on Monday. Thanks.

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc
(right):

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry_with_cache.cc:69:
event_name.c_str(), webview_key.first, webview_key.second);
On 2013/10/25 20:40:24, vabr (Chromium) wrote:
> Is this stable across browser restarts?
> As far as I understand, webview_key.first is the process ID of the webview
> renderer. It looks to me like what is the process ID of the webview of app A,
> can be a process ID of the webview of app B after browser restart. If that's
> correct, then we are storing wrong information, which can lead to registered
> rules not being restored on browser restart.
> 
> In case of changing this, please also update
> RulesRegistryWithCache::RuleStorageOnUI::GetRulesStoredKey.

I was hoping to chat with someone about these changes actually. <webview> rules
should never be stored and preserved across browser restarts. I'm not sure how
to best structure that. I'll dig into this locally a bit more and chat with you
about it on Monday.

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_api.cc:605:
std::pair<void*, RulesRegistryService::WebViewKey> key(profile, webview_key);
On 2013/10/25 20:40:24, vabr (Chromium) wrote:
> Use the already defined typedef RulesRegistryKey instead?

This is a <std::string, WebViewKey> pair, instead of a <Profile, WebViewKey>.

These are different keys. In RulesRegistryService we have different event types
that may or may not correspond to WebRequest API. Here all events are WebRequest
API events.

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_api.cc:1864: bool is_guest
= ExtensionRendererState::GetInstance()->
On 2013/10/25 20:40:24, vabr (Chromium) wrote:
> What does "guest" mean in this context?

The content of a <webview> lives in an different process referred to as the
"guest".

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_api.cc:1894:
rules_registries_.find(rules_key) != rules_registries_.end()) {
On 2013/10/25 20:40:24, vabr (Chromium) wrote:
> |cross_profile| is not contained in rules_key -- is that what you wanted here
> and on line 1896 below?
> If so, you can at least cache the test on this line (the exactly same test is
> also on line 1887.

Good catch! Fixed. Added a cross_profile_rules_key.

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
File chrome/browser/extensions/api/web_request/web_request_api.h (right):

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_api.h:429: // Maps each
profile (and OTRProfile) to its respective rules registry.
On 2013/10/25 20:40:24, vabr (Chromium) wrote:
> Please also update this comment. Now it's matching a profile and a webview key
> to a registry.

Done.

https://codereview.chromium.org/28273006/diff/510001/chrome/common/extensions...
File chrome/common/extensions/api/webview_request.json (right):

https://codereview.chromium.org/28273006/diff/510001/chrome/common/extensions...
chrome/common/extensions/api/webview_request.json:8: "description": "Use the
<code>chrome.webViewRequest</code> API to intercept, block, or modify requests
in-flight. It is significantly faster than the <a
href='webRequest.html'><code>chrome.webRequest</code> API</a> because you can
register rules that are evaluated in the browser rather than the JavaScript
engine with reduces roundtrip latencies and allows higher efficiency."
On 2013/10/25 20:40:24, vabr (Chromium) wrote:
> Maybe we should be careful with saying "significantly faster", unless we have
> benchmarks to prove it. While speed was one of the main motivations for
> introducing DWR, and it saves the round-trips, the difference might not be
that
> significant after all, due to recent improvements in V8 and other parts of
> Chrome critical for WebRequest.
> 
> There is still a good incentive to use DWR as opposed to the old WR:
declarative
> programming in general is a good practice to avoid boilerplate in the
> extensions, content scripts, and improve security. The security win is
prominent
> outside of webview, where certain more dangerous parts of DWR (redirecting as
> opposed to just blocking) can be separated by higher permissions. This is
> probably not so relevant for webview.

The original WebRequest requires more lines of code. The declarative WebRequest
API is more succinct in some cases I feel. The WebRequest API is used
extensively in <webview> and so the declarative WebRequest was requested. I will
soften the language to say it is potentially faster.

[vabr (Chromium), 2013-10-28 14:46:08]

Thanks, Fady. Responses to the easier comments now, and back to the GTalk window
with you it is. :)

Vaclav

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry_service.cc
(right):

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:165:
registries_to_delete.insert(key);
On 2013/10/25 22:48:08, Fady Samuel wrote:
> On 2013/10/25 20:40:24, vabr (Chromium) wrote:
> > Is there a reason to first build the set of registries to delete, and then
> > RemoveRulesRegistry them? (Also, delete vs. remove may be confusing.) I
think
> we
> > should RemoveRulesRegistry(key) directly, to avoid the overhead of creating
a
> > set. Am I missing something?
> 
> Yes, I am iterating over a map |rules_registries_|. RemoveRulesRegistry
modifies
> that map. Modifying a map while iterating produces undefined behavior. Thus,
we
> find out which keys we need to delete, store them in a separate set and then
> modify the original map.

Ah, good point. That's all right then.

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry_service.h (right):

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry_service.h:65: // has
been registered.
On 2013/10/25 22:48:08, Fady Samuel wrote:
> On 2013/10/25 20:40:24, vabr (Chromium) wrote:
> > Is this comment still up-to-date? It looks like it should mention the
> > registration on demand.
> 
> Done.

It's more precise now, but it still does not say that (and under which
conditions) the registry gets created if not existing before returning.

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_api.cc:605:
std::pair<void*, RulesRegistryService::WebViewKey> key(profile, webview_key);
On 2013/10/25 22:48:08, Fady Samuel wrote:
> On 2013/10/25 20:40:24, vabr (Chromium) wrote:
> > Use the already defined typedef RulesRegistryKey instead?
> 
> This is a <std::string, WebViewKey> pair, instead of a <Profile, WebViewKey>.
> 
> These are different keys. In RulesRegistryService we have different event
types
> that may or may not correspond to WebRequest API. Here all events are
WebRequest
> API events.

I think there is a misunderstanding.
In web_request_api.h, line 427 you define
ExtensionWebRequestEventRouter::RulesRegistryKey. This is the key used by
|rules_registries_| (declared just below that typedef).
Now here, in web_request_api.cc, in RegisterRulesRegistry, |key| is obviously a
key to be used for |rules_registries_|, so its type should be
ExtensionWebRequestEventRouter::RulesRegistryKey, right?

https://codereview.chromium.org/28273006/diff/510001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_api.cc:1864: bool is_guest
= ExtensionRendererState::GetInstance()->
On 2013/10/25 22:48:08, Fady Samuel wrote:
> On 2013/10/25 20:40:24, vabr (Chromium) wrote:
> > What does "guest" mean in this context?
> 
> The content of a <webview> lives in an different process referred to as the
> "guest".

OK, that makes sense. Could you please add this explanation as a comment in the
code?

https://codereview.chromium.org/28273006/diff/510001/chrome/common/extensions...
File chrome/common/extensions/api/webview_request.json (right):

https://codereview.chromium.org/28273006/diff/510001/chrome/common/extensions...
chrome/common/extensions/api/webview_request.json:8: "description": "Use the
<code>chrome.webViewRequest</code> API to intercept, block, or modify requests
in-flight. It is significantly faster than the <a
href='webRequest.html'><code>chrome.webRequest</code> API</a> because you can
register rules that are evaluated in the browser rather than the JavaScript
engine with reduces roundtrip latencies and allows higher efficiency."
On 2013/10/25 22:48:08, Fady Samuel wrote:
> On 2013/10/25 20:40:24, vabr (Chromium) wrote:
> > Maybe we should be careful with saying "significantly faster", unless we
have
> > benchmarks to prove it. While speed was one of the main motivations for
> > introducing DWR, and it saves the round-trips, the difference might not be
> that
> > significant after all, due to recent improvements in V8 and other parts of
> > Chrome critical for WebRequest.
> > 
> > There is still a good incentive to use DWR as opposed to the old WR:
> declarative
> > programming in general is a good practice to avoid boilerplate in the
> > extensions, content scripts, and improve security. The security win is
> prominent
> > outside of webview, where certain more dangerous parts of DWR (redirecting
as
> > opposed to just blocking) can be separated by higher permissions. This is
> > probably not so relevant for webview.
> 
> The original WebRequest requires more lines of code. The declarative
WebRequest
> API is more succinct in some cases I feel. The WebRequest API is used
> extensively in <webview> and so the declarative WebRequest was requested. I
will
> soften the language to say it is potentially faster.

Thanks, "potentially" sounds safer.

https://codereview.chromium.org/28273006/diff/930001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry_service.h (right):

https://codereview.chromium.org/28273006/diff/930001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry_service.h:64: //
Returns the RulesRegistry for |event_name| and |webview_key| orr NULL if no
typo: orr

(unless it means "or and only or" as in iff="if and only if" :))

[Fady Samuel, 2013-11-01 23:23:41]

PTAL vabr@ This is diffed against a decoupled RulesCacheDelegate! w00t!

[vabr (Chromium), 2013-11-04 11:12:12]

DWR part LGTM with comments.
Thanks Fady, for your great efforts with this change!

Vaclav

https://codereview.chromium.org/28273006/diff/1140001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/rules_registry_service.cc
(right):

https://codereview.chromium.org/28273006/diff/1140001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:153: if
(!content_rules_registry_) {
We construct the content rules registry in the RulesRegistryService constructor.
So I suggest putting a CHECK(content_rules_registry) here instead of the whole
if branch, and probably downgrade this to the simple accessor
content_rules_registry() as before.

https://codereview.chromium.org/28273006/diff/1140001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/rules_registry_service.h (right):

https://codereview.chromium.org/28273006/diff/1140001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service.h:65: // such
registry has been registered.
This comment is more precise now, but it still does not say that (and under
which conditions) the registry gets created if not existing before returning.

Also "orr" looks like a typo.

https://codereview.chromium.org/28273006/diff/1140001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/28273006/diff/1140001/chrome/browser/extensio...
chrome/browser/extensions/api/web_request/web_request_api.cc:605:
std::pair<void*, RulesRegistryService::WebViewKey> key(profile, webview_key);
I don't think we resolved this thread:

On 2013/10/25 22:48:08, Fady Samuel wrote:
> On 2013/10/25 20:40:24, vabr (Chromium) wrote:
> > Use the already defined typedef RulesRegistryKey instead?
> 
> This is a <std::string, WebViewKey> pair, instead of a <Profile, WebViewKey>.
> 
> These are different keys. In RulesRegistryService we have different event
types
> that may or may not correspond to WebRequest API. Here all events are
WebRequest
> API events.

I think there is a misunderstanding.
In web_request_api.h, line 427 you define
ExtensionWebRequestEventRouter::RulesRegistryKey. This is the key used by
|rules_registries_| (declared just below that typedef).
Now here, in web_request_api.cc, in RegisterRulesRegistry, |key| is obviously a
key to be used for |rules_registries_|, so its type should be
ExtensionWebRequestEventRouter::RulesRegistryKey, right?

[Fady Samuel, 2013-11-04 20:13:52]

+lazyboy@: Please take a look at the webview parts of this change. We can chat
about this works.

https://codereview.chromium.org/28273006/diff/1140001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/rules_registry_service.cc
(right):

https://codereview.chromium.org/28273006/diff/1140001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:153: if
(!content_rules_registry_) {
On 2013/11/04 11:12:13, vabr (Chromium) wrote:
> We construct the content rules registry in the RulesRegistryService
constructor.
> So I suggest putting a CHECK(content_rules_registry) here instead of the whole
> if branch, and probably downgrade this to the simple accessor
> content_rules_registry() as before.

Done.

https://codereview.chromium.org/28273006/diff/1140001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/rules_registry_service.h (right):

https://codereview.chromium.org/28273006/diff/1140001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service.h:65: // such
registry has been registered.
On 2013/11/04 11:12:13, vabr (Chromium) wrote:
> This comment is more precise now, but it still does not say that (and under
> which conditions) the registry gets created if not existing before returning.
> 
> Also "orr" looks like a typo.

Done.

https://codereview.chromium.org/28273006/diff/1140001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/28273006/diff/1140001/chrome/browser/extensio...
chrome/browser/extensions/api/web_request/web_request_api.cc:605:
std::pair<void*, RulesRegistryService::WebViewKey> key(profile, webview_key);
On 2013/11/04 11:12:13, vabr (Chromium) wrote:
> I don't think we resolved this thread:
> 
> On 2013/10/25 22:48:08, Fady Samuel wrote:
> > On 2013/10/25 20:40:24, vabr (Chromium) wrote:
> > > Use the already defined typedef RulesRegistryKey instead?
> > 
> > This is a <std::string, WebViewKey> pair, instead of a <Profile,
WebViewKey>.
> > 
> > These are different keys. In RulesRegistryService we have different event
> types
> > that may or may not correspond to WebRequest API. Here all events are
> WebRequest
> > API events.
> 
> I think there is a misunderstanding.
> In web_request_api.h, line 427 you define
> ExtensionWebRequestEventRouter::RulesRegistryKey. This is the key used by
> |rules_registries_| (declared just below that typedef).
> Now here, in web_request_api.cc, in RegisterRulesRegistry, |key| is obviously
a
> key to be used for |rules_registries_|, so its type should be
> ExtensionWebRequestEventRouter::RulesRegistryKey, right?

Done.

[lazyboy, 2013-11-04 23:16:01]

Looked at the web_view related files, some comments.

https://codereview.chromium.org/28273006/diff/1270002/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/web_view_experimental.js (right):

https://codereview.chromium.org/28273006/diff/1270002/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view_experimental.js:36:
WebViewInternal.prototype.maybeAttachWebRequestEventToWebview_ =
The function name should be changed to maybeAttachWebRequestEventToObject or
something since it is not always attaching the event to a <webview> anymore.

https://codereview.chromium.org/28273006/diff/1270002/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/webview_request_custom_bindings.js
(right):

https://codereview.chromium.org/28273006/diff/1270002/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webview_request_custom_bindings.js:17: //
Returns the schema definition of type |typeId| defined in |namespace|.
There's no |namespace| here, you probably mean
|declarativeWebRequestScheme.types|?

https://codereview.chromium.org/28273006/diff/1270002/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webview_request_custom_bindings.js:27: //
{'instanceType': 'webViewRequest.' + typeId} and validates the
s/webViewRequest/declarativeWebRequest

https://codereview.chromium.org/28273006/diff/1270002/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):

https://codereview.chromium.org/28273006/diff/1270002/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:927: webview.src
= embedder.windowOpenGuestURL;
Why is it required to load a guest.html that calls window.open?

[Fady Samuel, 2013-11-05 19:53:00]

PTAL Istiaque.

https://codereview.chromium.org/28273006/diff/1270002/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/web_view_experimental.js (right):

https://codereview.chromium.org/28273006/diff/1270002/chrome/renderer/resourc...
chrome/renderer/resources/extensions/web_view_experimental.js:36:
WebViewInternal.prototype.maybeAttachWebRequestEventToWebview_ =
On 2013/11/04 23:16:01, lazyboy wrote:
> The function name should be changed to maybeAttachWebRequestEventToObject or
> something since it is not always attaching the event to a <webview> anymore.

Done.

https://codereview.chromium.org/28273006/diff/1270002/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/webview_request_custom_bindings.js
(right):

https://codereview.chromium.org/28273006/diff/1270002/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webview_request_custom_bindings.js:17: //
Returns the schema definition of type |typeId| defined in |namespace|.
On 2013/11/04 23:16:01, lazyboy wrote:
> There's no |namespace| here, you probably mean
> |declarativeWebRequestScheme.types|?

Done.

https://codereview.chromium.org/28273006/diff/1270002/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webview_request_custom_bindings.js:27: //
{'instanceType': 'webViewRequest.' + typeId} and validates the
On 2013/11/04 23:16:01, lazyboy wrote:
> s/webViewRequest/declarativeWebRequest

Done.

https://codereview.chromium.org/28273006/diff/1270002/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):

https://codereview.chromium.org/28273006/diff/1270002/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:927: webview.src
= embedder.windowOpenGuestURL;
On 2013/11/04 23:16:01, lazyboy wrote:
> Why is it required to load a guest.html that calls window.open?

doh! I just realized this test is broken, I should be listening for loadcommit
not loadstop for step 2. Fixed. Also now we navigate to empty guest.

[lazyboy, 2013-11-05 20:43:50]

web_view API related part + webview tests LGTM

https://chromiumcodereview.appspot.com/28273006/diff/1450001/chrome/browser/e...
File chrome/browser/extensions/api/web_request/web_request_api.h (right):

https://chromiumcodereview.appspot.com/28273006/diff/1450001/chrome/browser/e...
chrome/browser/extensions/api/web_request/web_request_api.h:479: // Send updates
to |host| with information about what webRequest-related
Bad merge?

https://chromiumcodereview.appspot.com/28273006/diff/1450001/chrome/test/data...
File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):

https://chromiumcodereview.appspot.com/28273006/diff/1450001/chrome/test/data...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:917:
webview.addEventListener('loadcommit', function(e) {
nit: move this after 'loadabort' event registration so it is easier to follow.

https://chromiumcodereview.appspot.com/28273006/diff/1450001/extensions/commo...
File extensions/common/extension_api.cc (right):

https://chromiumcodereview.appspot.com/28273006/diff/1450001/extensions/commo...
extensions/common/extension_api.cc:265: RegisterSchemaResource("webViewRequest",
nit: sort

[Fady Samuel, 2013-11-06 14:54:14]

Attempting CQ. I'm not sure if the failing bots are the fault of this CL or not
yet.

https://codereview.chromium.org/28273006/diff/1450001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_request/web_request_api.h (right):

https://codereview.chromium.org/28273006/diff/1450001/chrome/browser/extensio...
chrome/browser/extensions/api/web_request/web_request_api.h:479: // Send updates
to |host| with information about what webRequest-related
On 2013/11/05 20:43:51, lazyboy wrote:
> Bad merge?

Done.

https://codereview.chromium.org/28273006/diff/1450001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):

https://codereview.chromium.org/28273006/diff/1450001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:917:
webview.addEventListener('loadcommit', function(e) {
On 2013/11/05 20:43:51, lazyboy wrote:
> nit: move this after 'loadabort' event registration so it is easier to follow.

Done.

https://codereview.chromium.org/28273006/diff/1450001/extensions/common/exten...
File extensions/common/extension_api.cc (right):

https://codereview.chromium.org/28273006/diff/1450001/extensions/common/exten...
extensions/common/extension_api.cc:265: RegisterSchemaResource("webViewRequest",
On 2013/11/05 20:43:51, lazyboy wrote:
> nit: sort

It is sorted assuming case insensitivity.

[commit-bot: I haz the power, 2013-11-06 14:54:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/28273006/1610001

[commit-bot: I haz the power, 2013-11-06 15:12:59]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Jeffrey Yasskin, 2013-11-09 02:47:21]

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/declarative_api.cc (right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/declarative_api.cc:29: using
extensions::RulesRegistryService;
Since the main body of this file is actually inside the extensions namespace,
you don't need this line. If you were to need something from the extensions
namespace inside the anonymous namespace, I'd move the anonymous namespace
inside the 'namespace extensions' block.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/declarative_api.cc:35: return
!event_name.find(kWebView);
This is hard to read with the '!', since it's expression "the found location is
0", not "the found location is false". You could use event_name.compare(0,
strlen(kWebView), kWebView) == 0 to be clearer that this is a comparison and not
a search.

It probably also makes sense to include a few example event names in a comment
for this and the next function, so readers can see what they're intended to do.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/declarative_api.cc:41:
web_request_event_name.replace(0, sizeof(kWebView) - 1, kWebRequest);
strlen(kWebView) will get optimized to a constant and is a bit easier to
understand than sizeof()-1.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/declarative_api.cc:81: if
(extension_->HasAPIPermission(extensions::APIPermission::kWebView)) {
Shouldn't this be IsWebViewEvent()? It's possible that in the future we'd allow
extensions with the declarativeWebRequest permission to also embed <webview>,
and this would prevent that.

Permission checks should go in HasPermission, not RunImpl, but I think this is
actually a call validity check rather than a permission check.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/declarative_api.cc:82: if
(!webview_instance_id)
I'd probably spell this:
  bool have_webview = webview_instance_id != 0;
  if (have_webview != IsWebViewEvent())
    EXTENSION_FUNCTION_ERROR(kUsefulErrorMessage);

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/declarative_api.h (right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/declarative_api.h:32: int
webview_instance_id_;
Please comment what value this has when there's no webview, and if possible what
function's return value it holds when there is a webview.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/rules_registry.cc (right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry.cc:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
We don't generally update copyright years:
http://www.chromium.org/developers/coding-style#TOC-File-headers

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/rules_registry.h (right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry.h:43: typedef
std::pair<int, int> WebViewKey;
I'd rather make this an explicit struct so it can have comments and well-named
fields rather than "first" and "second".

Also, I wonder if WebViewKey is the most general way to say this. However, until
we have multiple use cases, it's probably a bad idea to try to generalize it, so
this name is fine.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry.h:135: // The key that
identifies the webview (or tabs) in which these rules apply.
Document what this returns if the rules apply to the main browser.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/rules_registry_service.cc
(right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:65:
RulesRegistryMap::const_iterator i = rule_registries_.find(key);
If you don't use this iterator again, use ContainsKey instead:
https://code.google.com/p/chromium/codesearch/#chromium/src/base/stl_util.h&l...
If you do use it again, give it a more descriptive name.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:201: void
RulesRegistryService::RemoveRulesRegistry(const RulesRegistryKey& key) {
Unless this is called from multiple places, I'd just inline it.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/rules_registry_service.h (right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service.h:42: typedef
std::pair<std::string, WebViewKey> RulesRegistryKey;
Similarly, this should at the very least have a comment about what the string
means, and probably be a struct with a named field.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service.h:58: void
RegisterDefaultRulesRegistriesIfNotAvailable(
If what aren't available? If this is a "do it the first time" thing, I'd name it
"EnsureDefaultRulesRegistriesRegistered" or just leave the original name and
comment that it's idempotent.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service.h:65: // such
registry has been registered. Default rules registries (such as the
Can this return NULL anymore, since it auto-registers the registries we know
about? If a NULL return is a programming error, DCHECK it.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service.h:72: // rules
registries if they haven't been installed yet.
No it won't. :)

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service.h:91: typedef
std::set<WebViewKey> WebViewKeySet;
FWIW, I'm not a fan of typedefs that save 7 characters since they make readers
look up the definition to be sure it's as expected.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service.h:92: typedef
std::set<RulesRegistryKey> RulesRegistryKeySet;
This doesn't look used.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative/rules_registry_service_unittest.cc
(right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service_unittest.cc:36:
class RulesRegistryServiceTest : public testing::Test {
Please test that the remove-by-process logic does the right thing.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative/rules_registry_with_cache_unittest.cc
(right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_with_cache_unittest.cc:217:
TEST_F(RulesRegistryWithCacheTest, DeclarativeRulesStored) {
We should also have a test that for webview registries, the rules are *not*
stored.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/content_rules_registry_unittest.cc
(right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/content_rules_registry_unittest.cc:40:
const RulesRegistry::WebViewKey key(0, 0);
If this is never used, don't add it.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/web_request/web_request_api.cc:1870:
ExtractRequestInfoDetails(request, &is_main_frame, &frame_id,
OMG, who approved a function with 9 out-parameters in a row? :-P

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/web_request/web_request_api.cc:1875: bool is_guest
= ExtensionRendererState::GetInstance()->
"guest" rather than "webview"?

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:355: Apply(web_contents(),
css_selectors);
Try not to change files you don't have to.

https://codereview.chromium.org/28273006/diff/1940001/chrome/common/extension...
File chrome/common/extensions/api/events.json (right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/common/extension...
chrome/common/extensions/api/events.json:118: "nodoc": true
You should add a description explaining the meaning of this field (and what 0
means) even though it's nodoc. You should probably add a comment saying why it's
nodoc, even though eventName doesn't have one.

https://codereview.chromium.org/28273006/diff/1940001/chrome/common/extension...
File chrome/common/extensions/api/webview_request.json (right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/common/extension...
chrome/common/extensions/api/webview_request.json:8: "description": "Use the
<code>chrome.webViewRequest</code> API to intercept, block, or modify requests
in-flight. It is potentially faster than the <a
href='webRequest.html'><code>chrome.webRequest</code> API</a> because you can
register rules that are evaluated in the browser rather than the JavaScript
engine with reduces roundtrip latencies and allows higher efficiency."
Does webRequest exist on <webview> objects? It seems like the difference between
webRequest and webViewRequest is subtle. Why not have "declarative" on
webViewRequest?

https://codereview.chromium.org/28273006/diff/1940001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/event.js (right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/event.js:191: opt_webViewInstanceId) {
Add a comment for this parameter.

https://codereview.chromium.org/28273006/diff/1940001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/webview_request_custom_bindings.js
(right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webview_request_custom_bindings.js:28: //
{'instanceType': 'declarativeWebRequest.' + typeId} and validates the
Is this line right?

https://codereview.chromium.org/28273006/diff/1940001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webview_request_custom_bindings.js:37:
instance.instanceType = 'declarativeWebRequest.' + typeId;
Why is this declarativeWebRequest instead of webViewRequest? (comment)

https://codereview.chromium.org/28273006/diff/1940001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/web_view/newwindow/embedder.js
(right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/newwindow/embedder.js:354:
newwebview.request.onRequest.addRules([rule]);
You might have a race condition in that new rules aren't guaranteed to have been
seen by the browser process until addRules()' callback is run.

[Fady Samuel, 2013-11-10 03:39:55]

Wow, that's great feedback Jeffrey! Thanks!

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/declarative_api.cc (right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/declarative_api.cc:29: using
extensions::RulesRegistryService;
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> Since the main body of this file is actually inside the extensions namespace,
> you don't need this line. If you were to need something from the extensions
> namespace inside the anonymous namespace, I'd move the anonymous namespace
> inside the 'namespace extensions' block.

Done.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/declarative_api.cc:35: return
!event_name.find(kWebView);
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> This is hard to read with the '!', since it's expression "the found location
is
> 0", not "the found location is false". You could use event_name.compare(0,
> strlen(kWebView), kWebView) == 0 to be clearer that this is a comparison and
not
> a search.
> 
> It probably also makes sense to include a few example event names in a comment
> for this and the next function, so readers can see what they're intended to
do.

Done.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/declarative_api.cc:41:
web_request_event_name.replace(0, sizeof(kWebView) - 1, kWebRequest);
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> strlen(kWebView) will get optimized to a constant and is a bit easier to
> understand than sizeof()-1.

Done.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/declarative_api.cc:81: if
(extension_->HasAPIPermission(extensions::APIPermission::kWebView)) {
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> Shouldn't this be IsWebViewEvent()? It's possible that in the future we'd
allow
> extensions with the declarativeWebRequest permission to also embed <webview>,
> and this would prevent that.
> 
> Permission checks should go in HasPermission, not RunImpl, but I think this is
> actually a call validity check rather than a permission check.

Done.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/declarative_api.cc:82: if
(!webview_instance_id)
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> I'd probably spell this:
>   bool have_webview = webview_instance_id != 0;
>   if (have_webview != IsWebViewEvent())
>     EXTENSION_FUNCTION_ERROR(kUsefulErrorMessage);

Done.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/declarative_api.h (right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/declarative_api.h:32: int
webview_instance_id_;
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> Please comment what value this has when there's no webview, and if possible
what
> function's return value it holds when there is a webview.

Looks like it's stale unused code! Thanks for catching that!

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/rules_registry.cc (right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry.cc:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> We don't generally update copyright years:
> http://www.chromium.org/developers/coding-style#TOC-File-headers

Done.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/rules_registry.h (right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry.h:43: typedef
std::pair<int, int> WebViewKey;
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> I'd rather make this an explicit struct so it can have comments and well-named
> fields rather than "first" and "second".
> 
> Also, I wonder if WebViewKey is the most general way to say this. However,
until
> we have multiple use cases, it's probably a bad idea to try to generalize it,
so
> this name is fine.

I've begun a discussion with some people on this topic. I've made it a struct
for now, but I think we need to revisit extension API isolation in general at
some point in the future. Let's not make major design decisions in a code review
:-)

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry.h:135: // The key that
identifies the webview (or tabs) in which these rules apply.
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> Document what this returns if the rules apply to the main browser.

Done.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/rules_registry_service.cc
(right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:65:
RulesRegistryMap::const_iterator i = rule_registries_.find(key);
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> If you don't use this iterator again, use ContainsKey instead:
>
https://code.google.com/p/chromium/codesearch/#chromium/src/base/stl_util.h&l...
> If you do use it again, give it a more descriptive name.

Done.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:201: void
RulesRegistryService::RemoveRulesRegistry(const RulesRegistryKey& key) {
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> Unless this is called from multiple places, I'd just inline it.

Done.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/rules_registry_service.h (right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service.h:42: typedef
std::pair<std::string, WebViewKey> RulesRegistryKey;
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> Similarly, this should at the very least have a comment about what the string
> means, and probably be a struct with a named field.

Made it a struct. I agree, this is far more readable. Thanks!

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service.h:58: void
RegisterDefaultRulesRegistriesIfNotAvailable(
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> If what aren't available? If this is a "do it the first time" thing, I'd name
it
> "EnsureDefaultRulesRegistriesRegistered" or just leave the original name and
> comment that it's idempotent.

Done.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service.h:65: // such
registry has been registered. Default rules registries (such as the
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> Can this return NULL anymore, since it auto-registers the registries we know
> about? If a NULL return is a programming error, DCHECK it.

Yes, it's possible for this to return NULL if there's some rules registry that
was registered that isn't the default.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service.h:72: // rules
registries if they haven't been installed yet.
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> No it won't. :)

Doh. Stale comment. Removed!

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service.h:91: typedef
std::set<WebViewKey> WebViewKeySet;
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> FWIW, I'm not a fan of typedefs that save 7 characters since they make readers
> look up the definition to be sure it's as expected.

Removed.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service.h:92: typedef
std::set<RulesRegistryKey> RulesRegistryKeySet;
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> This doesn't look used.

Removed.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative/rules_registry_service_unittest.cc
(right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service_unittest.cc:36:
class RulesRegistryServiceTest : public testing::Test {
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> Please test that the remove-by-process logic does the right thing.

Done.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative/rules_registry_with_cache_unittest.cc
(right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_with_cache_unittest.cc:217:
TEST_F(RulesRegistryWithCacheTest, DeclarativeRulesStored) {
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> We should also have a test that for webview registries, the rules are *not*
> stored.

This is enforced by the RulesRegistryService. I added a test in the
RulesRegistryService unit tests.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/content_rules_registry_unittest.cc
(right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/content_rules_registry_unittest.cc:40:
const RulesRegistry::WebViewKey key(0, 0);
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> If this is never used, don't add it.

Done.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/web_request/web_request_api.cc:1870:
ExtractRequestInfoDetails(request, &is_main_frame, &frame_id,
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> OMG, who approved a function with 9 out-parameters in a row? :-P

I didn't write it :P I'm all for cleaning this up in a separate CL though :P

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/web_request/web_request_api.cc:1875: bool is_guest
= ExtensionRendererState::GetInstance()->
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> "guest" rather than "webview"?

Yes, a guest process is the content of a webview. Listeners/rules can be
attached to a webview prior to acquiring a guest (initial navigation or
attachment on window.open).

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:355: Apply(web_contents(),
css_selectors);
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> Try not to change files you don't have to.

Done.

https://codereview.chromium.org/28273006/diff/1940001/chrome/common/extension...
File chrome/common/extensions/api/events.json (right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/common/extension...
chrome/common/extensions/api/events.json:118: "nodoc": true
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> You should add a description explaining the meaning of this field (and what 0
> means) even though it's nodoc. You should probably add a comment saying why
it's
> nodoc, even though eventName doesn't have one.

Done.

https://codereview.chromium.org/28273006/diff/1940001/chrome/common/extension...
File chrome/common/extensions/api/webview_request.json (right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/common/extension...
chrome/common/extensions/api/webview_request.json:8: "description": "Use the
<code>chrome.webViewRequest</code> API to intercept, block, or modify requests
in-flight. It is potentially faster than the <a
href='webRequest.html'><code>chrome.webRequest</code> API</a> because you can
register rules that are evaluated in the browser rather than the JavaScript
engine with reduces roundtrip latencies and allows higher efficiency."
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> Does webRequest exist on <webview> objects? It seems like the difference
between
> webRequest and webViewRequest is subtle. Why not have "declarative" on
> webViewRequest?

This description is slightly incorrect. The condition/action objects exist on
chrome.webViewRequest because they're independent of any particular webview, but
the rules based events are on <webview>.request.*. I'm not sure how to describe
this well here. This API probably needs to go through API review.

https://codereview.chromium.org/28273006/diff/1940001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/event.js (right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/event.js:191: opt_webViewInstanceId) {
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> Add a comment for this parameter.

Done.

https://codereview.chromium.org/28273006/diff/1940001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/webview_request_custom_bindings.js
(right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webview_request_custom_bindings.js:28: //
{'instanceType': 'declarativeWebRequest.' + typeId} and validates the
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> Is this line right?

Yes, we change it to declarativeWebRequest for verification.  This is a bit
weird but this is to try to keep the webview declarative WebRequest API in sync
with the extension declarative WebRequest API. I've begun talking to Ben Kalman
about a better way to do this.

https://codereview.chromium.org/28273006/diff/1940001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webview_request_custom_bindings.js:37:
instance.instanceType = 'declarativeWebRequest.' + typeId;
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> Why is this declarativeWebRequest instead of webViewRequest? (comment)

For validation purposes. That way I don't need to copy and paste the schema. We
simply use the declarative Webrequest schema.

https://codereview.chromium.org/28273006/diff/1940001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/web_view/newwindow/embedder.js
(right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/newwindow/embedder.js:354:
newwebview.request.onRequest.addRules([rule]);
On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> You might have a race condition in that new rules aren't guaranteed to have
been
> seen by the browser process until addRules()' callback is run.

We add the rule prior to initial navigation.  The navigation IPC Is sent on the
same channel after this rule is added. I don't believe there is a race.

[Jeffrey Yasskin, 2013-11-11 05:37:35]

lgtm with some nits and small changes. No need to wait for a re-review after the
changes, since I probably won't be able to get back to you promptly.

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/rules_registry.h (right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry.h:43: typedef
std::pair<int, int> WebViewKey;
On 2013/11/10 03:39:56, Fady Samuel wrote:
> On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> > I'd rather make this an explicit struct so it can have comments and
well-named
> > fields rather than "first" and "second".
> > 
> > Also, I wonder if WebViewKey is the most general way to say this. However,
> until
> > we have multiple use cases, it's probably a bad idea to try to generalize
it,
> so
> > this name is fine.
> 
> I've begun a discussion with some people on this topic. I've made it a struct
> for now, but I think we need to revisit extension API isolation in general at
> some point in the future. Let's not make major design decisions in a code
review
> :-)

Thanks :)

https://codereview.chromium.org/28273006/diff/1940001/chrome/common/extension...
File chrome/common/extensions/api/webview_request.json (right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/common/extension...
chrome/common/extensions/api/webview_request.json:8: "description": "Use the
<code>chrome.webViewRequest</code> API to intercept, block, or modify requests
in-flight. It is potentially faster than the <a
href='webRequest.html'><code>chrome.webRequest</code> API</a> because you can
register rules that are evaluated in the browser rather than the JavaScript
engine with reduces roundtrip latencies and allows higher efficiency."
On 2013/11/10 03:39:56, Fady Samuel wrote:
> On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> > Does webRequest exist on <webview> objects? It seems like the difference
> between
> > webRequest and webViewRequest is subtle. Why not have "declarative" on
> > webViewRequest?
> 
> This description is slightly incorrect. The condition/action objects exist on
> chrome.webViewRequest because they're independent of any particular webview,
but
> the rules based events are on <webview>.request.*. I'm not sure how to
describe
> this well here. This API probably needs to go through API review.

Mhmm. Let's do that before this API launches, but after this CL goes in (unless
this CL would launch the new APIs, which I don't think it will). We can try to
help word the docs better at that point.

https://codereview.chromium.org/28273006/diff/1940001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/webview_request_custom_bindings.js
(right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webview_request_custom_bindings.js:28: //
{'instanceType': 'declarativeWebRequest.' + typeId} and validates the
On 2013/11/10 03:39:56, Fady Samuel wrote:
> On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> > Is this line right?
> 
> Yes, we change it to declarativeWebRequest for verification.  This is a bit
> weird but this is to try to keep the webview declarative WebRequest API in
sync
> with the extension declarative WebRequest API. I've begun talking to Ben
Kalman
> about a better way to do this.

It's probably worth commenting this.

https://codereview.chromium.org/28273006/diff/1940001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/webview_request_custom_bindings.js:37:
instance.instanceType = 'declarativeWebRequest.' + typeId;
On 2013/11/10 03:39:56, Fady Samuel wrote:
> On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> > Why is this declarativeWebRequest instead of webViewRequest? (comment)
> 
> For validation purposes. That way I don't need to copy and paste the schema.
We
> simply use the declarative Webrequest schema.

SG. Please comment that.

https://codereview.chromium.org/28273006/diff/1940001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/web_view/newwindow/embedder.js
(right):

https://codereview.chromium.org/28273006/diff/1940001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/newwindow/embedder.js:354:
newwebview.request.onRequest.addRules([rule]);
On 2013/11/10 03:39:56, Fady Samuel wrote:
> On 2013/11/09 02:47:22, Jeffrey Yasskin wrote:
> > You might have a race condition in that new rules aren't guaranteed to have
> been
> > seen by the browser process until addRules()' callback is run.
> 
> We add the rule prior to initial navigation.  The navigation IPC Is sent on
the
> same channel after this rule is added. I don't believe there is a race.

Mhmm. I'm worried that something will happen to yield processing one of those
IPCs to the message loop or another thread, which would let the other IPC get
ahead of it. We have other races like this in the rest of the system, though,
and that yield probably doesn't happen today, so you don't absolutely need to
fix this one.

https://codereview.chromium.org/28273006/diff/2130001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/declarative_api.cc (right):

https://codereview.chromium.org/28273006/diff/2130001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/declarative_api.cc:40: return
event_name.compare(0, strlen(kWebView), kWebView) == 0;
Would it make sense to include the '.' in kWebView, to protect against some
future API being named webviewSomething.*?

https://codereview.chromium.org/28273006/diff/2130001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/rules_registry.h (right):

https://codereview.chromium.org/28273006/diff/2130001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry.h:52:
(webview_instance_id < other.webview_instance_id));
Nit: indent the nested parenthesis by one more space.

https://codereview.chromium.org/28273006/diff/2130001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative/rules_registry_service_unittest.cc
(right):

https://codereview.chromium.org/28273006/diff/2130001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service_unittest.cc:129:
EXPECT_FALSE(registry_service.GetRulesRegistry(key, "ui").get());
This should probably check that a registry with a different process ID (but
maybe the same webview_instance_id and event_id) *doesn't* get removed.

[Fady Samuel, 2013-11-11 19:52:32]

Addressed nits. CQ'ing now.

https://codereview.chromium.org/28273006/diff/2130001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/declarative_api.cc (right):

https://codereview.chromium.org/28273006/diff/2130001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/declarative_api.cc:40: return
event_name.compare(0, strlen(kWebView), kWebView) == 0;
On 2013/11/11 05:37:36, Jeffrey Yasskin wrote:
> Would it make sense to include the '.' in kWebView, to protect against some
> future API being named webviewSomething.*?

Done.

https://codereview.chromium.org/28273006/diff/2130001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/rules_registry.h (right):

https://codereview.chromium.org/28273006/diff/2130001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry.h:52:
(webview_instance_id < other.webview_instance_id));
On 2013/11/11 05:37:36, Jeffrey Yasskin wrote:
> Nit: indent the nested parenthesis by one more space.

Done.

https://codereview.chromium.org/28273006/diff/2130001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative/rules_registry_service_unittest.cc
(right):

https://codereview.chromium.org/28273006/diff/2130001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service_unittest.cc:129:
EXPECT_FALSE(registry_service.GetRulesRegistry(key, "ui").get());
On 2013/11/11 05:37:36, Jeffrey Yasskin wrote:
> This should probably check that a registry with a different process ID (but
> maybe the same webview_instance_id and event_id) *doesn't* get removed.

Done.

[commit-bot: I haz the power, 2013-11-11 21:35:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/28273006/2730001

[commit-bot: I haz the power, 2013-11-11 22:04:18]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Fady Samuel, 2013-11-11 22:07:06]

+jam@ for OWNER approval of trivial *.grd changes.

[Fady Samuel, 2013-11-11 22:42:38]

-jam@ as he's backed up. +jhawkins@ for chrome OWNER approval. It looks like I'm
blocked on OWNER approval for *.grd files. Thanks!

[James Hawkins, 2013-11-11 23:48:07]

LGTM

[commit-bot: I haz the power, 2013-11-11 23:53:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/28273006/2730001

[commit-bot: I haz the power, 2013-11-12 23:27:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/28273006/2730001

[commit-bot: I haz the power, 2013-11-13 01:40:24]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-13 01:49:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/28273006/2730001

[commit-bot: I haz the power, 2013-11-13 03:28:27]

Change committed as 234724