[GRC] Coordination Unit Graph Observer V3

services/resource_coordinator/coordination_unit/coordination_unit_manager.cc

Index: services/resource_coordinator/coordination_unit/coordination_unit_manager.cc
diff --git a/services/resource_coordinator/coordination_unit/coordination_unit_manager.cc b/services/resource_coordinator/coordination_unit/coordination_unit_manager.cc
index 2a31c2551deb93332a0ca0f7bdad1ca4fc431531..c92fdaf2fded09373974905ed834751e38cfcf83 100644
--- a/services/resource_coordinator/coordination_unit/coordination_unit_manager.cc
+++ b/services/resource_coordinator/coordination_unit/coordination_unit_manager.cc
@@ -4,7 +4,6 @@
 
 #include "services/resource_coordinator/coordination_unit/coordination_unit_manager.h"
 
-#include <memory>
 #include <utility>
 
 #include "base/bind.h"
@@ -36,18 +35,27 @@ void CoordinationUnitManager::RegisterObserver(
   observers_.push_back(std::move(observer));
 }
 
-void CoordinationUnitManager::NotifyObserversCoordinationUnitCreated(
+void CoordinationUnitManager::AddObserversToCoordinationUnit(
     CoordinationUnitImpl* coordination_unit) {
   for (auto& observer : observers_) {
-    if (observer->ShouldObserve(coordination_unit)) {
-      observer->OnCoordinationUnitCreatedEvent(coordination_unit);
-    }
+    coordination_unit->AddObserver(observer.get());
   }
 }
 
-void CoordinationUnitManager::NotifyObserversCoordinationUnitWillBeDestroyed(
+void CoordinationUnitManager::OnCoordinationUnitCreated(
     CoordinationUnitImpl* coordination_unit) {
-  coordination_unit->WillBeDestroyed();
+  AddObserversToCoordinationUnit(coordination_unit);

[lpy, 2017/06/20 19:53:46]

Should we leave this to CUGraphObserver to decide whether or not it wants to add
itself to a CU? I imagine that in a CUGraphObserver implementation, it would
have code like:

void ConcreteCoordinationUnitObserver::OnCoordinationUnitCreated(cu) {
  if (cu is the type of CUs I want to observe) {
    cu->AddObserver(this);
  }
}

WDYT?

[oystein (OOO til 10th of July), 2017/06/20 20:21:07]

Minor API nit: As discussed in the meeting, I think it's slightly cleaner to
separate the event callbacks from the "should I observe X" virtual which always
gets called.

so:
virtual bool ShouldObserve(cu) = 0;
virtual void OnCoordinationUnitCreated(cu) {};

[matthalp, 2017/06/20 20:56:09]

I agree with oysteine  that ShouldObserve should be used in place of deciding
within CoordinationUnitCreated. This decoupling allows us to make the
CoordinationUnitCreated(const CoordinationUnitImpl*) which is consistent with
the parameters for the rest of the API.

+
+  for (auto& observer : observers_) {
+    observer->OnCoordinationUnitCreated(coordination_unit);
+  }
+}
+
+void CoordinationUnitManager::OnCoordinationUnitWillBeDestroyed(
+    CoordinationUnitImpl* coordination_unit) {
+  for (auto& observer : observers_) {
+    observer->OnCoordinationUnitWillBeDestroyed(coordination_unit);
+  }
 }
 
 }  // namespace resource_coordinator