[GRC] Coordination Unit Graph Observer

services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h

+// Copyright 2017 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 SERVICES_RESOURCE_COORDINATOR_COORDINATION_UNIT_COORDINATION_UNIT_GRAPH_OBSERVER_H_
+#define SERVICES_RESOURCE_COORDINATOR_COORDINATION_UNIT_COORDINATION_UNIT_GRAPH_OBSERVER_H_
+
+#include <memory>
+#include <unordered_map>
+
+#include "base/macros.h"
+#include "services/resource_coordinator/coordination_unit/coordination_unit_impl.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"
+
+namespace resource_coordinator {
+
+// An observer API for the CoordinationUnitGraph maintained by GRC.

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

nit: CoordinationUnitGraph is not a real class. So no need to use camel case.
Just use "coordination unit graph".

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

Done.

+//
+// Observers are instantiated when the resource_coordinator serivce

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

nit: s/serivce/service/

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

Done.

+// is created and are destroyed when the resource_coordinator service
+// is destroyed. Therefore observers are guaranteed to be alive before
+// any coordination unit is created and will be alive after any
+// coordination unit is destroyed. Additionally, any
+// Coordination Unit reachable within a callback will always be
+// initialized and valid.
+//
+// To create and install a new observer:
+//   (1) derive from this class
+//   (2) register in CoordinationUnitManager::RegisterObserver
+//       inside of CoordinationUnitManager::CoordinationUnitManager
+//

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

This document needs to be updated because the life of an observer can be shorter
now.

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

The observer lifetime behavior has returned to being bound to the lifetime of
GRC for now -- this no longer needs to be addressed. 

+class CoordinationUnitGraphObserver {
+ public:
+  CoordinationUnitGraphObserver();

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

It is probably better to also have some comments for the default constructor to
indicate that the default one does not filter anything.

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

Filtering has been removed for now -- this no longer needs to be addressed.

+  // The CoordinationUnitGraphObserver will only call

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

s/CoordinationUnitGraphObserver/CoordinationUnitManager ?

Actually, Maybe it is better not mention CUManager here because now the observer
can be created by some other objects. Just say "CUCreated is called when ..."

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

Filtering has been removed for now -- this no longer needs to be addressed.

+  // CoordinationUnitGraphObserver::CoordinationUnitCreated when the
+  // CoordinationUnit's CoordinationUnitID.type matches filter.
+  explicit CoordinationUnitGraphObserver(CoordinationUnitType filter);
+  virtual ~CoordinationUnitGraphObserver();
+
+  // Determines whether or not OnCoordinationUnitCreated should be
+  // invoked on the newly created CoordinationUnit.

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

Nit: This comment is too specific to OnCoordinationUnitCreated. Maybe add a
general description at the beginning, something like:

Determines whether or not this observer should observe the coordination unit.
This will affect whether or not OnCoordinationUnitCreated should be invoked on
the newly created CoordinationUnit.

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

Filtering has been removed for now -- this no longer needs to be addressed.

+  bool ShouldObserve(CoordinationUnitImpl* coordination_unit);

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

defer the functionality to CUGraphObserver itself in OnCoordinationUnitCreated.

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

Done.

+
+  // Called whenever any new CoordinationUnit of interest is created
+  // (i.e. ShouldObserve evaluates to true).
+  virtual void OnCoordinationUnitCreatedEvent(

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

OnCoordinationUnitCreated

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

Done. Event has been removed from On*Event.

+      CoordinationUnitImpl* coordination_unit) {}
+
+  // Called whenever a new parent-child relationship occurs where the
+  // |coordination_unit| is the parent and the |child_coordination_unit|
+  // type matches the CoordinationUnitType filter that the observer
+  // was registered with (e.g. CoordinationUnit::ObserveAddChildEvent).
+  virtual void OnChildAddedEvent(

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

I think we can remove "Event" in the API to be concise, so just |OnChildAdded|.
Same for other ones.

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

OnChildAdded

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

Done.

+      const CoordinationUnitImpl* coordination_unit,
+      const CoordinationUnitImpl* child_coordination_unit) {}
+
+  // Called whenever a new parent-child relationship occurs where the
+  // |coordination_unit| is the child and the |parent_coordination_unit|
+  // type matches the CoordinationUnitType filter that the observer
+  // was registered with (e.g. CoordinationUnit::ObserveAddParentEvent).
+  virtual void OnParentAddedEvent(

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

OnParentAdded.

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

Done.

+      const CoordinationUnitImpl* coordination_unit,
+      const CoordinationUnitImpl* parent_coordination_unit) {}
+
+  // Called whenever a |property| within the |coordination_unit|'s internal
+  // property store changes and the |property| type matches the
+  // mojom::Property filter that the observer was registered with
+  // (e.g. CoordinationUnit::ObservePropertyChangedEvent).
+  virtual void OnPropertyChangedEvent(

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

OnPropertyChanged

Will it be useful to also pass the original value?

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

If it is determined to be useful for someone we can added it. I am willing to
take on the yak shaving for that.

+      const CoordinationUnitImpl* coordination_unit,
+      mojom::PropertyType property) {}
+
+  // Called whenever parent-child relationship ends where the
+  // |coordination_unit| was the parent and the |child_coordination_unit|
+  // type matches the CoordinationUnitType filter that the observer
+  // was registered with (e.g. CoordinationUnit::ObserveAddChildEvent).
+  virtual void OnChildRemovedEvent(

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

OnChildRemoved

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

Done.

+      const CoordinationUnitImpl* coordination_unit,
+      const CoordinationUnitImpl* child_coordination_unit) {}
+
+  // Called whenever parent-child relationship ends where the
+  // |coordination_unit| was the child and the |child_coordination_unit|
+  // type matches the CoordinationUnitType filter that the observer
+  // was registered with (e.g. CoordinationUnit::ObserveRemoveParentEvent).
+  virtual void OnParentRemovedEvent(

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

OnParentRemoved

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

Done.

+      const CoordinationUnitImpl* coordination_unit,
+      const CoordinationUnitImpl* parent_coordination_unit) {}
+
+  // Called when the |coordination_unit| is about to be destroyed.
+  virtual void OnWillBeDestroyedEvent(

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

OnWillBeDestroyed

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

Done.

+      const CoordinationUnitImpl* coordination_unit) {}
+
+ protected:
+  CoordinationUnitType filter_;

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

Why does this need to be protected? I did a quick search and didn't find any
subclass using it directly. Even if some subclass wants to use it, it is better
to put it in private and have a protected filter() function.

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

Filter has been removed -- this is no longer applicable.

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CoordinationUnitGraphObserver);
+};
+
+}  // namespace resource_coordinator
+
+#endif  // SERVICES_RESOURCE_COORDINATOR_COORDINATION_UNIT_COORDINATION_UNIT_GRAPH_OBSERVER_H_