Add a method to the content_settings::ProviderInterface to return the content settings Value.

Add a method to the content_settings::ProviderInterface to return the |Value| of a content setting instead of a |ContentSetting| type.

The HostContentSettingsMap should allow storing arbitrary |Value|s instead of only |ContentSettings| for content settings types (BTW: content settings should be called website settings). This allows to add other content settings type(website settings types) to the host content settings map that require more complex settings than ALLOW, BLOCK, ASK, ... . CONTENT_TYPE_AUTO_SELECT_CERTIFICATE is one example of such a content settings type. CONTENT_TYPE_AUTO_SELECT_CERTIFICATE requires a map with certain criteria that a certificate must match to be selected.
 
This is the first CL to migrate the content settings code to user |Value|s instead of |ContentSetting|s.


BUG=63656, 81825
TEST=host_content_settings_map_unittest.cc,
     content_settings_pref_provider.cc,
     content_settings_policy_provider.cc,
     content_settings_provider_unittest.cc

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

[markusheintz_, 2011-09-01 13:20:28]

@pam: Could you review this CL for me please?

@bauerb: This is the first step in a series of CLs to come. Feel free to comment
please.

@mnissler,
@wtc: This is FYI for you as it is also the first setop to add filters to the
CertificateAutoSelect policy.

[Bernhard Bauer, 2011-09-01 13:33:19]

http://codereview.chromium.org/7831004/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/content_settings_policy_provider.cc
(right):

http://codereview.chromium.org/7831004/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_policy_provider.cc:397:
resource_identifier);
Nit: indent

http://codereview.chromium.org/7831004/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7831004/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_pref_provider.cc:446: Value*
value = GetContentSettingValue(primary_url,
This leaks |value|.

[markusheintz_, 2011-09-01 14:32:49]

http://codereview.chromium.org/7831004/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/content_settings_policy_provider.cc
(right):

http://codereview.chromium.org/7831004/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_policy_provider.cc:397:
resource_identifier);
On 2011/09/01 13:33:19, Bernhard Bauer wrote:
> Nit: indent

Done.

http://codereview.chromium.org/7831004/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7831004/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_pref_provider.cc:446: Value*
value = GetContentSettingValue(primary_url,
On 2011/09/01 13:33:19, Bernhard Bauer wrote:
> This leaks |value|.

Fixed here and in the PolicyProvider as well.

[wtc, 2011-09-01 18:21:09]

markusheintz: my review is a "drive-by" review, for your
reference only.

Your CL looks correct to me.  I checked the correctness
of your refactoring, even though I don't understand what
the code is doing.

http://codereview.chromium.org/7831004/diff/1010/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_policy_provider.cc
(right):

http://codereview.chromium.org/7831004/diff/1010/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:417: return
value ? value->DeepCopy() : NULL;
I hope these DeepCopy() calls are not expensive.  You call
them to create the function's return value |Value| object,
only to be deleted by the caller.

Another option is to return |Value| instead of |Value*|.
Does |Value| have a copy constructor?

http://codereview.chromium.org/7831004/diff/1010/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:420: 
Nit: remove these two blank lines.

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

http://codereview.chromium.org/7831004/diff/1010/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_provider.h:82: // settings
value is available for the given parameter then NULL is returned.
Nit: perhaps "parameter" should be "parameters" (plural) because
the method has four parameters?  Or are you referring to a
particular parameter?  If so, please name it.

[Pam (message me for reviews), 2011-09-02 10:50:16]

LGTM. Please also get other reviewers' approvals.

- Pam

http://codereview.chromium.org/7831004/diff/1010/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_extension_provider.cc
(right):

http://codereview.chromium.org/7831004/diff/1010/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_extension_provider.cc:46: //
OriginIdentifierValueMap to allow arbitray |Value|s to be stored inseted of
typo "instead"

[Bernhard Bauer, 2011-09-02 14:52:54]

LGTM

[markusheintz_, 2011-09-02 15:22:20]

http://codereview.chromium.org/7831004/diff/1010/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_extension_provider.cc
(right):

http://codereview.chromium.org/7831004/diff/1010/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_extension_provider.cc:46: //
OriginIdentifierValueMap to allow arbitray |Value|s to be stored inseted of
On 2011/09/02 10:50:16, Pam wrote:
> typo "instead"

Done.

http://codereview.chromium.org/7831004/diff/1010/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_policy_provider.cc
(right):

http://codereview.chromium.org/7831004/diff/1010/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:417: return
value ? value->DeepCopy() : NULL;
On 2011/09/01 18:21:09, wtc wrote:
> I hope these DeepCopy() calls are not expensive.  You call
> them to create the function's return value |Value| object,
> only to be deleted by the caller.
>

> Another option is to return |Value| instead of |Value*|.

Using a pointer may cause synchronization issues.

> Does |Value| have a copy constructor?

Value has no copy constructor.

http://codereview.chromium.org/7831004/diff/1010/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_policy_provider.cc:420: 
On 2011/09/01 18:21:09, wtc wrote:
> Nit: remove these two blank lines.

Done.

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

http://codereview.chromium.org/7831004/diff/1010/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_provider.h:82: // settings
value is available for the given parameter then NULL is returned.
On 2011/09/01 18:21:09, wtc wrote:
> Nit: perhaps "parameter" should be "parameters" (plural) because
> the method has four parameters?  Or are you referring to a
> particular parameter?  If so, please name it.

Done.

[wtc, 2011-09-02 20:02:26]

Drive-by review: I reviewed the diffs between Patch Sets 4
and 5.  They look good to me.  Thanks.