[GRC] Coordination Unit Graph Observer

services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h

Index: services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h
diff --git a/services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h b/services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h
new file mode 100644
index 0000000000000000000000000000000000000000..2a88a0a33cc744b037fe3053f48a176df0f3affa
--- /dev/null
+++ b/services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h
@@ -0,0 +1,52 @@
+// 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/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 {
+
+class CoordinationUnitImpl;
+
+// An observer API for the CoordinationUnitManager within GRC
+//
+// To create a new observer:
+//   (1) derive from this class
+//   (2) register in CoordinationUnitManager::RegisterObserver
+//       inside of CoordinationUnitManager::CoordinationUnitManager
+//       in coordination_unit_manager.cc
+class CoordinationUnitGraphObserver {

[zhenw, 2017/06/13 18:38:41]

Do observers outlive all CUs?

The real question is whether we should support removing listeners. Currently it
seems only AddListener is supported. If an observer dies before some CU, it
becomes a problem when that CU later wants to notify the already-dead observer.

[matthalp, 2017/06/13 20:57:10]

(1)  All observers will outlive any CU created within GRC.

  Furthermore, the observers will be alive as long as the 
  ResourceCoordinatorService is alive. Observers are be 
  registered in the CoordinationUnitManager constructor, 
  which is instantiated when the ResourceCoordinatorService
  is created. Observers are not intended to be added to the
  CoordinationUnitManager dynamically.

(2) Future support for RemoveListener

  I have no problem adding a RemoveListener capability once
  we have a definitive use case for it (which I do believe
  there are). It will also add some additional complexity as
  now there has to be a way to identify uniquely identify the
  listener to be removed.

  If you feel like we should add RemoveListener support now or
  at least converge on an implementation for it, I am more than
  happy to do so.

[Zhen Wang, 2017/06/14 00:33:03]

OK. As long as observers outlive any CU, it is fine. Please also document this.

+ public:
+  CoordinationUnitGraphObserver();
+  // The CoordinationUnitmanagerObserver will only call
+  // CoordinationUnitGraphObserver::CoordinationUnitCreated when the
+  // CoordinationUnit's CoordinationUnitID.type matches filter.
+  explicit CoordinationUnitGraphObserver(CoordinationUnitType filter);

[zhenw, 2017/06/13 18:38:41]

I am not entirely sure the reason of using filters. Currently there 2 types of
filters:
1. CoordinationUnitGraphObserver(CoordinationUnitType filter)
2. AddListener(listener, filter)

For the 1st one, when a CU is created, manager can just broadcast to all
observers. The observer itself can check the CU type and decide whether to add
listeners.

For the 2nd one, when a CU adds a child, the CU can just broadcast to all
listeners. The listener can just check the CU type and decide whether to
proceed.

The down side of using filter is that the filtering functionality is redundant,
because it can always be achieved by the observers do some checking internally.

The benefit I see is mostly performance, i.e., save work to talk to some
observer/listener. If that is the main reason, map should be used instead of
vector.

[matthalp, 2017/06/13 20:57:10]

The main intention for filtering was for code health, but also
agree there is a performance benefit with the map approach:

  (1) Readability
    The if-/switch-statement adds an extra level of indentation

  (2) Modularity
    The callback methods can focus on a particular event type.
    I think the TabCPUUsageObserver does a good job demonstrating this:
    https://codereview.chromium.org/2925123002
    Nonetheless, I'm open to removing this capability or figuring out
    an alternative approach. I was motivated by the webRequest WebExtension
    API which uses filtering to isolate traffic from certain domains:
    https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest

[Zhen Wang, 2017/06/14 00:33:03]

OK. Let's keep filtering.

+  virtual ~CoordinationUnitGraphObserver();
+
+  CoordinationUnitType filter() const { return filter_; }
+
+  // Called whenever any new CoordinationUnit of any type is created. Note that
+  // this will be called after any specialized *CoordinationUnitCreated (e.g.
+  // FrameCoordinationUnitCreation, etc.) is called.
+  virtual void CoordinationUnitCreated(
+      CoordinationUnitImpl* coordination_unit) {}
+
+ private:
+  CoordinationUnitType filter_;
+
+  DISALLOW_COPY_AND_ASSIGN(CoordinationUnitGraphObserver);
+};
+
+}  // namespace resource_coordinator
+
+#endif  // SERVICES_RESOURCE_COORDINATOR_COORDINATION_UNIT_COORDINATION_UNIT_GRAPH_OBSERVER_H_