Issue 53273002: Decouple RulesCacheDelegate from RulesRegistry

Issue 53273002: Decouple RulesCacheDelegate from RulesRegistry (Closed)

Created:
7 years, 1 month ago by Fady Samuel

Modified:
7 years, 1 month ago

Reviewers:
Jeffrey Yasskin

CC:
chromium-reviews, joi+watch-content_chromium.org, chromium-apps-reviews_chromium.org, darin-cc_chromium.org, jam, extensions-reviews_chromium.org, vabr (Chromium)

Base URL:
https://chromium.googlesource.com/chromium/src.git@refactor_rules_registry_with_cache

Visibility:
Public.

More Reviews

Description

Decouple RulesCacheDelegate from RulesRegistry This is a prerequisite for declarative API support in <webview>. <webview>s should not cache their rules. BUG=312461 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232651

Patch Set 1 #

Patch Set 2 : Merge with ToT #

Total comments: 14

Patch Set 3 : Addressed comments #

Patch Set 4 : A little bit more cleanup #

Patch Set 5 : Fix crash #

Patch Set 6 : Fixed tests #

Patch Set 7 : Updated #

Created: 7 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+128 lines, -133 lines)			Patch
M	chrome/browser/extensions/api/declarative/rules_cache_delegate.h	View	1 2 3 4 5 6	5 chunks	+10 lines, -18 lines	0 comments	Download
M	chrome/browser/extensions/api/declarative/rules_cache_delegate.cc	View	1 2 3 4 5 6	6 chunks	+24 lines, -19 lines	0 comments	Download
M	chrome/browser/extensions/api/declarative/rules_registry.h	View	1 2 3 4 5 6	6 chunks	+13 lines, -7 lines	0 comments	Download
M	chrome/browser/extensions/api/declarative/rules_registry.cc	View	1 2 3 4 5 6	3 chunks	+16 lines, -25 lines	0 comments	Download
M	chrome/browser/extensions/api/declarative/rules_registry_service.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/extensions/api/declarative/rules_registry_service.cc	View	1 2 3 4 5 6	2 chunks	+8 lines, -5 lines	0 comments	Download
M	chrome/browser/extensions/api/declarative/rules_registry_with_cache_unittest.cc	View	1 2 3 4 5 6	3 chunks	+33 lines, -26 lines	0 comments	Download
M	chrome/browser/extensions/api/declarative/test_rules_registry.h	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/extensions/api/declarative/test_rules_registry.cc	View	1 2 3 4 5 6	1 chunk	+3 lines, -5 lines	0 comments	Download
M	chrome/browser/extensions/api/declarative_content/content_rules_registry.h	View	1 2 3 4 5 6	2 chunks	+1 line, -4 lines	0 comments	Download
M	chrome/browser/extensions/api/declarative_content/content_rules_registry.cc	View	1 2 3 4 5 6	6 chunks	+10 lines, -13 lines	0 comments	Download
M	chrome/browser/extensions/api/declarative_content/content_rules_registry_unittest.cc	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc	View	1 2 3 4 5 6	1 chunk	+5 lines, -6 lines	0 comments	Download
M	chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 19 (0 generated)

Expand Messages | Collapse Messages

Jeffrey Yasskin

lgtm, with a couple tweaks and things to think about below. https://codereview.chromium.org/53273002/diff/30001/chrome/browser/extensions/api/declarative/rules_cache_delegate.cc File chrome/browser/extensions/api/declarative/rules_cache_delegate.cc (right): ...

7 years, 1 month ago (2013-11-01 00:15:09 UTC) #2

Fady Samuel

I've addressed the comments you provided for this patch. At least two more patches are ...

7 years, 1 month ago (2013-11-01 15:13:38 UTC) #3

I've addressed the comments you provided for this patch. At least two more
patches are coming. One to isolate rules registries on a per-webview basis, and
one to do further cleanup and decoupling.

https://codereview.chromium.org/53273002/diff/30001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative/rules_cache_delegate.cc (right):

https://codereview.chromium.org/53273002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/rules_cache_delegate.cc:266: void
RulesCacheDelegate::DeserializeAndAddRules(
On 2013/11/01 00:15:09, Jeffrey Yasskin wrote:
> I'm curious why this moved. It doesn't seem to need a RulesCacheDelegate, and
it
> runs on the registry's thread rather than the UI thread, which is the general
> split between methods of RulesRegistry and RulesCacheDelegate.

I thought it would be cleaner but it didn't really turn out that way so I'll
move it back.

https://codereview.chromium.org/53273002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/rules_cache_delegate.cc:272: if
(!registry)
On 2013/11/01 00:15:09, Jeffrey Yasskin wrote:
> This needs to be before the CurrentlyOn call, which assumes |registry| is not
> null.

This is no longer relevant. DeserialAndAddRules has been moved back.

https://codereview.chromium.org/53273002/diff/30001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative/rules_cache_delegate.h (right):

https://codereview.chromium.org/53273002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/rules_cache_delegate.h:25: // by the
RulesRegistry, but right after that it changes owner to the
On 2013/11/01 00:15:09, Jeffrey Yasskin wrote:
> Is "It gets created by the RulesRegistry" still true?

No. Removed.

https://codereview.chromium.org/53273002/diff/30001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative/rules_registry.cc (right):

https://codereview.chromium.org/53273002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/rules_registry.cc:58:
event_name_(event_name),
On 2013/11/01 00:15:09, Jeffrey Yasskin wrote:
> event_name_ is never used in RulesRegistry, just the CacheDelegate, so it'd be
> nice to store it only in the right place. 
> ... I think I see a way to do that by calling Init() from the subclasses, but
> that's definitely more than you need to do in this CL.

More cleanup coming in subsequent CLs. I'd like to keep this simple so I can
make continuous forward progress. :-)

https://codereview.chromium.org/53273002/diff/30001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative/rules_registry.h (right):

https://codereview.chromium.org/53273002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/rules_registry.h:58:
base::WeakPtr<RulesRegistry> GetWeakPtr() {
On 2013/11/01 00:15:09, Jeffrey Yasskin wrote:
> It's good to avoid passing WeakPtrs out of the class when possible. Can we
> organize ownership so we don't need this anymore?
> 
> Probably not in this CL.

I'll take care of this in a subsequent CL. Thanks!

https://codereview.chromium.org/53273002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/rules_registry.h:125: bool
SetCacheDelegate(base::WeakPtr<RulesCacheDelegate> cache_delegate);
On 2013/11/01 00:15:09, Jeffrey Yasskin wrote:
> For functions that return bool, you need to say what the return value means.
> However, I think in this case it'd be better to CHECK or DCHECK and return
void,
> since it's a programming error to set the cache delegate twice.

Done.

https://codereview.chromium.org/53273002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/rules_registry.h:125: bool
SetCacheDelegate(base::WeakPtr<RulesCacheDelegate> cache_delegate);
On 2013/11/01 00:15:09, Jeffrey Yasskin wrote:
> I think it's safe to pass WeakPtrs by reference, which can avoid some refcount
> operations.

Done.