Refactoring how conditions without URL attributes are handled.

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

Index: chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc
diff --git a/chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc b/chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc
index 1ece019e6a397c6c4e7e31d0ecc7cc457b17551c..48c8c10f02c33301098a11a29092ff9e7a100402 100644
--- a/chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc
+++ b/chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.cc
@@ -6,6 +6,7 @@
 
 #include "base/bind.h"
 #include "base/logging.h"
+#include "base/stl_util.h"
 #include "base/stringprintf.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/api/declarative_webrequest/request_stage.h"
@@ -49,7 +50,6 @@ WebRequestCondition::WebRequestCondition(
     : url_matcher_conditions_(url_matcher_conditions),
       condition_attributes_(condition_attributes),
       applicable_request_stages_(~0) {
-  CHECK(url_matcher_conditions.get());
   for (WebRequestConditionAttributes::const_iterator i =
        condition_attributes_.begin(); i != condition_attributes_.end(); ++i) {
     applicable_request_stages_ &= (*i)->GetStages();
@@ -59,7 +59,13 @@ WebRequestCondition::WebRequestCondition(
 WebRequestCondition::~WebRequestCondition() {}
 
 bool WebRequestCondition::IsFulfilled(
+    const std::set<URLMatcherConditionSet::ID>& url_matches,
     const WebRequestRule::RequestData& request_data) const {
+  // Check a UrlFilter attribute if present.
+  if (url_matcher_conditions_.get() &&
+      !ContainsKey(url_matches, url_matcher_conditions_->id()))
+    return false;
+
   // All condition attributes must be fulfilled for a fulfilled condition.
   if (!(request_data.stage & applicable_request_stages_)) {
     // A condition that cannot be evaluated is considered as violated.
@@ -132,13 +138,6 @@ scoped_ptr<WebRequestCondition> WebRequestCondition::Create(
       return scoped_ptr<WebRequestCondition>(NULL);
   }
 
-  if (!url_matcher_condition_set) {
-    URLMatcherConditionSet::Conditions url_matcher_conditions;
-    url_matcher_conditions.insert(
-        url_matcher_condition_factory->CreateHostPrefixCondition(""));
-    url_matcher_condition_set =
-        new URLMatcherConditionSet(++g_next_id, url_matcher_conditions);
-  }
   scoped_ptr<WebRequestCondition> result(
       new WebRequestCondition(url_matcher_condition_set, attributes));
 
@@ -150,38 +149,54 @@ scoped_ptr<WebRequestCondition> WebRequestCondition::Create(
   return result.Pass();
 }
 
-
 //
 // WebRequestConditionSet
 //
 
+namespace {
+
+// Returns whether there is a condition among |conditions| which has no
+// URLMatcherConditionSet assigned.
+bool HasConditionWithoutUrlMatcher(

[vabr (Chromium), 2012/12/19 18:26:52]

This was completely moved to the Create method.

+    const WebRequestConditionSet::Conditions& conditions) {
+  for (WebRequestConditionSet::Conditions::const_iterator i =
+           conditions.begin();
+       i != conditions.end(); ++i) {
+    if (!(*i)->url_matcher_condition_set().get())
+      return true;
+  }
+  return false;
+}
+
+}  // namespace
+
 WebRequestConditionSet::WebRequestConditionSet(
     const WebRequestConditionSet::Conditions& conditions)
-    : conditions_(conditions) {
-  for (Conditions::iterator i = conditions_.begin(); i != conditions_.end();
-       ++i) {
-    URLMatcherConditionSet::ID trigger_id =
-        (*i)->url_matcher_condition_set_id();
-    match_triggers_[trigger_id] = i->get();
-  }
+    : conditions_(conditions),
+      has_conditions_without_urls_(HasConditionWithoutUrlMatcher(conditions)) {
 }
 
 WebRequestConditionSet::~WebRequestConditionSet() {}
 
 bool WebRequestConditionSet::IsFulfilled(
-    URLMatcherConditionSet::ID url_match,
+    const std::set<URLMatcherConditionSet::ID>& url_matches,
     const WebRequestRule::RequestData& request_data) const {
-  MatchTriggers::const_iterator trigger = match_triggers_.find(url_match);
-  DCHECK(trigger != match_triggers_.end());
-  DCHECK_EQ(url_match, trigger->second->url_matcher_condition_set_id());
-  return trigger->second->IsFulfilled(request_data);
+  for (Conditions::const_iterator i = conditions_.begin();
+       i != conditions_.end(); ++i) {
+    if ((*i)->IsFulfilled(url_matches, request_data))
+      return true;

[battre, 2012/12/19 14:38:01]

I continued thinking about this but I am not convinced anymore that this is the
right thing to do.

Say you have a condition set with 10,000 conditions (cancel the request if any
of these 10,000 URL filters matches). This degrades from O(1) to O(10,000) here.
It is ok if we have false positives but not 9,999 of them.

[vabr (Chromium), 2012/12/19 18:26:52]

I returned the triggers. They are now used to prioritise which conditions to
test in ConditionSet::IsFulfilled.

+  }
+  return false;
 }
 
 void WebRequestConditionSet::GetURLMatcherConditionSets(
     URLMatcherConditionSet::Vector* condition_sets) const {
   for (Conditions::const_iterator i = conditions_.begin();
        i != conditions_.end(); ++i) {
-    condition_sets->push_back((*i)->url_matcher_condition_set());
+    scoped_refptr<URLMatcherConditionSet> set =
+        (*i)->url_matcher_condition_set();
+    if (set.get())
+      condition_sets->push_back(set);
   }
 }