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

chrome/browser/content_settings/content_settings_pattern.cc

Index: chrome/browser/content_settings/content_settings_pattern.cc
diff --git a/chrome/browser/content_settings/content_settings_pattern.cc b/chrome/browser/content_settings/content_settings_pattern.cc
index ee155646ec3ef9dc9e6dca79ff1d8c2e673a8521..4e3e943e0a6be4c2abffa8d5b53d5823fc2d89f3 100644
--- a/chrome/browser/content_settings/content_settings_pattern.cc
+++ b/chrome/browser/content_settings/content_settings_pattern.cc
@@ -4,6 +4,9 @@
 
 #include "chrome/browser/content_settings/content_settings_pattern.h"
 
+#include <vector>
+
+#include "base/string_split.h"
 #include "base/string_util.h"
 #include "base/scoped_ptr.h"
 #include "chrome/browser/content_settings/content_settings_pattern_parser.h"
@@ -41,6 +44,37 @@ bool IsSubDomainOrEqual(const std::string sub_domain,
   return true;
 }
 
+// Compares two domain names.
+int CompareDomainNames(const std::string& str1, const std::string& str2) {

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

We should think about sorting them by registry-controlled domain (like
CookieTreeModel::CanonicalizeHost does) at some point, but for now this is fine.

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

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.

+  std::vector<std::string> domain_name1;
+  std::vector<std::string> domain_name2;
+
+  base::SplitString(str1, '.', &domain_name1);
+  base::SplitString(str2, '.', &domain_name2);
+
+  int i1 = domain_name1.size() - 1;
+  int i2 = domain_name2.size() - 1;
+  int rv;
+  while (i1 >= 0 && i2 >= 0) {
+    // domain names are stored in puny code. So it's fine to use the compare
+    // method.
+    rv = domain_name1[i1].compare(domain_name2[i2]);
+    if (rv != 0)
+      return rv;
+    --i1;
+    --i2;
+  }
+
+  if (i1 > i2)
+    return 1;
+
+  if (i1 < i2)
+    return -1;
+
+  // The domain names are identical.
+  return 0;
+}
+
 typedef ContentSettingsPattern::BuilderInterface BuilderInterface;
 
 }  // namespace
@@ -343,24 +377,33 @@ const std::string ContentSettingsPattern::ToString() const {
 
 ContentSettingsPattern::Relation ContentSettingsPattern::Compare(
     const ContentSettingsPattern& other) const {
-  if (this == &other)
+  // Two invalid patterns are identical in the way they behave. They don't match
+  // anything and are represented as an empty string. So it's fair to treat them
+  // as identical.
+  if ((this == &other) ||
+      (!is_valid_ && !other.is_valid_))
     return IDENTITY;
 
-  if (!is_valid_ || !other.is_valid_) {
-    NOTREACHED();
-    return DISJOINT;
-  }
+  if (!is_valid_ && other.is_valid_)
+    return DISJOINT_ORDER_POST;
+  if (is_valid_ && !other.is_valid_)
+    return DISJOINT_ORDER_PRE;
 
   // If either host, port or scheme are disjoint return immediately.
   Relation host_relation = CompareHost(parts_, other.parts_);
-  if (host_relation == DISJOINT)
-    return DISJOINT;
+  if (host_relation == DISJOINT_ORDER_PRE ||
+      host_relation == DISJOINT_ORDER_POST)
+    return host_relation;
+
   Relation port_relation = ComparePort(parts_, other.parts_);
-  if (port_relation == DISJOINT)
-    return DISJOINT;
+  if (port_relation == DISJOINT_ORDER_PRE ||
+      port_relation == DISJOINT_ORDER_POST)
+    return port_relation;
+
   Relation scheme_relation = CompareScheme(parts_, other.parts_);
-  if (scheme_relation == DISJOINT)
-    return DISJOINT;
+  if (scheme_relation == DISJOINT_ORDER_PRE ||
+      scheme_relation == DISJOINT_ORDER_POST)
+    return scheme_relation;
 
   if (host_relation != IDENTITY)
     return host_relation;
@@ -369,17 +412,33 @@ ContentSettingsPattern::Relation ContentSettingsPattern::Compare(
   return scheme_relation;
 }
 
+bool ContentSettingsPattern::operator==(
+    const ContentSettingsPattern& other) const {
+  return Compare(other) == IDENTITY;
+}
+
+bool ContentSettingsPattern::operator<(
+    const ContentSettingsPattern& other) const {
+  return  Compare(other) < 0;

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

Nit: superfluous space before Compare.

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

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.

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

Done.

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

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

+}
+
+bool ContentSettingsPattern::operator>(
+    const ContentSettingsPattern& other) const {
+  return Compare(other) > 0;
+}
+
 // static
 ContentSettingsPattern::Relation ContentSettingsPattern::CompareHost(
     const ContentSettingsPattern::PatternParts& parts,
     const ContentSettingsPattern::PatternParts& other_parts) {
   if (!parts.has_domain_wildcard && !other_parts.has_domain_wildcard) {
     // Case 1: No host starts with a wild card
-    if (parts.host == other_parts.host) {
+    int result = CompareDomainNames(parts.host, other_parts.host);
+    if (result == 0)
       return ContentSettingsPattern::IDENTITY;
-    } else {
-      return ContentSettingsPattern::DISJOINT;
-    }
+    if (result < 0)
+      return ContentSettingsPattern::DISJOINT_ORDER_PRE;
+    return ContentSettingsPattern::DISJOINT_ORDER_POST;
   } else if (parts.has_domain_wildcard && !other_parts.has_domain_wildcard) {
     // Case 2: |host| starts with a domain wildcard and |other_host| does not
     // start with a domain wildcard.
@@ -404,7 +463,9 @@ ContentSettingsPattern::Relation ContentSettingsPattern::CompareHost(
     if (IsSubDomainOrEqual(other_parts.host, parts.host)) {
       return ContentSettingsPattern::SUCCESSOR;
     } else {
-      return ContentSettingsPattern::DISJOINT;
+       if (CompareDomainNames(parts.host, other_parts.host) < 0)
+         return ContentSettingsPattern::DISJOINT_ORDER_PRE;
+       return ContentSettingsPattern::DISJOINT_ORDER_POST;
     }
   } else if (!parts.has_domain_wildcard && other_parts.has_domain_wildcard) {
     // Case 3: |host| starts NOT with a domain wildcard and |other_host| starts
@@ -412,7 +473,9 @@ ContentSettingsPattern::Relation ContentSettingsPattern::CompareHost(
     if (IsSubDomainOrEqual(parts.host, other_parts.host)) {
       return ContentSettingsPattern::PREDECESSOR;
     } else {
-      return ContentSettingsPattern::DISJOINT;
+      if (CompareDomainNames(parts.host, other_parts.host) < 0)
+        return ContentSettingsPattern::DISJOINT_ORDER_PRE;
+      return ContentSettingsPattern::DISJOINT_ORDER_POST;
     }
   } else if (parts.has_domain_wildcard && other_parts.has_domain_wildcard) {
     // Case 4: |host| and |other_host| both start with a domain wildcard.
@@ -441,7 +504,9 @@ ContentSettingsPattern::Relation ContentSettingsPattern::CompareHost(
     } else if (IsSubDomainOrEqual(parts.host, other_parts.host)) {
       return ContentSettingsPattern::PREDECESSOR;
     } else {
-      return ContentSettingsPattern::DISJOINT;
+      if (CompareDomainNames(parts.host, other_parts.host) < 0)
+        return ContentSettingsPattern::DISJOINT_ORDER_PRE;
+      return ContentSettingsPattern::DISJOINT_ORDER_POST;
     }
   }
 
@@ -457,9 +522,13 @@ ContentSettingsPattern::Relation ContentSettingsPattern::CompareScheme(
     return ContentSettingsPattern::SUCCESSOR;
   if (!parts.is_scheme_wildcard && other_parts.is_scheme_wildcard)
     return ContentSettingsPattern::PREDECESSOR;
-  if (parts.scheme != other_parts.scheme)
-    return ContentSettingsPattern::DISJOINT;
-  return ContentSettingsPattern::IDENTITY;
+
+  int result = parts.scheme.compare(other_parts.scheme);
+  if (result == 0)
+    return ContentSettingsPattern::IDENTITY;
+  if (result > 0)
+    return ContentSettingsPattern::DISJOINT_ORDER_PRE;
+  return ContentSettingsPattern::DISJOINT_ORDER_POST;
 }
 
 // static
@@ -470,7 +539,11 @@ ContentSettingsPattern::Relation ContentSettingsPattern::ComparePort(
     return ContentSettingsPattern::SUCCESSOR;
   if (!parts.is_port_wildcard && other_parts.is_port_wildcard)
     return ContentSettingsPattern::PREDECESSOR;
-  if (parts.port != other_parts.port)
-    return ContentSettingsPattern::DISJOINT;
-  return ContentSettingsPattern::IDENTITY;
+
+  int result = parts.port.compare(other_parts.port);
+  if (result == 0)
+    return ContentSettingsPattern::IDENTITY;
+  if (result > 0)
+    return ContentSettingsPattern::DISJOINT_ORDER_PRE;
+  return ContentSettingsPattern::DISJOINT_ORDER_POST;
 }