Add a content_settings::BaseProvider for code that is shared by all content settings providers.

Add a content_settings::BaseProvider for code that is shared by all content settings providers.

BUG=63656
TEST=host_content_settings_map_unittest.cc

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=74944

[markusheintz_, 2011-02-14 15:45:24]

Please review my CL Thanks.

[Bernhard Bauer, 2011-02-14 16:58:36]

Nice, I like the separation of the content settings lookup logic and the mapping
to user preferences.

http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_provider.cc (right):

http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_provider.cc:67: //
TODO(markuseheintz) Remove this DCHECK.
Your attempt to keep people from grepping for your TODOs by misspelling your
LDAP does not fool me ;-)

http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_provider.cc:174: // LEGACY: TBR
I think repeating the comment from the header is unnecessary. Also, when I read
"TBR" I always think "to be reviewed" ;-)

http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_provider.h (right):

http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_provider.h:138: // BaseProvider
is the abstract base class for all PolicyProvider classes.
Nit: and also for PrefProviders?

http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_provider.h:139: class
BaseProvider : public ProviderInterface {
With this class and ProviderUtil below, this file will declare four classes,
none of which have a name that corresponds to the filename. That makes searching
for classes really difficult. I'd propose the following: 

- Only subclasses of BaseProvider need to know about it, so you could move it
out to separate header/implementation files.
- A class named namespace::ClassName should live in namespace_class_name.*
files, so in this case you should have all filenames start with
content_settings_, like content_settings_base_provider,
content_settings_pref_provider, etc.
- If a header file defines multiple classes, we usually use a plural name for it
(*Value classes are defined in values.h), so you could rename this file
content_settings_providers.h.

http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_provider.h:185: //
TODO(markusheintz): LEGACY method. Will be removed in a futur re-factoring
Nit: "future"

http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
File chrome/browser/content_settings/pref_content_settings_provider.cc (right):

http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.cc:581:
&(*host_content_settings())[pattern].content_settings_for_resources);
This just looks wrong ;-)

Could you declare a local |ExtendedContentSettings| variable, assign |settings|
to its |content_settings| member and call |GetResourceSettingsFromDictionary| on
its |content_settings_for_resources| member, and afterwards assign the whole
thing to |(*host_content_settings())[pattern]|?

http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
File chrome/browser/content_settings/pref_content_settings_provider.h (right):

http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.h:93: //
PolicyProvider that provides content settings from the user preference.
You mean ContentSettingsProvider?

[markusheintz_, 2011-02-14 18:25:28]

http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_provider.cc (right):

http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_provider.cc:67: //
TODO(markuseheintz) Remove this DCHECK.
On 2011/02/14 16:58:36, Bernhard Bauer wrote:
> Your attempt to keep people from grepping for your TODOs by misspelling your
> LDAP does not fool me ;-)

Damn I have to try harder. Oh ... I mean I didn't do this on purpose ;-)

http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_provider.cc:174: // LEGACY: TBR
On 2011/02/14 16:58:36, Bernhard Bauer wrote:
> I think repeating the comment from the header is unnecessary. Also, when I
read
> "TBR" I always think "to be reviewed" ;-)

:-) that's fair. Removed

http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_provider.h (right):

http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_provider.h:138: // BaseProvider
is the abstract base class for all PolicyProvider classes.
On 2011/02/14 16:58:36, Bernhard Bauer wrote:
> Nit: and also for PrefProviders?

Done.

http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_provider.h:139: class
BaseProvider : public ProviderInterface {
On 2011/02/14 16:58:36, Bernhard Bauer wrote:
> With this class and ProviderUtil below, this file will declare four classes,
> none of which have a name that corresponds to the filename. That makes
searching
> for classes really difficult. I'd propose the following: 
> 
> - Only subclasses of BaseProvider need to know about it, so you could move it
> out to separate header/implementation files.
> - A class named namespace::ClassName should live in namespace_class_name.*
> files, so in this case you should have all filenames start with
> content_settings_, like content_settings_base_provider,
> content_settings_pref_provider, etc.
> - If a header file defines multiple classes, we usually use a plural name for
it
> (*Value classes are defined in values.h), so you could rename this file
> content_settings_providers.h.

You are right this file grew into a real beast now. I'll cut off a couple of
heads.

- I'll move BaseProvider to base_provider.[h,cc] incl. the typedefs

- It's likely that DefaultProviderInterface will go away r merged again in
ProviderInterface. So I'd like to keep these two classes in the same file
content_settings_provider.h and not rename it. Would you be fine with me adding
a comment that this is work in progress?

- The method in ProviderUtil could become a static method of BaseProvider. It's
so tiny and does not deserve a file on it's own yet. And ProviderUtil can go
away again.

wdyt

http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_provider.h:185: //
TODO(markusheintz): LEGACY method. Will be removed in a futur re-factoring
On 2011/02/14 16:58:36, Bernhard Bauer wrote:
> Nit: "future"

Done.

http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
File chrome/browser/content_settings/pref_content_settings_provider.cc (right):

http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.cc:581:
&(*host_content_settings())[pattern].content_settings_for_resources);
On 2011/02/14 16:58:36, Bernhard Bauer wrote:
> This just looks wrong ;-)
> 
> Could you declare a local |ExtendedContentSettings| variable, assign
|settings|
> to its |content_settings| member and call |GetResourceSettingsFromDictionary|
on
> its |content_settings_for_resources| member, and afterwards assign the whole
> thing to |(*host_content_settings())[pattern]|?

Done :-) with slight variation.

http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
File chrome/browser/content_settings/pref_content_settings_provider.h (right):

http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.h:93: //
PolicyProvider that provides content settings from the user preference.
On 2011/02/14 16:58:36, Bernhard Bauer wrote:
> You mean ContentSettingsProvider?

Of course. Done :-)

[Bernhard Bauer, 2011-02-15 09:54:21]

On 2011/02/14 18:25:28, markusheintz_ wrote:
>
http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
> File chrome/browser/content_settings/content_settings_provider.cc (right):
> 
>
http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
> chrome/browser/content_settings/content_settings_provider.cc:67: //
> TODO(markuseheintz) Remove this DCHECK.
> On 2011/02/14 16:58:36, Bernhard Bauer wrote:
> > Your attempt to keep people from grepping for your TODOs by misspelling your
> > LDAP does not fool me ;-)
> 
> Damn I have to try harder. Oh ... I mean I didn't do this on purpose ;-)
> 
>
http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
> chrome/browser/content_settings/content_settings_provider.cc:174: // LEGACY:
TBR
> On 2011/02/14 16:58:36, Bernhard Bauer wrote:
> > I think repeating the comment from the header is unnecessary. Also, when I
> read
> > "TBR" I always think "to be reviewed" ;-)
> 
> :-) that's fair. Removed
> 
>
http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
> File chrome/browser/content_settings/content_settings_provider.h (right):
> 
>
http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
> chrome/browser/content_settings/content_settings_provider.h:138: //
BaseProvider
> is the abstract base class for all PolicyProvider classes.
> On 2011/02/14 16:58:36, Bernhard Bauer wrote:
> > Nit: and also for PrefProviders?
> 
> Done.
> 
>
http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
> chrome/browser/content_settings/content_settings_provider.h:139: class
> BaseProvider : public ProviderInterface {
> On 2011/02/14 16:58:36, Bernhard Bauer wrote:
> > With this class and ProviderUtil below, this file will declare four classes,
> > none of which have a name that corresponds to the filename. That makes
> searching
> > for classes really difficult. I'd propose the following: 
> > 
> > - Only subclasses of BaseProvider need to know about it, so you could move
it
> > out to separate header/implementation files.
> > - A class named namespace::ClassName should live in namespace_class_name.*
> > files, so in this case you should have all filenames start with
> > content_settings_, like content_settings_base_provider,
> > content_settings_pref_provider, etc.
> > - If a header file defines multiple classes, we usually use a plural name
for
> it
> > (*Value classes are defined in values.h), so you could rename this file
> > content_settings_providers.h.
> 
> You are right this file grew into a real beast now. I'll cut off a couple of
> heads.
> 
> - I'll move BaseProvider to base_provider.[h,cc] incl. the typedefs
> 
> - It's likely that DefaultProviderInterface will go away r merged again in
> ProviderInterface. So I'd like to keep these two classes in the same file
> content_settings_provider.h and not rename it. Would you be fine with me
adding
> a comment that this is work in progress?

ok.

> - The method in ProviderUtil could become a static method of BaseProvider.
It's
> so tiny and does not deserve a file on it's own yet. And ProviderUtil can go
> away again.

yes, that's probably better than adding a new file for it.

> wdyt

I'd still rename |pref_content_settings_provider.*| to
|content_settings_pref_provider.*|. If the class is named |PrefProvider|, the
corresponding file name should not have both parts separated.

>
http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
> chrome/browser/content_settings/content_settings_provider.h:185: //
> TODO(markusheintz): LEGACY method. Will be removed in a futur re-factoring
> On 2011/02/14 16:58:36, Bernhard Bauer wrote:
> > Nit: "future"
> 
> Done.
> 
>
http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
> File chrome/browser/content_settings/pref_content_settings_provider.cc
(right):
> 
>
http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
> chrome/browser/content_settings/pref_content_settings_provider.cc:581:
> &(*host_content_settings())[pattern].content_settings_for_resources);
> On 2011/02/14 16:58:36, Bernhard Bauer wrote:
> > This just looks wrong ;-)
> > 
> > Could you declare a local |ExtendedContentSettings| variable, assign
> |settings|
> > to its |content_settings| member and call
|GetResourceSettingsFromDictionary|
> on
> > its |content_settings_for_resources| member, and afterwards assign the whole
> > thing to |(*host_content_settings())[pattern]|?
> 
> Done :-) with slight variation.
> 
>
http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
> File chrome/browser/content_settings/pref_content_settings_provider.h (right):
> 
>
http://codereview.chromium.org/6484035/diff/1006/chrome/browser/content_setti...
> chrome/browser/content_settings/pref_content_settings_provider.h:93: //
> PolicyProvider that provides content settings from the user preference.
> On 2011/02/14 16:58:36, Bernhard Bauer wrote:
> > You mean ContentSettingsProvider?
> 
> Of course. Done :-)

[Bernhard Bauer, 2011-02-15 09:55:25]

http://codereview.chromium.org/6484035/diff/3007/chrome/browser/content_setti...
File chrome/browser/content_settings/pref_content_settings_provider.h (right):

http://codereview.chromium.org/6484035/diff/3007/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.h:93: //
Content-settings-provider that provides content-settings from the user
Nit: Why the dashes?

[markusheintz_, 2011-02-15 11:00:16]

I'd like to rename the file pref_content_settings_provider.* in a separate CL.

http://codereview.chromium.org/6484035/diff/3007/chrome/browser/content_setti...
File chrome/browser/content_settings/pref_content_settings_provider.h (right):

http://codereview.chromium.org/6484035/diff/3007/chrome/browser/content_setti...
chrome/browser/content_settings/pref_content_settings_provider.h:93: //
Content-settings-provider that provides content-settings from the user
On 2011/02/15 09:55:25, Bernhard Bauer wrote:
> Nit: Why the dashes?

For consistence.
e.g. host-content-settings-maps
     content-settings 
     ...

[Bernhard Bauer, 2011-02-15 11:59:40]

On Tue, Feb 15, 2011 at 12:00,  <markusheintz@chromium.org> wrote:
> I'd like to rename the file pref_content_settings_provider.* in a separate
> CL.
>
>
>
http://codereview.chromium.org/6484035/diff/3007/chrome/browser/content_setti...
> File chrome/browser/content_settings/pref_content_settings_provider.h
> (right):
>
>
http://codereview.chromium.org/6484035/diff/3007/chrome/browser/content_setti...
> chrome/browser/content_settings/pref_content_settings_provider.h:93: //
> Content-settings-provider that provides content-settings from the user
> On 2011/02/15 09:55:25, Bernhard Bauer wrote:
>>
>> Nit: Why the dashes?
>
> For consistence.
> e.g. host-content-settings-maps
>     content-settings

I think it reads more natural without them (host content settings map,
content settings, etc.).

>     ...
>
> http://codereview.chromium.org/6484035/
>

[markusheintz_, 2011-02-15 12:06:22]

On 2011/02/15 11:59:40, Bernhard Bauer wrote:
> On Tue, Feb 15, 2011 at 12:00,  <mailto:markusheintz@chromium.org> wrote:
> > I'd like to rename the file pref_content_settings_provider.* in a separate
> > CL.
> >
> >
> >
>
http://codereview.chromium.org/6484035/diff/3007/chrome/browser/content_setti...
> > File chrome/browser/content_settings/pref_content_settings_provider.h
> > (right):
> >
> >
>
http://codereview.chromium.org/6484035/diff/3007/chrome/browser/content_setti...
> > chrome/browser/content_settings/pref_content_settings_provider.h:93: //
> > Content-settings-provider that provides content-settings from the user
> > On 2011/02/15 09:55:25, Bernhard Bauer wrote:
> >>
> >> Nit: Why the dashes?
> >
> > For consistence.
> > e.g. host-content-settings-maps
> > &nbsp; &nbsp; content-settings
> 
> I think it reads more natural without them (host content settings map,
> content settings, etc.).

Pam usually made me add the dashes but I'm fine with removing them. Removed :)

> 
> > &nbsp; &nbsp; ...
> >
> > http://codereview.chromium.org/6484035/
> >

[Bernhard Bauer, 2011-02-15 12:28:05]

LGTM.

http://codereview.chromium.org/6484035/diff/14004/chrome/browser/content_sett...
File chrome/browser/content_settings/pref_content_settings_provider.cc (right):

http://codereview.chromium.org/6484035/diff/14004/chrome/browser/content_sett...
chrome/browser/content_settings/pref_content_settings_provider.cc:656:
BaseProvider::ClickToPlayFixup(
Is that even necessary if we're inheriting from BaseProvider?

[markusheintz_, 2011-02-15 12:29:53]

On 2011/02/15 12:28:05, Bernhard Bauer wrote:
> LGTM.
> 
>
http://codereview.chromium.org/6484035/diff/14004/chrome/browser/content_sett...
> File chrome/browser/content_settings/pref_content_settings_provider.cc
(right):
> 
>
http://codereview.chromium.org/6484035/diff/14004/chrome/browser/content_sett...
> chrome/browser/content_settings/pref_content_settings_provider.cc:656:
> BaseProvider::ClickToPlayFixup(
> Is that even necessary if we're inheriting from BaseProvider?

Sadly yes. the PrefDefaultProvider also uses this method. But this will be fix
during the re-factoring.

[Bernhard Bauer, 2011-02-15 12:45:34]

On Tue, Feb 15, 2011 at 13:29,  <markusheintz@chromium.org> wrote:
> On 2011/02/15 12:28:05, Bernhard Bauer wrote:
>>
>> LGTM.
>
>
>
http://codereview.chromium.org/6484035/diff/14004/chrome/browser/content_sett...
>>
>> File chrome/browser/content_settings/pref_content_settings_provider.cc
>
> (right):
>
>
>
http://codereview.chromium.org/6484035/diff/14004/chrome/browser/content_sett...
>>
>> chrome/browser/content_settings/pref_content_settings_provider.cc:656:
>> BaseProvider::ClickToPlayFixup(
>> Is that even necessary if we're inheriting from BaseProvider?
>
> Sadly yes. the PrefDefaultProvider also uses this method. But this will be
> fix
> during the re-factoring.

What I meant was, do you explicitly need to call the method with the
BaseProvider:: prefix?

> http://codereview.chromium.org/6484035/
>

[markusheintz, 2011-02-15 13:25:44]

No. If it is fine to omit the prefix. Fine with me. Two places less to
touch.

Removed the BaseProvider: prefix in the PrefProvider class.

On Tue, Feb 15, 2011 at 1:32 PM, Bernhard Bauer <bauerb@chromium.org> wrote:

> On Tue, Feb 15, 2011 at 13:29,  <markusheintz@chromium.org> wrote:
> > On 2011/02/15 12:28:05, Bernhard Bauer wrote:
> >>
> >> LGTM.
> >
> >
> >
>
http://codereview.chromium.org/6484035/diff/14004/chrome/browser/content_sett...
> >>
> >> File chrome/browser/content_settings/pref_content_settings_provider.cc
> >
> > (right):
> >
> >
> >
>
http://codereview.chromium.org/6484035/diff/14004/chrome/browser/content_sett...
> >>
> >> chrome/browser/content_settings/pref_content_settings_provider.cc:656:
> >> BaseProvider::ClickToPlayFixup(
> >> Is that even necessary if we're inheriting from BaseProvider?
> >
> > Sadly yes. the PrefDefaultProvider also uses this method. But this will
> be
> > fix
> > during the re-factoring.
>
> What I meant was, do you explicitly need to call the method with the
> BaseProvider:: prefix?
>
> > http://codereview.chromium.org/6484035/
> >
>



-- 
Markus