Move ContentSettingsDetails and Pattern out of HostContentSettingsMap as separate classes.

Move ContentSettingsDetails and Pattern out of HostContentSettingsMap as separate classes.

BUG=64753
TEST=compiles

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

[jochen (gone - plz use gerrit), 2010-12-02 12:28:01]

please review

[gfeher, 2010-12-02 13:07:50]

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_details.h (right):

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_details.h:5: // Sends
notifications about updates to the content settings.
If I understand correctly, the notification is not actually sent from here.

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pattern.h (right):

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.h:55: static const char
*kDomainWildcard;
static const char* kDomainWildcard; ?

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/ui/views/opti...
File chrome/browser/ui/views/options/content_filter_page_view.cc (right):

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/ui/views/opti...
chrome/browser/ui/views/options/content_filter_page_view.cc:268:
NotifyContentSettingsChanged(
This actually fits into 80 chars.

[jochen (gone - plz use gerrit), 2010-12-02 13:18:43]

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_details.h (right):

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_details.h:5: // Sends
notifications about updates to the content settings.
On 2010/12/02 13:07:50, gfeher wrote:
> If I understand correctly, the notification is not actually sent from here.

Done.

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pattern.h (right):

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.h:55: static const char
*kDomainWildcard;
On 2010/12/02 13:07:50, gfeher wrote:
> static const char* kDomainWildcard; ?

Done.

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/ui/views/opti...
File chrome/browser/ui/views/options/content_filter_page_view.cc (right):

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/ui/views/opti...
chrome/browser/ui/views/options/content_filter_page_view.cc:268:
NotifyContentSettingsChanged(
On 2010/12/02 13:07:50, gfeher wrote:
> This actually fits into 80 chars.

almost :)

[markusheintz_, 2010-12-02 13:28:18]

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_details.h (right):

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_details.h:5: // Sends
notifications about updates to the content settings.
This comment doesn't seem to belong here. Pls remove this comment if that's the
case.

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_details.h:17: // than one
pattern in one user interaction, this will usually send a single
[..., this will usually send ...], pls remove "usually".

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pattern.h (right):

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.h:16: class
ContentSettingsPattern {
In order to prevent confusion or misunderstanding I think this class should be
re-named to HostnamePattern. The pattern describes host names and not content
settings.

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.h:48: // in original or
punycode form.
In which case is it the original and in which case the punycode form? I guess
that should be clear to the reader(Sorry to bug you with that comment rewriting)

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.h:54: // The format of
a domain wildcard.
Ignore this comment if the following twi constants are used outside the pattern
implementation:

If the following two constants are only used by the Pattern implementation then
they could live in a anonymous namespace in the .cc file. This we you don't
expose implementation detais.

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/ui/views/opti...
File chrome/browser/ui/views/options/content_filter_page_view.cc (right):

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/ui/views/opti...
chrome/browser/ui/views/options/content_filter_page_view.cc:257: const
ContentSettingsDetails *details) {
s/ContentSettingsDetails *details/ContentSettingsDetails* details/g

[jochen (gone - plz use gerrit), 2010-12-02 13:41:38]

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_details.h (right):

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_details.h:17: // than one
pattern in one user interaction, this will usually send a single
On 2010/12/02 13:28:18, markusheintz_ wrote:
> [..., this will usually send ...], pls remove "usually".
well, it's not always the case, i.e. when you change settings through the
content setting bubble, you might get several notifications

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pattern.h (right):

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.h:16: class
ContentSettingsPattern {
On 2010/12/02 13:28:18, markusheintz_ wrote:
> In order to prevent confusion or misunderstanding I think this class should be
> re-named to HostnamePattern. The pattern describes host names and not content
> settings.

it doesn't describe content settings, but it also doesn't describe hostnames.
IPs and URLs are valid patterns as well

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.h:48: // in original or
punycode form.
On 2010/12/02 13:28:18, markusheintz_ wrote:
> In which case is it the original and in which case the punycode form? I guess
> that should be clear to the reader(Sorry to bug you with that comment
rewriting)

Done.

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.h:48: // in original or
punycode form.
On 2010/12/02 13:28:18, markusheintz_ wrote:
> In which case is it the original and in which case the punycode form? I guess
> that should be clear to the reader(Sorry to bug you with that comment
rewriting)

Done.

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.h:54: // The format of
a domain wildcard.
On 2010/12/02 13:28:18, markusheintz_ wrote:
> Ignore this comment if the following twi constants are used outside the
pattern
> implementation:

it's still used in the host content settings map. this might change in the
future.

[markusheintz_, 2010-12-02 13:50:56]

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_details.h (right):

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_details.h:17: // than one
pattern in one user interaction, this will usually send a single
On 2010/12/02 13:41:39, jochen wrote:
> On 2010/12/02 13:28:18, markusheintz_ wrote:
> > [..., this will usually send ...], pls remove "usually".
> well, it's not always the case, i.e. when you change settings through the
> content setting bubble, you might get several notifications

But these are several separate actions/changes/requests/operation then. Right?
If you change more than one content setting in one operation than you'll always
get a signal notification. Right?

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pattern.h (right):

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.h:16: class
ContentSettingsPattern {
On 2010/12/02 13:41:39, jochen wrote:
> On 2010/12/02 13:28:18, markusheintz_ wrote:
> > In order to prevent confusion or misunderstanding I think this class should
be
> > re-named to HostnamePattern. The pattern describes host names and not
content
> > settings.
> 
> it doesn't describe content settings, but it also doesn't describe hostnames.
> IPs and URLs are valid patterns as well

I see. Pls change the comment then.
Hm... hard to find a good name for that
"OriginPattern/RequestSourcePattern/TargetPattern"?  Since I can't offer a good
alternative just keep the name if you like.

[jochen (gone - plz use gerrit), 2010-12-02 13:55:40]

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_details.h (right):

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_details.h:17: // than one
pattern in one user interaction, this will usually send a single
On 2010/12/02 13:50:56, markusheintz_ wrote:
> On 2010/12/02 13:41:39, jochen wrote:
> > On 2010/12/02 13:28:18, markusheintz_ wrote:
> > > [..., this will usually send ...], pls remove "usually".
> > well, it's not always the case, i.e. when you change settings through the
> > content setting bubble, you might get several notifications
> 
> But these are several separate actions/changes/requests/operation then. Right?
> If you change more than one content setting in one operation than you'll
always
> get a signal notification. Right?

nope, that's not part of the contract

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pattern.h (right):

http://codereview.chromium.org/5574001/diff/3001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.h:16: class
ContentSettingsPattern {
On 2010/12/02 13:50:56, markusheintz_ wrote:
> On 2010/12/02 13:41:39, jochen wrote:
> > On 2010/12/02 13:28:18, markusheintz_ wrote:
> > > In order to prevent confusion or misunderstanding I think this class
should
> be
> > > re-named to HostnamePattern. The pattern describes host names and not
> content
> > > settings.
> > 
> > it doesn't describe content settings, but it also doesn't describe
hostnames.
> > IPs and URLs are valid patterns as well
> 
> I see. Pls change the comment then.
> Hm... hard to find a good name for that
> "OriginPattern/RequestSourcePattern/TargetPattern"?  Since I can't offer a
good
> alternative just keep the name if you like.

Done.

[markusheintz_, 2010-12-02 14:12:27]

http://codereview.chromium.org/5574001/diff/21001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_details.h (right):

http://codereview.chromium.org/5574001/diff/21001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_details.h:17: // than one
pattern in one user interaction, this will usually send a single
There is a deterministic behavior in the host content settings map. Right? At
least there should be.
Whenever you call a set default setting method the signaling notification is
fired and whenever you change the content setting for one specific pattern you
include the the pattern.

If you reset to the default than also more than one pattern changes and hence
you send a signaling notification. I guess the contract should explain the
deterministic behavior on which you can count. Feel free to point me to the
right code location in case I missed something.

[jochen (gone - plz use gerrit), 2010-12-02 14:22:54]

On 2010/12/02 14:12:27, markusheintz_ wrote:
>
http://codereview.chromium.org/5574001/diff/21001/chrome/browser/content_sett...
> File chrome/browser/content_settings/content_settings_details.h (right):
> 
>
http://codereview.chromium.org/5574001/diff/21001/chrome/browser/content_sett...
> chrome/browser/content_settings/content_settings_details.h:17: // than one
> pattern in one user interaction, this will usually send a single
> There is a deterministic behavior in the host content settings map. Right? At
> least there should be.
> Whenever you call a set default setting method the signaling notification is
> fired and whenever you change the content setting for one specific pattern you
> include the the pattern.
> 
> If you reset to the default than also more than one pattern changes and hence
> you send a signaling notification. I guess the contract should explain the
> deterministic behavior on which you can count. Feel free to point me to the
> right code location in case I missed something.

The host content settings map is deterministic, but that doesn't mean that for
any single user action, there exists a single method in the host content
settings map.

If some UI code does a series of content setting updates based on a single user
action, a series of events is fired.

To provide a contract that says 1 user action = 1 notification, we'd need "start
user action" and "commit user action" methods on the host content settings map.

does that make sense?

[markusheintz_, 2010-12-02 14:34:55]

That makes all sense. I just thought that the comment is not really suggesting
when you can expect what. But anyway it gets pretty clear from a look to the
impl. :-) I think that is all good.

LGTM

[gfeher, 2010-12-02 14:35:50]

LGTM