Implement correct logic for URLPatternSet set operators.

extensions/common/url_pattern_set.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "extensions/common/url_pattern_set.h"
 
 #include <algorithm>
 #include <iterator>
 
 #include "base/logging.h"
@@ skipping 11 matching lines @@
 
 const char kInvalidURLPatternError[] = "Invalid url pattern '*'";
 
 }  // namespace
 
 // static
 void URLPatternSet::CreateDifference(const URLPatternSet& set1,
                                      const URLPatternSet& set2,
                                      URLPatternSet* out) {
   out->ClearPatterns();
-  out->patterns_ = base::STLSetDifference<std::set<URLPattern> >(
-      set1.patterns_, set2.patterns_);
+  for (URLPatternSet::const_iterator pattern1 = set1.patterns_.begin();
+       pattern1 != set1.patterns_.end(); ++pattern1) {
+    bool keepPattern1 = true;

[not at google - send to devlin, 2014/02/27 21:10:46]

is this whole block just

if (!set.ContainsPattern(pattern1))
  out->InsertPattern(*pattern1)

? (in which case "pattern1" could be just "pattern" or "it")

[rpaquay, 2014/02/28 01:46:47]

Done.

+    for (URLPatternSet::const_iterator pattern2 = set2.patterns_.begin();
+         pattern2 != set2.patterns_.end(); ++pattern2) {
+      if (*pattern2 == *pattern1 || pattern2->Contains(*pattern1)) {

[not at google - send to devlin, 2014/02/27 21:10:46]

Could you add the equality shortcut (assuming its useful) to
URLPattern::Contains and then just have pattern2->Contains(pattern1) here?

[rpaquay, 2014/02/28 01:46:47]

Done.

+        keepPattern1 = false;
+        break;
+      }
+    }
+    if (keepPattern1)
+      out->InsertPattern(*pattern1);
+  }
 }
 
 // static
 void URLPatternSet::CreateIntersection(const URLPatternSet& set1,
                                        const URLPatternSet& set2,
                                        URLPatternSet* out) {
   out->ClearPatterns();
-  std::set_intersection(set1.patterns_.begin(), set1.patterns_.end(),
-                        set2.patterns_.begin(), set2.patterns_.end(),
-                        std::inserter<std::set<URLPattern> >(
-                            out->patterns_, out->patterns_.begin()));
+  for (URLPatternSet::const_iterator pattern1 = set1.patterns_.begin();
+       pattern1 != set1.patterns_.end(); ++pattern1) {
+    for (URLPatternSet::const_iterator pattern2 = set2.patterns_.begin();
+         pattern2 != set2.patterns_.end(); ++pattern2) {
+      if (*pattern1 == *pattern2) {
+        out->InsertPattern(*pattern1);

[not at google - send to devlin, 2014/02/27 21:10:46]

likewise above comment; this *pattern1 == *pattern2 check shouldn't be needed
and would be caught in the next condition.

[rpaquay, 2014/02/28 01:46:47]

Done.

+      } else if (pattern1->Contains(*pattern2)) {
+        out->InsertPattern(*pattern2);
+      } else if (pattern2->Contains(*pattern1)) {
+        out->InsertPattern(*pattern1);
+      }
+    }
+  }
 }
 
 // static
 void URLPatternSet::CreateUnion(const URLPatternSet& set1,
                                 const URLPatternSet& set2,
                                 URLPatternSet* out) {
   out->ClearPatterns();
-  std::set_union(set1.patterns_.begin(), set1.patterns_.end(),
-                 set2.patterns_.begin(), set2.patterns_.end(),
-                 std::inserter<std::set<URLPattern> >(
-                     out->patterns_, out->patterns_.begin()));
+
+  // Add all elements from set1 except the ones that have a "better" element in
+  // set2.
+  for (URLPatternSet::const_iterator pattern1 = set1.patterns_.begin();
+       pattern1 != set1.patterns_.end(); ++pattern1) {
+    bool addPattern1 = true;
+    for (URLPatternSet::const_iterator pattern2 = set2.patterns_.begin();
+         pattern2 != set2.patterns_.end(); ++pattern2) {
+      if (!(*pattern2 == *pattern1) && pattern2->Contains(*pattern1)) {
+        addPattern1 = false;
+        break;
+      }
+    }
+    if (addPattern1)
+      out->InsertPattern(*pattern1);
+  }
+
+  // Add all elements from set2 except the ones that have a "better" element in
+  // set1.
+  for (URLPatternSet::const_iterator pattern2 = set2.patterns_.begin();
+       pattern2 != set2.patterns_.end(); ++pattern2) {
+    bool addPattern2 = true;
+    for (URLPatternSet::const_iterator pattern1 = set1.patterns_.begin();
+         pattern1 != set1.patterns_.end(); ++pattern1) {
+      if (!(*pattern1 == *pattern2) && pattern1->Contains(*pattern2)) {
+        addPattern2 = false;
+        break;
+      }
+    }
+    if (addPattern2)
+      out->InsertPattern(*pattern2);
+  }

[not at google - send to devlin, 2014/02/27 21:10:46]

see above comment(s) that apply to this function too. It also looks like a
candidate for calling a single function twice?

[rpaquay, 2014/02/28 01:46:47]

Actually, since I updated AddPattern to do the "right thing", I could simplify
the code even further.

 }
 
 // static
 void URLPatternSet::CreateUnion(const std::vector<URLPatternSet>& sets,
                                 URLPatternSet* out) {
   out->ClearPatterns();
   if (sets.empty())
     return;
 
   // N-way union algorithm is basic O(nlog(n)) merge algorithm.
@@ skipping 43 matching lines @@
 
 bool URLPatternSet::is_empty() const {
   return patterns_.empty();
 }
 
 size_t URLPatternSet::size() const {
   return patterns_.size();
 }
 
 bool URLPatternSet::AddPattern(const URLPattern& pattern) {
-  return patterns_.insert(pattern).second;
+  // Note: Adding a more specific pattern should be a no-op.
+  for (URLPatternSet::const_iterator it = patterns_.begin();
+       it != patterns_.end(); ++it) {
+    if (it->Contains(pattern))
+      return false;
+  }

[not at google - send to devlin, 2014/02/27 21:10:46]

if (ContainsPattern(pattern))
  return false;

?

[rpaquay, 2014/02/28 01:46:47]

Done.

+  // Adding a more general pattern should remove the more specific patterns
+  for (URLPatternSet::const_iterator it = patterns_.begin();
+       it != patterns_.end();) {
+    if (pattern.Contains(*it))
+      patterns_.erase(it++);
+    else
+      ++it;

[not at google - send to devlin, 2014/02/27 21:10:46]

nice

[rpaquay, 2014/02/28 01:46:47]

http://stackoverflow.com/questions/2874441/deleting-elements-from-stl-set-whi...

:)

+  }
+  return InsertPattern(pattern);
 }
 
 void URLPatternSet::AddPatterns(const URLPatternSet& set) {
-  patterns_.insert(set.patterns().begin(),
-                   set.patterns().end());
+  for (URLPatternSet::const_iterator pattern = set.patterns_.begin();
+       pattern != set.patterns_.end(); ++pattern) {
+    AddPattern(*pattern);
+  }
 }
 
 void URLPatternSet::ClearPatterns() {
   patterns_.clear();
 }
 
 bool URLPatternSet::Contains(const URLPatternSet& other) const {
   for (URLPatternSet::const_iterator it = other.begin();
        it != other.end(); ++it) {
     if (!ContainsPattern(*it))
@@ skipping 87 matching lines @@
   std::vector<std::string> patterns;
   for (size_t i = 0; i < value.GetSize(); ++i) {
     std::string item;
     if (!value.GetString(i, &item))
       return false;
     patterns.push_back(item);
   }
   return Populate(patterns, valid_schemes, allow_file_access, error);
 }
 
+bool URLPatternSet::InsertPattern(const URLPattern& pattern) {
+  return patterns_.insert(pattern).second;
+}
+
 }  // namespace extensions