[GRC] Coordination Unit Graph Observer

services/resource_coordinator/coordination_unit/coordination_unit_impl.h

Index: services/resource_coordinator/coordination_unit/coordination_unit_impl.h
diff --git a/services/resource_coordinator/coordination_unit/coordination_unit_impl.h b/services/resource_coordinator/coordination_unit/coordination_unit_impl.h
index 94ebb044e624c8a0b0f6217f2b7e71e05ed5a62d..492c34c973d6afb55e3b48731f36963b74304af1 100644
--- a/services/resource_coordinator/coordination_unit/coordination_unit_impl.h
+++ b/services/resource_coordinator/coordination_unit/coordination_unit_impl.h
@@ -6,23 +6,83 @@
 #define SERVICES_RESOURCE_COORDINATOR_COORDINATION_UNIT_COORDINATION_UNIT_IMPL_H_
 
 #include <list>
+#include <map>
 #include <memory>
 #include <set>
 #include <unordered_map>
 #include <utility>
+#include <vector>
 
+#include "base/callback.h"
 #include "base/optional.h"
 #include "base/values.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
 #include "mojo/public/cpp/bindings/interface_request.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
 #include "services/resource_coordinator/public/cpp/coordination_unit_id.h"
+#include "services/resource_coordinator/public/cpp/coordination_unit_types.h"
 #include "services/resource_coordinator/public/interfaces/coordination_unit.mojom.h"
 #include "services/resource_coordinator/public/interfaces/coordination_unit_provider.mojom.h"
 #include "services/service_manager/public/cpp/service_context_ref.h"
 
 namespace resource_coordinator {
 
+class CoordinationUnitGraphObserver;
+
+// Collection to manage CoordinationUnitEvent listeners.

[Zhen Wang, 2017/06/19 22:54:25]

s/CoordinationUnitEvent listeners/CoordinatorUnitGraphObservers

[matthalp, 2017/06/20 19:02:52]

Removed CoordinationGraphObserverRegistry -- no longer needs to be addressed.

+template <typename Filter>
+class CoordinationUnitGraphObserverRegistry {

[Zhen Wang, 2017/06/19 22:54:29]

Remember to add RemoveObserver API. (This is just a reminder to myself for our
discussion offline).

[lpy, 2017/06/19 23:21:38]

Let's leave the filtering to each CUGraphObserver implementation. An observed CU
should do nothing but forward changes to the CUGraphObserver that is observing
it, and it's the CUGraphObserver's responsibility to select whatever messages it
needs. A CU can then only hold a list of observers that are observing the graph
it belongs.

[oystein (OOO til 10th of July), 2017/06/20 18:39:23]

Agreed. I think it's better to defer the per-event granularity until there's a
real need for it for performance reasons, which should simplify this CL a lot
(i.e. a single list, and no filters).

i.e.

if (observer->ShouldObserve(cu) {
  cu->AddObserver)
}

[matthalp, 2017/06/20 19:02:52]

Removed CoordinationGraphObserverRegistry -- no longer needs to be addressed.

However, CoordinationUnitImpl::RemoveObserver has been added.

+ public:
+  CoordinationUnitGraphObserverRegistry() = default;
+  ~CoordinationUnitGraphObserverRegistry() = default;
+
+  // callbacks paired with this kNoFilter will always be invoked
+  static const Filter kNoFilter = static_cast<Filter>(0);
+
+  void AddObserver(CoordinationUnitGraphObserver* observer, Filter filter) {
+    registry_[filter].push_back(observer);
+  }
+
+  void AddObserver(CoordinationUnitGraphObserver* observer) {

[Zhen Wang, 2017/06/19 22:54:28]

Comments needed for the above two functions.

[matthalp, 2017/06/20 19:02:52]

Removed CoordinationGraphObserverRegistry -- no longer needs to be addressed.

+    AddObserver(observer, kNoFilter);
+  }
+
+  // Get the observers who are are either registered to be invoked for
+  // a specific filter or were registered with the kNoFilter.
+  // TODO(matthalp) add iterator support to replace this call and
+  // avoid excessive copying that currently occurs
+  std::vector<CoordinationUnitGraphObserver*> GetObserversForFilter(
+      Filter filter) {
+    std::vector<CoordinationUnitGraphObserver*> observers;
+
+    // Get listeners registered with a listener first.
+    AppendObservers(&observers, filter);
+
+    // Get listeners registered without a filter after.

[Zhen Wang, 2017/06/19 22:54:25]

I think it should be s/without/with here? If you get the comment here correctly,
the comment for the above |AppendObservers| on line 58 is not needed.

+    if (filter != kNoFilter) {
+      AppendObservers(&observers, kNoFilter);
+    }
+
+    return observers;
+  }
+
+  std::vector<CoordinationUnitGraphObserver*> GetObserversWithoutAFilter() {

[Zhen Wang, 2017/06/19 22:54:25]

nit: s/GetObserversWithoutAFilter/GetObserversWithoutFilter (removing "A").

+    return GetObserversForFilter(kNoFilter);
+  }
+
+ private:
+  void AppendObservers(std::vector<CoordinationUnitGraphObserver*>* observers,
+                       Filter filter) {
+    observers->insert(observers->end(), registry_[filter].begin(),
+                      registry_[filter].end());
+  }
+
+  // TODO(matthalp) Consider using a std::unordered_map instead.
+  std::map<Filter, std::vector<CoordinationUnitGraphObserver*>> registry_;
+
+  DISALLOW_COPY_AND_ASSIGN(CoordinationUnitGraphObserverRegistry);
+};
+
 class CoordinationUnitImpl : public mojom::CoordinationUnit {
  public:
   CoordinationUnitImpl(
@@ -55,10 +115,31 @@ class CoordinationUnitImpl : public mojom::CoordinationUnit {
   // Clear property from internal key-value store
   void ClearProperty(mojom::PropertyType property);
   // Retrieve property from internal key-value store
-  base::Value GetProperty(mojom::PropertyType property);
+  base::Value GetProperty(mojom::PropertyType property) const;
   // Set property from internal key-value store
   void SetProperty(mojom::PropertyType property, base::Value value);
 
+  // Notify an instance it will be destroyed before its destructor is
+  // actually called.
+  void WillBeDestroyed();
+
+  void ObserveOnChildAddedEvent(CoordinationUnitGraphObserver* observer,

[Zhen Wang, 2017/06/19 22:54:26]

Can you also add some comments for those APIs.

[matthalp, 2017/06/20 19:02:52]

Filter has been removed -- no long applicable.

+                                CoordinationUnitType child_filter);
+  void ObserveOnChildAddedEvent(CoordinationUnitGraphObserver* observer);

[Zhen Wang, 2017/06/19 22:54:25]

I forget if using default arguments are allowed in chromium style, which would
lead to cleaner code here. Double check with Oystein.

If not allowed, I suggest separating those with blank lines for better
readability. For example:

void ObserveOnChildAddedEvent(CoordinationUnitGraphObserver* observer);
void ObserveOnChildAddedEvent(CoordinationUnitGraphObserver* observer,
                              CoordinationUnitType child_filter);

void ObserveOnChildRemovedEvent(CoordinationUnitGraphObserver* observer);
void ObserveOnChildRemovedEvent(CoordinationUnitGraphObserver* observer,
                                CoordinationUnitType child_filter);

void ObserveOnParentAddedEvent(CoordinationUnitGraphObserver* observer);
void ObserveOnParentAddedEvent(CoordinationUnitGraphObserver* observer,
                               CoordinationUnitType parent_filter);

...

[oystein (OOO til 10th of July), 2017/06/20 18:39:23]

AFAIK default arguments are allowed on non-virtual functions, but I don't see
them used a lot.

[matthalp, 2017/06/20 19:02:52]

Filter has been removed -- no long applicable.

+  void ObserveOnParentAddedEvent(CoordinationUnitGraphObserver* observer,
+                                 CoordinationUnitType parent_filter);
+  void ObserveOnParentAddedEvent(CoordinationUnitGraphObserver* observer);
+  void ObserveOnPropertyChangedEvent(CoordinationUnitGraphObserver* observer,
+                                     mojom::PropertyType property_filter);
+  void ObserveOnPropertyChangedEvent(CoordinationUnitGraphObserver* observer);
+  void ObserveOnChildRemovedEvent(CoordinationUnitGraphObserver* observer,
+                                  CoordinationUnitType child_filter);
+  void ObserveOnChildRemovedEvent(CoordinationUnitGraphObserver* observer);
+  void ObserveOnParentRemovedEvent(CoordinationUnitGraphObserver* observer,
+                                   CoordinationUnitType parent_filter);
+  void ObserveOnParentRemovedEvent(CoordinationUnitGraphObserver* observer);
+  void ObserveOnWillBeDestroyedEvent(CoordinationUnitGraphObserver* observer);
+
  protected:
   const CoordinationUnitID id_;
   std::set<CoordinationUnitImpl*> children_;
@@ -94,6 +175,21 @@ class CoordinationUnitImpl : public mojom::CoordinationUnit {
 
   base::Optional<bool> state_flags_[kNumStateFlags];
 
+  CoordinationUnitGraphObserverRegistry<CoordinationUnitType>
+      on_child_added_event_observer_registry_;
+  CoordinationUnitGraphObserverRegistry<CoordinationUnitType>
+      on_parent_added_event_observer_registry_;
+  CoordinationUnitGraphObserverRegistry<CoordinationUnitType>
+      on_child_removed_event_observer_registry_;
+  CoordinationUnitGraphObserverRegistry<CoordinationUnitType>
+      on_parent_removed_event_observer_registry_;
+  CoordinationUnitGraphObserverRegistry<mojom::PropertyType>
+      on_property_changed_event_observer_registry_;
+  // There is nothing to filter WillBeDestroyedEventCallbacks on so the
+  // CoordinationUnitType is used as a filter placeholder
+  CoordinationUnitGraphObserverRegistry<CoordinationUnitType>
+      on_will_be_destroyed_event_observer_registry_;
+
   DISALLOW_COPY_AND_ASSIGN(CoordinationUnitImpl);
 };