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..34d81a5572e2f3588761c4d950c35aa089d89b72 100644
--- a/chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
+++ b/chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
@@ -21,26 +21,33 @@ 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;
+  // 1st phase -- add all rules with some conditions without UrlFilter
+  // attributes.
+  std::set<const WebRequestRule*> result(rules_with_untriggered_conditions_);
 
-  // Figure out for which rules the URL match conditions were fulfilled.
+  // 2nd phase -- add all rules with some conditions triggered by URL matches.
   typedef std::set<URLMatcherConditionSet::ID> URLMatches;
   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();
+  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());
+    result.insert(rule_trigger->second);
+  }
 
-    WebRequestRule* rule = rule_trigger->second;
-    if (rule->conditions().IsFulfilled(*url_match, request_data))
-      result.insert(rule->id());
+  // 3rd phase -- eliminate all rules with no satisfied condition (that may
+  // happen due to non-UrlFilter attributes).
+  std::set<const WebRequestRule*>::iterator it = result.begin();
+  while (it != result.end()) {
+    if ((*it)->conditions().IsFulfilled(url_matches, request_data))
+      ++it;
+    else
+      result.erase(it++);
   }

[battre, 2013/01/07 17:36:27]

I think this can be simplified by just iterating over
rules_with_untriggered_conditions_ and url_matches and only inserting what
matches. I think it may be fewer lines of code and require less ressources.

[Jeffrey Yasskin, 2013/01/07 20:47:41]

Yes, that's cheaper. I had guessed that it would be more lines of code since
you'd want to avoid checking IsFulfilled on rules you'd already checked, but I
could be wrong.

[vabr (Chromium), 2013/01/10 15:12:22]

Done.

+
   return result;
 }
 
@@ -51,8 +58,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 +66,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 +161,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 +202,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);
   }