Refactoring how conditions without URL attributes are handled.

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h"
 
 #include <vector>
 
 #include "base/memory/linked_ptr.h"
 #include "base/message_loop.h"
+#include "base/stl_util.h"
 #include "base/values.h"
 #include "chrome/common/extensions/matcher/url_matcher_constants.h"
 #include "chrome/browser/extensions/api/declarative_webrequest/webrequest_constants.h"
 #include "chrome/browser/extensions/api/declarative_webrequest/webrequest_rule.h"
 #include "chrome/browser/extensions/api/web_request/web_request_api_helpers.h"
 #include "content/public/test/test_browser_thread.h"
 #include "net/url_request/url_request_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
@@ skipping 14 matching lines @@
 class TestWebRequestRulesRegistry : public WebRequestRulesRegistry {
  public:
   TestWebRequestRulesRegistry()
       : WebRequestRulesRegistry(NULL, NULL),
         num_clear_cache_calls_(0) {}
 
   // Returns how often the in-memory caches of the renderers were instructed
   // to be cleared.
   int num_clear_cache_calls() const { return num_clear_cache_calls_; }
 
+  // How many rules are there which have some conditions not triggered by URL
+  // matches.
+  size_t RulesWithoutTriggers() const {
+    return rules_with_untriggered_conditions_for_test().size();
+  }
+
  protected:
   virtual ~TestWebRequestRulesRegistry() {}
 
   virtual base::Time GetExtensionInstallationTime(
       const std::string& extension_id) const {
     if (extension_id == kExtensionId)
       return base::Time() + base::TimeDelta::FromDays(1);
     else if (extension_id == kExtensionId2)
       return base::Time() + base::TimeDelta::FromDays(2);
     else
@@ skipping 143 matching lines @@
 
     linked_ptr<RulesRegistry::Rule> rule =
         make_linked_ptr(new RulesRegistry::Rule);
     rule->id.reset(new std::string(kRuleId4));
     rule->priority.reset(new int(200));
     rule->actions.push_back(action);
     rule->conditions.push_back(condition);
     return rule;
   }
 
+  // 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) {
+    linked_ptr<json_schema_compiler::any::Any> condition = make_linked_ptr(
+        new json_schema_compiler::any::Any);
+    DictionaryValue condition_dict;

[Jeffrey Yasskin, 2013/01/10 22:00:57]

FWIW, I would probably try to initialize this using base::test::ParseJson or
extensions::DictionaryBuilder, to try to make the resulting value easier to
read.

[vabr (Chromium), 2013/01/15 10:24:57]

Done.

The way I did it I separated the condition string into the boilerplate and the
interesting stuff (see CreateCondition). That might potentially harm
readability, but personally I found it helpful, mostly for the removal of code
duplication.

Dominic, do you want me to convert also the other rule creating methods here to
use ParseJson? The only possible drawback I see is replacing of key::...
constants by hard-coded strings, but I don't see that as a maintenance problem,
because the constant's name is always made dependent on its content (so changing
the definition of the constant means also changing its name everywhere).

+    condition_dict.SetString(keys::kInstanceTypeKey, keys::kRequestMatcherType);
+    ListValue* resource_condition_list = new ListValue();
+    // In this unit-test we don't generate requests with the stylesheet resource
+    // type, so this attribute is not going to match.
+    resource_condition_list->AppendString("stylesheet");
+    condition_dict.Set(keys::kResourceTypeKey, resource_condition_list);
+    if (with_url_attribute) {
+      DictionaryValue* http_condition_dict = new DictionaryValue();
+      http_condition_dict->SetString(keys2::kPathContainsKey, "");
+      condition_dict.Set(keys::kUrlKey, http_condition_dict);
+    }
+    condition->Init(condition_dict);
+
+    DictionaryValue action_dict;
+    action_dict.SetString(keys::kInstanceTypeKey, keys::kCancelRequestType);
+    linked_ptr<json_schema_compiler::any::Any> action = make_linked_ptr(
+        new json_schema_compiler::any::Any);
+    action->Init(action_dict);
+
+    linked_ptr<RulesRegistry::Rule> rule =
+        make_linked_ptr(new RulesRegistry::Rule);

[Jeffrey Yasskin, 2013/01/10 22:00:57]

Just use
  linked_ptr<RulesRegistry::Rule> rule(new RulesRegistry::Rule);
make_linked_ptr is only helpful if you didn't otherwise need to write the type.

[vabr (Chromium), 2013/01/15 10:24:57]

Oh, right, thanks for catching this.
I corrected this on other places in this file as well (from where I copy-pasted
that).

+    rule->id.reset(new std::string(rule_id));
+    rule->priority.reset(new int(1));
+    rule->actions.push_back(action);
+    rule->conditions.push_back(condition);
+    return rule;
+  }
+
  protected:
   MessageLoop message_loop;
   content::TestBrowserThread ui;
   content::TestBrowserThread io;
 };
 
 TEST_F(WebRequestRulesRegistryTest, AddRulesImpl) {
   scoped_refptr<TestWebRequestRulesRegistry> registry(
       new TestWebRequestRulesRegistry());
   std::string error;
 
   std::vector<linked_ptr<RulesRegistry::Rule> > rules;
   rules.push_back(CreateRule1());
   rules.push_back(CreateRule2());
 
   error = registry->AddRules(kExtensionId, rules);
   EXPECT_EQ("", error);
   EXPECT_EQ(1, registry->num_clear_cache_calls());
 
-  std::set<WebRequestRule::GlobalRuleId> matches;
+  std::set<const WebRequestRule*> matches;
 
   GURL http_url("http://www.example.com");
   net::TestURLRequestContext context;
   net::TestURLRequest http_request(http_url, NULL, &context);
   matches = registry->GetMatches(
       WebRequestRule::RequestData(&http_request, ON_BEFORE_REQUEST));
   EXPECT_EQ(2u, matches.size());
-  EXPECT_TRUE(matches.find(std::make_pair(kExtensionId, kRuleId1)) !=
-      matches.end());
-  EXPECT_TRUE(matches.find(std::make_pair(kExtensionId, kRuleId2)) !=
-      matches.end());
+
+  std::set<WebRequestRule::GlobalRuleId> matches_ids;
+  for (std::set<const WebRequestRule*>::const_iterator it = matches.begin();
+       it != matches.end(); ++it)
+    matches_ids.insert((*it)->id());
+  EXPECT_TRUE(ContainsKey(matches_ids, std::make_pair(kExtensionId, kRuleId1)));
+  EXPECT_TRUE(ContainsKey(matches_ids, std::make_pair(kExtensionId, kRuleId2)));
 
   GURL foobar_url("http://www.foobar.com");
   net::TestURLRequest foobar_request(foobar_url, NULL, &context);
   matches = registry->GetMatches(
       WebRequestRule::RequestData(&foobar_request, ON_BEFORE_REQUEST));
   EXPECT_EQ(1u, matches.size());
-  EXPECT_TRUE(matches.find(std::make_pair(kExtensionId, kRuleId2)) !=
-      matches.end());
+  WebRequestRule::GlobalRuleId expected_pair =
+      std::make_pair(kExtensionId, kRuleId2);
+  EXPECT_EQ(expected_pair, (*matches.begin())->id());
 }
 
 TEST_F(WebRequestRulesRegistryTest, RemoveRulesImpl) {
   scoped_refptr<TestWebRequestRulesRegistry> registry(
       new TestWebRequestRulesRegistry());
   std::string error;
 
   // Setup RulesRegistry to contain two rules.
   std::vector<linked_ptr<RulesRegistry::Rule> > rules_to_add;
   rules_to_add.push_back(CreateRule1());
   rules_to_add.push_back(CreateRule2());
   error = registry->AddRules(kExtensionId, rules_to_add);
   EXPECT_EQ("", error);
   EXPECT_EQ(1, registry->num_clear_cache_calls());
 
   // Verify initial state.
   std::vector<linked_ptr<RulesRegistry::Rule> > registered_rules;
   registry->GetAllRules(kExtensionId, &registered_rules);
   EXPECT_EQ(2u, registered_rules.size());
+  EXPECT_EQ(1u, registry->RulesWithoutTriggers());
 
   // Remove first rule.
   std::vector<std::string> rules_to_remove;
   rules_to_remove.push_back(kRuleId1);
   error = registry->RemoveRules(kExtensionId, rules_to_remove);
   EXPECT_EQ("", error);
   EXPECT_EQ(2, registry->num_clear_cache_calls());
 
   // Verify that only one rule is left.
   registered_rules.clear();
   registry->GetAllRules(kExtensionId, &registered_rules);
   EXPECT_EQ(1u, registered_rules.size());
+  EXPECT_EQ(1u, registry->RulesWithoutTriggers());
 
   // Now rules_to_remove contains both rules, i.e. one that does not exist in
   // the rules registry anymore. Effectively we only remove the second rule.
   rules_to_remove.push_back(kRuleId2);
   error = registry->RemoveRules(kExtensionId, rules_to_remove);
   EXPECT_EQ("", error);
   EXPECT_EQ(3, registry->num_clear_cache_calls());
 
   // Verify that everything is gone.
   registered_rules.clear();
   registry->GetAllRules(kExtensionId, &registered_rules);
   EXPECT_EQ(0u, registered_rules.size());
+  EXPECT_EQ(0u, registry->RulesWithoutTriggers());
 
   EXPECT_TRUE(registry->IsEmpty());
 }
 
 TEST_F(WebRequestRulesRegistryTest, RemoveAllRulesImpl) {
   scoped_refptr<TestWebRequestRulesRegistry> registry(
       new TestWebRequestRulesRegistry());
   std::string error;
 
   // Setup RulesRegistry to contain two rules, one for each extension.
@@ skipping 119 matching lines @@
 
   // The redirect by the first extension is ignored due to the ignore rule.
   ASSERT_EQ(1u, deltas.size());
   LinkedPtrEventResponseDelta effective_rule = *(deltas.begin());
 
   EXPECT_EQ(kExtensionId2, effective_rule->extension_id);
   EXPECT_EQ(base::Time() + base::TimeDelta::FromDays(2),
             effective_rule->extension_install_time);
   EXPECT_EQ(GURL("http://www.bar.com"), effective_rule->new_url);
 }
+
+// Test that rules failing IsFulfilled on their conditions are never returned by
+// GetMatches.
+TEST_F(WebRequestRulesRegistryTest, GetMatchesCheckFulfilled) {

[vabr (Chromium), 2013/01/10 15:12:22]

I almost forgot to check IsFulfilled in the second stage of GetMatches, so I
though it best to add a unit test for that here.

+  scoped_refptr<TestWebRequestRulesRegistry> registry(
+      new TestWebRequestRulesRegistry());
+  std::string error;
+
+  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));
+
+  error = registry->AddRules(kExtensionId, rules);
+  EXPECT_EQ("", error);
+  EXPECT_EQ(1, registry->num_clear_cache_calls());
+
+  std::set<const WebRequestRule*> matches;
+
+  GURL http_url("http://www.example.com");
+  net::TestURLRequestContext context;
+  net::TestURLRequest http_request(http_url, NULL, &context);
+  matches = registry->GetMatches(
+      WebRequestRule::RequestData(&http_request, ON_BEFORE_REQUEST));
+  EXPECT_EQ(0u, matches.size());
+}
+
 }  // namespace extensions