Detach the dependency from host_content_settings_map to extension.

Detach the dependency from host_content_settings_map to extension.

In order to componentize host_content_settings_map, one big obstacle
is the extension system. This CL changes the direction of the
dependency.
old: host_content_settings_map register extension system
new: host_content_settings_map accepts providers which allows
     extension system to register itself.

I believe this is a better design.

BUG=384869
R=yoz@chromium.org, markusheintz@chromium.org, bauerb@chromium.org
TBR=rlp@chromium.org, sky@chromium.org
TEST=no regression

Committed: https://crrev.com/87a8d403e43c43bc4c5f24102531b9f15d794263
Cr-Commit-Position: refs/heads/master@{#294877}

[Yoyo Zhou, 2014-09-08 19:51:26]

LGTM with nits

https://codereview.chromium.org/545413002/diff/20001/chrome/browser/content_s...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

https://codereview.chromium.org/545413002/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/host_content_settings_map.cc:133:
DCHECK(used_from_thread_id_ != base::kInvalidThreadId)
DCHECK_NE

https://codereview.chromium.org/545413002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/545413002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.h:366: // Register myself and
content settings API to the specified map.
Change 'myself' to 'self' and 'to' to 'with'

[Jun Mukai, 2014-09-08 20:01:39]

https://codereview.chromium.org/545413002/diff/20001/chrome/browser/content_s...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

https://codereview.chromium.org/545413002/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/host_content_settings_map.cc:133:
DCHECK(used_from_thread_id_ != base::kInvalidThreadId)
On 2014/09/08 19:51:26, Yoyo Zhou wrote:
> DCHECK_NE

Done.

https://codereview.chromium.org/545413002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/545413002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.h:366: // Register myself and
content settings API to the specified map.
On 2014/09/08 19:51:26, Yoyo Zhou wrote:
> Change 'myself' to 'self' and 'to' to 'with'

Done.

[Jun Mukai, 2014-09-11 00:00:51]

mukai@chromium.org changed reviewers:
+ bauerb@chromium.org

[Jun Mukai, 2014-09-11 00:01:02]

bauerb, could you take a look?

[Bernhard Bauer, 2014-09-11 09:39:28]

bauerb@chromium.org changed reviewers:
+ vabr@chromium.org

[Bernhard Bauer, 2014-09-11 09:39:29]

+vabr

https://codereview.chromium.org/545413002/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/host_content_settings_map.h (right):

https://codereview.chromium.org/545413002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/host_content_settings_map.h:50:
INTERNAL_EXTENSION_PROVIDER = 0,
Hm... here we still have references to extensions.

I suppose if we were to do this really cleanly, we would have a skeleton HCSM
that allows registering providers but doesn't know anything about them, and one
embedder (maybe a subclass, maybe owning it) which would add thoses providers.
Not sure if that's worth it though or whether we can live with this layering
violation.

[Jun Mukai, 2014-09-11 22:58:37]

https://codereview.chromium.org/545413002/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/host_content_settings_map.h (right):

https://codereview.chromium.org/545413002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/host_content_settings_map.h:50:
INTERNAL_EXTENSION_PROVIDER = 0,
On 2014/09/11 09:39:29, Bernhard Bauer wrote:
> Hm... here we still have references to extensions.
> 
> I suppose if we were to do this really cleanly, we would have a skeleton HCSM
> that allows registering providers but doesn't know anything about them, and
one
> embedder (maybe a subclass, maybe owning it) which would add thoses providers.
> Not sure if that's worth it though or whether we can live with this layering
> violation.

What I care most in this CL is the coding level dependency, i.e. #include of
"chrome/browser/extensions".  I have little concerns on 'EXTENSION' names.

However, I agree taht ideally this types are not great. Therefore, I uploaded a
new patchset, which removes ProviderTypes. Can you take another look?

[Bernhard Bauer, 2014-09-12 09:07:25]

https://codereview.chromium.org/545413002/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/host_content_settings_map.h (right):

https://codereview.chromium.org/545413002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/host_content_settings_map.h:50:
INTERNAL_EXTENSION_PROVIDER = 0,
On 2014/09/11 22:58:37, Jun Mukai wrote:
> On 2014/09/11 09:39:29, Bernhard Bauer wrote:
> > Hm... here we still have references to extensions.
> > 
> > I suppose if we were to do this really cleanly, we would have a skeleton
HCSM
> > that allows registering providers but doesn't know anything about them, and
> one
> > embedder (maybe a subclass, maybe owning it) which would add thoses
providers.
> > Not sure if that's worth it though or whether we can live with this layering
> > violation.
> 
> What I care most in this CL is the coding level dependency, i.e. #include of
> "chrome/browser/extensions".  I have little concerns on 'EXTENSION' names.

Yeah, it was more of a layering violation in spirit, not in code (and like I
said, maybe we can just live with it for now).

> However, I agree taht ideally this types are not great. Therefore, I uploaded
a
> new patchset, which removes ProviderTypes. Can you take another look?

This enum also defined the precedence of the different providers, making sure
that e.g. policy-defined content settings take precedence over user-defined,
etc. If you just keep the providers in an array, are you still going to have the
right precedences?

https://codereview.chromium.org/545413002/diff/100001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

https://codereview.chromium.org/545413002/diff/100001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map.cc:119:
HostContentSettingsMap::GetProviderAt(
The parameter should fit on this line, no?

[Jun Mukai, 2014-09-12 20:21:05]

https://codereview.chromium.org/545413002/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/host_content_settings_map.h (right):

https://codereview.chromium.org/545413002/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/host_content_settings_map.h:50:
INTERNAL_EXTENSION_PROVIDER = 0,
On 2014/09/12 09:07:25, Bernhard Bauer wrote:
> On 2014/09/11 22:58:37, Jun Mukai wrote:
> > On 2014/09/11 09:39:29, Bernhard Bauer wrote:
> > > Hm... here we still have references to extensions.
> > > 
> > > I suppose if we were to do this really cleanly, we would have a skeleton
> HCSM
> > > that allows registering providers but doesn't know anything about them,
and
> > one
> > > embedder (maybe a subclass, maybe owning it) which would add thoses
> providers.
> > > Not sure if that's worth it though or whether we can live with this
layering
> > > violation.
> > 
> > What I care most in this CL is the coding level dependency, i.e. #include of
> > "chrome/browser/extensions".  I have little concerns on 'EXTENSION' names.
> 
> Yeah, it was more of a layering violation in spirit, not in code (and like I
> said, maybe we can just live with it for now).
> 
> > However, I agree taht ideally this types are not great. Therefore, I
uploaded
> a
> > new patchset, which removes ProviderTypes. Can you take another look?
> 
> This enum also defined the precedence of the different providers, making sure
> that e.g. policy-defined content settings take precedence over user-defined,
> etc. If you just keep the providers in an array, are you still going to have
the
> right precedences?

Hm. Considering the situation, I think it's better to keep this enum and the
structure of the code. EXTENSION_PROVIDER things are okay as the names.
We may want to remove this enum eventually, but not now.

Revert the patchset back to the original one. Do you think this is okay?

[Bernhard Bauer, 2014-09-15 09:07:58]

LGTM

https://codereview.chromium.org/545413002/diff/120001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map.h (right):

https://codereview.chromium.org/545413002/diff/120001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map.h:50:
INTERNAL_EXTENSION_PROVIDER = 0,
Can you add a TODO here that this is a layering violation once we move HCSM to a
component?

[vabr (Chromium), 2014-09-15 15:02:22]

vabr@chromium.org changed reviewers:
- vabr@chromium.org

[Jun Mukai, 2014-09-15 16:49:26]

https://codereview.chromium.org/545413002/diff/120001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map.h (right):

https://codereview.chromium.org/545413002/diff/120001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map.h:50:
INTERNAL_EXTENSION_PROVIDER = 0,
On 2014/09/15 09:07:58, Bernhard Bauer wrote:
> Can you add a TODO here that this is a layering violation once we move HCSM to
a
> component?

Done.

[Jun Mukai, 2014-09-15 16:50:05]

mukai@chromium.org changed reviewers:
+ rlp@chromium.org, sky@chromium.org

[Jun Mukai, 2014-09-15 16:51:31]

The following files need OWNERS:
    chrome/browser/profiles/off_the_record_profile_impl.cc
    chrome/browser/profiles/profile_manager.cc
    chrome/test/base/testing_profile.cc

Let me move forward with TBR=rlp, sky because their changes are mechanical.

[Jun Mukai, 2014-09-15 16:51:37]

The CQ bit was checked by mukai@chromium.org

[sky, 2014-09-15 17:10:47]

Said files LGTM

[commit-bot: I haz the power, 2014-09-15 17:18:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/545413002/140001

[commit-bot: I haz the power, 2014-09-15 20:16:37]

Committed patchset #8 (id:140001) as d55576790b0f53b255cbc04e46db44769ce2eb79

[commit-bot: I haz the power, 2014-09-15 20:21:14]

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