Decoupled the SchemaRegistryService from SchemaRegistry.

components/policy/core/common/schema_registry.h

 // Copyright 2013 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 COMPONENTS_POLICY_CORE_COMMON_SCHEMA_REGISTRY_H_
 #define COMPONENTS_POLICY_CORE_COMMON_SCHEMA_REGISTRY_H_
 
 #include <set>
 
 #include "base/basictypes.h"
@@ skipping 25 matching lines @@
     // (e.g. for periodic reloads).
     virtual void OnSchemaRegistryUpdated(bool has_new_schemas) = 0;
 
     // Invoked when all policy domains become ready.
     virtual void OnSchemaRegistryReady() = 0;
 
    protected:
     virtual ~Observer();
   };
 
+  // This observer is only meant to be used by subclasses.

[bartfab (slow), 2014/06/20 14:48:01]

Why make it public then?

[Joao da Silva, 2014/06/20 15:30:14]

Ha :-) It was protected initially, but it doesn't work as you'd expect.

Basically ForwardingSchemaRegistry can't inherit from this if this is protected,
even if it inherits from SchemaRegistry.

Same for the AddInternalObserver() method: you can call that method on |this|
but you can't call it on other objects of the superclass (you can call it on
objects of the same class though).

[bartfab (slow), 2014/06/20 15:44:18]

Makes sense, when you take the meaning of "protected" absolutely literally (as I
guess compilers do). LGTM as-is then. I wonder though whether a cryptic name
like InternalObserver is really necessary/useful here. It could be called
something like LifetimeObserver. It is mostly useful for subclasses but if some
other class ever decides it wants to track this class' lifetime, why not let it?

[Joao da Silva, 2014/06/20 15:47:43]

Sounds reasonable. The current name hints at its current usage; let's rename it
when we get an external observer then.

+  class POLICY_EXPORT InternalObserver {
+   public:
+    // Invoked when |registry| is about to be destroyed.
+    virtual void OnSchemaRegistryShuttingDown(SchemaRegistry* registry) = 0;
+
+   protected:
+    virtual ~InternalObserver();
+  };
+
   SchemaRegistry();
   virtual ~SchemaRegistry();
 
   const scoped_refptr<SchemaMap>& schema_map() const { return schema_map_; }
 
   // Register a single component.
   void RegisterComponent(const PolicyNamespace& ns,
                          const Schema& schema);
 
   // Register a list of components for a given domain.
   virtual void RegisterComponents(PolicyDomain domain,
                                   const ComponentMap& components);
 
   virtual void UnregisterComponent(const PolicyNamespace& ns);
 
   // Returns true if all domains have registered the initial components.
   bool IsReady() const;
 
   // This indicates that the initial components for |domain| have all been
   // registered. It must be invoked at least once for each policy domain;
   // subsequent calls for the same domain are ignored.
   void SetReady(PolicyDomain domain);
 
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
-  bool HasObservers() const;
+  void AddInternalObserver(InternalObserver* observer);

[bartfab (slow), 2014/06/20 14:48:01]

Why are these public if they are meant for subclasses only?

[Joao da Silva, 2014/06/20 15:30:14]

See above

+  void RemoveInternalObserver(InternalObserver* observer);
 
  protected:
   void Notify(bool has_new_schemas);
 
   scoped_refptr<SchemaMap> schema_map_;
 
  private:
   ObserverList<Observer, true> observers_;
+  ObserverList<InternalObserver, true> internal_observers_;
   bool domains_ready_[POLICY_DOMAIN_SIZE];
 
   DISALLOW_COPY_AND_ASSIGN(SchemaRegistry);
 };
 
 // A registry that combines the maps of other registries.
-class POLICY_EXPORT CombinedSchemaRegistry : public SchemaRegistry,
-                                             public SchemaRegistry::Observer {
+class POLICY_EXPORT CombinedSchemaRegistry
+    : public SchemaRegistry,
+      public SchemaRegistry::Observer,
+      public SchemaRegistry::InternalObserver {
  public:
   CombinedSchemaRegistry();
   virtual ~CombinedSchemaRegistry();
 
   void Track(SchemaRegistry* registry);
-  void Untrack(SchemaRegistry* registry);
 
+  // SchemaRegistry:
   virtual void RegisterComponents(PolicyDomain domain,
                                   const ComponentMap& components) OVERRIDE;
-
   virtual void UnregisterComponent(const PolicyNamespace& ns) OVERRIDE;
 
+  // SchemaRegistry::Observer:
   virtual void OnSchemaRegistryUpdated(bool has_new_schemas) OVERRIDE;
+  virtual void OnSchemaRegistryReady() OVERRIDE;
 
-  virtual void OnSchemaRegistryReady() OVERRIDE;
+  // SchemaRegistry::InternalObserver:
+  virtual void OnSchemaRegistryShuttingDown(SchemaRegistry* registry) OVERRIDE;
 
  private:
   void Combine(bool has_new_schemas);
 
   std::set<SchemaRegistry*> registries_;
   scoped_refptr<SchemaMap> own_schema_map_;
 
   DISALLOW_COPY_AND_ASSIGN(CombinedSchemaRegistry);
 };
 
+// A registry that wraps another schema registry.
+class POLICY_EXPORT ForwardingSchemaRegistry
+    : public SchemaRegistry,
+      public SchemaRegistry::Observer,
+      public SchemaRegistry::InternalObserver {
+ public:
+  // This registry will stop updating its SchemaMap when |wrapped| is
+  // destroyed.
+  explicit ForwardingSchemaRegistry(SchemaRegistry* wrapped);
+  virtual ~ForwardingSchemaRegistry();
+
+  // SchemaRegistry:
+  virtual void RegisterComponents(PolicyDomain domain,
+                                  const ComponentMap& components) OVERRIDE;
+  virtual void UnregisterComponent(const PolicyNamespace& ns) OVERRIDE;
+
+  // SchemaRegistry::Observer:
+  virtual void OnSchemaRegistryUpdated(bool has_new_schemas) OVERRIDE;
+  virtual void OnSchemaRegistryReady() OVERRIDE;
+
+  // SchemaRegistry::InternalObserver:
+  virtual void OnSchemaRegistryShuttingDown(SchemaRegistry* registry) OVERRIDE;
+
+ private:
+  SchemaRegistry* wrapped_;
+
+  DISALLOW_COPY_AND_ASSIGN(ForwardingSchemaRegistry);
+};
+
 }  // namespace policy
 
 #endif  // COMPONENTS_POLICY_CORE_COMMON_SCHEMA_REGISTRY_H_