Origin Identifier Value Map.

In memory map to store |Value| objects for 
(origin pattern, origin pattern, content type, resource) identifier) tuples.

This is supposed to replace the content_settings::BaseProvider.

BUG=63656
TEST=origin_identifier_value_map_unittest.cc,
     content_settings_pref_provider_unittest.cc,
     host_content_settings_map_unittest.cc

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

[markusheintz_, 2011-05-19 13:57:04]

Pls take a first look but don't review yet. Thanks

[jochen (gone - plz use gerrit), 2011-05-19 14:37:11]

only a few nits from my side

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

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:25:
if (first->a.Compare(second->a) == ContentSettingsPattern::PREDECESSOR)
comments should always end in dot

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:122:

only one empty line

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
File
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h
(right):

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:13:
#include "chrome/browser/content_settings/content_settings_pattern.h"
alphabetical ordering

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:30:
typedef std::list<Entry> EntryList;
move to private?

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:63:
EntryList::const_iterator HighestPrecedence(
maybe ReturnPatternWithHighestPrecedence?

function names should always be Verb+Noun

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:78:
};
DISALLOW COPY AND ASSIGN?

[Bernhard Bauer, 2011-05-19 15:05:16]

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

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:12:
typedef content_settings::OriginIdentifierValueMap::EntryList::const_iterator
The typedef doesn't need to be in an anoymous namespace, as it doesn't create a
symbol that could cause linking errors.

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:25:
if (first->a.Compare(second->a) == ContentSettingsPattern::PREDECESSOR)
Pull out the comparison result into a variable and then check whether it's
PREDECESSOR or SUCCESSOR?

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:55:
best_match = HighestPrecedence(best_match, entry);
Do you have plans to use that method somewhere else? Otherwise you can just
inline it (getting rid of the naming problem as well :-D )

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:60:
return best_match->e->DeepCopy();
As discussed offline, it's probably preferable to just return the value.

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:61:
return Value::CreateNullValue();
return an actual NULL?

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:83:
// Update or entry
Nit: "our" entry? Also, you can move this into the else clause.

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:115:
list_entry = entry;
Directly |return entry|?

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
File
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h
(right):

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:63:
EntryList::const_iterator HighestPrecedence(
On 2011/05/19 14:37:11, jochen wrote:
> maybe ReturnPatternWithHighestPrecedence?
> 
> function names should always be Verb+Noun

GetPatternWithHighestPrecedence?

[markusheintz_, 2011-05-23 19:55:09]

I will fix the TODOs and nits.

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

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:12:
typedef content_settings::OriginIdentifierValueMap::EntryList::const_iterator
On 2011/05/19 15:05:17, Bernhard Bauer wrote:
> The typedef doesn't need to be in an anoymous namespace, as it doesn't create
a
> symbol that could cause linking errors.

moved it back to the content_settings namespace.

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:25:
if (first->a.Compare(second->a) == ContentSettingsPattern::PREDECESSOR)
On 2011/05/19 14:37:11, jochen wrote:
> comments should always end in dot

Done.

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:25:
if (first->a.Compare(second->a) == ContentSettingsPattern::PREDECESSOR)
On 2011/05/19 15:05:17, Bernhard Bauer wrote:
> Pull out the comparison result into a variable and then check whether it's
> PREDECESSOR or SUCCESSOR?

Actually I'd like to add "<" and ">" operator to the content settings pattern
class. Then it would look like:

first->a > second->a

would that be fine with you?

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:55:
best_match = HighestPrecedence(best_match, entry);
On 2011/05/19 15:05:17, Bernhard Bauer wrote:
> Do you have plans to use that method somewhere else? Otherwise you can just
> inline it (getting rid of the naming problem as well :-D )

No I don't have plans to use it anywhere else. I just put it into it's own
method to improve readability. Found it more readable this way. But I don't
object in-lining. So I can inline if you like.

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:60:
return best_match->e->DeepCopy();
On 2011/05/19 15:05:17, Bernhard Bauer wrote:
> As discussed offline, it's probably preferable to just return the value.

Done.

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:61:
return Value::CreateNullValue();
On 2011/05/19 15:05:17, Bernhard Bauer wrote:
> return an actual NULL?

Cool I returned a NULL in first place anyway. Done

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:83:
// Update or entry
On 2011/05/19 15:05:17, Bernhard Bauer wrote:
> Nit: "our" entry? Also, you can move this into the else clause.

Done. Done.

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:115:
list_entry = entry;
On 2011/05/19 15:05:17, Bernhard Bauer wrote:
> Directly |return entry|?

Done.

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:122:

On 2011/05/19 14:37:11, jochen wrote:
> only one empty line

Done.

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
File
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h
(right):

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:13:
#include "chrome/browser/content_settings/content_settings_pattern.h"
On 2011/05/19 14:37:11, jochen wrote:
> alphabetical ordering

Done.

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:30:
typedef std::list<Entry> EntryList;
On 2011/05/19 14:37:11, jochen wrote:
> move to private?

Let's wait and see. I guess I'll need to expose some of these typedefs. But I
make all of them private if I don't need them.

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:63:
EntryList::const_iterator HighestPrecedence(
On 2011/05/19 14:37:11, jochen wrote:
> maybe ReturnPatternWithHighestPrecedence?
> 
> function names should always be Verb+Noun

I chose ReturnEntryWithHighestPrecedence because an Entry is returned.

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:78:
};
On 2011/05/19 14:37:11, jochen wrote:
> DISALLOW COPY AND ASSIGN?

Done.

[Bernhard Bauer, 2011-05-24 14:21:28]

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

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:25:
if (first->a.Compare(second->a) == ContentSettingsPattern::PREDECESSOR)
On 2011/05/23 19:55:11, markusheintz_ wrote:
> On 2011/05/19 15:05:17, Bernhard Bauer wrote:
> > Pull out the comparison result into a variable and then check whether it's
> > PREDECESSOR or SUCCESSOR?
> 
> Actually I'd like to add "<" and ">" operator to the content settings pattern
> class. Then it would look like:
> 
> first->a > second->a
> 
> would that be fine with you?

Yes, if you inline it below (so it just becomes:)

if (best_match == entries.end() || best_match < entry)
  best_match = entry;

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
File
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h
(right):

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:63:
EntryList::const_iterator HighestPrecedence(
On 2011/05/23 19:55:11, markusheintz_ wrote:
> On 2011/05/19 14:37:11, jochen wrote:
> > maybe ReturnPatternWithHighestPrecedence?
> > 
> > function names should always be Verb+Noun
> 
> I chose ReturnEntryWithHighestPrecedence because an Entry is returned.
> 

ok, but we usually don't put "return" into method names (every method which
doesn't have return type void returns something).

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_base_provider.h (right):

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_base_provider.h:45: static bool
RequiresResourceIdentifier(
I think that method might be useful to clients of the HostContentSettingsMap as
well, so maybe you could move it there?

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
File
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc
(right):

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:13:
typedef OriginIdentifierValueMap::EntryList::const_iterator ConstEntryIterator;
This is already defined in the header?

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:78:
list_entry = entries_.insert(list_entry, MakeTuple(item_pattern,
aaaand the assignment to list_entry is not necessary anymore.

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
File
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h
(right):

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:20:
class OriginIdentifierValueMap {
Nit: I think "OriginValueMap" would roll easier off the tongue. The identifier
is just an additional thingy which is not used that much anyway.

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:36:
EntryList::iterator Begin() {
use lower case?

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:93:
ContentSetting ValueToContentSetting(Value * value) {
Nit: no space between |Value| and |*|

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:94:
DCHECK(value);
Nit: I think that DCHECK is kind of unnecessary (GetAsInteger is virtual, so it
would crash anyway below).

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:405: // For a
non incognito provider this will alwasy return NULL.
Nit: "always"

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:405: // For a
non incognito provider this will alwasy return NULL.
How does this work? Do we have separate providers for incognito and regular
preferences? Do they share value maps?

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:411: if
(incognito_value != NULL)
Nit: just |if (incognito_value)|?

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:600: // TODO:
What happens if no resource identifier is required?
Nit: TODO(markusheintz) ;-)

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:622: // No
action required move to next value map entry.
Nit: semicolon between "No action required" and "move to next value map entry"?

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.h (right):

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.h:185: // Used
around accesses to the content_settings_ object to guarantee
Nit: "around accesses to the value maps" or something.

[markusheintz_, 2011-05-26 13:22:12]

Here we go. CL ready for review.

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

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:25:
if (first->a.Compare(second->a) == ContentSettingsPattern::PREDECESSOR)
On 2011/05/24 14:21:29, Bernhard Bauer wrote:
> On 2011/05/23 19:55:11, markusheintz_ wrote:
> > On 2011/05/19 15:05:17, Bernhard Bauer wrote:
> > > Pull out the comparison result into a variable and then check whether it's
> > > PREDECESSOR or SUCCESSOR?
> > 
> > Actually I'd like to add "<" and ">" operator to the content settings
pattern
> > class. Then it would look like:
> > 
> > first->a > second->a
> > 
> > would that be fine with you?
> 
> Yes, if you inline it below (so it just becomes:)
> 
> if (best_match == entries.end() || best_match < entry)
>   best_match = entry;

best_match < entry requires to add a < operator for the
OriginIdentifierValueMap::Entry(tuple). So I could do the following:


namespace {
bool operator<(OriginIdentifierValueMap::const_iterator first,
               OriginIdentifierValueMap::const_iterator second) {
...
}
}

That would replace the GetEntryWithHighest....

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
File
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h
(right):

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:63:
EntryList::const_iterator HighestPrecedence(
On 2011/05/24 14:21:29, Bernhard Bauer wrote:
> On 2011/05/23 19:55:11, markusheintz_ wrote:
> > On 2011/05/19 14:37:11, jochen wrote:
> > > maybe ReturnPatternWithHighestPrecedence?
> > > 
> > > function names should always be Verb+Noun
> > 
> > I chose ReturnEntryWithHighestPrecedence because an Entry is returned.
> > 
> 
> ok, but we usually don't put "return" into method names (every method which
> doesn't have return type void returns something).

Return -> Get

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_base_provider.h (right):

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_base_provider.h:45: static bool
RequiresResourceIdentifier(
On 2011/05/24 14:21:29, Bernhard Bauer wrote:
> I think that method might be useful to clients of the HostContentSettingsMap
as
> well, so maybe you could move it there?

Strike :) Already committed in a separate CL :)

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
File
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc
(right):

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:13:
typedef OriginIdentifierValueMap::EntryList::const_iterator ConstEntryIterator;
On 2011/05/24 14:21:29, Bernhard Bauer wrote:
> This is already defined in the header?

Done.

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:78:
list_entry = entries_.insert(list_entry, MakeTuple(item_pattern,
On 2011/05/24 14:21:29, Bernhard Bauer wrote:
> aaaand the assignment to list_entry is not necessary anymore.

Done

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
File
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h
(right):

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:20:
class OriginIdentifierValueMap {
On 2011/05/24 14:21:29, Bernhard Bauer wrote:
> Nit: I think "OriginValueMap" would roll easier off the tongue. The identifier
> is just an additional thingy which is not used that much anyway.

I'd like to emphasis that there is a second key; which is actually the content
settings type. But I'd like to combine the type and identifier one day to one
single identifier.

So actually I should name it |OriginPatternContentSettingsTypeValueMap| but that
name sucks :) OriginValueMap is cool but missing that you need  ans additional
key to get-set a value. But since he have two patterns anyway I might rename it.
Don't know yet whether I really will.

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:36:
EntryList::iterator Begin() {
On 2011/05/24 14:21:29, Bernhard Bauer wrote:
> use lower case?

Done. Also change end and the names used in the typedefs.

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:93:
ContentSetting ValueToContentSetting(Value * value) {
On 2011/05/24 14:21:29, Bernhard Bauer wrote:
> Nit: no space between |Value| and |*|

Done.

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:94:
DCHECK(value);
On 2011/05/24 14:21:29, Bernhard Bauer wrote:
> Nit: I think that DCHECK is kind of unnecessary (GetAsInteger is virtual, so
it
> would crash anyway below).

Done.

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:405: // For a
non incognito provider this will alwasy return NULL.
On 2011/05/24 14:21:29, Bernhard Bauer wrote:
> Nit: "always"

Done.

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:405: // For a
non incognito provider this will alwasy return NULL.
On 2011/05/24 14:21:29, Bernhard Bauer wrote:
> How does this work? Do we have separate providers for incognito and regular
> preferences? Do they share value map

Sorry that comment was garbage. Fixed

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:411: if
(incognito_value != NULL)
On 2011/05/24 14:21:29, Bernhard Bauer wrote:
> Nit: just |if (incognito_value)|?

Done. Also for the if (value ... below

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:600: // TODO:
What happens if no resource identifier is required?
On 2011/05/24 14:21:29, Bernhard Bauer wrote:
> Nit: TODO(markusheintz) ;-)

Removed.

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:622: // No
action required move to next value map entry.
On 2011/05/24 14:21:29, Bernhard Bauer wrote:
> Nit: semicolon between "No action required" and "move to next value map
entry"?

Done.

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.h (right):

http://codereview.chromium.org/7049007/diff/8002/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.h:185: // Used
around accesses to the content_settings_ object to guarantee
On 2011/05/24 14:21:29, Bernhard Bauer wrote:
> Nit: "around accesses to the value maps" or something.

Done.

[Bernhard Bauer, 2011-05-26 23:14:42]

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

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:25:
if (first->a.Compare(second->a) == ContentSettingsPattern::PREDECESSOR)
On 2011/05/26 13:22:13, markusheintz_ wrote:
> On 2011/05/24 14:21:29, Bernhard Bauer wrote:
> > On 2011/05/23 19:55:11, markusheintz_ wrote:
> > > On 2011/05/19 15:05:17, Bernhard Bauer wrote:
> > > > Pull out the comparison result into a variable and then check whether
it's
> > > > PREDECESSOR or SUCCESSOR?
> > > 
> > > Actually I'd like to add "<" and ">" operator to the content settings
> pattern
> > > class. Then it would look like:
> > > 
> > > first->a > second->a
> > > 
> > > would that be fine with you?
> > 
> > Yes, if you inline it below (so it just becomes:)
> > 
> > if (best_match == entries.end() || best_match < entry)
> >   best_match = entry;
> 
> best_match < entry requires to add a < operator for the
> OriginIdentifierValueMap::Entry(tuple). So I could do the following:
> 
> 
> namespace {
> bool operator<(OriginIdentifierValueMap::const_iterator first,
>                OriginIdentifierValueMap::const_iterator second) {
> ...
> }
> }
> 
> That would replace the GetEntryWithHighest....

Yes. Ideally you could use ConstEntryIterator to make the signature less ugly.

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
File
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h
(right):

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:28:
Value*> Entry;
If you typedef the Entry as Tuple, you might just as well define a custom struct
to make it easier to identify what entry->a means.

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:61:
// no value is stored for the passed parameter Null is returned.
Nit: |NULL|?

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

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.h:125: bool operator <
(const ContentSettingsPattern& other) const {
Nit: no space around the less-than-sign.

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:30: 
Nit: Unnecessary newline?

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:115: //
TODO(markusheintz): There should be an UNKNOWN ENUM item
There is CONTENT_SETTINGS_TYPE_DEFAULT.

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:471:
is_incognito_ ? &incognito_value_map_ : &value_map_;
Does this method return only incognito rules when |is_incognito_| is true?

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:898:
LOG(WARNING) << "Skip settings for invalid content settings type '"
You can use LOG(DFATAL) to die in a debug build but print an error message in a
release build.

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.h (right):

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.h:111: //
ProviderInterface implementations.
No, this one's not. It should be non-virtual and private.

[markusheintz_, 2011-05-31 11:46:41]

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

http://codereview.chromium.org/7049007/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:25:
if (first->a.Compare(second->a) == ContentSettingsPattern::PREDECESSOR)
On 2011/05/26 23:14:42, Bernhard Bauer wrote:
> On 2011/05/26 13:22:13, markusheintz_ wrote:
> > On 2011/05/24 14:21:29, Bernhard Bauer wrote:
> > > On 2011/05/23 19:55:11, markusheintz_ wrote:
> > > > On 2011/05/19 15:05:17, Bernhard Bauer wrote:
> > > > > Pull out the comparison result into a variable and then check whether
> it's
> > > > > PREDECESSOR or SUCCESSOR?
> > > > 
> > > > Actually I'd like to add "<" and ">" operator to the content settings
> > pattern
> > > > class. Then it would look like:
> > > > 
> > > > first->a > second->a
> > > > 
> > > > would that be fine with you?
> > > 
> > > Yes, if you inline it below (so it just becomes:)
> > > 
> > > if (best_match == entries.end() || best_match < entry)
> > >   best_match = entry;
> > 
> > best_match < entry requires to add a < operator for the
> > OriginIdentifierValueMap::Entry(tuple). So I could do the following:
> > 
> > 
> > namespace {
> > bool operator<(OriginIdentifierValueMap::const_iterator first,
> >                OriginIdentifierValueMap::const_iterator second) {
> > ...
> > }
> > }
> > 
> > That would replace the GetEntryWithHighest....
> 
> Yes. Ideally you could use ConstEntryIterator to make the signature less ugly.

I used Entry instead of the ConstEntryIterator (does not exist anymore I renamed
it to const_iterator).

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
File
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h
(right):

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:28:
Value*> Entry;
On 2011/05/26 23:14:42, Bernhard Bauer wrote:
> If you typedef the Entry as Tuple, you might just as well define a custom
struct
> to make it easier to identify what entry->a means.

Changed to a struct.

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:61:
// no value is stored for the passed parameter Null is returned.
On 2011/05/26 23:14:42, Bernhard Bauer wrote:
> Nit: |NULL|?

Done.

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

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.h:125: bool operator <
(const ContentSettingsPattern& other) const {
On 2011/05/26 23:14:42, Bernhard Bauer wrote:
> Nit: no space around the less-than-sign.

Done. Same below

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:30: 
On 2011/05/26 23:14:42, Bernhard Bauer wrote:
> Nit: Unnecessary newline?

Done.

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:115: //
TODO(markusheintz): There should be an UNKNOWN ENUM item
On 2011/05/26 23:14:42, Bernhard Bauer wrote:
> There is CONTENT_SETTINGS_TYPE_DEFAULT.

I changed to CONTENT_SETTINGS_TYPE_DEFAULT but I don't like the name. (That's
why I didn't use it in first place)

Do you mind renaming CONTENT_SETTINGS_TYPE_DEFAULT to
CONTENT_SETTINGS_TYPE_UNKNOWN as this is how it is usually used?

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:471:
is_incognito_ ? &incognito_value_map_ : &value_map_;
On 2011/05/26 23:14:42, Bernhard Bauer wrote:
> Does this method return only incognito rules when |is_incognito_| is true?

Yes.

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:898:
LOG(WARNING) << "Skip settings for invalid content settings type '"
On 2011/05/26 23:14:42, Bernhard Bauer wrote:
> You can use LOG(DFATAL) to die in a debug build but print an error message in
a
> release build.

Done.

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.h (right):

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.h:111: //
ProviderInterface implementations.
On 2011/05/26 23:14:42, Bernhard Bauer wrote:
> No, this one's not. It should be non-virtual and private.

Thanks for catching this. This used to override the Init of the BaseProvider.

[Bernhard Bauer, 2011-05-31 14:55:11]

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:115: //
TODO(markusheintz): There should be an UNKNOWN ENUM item
On 2011/05/31 11:46:41, markusheintz_ wrote:
> On 2011/05/26 23:14:42, Bernhard Bauer wrote:
> > There is CONTENT_SETTINGS_TYPE_DEFAULT.
> 
> I changed to CONTENT_SETTINGS_TYPE_DEFAULT but I don't like the name. (That's
> why I didn't use it in first place)
> 
> Do you mind renaming CONTENT_SETTINGS_TYPE_DEFAULT to
> CONTENT_SETTINGS_TYPE_UNKNOWN as this is how it is usually used?

Sure, good idea.

http://codereview.chromium.org/7049007/diff/28001/chrome/browser/content_sett...
File
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc
(right):

http://codereview.chromium.org/7049007/diff/28001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:109:
return entries_.erase(entry);
Does this leak the value?

If it's not supposed to, you can call this method from the other |DeleteValue|
above.

http://codereview.chromium.org/7049007/diff/28001/chrome/browser/ui/cocoa/tab...
File chrome/browser/ui/cocoa/table_model_array_controller_unittest.mm (right):

http://codereview.chromium.org/7049007/diff/28001/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/table_model_array_controller_unittest.mm:112: @"   
\"[*.]example.com\"\n"
So we now return the content settings rules in the order in which they were
added? That seems like it could get unclear pretty fast (and regress from the
current behavior). I think we're going to have to sort the rules before
returning them from the HostContentSettingsMap.

[markusheintz_, 2011-06-01 20:08:22]

I'll break this CL into 3 smaller CLs. But maybe you could have a quick look.

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7049007/diff/5004/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:115: //
TODO(markusheintz): There should be an UNKNOWN ENUM item
On 2011/05/31 14:55:11, Bernhard Bauer wrote:
> On 2011/05/31 11:46:41, markusheintz_ wrote:
> > On 2011/05/26 23:14:42, Bernhard Bauer wrote:
> > > There is CONTENT_SETTINGS_TYPE_DEFAULT.
> > 
> > I changed to CONTENT_SETTINGS_TYPE_DEFAULT but I don't like the name.
(That's
> > why I didn't use it in first place)
> > 
> > Do you mind renaming CONTENT_SETTINGS_TYPE_DEFAULT to
> > CONTENT_SETTINGS_TYPE_UNKNOWN as this is how it is usually used?
> 
> Sure, good idea.

I'll create a separate CL

http://codereview.chromium.org/7049007/diff/28001/chrome/browser/content_sett...
File
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc
(right):

http://codereview.chromium.org/7049007/diff/28001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:109:
return entries_.erase(entry);
On 2011/05/31 14:55:12, Bernhard Bauer wrote:
> Does this leak the value?
> 
> If it's not supposed to, you can call this method from the other |DeleteValue|
> above.

I knew this would bit me in the ... .Thanks for catching. I'm using a
linked_ptr<Value> now  in the entry. I guess that should help.

http://codereview.chromium.org/7049007/diff/28001/chrome/browser/ui/cocoa/tab...
File chrome/browser/ui/cocoa/table_model_array_controller_unittest.mm (right):

http://codereview.chromium.org/7049007/diff/28001/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/table_model_array_controller_unittest.mm:112: @"   
\"[*.]example.com\"\n"
On 2011/05/31 14:55:12, Bernhard Bauer wrote:
> So we now return the content settings rules in the order in which they were
> added? That seems like it could get unclear pretty fast (and regress from the
> current behavior). I think we're going to have to sort the rules before
> returning them from the HostContentSettingsMap.

You caught me being lazy in this CL. But I agree.
I extended the ContentSettingsPattern::Compare method and the <, > operator so
that I'm able to sort patterns.

content_settings::PrefProvider::GetAllContent... returns now a sorted list of
entries.

[Bernhard Bauer, 2011-06-01 20:18:25]

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_pattern.cc (right):

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pattern.cc:48: int
CompareDomainNames(const std::string& str1, const std::string& str2) {
How did we sort the patterns before? How does chrome://settings/cookies do it?
It seems they do something like sort by effective TLD, which would be nice in
this case as well, I guess.

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pattern.cc:388: return
DISJOINT_ORDER_POST;
If we're being totally exact, it should be SUCCESSOR, as a pattern which matches
the empty set is a subset of every other pattern?

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pattern.cc:422: return 
Compare(other) < 0 ;
Nit: superfluous spaces.

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_pattern.h (right):

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pattern.h:33: // -
DISJOINT_DISPLAY_PRE :
It's called _ORDER_ below.

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pattern.h:137: bool
operator!=(const ContentSettingsPattern& other) const;
That seems like it could cause very subtle problems, as !IDENTICAL isn't the
same as disjoint.

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:461: Rules*
content_setting_rules) const {
If the ordering of rules isn't up to the provider, maybe we could have this
method return a set of rules instead of a list, using the comparison operator to
keep it sorted?

If that's too much for this CL, you could still first construct a set, then
iterate over its elements in order and construct the list from it (or sort the
list at the end), to get O(n log n) performance instead of O(n^2).

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:472: entry != 
map_to_return->end();
Nit: one space too many after !=

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:478: Rule
new_rule(entry->item_pattern, entry->top_level_frame_pattern, setting);
Nit: 80 cols?

[markusheintz_, 2011-06-01 20:55:26]

How about handling the ordering in a separate CL. I need it anyway for the UI.

So I could land the original Changes.

I have another CL in the pipeline for migrating single pattern prefs to two
pattern prefs. 

Next would be the CL the gets the order of the patterns for the UI right.

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_pattern.cc (right):

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pattern.cc:48: int
CompareDomainNames(const std::string& str1, const std::string& str2) {
On 2011/06/01 20:18:25, Bernhard Bauer wrote:
> How did we sort the patterns before? How does chrome://settings/cookies do it?
> It seems they do something like sort by effective TLD, which would be nice in
> this case as well, I guess.

I'm sorting it:

1) per domain starting with top level domains

de.google.www 
com.google.foo.bar
com.google.mail
com.google
com.youtube
org.chromium.www

2) port - lexicographic

3) scheme - lexicographic

In case wildcards are used more specific patterns go first.

com.google      , 80 , *
com.google      ,  *   , http
com.google[*.] ,  *   , *

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pattern.cc:388: return
DISJOINT_ORDER_POST;
On 2011/06/01 20:18:25, Bernhard Bauer wrote:
> If we're being totally exact, it should be SUCCESSOR, as a pattern which
matches
> the empty set is a subset of every other pattern? That's why I use DISJOINt
here.

But the intersection of the empty set with any other set it empty.

I use SUCCESSOR and PREDECESSOR only for two sets with a non empty intersection.

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:461: Rules*
content_setting_rules) const {
On 2011/06/01 20:18:25, Bernhard Bauer wrote:
> If the ordering of rules isn't up to the provider, maybe we could have this
> method return a set of rules instead of a list, using the comparison operator
to
> keep it sorted?
> 
> If that's too much for this CL, you could still first construct a set, then
> iterate over its elements in order and construct the list from it (or sort the
> list at the end), to get O(n log n) performance instead of O(n^2).

I guess I should have a separate CL for this. I need to revisit his anyway when
I work on the UI.

I didn't care about performance since the number of entries is low.

Would you be fine with me getting the order of the entries right in a different
CL?

Then I could land this one and move and the following CL that migrates single
pattern prefs to two pattern prefs. Then I could get the ordering and the UI
right.

[Bernhard Bauer, 2011-06-02 00:03:31]

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_pattern.cc (right):

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pattern.cc:48: int
CompareDomainNames(const std::string& str1, const std::string& str2) {
On 2011/06/01 20:55:26, markusheintz_ wrote:
> On 2011/06/01 20:18:25, Bernhard Bauer wrote:
> > How did we sort the patterns before? How does chrome://settings/cookies do
it?
> > It seems they do something like sort by effective TLD, which would be nice
in
> > this case as well, I guess.
> 
> I'm sorting it:
> 
> 1) per domain starting with top level domains
> 
> de.google.www 
> com.google.foo.bar
> com.google.mail
> com.google
> com.youtube
> org.chromium.www

Wouldn't it be sorted like this with your CL:

 com.google
 com.google.foo.bar
 com.google.mail
 com.youtube
 de.google.www
 org.chromium.www

?

In any case, I think we should not change the outward behavior in this CL (and
update the test expectations), only to change it back later. Bonus points (read:
not required for this CL) if we document the sorting order with a unit test (in
the current test it's just an artifact).

The current outward behavior is just lexicographically sorting the pattern
strings. I'm not really worried about what we do for scheme or port wildcards,
as that's not yet widely used anyway, and sorting non-disjoint patterns together
also makes sense (although one might argue that we want to show the most general
pattern first, as it's the default). If we want to change the sorting behavior
further than that, we should do it in a standalone CL, which would then also
update the test expectations.

> 2) port - lexicographic
> 
> 3) scheme - lexicographic
> 
> In case wildcards are used more specific patterns go first.
> 
> com.google      , 80 , *
> com.google      ,  *   , http
> com.google[*.] ,  *   , *
> 
>

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pattern.cc:388: return
DISJOINT_ORDER_POST;
On 2011/06/01 20:55:26, markusheintz_ wrote:
> On 2011/06/01 20:18:25, Bernhard Bauer wrote:
> > If we're being totally exact, it should be SUCCESSOR, as a pattern which
> matches
> > the empty set is a subset of every other pattern? That's why I use DISJOINt
> here.
> 
> But the intersection of the empty set with any other set it empty.
> 
> I use SUCCESSOR and PREDECESSOR only for two sets with a non empty
intersection.
> 

My reasoning here is that the empty set is "more specific" than any pattern, but
I guess it doesn't really matter as they both result in the same total ordering
in the UI.

[markusheintz_, 2011-06-03 13:08:53]

This CL is now independent of the changes to the ContentSettingsPattern::Compare
method.

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_pattern.cc (right):

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pattern.cc:48: int
CompareDomainNames(const std::string& str1, const std::string& str2) {
On 2011/06/02 00:03:31, Bernhard Bauer wrote:
> On 2011/06/01 20:55:26, markusheintz_ wrote:
> > On 2011/06/01 20:18:25, Bernhard Bauer wrote:
> > > How did we sort the patterns before? How does chrome://settings/cookies do
> it?
> > > It seems they do something like sort by effective TLD, which would be nice
> in
> > > this case as well, I guess.
> > 
> > I'm sorting it:
> > 
> > 1) per domain starting with top level domains
> > 
> > de.google.www 
> > com.google.foo.bar
> > com.google.mail
> > com.google
> > com.youtube
> > org.chromium.www
> 
> Wouldn't it be sorted like this with your CL:
> 
>  com.google
>  com.google.foo.bar
>  com.google.mail
>  com.youtube
>  de.google.www
>  org.chromium.www
> 
> ?
> 
Sorry you are right of course. I guess I need practice the ABC a little bit.

> In any case, I think we should not change the outward behavior in this CL (and
> update the test expectations), only to change it back later. Bonus points
(read:
> not required for this CL) if we document the sorting order with a unit test
(in
> the current test it's just an artifact).
> 

This CL should not change any outward behavior now.

> The current outward behavior is just lexicographically sorting the pattern
> strings. I'm not really worried about what we do for scheme or port wildcards,
> as that's not yet widely used anyway, and sorting non-disjoint patterns
together
> also makes sense (although one might argue that we want to show the most
general
> pattern first, as it's the default). If we want to change the sorting behavior
> further than that, we should do it in a standalone CL, which would then also
> update the test expectations.

I forked another CL for these changes. So we can continue the discussion there.

> 
> > 2) port - lexicographic
> > 
> > 3) scheme - lexicographic
> > 
> > In case wildcards are used more specific patterns go first.
> > 
> > com.google      , 80 , *
> > com.google      ,  *   , http
> > com.google[*.] ,  *   , *
> > 
> > 
>

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pattern.cc:388: return
DISJOINT_ORDER_POST;
On 2011/06/02 00:03:31, Bernhard Bauer wrote:
> On 2011/06/01 20:55:26, markusheintz_ wrote:
> > On 2011/06/01 20:18:25, Bernhard Bauer wrote:
> > > If we're being totally exact, it should be SUCCESSOR, as a pattern which
> > matches
> > > the empty set is a subset of every other pattern? That's why I use
DISJOINt
> > here.
> > 
> > But the intersection of the empty set with any other set it empty.
> > 
> > I use SUCCESSOR and PREDECESSOR only for two sets with a non empty
> intersection.
> > 
> 
> My reasoning here is that the empty set is "more specific" than any pattern,
but
> I guess it doesn't really matter as they both result in the same total
ordering
> in the UI.

Ok.

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pattern.cc:422: return 
Compare(other) < 0 ;
On 2011/06/01 20:18:25, Bernhard Bauer wrote:
> Nit: superfluous spaces.

Done in forked CL

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_pattern.h (right):

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pattern.h:33: // -
DISJOINT_DISPLAY_PRE :
On 2011/06/01 20:18:25, Bernhard Bauer wrote:
> It's called _ORDER_ below.

Done. In forked CL

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pattern.h:137: bool
operator!=(const ContentSettingsPattern& other) const;
On 2011/06/01 20:18:25, Bernhard Bauer wrote:
> That seems like it could cause very subtle problems, as !IDENTICAL isn't the
> same as disjoint.

I also have a bad feeling about his. So I removed it. See forked CL

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:461: Rules*
content_setting_rules) const {
On 2011/06/01 20:18:25, Bernhard Bauer wrote:
> If the ordering of rules isn't up to the provider, maybe we could have this
> method return a set of rules instead of a list, using the comparison operator
to
> keep it sorted?
> 
> If that's too much for this CL, you could still first construct a set, then
> iterate over its elements in order and construct the list from it (or sort the
> list at the end), to get O(n log n) performance instead of O(n^2).

I removed the sorting here and moved to the
host_content_settings_map.cc::GetContentSettingsForType

I followed your suggestion with the variation that I use a std::map instead of a
std::set. Pls check there.

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:472: entry != 
map_to_return->end();
On 2011/06/01 20:18:25, Bernhard Bauer wrote:
> Nit: one space too many after !=

Done.

http://codereview.chromium.org/7049007/diff/35001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:478: Rule
new_rule(entry->item_pattern, entry->top_level_frame_pattern, setting);
On 2011/06/01 20:18:25, Bernhard Bauer wrote:
> Nit: 80 cols?

Done.

http://codereview.chromium.org/7049007/diff/44001/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7049007/diff/44001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.cc:266:
std::map<std::string, PatternSettingPair> tmp_map;
I moved the sorting from the PrefProvider to his place here.

Instead of using a std::set I use a std::map. That should work similar to the
set.

[Bernhard Bauer, 2011-06-03 14:50:23]

LGTM.

http://codereview.chromium.org/7049007/diff/48001/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7049007/diff/48001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.cc:285: for
(std::map<std::string, PatternSettingPair>::iterator i(tmp_map.begin());
Longer-term it might make sense to group the rules by provider and only sort
among themselves, to make the precedence more clear, but this'll do for now.

[markusheintz_, 2011-06-03 15:29:31]

THX

http://codereview.chromium.org/7049007/diff/48001/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7049007/diff/48001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.cc:285: for
(std::map<std::string, PatternSettingPair>::iterator i(tmp_map.begin());
On 2011/06/03 14:50:23, Bernhard Bauer wrote:
> Longer-term it might make sense to group the rules by provider and only sort
> among themselves, to make the precedence more clear, but this'll do for now.

FYI: Hey I'm sorry I actually wanted to rename that before uploading. Done now.

[Bernhard Bauer, 2011-06-03 16:36:54]

http://codereview.chromium.org/7049007/diff/48001/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7049007/diff/48001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.cc:285: for
(std::map<std::string, PatternSettingPair>::iterator i(tmp_map.begin());
On 2011/06/03 15:29:31, markusheintz_ wrote:
> On 2011/06/03 14:50:23, Bernhard Bauer wrote:
> > Longer-term it might make sense to group the rules by provider and only sort
> > among themselves, to make the precedence more clear, but this'll do for now.
> 
> FYI: Hey I'm sorry I actually wanted to rename that before uploading. Done
now.

Ok, doesn't change the sorting behavior though ;-)

[markusheintz_, 2011-06-08 15:30:50]

Hey Bernhard

maybe you want to take a look at the last to uploads and check if you LGTM still
holds.

Thanks

[Bernhard Bauer, 2011-06-08 15:50:15]

LGTM still holds.

[Bernhard Bauer, 2011-06-14 11:50:57]

Whoops, seems I missed a couple of things.

http://codereview.chromium.org/7049007/diff/50005/chrome/browser/content_sett...
File
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc
(right):

http://codereview.chromium.org/7049007/diff/50005/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:16:
Clear();
Unnecessary now.

http://codereview.chromium.org/7049007/diff/50005/chrome/browser/content_sett...
File
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h
(right):

http://codereview.chromium.org/7049007/diff/50005/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:104:
EntryList::iterator DeleteValue(EntryList::iterator entry);
This method overloads |DeleteValue| and seems to be unnecessary anyway.

http://codereview.chromium.org/7049007/diff/50005/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:107:
void Clear();
This should be unnecessary now.

http://codereview.chromium.org/7049007/diff/50005/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/7049007/diff/50005/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:765:
'browser/content_settings/content_settings_origin_identifier_value.h',
This should read "...value_map.h".

[markusheintz_, 2011-06-14 12:24:33]

Created CL:

http://codereview.chromium.org/7149017

to fix these issues.

http://codereview.chromium.org/7049007/diff/50005/chrome/browser/content_sett...
File
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc
(right):

http://codereview.chromium.org/7049007/diff/50005/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.cc:16:
Clear();
On 2011/06/14 11:50:57, Bernhard Bauer wrote:
> Unnecessary now.

Removed

http://codereview.chromium.org/7049007/diff/50005/chrome/browser/content_sett...
File
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h
(right):

http://codereview.chromium.org/7049007/diff/50005/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:104:
EntryList::iterator DeleteValue(EntryList::iterator entry);
On 2011/06/14 11:50:57, Bernhard Bauer wrote:
> This method overloads |DeleteValue| and seems to be unnecessary anyway.

Will remove.

http://codereview.chromium.org/7049007/diff/50005/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:107:
void Clear();
On 2011/06/14 11:50:57, Bernhard Bauer wrote:
> This should be unnecessary now.

I think that's still necessary. I use it in the PolicyProvider.

http://codereview.chromium.org/7049007/diff/50005/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/7049007/diff/50005/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:765:
'browser/content_settings/content_settings_origin_identifier_value.h',
On 2011/06/14 11:50:57, Bernhard Bauer wrote:
> This should read "...value_map.h".

Uuuuuuuuups . DONE.

[Bernhard Bauer, 2011-06-14 12:34:27]

On 2011/06/14 12:24:33, markusheintz_ wrote:
> Created CL:
> 
> http://codereview.chromium.org/7149017
> 
> to fix these issues.

Thanks!

http://codereview.chromium.org/7049007/diff/50005/chrome/browser/content_sett...
File
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h
(right):

http://codereview.chromium.org/7049007/diff/50005/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_origin_identifier_value_map.h:107:
void Clear();
On 2011/06/14 12:24:33, markusheintz_ wrote:
> On 2011/06/14 11:50:57, Bernhard Bauer wrote:
> > This should be unnecessary now.
> 
> I think that's still necessary. I use it in the PolicyProvider.

Oh, right. You could rename it to lowercase to make it more analog to
|begin|/|end|/|size|/etc., but it's up to you.