Refactoring how conditions without URL attributes are handled.

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

 // 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.
 
 #ifndef CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_WEBREQUEST_WEBREQUEST_CONDITION_H_
 #define CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_WEBREQUEST_WEBREQUEST_CONDITION_H_
 
+#include <set>
 #include <string>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/memory/linked_ptr.h"
 #include "chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h"
 #include "chrome/browser/extensions/api/declarative_webrequest/webrequest_rule.h"
 #include "chrome/common/extensions/matcher/url_matcher.h"
 
 namespace extensions {
@@ skipping 22 matching lines @@
       const WebRequestConditionAttributes& condition_attributes);
   ~WebRequestCondition();
 
   // Factory method that instantiates a WebRequestCondition according to
   // the description |condition| passed by the extension API.
   static scoped_ptr<WebRequestCondition> Create(
       URLMatcherConditionFactory* url_matcher_condition_factory,
       const base::Value& condition,
       std::string* error);
 
-  // Returns whether the request is a match, given that the URLMatcher found
-  // a match for |url_matcher_conditions_|.
-  bool IsFulfilled(const WebRequestRule::RequestData& request_data) const;
+  // Returns whether the request matches this condition.  |url_matches| lists
+  // the IDs that match the request's URL.
+  bool IsFulfilled(const std::set<URLMatcherConditionSet::ID>& url_matches,
+                   const WebRequestRule::RequestData& request_data) const;
 
   // Returns a URLMatcherConditionSet::ID which is the canonical representation
   // for all URL patterns that need to be matched by this WebRequestCondition.
   // This ID is registered in a URLMatcher that can inform us in case of a
   // match.
   URLMatcherConditionSet::ID url_matcher_condition_set_id() const {
+    DCHECK(url_matcher_conditions_.get());
     return url_matcher_conditions_->id();
   }
 
-  // Returns the set of conditions that are checked on the URL. This is the
-  // primary trigger for WebRequestCondition and therefore never empty.
-  // (If it was empty, the URLMatcher would never notify us about network
-  // requests which might fulfill the entire WebRequestCondition).
+  // Returns the set of conditions that are checked on the URL. May be NULL.
   scoped_refptr<URLMatcherConditionSet> url_matcher_condition_set() const {
     return url_matcher_conditions_;
   }
 
   // Returns the condition attributes checked by this condition.
   const WebRequestConditionAttributes condition_attributes() const {
     return condition_attributes_;
   }
 
   // Returns a bit vector representing extensions::RequestStage. The bit vector
   // contains a 1 for each request stage during which the condition can be
   // tested.
   int stages() const { return applicable_request_stages_; }
 
  private:
+  // Represents the 'url' attribute of this condition. If NULL, then there was
+  // no 'url' attribute in this condition.
   scoped_refptr<URLMatcherConditionSet> url_matcher_conditions_;
+
+  // All non-UrlFilter attributes of this condition.
   WebRequestConditionAttributes condition_attributes_;
 
   // Bit vector indicating all RequestStage during which all
   // |condition_attributes_| can be evaluated.
   int applicable_request_stages_;
 
   DISALLOW_COPY_AND_ASSIGN(WebRequestCondition);
 };
 
 // This class stores a set of conditions that may be part of a WebRequestRule.
 // If any condition is fulfilled, the WebRequestActions of the WebRequestRule
 // can be triggered.
 class WebRequestConditionSet {
  public:
   typedef std::vector<linked_ptr<json_schema_compiler::any::Any> > AnyVector;
   typedef std::vector<linked_ptr<WebRequestCondition> > Conditions;
 
-  explicit WebRequestConditionSet(const Conditions& conditions);
   ~WebRequestConditionSet();
 
   // Factory method that creates an WebRequestConditionSet according to the JSON
   // array |conditions| passed by the extension API.
   // Sets |error| and returns NULL in case of an error.
   static scoped_ptr<WebRequestConditionSet> Create(
       URLMatcherConditionFactory* url_matcher_condition_factory,
       const AnyVector& conditions,
       std::string* error);
 
   const Conditions& conditions() const {
     return conditions_;
   }
 
-  // Returns whether any condition in the condition set is fulfilled
-  // based on a match |url_match| and the value of |request_data.request|.
-  // This function should be called for each URLMatcherConditionSet::ID
-  // that was found by the URLMatcher to ensure that the each trigger in
-  // |match_triggers_| is found.
+  // Returns whether any condition in the condition set is fulfilled based on
+  // matches in |url_matches| and the value of |request_data.request|. This
+  // function should be called for a ConditionSet if any Condition in the
+  // ConditionSet was triggered by a url match or if the ConditionSet contains
+  // a Condition without a UrlFilter.
   bool IsFulfilled(
-      URLMatcherConditionSet::ID url_match,
+      const std::set<URLMatcherConditionSet::ID>& url_matches,
       const WebRequestRule::RequestData& request_data) const;
 
   // Appends the URLMatcherConditionSet from all conditions to |condition_sets|.
   void GetURLMatcherConditionSets(
       URLMatcherConditionSet::Vector* condition_sets) const;
 
+  // Returns whether there are some conditions without UrlFilter attributes.
+  bool HasConditionsWithoutUrls() const;
+
  private:
-  Conditions conditions_;
+  typedef std::map<URLMatcherConditionSet::ID, const WebRequestCondition*>

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

For optimal memory use and performance, this ought to be a sorted vector. That
would also help optimize IsFulfilled, since we could search a smaller range as
we go through the url_matches set, or even skip ahead in url_matches if the
smallest ID for a condition is bigger than the current match ID. No need to
optimize this now, since it'll make the code a bit less readable.

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

I'm not sure I understand. If we iterate over a map, it already gives us the
key-value pairs sorted by keys. What do you suggest to be the type of the vector
elements, and how do you want to sort them?

I tried to modify IsFulfilled to only explore the range of |url_matches|
corresponding to the |match_triggers_|.

[Jeffrey Yasskin, 2012/12/20 18:15:38]

The vector's elements should be std::pair<URLMatcherConditionSet::ID, const
WebRequestCondition*>, and they should be sorted by the ID, just like the map.
You'd look things up using std::lower_bound. The reason to pick vector<> over
map<> is just that map is big (~4 pointers of overhead per value, plus N/2
internal nodes) and slow (pointer-hopping == cache misses), and for immutable
mappings none of vector<>'s downsides apply.

BUT, switching to vector makes the code harder to read, so we really should only
do it if this code shows up on a profile. I definitely don't want to delay
checking this CL in by making you optimize it.

+      MatchTriggers;
 
-  typedef std::map<URLMatcherConditionSet::ID, WebRequestCondition*>
-      MatchTriggers;
-  MatchTriggers match_triggers_;
+  WebRequestConditionSet(
+      const Conditions& conditions,
+      const MatchTriggers& match_triggers,
+      const std::vector<const WebRequestCondition*> conditions_without_urls);

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

You forgot an & on conditions_without_urls.

(There's an argument for taking all of these arguments by value, but it requires
that I write a base::move() function, which I haven't done yet.)

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

Done.

+
+  const MatchTriggers match_triggers_;
+  const Conditions conditions_;
+  const std::vector<const WebRequestCondition*> conditions_without_urls_;
 
   DISALLOW_COPY_AND_ASSIGN(WebRequestConditionSet);
 };
 
 }  // namespace extensions
 
 #endif  // CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_WEBREQUEST_WEBREQUEST_CONDITION_H_