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..b61e9d8824cb9df47ea36f684b68cb0a04aa9f29 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,44 @@ scoped_ptr<WebRequestCondition> WebRequestCondition::Create(
   return result.Pass();
 }
 
-
 //
 // WebRequestConditionSet
 //
 
-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();
-  }
-}
-
 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);
+  // Prioritize the conditions triggered by URL matches.
+  for (std::set<URLMatcherConditionSet::ID>::const_iterator it =
+           url_matches.begin();
+       it != url_matches.end(); ++it) {
+    MatchTriggers::const_iterator trigger = match_triggers_.find(*it);
+    if (trigger != match_triggers_.end() &&
+        trigger->second->IsFulfilled(url_matches, request_data))
+      return true;
+  }
+
+  // Now try out all conditions without URL attributes.
+  for (std::vector<const WebRequestCondition*>::const_iterator it =
+           conditions_without_urls_.begin();
+       it != conditions_without_urls_.end(); ++it) {
+    if ((*it)->IsFulfilled(url_matches, request_data))
+      return true;
+  }
+
+  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);
   }
 }
 
@@ -190,7 +195,7 @@ scoped_ptr<WebRequestConditionSet> WebRequestConditionSet::Create(
     URLMatcherConditionFactory* url_matcher_condition_factory,
     const AnyVector& conditions,
     std::string* error) {
-  WebRequestConditionSet::Conditions result;
+  Conditions result;
 
   for (AnyVector::const_iterator i = conditions.begin();
        i != conditions.end(); ++i) {
@@ -203,7 +208,33 @@ scoped_ptr<WebRequestConditionSet> WebRequestConditionSet::Create(
     result.push_back(make_linked_ptr(condition.release()));
   }
 
-  return scoped_ptr<WebRequestConditionSet>(new WebRequestConditionSet(result));
+  MatchTriggers match_triggers;

[Jeffrey Yasskin, 2012/12/19 19:10:52]

<unactionable-complaining>I wish we had a better name for this. I see where it
came from, but neither part of the name really explains what it's for. It maps
URL match IDs to Conditions. All it "triggers" is the full IsFulfilled() check.
Maybe "conditions_matching_url", but that doesn't really capture that it's a
URLMatcher ID. Anyway, no need to fix this in this change. I'm just complaining.

[vabr (Chromium), 2012/12/20 08:57:13]

This makes sense to me. During the programming I thought of |match_triggers_|
exactly as "id to condition lookup". I believe renaming it so makes more sense,
and I went for |match_id_to_condition_|.

Dominic, flagging this to you in case you want to object.

+  std::vector<const WebRequestCondition*> conditions_without_urls;
+
+  for (Conditions::const_iterator i = result.begin(); i != result.end(); ++i) {
+    const URLMatcherConditionSet* set = (*i)->url_matcher_condition_set().get();
+    if (set) {
+      URLMatcherConditionSet::ID trigger_id = set->id();
+      match_triggers[trigger_id] = i->get();
+    } else {
+      conditions_without_urls.push_back(i->get());
+    }
+  }
+
+  return make_scoped_ptr(new WebRequestConditionSet(
+      result, match_triggers, conditions_without_urls));
+}
+
+bool WebRequestConditionSet::HasConditionsWithoutUrls() const {
+  return conditions_without_urls_.size() > 0u;

[Jeffrey Yasskin, 2012/12/19 19:10:52]

You can use !conditions_without_urls_.empty()

[vabr (Chromium), 2012/12/20 08:57:13]

Done.

 }
 
+WebRequestConditionSet::WebRequestConditionSet(
+    const Conditions& conditions,
+    const MatchTriggers& match_triggers,
+    const std::vector<const WebRequestCondition*> conditions_without_urls)
+    : match_triggers_(match_triggers),
+      conditions_(conditions),
+      conditions_without_urls_(conditions_without_urls) {}
+
 }  // namespace extensions