Allow invalid patterns for comparison; Make the compare method a order relation.

Allow invalid ContentSettingsPatterns to be compared.

Modify the ContentSettingsPatterns::Compare method so that it describes an order on content settings patterns.

BUG=63656, 84644
TEST=content_settings_pattern_unittest.cc

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

[markusheintz_, 2011-06-02 18:30:04]

I extracted these changes from 7049007: Origin Identifier Value Map as they are
independent.

[Bernhard Bauer, 2011-06-02 18:39:29]

Generally LGTM, some comments and nits below:

http://codereview.chromium.org/6969095/diff/2001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pattern.cc (right):

http://codereview.chromium.org/6969095/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.cc:48: int
CompareDomainNames(const std::string& str1, const std::string& str2) {
We should think about sorting them by registry-controlled domain (like
CookieTreeModel::CanonicalizeHost does) at some point, but for now this is fine.

http://codereview.chromium.org/6969095/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.cc:422: return 
Compare(other) < 0;
Nit: superfluous space before Compare.

http://codereview.chromium.org/6969095/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.cc:422: return 
Compare(other) < 0;
I'm not completely sure if we want to show more specific patterns before less
specific ones in the UI, or the other way around. In the mocks for site-centric
content settings, we had the default setting always at the top, which is the
most general one, and it would be nice if that was just a result of the overall
ordering.

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

http://codereview.chromium.org/6969095/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.h:29: // (A compare B)
interessting relations are:
Nit: "interesting", while you're at it :-)

http://codereview.chromium.org/6969095/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.h:30: // - IDENTITY :
Nit: no space before :

[markusheintz_, 2011-06-03 15:20:36]

http://codereview.chromium.org/6969095/diff/2001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pattern.cc (right):

http://codereview.chromium.org/6969095/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.cc:48: int
CompareDomainNames(const std::string& str1, const std::string& str2) {
On 2011/06/02 18:39:29, Bernhard Bauer wrote:
> We should think about sorting them by registry-controlled domain (like
> CookieTreeModel::CanonicalizeHost does) at some point, but for now this is
fine.

Hm ... good point.

I guess I would care more about grouping for example

google.com
google.de

yahoo.com
yahoo.de

together than 

com.google
com.yahoo

de.google
de.yahoo

Pls correct me if I get the sorting wrong.

http://codereview.chromium.org/6969095/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.cc:422: return 
Compare(other) < 0;
On 2011/06/02 18:39:29, Bernhard Bauer wrote:
> Nit: superfluous space before Compare.

Done.

http://codereview.chromium.org/6969095/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.cc:422: return 
Compare(other) < 0;
On 2011/06/02 18:39:29, Bernhard Bauer wrote:
> I'm not completely sure if we want to show more specific patterns before less
> specific ones in the UI, or the other way around. In the mocks for
site-centric
> content settings, we had the default setting always at the top, which is the
> most general one, and it would be nice if that was just a result of the
overall
> ordering.

In the end it doesn't matter since you can use '>' or '<' so sort the list of
patterns in the UI.

But I guess I see what you mean.

That would be:

"*"               : ALLOW
"[*.]example.com" : BLOCK
"bar.example.com" : ALLOW
"foo.example.com" : ALLOW


I thought it might be good to have to most specific pattern on top, in order to
see what get's applied first. So you don't have to search don't the stack.

"bar.example.com" : ALLOW
"foo.example.com" : ALLOW
"[*.]example.com" : BLOCK
"*"               : ALLOW

But I guess I have not justifying reason other than my personal taste.

Or do you mean the '<' and '>' relations should be inverted?

so pattern_a < pattern_b becomes pattern_a > pattern_b

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

http://codereview.chromium.org/6969095/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.h:29: // (A compare B)
interessting relations are:
On 2011/06/02 18:39:29, Bernhard Bauer wrote:
> Nit: "interesting", while you're at it :-)

Done.

http://codereview.chromium.org/6969095/diff/2001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pattern.h:30: // - IDENTITY :
On 2011/06/02 18:39:29, Bernhard Bauer wrote:
> Nit: no space before :

Done. Also below.