RulesRegistry for declarative APIs.

chrome/browser/extensions/api/declarative/rules_registry_service.h

+// 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.
+
+#ifndef CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_RULES_REGISTRY_SERVICE_H__
+#define CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_RULES_REGISTRY_SERVICE_H__
+#pragma once
+
+#include <map>
+#include <set>
+#include <string>
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/values.h"
+#include "chrome/browser/extensions/api/declarative/rules_registry.h"
+#include "chrome/browser/profiles/profile_keyed_service.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+
+class Profile;
+
+namespace extensions {
+class RuleIdentifier;
+}
+
+namespace content {
+class NotificationSource;
+class NotificationSource;
+}
+
+namespace extensions {
+
+// This is a dispatcher that delegates function calls to other RuleRegistries.
+// Use the RulesRegistryServiceFactory to retrieve an instance of this class.

[not at google - send to devlin, 2012/02/02 11:59:16]

Why the need for RulesRegistryServiceFactory?  Can this just directly belong to
a Profile (or an ExtensionService, which is basically a dumping ground for
extensions things that are tied to the lifetime of a Profile).

Reading the documentation about ProfileFactoryServiceBlahStuff, it seems to be
for Profile-lifetime objets that have initialisation interdependencies and
therefore complicated initialisation procedures, so hopefully unnecessary here.

[battre, 2012/02/02 19:12:36]

I raised this concern in a previous Patch Set version as well. I had the
previous request not to let ExtensionService grow but I think in this case it is
still the cleaner solution. I will get rid of the Factory.

Done.

[Aaron Boodman, 2012/02/03 18:04:51]

It is OK to add members to ExtensionService right now, because there is no other
home for them. But I don't want significant additional logic in there. See:
crbug.com/104095 for the bigger context.

+class RulesRegistryService
+    : public ProfileKeyedService, content::NotificationObserver {
+ public:
+  RulesRegistryService();
+  virtual ~RulesRegistryService();
+  void Init(Profile* profile);
+
+  // Registers a RuleRegistry such that all calls for |event_name| are
+  // dispatched to this RuleRegistry.
+  // Public for testing.
+  void RegisterRuleRegistry(

[not at google - send to devlin, 2012/02/02 11:59:16]

s/Rule/Rules/

[battre, 2012/02/02 19:12:36]

Done.

+      const std::string& event_name,
+      scoped_ptr<RulesRegistry> rule_registry);
+
+  // RulesRegistry alike implementation. We add an additional |event_name|
+  // to each function, which represents the name of the JavaScript object
+  // on which the function was called.

[not at google - send to devlin, 2012/02/02 11:59:16]

Is it possible to move much of the logic in this class to a decorator-type
RulesRegistry?  And have RulesRegistryService be a wrapper around an event_name
-> RulesRegistry map?

Something like

== rules_registry ==

interface RulesRegistry {
  // you already have this
}

== initializing_rules_registry ==

// New class which has the job of filling in missing fields during AddRules,
then
// delegating that call to another RulesRegistry.
class InitializingRulesRulesRegistry : RulesRegistry {
 public:
  InitializingRulesRegistry(scoped_ptr<RulesRegistry> delegate)
      : delegate_(delegate) {}

  // RulesRegistry implementation.
  std::string AddRules(...) {
    CheckAndFillInOptionalRules(...);
    return delegate_->AddRules(...);
  }

  std::string RemoveRules(...) {
    return delegte_->RemoveRules(...);
  }

 private:
  scoped_ptr<RulesRegistry> delegate_;
}

== rules_registry_service ==

class RulesRegistryService {
  ..

  void RegisterRulesRegistry(name, delegate_registry) {
    rule_registries_map_[name] = new
InitializingRulesRegistry(delegate_registry);
  }
}

== etc with other implementations ==

[Aaron Boodman, 2012/02/02 16:28:51]

Feels like overdesign to use a decorator to add logic to one method (AddRules).
Also there is only ever one decorator here (the one that does the rest of the
parsing of AddRules) - it's not like it's a dynamic system where you many
different combinations of decorations might apply at runtime.

Could you just as easily move the rest of the parsing code out into
declarative_api.cc and pass something fully-parsed into
RulesRegistryService::AddRules()?

Please remember that the higher-level patterns also have a cost! Everyone
reading the code must parse the system into a structure in their head before
they can reason about it.

[Aaron Boodman, 2012/02/02 16:30:25]

Sorry, I meant to remove this paragraph. I think the code is fine just as it is.

[battre, 2012/02/02 19:12:36]

At first I considered this overdesign as well. But then I recognized that is
also makes the RuleIdentifier slightly cleaner because we have different scopes
for each event name. I think I like this design.

+  bool AddRules(const std::string& event_name,
+                const std::string& extension_id,
+                const std::vector<DictionaryValue*>& rules,
+                RulesRegistry::SetErrorCallback set_error_callback);
+  bool RemoveRules(const std::string& event_name,
+                   const std::string& extension_id,
+                   const std::vector<std::string>& rule_identifiers,
+                   RulesRegistry::SetErrorCallback set_error_callback);
+  void GetRules(const std::string& event_name,
+                const std::string& extension_id,
+                const std::vector<std::string>& rule_identifiers,
+                std::vector<DictionaryValue*>* out);
+  void OnExtensionUnloaded(const std::string& extension_id);
+
+ private:
+  // Maps event names to RuleRegistries that handle these events.
+  // Owns the RuleRegistry objects.
+  typedef std::map<std::string, RulesRegistry*> RulesRegistryMap;
+
+  // Returns whether any existing rule is registered with identifier |id|.
+  bool IsUniqueId(const RuleIdentifier& id) const;
+
+  // Creates a ID that is unique within the scope of
+  // (|extension_id|, |event_name|).
+  std::string GenerateUniqueId(std::string extension_id,
+                               std::string event_name);
+
+  // Verifies that all |rules| have unique IDs or initializes them with
+  // unique IDs if they don't have one. In case of duplicate IDs, this function
+  // calls |set_error_callback| and returns false.
+  bool CheckAndFillInOptionalRules(
+    const std::string& event_name,
+    const std::string& extension_id,
+    const std::vector<DictionaryValue*>& rules,
+    RulesRegistry::SetErrorCallback set_error_callback);
+
+  // Initializes the priority fields in case they have not been set.
+  void FillInOptionalPriorities(const std::vector<DictionaryValue*>& rules);
+
+  // Implementation of content::NotificationObserver.
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
+
+  content::NotificationRegistrar registrar_;
+
+  RulesRegistryMap rule_registries_;
+
+  std::set<RuleIdentifier> used_rule_identifiers_;
+  int last_generated_rule_identifier_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(RulesRegistryService);
+};
+
+}
+
+#endif  // CHROME_BROWSER_EXTENSIONS_API_DECLARATIVE_RULES_REGISTRY_SERVICE_H__