Declarative Web Request: firstPartyForCookiesUrl

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

Index: chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc
diff --git a/chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc b/chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc
index d584b10953ebe217b9fe5ac9d6c0d1fce69069a7..292fa135a56417e41b05f01832e1bac347baf042 100644
--- a/chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc
+++ b/chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc
@@ -4,8 +4,10 @@
 
 #include "chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h"
 
+#include <string>
 #include <vector>
 
+#include "base/basictypes.h"
 #include "base/json/json_reader.h"
 #include "base/memory/linked_ptr.h"
 #include "base/message_loop.h"
@@ -183,61 +185,25 @@ class WebRequestRulesRegistryTest : public testing::Test {
     return rule;
   }
 
-  // Create a condition with the attributes specified. An example value of a
-  // string from |attributes| is: "\"resourceType\": [\"stylesheet\"], \n".
-  linked_ptr<base::Value> CreateCondition(
-      const std::vector<const char *>& attributes) {
-    // Starting boilerplate.
+  // Create a condition with the attributes specified. An example value of
+  // |attributes| is: "\"resourceType\": [\"stylesheet\"], \n".
+  linked_ptr<base::Value> CreateCondition(const std::string& attributes) {
     std::string json_description =
         "{ \n"
         "  \"instanceType\": \"declarativeWebRequest.RequestMatcher\", \n";
-    for (size_t i = 0; i < attributes.size(); ++i) {
-      json_description += attributes[i];
-    }
-    // Ending boilerplate.
+    json_description += attributes;
     json_description += "}";
 
     return linked_ptr<base::Value>(
         base::test::ParseJson(json_description).release());
   }
 
-  // Create a rule with the ID |rule_id| and with a single condition and a
-  // cancelling action. The condition contains a non-matching non-URL attribute,
-  // and optionally, according to |with_url_attribute| also a URL attribute
-  // matching any (!) URL.
-  linked_ptr<RulesRegistry::Rule> CreateNonMatchingRule(
-      bool with_url_attribute,
-      const char* rule_id) {
-    std::vector<const char*> attributes;
-    if (with_url_attribute)
-      attributes.push_back("\"url\": { \"pathContains\": \"\" }, \n");
-
-    // The following attribute never matches in this unit test.
-    attributes.push_back("\"resourceType\": [\"stylesheet\"], \n");
-
-    DictionaryValue action_dict;
-    action_dict.SetString(keys::kInstanceTypeKey, keys::kCancelRequestType);
-
-    linked_ptr<RulesRegistry::Rule> rule(new RulesRegistry::Rule);
-    rule->id.reset(new std::string(rule_id));
-    rule->priority.reset(new int(1));
-    rule->actions.push_back(linked_ptr<base::Value>(action_dict.DeepCopy()));
-    rule->conditions.push_back(CreateCondition(attributes));
-    return rule;
-  }
-
-  // Create a rule with the ID |rule_id| and with two conditions and a
-  // cancelling action. One condition contains a non-matching non-URL attribute,
-  // and the other one a URL attribute matching any URL.
-  linked_ptr<RulesRegistry::Rule> CreateRuleWithTwoConditions(
-      const char* rule_id) {
-    std::vector<const char*> url_attributes;
-    url_attributes.push_back("\"url\": { \"pathContains\": \"\" }, \n");
-
-    // The following attribute never matches in this unit test.
-    std::vector<const char*> non_matching_attributes;
-    non_matching_attributes.push_back("\"resourceType\": [\"stylesheet\"], \n");
-
+  // Create a rule with the ID |rule_id| and with conditions created from the
+  // |attributes| specified (one entry one condition). An example value of a
+  // string from |attributes| is: "\"resourceType\": [\"stylesheet\"], \n".
+  linked_ptr<RulesRegistry::Rule> CreateCancellingRule(
+      const char* rule_id,
+      const std::vector<const std::string*>& attributes) {
     DictionaryValue action_dict;
     action_dict.SetString(keys::kInstanceTypeKey, keys::kCancelRequestType);
 
@@ -245,8 +211,10 @@ class WebRequestRulesRegistryTest : public testing::Test {
     rule->id.reset(new std::string(rule_id));
     rule->priority.reset(new int(1));
     rule->actions.push_back(linked_ptr<base::Value>(action_dict.DeepCopy()));
-    rule->conditions.push_back(CreateCondition(url_attributes));
-    rule->conditions.push_back(CreateCondition(non_matching_attributes));
+    for (std::vector<const std::string*>::const_iterator it =
+             attributes.begin();
+         it != attributes.end(); ++it)
+      rule->conditions.push_back(CreateCondition(**it));
     return rule;
   }
 
@@ -274,8 +242,8 @@ TEST_F(WebRequestRulesRegistryTest, AddRulesImpl) {
   GURL http_url("http://www.example.com");
   net::TestURLRequestContext context;
   net::TestURLRequest http_request(http_url, NULL, &context);
-  matches = registry->GetMatches(
-      DeclarativeWebRequestData(&http_request, ON_BEFORE_REQUEST));
+  WebRequestData request_data(&http_request, ON_BEFORE_REQUEST);
+  matches = registry->GetMatches(request_data);
   EXPECT_EQ(2u, matches.size());
 
   std::set<WebRequestRule::GlobalRuleId> matches_ids;
@@ -287,8 +255,8 @@ TEST_F(WebRequestRulesRegistryTest, AddRulesImpl) {
 
   GURL foobar_url("http://www.foobar.com");
   net::TestURLRequest foobar_request(foobar_url, NULL, &context);
-  matches = registry->GetMatches(
-      DeclarativeWebRequestData(&foobar_request, ON_BEFORE_REQUEST));
+  request_data.request = &foobar_request;
+  matches = registry->GetMatches(request_data);
   EXPECT_EQ(1u, matches.size());
   WebRequestRule::GlobalRuleId expected_pair =
       std::make_pair(kExtensionId, kRuleId2);
@@ -413,11 +381,9 @@ TEST_F(WebRequestRulesRegistryTest, Precedences) {
   GURL url("http://www.google.com");
   net::TestURLRequestContext context;
   net::TestURLRequest request(url, NULL, &context);
+  WebRequestData request_data(&request, ON_BEFORE_REQUEST);
   std::list<LinkedPtrEventResponseDelta> deltas =
-      registry->CreateDeltas(
-          NULL,
-          DeclarativeWebRequestData(&request, ON_BEFORE_REQUEST),
-          false);
+      registry->CreateDeltas(NULL, request_data, false);
 
   // The second extension is installed later and will win for this reason
   // in conflict resolution.
@@ -463,11 +429,9 @@ TEST_F(WebRequestRulesRegistryTest, Priorities) {
   GURL url("http://www.google.com/index.html");
   net::TestURLRequestContext context;
   net::TestURLRequest request(url, NULL, &context);
+  WebRequestData request_data(&request, ON_BEFORE_REQUEST);
   std::list<LinkedPtrEventResponseDelta> deltas =
-      registry->CreateDeltas(
-          NULL,
-          DeclarativeWebRequestData(&request, ON_BEFORE_REQUEST),
-          false);
+      registry->CreateDeltas(NULL, request_data, false);
 
   // The redirect by the first extension is ignored due to the ignore rule.
   ASSERT_EQ(1u, deltas.size());
@@ -484,19 +448,30 @@ TEST_F(WebRequestRulesRegistryTest, Priorities) {
 TEST_F(WebRequestRulesRegistryTest, GetMatchesCheckFulfilled) {
   scoped_refptr<TestWebRequestRulesRegistry> registry(
       new TestWebRequestRulesRegistry());
+  const std::string kMatchingUrlAttribute(
+      "\"url\": { \"pathContains\": \"\" }, \n");
+  const std::string kNonMatchingNonUrlAttribute(
+      "\"resourceType\": [\"stylesheet\"], \n");
+  const std::string kBothAttributes(kMatchingUrlAttribute +
+                                    kNonMatchingNonUrlAttribute);
   std::string error;
-
+  std::vector<const std::string*> attributes;
   std::vector<linked_ptr<RulesRegistry::Rule> > rules;
-  // Both rules have one condition, neither of them should fire.
-  // This rule's condition has only one, non-matching and non-URL attribute.
-  rules.push_back(CreateNonMatchingRule(false, kRuleId1));
-  // This rule's condition has two attributes: a matching URL, and a
-  // non-matching non-URL attribute.
-  rules.push_back(CreateNonMatchingRule(true, kRuleId2));
-
-  // The 3rd rule has two conditions, one with a matching URL attribute, and one
+
+  // Rules 1 and 2 have one condition, neither of them should fire.
+  attributes.push_back(&kNonMatchingNonUrlAttribute);
+  rules.push_back(CreateCancellingRule(kRuleId1, attributes));
+
+  attributes.clear();
+  attributes.push_back(&kBothAttributes);
+  rules.push_back(CreateCancellingRule(kRuleId2, attributes));
+
+  // Rule 3 has two conditions, one with a matching URL attribute, and one
   // with a non-matching non-URL attribute.
-  rules.push_back(CreateRuleWithTwoConditions(kRuleId3));
+  attributes.clear();
+  attributes.push_back(&kMatchingUrlAttribute);
+  attributes.push_back(&kNonMatchingNonUrlAttribute);
+  rules.push_back(CreateCancellingRule(kRuleId3, attributes));
 
   error = registry->AddRules(kExtensionId, rules);
   EXPECT_EQ("", error);
@@ -507,14 +482,70 @@ TEST_F(WebRequestRulesRegistryTest, GetMatchesCheckFulfilled) {
   GURL http_url("http://www.example.com");
   net::TestURLRequestContext context;
   net::TestURLRequest http_request(http_url, NULL, &context);
-  matches = registry->GetMatches(
-      DeclarativeWebRequestData(&http_request, ON_BEFORE_REQUEST));
+  WebRequestData request_data(&http_request, ON_BEFORE_REQUEST);
+  matches = registry->GetMatches(request_data);
   EXPECT_EQ(1u, matches.size());
   WebRequestRule::GlobalRuleId expected_pair = std::make_pair(kExtensionId,
                                                               kRuleId3);
   EXPECT_EQ(expected_pair, (*matches.begin())->id());
 }
 
+// Test that the url and firstPartyForCookiesUrl attributes are evaluated
+// against corresponding URLs. Tested on requests where these URLs actually
+// differ.
+TEST_F(WebRequestRulesRegistryTest, GetMatchesDifferentUrls) {
+  scoped_refptr<TestWebRequestRulesRegistry> registry(
+      new TestWebRequestRulesRegistry());
+  const std::string kUrlAttribute(
+      "\"url\": { \"hostContains\": \"url\" }, \n");
+  const std::string kFirstPartyUrlAttribute(
+      "\"firstPartyForCookiesUrl\": { \"hostContains\": \"fpfc\" }, \n");
+  std::string error;
+  std::vector<const std::string*> attributes;
+  std::vector<linked_ptr<RulesRegistry::Rule> > rules;
+
+  // Rule 1 has one condition, with a url attribute
+  attributes.push_back(&kUrlAttribute);
+  rules.push_back(CreateCancellingRule(kRuleId1, attributes));
+
+  // Rule 2 has one condition, with a firstPartyForCookiesUrl attribute
+  attributes.clear();
+  attributes.push_back(&kFirstPartyUrlAttribute);
+  rules.push_back(CreateCancellingRule(kRuleId2, attributes));
+
+  error = registry->AddRules(kExtensionId, rules);
+  EXPECT_EQ("", error);
+  EXPECT_EQ(1, registry->num_clear_cache_calls());
+
+  std::set<const WebRequestRule*> matches;
+
+  const GURL urls[] = {
+    GURL("http://url.example.com"),  // matching
+    GURL("http://www.example.com")   // non-matching
+  };
+  const GURL firstPartyUrls[] = {
+    GURL("http://www.example.com"),  // non-matching
+    GURL("http://fpfc.example.com")  // matching
+  };
+  const char* matchingRuleIds[] = { kRuleId1, kRuleId2 };

[Jeffrey Yasskin, 2013/01/24 19:54:46]

It makes me nervous when a test changes both the input and the code that runs at
the same time. When you're looking for code (rules in this case) to be sensitive
to inputs, you need to run the same code over different inputs or the same input
through different code. Running different input through different code lets bugs
creep in.

[vabr (Chromium), 2013/01/24 21:12:19]

Clarified with Jeffrey, that both rules are used in each iteration of the for
loop. So it is only the inputs which change.

+  COMPILE_ASSERT(arraysize(urls) == arraysize(firstPartyUrls),
+                 urls_and_firstPartyUrls_need_to_have_the_same_size);
+  COMPILE_ASSERT(arraysize(urls) == arraysize(matchingRuleIds),
+                 urls_and_matchingRuleIds_need_to_have_the_same_size);
+  net::TestURLRequestContext context;
+
+  for (size_t i = 0; i < arraysize(matchingRuleIds); ++i) {
+    net::TestURLRequest http_request(urls[i], NULL, &context);
+    WebRequestData request_data(&http_request, ON_BEFORE_REQUEST);
+    http_request.set_first_party_for_cookies(firstPartyUrls[i]);
+    matches = registry->GetMatches(request_data);
+    EXPECT_EQ(1u, matches.size());

[Jeffrey Yasskin, 2013/01/24 19:54:46]

When you put an EXPECT in a loop, always be sure to include enough context to
figure out which iteration it failed in. SCOPED_TRACE
(https://code.google.com/p/googletest/wiki/V1_6_AdvancedGuide#Adding_Traces_to...)
can be enough for this.

[vabr (Chromium), 2013/01/24 21:12:19]

Thanks! Done.

+    WebRequestRule::GlobalRuleId expected_pair =
+      std::make_pair(kExtensionId, matchingRuleIds[i]);
+    EXPECT_EQ(expected_pair, (*matches.begin())->id());

[Jeffrey Yasskin, 2013/01/24 19:54:46]

I tend to prefer inlining the construction of the expected value. gtest prints
the source code it saw, and "expected_pair" is less informative than
"std::make_pair(kExtensionId, matchingRuleIds[i]);", which in turn would be less
informative than "std::make_pair(kExtensionId, kRuleId1);"

[vabr (Chromium), 2013/01/24 21:12:19]

Done. Which rule ID it is should be now clear from the SCOPED_TRACE.

+  }
+}
+
 TEST_F(WebRequestRulesRegistryTest, CheckConsistency) {
   // The contentType condition can only be evaluated during ON_HEADERS_RECEIVED
   // but the redirect action can only be executed during ON_BEFORE_REQUEST.