Refactor RulesRegistryWithCache to RulesRegistry

Refactor RulesRegistryWithCache to RulesRegistry

As a prerequisites to isolating declarative RulesRegistry on a per-webview basis, we need to do some cleanup.

1. Get rid of the abstract class RulesRegistry, RulesRegistryWithCache becomes RulesRegistry. The caching component becomes an optional delegate.
2. Get rid of InitializingRulesRegistry. Move that functionality down into RulesRegistry.

This CL accomplishes the above but the RulesCacheDelegate is always installed. It will become optional in a subsequent CL.

BUG=312461
R=jyasskin@chromium.org, vabr@chromium.org

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

[vabr (Chromium), 2013-10-29 13:08:23]

Hi Fady,

Thanks for going the systematic way to dropping the rules store for webview!

Having said that, I believe we can improve the separation of the delegate and
the RulesRegistry made so far.

Currently, RulesRegistry is just the interface, RulesRegistryWithCache takes
care of storing the rules on disk, and the API-dependent rules registries handle
interpreting the rules.
I don't think we should change that separation, we only should change the way
the RulesRegistryWithCache is plugged in.

Currently we do that by inheritance, which is a problem for you, because for
webview, you would like to leave out RulesRegistryWithCache. So we should make
RulesRegistryWithCache a delegate.

For implementation reasons, RulesRegistryWithCache comes in two parts (UI part,
and the part to run on the registry thread, which might be UI or IO). You
separated only the UI part as the delegate, and left the registry-thread part in
RulesRegistry.

Note that even if the delegate is not used (in the case of webview registries),
there will be a lot of things done in the RulesRegistry for no reason, like
keeping the |rules_| dictionary. Additionally, this creates unnecessary
dependencies between the delegate and RulesRegistry such as the need to friend
the delegate in RulesRegistry.

So I suggest keeping the RulesRegistry an interface, only with the changes that
allow the delegate to hook into the public methods of RulesRegistry. This will
also define the necessary interface of the delegate (say,
RulesRegistry::Delegate).

The delegate itself would then be again 2-part: registry-thread and UI-thread
part, living in one .h/.cc. It will be almost exactly like the current
RulesRegistryWithCache, only it will not inheriting from RulesRegistry, but from
RulesRegistry::Delegate.


I am very interested what Jeffrey thinks about this, and I'm happy to meet with
you guys over VC if you consider it helpful.

Cheers,
Vaclav

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

https://codereview.chromium.org/49693003/diff/60001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/rules_registry.h:62: // RulesRegistry
implementation:
Is this comment just a relict from RulesRegistryWithCache?

https://codereview.chromium.org/49693003/diff/60001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/rules_registry.h:149: friend class
RulesCacheDelegate;
I'm a bit uncertain about friending the delegate. Previously, the 
RuleStorageOnUI (now delegate) and the RulesRegistryWithCache (now
RulesRegistry) were in the same .cc file, and both fixed parts of a one working
unit.
Now the delegate is separate, and potentially replaceable (someone could write a
different one). Making (potentially multiple) friends outside of the current .cc
file makes maintaining the implementation more complex.

Since the delegate only seems to use MarkReady and DeserializeAndAddRules, one
solution would be to pass them to the delegate as base::Callbacks initially.

But I think a better option would be to separate the caching facility more
systematically (see the beginning of my e-mail).

[vabr (Chromium), 2013-10-29 16:05:54]

Hi Fady,

Thanks for clarifying your intents over IM.

So now I understand you want to complete the separation of the rules caching and
the RulesRegistry in a subsequent CL, by pulling code remaining after
RulesRegistryWithCache out of RulesRegistry. That's fine by me, as long as you
document it in the code with TODOs.

I tried to read the code carefully, but most of it is just parts moved around,
so my attention might have slipped. If you want me to have a special look at
sections you modified beyond just moving (or simple merging independent pieces
of code, like the RulesRegistry::AddRules getting code both from InitializingRR
and RRWithCache), please let me know.

Otherwise this LGTM. Please also wait until Jeffrey has a look.

Thanks for your efforts!

Vaclav

https://codereview.chromium.org/49693003/diff/60001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative/initializing_rules_registry_unittest.cc
(right):

https://codereview.chromium.org/49693003/diff/60001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/initializing_rules_registry_unittest.cc:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
The filename looks a bit strange, given that there is no more
InitializingRulesRegistry. Given there is no rulesr_registry_unittest.cc, should
we rename this to rulesr_registry_unittest.cc?

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

https://codereview.chromium.org/49693003/diff/60001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/rules_registry.cc:127: // Commit all
rules into |rules_| on success.
This is one of the parts when the rules caching code stays in the RulesRegistry,
and should be pulled out in the next step. It would be good to add TODOs to
those parts to make it clear that these parts would be moved soon.

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

https://codereview.chromium.org/49693003/diff/60001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/rules_registry.h:214: // Creates an ID
that is unique within the scope of|extension_id|.
typo from the original code: space between "of" and "|extension_id|".

https://codereview.chromium.org/49693003/diff/60001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative/rules_registry_service.h (right):

https://codereview.chromium.org/49693003/diff/60001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/rules_registry_service.h:77:
RulesRegistryMap;
nit: This line seems to actually fit on one line.

https://codereview.chromium.org/49693003/diff/60001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative/rules_registry_with_cache_unittest.cc
(right):

https://codereview.chromium.org/49693003/diff/60001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/rules_registry_with_cache_unittest.cc:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
Again, the filename of this unittest is slightly confusing, now that there's no
rules_registry_with_cache.

Given that the rule-caching functionality is about to be moved completely to the
delegate, should this become rules_cache_delegate_unittest.cc?

https://codereview.chromium.org/49693003/diff/60001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/rules_registry_with_cache_unittest.cc:113:
EXPECT_EQ("", AddRule(kExtensionId, kRule2Id));
Just out of curiosity, why this change? :)
(No need to change back, I see no difference. I'm just curious.)

[vabr (Chromium), 2013-10-29 16:06:48]

Hi Fady,

Thanks for clarifying your intents over IM.

So now I understand you want to complete the separation of the rules caching and
the RulesRegistry in a subsequent CL, by pulling code remaining after
RulesRegistryWithCache out of RulesRegistry. That's fine by me, as long as you
document it in the code with TODOs.

I tried to read the code carefully, but most of it is just parts moved around,
so my attention might have slipped. If you want me to have a special look at
sections you modified beyond just moving (or simple merging independent pieces
of code, like the RulesRegistry::AddRules getting code both from InitializingRR
and RRWithCache), please let me know.

Otherwise this LGTM. Please also wait until Jeffrey has a look.

Thanks for your efforts!

Vaclav

[Jeffrey Yasskin, 2013-10-30 22:30:48]

lgtm

https://codereview.chromium.org/49693003/diff/60001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative/initializing_rules_registry_unittest.cc
(right):

https://codereview.chromium.org/49693003/diff/60001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/initializing_rules_registry_unittest.cc:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2013/10/29 16:05:54, vabr (Chromium) wrote:
> The filename looks a bit strange, given that there is no more
> InitializingRulesRegistry. Given there is no rulesr_registry_unittest.cc,
should
> we rename this to rulesr_registry_unittest.cc?

+1 to renaming this file eventually, but a separate CL is fine in order to keep
the diffs clear.

https://codereview.chromium.org/49693003/diff/230001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_cache_delegate.h (right):

https://codereview.chromium.org/49693003/diff/230001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_cache_delegate.h:26: //
RulesRegistryService, and is deleted by the service.
Although note that if the tests use TestBrowserThreadBundle, we could probably
go back to having the RulesCacheDelegate actually owned by its RulesRegistry.

https://codereview.chromium.org/49693003/diff/230001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry.cc (right):

https://codereview.chromium.org/49693003/diff/230001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry.cc:108: std::string
error = CheckAndFillInOptionalRules(extension_id, rules);
This is probably fine, but note that, now that InitializingRulesRegistry and
RulesRegistryWithCache are the same, on startup we'll call these FillIn methods
redundantly when re-adding the rules from the cache.

https://codereview.chromium.org/49693003/diff/230001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry.cc:319: // Now we
generate IDs in case they were not specificed in the rules. This
sp: specificed

https://codereview.chromium.org/49693003/diff/230001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry.h (right):

https://codereview.chromium.org/49693003/diff/230001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry.h:53: const
std::string& event_name,
Indentation is odd here.

https://codereview.chromium.org/49693003/diff/230001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry.h:137: // RulesRegistry
counterparts. They need to be atomic.
There aren't counterparts anymore, and the RulesRegistryWithCache comments were
bad anyway ... Maybe say that these functions need to apply the rules to the
browser, while the base class will handle defaulting empty fields before calling
*Impl, and will automatically cache the rules and re-call *Impl on browser
startup.

[Fady Samuel, 2013-10-31 14:02:23]

Addressed nits. CQ'ing.

https://codereview.chromium.org/49693003/diff/230001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_cache_delegate.h (right):

https://codereview.chromium.org/49693003/diff/230001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_cache_delegate.h:26: //
RulesRegistryService, and is deleted by the service.
On 2013/10/30 22:30:48, Jeffrey Yasskin wrote:
> Although note that if the tests use TestBrowserThreadBundle, we could probably
> go back to having the RulesCacheDelegate actually owned by its RulesRegistry.

In a subsequent patch, RulesCacheDelegate will be made optional. However
transferring ownership to the RulesRegistry might be nice. I'll look into it.

https://codereview.chromium.org/49693003/diff/230001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry.cc (right):

https://codereview.chromium.org/49693003/diff/230001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry.cc:108: std::string
error = CheckAndFillInOptionalRules(extension_id, rules);
On 2013/10/30 22:30:48, Jeffrey Yasskin wrote:
> This is probably fine, but note that, now that InitializingRulesRegistry and
> RulesRegistryWithCache are the same, on startup we'll call these FillIn
methods
> redundantly when re-adding the rules from the cache.

OK. This is one time operation on browser startup that hopefully isn't too
expensive.

https://codereview.chromium.org/49693003/diff/230001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry.cc:319: // Now we
generate IDs in case they were not specificed in the rules. This
On 2013/10/30 22:30:48, Jeffrey Yasskin wrote:
> sp: specificed

Done.

https://codereview.chromium.org/49693003/diff/230001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry.h (right):

https://codereview.chromium.org/49693003/diff/230001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry.h:53: const
std::string& event_name,
On 2013/10/30 22:30:48, Jeffrey Yasskin wrote:
> Indentation is odd here.

Done.

https://codereview.chromium.org/49693003/diff/230001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry.h:137: // RulesRegistry
counterparts. They need to be atomic.
On 2013/10/30 22:30:48, Jeffrey Yasskin wrote:
> There aren't counterparts anymore, and the RulesRegistryWithCache comments
were
> bad anyway ... Maybe say that these functions need to apply the rules to the
> browser, while the base class will handle defaulting empty fields before
calling
> *Impl, and will automatically cache the rules and re-call *Impl on browser
> startup.

Done.

[commit-bot: I haz the power, 2013-10-31 14:04:29]

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

[Fady Samuel, 2013-10-31 15:33:13]

Committed patchset #5 manually as r232124 (presubmit successful).

[vabr (Chromium), 2013-10-31 16:35:05]

https://codereview.chromium.org/49693003/diff/230001/chrome/browser/extension...
File chrome/browser/extensions/api/declarative/rules_registry.cc (right):

https://codereview.chromium.org/49693003/diff/230001/chrome/browser/extension...
chrome/browser/extensions/api/declarative/rules_registry.cc:108: std::string
error = CheckAndFillInOptionalRules(extension_id, rules);
On 2013/10/31 14:02:24, Fady Samuel wrote:
> On 2013/10/30 22:30:48, Jeffrey Yasskin wrote:
> > This is probably fine, but note that, now that InitializingRulesRegistry and
> > RulesRegistryWithCache are the same, on startup we'll call these FillIn
> methods
> > redundantly when re-adding the rules from the cache.
> 
> OK. This is one time operation on browser startup that hopefully isn't too
> expensive.

Actually, the start-up time is a critical point. We spent some time this Spring
trying to speed that part up. For Declarative Web Request rules we have to deal
with the order of tens of thousands of rules. For that numbers, all the string
processing like in CheckAndFillInOptionalRules could easily sum up to hundreds
of ms. So if there is a way to remove redundant calls (in a follow-up CL), that
would be great.