Refactoring how conditions without URL attributes are handled.

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 3d6895c559d3856079aab304eb38a3142c7e4d0d..2267a8cc717320501b6e4f1f06a6dd3528b29455 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
@@ -8,6 +8,7 @@
 
 #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"
@@ -42,6 +43,12 @@ class TestWebRequestRulesRegistry : public WebRequestRulesRegistry {
   // 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() {}
 
@@ -205,6 +212,44 @@ class WebRequestRulesRegistryTest : public testing::Test {
     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;
@@ -224,7 +269,7 @@ TEST_F(WebRequestRulesRegistryTest, AddRulesImpl) {
   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;
@@ -232,18 +277,22 @@ TEST_F(WebRequestRulesRegistryTest, AddRulesImpl) {
   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) {
@@ -263,6 +312,7 @@ TEST_F(WebRequestRulesRegistryTest, RemoveRulesImpl) {
   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;
@@ -275,6 +325,7 @@ TEST_F(WebRequestRulesRegistryTest, RemoveRulesImpl) {
   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.
@@ -287,6 +338,7 @@ TEST_F(WebRequestRulesRegistryTest, RemoveRulesImpl) {
   registered_rules.clear();
   registry->GetAllRules(kExtensionId, &registered_rules);
   EXPECT_EQ(0u, registered_rules.size());
+  EXPECT_EQ(0u, registry->RulesWithoutTriggers());
 
   EXPECT_TRUE(registry->IsEmpty());
 }
@@ -426,4 +478,34 @@ TEST_F(WebRequestRulesRegistryTest, Priorities) {
             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