Create a mechanism define declarative rules via the extension manifest.

extensions/browser/api/declarative/rules_registry.cc

Index: extensions/browser/api/declarative/rules_registry.cc
diff --git a/extensions/browser/api/declarative/rules_registry.cc b/extensions/browser/api/declarative/rules_registry.cc
index dfd9c35260ace0ea706aa383d78d355c8177e57b..fac9719ebc5cd7d7b5983e2d9a5f6c8adca0dec6 100644
--- a/extensions/browser/api/declarative/rules_registry.cc
+++ b/extensions/browser/api/declarative/rules_registry.cc
@@ -22,23 +22,27 @@
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/state_store.h"
 #include "extensions/common/extension.h"
+#include "extensions/common/manifest_constants.h"
+
+namespace extensions {
 
 namespace {
 
 const char kSuccess[] = "";
 const char kDuplicateRuleId[] = "Duplicate rule ID: %s";
+const char kErrorNonRemovableRules[] = "List contains non-removable rules";

[not at google - send to devlin, 2015/06/05 16:16:14]

Be more literal:

const char kErrorCannotRemoveManifestRules[] =
    "Rules declared in the 'event_rules' manifest field cannot be removed";

[danduong, 2015/06/05 21:35:07]

Done.

 
 scoped_ptr<base::Value> RulesToValue(
-    const std::vector<linked_ptr<extensions::RulesRegistry::Rule> >& rules) {
+    const std::vector<linked_ptr<RulesRegistry::Rule>>& rules) {
   scoped_ptr<base::ListValue> list(new base::ListValue());
   for (size_t i = 0; i < rules.size(); ++i)
     list->Append(rules[i]->ToValue().release());
   return list.Pass();
 }
 
-std::vector<linked_ptr<extensions::RulesRegistry::Rule> > RulesFromValue(
+std::vector<linked_ptr<RulesRegistry::Rule>> RulesFromValue(
     const base::Value* value) {
-  std::vector<linked_ptr<extensions::RulesRegistry::Rule> > rules;
+  std::vector<linked_ptr<RulesRegistry::Rule>> rules;
 
   const base::ListValue* list = NULL;
   if (!value || !value->GetAsList(&list))
@@ -49,15 +53,61 @@ std::vector<linked_ptr<extensions::RulesRegistry::Rule> > RulesFromValue(
     const base::DictionaryValue* dict = NULL;
     if (!list->GetDictionary(i, &dict))
       continue;
-    linked_ptr<extensions::RulesRegistry::Rule> rule(
-        new extensions::RulesRegistry::Rule());
-    if (extensions::RulesRegistry::Rule::Populate(*dict, rule.get()))
+    linked_ptr<RulesRegistry::Rule> rule(new RulesRegistry::Rule());
+    if (RulesRegistry::Rule::Populate(*dict, rule.get()))
       rules.push_back(rule);
   }
 
   return rules;
 }
 
+bool ConvertManifestRule(linked_ptr<RulesRegistry::Rule> rule) {

[not at google - send to devlin, 2015/06/05 16:16:14]

avoid copy and use a const linked_ptr&

[danduong, 2015/06/05 21:35:07]

Done.

+  // Swaps key 'type' (used in manifest) for 'instanceType'.

[not at google - send to devlin, 2015/06/05 16:16:14]

Could you move this comment up to a function-level comment? All the existing
functions should have comments too, oh well. Describe what the differences are,
explain that it's specifically the 'conditions' and 'actions' that need
converting, etc.

[danduong, 2015/06/05 21:35:06]

Done.

+  auto convert_list = [](std::vector<linked_ptr<base::Value>>& list) {
+    for (linked_ptr<base::Value> value : list) {

[not at google - send to devlin, 2015/06/05 16:16:14]

avoid copy and use a (const?) linked_ptr&

[danduong, 2015/06/05 21:35:06]

Done.

+      base::DictionaryValue* dictionary = nullptr;
+      if (!value->GetAsDictionary(&dictionary))
+        return false;
+      std::string type;
+      if (!dictionary->GetString("type", &type))
+        return false;
+      dictionary->Remove("type", nullptr);
+      dictionary->SetString("instanceType", type);
+    }
+    return true;
+  };
+  return convert_list(rule->actions) && convert_list(rule->conditions);
+}
+
+std::vector<linked_ptr<RulesRegistry::Rule>> RulesFromManifest(
+    const base::Value* value,
+    const std::string& event_name) {
+  std::vector<linked_ptr<RulesRegistry::Rule>> rules;
+
+  const base::ListValue* list = nullptr;
+  if (!value || !value->GetAsList(&list))

[not at google - send to devlin, 2015/06/05 16:16:14]

The (!value) here here shouldn't be necessary, it doesn't make sense for a
caller to pass in a null |value|. I don't know why the existing code does it
either :-\

[danduong, 2015/06/05 21:35:06]

Done.

+    return rules;

[not at google - send to devlin, 2015/06/05 16:16:13]

Another problem is that this silently fails if an extension passes anything
other than a list into here. Regarding your comment:

"I looked into it and I ran into some problems with the code generator since
event_rules is type:array."

That is a problem that I recall somebody else having, but if

"type": {
  "id": "RuleList",
  "type": "array",
  "items": {"$ref": "events.Rule"}
}

or whatever doesn't work, then you could work around that by doing

"type": {
  "id": "RuleList",
  "type": "object",
  "properties": [{
    "rules": {
      "type": "array",
      "items": {"$ref": "events.Rule"}
    }
  }]
}

i.e. nest the list inside an object.

[danduong, 2015/06/05 21:35:06]

Should we just log error in these cases?

[not at google - send to devlin, 2015/06/05 23:29:42]

Ideally we'd be parsing these rules when the extension is installed. That way
you can actually report warnings on installation. To do that you'd set up a
manifest handler (ManifestHandler).

I was debating asking you to do this. It's idiomatic, whereas doing the parsing
ad-hoc like you've done is uncommon, but it seemed easier enough to do it like
you've done (given you'd written it that way).

To show a warning you could use the ErrorConsole:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

but preferable solution is really to use a manifest handler.

[danduong, 2015/06/06 03:10:32]

added a manifest handler.

+
+  for (size_t i = 0; i < list->GetSize(); ++i) {
+    const base::DictionaryValue* dict = nullptr;
+    if (!list->GetDictionary(i, &dict))
+      continue;
+    std::string event;
+    if (!dict->GetString("event", &event))
+      continue;
+    if (event_name != event)
+      continue;

[not at google - send to devlin, 2015/06/05 16:16:14]

It would help to document this stuff. I only know what's going on because I
reviewed the proposal. I.e. what is "event", what is "type", and so forth.

[danduong, 2015/06/05 21:35:06]

Done.

+    linked_ptr<RulesRegistry::Rule> rule(new RulesRegistry::Rule());
+    if (!RulesRegistry::Rule::Populate(*dict, rule.get()))
+      continue;
+    if (!ConvertManifestRule(rule))
+      continue;
+    rules.push_back(rule);

[not at google - send to devlin, 2015/06/05 16:16:14]

Apologies for this nit, but btw, these methods that return bools are designed to
be chained together like

if (RulesRegistry::Rule::Populate(*dict, rule.get()) &&
    ConvertManifestRule(rule)) {
  rules.push_back(rule);
}

and likewise above,

if (!dict->GetString("event", &event) || event_name != event)) {
  continue;
}

[danduong, 2015/06/05 21:35:06]

Done.

+  }
+
+  return rules;
+}
+
 std::string ToId(int identifier) {
   return base::StringPrintf("_%d_", identifier);
 }
@@ -65,8 +115,6 @@ std::string ToId(int identifier) {
 }  // namespace
 
 
-namespace extensions {
-
 // RulesRegistry
 
 RulesRegistry::RulesRegistry(content::BrowserContext* browser_context,
@@ -90,7 +138,8 @@ RulesRegistry::RulesRegistry(content::BrowserContext* browser_context,
 
 std::string RulesRegistry::AddRulesNoFill(
     const std::string& extension_id,
-    const std::vector<linked_ptr<Rule> >& rules) {
+    const std::vector<linked_ptr<Rule>>& rules,
+    bool from_manifest) {

[not at google - send to devlin, 2015/06/05 16:16:14]

It's more direct to pass around the list of rules to add to, rather than a flag
to which one to use.

[danduong, 2015/06/05 21:35:06]

Done.

[not at google - send to devlin, 2015/06/08 21:44:58]

except, pass as a pointer not a reference.

[danduong, 2015/06/09 01:21:26]

Done.

   DCHECK_CURRENTLY_ON(owner_thread());
 
   // Verify that all rule IDs are new.
@@ -100,7 +149,8 @@ std::string RulesRegistry::AddRulesNoFill(
     // Every rule should have a priority assigned.
     DCHECK((*i)->priority);
     RulesDictionaryKey key(extension_id, rule_id);
-    if (rules_.find(key) != rules_.end())
+    if (rules_.find(key) != rules_.end() ||
+        manifest_rules_.find(key) != manifest_rules_.end())
       return base::StringPrintf(kDuplicateRuleId, rule_id.c_str());
   }
 
@@ -114,16 +164,19 @@ std::string RulesRegistry::AddRulesNoFill(
       rules.begin(); i != rules.end(); ++i) {
     const RuleId& rule_id = *((*i)->id);
     RulesDictionaryKey key(extension_id, rule_id);
-    rules_[key] = *i;
+    if (from_manifest)
+      manifest_rules_[key] = *i;
+    else
+      rules_[key] = *i;
   }
 
   MaybeProcessChangedRules(extension_id);
   return kSuccess;
 }
 
-std::string RulesRegistry::AddRules(
-    const std::string& extension_id,
-    const std::vector<linked_ptr<Rule> >& rules) {
+std::string RulesRegistry::AddRules(const std::string& extension_id,
+                                    const std::vector<linked_ptr<Rule>>& rules,
+                                    bool from_manifest) {
   DCHECK_CURRENTLY_ON(owner_thread());
 
   std::string error = CheckAndFillInOptionalRules(extension_id, rules);
@@ -131,7 +184,7 @@ std::string RulesRegistry::AddRules(
     return error;
   FillInOptionalPriorities(rules);
 
-  return AddRulesNoFill(extension_id, rules);
+  return AddRulesNoFill(extension_id, rules, from_manifest);

[not at google - send to devlin, 2015/06/05 16:16:14]

This can only ever be false, so remove the |from_manifest| flag from the public
interface, and pass in false here. Or rather, pass in |&rules_|.

[danduong, 2015/06/05 21:35:06]

Done.

 }
 
 std::string RulesRegistry::RemoveRules(
@@ -139,6 +192,14 @@ std::string RulesRegistry::RemoveRules(
     const std::vector<std::string>& rule_identifiers) {
   DCHECK_CURRENTLY_ON(owner_thread());
 
+  // Check if any of the rules are non-removable.
+  for (RuleId rule_id : rule_identifiers) {
+    RulesDictionaryKey lookup_key(extension_id, rule_id);
+    RulesDictionary::iterator itr = manifest_rules_.find(lookup_key);
+    if (itr != manifest_rules_.end())
+      return kErrorNonRemovableRules;
+  }
+
   std::string error = RemoveRulesImpl(extension_id, rule_identifiers);
 
   if (!error.empty())
@@ -171,13 +232,18 @@ std::string RulesRegistry::RemoveAllRulesNoStoreUpdate(
   if (!error.empty())
     return error;
 
-  for (RulesDictionary::const_iterator i = rules_.begin();
-      i != rules_.end();) {
-    const RulesDictionaryKey& key = i->first;
-    ++i;
-    if (key.first == extension_id)
-      rules_.erase(key);
-  }
+  auto remove_rules = [&extension_id](RulesDictionary& dictionary) {
+    for (RulesDictionary::const_iterator i = dictionary.begin();
+         i != dictionary.end();) {
+      const RulesDictionaryKey& key = i->first;
+      ++i;
+      if (key.first == extension_id) {
+        dictionary.erase(key);
+      }

[not at google - send to devlin, 2015/06/05 16:16:14]

Btw the way I usually see this written is along the lines of

for (auto it = dictionary.begin(); it != dictionary.end();) {
  if ((*it->first).first == extension_id)
    dictionary.erase(it++)
  else
    ++it;
}

but you could also using std::remove_if? Not worth making significant changes to
existing code, I suppose.

[danduong, 2015/06/05 21:35:07]

Done.

+    }
+  };
+  remove_rules(rules_);
+  remove_rules(manifest_rules_);
 
   RemoveAllUsedRuleIdentifiers(extension_id);
   return kSuccess;
@@ -192,41 +258,57 @@ void RulesRegistry::GetRules(const std::string& extension_id,
       i != rule_identifiers.end(); ++i) {
     RulesDictionaryKey lookup_key(extension_id, *i);
     RulesDictionary::iterator entry = rules_.find(lookup_key);
-    if (entry != rules_.end())
+    if (entry != rules_.end()) {
       out->push_back(entry->second);
+    } else {
+      entry = manifest_rules_.find(lookup_key);
+      if (entry != manifest_rules_.end())
+        out->push_back(entry->second);
+    }

[not at google - send to devlin, 2015/06/05 16:16:14]

Simpler:

RulesDictionary::iterator entry = rules_.find(lookup_key);
if (entry == rules_.end())
  entry = manifest_rules_.find(lookup_key);
if (entry != rules_.end())
  out->push_back(entry->second);

[danduong, 2015/06/05 21:35:06]

Done.

   }
 }
 
 void RulesRegistry::GetAllRules(const std::string& extension_id,
                                 std::vector<linked_ptr<Rule> >* out) {
   DCHECK_CURRENTLY_ON(owner_thread());
-  for (RulesDictionary::const_iterator i = rules_.begin();
-      i != rules_.end(); ++i) {
-    const RulesDictionaryKey& key = i->first;
-    if (key.first == extension_id)
-      out->push_back(i->second);
-  }
+  auto get_rules = [&extension_id, out](const RulesDictionary& dictionary) {
+    for (RulesDictionary::const_iterator i = dictionary.begin();
+         i != dictionary.end(); ++i) {

[not at google - send to devlin, 2015/06/05 16:16:13]

(btw, consider changing all of these sorts of loops to C++11 range-based loops
as you go, cleaner)

[danduong, 2015/06/05 21:35:07]

Acknowledged.

+      const RulesDictionaryKey& key = i->first;
+      if (key.first == extension_id)
+        out->push_back(i->second);
+    }
+  };
+  get_rules(manifest_rules_);
+  get_rules(rules_);
 }
 
-void RulesRegistry::OnExtensionUnloaded(const std::string& extension_id) {
+void RulesRegistry::OnExtensionUnloaded(const Extension* extension) {
   DCHECK_CURRENTLY_ON(owner_thread());
-  std::string error = RemoveAllRulesImpl(extension_id);
+  std::string error = RemoveAllRulesImpl(extension->id());
   if (!error.empty())
     LOG(ERROR) << error;
 }
 
-void RulesRegistry::OnExtensionUninstalled(const std::string& extension_id) {
+void RulesRegistry::OnExtensionUninstalled(const Extension* extension) {
   DCHECK_CURRENTLY_ON(owner_thread());
-  std::string error = RemoveAllRulesNoStoreUpdate(extension_id);
+  std::string error = RemoveAllRulesNoStoreUpdate(extension->id());
   if (!error.empty())
     LOG(ERROR) << error;
 }
 
-void RulesRegistry::OnExtensionLoaded(const std::string& extension_id) {
+void RulesRegistry::OnExtensionLoaded(const Extension* extension) {
   DCHECK_CURRENTLY_ON(owner_thread());
-  std::vector<linked_ptr<Rule> > rules;
-  GetAllRules(extension_id, &rules);
-  std::string error = AddRulesImpl(extension_id, rules);
+  std::vector<linked_ptr<Rule>> rules;
+  GetAllRules(extension->id(), &rules);
+  const base::ListValue* value = NULL;
+  if (extension->manifest()->GetList(manifest_keys::kEventRules, &value)) {
+    std::string error =
+        AddRules(extension->id(), RulesFromManifest(value, event_name_), true);
+    if (!error.empty())
+      LOG(ERROR) << error;
+  }
+  std::string error = AddRulesImpl(extension->id(), rules);
   if (!error.empty())
     LOG(ERROR) << error;
 }
@@ -249,7 +331,7 @@ void RulesRegistry::DeserializeAndAddRules(
     scoped_ptr<base::Value> rules) {
   DCHECK_CURRENTLY_ON(owner_thread());
 
-  AddRulesNoFill(extension_id, RulesFromValue(rules.get()));
+  AddRulesNoFill(extension_id, RulesFromValue(rules.get()), false);
 }
 
 RulesRegistry::~RulesRegistry() {
@@ -274,13 +356,18 @@ void RulesRegistry::ProcessChangedRules(const std::string& extension_id) {
 
   std::vector<linked_ptr<Rule> > new_rules;
   GetAllRules(extension_id, &new_rules);
+  std::vector<linked_ptr<Rule>> rules_to_write;
+  for (auto rule : new_rules) {
+    const RuleId& rule_id = *(rule->id);
+    RulesDictionaryKey key(extension_id, rule_id);
+    // Only write rules that were added programmatically.
+    if (manifest_rules_.find(key) == manifest_rules_.end())
+      rules_to_write.push_back(rule);
+  }

[not at google - send to devlin, 2015/06/05 16:16:13]

This is pretty wasteful.
1. GetAllRules gets |rules_| and |manifest_rules_|.
2. This code removes |manifest_rules_|.
3. Write out.

there is no point getting |manifest_rules_| so factor code accordingly.

[danduong, 2015/06/05 21:35:06]

Done.

   content::BrowserThread::PostTask(
-      content::BrowserThread::UI,
-      FROM_HERE,
-      base::Bind(&RulesCacheDelegate::WriteToStorage,
-                 cache_delegate_,
-                 extension_id,
-                 base::Passed(RulesToValue(new_rules))));
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&RulesCacheDelegate::WriteToStorage, cache_delegate_,
+                 extension_id, base::Passed(RulesToValue(rules_to_write))));
 }
 
 void RulesRegistry::MaybeProcessChangedRules(const std::string& extension_id) {