RulesRegistry for declarative APIs.

chrome/browser/extensions/api/declarative/declarative_api.cc

Index: chrome/browser/extensions/api/declarative/declarative_api.cc
diff --git a/chrome/browser/extensions/api/declarative/declarative_api.cc b/chrome/browser/extensions/api/declarative/declarative_api.cc
index e3806675770561476bd146bfdff8f2956178916f..aee1db1715a19b64b28b21f04debc23ef4f1b3d7 100644
--- a/chrome/browser/extensions/api/declarative/declarative_api.cc
+++ b/chrome/browser/extensions/api/declarative/declarative_api.cc
@@ -5,29 +5,116 @@
 #include "chrome/browser/extensions/api/declarative/declarative_api.h"
 
 #include "base/values.h"
+#include "chrome/browser/extensions/api/declarative/rules_registry.h"
+#include "chrome/browser/extensions/api/declarative/rules_registry_service.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/profiles/profile.h"
 
 namespace extensions {
 
 bool AddRulesFunction::RunImpl() {
-  // LOG(ERROR) << "AddRulesFunction called";
+  std::string event_name;
+  EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &event_name));
 
   ListValue* rules_list = NULL;
   EXTENSION_FUNCTION_VALIDATE(args_->GetList(1, &rules_list));
 
-  // TODO(battre): Generate unique IDs and priorities here.
+  std::vector<DictionaryValue*> rules;
+  for (ListValue::iterator i = rules_list->begin();
+       i != rules_list->end();
+       ++i) {
+    DictionaryValue* rule;

[not at google - send to devlin, 2012/02/07 03:25:10]

= NULL

[battre, 2012/02/07 18:45:33]

Done.

+    EXTENSION_FUNCTION_VALIDATE((*i)->GetAsDictionary(&rule));
+    rules.push_back(rule);
+  }
+
+  RulesRegistryService* rules_registry =
+      profile()->GetExtensionService()->GetRulesRegistryService();
+
+  if (!rules_registry->AddRules(event_name, extension_id(), rules, &error_))
+    return false;
 
   result_.reset(rules_list->DeepCopy());
   return true;
 }
 
 bool RemoveRulesFunction::RunImpl() {
-  // LOG(ERROR) << "RemoveRulesFunction called";
-  return true;
+  std::string event_name;
+  EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &event_name));
+

[not at google - send to devlin, 2012/02/07 03:25:10]

my biased opinion based on how similar code in the storage API ended up being,
but I kind of like it as follows. It clearly enumerates what the input values
can be (and as a result does more error checking), doesn't instantiate unneeded
objects, and has a clearly defined exit point.

===

std::string event_name;
EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &event_name));
Value* rule_identifiers = NULL; // it's not necessary a list value so
                                // misleading to call it as such.
EXTENSION_FUNCTION_VALIDATE(args_->Get(1, &rule_identifiers));

RulesRegistry* rules_registry =
    profile()->GetExtensionService()->GetRulesRegistryService()->
        GetRegistry(event_name);

switch (rule_identifiers->GetType()) {
  case Value::TYPE_NULL:
    error_ = rules_registry->RemoveAllRules(extension_id());
    break;

  case Value::TYPE_LIST: {
    std::vector<std::string> rule_identifiers_list;
    // Write this out, or have it as a util function, whatever.
    // It makes sense to have it as a util since you'd use it in AddRules too.
    AddAllStringValues(static_cast<ListValue*>(rules_registry),
                       &rule_identifiers_list));
    error_ = rules_registry->RemoveRules(extension_id(), rule_identifiers_list);
    break;
  }

  default:
    error_ = kUnsupportedTypeOrSomething;
    break;
}

return !error_.empty();

[battre, 2012/02/07 18:45:33]

Done.

+  std::vector<std::string> rule_identifiers;
+
+  Value* rule_identifiers_list_value = NULL;
+  EXTENSION_FUNCTION_VALIDATE(args_->Get(1, &rule_identifiers_list_value));
+
+  ListValue* rule_identifiers_list = NULL;
+  if (!rule_identifiers_list_value->IsType(Value::TYPE_NULL)) {
+    EXTENSION_FUNCTION_VALIDATE(args_->GetList(1, &rule_identifiers_list));

[not at google - send to devlin, 2012/02/07 03:25:10]

This could fail if the extension does a call like

chrome.declarative.addRules("hello"), or anything else in the place of "hello"
which isn't a list or null.

We shouldn't be killing the extension in those cases, just returning an error to
that call.

[not at google - send to devlin, 2012/02/07 05:08:01]

(this is a bit moot given my other comments anyway; but my bad, this code
wouldn't get reached because the renderer should already be doing validation so
killing it would be ok)

+
+    for (ListValue::iterator i = rule_identifiers_list->begin();
+         i != rule_identifiers_list->end();
+         ++i) {
+      std::string rule_id;
+      EXTENSION_FUNCTION_VALIDATE((*i)->GetAsString(&rule_id));
+      rule_identifiers.push_back(rule_id);
+    }
+
+    // If the extension passed an empty list, this means no rules can be
+    // contained in the list and we bail out.
+    if (rule_identifiers.empty())
+      return true;

[not at google - send to devlin, 2012/02/07 03:25:10]

This is still a confusing API, where an empty list -> get all rules.
 - RemoveRules should just remove what it's told, even if that's empty. That
would let you delete this logic.
 - Add a new method RemoveAllRules, or RemoveRules overloaded without a list
argument, to remove all the rules and call that.

[battre, 2012/02/07 18:45:33]

Done.

+  }
+
+  RulesRegistryService* rules_registry =
+      profile()->GetExtensionService()->GetRulesRegistryService();
+
+  return rules_registry->RemoveRules(event_name, extension_id(),
+                                     rule_identifiers, &error_);
 }
 
 bool GetRulesFunction::RunImpl() {
-  // LOG(ERROR) << "GetRulesFunction called";
-  result_.reset(new ListValue());
+  std::string event_name;
+  EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &event_name));
+

[not at google - send to devlin, 2012/02/07 03:25:10]

Same comment here as in RemoveRulesFunction, both in terms of code structure and
for the Get(All)Rules thing.

[battre, 2012/02/07 18:45:33]

Done.

+  std::vector<std::string> rule_identifiers;
+
+  Value* rule_identifiers_list_value = NULL;
+  EXTENSION_FUNCTION_VALIDATE(args_->Get(1, &rule_identifiers_list_value));
+
+  ListValue* rule_identifiers_list = NULL;
+  if (!rule_identifiers_list_value->IsType(Value::TYPE_NULL)) {
+    EXTENSION_FUNCTION_VALIDATE(args_->GetList(1, &rule_identifiers_list));
+
+    for (ListValue::iterator i = rule_identifiers_list->begin();
+         i != rule_identifiers_list->end();
+         ++i) {
+      std::string rule_id;
+      EXTENSION_FUNCTION_VALIDATE((*i)->GetAsString(&rule_id));
+      rule_identifiers.push_back(rule_id);
+    }
+
+    // If the extension passed an empty list, this means no rules can be
+    // contained in the list and we bail out.
+    if (rule_identifiers.empty()) {
+      result_.reset(new ListValue);
+      return true;
+    }
+  }
+
+  RulesRegistryService* rules_registry =
+      profile()->GetExtensionService()->GetRulesRegistryService();
+
+  typedef std::vector<DictionaryValue*> RulesList;

[not at google - send to devlin, 2012/02/07 03:25:10]

IMO typedef unnecessary.

[battre, 2012/02/07 18:45:33]

Done.

+
+  RulesList rules;
+  rules_registry->GetRules(
+      event_name, extension_id(), rule_identifiers, &rules);
+
+  scoped_ptr<ListValue> result(new ListValue);
+  for (RulesList::iterator i = rules.begin(); i != rules.end(); ++i)
+    result->Append(*i);
+
+  result_.reset(result.release());
   return true;
 }