Remove ContentRulesRegistry dependence from RulesRegistryService.

Move RulesRegistryService to //extensions.

This CL moves RulesRegistryService to //extensions. Since it was dependent on ContentRulesRegistry, we move the essential elements of that interface to //extensions as well, renaming what remains as ChromeContentRulesRegistry.

BUG=352293
Committed: https://crrev.com/76c67583b565e93ba11f8c0337e696191d760009
Cr-Commit-Position: refs/heads/master@{#296255}

[wjmaclean, 2014-09-19 20:39:23]

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

[wjmaclean, 2014-09-19 20:39:23]

Please take a look when you get a moment? The actual move of the
rules_registry_service files will have to wait until after webrequest_api moves.

[Ken Rockot(use gerrit already), 2014-09-19 22:56:10]

https://codereview.chromium.org/590573002/diff/1/extensions/browser/api/exten...
File extensions/browser/api/extensions_api_client.h (right):

https://codereview.chromium.org/590573002/diff/1/extensions/browser/api/exten...
extensions/browser/api/extensions_api_client.h:100: virtual
ContentRulesRegistry* ConvertToContentRulesRegistryPtr(
I don't think this should be in ExtensionsAPIClient. How about just making a
static method on ContentRulesRegistry which can do this cast?

[wjmaclean, 2014-09-22 13:46:26]

On 2014/09/19 22:56:10, Ken Rockot wrote:
>
https://codereview.chromium.org/590573002/diff/1/extensions/browser/api/exten...
> File extensions/browser/api/extensions_api_client.h (right):
> 
>
https://codereview.chromium.org/590573002/diff/1/extensions/browser/api/exten...
> extensions/browser/api/extensions_api_client.h:100: virtual
> ContentRulesRegistry* ConvertToContentRulesRegistryPtr(
> I don't think this should be in ExtensionsAPIClient. How about just making a
> static method on ContentRulesRegistry which can do this cast?

I don't think it can be static ... ContentRulesRegistry will still be in Chrome
for a while, and so RulesRegistryService cannot directly reference it.

I've added a ToContentRulesRegistry function on RulesRegistry, overriden in
ContentRulesRegistry in this next patch; does that seem ok?

[wjmaclean, 2014-09-22 17:05:27]

On 2014/09/22 13:46:26, wjmaclean wrote:
> 
> I don't think it can be static ... ContentRulesRegistry will still be in
Chrome
> for a while ...

I spoke with fsamuel@ about this, and declarative_content won't move for some
time yet (though it is slated to eventually move).

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

On 2014/09/22 17:05:27, wjmaclean wrote:
> On 2014/09/22 13:46:26, wjmaclean wrote:
> > 
> > I don't think it can be static ... ContentRulesRegistry will still be in
> Chrome
> > for a while ...
> 
> I spoke with fsamuel@ about this, and declarative_content won't move for some
> time yet (though it is slated to eventually move).

I don't understand the reason why it can't be static.

It's just a downcast from a base type to a more specific type. A static method
to do this, living in the specific type's interface, is not unprecedented
(consider Profile::FromBrowserContext for example).

I see no references to this casting function inside //extensions, so why can't
it be done by a static function in Chrome's ContentRulesRegistry?

What's wrong with static ContentRulesRegistry::FromRulesRegistry?

[wjmaclean, 2014-09-22 20:11:04]

I've added an //extensions intermediary interface for ContentRulesRegistry so
that RuleRegistryService is never dealing with something that isn't fully
defined within //extensions.

PTAL?

[Ken Rockot(use gerrit already), 2014-09-22 21:13:25]

On 2014/09/22 20:11:04, wjmaclean wrote:
> I've added an //extensions intermediary interface for ContentRulesRegistry so
> that RuleRegistryService is never dealing with something that isn't fully
> defined within //extensions.
> 
> PTAL?

Hmm - I like where this is going, but I think it can be simplified further.

Why not just change ContentRulesRegistry to BE the intermediary class and
define it in //extensions as a pure virtual interface that inherits
RulesRegistry. (Side note: preferably defined in its own file rather than
inside of rules_registry.h)

Then the current ContentRulesRegistry can become ChromeContentRulesRegistry
and it can inherit/implement ContentRulesRegistry rather than just
RulesRegistry.

I think pretty much everything else can stay the way it was then, just have
ChromeExtensionsAPIClient::GetContentRulesRegistry return an instance of
ChromeContentRulesRegistry.

I think this would be more in line with how we typically handle progressive
refactors out of chrome. WDYT?

[wjmaclean, 2014-09-23 14:59:01]

PTAL

I had resisted renaming ContentRulesRegistry before since I figured it would
create a lot of churn, and since it will have to be un-re-named when the
declarative_content/ stuff actually moves.

That being said, I made the change, as well as moved the RulesRegistryService
files (since webrequest_api has moved in the meantime, and that was the only
remaining blocker).

[Fady Samuel, 2014-09-23 15:04:50]

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

[Fady Samuel, 2014-09-23 15:04:51]

https://codereview.chromium.org/590573002/diff/60001/extensions/browser/api/d...
File extensions/browser/api/declarative/rules_registry_service.cc (right):

https://codereview.chromium.org/590573002/diff/60001/extensions/browser/api/d...
extensions/browser/api/declarative/rules_registry_service.cc:96:
ExtensionsAPIClient::Get()->GetContentRulesRegistry(
nit: Could we please rename this CreateContentRulesRegistry?
GetContentRulesRegistry suggests a getter, rather than constructing a new
object.

[wjmaclean, 2014-09-23 15:19:05]

PTAL

https://codereview.chromium.org/590573002/diff/60001/extensions/browser/api/d...
File extensions/browser/api/declarative/rules_registry_service.cc (right):

https://codereview.chromium.org/590573002/diff/60001/extensions/browser/api/d...
extensions/browser/api/declarative/rules_registry_service.cc:96:
ExtensionsAPIClient::Get()->GetContentRulesRegistry(
On 2014/09/23 15:04:50, Fady Samuel wrote:
> nit: Could we please rename this CreateContentRulesRegistry?
> GetContentRulesRegistry suggests a getter, rather than constructing a new
> object.

Sure, good point!

Done.

[Ken Rockot(use gerrit already), 2014-09-23 16:13:41]

Thanks for the rename, I think this looks great. lgtm!

https://codereview.chromium.org/590573002/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry.h
(right):

https://codereview.chromium.org/590573002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry.h:70:
// ChromeContentRulesRegistryIntermediary implementation:
nit: Update comment.

[wjmaclean, 2014-09-23 17:55:22]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-09-23 17:59:26]

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

[commit-bot: I haz the power, 2014-09-23 21:34:05]

Committed patchset #6 (id:100001) as 168e9057c22f41e97f81d0789fea8136c7d91201

[commit-bot: I haz the power, 2014-09-23 21:34:49]

Patchset 6 (id:??) landed as
https://crrev.com/76c67583b565e93ba11f8c0337e696191d760009
Cr-Commit-Position: refs/heads/master@{#296255}