Extracting header testing code from WebRequestConditionAttributeResponseHeaders.

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

Index: chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc
diff --git a/chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc b/chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc
index a0ca0d986ecede3a99c906d0f65bb4f9047af90c..df6b0d447f6e6cd5b36e3c61f2e366a3e7e968c3 100644
--- a/chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc
+++ b/chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc
@@ -232,122 +232,144 @@ WebRequestConditionAttributeContentType::GetType() const {
   return CONDITION_CONTENT_TYPE;
 }
 
-//
-// WebRequestConditionAttributeResponseHeaders
-//
-
-WebRequestConditionAttributeResponseHeaders::StringMatchTest::StringMatchTest(
-    const std::string& data,
-    MatchType type)
-    : data_(data),
-      type_(type) {}
-
-WebRequestConditionAttributeResponseHeaders::StringMatchTest::~StringMatchTest()
-{}
-
-WebRequestConditionAttributeResponseHeaders::HeaderMatchTest::HeaderMatchTest(
-    ScopedVector<const StringMatchTest>* name,
-    ScopedVector<const StringMatchTest>* value)
-    : name_(name->Pass()),
-      value_(value->Pass()) {}
-
-WebRequestConditionAttributeResponseHeaders::HeaderMatchTest::~HeaderMatchTest()
-{}
-
-WebRequestConditionAttributeResponseHeaders::
-WebRequestConditionAttributeResponseHeaders(
-    bool positive_test, ScopedVector<const HeaderMatchTest>* tests)
-    : tests_(tests->Pass()),
-      positive_test_(positive_test) {}
-
-WebRequestConditionAttributeResponseHeaders::
-~WebRequestConditionAttributeResponseHeaders() {}
+// Manages a set of tests to be applied to name-value pairs representing
+// headers. This is a helper class to header-related condition attributes.
+// It contains a set of test groups. A name-value pair satisfies the whole
+// set of test groups iff it passes at least one test group.
+class HeaderMatcher {
+ public:
+  ~HeaderMatcher();
+
+  // Creates an instance based on a list |tests| of test groups, encoded as
+  // dictionaries of the type declarativeWebRequest.HeaderFilter (see
+  // declarative_web_request.json).
+  static scoped_ptr<const HeaderMatcher> Create(const base::ListValue* tests);
+
+  // Does |this| match the header "|name|: |value|"?
+  bool TestNameValue(const std::string& name, const std::string& value) const;
+
+ private:
+  // Represents a single string-matching test.
+  class StringMatchTest {
+   public:
+    enum MatchType { kPrefix, kSuffix, kEquals, kContains };
+
+    StringMatchTest(const std::string& data, MatchType type);
+    ~StringMatchTest();
+
+    // Takes a string to be matched, as a StringValue in |content|, an
+    // indication |is_name_test| of whether the test will be used for matching
+    // against header values or names, and a |match_type|. Never returns NULL,
+    // except for memory failures.
+    static scoped_ptr<const StringMatchTest> Create(const Value* content,
+                                                    bool is_name_test,
+                                                    MatchType match_type);

[battre, 2012/09/03 08:46:47]

You can do this in a separate CL, but I think we can clean this up by removing
is_name_test. That way StringMatchTest does not need to know about the concept
of key/value paris of headers. Also the StringToLowerASCII() would be
constrained to a single class.

[vabr (Chromium), 2012/09/03 10:12:36]

I thought about this before, but decided against it. This way I can pass
StringValue |content| to Create, instead of std::string, and only write the
StringValue->std::string conversion code in Create once, instead of twice in its
callers.
If you think having StringToLowerASCII() constrained to a single class is of
greater benefit than non-repeating the code for StringValue->std::string
conversion, please let me know why, and I will consider that.

[battre, 2012/09/03 12:48:08]

My thoughts were from the perspective of somebody who reads the code for the
first time:
- A StringMatchTest should match strings, not parts of headers.
- I am not very fond of the asymmetry of the StringToLowerASCII() calls. For
Matches() we expect that it is the responsibility of the caller to call it (but
only for headers) and for Create() this class takes care.

I think that moving StringToLowerASCII out of StringMatchTest would be cleaner.

[vabr (Chromium), 2012/09/03 14:21:29]

I agree with both points. StringToLowerASCII is not used any more at all.
Surprisingly :), I moved the case-insensitive logic all to StringMatchTest,
which now has a construction flag to be case_sensitive or not. The class itself
knows nothing about headers vs. values, only case_sensitive or not. That flag is
set on construction in HeaderMatcher::HeaderMatchTest::Create, based on whether
that test is for header name or values.
I hope that this solution satisfies both the points you raised.

I also checked that the functions used inside
HeaderMatcher::StringMatchTest::Matches do not create copies of the strings they
compare -- the StartsWith/EndsWith use strcasecmp, and the std::search compares
byte per byte, so neither memory nor speed overhead should be too much.

+
+    // Does |str| pass |this| StringMatchTest?
+    bool Matches(const std::string& str) const;
+
+   private:
+    const std::string data_;
+    const MatchType type_;
+    DISALLOW_COPY_AND_ASSIGN(StringMatchTest);
+  };
+
+  // Represents a test group -- a set of string matching tests to be applied to
+  // both the header name and value.
+  class HeaderMatchTest {
+   public:
+    ~HeaderMatchTest();
+
+    // Gets the test group description in |tests| and creates the corresponding
+    // HeaderMatchTest. On failure returns NULL.
+    static scoped_ptr<const HeaderMatchTest> Create(
+        const base::DictionaryValue* tests);
+
+    // Does the header "|name|: |value|" match all tests in |this|?
+    bool Matches(const std::string& name, const std::string& value) const;
+
+   private:
+    // Takes ownership of the content of both |name_match| and |value_match|.
+    HeaderMatchTest(ScopedVector<const StringMatchTest>* name_match,
+                    ScopedVector<const StringMatchTest>* value_match);
+
+    // Tests to be passed by a header's name.
+    const ScopedVector<const StringMatchTest> name_match_;
+    // Tests to be passed by a header's value.
+    const ScopedVector<const StringMatchTest> value_match_;
+    DISALLOW_COPY_AND_ASSIGN(HeaderMatchTest);
+  };
+
+  explicit HeaderMatcher(ScopedVector<const HeaderMatchTest>* tests);
+
+  const ScopedVector<const HeaderMatchTest> tests_;
+
+  DISALLOW_COPY_AND_ASSIGN(HeaderMatcher);
+};
+
+// HeaderMatcher implementation.
+
+HeaderMatcher::~HeaderMatcher() {}
 
 // static
-bool WebRequestConditionAttributeResponseHeaders::IsMatchingType(
-    const std::string& instance_type) {
-  return instance_type == keys::kResponseHeadersKey ||
-      instance_type == keys::kExcludeResponseHeadersKey;
-}
-
-// static
-scoped_ptr<WebRequestConditionAttribute>
-WebRequestConditionAttributeResponseHeaders::Create(
-    const std::string& name,
-    const base::Value* value,
-    std::string* error) {
-  DCHECK(IsMatchingType(name));
-
-  const ListValue* value_as_list = NULL;
-  if (!value->GetAsList(&value_as_list)) {
-    *error = ExtensionErrorUtils::FormatErrorMessage(kInvalidValue, name);
-    return scoped_ptr<WebRequestConditionAttribute>(NULL);
-  }
-
+scoped_ptr<const HeaderMatcher> HeaderMatcher::Create(
+    const base::ListValue* tests) {
   ScopedVector<const HeaderMatchTest> header_tests;
-  for (ListValue::const_iterator it = value_as_list->begin();
-       it != value_as_list->end(); ++it) {
+  for (ListValue::const_iterator it = tests->begin();
+       it != tests->end(); ++it) {
     const DictionaryValue* tests = NULL;
-    if (!(*it)->GetAsDictionary(&tests)) {
-      *error = ExtensionErrorUtils::FormatErrorMessage(kInvalidValue, name);
-      return scoped_ptr<WebRequestConditionAttribute>(NULL);
-    }
+    if (!(*it)->GetAsDictionary(&tests))
+      return scoped_ptr<const HeaderMatcher>(NULL);
 
     scoped_ptr<const HeaderMatchTest> header_test(
-        CreateHeaderMatchTest(tests, error));
+        HeaderMatchTest::Create(tests));
     if (header_test.get() == NULL)
-      return scoped_ptr<WebRequestConditionAttribute>(NULL);
+      return scoped_ptr<const HeaderMatcher>(NULL);
     header_tests.push_back(header_test.release());
   }
 
-  scoped_ptr<WebRequestConditionAttributeResponseHeaders> result;
-  result.reset(new WebRequestConditionAttributeResponseHeaders(
-      name == keys::kResponseHeadersKey, &header_tests));
-
-  return result.PassAs<WebRequestConditionAttribute>();
+  return scoped_ptr<const HeaderMatcher>(
+      new HeaderMatcher(&header_tests));

[battre, 2012/09/03 08:46:47]

nit: single line

[vabr (Chromium), 2012/09/03 10:12:36]

Done.

 }
 
-int WebRequestConditionAttributeResponseHeaders::GetStages() const {
-  return ON_HEADERS_RECEIVED;
+bool HeaderMatcher::TestNameValue(const std::string& name,
+                                  const std::string& value) const {
+  for (size_t i = 0; i < tests_.size(); ++i) {
+    if (tests_[i]->Matches(name, value))
+      return true;
+  }
+  return false;
 }
 
-bool WebRequestConditionAttributeResponseHeaders::IsFulfilled(
-    const WebRequestRule::RequestData& request_data) const {
-  if (!(request_data.stage & GetStages()))
-    return false;
+HeaderMatcher::HeaderMatcher(ScopedVector<const HeaderMatchTest>* tests)
+  : tests_(tests->Pass()) {}
 
-  const net::HttpResponseHeaders* headers =
-      request_data.original_response_headers;
-  if (headers == NULL) {
-    // Each header of an empty set satisfies (the negation of) everything;
-    // OTOH, there is no header to satisfy even the most permissive test.
-    return !positive_test_;
-  }
+// HeaderMatcher::StringMatchTest implementation.
 
-  // Has some header already passed some header test?
-  bool header_found = false;
+HeaderMatcher::StringMatchTest::StringMatchTest(const std::string& data,
+                                                MatchType type)
+    : data_(data),
+      type_(type) {}
 
-  for (size_t i = 0; !header_found && i < tests_.size(); ++i) {
-    std::string name;
-    std::string value;
+HeaderMatcher::StringMatchTest::~StringMatchTest() {}
 
-    void* iter = NULL;
-    while (!header_found &&
-           headers->EnumerateHeaderLines(&iter, &name, &value)) {
-      StringToLowerASCII(&name);  // Header names are case-insensitive.
-      header_found |= tests_[i]->Matches(name, value);
-    }
-  }
+// static
+scoped_ptr<const HeaderMatcher::StringMatchTest>
+HeaderMatcher::StringMatchTest::Create(const Value* content,
+                                       bool is_name_test,
+                                       MatchType match_type) {
+  std::string str;
 
-  return (positive_test_ ? header_found : !header_found);
-}
+  CHECK(content->GetAsString(&str));
+  // Header names are case-insensitive, we match them in lowercase.
+  if (is_name_test)
+    StringToLowerASCII(&str);
 
-WebRequestConditionAttribute::Type
-WebRequestConditionAttributeResponseHeaders::GetType() const {
-  return CONDITION_RESPONSE_HEADERS;
+  return scoped_ptr<const StringMatchTest>(
+      new StringMatchTest(str, match_type));
 }
 
-bool WebRequestConditionAttributeResponseHeaders::StringMatchTest::Matches(
+bool HeaderMatcher::StringMatchTest::Matches(
     const std::string& str) const {
   switch (type_) {
     case kPrefix:
@@ -358,65 +380,56 @@ bool WebRequestConditionAttributeResponseHeaders::StringMatchTest::Matches(
       return data_ == str;
     case kContains:
       return str.find(data_) != std::string::npos;
+    default:
+      // We never go here, all cases done above. Just to silence the compiler.
+      NOTREACHED();
+      return false;

[battre, 2012/09/03 08:46:47]

I disagree with this change. This makes the compile-time check whether all enum
values are covered a runtime ceck.

[vabr (Chromium), 2012/09/03 10:12:36]

You're right, reverted.
I guess I need to learn to be more assertive against review arguments involving
"usually" as the only reason. :)

   }
-  // We never get past the "switch", but the compiler worries about no return.
-  NOTREACHED();
-  return false;
 }
 
-bool WebRequestConditionAttributeResponseHeaders::HeaderMatchTest::Matches(
-    const std::string& name,
-    const std::string& value) const {
-  for (size_t i = 0; i < name_.size(); ++i) {
-    if (!name_[i]->Matches(name))
-      return false;
-  }
+// HeaderMatcher::HeaderMatchTest implementation.
 
-  for (size_t i = 0; i < value_.size(); ++i) {
-    if (!value_[i]->Matches(value))
-      return false;
-  }
-
-  return true;
-}
+HeaderMatcher::HeaderMatchTest::HeaderMatchTest(
+    ScopedVector<const StringMatchTest>* name_match,
+    ScopedVector<const StringMatchTest>* value_match)
+    : name_match_(name_match->Pass()),
+      value_match_(value_match->Pass()) {}
 
+HeaderMatcher::HeaderMatchTest::~HeaderMatchTest() {}
 
 // static
-scoped_ptr<const WebRequestConditionAttributeResponseHeaders::HeaderMatchTest>
-WebRequestConditionAttributeResponseHeaders::CreateHeaderMatchTest(
-    const DictionaryValue* tests,
-    std::string* error) {
-  ScopedVector<const StringMatchTest> name;
-  ScopedVector<const StringMatchTest> value;
+scoped_ptr<const HeaderMatcher::HeaderMatchTest>
+HeaderMatcher::HeaderMatchTest::Create(const base::DictionaryValue* tests) {
+  ScopedVector<const StringMatchTest> name_match;
+  ScopedVector<const StringMatchTest> value_match;
 
   for (DictionaryValue::key_iterator key = tests->begin_keys();
        key != tests->end_keys();
        ++key) {
     bool is_name = false;  // Is this test for header name?
-    MatchType match_type;
+    StringMatchTest::MatchType match_type;
     if (*key == keys::kNamePrefixKey) {
       is_name = true;
-      match_type = kPrefix;
+      match_type = StringMatchTest::kPrefix;
     } else if (*key == keys::kNameSuffixKey) {
       is_name = true;
-      match_type = kSuffix;
+      match_type = StringMatchTest::kSuffix;
     } else if (*key == keys::kNameContainsKey) {
       is_name = true;
-      match_type = kContains;
+      match_type = StringMatchTest::kContains;
     } else if (*key == keys::kNameEqualsKey) {
       is_name = true;
-      match_type = kEquals;
+      match_type = StringMatchTest::kEquals;
     } else if (*key == keys::kValuePrefixKey) {
-      match_type = kPrefix;
+      match_type = StringMatchTest::kPrefix;
     } else if (*key == keys::kValueSuffixKey) {
-      match_type = kSuffix;
+      match_type = StringMatchTest::kSuffix;
     } else if (*key == keys::kValueContainsKey) {
-      match_type = kContains;
+      match_type = StringMatchTest::kContains;
     } else if (*key == keys::kValueEqualsKey) {
-      match_type = kEquals;
+      match_type = StringMatchTest::kEquals;
     } else {
       NOTREACHED();  // JSON schema type checking should prevent this.
-      *error = ExtensionErrorUtils::FormatErrorMessage(kInvalidValue, *key);

[battre, 2012/09/03 08:46:47]

why did you remove this?

[vabr (Chromium), 2012/09/03 10:12:36]

In all situations where creating a HeaderMatchTest (and the whole HeaderMatcher,
in fact) reported and error, kInvalidValue was reported. So I removed the
dependency of this create method on the |error| argument and report it in the
create method of the condition attribute instead. The output of the code will
not change by this.

[battre, 2012/09/03 12:48:08]

Thanks. Makes sense.

       return scoped_ptr<const HeaderMatchTest>(NULL);
     }
     const Value* content = NULL;
@@ -429,43 +442,125 @@ WebRequestConditionAttributeResponseHeaders::CreateHeaderMatchTest(
         CHECK(content->GetAsList(&list));
         for (ListValue::const_iterator it = list->begin();
              it != list->end(); ++it) {
-          ScopedVector<const StringMatchTest>* tests = is_name ? &name : &value;
+          ScopedVector<const StringMatchTest>* tests =
+              is_name ? &name_match : &value_match;
           tests->push_back(
-              CreateStringMatchTest(*it, is_name, match_type).release());
+              StringMatchTest::Create(*it, is_name, match_type).release());
         }
         break;
       }
       case Value::TYPE_STRING: {
-        ScopedVector<const StringMatchTest>* tests = is_name ? &name : &value;
+        ScopedVector<const StringMatchTest>* tests =
+            is_name ? &name_match : &value_match;
         tests->push_back(
-            CreateStringMatchTest(content, is_name, match_type).release());
+            StringMatchTest::Create(content, is_name, match_type).release());
         break;
       }
       default: {
         NOTREACHED();  // JSON schema type checking should prevent this.
-        *error = ExtensionErrorUtils::FormatErrorMessage(kInvalidValue, *key);

[battre, 2012/09/03 08:46:47]

why did you remove this?

[vabr (Chromium), 2012/09/03 10:12:36]

Please see above the explanation for HeaderMatchTest, it applies here as well.

         return scoped_ptr<const HeaderMatchTest>(NULL);
       }
     }
   }
 
-  return scoped_ptr<const HeaderMatchTest>(new HeaderMatchTest(&name, &value));
+  return scoped_ptr<const HeaderMatchTest>(
+      new HeaderMatchTest(&name_match, &value_match));
 }
 
+bool HeaderMatcher::HeaderMatchTest::Matches(const std::string& name,
+                                             const std::string& value) const {
+  for (size_t i = 0; i < name_match_.size(); ++i) {
+    if (!name_match_[i]->Matches(name))
+      return false;
+  }
+
+  for (size_t i = 0; i < value_match_.size(); ++i) {
+    if (!value_match_[i]->Matches(value))
+      return false;
+  }
+
+  return true;
+}
+
+//
+// WebRequestConditionAttributeResponseHeaders
+//
+
+WebRequestConditionAttributeResponseHeaders::
+WebRequestConditionAttributeResponseHeaders(
+    scoped_ptr<const HeaderMatcher>* header_matcher, bool positive)

[battre, 2012/09/03 08:46:47]

nit: bool positive goes into next line (either all parameters in one line or one
per line)

[vabr (Chromium), 2012/09/03 10:12:36]

Done.

+    : header_matcher_(header_matcher->Pass()),
+      positive_(positive) {}
+
+WebRequestConditionAttributeResponseHeaders::
+~WebRequestConditionAttributeResponseHeaders() {}
+
 // static
-scoped_ptr<const WebRequestConditionAttributeResponseHeaders::StringMatchTest>
-WebRequestConditionAttributeResponseHeaders::CreateStringMatchTest(
-    const Value* content,
-    bool is_name_test,
-    MatchType match_type) {
-  std::string str;
+bool WebRequestConditionAttributeResponseHeaders::IsMatchingType(
+    const std::string& instance_type) {
+  return instance_type == keys::kResponseHeadersKey ||
+      instance_type == keys::kExcludeResponseHeadersKey;
+}
 
-  CHECK(content->GetAsString(&str));
-  if (is_name_test)  // Header names are case-insensitive.
-    StringToLowerASCII(&str);
+// static
+scoped_ptr<WebRequestConditionAttribute>
+WebRequestConditionAttributeResponseHeaders::Create(
+    const std::string& name,
+    const base::Value* value,
+    std::string* error) {
+  DCHECK(IsMatchingType(name));
 
-  return scoped_ptr<const StringMatchTest>(
-      new StringMatchTest(str, match_type));
+  const ListValue* value_as_list = NULL;
+  if (!value->GetAsList(&value_as_list)) {
+    *error = ExtensionErrorUtils::FormatErrorMessage(kInvalidValue, name);
+    return scoped_ptr<WebRequestConditionAttribute>(NULL);
+  }
+
+  scoped_ptr<const HeaderMatcher> header_matcher(
+      HeaderMatcher::Create(value_as_list));
+  if (header_matcher.get() == NULL) {
+    *error = ExtensionErrorUtils::FormatErrorMessage(kInvalidValue, name);
+    return scoped_ptr<WebRequestConditionAttribute>(NULL);
+  }
+
+  const bool positive = name == keys::kResponseHeadersKey;

[battre, 2012/09/03 08:46:47]

nit: no need for const

[vabr (Chromium), 2012/09/03 10:12:36]

I disagree. I want to document that |positive| is just an abbreviation for its
definition. I am not sure whether the compiler can optimise the code more based
on that, but the documentation purpose is good enough for me.
If you think there is a downside to declaring it const, please let me know.

[battre, 2012/09/03 12:48:08]

I think the downside is the additional amount of code to read. It is pretty
uncommon in the code to use const temporary variables. I don't feel strongly
about this, though. Feel free to leave it.

+  return scoped_ptr<WebRequestConditionAttribute>(
+      new WebRequestConditionAttributeResponseHeaders(
+          &header_matcher, positive));
+}
+
+int WebRequestConditionAttributeResponseHeaders::GetStages() const {
+  return ON_HEADERS_RECEIVED;
+}
+
+bool WebRequestConditionAttributeResponseHeaders::IsFulfilled(
+    const WebRequestRule::RequestData& request_data) const {
+  if (!(request_data.stage & GetStages()))
+    return false;
+
+  const net::HttpResponseHeaders* headers =
+      request_data.original_response_headers;
+  if (headers == NULL) {
+    // Each header of an empty set satisfies (the negation of) everything;
+    // OTOH, there is no header to satisfy even the most permissive test.
+    return !positive_;
+  }
+
+  bool passed = false;  // Did some header pass TestNameValue?
+  std::string name;
+  std::string value;
+  void* iter = NULL;
+  while (!passed && headers->EnumerateHeaderLines(&iter, &name, &value)) {
+    StringToLowerASCII(&name);  // Header names are case-insensitive.
+    passed |= header_matcher_->TestNameValue(name, value);
+  }
+
+  return (positive_ ? passed : !passed);
+}
+
+WebRequestConditionAttribute::Type
+WebRequestConditionAttributeResponseHeaders::GetType() const {
+  return CONDITION_RESPONSE_HEADERS;
 }
 
 }  // namespace extensions