Implementation of beginning of Declarative Web Request API backend

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_
+#pragma once
+
+#include <string>
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/gtest_prod_util.h"
+#include "base/memory/linked_ptr.h"
+#include "chrome/browser/extensions/api/declarative/url_matcher.h"
+#include "chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h"
+
+namespace extensions {
+namespace declarative_webrequest {
+
+// Representation of a condition in the Declarative WebRequest API. A condition
+// consists of several attributes. Each of these attributes needs to be
+// fulfilled in order for the condition to be fulfilled.
+//
+// We distinguish between two types of conditions:
+// - URL Matcher conditions are conditions that test the URL of a request.
+//   These are treated separately because we use a URLMatcher to efficiently
+//   test many of these conditions in parallel by using some advanced
+//   data structures. The URLMatcher tells us if all URL Matcher conditions
+//   are fulfilled for a WebRequestCondition.
+// - All other conditions are represented as WebRequestConditionAttributes.
+//   These conditions are probed linearly (only if the URL Matcher found a hit).
+class WebRequestCondition {
+ public:
+  WebRequestCondition(
+      const URLMatcherConditionSet& url_matcher_conditions,
+      const WebRequestConditionAttributes& condition_attributes);
+  ~WebRequestCondition();
+
+  // Returns whether |request| is a match, given that the URLMatcher found
+  // |url_match|.
+  bool IsFulfilled(URLMatcherConditionSet::ID url_match,
+                   net::URLRequest* request) 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 {
+    return url_matcher_conditions_.id();
+  }
+
+  // Appends the url matcher condition set to |condition_sets|.
+  void AppendURLMatcherConditionSet(
+      std::vector<URLMatcherConditionSet>* condition_sets) const;

[Matt Perry, 2012/03/22 23:07:25]

There's only one condition set - just provide an accessor to return a reference
to it. Callers can decide if they want to add it to a vector.

[battre, 2012/03/26 18:35:51]

Done.

+
+ private:
+  URLMatcherConditionSet url_matcher_conditions_;
+  WebRequestConditionAttributes condition_attributes_;
+
+  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 WebRequestConditionCollection {

[Matt Perry, 2012/03/22 23:07:25]

nit: name WebRequestConditionSet to mirror URLMatcherConditionSet?

ditto for WebRequestActionCollection

[battre, 2012/03/26 18:35:51]

In the case of URLMatcherConditionSet, you favored if class had actually Set
semantics. I picked "Collection" to have a more neutral name.

I don't want to introduce set semantic here, do you still favor renaming this
class?

[Matt Perry, 2012/03/26 19:11:06]

Do you expect duplicate WebRequestConditions/Actions in the collections? Or more
importantly, would a duplicate have any noticeable effect on the output? If
duplicates don't matter, I'd suggest "Set". You don't have to actually use a
std::set (I agree it adds unnecessary overhead in code and perf).

[battre, 2012/03/26 20:38:10]

No, there should not be any duplicates. If there are, they would not have any
impact.

Done.

+ public:
+  explicit WebRequestConditionCollection(
+      const std::vector<linked_ptr<WebRequestCondition> >& conditions);
+  ~WebRequestConditionCollection();
+
+  const std::vector<linked_ptr<WebRequestCondition> >& conditions() const {
+    return conditions_;
+  }
+
+  bool IsFulfilled(const std::set<URLMatcherConditionSet::ID>& url_matches,
+                   net::URLRequest* request) const;
+
+  // Appends the URLMatcherConditionSet from all conditions to |condition_sets|.
+  void AppendURLMatcherConditionSets(
+      std::vector<URLMatcherConditionSet>* condition_sets) const;

[Matt Perry, 2012/03/22 23:07:25]

I think this should collect a vector of const-references to the sets. An
URLMatcherConditionSet is a fairly heavyweight thing to be copying.

I'm noticing now that they end up getting copied into the URLMatcher anyway.
That doesn't seem right. Is there any way to have one owner of the
URLMatcherConditionSet? Other classes should just have a reference to it, or
perhaps just hold onto the ID.

[battre, 2012/03/26 18:35:51]

The URLMatcherConditionSet consists of an ID (int) and a set of
URLMatchingConditions (each sonsisting of an enum and a pointer). Note that the
pattern string is not copied. I would argue that this is not sooooo heavyweight
to be copied. Besides this, they are copied only once, when they are registered
to the URLMatcher.
We can set up a similar structure as we do in the URLMatcherConditionFactory but
currently we don't have a factory for URLMatcherConditionSets. They may be
created by anybody. We change the URLMatcher so that it can serve as an owner of
URLMatcherConditionSet regardless whether they are active or not.

I have added a TODO because this will be a refactoring of the URLMatcher that
justifies a separate CL.

[Matt Perry, 2012/03/26 19:11:06]

True, but both copies are still stored in memory forever. It could add up if
there are a large amount of web request rules. I could be prematurely
optimizing; we can measure and revisit later.

+
+ private:
+  typedef std::vector<linked_ptr<WebRequestCondition> > Collection;
+  Collection conditions_;
+
+  typedef std::map<URLMatcherConditionSet::ID,
+                   WebRequestCondition*> MatchTriggers;
+  MatchTriggers match_triggers_;
+
+  DISALLOW_COPY_AND_ASSIGN(WebRequestConditionCollection);
+};
+
+class WebRequestConditionFactory {

[Matt Perry, 2012/03/22 23:07:25]

since this is just a class with all static methods, I would just move the
methods onto the classes they create. e.g. WebRequestCondition::Create().

Ditto for the other factory classes you have.

[battre, 2012/03/26 18:35:51]

Done.

+ public:
+  typedef std::vector<linked_ptr<json_schema_compiler::any::Any> > AnyVector;
+
+  static scoped_ptr<WebRequestCondition> CreateCondition(
+      URLMatcherConditionFactory* url_matcher_condition_factory,
+      const base::Value& condition,
+      std::string* error);
+
+  static scoped_ptr<WebRequestConditionCollection> CreateConditionCollection(
+      URLMatcherConditionFactory* url_matcher_condition_factory,
+      const AnyVector& conditions,
+      std::string* error);
+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(WebRequestConditionFactoryTest,
+                           GetAllPathAttributesSorted);
+  // A method for testing that an internal array kAllPathAttributes remains
+  // sorted.
+  static std::vector<std::string> GetAllPathAttributes();

[Matt Perry, 2012/03/22 23:07:25]

this and the test are unnecessary if you take my suggestion about the method
map.

[battre, 2012/03/26 18:35:51]

Done.

+
+  // Returns whether an element of name |name| of a Condition needs to be
+  // handled by the UrlMatcher.
+  static bool IsUrlMatcherConditionAttribute(const std::string& name);
+
+  static URLMatcherCondition CreateURLMatcherCondition(
+      URLMatcherConditionFactory* url_matcher_condition_factory,
+      const std::string& condition_attribute_name,
+      const base::Value* value,
+      std::string* error);
+
+  DISALLOW_IMPLICIT_CONSTRUCTORS(WebRequestConditionFactory);
+};
+
+}  // namespace declarative_webrequest
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_WEBREQUEST_WEBREQUEST_CONDITION_H_