Adding condition attributes for response headers to Declarative WebRequest

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

Index: chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h
diff --git a/chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h b/chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h
index f9aa4e058313d306bd499f82d71d7912451c96a3..58f7993fdc3ec1e2b83ca83bef275b14c4c4e7a6 100644
--- a/chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h
+++ b/chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h
@@ -32,7 +32,8 @@ class WebRequestConditionAttribute {
  public:
   enum Type {
     CONDITION_RESOURCE_TYPE,
-    CONDITION_CONTENT_TYPE
+    CONDITION_CONTENT_TYPE,
+    CONDITION_CONTAINS_HEADERS

[battre, 2012/08/23 12:32:11]

We need support for request and response headers. You can handle these in
different CLs, but I think the class/enum names should reflect the type.

[vabr (Chromium), 2012/08/24 14:48:59]

Done. I don't think a condition attribute for request headers will be different
from this, except for how the headers are obtained. Anyway, I'll leave request
headers for a second CL, and in that CL I will probably put the common
functionality of the header-related condition attributes in a headers-named base
class, and derive the request- and response-instances out of it.

   };
 
   WebRequestConditionAttribute();
@@ -133,6 +134,75 @@ class WebRequestConditionAttributeContentType
   DISALLOW_COPY_AND_ASSIGN(WebRequestConditionAttributeContentType);
 };
 
+// Condition that performs matches against response headers' names and values.
+// In the comments below there is a distinction between when this condition is
+// "satisfied" and when it is "fulfilled". See the comments at PushMatchTests
+// and |tests_| for "satisfied" and the comment at |positive_test_| for
+// "fulfilled".
+class WebRequestConditionAttributeContainsHeaders

[battre, 2012/08/23 12:32:11]

I would suggest to rename this to WebRequestConditionAttributeResponseHeaders

[vabr (Chromium), 2012/08/24 14:48:59]

Done.

+    : public WebRequestConditionAttribute {
+ public:
+  virtual ~WebRequestConditionAttributeContainsHeaders();
+
+  static bool IsMatchingType(const std::string& instance_type);
+
+  // Factory method, see WebRequestConditionAttribute::Create.
+  static scoped_ptr<WebRequestConditionAttribute> Create(
+      const std::string& name,
+      const base::Value* value,
+      std::string* error);
+
+  // Implementation of WebRequestConditionAttribute:
+  virtual int GetStages() const OVERRIDE;
+  virtual bool IsFulfilled(const WebRequestRule::RequestData& request_data)
+      OVERRIDE;
+  virtual Type GetType() const OVERRIDE;
+
+ private:
+  enum MatchType { kPrefix, kSuffix, kEqual, kContains };

[battre, 2012/08/23 12:32:11]

kEquals to match the parameter of the attribute

[vabr (Chromium), 2012/08/24 14:48:59]

Done.

+  class MatchTest {
+   public:
+    explicit MatchTest(MatchType type) : type_(type) {}
+    std::string& data() {
+      return data_;

[battre, 2012/08/23 12:32:11]

I'd prefer if you moved this to the constructor, even if this means one more
copy operation. That way, the MatchTest becomes a pure immutable object.

[vabr (Chromium), 2012/08/24 14:48:59]

Done. And retaining the number of copy operations by using a pointer to a
string.

+    }
+    // Does |str| pass |*this| MatchTest?
+    bool Matches(const std::string& str);

[battre, 2012/08/23 12:32:11]

nit: const

[vabr (Chromium), 2012/08/24 14:48:59]

Done.

+   private:
+    std::string data_;
+    MatchType type_;

[battre, 2012/08/23 12:32:11]

nit: DISALLOW_COPY_AND_ASSIGN due to non-trivial datatype of |data_|

[vabr (Chromium), 2012/08/24 14:48:59]

Done.

+  };
+  struct TestGroup {
+    TestGroup();
+    ~TestGroup();
+    // Tests to be passed by a header's name.
+    std::vector<MatchTest> name;

[battre, 2012/08/23 12:32:11]

This should become a ScopedVector if you add the DISALLOW_COPY_AND_ASSIGNs.

[vabr (Chromium), 2012/08/24 14:48:59]

Done.

+    // Tests to be passed by a header's value.
+    std::vector<MatchTest> value;

[battre, 2012/08/23 12:32:11]

nit: DISALLOW_COPY_AND_ASSIGN due to non-trivial datatype of |name| and |value|

[vabr (Chromium), 2012/08/24 14:48:59]

Done.

+  };
+
+  explicit WebRequestConditionAttributeContainsHeaders(bool positive_test);
+
+  // Pushes another group of tests on our tests stack |tests_|. The whole
+  // condition is satisfied if there is a group of tests pushed by some call to
+  // PushMatchTests, and a header with a value which satisfies all of the tests
+  // from that group.
+  bool PushMatchTests(const base::DictionaryValue* tests, std::string* error);

[battre, 2012/08/23 12:32:11]

How about s/Push/Append/ to match the ListValue notation. I think that the whole
"push" notation of the STL is very strange.

[vabr (Chromium), 2012/08/24 14:48:59]

I ended up with GetTests and GetMatchTest, because no pushing/appending is done
there any more.

+  // Helper to PushMatchTests.
+  void PushOneMatchTest(
+      const Value* content, bool is_name_test, MatchType match_type);
+
+  // The condition is satisfied if there is a header and its value such that for
+  // some |i| the header passes all the tests from |tests_[i]|.
+  std::vector< TestGroup > tests_;

[battre, 2012/08/23 12:32:11]

nit: no spaces

[battre, 2012/08/23 12:32:11]

This should become a ScopedVector if you add the DISALLOW_COPY_AND_ASSIGNs.

[vabr (Chromium), 2012/08/24 14:48:59]

Done.

[vabr (Chromium), 2012/08/24 14:48:59]

Done.

+
+  // True means that IsFulfilled() reports whether the condition is satisfied.
+  // False means that it reports whether the condition is NOT satisfied.
+  bool positive_test_;
+
+  DISALLOW_COPY_AND_ASSIGN(WebRequestConditionAttributeContainsHeaders);
+};
+
 }  // namespace extensions
 
 #endif  // CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_WEBREQUEST_WEBREQUEST_CONDITION_ATTRIBUTE_H_