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..5cc6f6ce8e9570e85a8e58bd142b4d901bc06e86 100644
--- a/extensions/browser/api/declarative/rules_registry.cc
+++ b/extensions/browser/api/declarative/rules_registry.cc
@@ -19,9 +19,11 @@
 #include "content/public/browser/notification_source.h"
 #include "extensions/browser/api/declarative/rules_cache_delegate.h"
 #include "extensions/browser/extension_prefs.h"
+#include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/state_store.h"
 #include "extensions/common/extension.h"
+#include "extensions/common/manifest_constants.h"
 
 namespace {
 
@@ -58,6 +60,67 @@ std::vector<linked_ptr<extensions::RulesRegistry::Rule> > RulesFromValue(
   return rules;
 }
 
+scoped_ptr<base::ListValue> ConvertListFromManifest(
+    const base::ListValue* list) {
+  scoped_ptr<base::ListValue> result(new base::ListValue);
+  for (base::ListValue::const_iterator it = list->begin(); it != list->end();
+       ++it) {
+    base::DictionaryValue* dictionary = nullptr;
+    if (!(*it)->GetAsDictionary(&dictionary))
+      continue;
+    std::string type;
+    if (!dictionary->GetString("type", &type))
+      continue;
+    scoped_ptr<base::DictionaryValue> copy = dictionary->CreateDeepCopy();
+    copy->Remove("type", NULL);
+    copy->SetString("instanceType", type);
+    result->Append(copy.Pass());
+  }
+  return result.Pass();
+}
+
+std::vector<linked_ptr<extensions::RulesRegistry::Rule>> RulesFromManifest(

[not at google - send to devlin, 2015/06/04 17:58:49]

- This file is already in the extensions namespace, so no need for extensions::.
- You can return a ScopedVector<RulesRegistry::Rule> from here, because you can
.Pass() them.

[danduong, 2015/06/04 19:10:59]

 - All these helper functions are in anonymous namespace. Should I move them
into extensions?
 - The receiving function AddRules is expecting a
vector<linked_ptr<Registry::Rule>> how would I reconcile that?

[not at google - send to devlin, 2015/06/04 19:23:52]

I see. Move the "namespace extensions {" declaration to the top of the file.

[danduong, 2015/06/04 21:39:40]

Done.

+    const base::Value* value,
+    const std::string& event_name) {
+  std::vector<linked_ptr<extensions::RulesRegistry::Rule>> rules;
+
+  const base::ListValue* list = nullptr;
+  if (!value || !value->GetAsList(&list))
+    return rules;
+
+  for (size_t i = 0; i < list->GetSize(); ++i) {

[not at google - send to devlin, 2015/06/04 17:58:49]

You should look at defining this structure in extensions_manifest_types.json,
which will generate much of this parsing for you. See how OptionsUI is
implemented, for example.

[danduong, 2015/06/05 06:41:57]

I looked into it and I ran into some problems with the code generator since
event_rules is type:array. The individual rule object is already defined in
events.json. So I refactored the code a little bit to make better use of this.

+    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;
+    const base::ListValue* list;
+    if (!dict->GetList("conditions", &list) || !list->GetSize())
+      continue;
+    scoped_ptr<base::ListValue> conditions = ConvertListFromManifest(list);
+    if (!conditions->GetSize())
+      continue;
+    if (!dict->GetList("actions", &list) || !list->GetSize())
+      continue;
+    scoped_ptr<base::ListValue> actions = ConvertListFromManifest(list);
+    if (!actions->GetSize())
+      continue;
+    scoped_ptr<base::DictionaryValue> rule_dict(new base::DictionaryValue);
+    rule_dict->Set("conditions", conditions.Pass());
+    rule_dict->Set("actions", actions.Pass());
+    rule_dict->SetBoolean("removable", false);
+    linked_ptr<extensions::RulesRegistry::Rule> rule(
+        new extensions::RulesRegistry::Rule());
+    if (extensions::RulesRegistry::Rule::Populate(*rule_dict, rule.get()))
+      rules.push_back(rule);
+  }
+
+  return rules;
+}
+
 std::string ToId(int identifier) {
   return base::StringPrintf("_%d_", identifier);
 }
@@ -139,6 +202,17 @@ 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 entry = rules_.find(lookup_key);
+    if (entry != rules_.end()) {
+      Rule* rule = entry->second.get();
+      if (rule->removable.get() && !*(rule->removable))
+        return "List contains non-removable rules";

[not at google - send to devlin, 2015/06/04 17:58:48]

I'd prefer to define this as a constant.

[danduong, 2015/06/04 19:10:58]

Done.

+    }
+  }
+
   std::string error = RemoveRulesImpl(extension_id, rule_identifiers);
 
   if (!error.empty())
@@ -226,9 +300,21 @@ void RulesRegistry::OnExtensionLoaded(const std::string& extension_id) {
   DCHECK_CURRENTLY_ON(owner_thread());
   std::vector<linked_ptr<Rule> > rules;
   GetAllRules(extension_id, &rules);
-  std::string error = AddRulesImpl(extension_id, rules);
-  if (!error.empty())
-    LOG(ERROR) << error;
+  if (rules.empty()) {

[not at google - send to devlin, 2015/06/04 17:58:48]

Repeat previous comment:

Why only if there are no rules registered yet? It seems a perfectly reasonable
use case to have a set of rules in the manifest, with a dynamic set to add
later.

[danduong, 2015/06/04 19:10:59]

Isn't that use case still met, since the complete set of rules is persisted to
storage? On first load after install, the manifest rules will be added and
persisted. The reason why I put it here vs. an OnInstall is it makes adding
manifest extensions a little more robust just in-case we miss an onInstall event
for whatever reason.

[not at google - send to devlin, 2015/06/04 19:23:53]

I think this won't work for update, though. There might already be rules and
when the extension updates and adds a new rule, it won't be updated.

Basically I don't think you can implement this by writing the rules to disk,
plus it's wasteful to be duplicating them.

[danduong, 2015/06/04 19:56:07]

Update would work still right? Uninstall will remove all rules and start from
scratch. After the extension is loaded, programatic rules are added back. I'm
concerned about the additional complexity it adds to have some rules persisted
and others not.

[not at google - send to devlin, 2015/06/04 20:02:21]

Maybe. Either way this code looks wrong. I would hope the only changes necessary
would be to have 2 sets of rules, one you read from disk, the other from the
manifest.

[danduong, 2015/06/04 21:39:40]

Done.

+    // If no rules have been registered yet, check if there are any rules
+    // defined in the extension manifest to add.
+    const Extension* extension =
+        ExtensionRegistry::Get(browser_context())
+            ->GetExtensionById(extension_id, ExtensionRegistry::EVERYTHING);

[not at google - send to devlin, 2015/06/04 17:58:49]

Btw once https://codereview.chromium.org/1130353010 goes in you'll be able to
write "GetInstalledExtension" instead of "... EVERYTHING".

[danduong, 2015/06/04 19:10:59]

Acknowledged.

+    const base::ListValue* value = NULL;
+    if (extension->manifest()->GetList(manifest_keys::kEventRules, &value)) {
+      AddRules(extension_id, RulesFromManifest(value, event_name_));

[Mike Wittman, 2015/06/04 18:40:13]

this needs to handle the error which may be returned, and propagate it to the
extension's error log

[danduong, 2015/06/04 19:10:59]

Done.

+    }
+  } else {
+    std::string error = AddRulesImpl(extension_id, rules);
+    if (!error.empty())
+      LOG(ERROR) << error;
+  }
 }
 
 size_t RulesRegistry::GetNumberOfUsedRuleIdentifiersForTesting() const {