Document and test case sensitivity in URL matcher attributes

chrome/common/extensions/matcher/url_matcher_factory_unittest.cc

Index: chrome/common/extensions/matcher/url_matcher_factory_unittest.cc
diff --git a/chrome/common/extensions/matcher/url_matcher_factory_unittest.cc b/chrome/common/extensions/matcher/url_matcher_factory_unittest.cc
index aa2a76ca0ed2a920c2fa8cb11270d3868e4d5345..62d81ec8ffa9baf21ff7c427820fc8ab35db89bd 100644
--- a/chrome/common/extensions/matcher/url_matcher_factory_unittest.cc
+++ b/chrome/common/extensions/matcher/url_matcher_factory_unittest.cc
@@ -99,4 +99,147 @@ TEST(URLMatcherFactoryTest, CreateFromURLFilterDictionary) {
   EXPECT_EQ(0u, matcher.MatchURL(GURL("http://mail.example.com:81")).size());
 }
 
+// This class wraps a case sensitivity test for a single UrlFilter condition.
+class UrlConditionCaseTest {
+ public:
+  // The condition is identified by the key |condition_key|. If that key is
+  // associated with string values, then |use_list_of_strings| should be false,
+  // if the key is associated with list-of-string values, then
+  // |use_list_of_strings| should be true. In |url| is the URL to test against.
+  UrlConditionCaseTest(const char* condition_key,
+                       const bool use_list_of_strings,
+                       const std::string& expected_value,
+                       const std::string& incorrect_case_value,
+                       const bool case_sensitive,
+                       const GURL& url)
+      : condition_key_(condition_key),
+        use_list_of_strings_(use_list_of_strings),
+        expected_value_(expected_value),
+        incorrect_case_value_(incorrect_case_value),
+        case_sensitive_(case_sensitive),
+        url_(url) {}
+
+  ~UrlConditionCaseTest() {}
+
+  // Match the condition against |url_|. Checks via EXPECT_* macros that
+  // |expected_value_| matches always, and that |incorrect_case_value_| matches
+  // iff |case_sensitive_| is false.
+  void Test() const;
+
+ private:
+  // Check, via EXPECT_* macros, that the the condition |condition_key_|=|value|
+  // fails against |url_| iff |should_fail| is true. This check is expensive,
+  // its value should be cached if needed multiple times.
+  void CheckCondition(const std::string& value, bool should_fail) const;
+
+  const char* condition_key_;
+  const bool use_list_of_strings_;
+  const std::string& expected_value_;
+  const std::string& incorrect_case_value_;
+  const bool case_sensitive_;
+  const GURL& url_;
+};

[vabr (Chromium), 2013/01/07 18:33:48]

No DISALLOW_COPY_AND_ASSIGN here, because a public copy constructor is needed
for the definition of |case_tests| below to compile. The copy-con is never used,
though.

[battre, 2013/01/07 19:13:01]

How about adding a comment of the style

// Allow implicit copy and assign because ...

[vabr (Chromium), 2013/01/08 16:52:47]

Done.

+
+void UrlConditionCaseTest::Test() const {
+  CheckCondition(expected_value_, false);
+  CheckCondition(incorrect_case_value_, case_sensitive_);
+}
+
+void UrlConditionCaseTest::CheckCondition(const std::string& value,
+                                          bool should_fail) const {
+  DictionaryValue condition;
+  if (use_list_of_strings_) {
+    ListValue* list = new ListValue();
+    list->Append(Value::CreateStringValue(value));
+    condition.SetWithoutPathExpansion(condition_key_, list);
+  } else {
+    condition.SetStringWithoutPathExpansion(condition_key_, value);
+  }
+
+  URLMatcher matcher;
+  std::string error;
+  scoped_refptr<URLMatcherConditionSet> result;
+
+  error.clear();

[battre, 2013/01/07 19:13:01]

nit: I would remove this line.

[vabr (Chromium), 2013/01/08 16:52:47]

Done.

+  result = URLMatcherFactory::CreateFromURLFilterDictionary(
+      matcher.condition_factory(), &condition, 1, &error);
+  EXPECT_EQ("", error);
+  ASSERT_TRUE(result.get());
+
+  URLMatcherConditionSet::Vector conditions;
+  conditions.push_back(result);
+  matcher.AddConditionSets(conditions);
+  EXPECT_EQ((should_fail ? 0u : 1u), matcher.MatchURL(url_).size())
+      << "while matching condition " << condition_key_ << " with value "
+      << value  << " against url " << url_;
+}
+
+// This tests that the UrlFilter handles case sensitivity on various parts of
+// URLs correctly.
+TEST(URLMatcherFactoryTest, CaseSensitivity) {
+  const std::string kScheme("https");
+  const std::string kSchemeUpper("HTTPS");
+  const std::string kHost("www.example.com");
+  const std::string kHostUpper("WWW.EXAMPLE.COM");
+  const std::string kPath("/path");
+  const std::string kPathUpper("/PATH");
+  const std::string kQuery("?option=value&A=B");
+  const std::string kQueryUpper("?OPTION=VALUE&A=B");
+  const std::string kUrl(kScheme + "://" + kHost + ":1234" + kPath + kQuery);
+  const std::string kUrlUpper(
+      kSchemeUpper + "://" + kHostUpper + ":1234" + kPathUpper + kQueryUpper);
+  const GURL url(kUrl);
+  // Note: according to RFC 3986, and RFC 1034, schema and host, respectively
+  // should be case insensitive. See crbug.com/160702, comments 6 and 7, for why
+  // we still require them to be case sensitive in UrlFilter.
+  const bool kIsSchemeCaseSensitive = true;
+  const bool kIsHostCaseSensitive = true;
+  const bool kIsPathCaseSensitive = true;
+  const bool kIsQueryCaseSensitive = true;
+  const bool kIsUrlCaseSensitive = kIsSchemeCaseSensitive ||
+      kIsHostCaseSensitive || kIsPathCaseSensitive || kIsQueryCaseSensitive;
+
+  const UrlConditionCaseTest case_tests[] = {
+    UrlConditionCaseTest(keys::kSchemesKey, true, kScheme, kSchemeUpper,
+                         kIsSchemeCaseSensitive, url),
+    UrlConditionCaseTest(keys::kHostContainsKey, false, kHost, kHostUpper,
+                         kIsHostCaseSensitive, url),
+    UrlConditionCaseTest(keys::kHostEqualsKey, false, kHost, kHostUpper,
+                         kIsHostCaseSensitive, url),
+    UrlConditionCaseTest(keys::kHostPrefixKey, false, kHost, kHostUpper,
+                         kIsHostCaseSensitive, url),
+    UrlConditionCaseTest(keys::kHostSuffixKey, false, kHost, kHostUpper,
+                         kIsHostCaseSensitive, url),
+    UrlConditionCaseTest(keys::kPathContainsKey, false, kPath, kPathUpper,
+                         kIsPathCaseSensitive, url),
+    UrlConditionCaseTest(keys::kPathEqualsKey, false, kPath, kPathUpper,
+                         kIsPathCaseSensitive, url),
+    UrlConditionCaseTest(keys::kPathPrefixKey, false, kPath, kPathUpper,
+                         kIsPathCaseSensitive, url),
+    UrlConditionCaseTest(keys::kPathSuffixKey, false, kPath, kPathUpper,
+                         kIsPathCaseSensitive, url),
+    UrlConditionCaseTest(keys::kQueryContainsKey, false, kQuery, kQueryUpper,
+                         kIsQueryCaseSensitive, url),
+    UrlConditionCaseTest(keys::kQueryEqualsKey, false, kQuery, kQueryUpper,
+                         kIsQueryCaseSensitive, url),
+    UrlConditionCaseTest(keys::kQueryPrefixKey, false, kQuery, kQueryUpper,
+                         kIsQueryCaseSensitive, url),
+    UrlConditionCaseTest(keys::kQuerySuffixKey, false, kQuery, kQueryUpper,
+                         kIsQueryCaseSensitive, url),
+    // Excluding kURLMatchesKey because case sensitivity can be specified in the
+    // RE2 expression.
+    UrlConditionCaseTest(keys::kURLContainsKey, false, kUrl, kUrlUpper,
+                         kIsUrlCaseSensitive, url),
+    UrlConditionCaseTest(keys::kURLEqualsKey, false, kUrl, kUrlUpper,
+                         kIsUrlCaseSensitive, url),
+    UrlConditionCaseTest(keys::kURLPrefixKey, false, kUrl, kUrlUpper,
+                         kIsUrlCaseSensitive, url),
+    UrlConditionCaseTest(keys::kURLSuffixKey, false, kUrl, kUrlUpper,
+                         kIsUrlCaseSensitive, url),
+  };
+
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(case_tests); ++i)

[battre, 2013/01/07 19:13:01]

Tip: You may add a line like the following:
SCOPED_TRACE(std::string("Iteration: ") + i);
to generate better failure messages, see
http://code.google.com/p/googletest/wiki/AdvancedGuide

[vabr (Chromium), 2013/01/08 16:52:47]

TIL, thanks!
Done.

On 2013/01/07 19:13:01, battre wrote:

+    case_tests[i].Test();
+}
+
 }  // namespace extensions