Remove WebViewKey in rules registry.

Remove WebViewKey in rules registry. This is the first step of "Implement declarative content script injection in <webview>".

BUG=437566
Committed: https://crrev.com/a615e9c5e04dc5ffe13512d448e4a94b7107a102
Cr-Commit-Position: refs/heads/master@{#306287}

[Xi Han, 2014-11-26 20:38:16]

Patchset #1 (id:1) has been deleted

[Xi Han, 2014-11-26 21:38:11]

Patchset #3 (id:60001) has been deleted

[Xi Han, 2014-11-26 21:38:20]

Patchset #3 (id:80001) has been deleted

[Xi Han, 2014-11-26 22:24:52]

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

[Xi Han, 2014-11-26 22:24:53]

Hi Fady: could you please help me to look at this CL? I didn't see any dangerous
change but even chrome crashes. Thanks a lot!

[Xi Han, 2014-11-26 22:28:01]

Patchset #2 (id:40001) has been deleted

[Fady Samuel, 2014-11-26 23:39:41]

Some initial comments.

https://codereview.chromium.org/764643002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/declarative_apitest.cc (right):

https://codereview.chromium.org/764643002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:117:
RulesRegistryService::kDefultRulesRegistryID, false,
DefaultRulesRegistryID

https://codereview.chromium.org/764643002/diff/100001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc
(right):

https://codereview.chromium.org/764643002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc:57:
RulesRegistryService::kDefultRulesRegistryID),
kDefaultRulesRegistryID

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
File extensions/browser/api/declarative/rules_registry.h (right):

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry.h:137: int rules_registry_id()
const { return rules_registry_id_; }
Just call this ID. Also, don't mention webview or tabs here.

A unique identifier for this RulesRegistry object.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry.h:221: int rules_registry_id_;
int id_

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

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:24: #include
"extensions/browser/guest_view/web_view/web_view_guest.h"
Avoid this include.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:58: int
RulesRegistryService::GetNextRulesRegistryId() {
GetNextRulesRegistryID

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:64: bool
is_web_view) {
bool cache_rules

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:76: if
(!is_web_view) {
if (!cache_rules)

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:93: if
(!is_web_view) {
!cache_rules

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:118:
browser_context_, 0 /* not for web_view*/,
Use kDefaultRulesRegistryID

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:140:
RulesRegistryKey key(event_name, rule_registry->rules_registry_id());
This seems really redundant. How about just calling it rule_registry->id()

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:147: bool
is_web_view,
bool cache_rules

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:158: void
RulesRegistryService::RemoveWebViewRulesRegistries(int process_id) {
Can we move this to WebViewGuest?

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
File extensions/browser/api/declarative/rules_registry_service.h (right):

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.h:41: static const int
kDefultRulesRegistryID = 0;
typo: kDefaultRulesRegistryID

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.h:47: :
event_name(event_name), rules_registry_id(rules_registry_id) {}
Put this on the next line.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.h:69: int
GetNextRulesRegistryId();
GetNextRulesRegistryID

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.h:73: bool
is_web_view);
Avoid this is_web_view. Maybe instead:

GetNextRulesRegistryID(bool cache_rules);

Then store a map here from rulesregistry ID to bool.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
File extensions/browser/api/declarative/rules_registry_unittest.cc (right):

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_unittest.cc:18: const int key
= extensions::RulesRegistryService::kDefultRulesRegistryID;
kDefaultRulesRegistryID

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
File extensions/browser/api/declarative_content/content_rules_registry.h
(right):

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative_content/content_rules_registry.h:32: int
rule_registry_id)
rules_registry_id

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative_content/content_rules_registry.h:37:
rule_registry_id) {}
rules_registry_id

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
File extensions/browser/api/declarative_webrequest/webrequest_rules_registry.h
(right):

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative_webrequest/webrequest_rules_registry.h:83:
int rule_registry_id);
rules_registry_id

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:1970: is_web_view_guest ?
web_view_info.rules_registry_id : 0;
Use the default value here.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:196:
RulesRegistryService* rules_registry_service,
Pass in a BrowserContext instead.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:197: int*
rules_registry_id) {
return this value instead of making it an output parameter.

[Xi Han, 2014-11-27 23:25:10]

Patchset #3 (id:120001) has been deleted

[Xi Han, 2014-11-27 23:25:26]

PTAL, thanks.

https://codereview.chromium.org/764643002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/declarative_apitest.cc (right):

https://codereview.chromium.org/764643002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/declarative_apitest.cc:117:
RulesRegistryService::kDefultRulesRegistryID, false,
On 2014/11/26 23:39:40, Fady Samuel wrote:
> DefaultRulesRegistryID

Done.

https://codereview.chromium.org/764643002/diff/100001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc
(right):

https://codereview.chromium.org/764643002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc:57:
RulesRegistryService::kDefultRulesRegistryID),
On 2014/11/26 23:39:40, Fady Samuel wrote:
> kDefaultRulesRegistryID

Sorry for the typo, updated.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
File extensions/browser/api/declarative/rules_registry.h (right):

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry.h:137: int rules_registry_id()
const { return rules_registry_id_; }
On 2014/11/26 23:39:40, Fady Samuel wrote:
> Just call this ID. Also, don't mention webview or tabs here.
> 
> A unique identifier for this RulesRegistry object.
> 

Rename to id().

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry.h:221: int rules_registry_id_;
On 2014/11/26 23:39:40, Fady Samuel wrote:
> int id_

Done.

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

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:24: #include
"extensions/browser/guest_view/web_view/web_view_guest.h"
On 2014/11/26 23:39:40, Fady Samuel wrote:
> Avoid this include.

Done.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:58: int
RulesRegistryService::GetNextRulesRegistryId() {
On 2014/11/26 23:39:40, Fady Samuel wrote:
> GetNextRulesRegistryID

Done.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:64: bool
is_web_view) {
On 2014/11/26 23:39:40, Fady Samuel wrote:
> bool cache_rules

Done.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:76: if
(!is_web_view) {
On 2014/11/26 23:39:40, Fady Samuel wrote:
> if (!cache_rules)

Done.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:93: if
(!is_web_view) {
On 2014/11/26 23:39:41, Fady Samuel wrote:
> !cache_rules

Done.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:118:
browser_context_, 0 /* not for web_view*/,
On 2014/11/26 23:39:41, Fady Samuel wrote:
> Use kDefaultRulesRegistryID

Done.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:140:
RulesRegistryKey key(event_name, rule_registry->rules_registry_id());
On 2014/11/26 23:39:40, Fady Samuel wrote:
> This seems really redundant. How about just calling it rule_registry->id()

Done.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:147: bool
is_web_view,
On 2014/11/26 23:39:41, Fady Samuel wrote:
> bool cache_rules

Done.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:158: void
RulesRegistryService::RemoveWebViewRulesRegistries(int process_id) {
On 2014/11/26 23:39:41, Fady Samuel wrote:
> Can we move this to WebViewGuest?

Moved.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
File extensions/browser/api/declarative/rules_registry_service.h (right):

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.h:41: static const int
kDefultRulesRegistryID = 0;
On 2014/11/26 23:39:41, Fady Samuel wrote:
> typo: kDefaultRulesRegistryID

Renamed.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.h:47: :
event_name(event_name), rules_registry_id(rules_registry_id) {}
On 2014/11/26 23:39:41, Fady Samuel wrote:
> Put this on the next line.

Done.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.h:69: int
GetNextRulesRegistryId();
On 2014/11/26 23:39:41, Fady Samuel wrote:
> GetNextRulesRegistryID

Done.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.h:73: bool
is_web_view);
On 2014/11/26 23:39:41, Fady Samuel wrote:
> Avoid this is_web_view. Maybe instead:
> 
> GetNextRulesRegistryID(bool cache_rules);
> 
> Then store a map here from rulesregistry ID to bool.

Yes, this name is more clear.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
File extensions/browser/api/declarative_content/content_rules_registry.h
(right):

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative_content/content_rules_registry.h:32: int
rule_registry_id)
On 2014/11/26 23:39:41, Fady Samuel wrote:
> rules_registry_id

Done.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative_content/content_rules_registry.h:37:
rule_registry_id) {}
On 2014/11/26 23:39:41, Fady Samuel wrote:
> rules_registry_id

Done.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
File extensions/browser/api/declarative_webrequest/webrequest_rules_registry.h
(right):

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/declarative_webrequest/webrequest_rules_registry.h:83:
int rule_registry_id);
On 2014/11/26 23:39:41, Fady Samuel wrote:
> rules_registry_id

Done.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:1970: is_web_view_guest ?
web_view_info.rules_registry_id : 0;
On 2014/11/26 23:39:41, Fady Samuel wrote:
> Use the default value here. 

Done.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:196:
RulesRegistryService* rules_registry_service,
On 2014/11/26 23:39:41, Fady Samuel wrote:
> Pass in a BrowserContext instead.

Done.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:197: int*
rules_registry_id) {
On 2014/11/26 23:39:41, Fady Samuel wrote:
> return this value instead of making it an output parameter.

Done.

[Fady Samuel, 2014-11-28 11:34:25]

Getting closer. Please add a web_view_browser_test or two to replace the tests
you deleted.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:193: void
WebViewGuest::GenerateRulesRegistryID(
GetOrGenerateRulesRegistryID

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

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:58: bool
cache_rules) {
You know, now that I think about it, we can probably get rid of cache_rules too.
Then simply add a comment about how:

// Only cache rules for regular pages.

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:70: if
(!cache_rules) {
The logic here is reversed. if (cache_rules) then do whatever.

I would suggest:

if (rules_registry_id == kDefaultRulesRegistryID) {
  ....
  // create a RulesCacheDelegate here.

}

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:86: // Only create
a ContentRulesRegistry for regular pages and not webviews.
Get rid of the reference to <webview>.

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:87: if
(!cache_rules) {
This is inverted. As above, simply do the following:

if (rules_registry_id == kDefaultRulesRegistryID)
...

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:142: bool
cache_rules,
We can probably get rid of this.

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/api/...
File extensions/browser/api/declarative/rules_registry_service.h (right):

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.h:83: void
RemoveRulesRegistryID(int rules_registry_id);
RemoveRulesRegistry

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:202:
WebViewKeyToIDMap::iterator it = web_view_key_to_id_map.Get().find(key);
nit: Use auto.

auto it = web_view_key_to_id_map.Get().find(key);

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:203: if (it !=
web_view_key_to_id_map.Get().end()) {
No need for a new block.

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:384:
id_to_web_view_key_map.Get().find(rules_registry_id_);
Hmm, I don't think we need IDToWebViewKeyMap? A WebViewGuest should already know
about its embedder_process_id and view_instance_id()

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:711:
rules_registry_id_(0),
Use the kDefaultRulesRegistryID constant here?

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:934: rules_registry_id_
= web_view_key_to_id_map.Get().find(key)->second;
Hmm, what if add rules after attachment?

I feel like the right approach is to always allocate a rules registry ID at this
point:

rules_registry_id_ = GetOrGenerateRulesRegistryID(embedder_process_id,
view_instance_id());

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.h (right):

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.h:64: int
webview_instance_id,
nit: web_view_instance_id to be more consistent with spacing in this file

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.h:326: // Store the ID of
this webview in the RulesRegistryService.
nit:

// Identifies the set of rules registries belonging to this guest.

[Fady Samuel, 2014-11-28 11:35:51]

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

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/api/...
extensions/browser/api/declarative/declarative_api.cc:155: embedder_process_id,
webview_instance_id, browser_context());
make it web_view_instance_id to be consistent with the new naming.

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/api/...
extensions/browser/api/declarative/declarative_api.cc:162:
->GetRulesRegistry(rules_registry_id, has_webview, event_name);
I don't think we need to pass in has_webview anymore.

[Fady Samuel, 2014-11-28 11:41:31]

Some more thoughts.

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

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:40: :
current_rules_registry_id_(0),
Use kDefaultRulesRegistryID

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:158:
rule_registries_.erase(it);
Modifying a container while iterating over it can lead to badness. I would
restore "registries_to_delete".

[Xi Han, 2014-11-28 16:12:57]

Fady: I will add browser tests ASAP. Thanks.

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/764643002/diff/100001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:193: void
WebViewGuest::GenerateRulesRegistryID(
On 2014/11/28 11:34:24, Fady Samuel wrote:
> GetOrGenerateRulesRegistryID

Done.

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

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/api/...
extensions/browser/api/declarative/declarative_api.cc:155: embedder_process_id,
webview_instance_id, browser_context());
On 2014/11/28 11:35:51, Fady Samuel wrote:
> make it web_view_instance_id to be consistent with the new naming.

Done.

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/api/...
extensions/browser/api/declarative/declarative_api.cc:162:
->GetRulesRegistry(rules_registry_id, has_webview, event_name);
On 2014/11/28 11:35:51, Fady Samuel wrote:
> I don't think we need to pass in has_webview anymore.

Removed.

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

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:40: :
current_rules_registry_id_(0),
On 2014/11/28 11:41:31, Fady Samuel wrote:
> Use kDefaultRulesRegistryID

Done.

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:58: bool
cache_rules) {
On 2014/11/28 11:34:24, Fady Samuel wrote:
> You know, now that I think about it, we can probably get rid of cache_rules
too.
> Then simply add a comment about how:
> 
> // Only cache rules for regular pages.

This is a great idea, thanks.

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:70: if
(!cache_rules) {
On 2014/11/28 11:34:24, Fady Samuel wrote:
> The logic here is reversed. if (cache_rules) then do whatever.
> 
> I would suggest:
> 
> if (rules_registry_id == kDefaultRulesRegistryID) {
>   ....
>   // create a RulesCacheDelegate here.
> 
> }

Thanks a lot for catching up this reversed logic. Updated.

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:86: // Only create
a ContentRulesRegistry for regular pages and not webviews.
On 2014/11/28 11:34:24, Fady Samuel wrote:
> Get rid of the reference to <webview>.

Done.

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:87: if
(!cache_rules) {
On 2014/11/28 11:34:24, Fady Samuel wrote:
> This is inverted. As above, simply do the following:
> 
> if (rules_registry_id == kDefaultRulesRegistryID)
> ...

Done.

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:142: bool
cache_rules,
On 2014/11/28 11:34:24, Fady Samuel wrote:
> We can probably get rid of this.

Done.

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:158:
rule_registries_.erase(it);
On 2014/11/28 11:41:31, Fady Samuel wrote:
> Modifying a container while iterating over it can lead to badnes. I would
> restore "registries_to_delete".

I just realize that we might need to delete multiple items rather than one. It
is safe to have the registries_to_delete set. Restored.

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/api/...
File extensions/browser/api/declarative/rules_registry_service.h (right):

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.h:83: void
RemoveRulesRegistryID(int rules_registry_id);
On 2014/11/28 11:34:24, Fady Samuel wrote:
> RemoveRulesRegistry

Done.

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:202:
WebViewKeyToIDMap::iterator it = web_view_key_to_id_map.Get().find(key);
On 2014/11/28 11:34:24, Fady Samuel wrote:
> nit: Use auto.
> 
> auto it = web_view_key_to_id_map.Get().find(key);

Done. Do we use auto whenever possible?

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:203: if (it !=
web_view_key_to_id_map.Get().end()) {
On 2014/11/28 11:34:25, Fady Samuel wrote:
> No need for a new block.

Done.

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:384:
id_to_web_view_key_map.Get().find(rules_registry_id_);
On 2014/11/28 11:34:25, Fady Samuel wrote:
> Hmm, I don't think we need IDToWebViewKeyMap? A WebViewGuest should already
know
> about its embedder_process_id and view_instance_id()

You are right. It was used before I move the "remove rules registries" from
RulesRegistryService. Cleaned up.

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:711:
rules_registry_id_(0),
On 2014/11/28 11:34:24, Fady Samuel wrote:
> Use the kDefaultRulesRegistryID constant here?

Define kInvalidRulesRegistryID in RulesRegistryService.

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:934: rules_registry_id_
= web_view_key_to_id_map.Get().find(key)->second;
On 2014/11/28 11:34:25, Fady Samuel wrote:
> Hmm, what if add rules after attachment?
> 
> I feel like the right approach is to always allocate a rules registry ID at
this
> point:
> 
> rules_registry_id_ = GetOrGenerateRulesRegistryID(embedder_process_id,
> view_instance_id());

This is a good point. Updated.

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.h (right):

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.h:64: int
webview_instance_id,
On 2014/11/28 11:34:25, Fady Samuel wrote:
> nit: web_view_instance_id to be more consistent with spacing in this file

Done.

https://codereview.chromium.org/764643002/diff/140001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.h:326: // Store the ID of
this webview in the RulesRegistryService.
On 2014/11/28 11:34:25, Fady Samuel wrote:
> nit:
> 
> // Identifies the set of rules registries belonging to this guest.

Done.

[Xi Han, 2014-11-28 16:13:55]

Patchset #4 (id:160001) has been deleted

[Fady Samuel, 2014-11-28 16:39:30]

Mostly just nits now + waiting for a test.

https://codereview.chromium.org/764643002/diff/180001/extensions/browser/api/...
File extensions/browser/api/declarative/rules_registry_service.h (right):

https://codereview.chromium.org/764643002/diff/180001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.h:82: void
RemoveRulesRegistry(int rules_registry_id);
RemoveRulesRegistriesByID

https://codereview.chromium.org/764643002/diff/180001/extensions/browser/api/...
File extensions/browser/api/declarative/rules_registry_unittest.cc (right):

https://codereview.chromium.org/764643002/diff/180001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_unittest.cc:11: #include
"extensions/browser/api/declarative/rules_registry_service.h"
Is this necessary?

https://codereview.chromium.org/764643002/diff/180001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_unittest.cc:18: const int key
= extensions::RulesRegistryService::kDefaultRulesRegistryID;
Is this necessary?

https://codereview.chromium.org/764643002/diff/180001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/764643002/diff/180001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:381: WebViewKey
key(embedder_process_id, view_instance_id());
nit: embedder_render_process_id()

https://codereview.chromium.org/764643002/diff/180001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:926:
embedder_process_id, view_instance_id(), browser_context());
nit: use embedder_render_process_id()

[Xi Han, 2014-11-28 17:26:38]

https://codereview.chromium.org/764643002/diff/180001/extensions/browser/api/...
File extensions/browser/api/declarative/rules_registry_unittest.cc (right):

https://codereview.chromium.org/764643002/diff/180001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_unittest.cc:11: #include
"extensions/browser/api/declarative/rules_registry_service.h"
On 2014/11/28 16:39:29, Fady Samuel wrote:
> Is this necessary?

Yes, please see line 29 and 138.

https://codereview.chromium.org/764643002/diff/180001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_unittest.cc:18: const int key
= extensions::RulesRegistryService::kDefaultRulesRegistryID;
On 2014/11/28 16:39:29, Fady Samuel wrote:
> Is this necessary?

Yes, please see line 29 and 138.

https://codereview.chromium.org/764643002/diff/180001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/764643002/diff/180001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:381: WebViewKey
key(embedder_process_id, view_instance_id());
On 2014/11/28 16:39:30, Fady Samuel wrote:
> nit: embedder_render_process_id()

Done.

https://codereview.chromium.org/764643002/diff/180001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:926:
embedder_process_id, view_instance_id(), browser_context());
On 2014/11/28 16:39:30, Fady Samuel wrote:
> nit: use embedder_render_process_id()

Done.

[Fady Samuel, 2014-11-28 17:35:21]

This looks good. I'll sign off on it once you have tests :-)

[Xi Han, 2014-11-28 21:55:15]

Hi Fady: I add two WebView browser tests to cover the removed tests from
RulesRegistryServiceTest, PTAL, thanks.

[Xi Han, 2014-11-28 21:55:34]

Patchset #6 (id:210001) has been deleted

[Fady Samuel, 2014-11-28 22:02:49]

lgtm + nits. Please create a bug to implement declarative script injection in
<webview>. Thanks!

https://codereview.chromium.org/764643002/diff/230001/chrome/browser/apps/web...
File chrome/browser/apps/web_view_browsertest.cc (right):

https://codereview.chromium.org/764643002/diff/230001/chrome/browser/apps/web...
chrome/browser/apps/web_view_browsertest.cc:136: event_occur_ = true;
nit: terminated_

https://codereview.chromium.org/764643002/diff/230001/chrome/browser/apps/web...
chrome/browser/apps/web_view_browsertest.cc:2358: extensions::TestRulesRegistry*
key = new extensions::TestRulesRegistry(
this is a rules_registry not a key.

[Xi Han, 2014-11-28 22:24:38]

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

[Xi Han, 2014-11-28 22:24:39]

rockot@chromium.org: Please review all changes. Thanks a lot!

https://codereview.chromium.org/764643002/diff/230001/chrome/browser/apps/web...
File chrome/browser/apps/web_view_browsertest.cc (right):

https://codereview.chromium.org/764643002/diff/230001/chrome/browser/apps/web...
chrome/browser/apps/web_view_browsertest.cc:136: event_occur_ = true;
On 2014/11/28 22:02:49, Fady Samuel wrote:
> nit: terminated_

Done.

https://codereview.chromium.org/764643002/diff/230001/chrome/browser/apps/web...
chrome/browser/apps/web_view_browsertest.cc:2358: extensions::TestRulesRegistry*
key = new extensions::TestRulesRegistry(
On 2014/11/28 22:02:49, Fady Samuel wrote:
> this is a rules_registry not a key.

Done.

[Xi Han, 2014-12-01 14:48:21]

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

[Xi Han, 2014-12-01 14:56:09]

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

[Xi Han, 2014-12-01 14:56:10]

jyasskin@chromium.org: Please review all changes. Thanks a lot.

[Xi Han, 2014-12-01 14:56:24]

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

[Ken Rockot(use gerrit already), 2014-12-01 22:02:38]

lgtm

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

https://codereview.chromium.org/764643002/diff/250001/extensions/browser/api/...
extensions/browser/api/declarative/declarative_api.cc:146: bool has_webview =
web_view_instance_id != 0;
nit: If this is changing from webview_instance_id to web_view_instance_id,
perhaps has_webview should also change to has_web_view.

https://codereview.chromium.org/764643002/diff/250001/extensions/browser/api/...
File extensions/browser/api/declarative_content/content_rules_registry.h
(right):

https://codereview.chromium.org/764643002/diff/250001/extensions/browser/api/...
extensions/browser/api/declarative_content/content_rules_registry.h:24: //
TODO(wjmaclean): Remove this once ContentRulesRegistry moves to
nit: Can this TODO be removed?

[Fady Samuel, 2014-12-01 22:05:16]

https://codereview.chromium.org/764643002/diff/250001/extensions/browser/api/...
File extensions/browser/api/declarative_content/content_rules_registry.h
(right):

https://codereview.chromium.org/764643002/diff/250001/extensions/browser/api/...
extensions/browser/api/declarative_content/content_rules_registry.h:24: //
TODO(wjmaclean): Remove this once ContentRulesRegistry moves to
On 2014/12/01 22:02:38, Ken Rockot wrote:
> nit: Can this TODO be removed?

content rules are still in chrome.

[Fady Samuel, 2014-12-01 22:05:16]

https://codereview.chromium.org/764643002/diff/250001/extensions/browser/api/...
File extensions/browser/api/declarative_content/content_rules_registry.h
(right):

https://codereview.chromium.org/764643002/diff/250001/extensions/browser/api/...
extensions/browser/api/declarative_content/content_rules_registry.h:24: //
TODO(wjmaclean): Remove this once ContentRulesRegistry moves to
On 2014/12/01 22:02:38, Ken Rockot wrote:
> nit: Can this TODO be removed?

content rules are still in chrome.

[Xi Han, 2014-12-01 22:21:53]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-12-01 22:23:37]

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

[commit-bot: I haz the power, 2014-12-01 23:16:47]

Committed patchset #8 (id:260001)

[commit-bot: I haz the power, 2014-12-01 23:17:25]

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