Adding condition attributes for response headers to Declarative WebRequest

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

Index: chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc
diff --git a/chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc b/chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc
index c6027aac1c5ebc084db6ec11ec7a44d150cf75a1..be56701ef503654de337d6d0cba6c39dc5e22b82 100644
--- a/chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc
+++ b/chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.h"
 
+#include "base/basictypes.h"
 #include "base/file_path.h"
 #include "base/message_loop.h"
 #include "base/values.h"
@@ -14,6 +15,11 @@
 #include "net/test/test_server.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using base::DictionaryValue;
+using base::ListValue;
+using base::StringValue;
+using base::Value;
+
 namespace {
 const char kUnknownConditionName[] = "unknownType";
 }  // namespace
@@ -156,4 +162,232 @@ TEST(WebRequestConditionAttributeTest, ContentType) {
                                   url_request.response_headers())));
 }
 
+namespace {
+
+// Builds a vector of vectors of string pointers from an array of strings.
+// |array| is in fact a sequence of arrays. The array |sizes| captures the sizes
+// of all parts of |array|, and |size| is the length of |sizes| itself.
+// Example (this is pseudo-code, not C++):
+// array = { "a", "b", "c", "d", "e", "f" }
+// sizes = { 2, 0, 4 }
+// size = 3
+// results in out == { {&"a", &"b"}, {}, {&"c", &"d", &"e", &"f"} }
+void GetArrayAsVector(const std::string array[],
+                      const size_t sizes[],
+                      const size_t size,
+                      std::vector< std::vector<const std::string*> >* out) {
+  out->clear();
+  size_t next = 0;
+  for (size_t i = 0; i < size; ++i) {
+    out->push_back(std::vector<const std::string*>());
+    for (size_t j = next; j < next + sizes[i]; ++j) {
+      out->back().push_back(&(array[j]));
+    }
+    next += sizes[i];
+  }
+}
+
+// Builds a DictionaryValue from an array of the form {name1, value1, name2,
+// value2, ...}. Values for the same key are grouped in a ListValue.
+scoped_ptr<DictionaryValue> GetDictionaryFromArray(
+    const std::vector<const std::string*>& array) {
+  const size_t length = array.size();
+  if (length % 2 == 1)

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

I think you can just CHECK() this.

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

Done.

+    return scoped_ptr<DictionaryValue>(NULL);
+
+  scoped_ptr<DictionaryValue> dictionary(new DictionaryValue);
+  for (size_t i = 0; i < length; i += 2) {
+    const std::string* name = array[i];
+    const std::string* value = array[i+1];
+    if (dictionary->HasKey(*name)) {
+      Value* entry = NULL;
+      ListValue* list = NULL;
+      if (!dictionary->GetWithoutPathExpansion(*name, &entry))
+        return scoped_ptr<DictionaryValue>(NULL);
+      switch (entry->GetType()) {
+        case Value::TYPE_STRING:  // Replace the present string with a list.
+          list = new ListValue;
+          // Ignoring return value, we already verified the entry is there.
+          dictionary->RemoveWithoutPathExpansion(*name, &entry);
+          list->Append(entry);
+          list->Append(Value::CreateStringValue(*value));
+          dictionary->SetWithoutPathExpansion(*name, list);
+          break;
+        case Value::TYPE_LIST:  // Just append to the list.
+          if (!entry->GetAsList(&list))

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

CHECK?

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

Done.

+            return scoped_ptr<DictionaryValue>(NULL);
+          list->Append(Value::CreateStringValue(*value));
+          break;
+        default:
+          NOTREACHED();  // We never put other Values here.
+          return scoped_ptr<DictionaryValue>(NULL);
+      }
+    } else {
+      dictionary->SetString(*name, *value);
+    }
+  }
+  return dictionary.Pass();
+}
+
+// Returns how the response headers from |url_request| satisfy the match
+// criteria given in |tests|. For at least one |i| all tests from |tests[i]|
+// must pass. If |positive_test| is true, the dictionary is interpreted as the
+// containsHeaders property of a RequestMatcher, otherwise as
+// doesNotContainHeaders.
+void MatchAndCheck(const std::vector< std::vector<const std::string*> >& tests,
+                   net::URLRequest* url_request,
+                   const bool positive_test,

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

nit: we usually don't pass atomic types as const parameters.

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

Actually I got rid of the intermediate boolean an replaced it with the key
(which it chose originally) directly.

+                   bool* result) {

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

return the result instead?

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

Can't do that, using ASSERT_* requires void return type.

+  ListValue contains_headers;
+  for (size_t i = 0; i < tests.size(); ++i) {
+    scoped_ptr<DictionaryValue> temp(GetDictionaryFromArray(tests[i]));
+    ASSERT_TRUE(temp.get() != NULL);

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

is the != NULL necessary?

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

It is not necessary :).
I tend to write it because I find it more readable.

I guess this is a matter of personal opinion, as neither C++ nor the style guide
dictates how to use the implicit conversion pointer->boolean.

You are the owner of this code, so I'm happy to strip "!= NULL" from this and
other occurrences if you insist.

+    contains_headers.Append(temp.release());
+  }
+
+  std::string error;
+  const std::string key(positive_test ?
+                        keys::kContainsHeadersKey :
+                        keys::kDoesNotContainHeadersKey);
+  scoped_ptr<WebRequestConditionAttribute> attribute =
+      WebRequestConditionAttribute::Create(key, &contains_headers, &error);
+  ASSERT_EQ("", error);
+  ASSERT_TRUE(attribute.get() != NULL);
+
+  *result = attribute->IsFulfilled(WebRequestRule::RequestData(
+      url_request, ON_HEADERS_RECEIVED, url_request->response_headers()));
+}
+
+}  // namespace
+
+// Here we test WebRequestConditionAttributeContainsHeaders for:
+// 1. Correct implementation of prefix/suffix/contains/equals matching.
+// 2. Performing logical disjunction (||) between multiple specifications.
+// 3. Negating the match in case of 'doesNotContainHeaders'.
+TEST(WebRequestConditionAttributeTest, Headers) {
+  // Necessary for TestURLRequest.
+  MessageLoop message_loop(MessageLoop::TYPE_IO);
+
+  net::TestServer test_server(
+      net::TestServer::TYPE_HTTP,
+      net::TestServer::kLocalhost,
+      FilePath(FILE_PATH_LITERAL(
+          "chrome/test/data/extensions/api_test/webrequest/declarative")));
+  ASSERT_TRUE(test_server.Start());
+
+  TestURLRequestContext context;
+  TestDelegate delegate;
+  TestURLRequest url_request(test_server.GetURL("files/headers.html"),
+                             &delegate, &context);
+  url_request.Start();
+  MessageLoop::current()->Run();
+
+  // In all the tests below we assume that the server includes the header
+  // "Custom-Header: custom/value" and "Custom-Header-B: valueA, valueB" in the
+  // response, but does not include "Non-existing: void".
+
+  std::vector< std::vector<const std::string*> > tests;
+  bool result;
+
+  // 1.a. -- All these tests should pass.
+  const std::string kPassingCondition[] = {
+    keys::kNamePrefixKey, "Custom",
+    keys::kNameSuffixKey, "m-header",  // Header names are case insensitive.
+    keys::kValueContainsKey, "alu",
+    keys::kValueEqualsKey, "custom/value"
+  };
+  const size_t kPassingConditionSizes[] = { arraysize(kPassingCondition) };
+  GetArrayAsVector(kPassingCondition, kPassingConditionSizes, 1u, &tests);
+  MatchAndCheck(tests, &url_request, true /*positive_test*/, &result);
+  EXPECT_TRUE(result);
+
+  // 1.b. -- None of the following tests in the discjunction should pass.
+  const std::string kFailCondition[] = {
+    keys::kNamePrefixKey, " Custom",  // Test 1.
+    keys::kNameContainsKey, " -",     // Test 2.
+    keys::kValueSuffixKey, "alu",     // Test 3.
+    keys::kValueEqualsKey, "custom"   // Test 4.

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

how about a test for case sensitivity in the value?

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

Good idea! Done as 1.h.

+  };
+  const size_t kFailConditionSizes[] = { 2u, 2u, 2u, 2u };
+  GetArrayAsVector(kFailCondition, kFailConditionSizes, 4u, &tests);
+  MatchAndCheck(tests, &url_request, true /*positive_test*/, &result);
+  EXPECT_FALSE(result);
+
+  // 1.c. -- This should fail (mixing name and value from different headers)
+  const std::string kMixingCondition[] = {
+    keys::kNameSuffixKey, "Header-B",
+    keys::kValueEqualsKey, "custom/value"
+  };
+  const size_t kMixingConditionSizes[] = { arraysize(kMixingCondition) };
+  GetArrayAsVector(kMixingCondition, kMixingConditionSizes, 1u, &tests);
+  MatchAndCheck(tests, &url_request, true /*positive_test*/, &result);
+  EXPECT_FALSE(result);
+
+  // 1.d. -- Test handling multiple values for one header (both should pass).
+  const std::string kMoreValues1[] = {
+    keys::kNameEqualsKey, "Custom-header-b",
+    keys::kValueEqualsKey, "valueA"

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

Please see my comment in headers.html.mock-http-headers

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

Done.

+  };
+  const size_t kMoreValues1Sizes[] = { arraysize(kMoreValues1) };
+  GetArrayAsVector(kMoreValues1, kMoreValues1Sizes, 1u, &tests);
+  MatchAndCheck(tests, &url_request, true /*positive_test*/, &result);
+  EXPECT_TRUE(result);
+  const std::string kMoreValues2[] = {
+    keys::kNameEqualsKey, "Custom-header-b",
+    keys::kValueEqualsKey, "valueB"
+  };
+  const size_t kMoreValues2Sizes[] = { arraysize(kMoreValues2) };
+  GetArrayAsVector(kMoreValues2, kMoreValues2Sizes, 1u, &tests);
+  MatchAndCheck(tests, &url_request, true /*positive_test*/, &result);
+  EXPECT_TRUE(result);
+
+  // 1.e. -- This should fail as conjunction but pass as disjunction.
+  const std::string kConflict[] = {
+    keys::kNameSuffixKey, "Header",    // True for some header.
+    keys::kNameSuffixKey, "Header-B"   // True for a different header.
+  };
+  // First disjunction, no conflict.
+  const size_t kNoConflictSizes[] = { 2u, 2u };
+  GetArrayAsVector(kConflict, kNoConflictSizes, 2u, &tests);
+  MatchAndCheck(tests, &url_request, true /*positive_test*/, &result);
+  EXPECT_TRUE(result);
+  // Then conjunction, conflict.
+  const size_t kConflictSizes[] = { arraysize(kConflict) };
+  GetArrayAsVector(kConflict, kConflictSizes, 1u, &tests);
+  MatchAndCheck(tests, &url_request, true /*positive_test*/, &result);
+  EXPECT_FALSE(result);
+
+  // 2.a. -- This should pass as disjunction, because one of the tests passes.
+  const std::string kDisjunction[] = {
+    keys::kNamePrefixKey, "Non-existing",  // This one fails.
+    keys::kNameSuffixKey, "Non-existing",  // This one fails.
+    keys::kValueEqualsKey, "void",         // This one fails.
+    keys::kValueContainsKey, "alu"         // This passes.
+  };
+  const size_t kDisjunctionSizes[] = { 2u, 2u, 2u, 2u };
+  GetArrayAsVector(kDisjunction, kDisjunctionSizes, 4u, &tests);
+  MatchAndCheck(tests, &url_request, true /*positive_test*/, &result);
+  EXPECT_TRUE(result);
+
+  // 3.a. -- This should pass.
+  const std::string kNonExistent[] = {
+    keys::kNameEqualsKey, "Non-existing",
+    keys::kValueEqualsKey, "void"
+  };
+  const size_t kNonExistentSizes[] = { arraysize(kNonExistent) };
+  GetArrayAsVector(kNonExistent, kNonExistentSizes, 1u, &tests);
+  MatchAndCheck(tests, &url_request, false /*positive_test*/, &result);
+  EXPECT_TRUE(result);
+
+  // 3.b. -- This should fail.
+  const std::string kExisting[] = {
+    keys::kNameEqualsKey, "custom-header-b",
+    keys::kValueEqualsKey, "valueB"
+  };
+  const size_t kExistingSize[] = { arraysize(kExisting) };
+  GetArrayAsVector(kExisting, kExistingSize, 1u, &tests);
+  MatchAndCheck(tests, &url_request, false /*positive_test*/, &result);
+  EXPECT_FALSE(result);
+}
+
 }  // namespace extensions