Refactoring how conditions without URL attributes are handled.

chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.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 "chrome/browser/extensions/api/declarative_webrequest/webrequest_condition.h"
 
 #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"
 #include "chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h"
 #include "chrome/browser/extensions/api/declarative_webrequest/webrequest_constants.h"
 #include "chrome/browser/extensions/api/declarative_webrequest/webrequest_constants.h"
 #include "chrome/common/extensions/matcher/url_matcher.h"
 #include "chrome/common/extensions/matcher/url_matcher_factory.h"
 #include "net/url_request/url_request.h"
 
@@ skipping 23 matching lines @@
 //
 // WebRequestCondition
 //
 
 WebRequestCondition::WebRequestCondition(
     scoped_refptr<URLMatcherConditionSet> url_matcher_conditions,
     const WebRequestConditionAttributes& condition_attributes)
     : 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();
   }
 }
 
 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.
     return false;
   }
 
   for (WebRequestConditionAttributes::const_iterator i =
        condition_attributes_.begin(); i != condition_attributes_.end(); ++i) {
     if (!(*i)->IsFulfilled(request_data))
       return false;
@@ skipping 52 matching lines @@
       if (attribute.get())
         attributes.push_back(make_linked_ptr(attribute.release()));
     } else {
       *error = base::StringPrintf(kUnknownConditionAttribute,
                                   condition_attribute_name.c_str());
     }
     if (!error->empty())
       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));
 
   if (!result->stages()) {
     *error = kConditionCannotBeFulfilled;
     return scoped_ptr<WebRequestCondition>(NULL);
   }
 
   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);
   }
 }
 
 // static
 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) {
     CHECK(i->get());
     scoped_ptr<WebRequestCondition> condition =
         WebRequestCondition::Create(url_matcher_condition_factory,
                                     (*i)->value(), error);
     if (!error->empty())
       return scoped_ptr<WebRequestConditionSet>(NULL);
     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