Create a mechanism define declarative rules via the extension manifest.

extensions/browser/api/declarative/rules_registry.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 "extensions/browser/api/declarative/rules_registry.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_details.h"
 #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 {
 
 const char kSuccess[] = "";
 const char kDuplicateRuleId[] = "Duplicate rule ID: %s";
 
 scoped_ptr<base::Value> RulesToValue(
     const std::vector<linked_ptr<extensions::RulesRegistry::Rule> >& rules) {
   scoped_ptr<base::ListValue> list(new base::ListValue());
   for (size_t i = 0; i < rules.size(); ++i)
@@ skipping 16 matching lines @@
       continue;
     linked_ptr<extensions::RulesRegistry::Rule> rule(
         new extensions::RulesRegistry::Rule());
     if (extensions::RulesRegistry::Rule::Populate(*dict, rule.get()))
       rules.push_back(rule);
   }
 
   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);
 }
 
 }  // namespace
 
 
 namespace extensions {
 
 // RulesRegistry
@@ skipping 61 matching lines @@
   FillInOptionalPriorities(rules);
 
   return AddRulesNoFill(extension_id, rules);
 }
 
 std::string RulesRegistry::RemoveRules(
     const std::string& extension_id,
     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())
     return error;
 
   for (std::vector<std::string>::const_iterator i = rule_identifiers.begin();
        i != rule_identifiers.end();
        ++i) {
     RulesDictionaryKey lookup_key(extension_id, *i);
     rules_.erase(lookup_key);
@@ skipping 63 matching lines @@
     LOG(ERROR) << error;
 }
 
 void RulesRegistry::OnExtensionUninstalled(const std::string& extension_id) {
   DCHECK_CURRENTLY_ON(owner_thread());
   std::string error = RemoveAllRulesNoStoreUpdate(extension_id);
   if (!error.empty())
     LOG(ERROR) << error;
 }
 
 void RulesRegistry::OnExtensionLoaded(const std::string& extension_id) {

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

Repeat previous comment:

Could you update OnExtensionUnloaded/Uninstalled/Loaded to pass in an Extension
rather than its ID? It's trivial (call sites just grab the ID out of the
Extension object they already have access to) and it would save needing to do
the extra map lookup + the extensionregistry dependency.

   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 {
   size_t entry_count = 0u;
   for (RuleIdentifiersMap::const_iterator extension =
            used_rule_identifiers_.begin();
        extension != used_rule_identifiers_.end();
        ++extension) {
     // Each extension is counted as 1 just for being there. Otherwise we miss
     // keys with empty values.
@@ skipping 125 matching lines @@
   for (i = identifiers.begin(); i != identifiers.end(); ++i)
     used_rule_identifiers_[extension_id].erase(*i);
 }
 
 void RulesRegistry::RemoveAllUsedRuleIdentifiers(
     const std::string& extension_id) {
   used_rule_identifiers_.erase(extension_id);
 }
 
 }  // namespace extensions