Refactoring how conditions without URL attributes are handled.

chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc

Index: chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
diff --git a/chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc b/chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
index 77bc9b8f44ef22fa8bbbf5a3aa67ed9398a29687..38263a2cf20834633ee6bb67f429521705c92fa6 100644
--- a/chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
+++ b/chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
@@ -4,7 +4,9 @@
 
 #include "chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h"
 
+#include <algorithm>
 #include <limits>
+#include <utility>
 
 #include "chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h"
 #include "chrome/browser/extensions/api/web_request/web_request_api_helpers.h"
@@ -21,26 +23,37 @@ WebRequestRulesRegistry::WebRequestRulesRegistry(Profile* profile,
     extension_info_map_ = ExtensionSystem::Get(profile)->info_map();
 }
 
-std::set<WebRequestRule::GlobalRuleId>
+std::set<const WebRequestRule*>
 WebRequestRulesRegistry::GetMatches(
     const WebRequestRule::RequestData& request_data) {
-  std::set<WebRequestRule::GlobalRuleId> result;
-
-  // Figure out for which rules the URL match conditions were fulfilled.
+  typedef std::set<const WebRequestRule*> RuleSet;
   typedef std::set<URLMatcherConditionSet::ID> URLMatches;
+
+  RuleSet not_fulfilled;
+  RuleSet result;
   URLMatches url_matches = url_matcher_.MatchURL(request_data.request->url());
 
-  // Then we need to check for each of these, whether the other
-  // WebRequestConditionAttributes are also fulfilled.
-  for (URLMatches::iterator url_match = url_matches.begin();
+  // 1st phase -- add all rules with some conditions without UrlFilter
+  // attributes.
+  for (RuleSet::const_iterator it = rules_with_untriggered_conditions_.begin();
+       it != rules_with_untriggered_conditions_.end(); ++it) {
+    if ((*it)->conditions().IsFulfilled(-1, url_matches, request_data))
+      result.insert(*it);
+    else
+      not_fulfilled.insert(*it);
+  }
+
+  // 2nd phase -- add all rules with some conditions triggered by URL matches.
+  for (URLMatches::const_iterator url_match = url_matches.begin();
        url_match != url_matches.end(); ++url_match) {
-    RuleTriggers::iterator rule_trigger = rule_triggers_.find(*url_match);
+    RuleTriggers::const_iterator rule_trigger = rule_triggers_.find(*url_match);
     CHECK(rule_trigger != rule_triggers_.end());
-
-    WebRequestRule* rule = rule_trigger->second;
-    if (rule->conditions().IsFulfilled(*url_match, request_data))
-      result.insert(rule->id());
+    if (not_fulfilled.find(rule_trigger->second) == not_fulfilled.end() &&

[Jeffrey Yasskin, 2013/01/10 22:00:57]

Use ContainsKey from base/stl_util.h instead of find?

[Jeffrey Yasskin, 2013/01/10 22:00:57]

Is this right? Say there's a rule with conditions: [ {url: "google.com"},
{contentType: "neverMatch"} ]. Then this rule will end up in
rules_with_untriggered_conditions_, and since it doesn't match with no URL,
it'll end up in not_fulfilled. Now it comes up on a google.com request in this
triggered loop and gets skipped because it wasn't fulfilled with no URL, even
though it would be fulfilled with the current URL.

Could you add a test for this?  (This sort of bug or confusion-on-my-part is why
I pushed for the simplest implementation rather than trying to optimize it by
punching information through the abstraction layers.)

[battre, 2013/01/11 17:41:55]

I think the test we want here is:
if (!ContainsKey(result, rule_trigger->second) &&
rule_trigger->second->conditions().IsFulfilled(...))

If it is fulfilled already, we don't need to test any further.

[vabr (Chromium), 2013/01/15 10:24:57]

Thanks, Jeffrey, for pointing this out, this was a stupid mistake of mine.

I did as Dominic suggested, and also added the unit test.

+        rule_trigger->second->conditions().IsFulfilled(
+            *url_match, url_matches, request_data))
+      result.insert(rule_trigger->second);
   }
+
   return result;
 }
 
@@ -51,8 +64,7 @@ std::list<LinkedPtrEventResponseDelta> WebRequestRulesRegistry::CreateDeltas(
   if (webrequest_rules_.empty())
     return std::list<LinkedPtrEventResponseDelta>();
 
-  std::set<WebRequestRule::GlobalRuleId> matches =
-      GetMatches(request_data);
+  std::set<const WebRequestRule*> matches = GetMatches(request_data);
 
   // Sort all matching rules by their priority so that they can be processed
   // in decreasing order.
@@ -60,11 +72,9 @@ std::list<LinkedPtrEventResponseDelta> WebRequestRulesRegistry::CreateDeltas(
       PriorityRuleIdPair;
   std::vector<PriorityRuleIdPair> ordered_matches;
   ordered_matches.reserve(matches.size());
-  for (std::set<WebRequestRule::GlobalRuleId>::iterator i = matches.begin();
+  for (std::set<const WebRequestRule*>::iterator i = matches.begin();
        i != matches.end(); ++i) {
-    RulesMap::const_iterator rule = webrequest_rules_.find(*i);
-    CHECK(rule != webrequest_rules_.end());
-    ordered_matches.push_back(make_pair(rule->second->priority(), *i));
+    ordered_matches.push_back(make_pair((*i)->priority(), (*i)->id()));
   }
   // Sort from rbegin to rend in order to get descending priority order.
   std::sort(ordered_matches.rbegin(), ordered_matches.rend());
@@ -157,11 +167,14 @@ std::string WebRequestRulesRegistry::AddRulesImpl(
     }
   }
 
-  // Register url patterns in url_matcher_.
+  // Register url patterns in |url_matcher_| and
+  // |rules_with_untriggered_conditions_|.
   URLMatcherConditionSet::Vector all_new_condition_sets;
   for (RulesMap::iterator i = new_webrequest_rules.begin();
        i != new_webrequest_rules.end(); ++i) {
     i->second->conditions().GetURLMatcherConditionSets(&all_new_condition_sets);
+    if (i->second->conditions().HasConditionsWithoutUrls())
+      rules_with_untriggered_conditions_.insert(i->second.get());
   }
   url_matcher_.AddConditionSets(all_new_condition_sets);
 
@@ -195,7 +208,9 @@ std::string WebRequestRulesRegistry::RemoveRulesImpl(
       rule_triggers_.erase((*j)->id());
     }
 
-    // Remove reference to actual rule.
+    rules_with_untriggered_conditions_.erase(rule);
+
+    // Removes the owning references to (and thus deletes) the rule.
     webrequest_rules_.erase(webrequest_rules_entry);
   }