[GRC] Coordination Unit Graph Observer

services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc

Index: services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc
diff --git a/services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc b/services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..7ad44f721747b13865f1859b7ebffe7e038bc11c
--- /dev/null
+++ b/services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc
@@ -0,0 +1,302 @@
+// 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.
+
+#include <string>
+#include <utility>
+
+#include "base/memory/ptr_util.h"
+#include "base/process/process_handle.h"
+#include "base/run_loop.h"
+#include "services/resource_coordinator/coordination_unit/coordination_unit_factory.h"
+#include "services/resource_coordinator/coordination_unit/coordination_unit_graph_observer.h"
+#include "services/resource_coordinator/coordination_unit/coordination_unit_impl.h"
+#include "services/resource_coordinator/coordination_unit/coordination_unit_impl_unittest_util.h"
+#include "services/resource_coordinator/coordination_unit/coordination_unit_manager.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 "testing/gtest/include/gtest/gtest.h"
+
+namespace resource_coordinator {
+
+namespace {
+
+class CoordinationUnitGraphObserverTest : public CoordinationUnitImplTestBase {
+};
+
+class TestBasicAPICoordinationUnitGraphObserver
+    : public CoordinationUnitGraphObserver {
+ public:
+  TestBasicAPICoordinationUnitGraphObserver()
+      : TestBasicAPICoordinationUnitGraphObserver(
+            CoordinationUnitType::kInvalidType) {}
+
+  void OnCoordinationUnitCreatedEvent(
+      CoordinationUnitImpl* coordination_unit) override {
+    ++on_coordination_unit_created_invocations_;
+
+    coordination_unit->ObserveOnChildAddedEvent(this);
+    coordination_unit->ObserveOnParentAddedEvent(this);
+    coordination_unit->ObserveOnPropertyChangedEvent(this);
+    coordination_unit->ObserveOnChildRemovedEvent(this);
+    coordination_unit->ObserveOnParentRemovedEvent(this);
+    coordination_unit->ObserveOnWillBeDestroyedEvent(this);
+  }
+
+  size_t on_child_added_listener_invocations() {
+    return on_child_added_listener_invocations_;
+  }
+  size_t on_parent_added_listener_invocations() {
+    return on_parent_added_listener_invocations_;
+  }
+  size_t on_coordination_unit_created_invocations() {
+    return on_coordination_unit_created_invocations_;
+  }
+  size_t on_property_changed_listener_invocations() {
+    return on_property_changed_listener_invocations_;
+  }
+  size_t on_child_removed_listener_invocations() {
+    return on_child_removed_listener_invocations_;
+  }
+  size_t on_parent_removed_listener_invocations() {
+    return on_parent_removed_listener_invocations_;
+  }
+  size_t on_will_be_destroyed_listener_invocations() {
+    return on_will_be_destroyed_listener_invocations_;
+  }
+
+  void OnChildAddedEvent(
+      const CoordinationUnitImpl* coordination_unit,
+      const CoordinationUnitImpl* child_coordination_unit) override {
+    ++on_child_added_listener_invocations_;
+  }
+  void OnParentAddedEvent(
+      const CoordinationUnitImpl* coordination_unit,
+      const CoordinationUnitImpl* parent_coordination_unit) override {
+    ++on_parent_added_listener_invocations_;
+  }
+  void OnPropertyChangedEvent(const CoordinationUnitImpl* coordination_unit,
+                              mojom::PropertyType property) override {
+    ++on_property_changed_listener_invocations_;
+  }
+  void OnChildRemovedEvent(
+      const CoordinationUnitImpl* coordination_unit,
+      const CoordinationUnitImpl* former_child_coordination_unit) override {
+    ++on_child_removed_listener_invocations_;
+  }
+  void OnParentRemovedEvent(
+      const CoordinationUnitImpl* coordination_unit,
+      const CoordinationUnitImpl* former_parent_coordination_unit) override {
+    ++on_parent_removed_listener_invocations_;
+  }
+  void OnWillBeDestroyedEvent(
+      const CoordinationUnitImpl* coordination_unit) override {
+    ++on_will_be_destroyed_listener_invocations_;
+  }
+
+ protected:
+  explicit TestBasicAPICoordinationUnitGraphObserver(
+      CoordinationUnitType filter)
+      : CoordinationUnitGraphObserver(filter),
+        on_child_added_listener_invocations_(0u),
+        on_parent_added_listener_invocations_(0u),
+        on_coordination_unit_created_invocations_(0u),
+        on_property_changed_listener_invocations_(0u),
+        on_child_removed_listener_invocations_(0u),
+        on_parent_removed_listener_invocations_(0u),
+        on_will_be_destroyed_listener_invocations_(0u) {}
+
+  size_t on_child_added_listener_invocations_;
+  size_t on_parent_added_listener_invocations_;
+  size_t on_coordination_unit_created_invocations_;
+  size_t on_property_changed_listener_invocations_;
+  size_t on_child_removed_listener_invocations_;
+  size_t on_parent_removed_listener_invocations_;
+  size_t on_will_be_destroyed_listener_invocations_;
+};
+
+class TestFilterAPICoordinationUnitGraphObserver
+    : public TestBasicAPICoordinationUnitGraphObserver {
+ public:
+  explicit TestFilterAPICoordinationUnitGraphObserver(
+      CoordinationUnitType filter)
+      : TestBasicAPICoordinationUnitGraphObserver(filter) {}
+
+  void OnCoordinationUnitCreatedEvent(
+      CoordinationUnitImpl* coordination_unit) override {
+    ++on_coordination_unit_created_invocations_;
+
+    coordination_unit->ObserveOnChildAddedEvent(this,
+                                                CoordinationUnitType::kFrame);
+    coordination_unit->ObserveOnParentAddedEvent(
+        this, CoordinationUnitType::kProcess);
+    // TODO(matthalp) Use property mojom::PropertyType enum once it is created.
+    // Currently the only enum is mojom::PropertyType::kTest which is mean to
+    // be filtered out in the test this observer class is used in.
+    coordination_unit->ObserveOnPropertyChangedEvent(
+        this, static_cast<mojom::PropertyType>(1));
+    coordination_unit->ObserveOnChildRemovedEvent(
+        this, CoordinationUnitType::kNavigation);
+    coordination_unit->ObserveOnParentRemovedEvent(
+        this, CoordinationUnitType::kWebContents);
+    coordination_unit->ObserveOnWillBeDestroyedEvent(this);
+  }
+};
+
+}  // namespace
+
+TEST_F(CoordinationUnitGraphObserverTest, CallbacksInvokedNoFilters) {
+  TestBasicAPICoordinationUnitGraphObserver* observer =
+      new TestBasicAPICoordinationUnitGraphObserver();
+
+  // The observer will be deleted after this test executes when the
+  // CoordinationUnitManager object goes out of scope and destructs.
+  coordination_unit_manager()->RegisterObserver(
+      std::unique_ptr<TestBasicAPICoordinationUnitGraphObserver>(observer));

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

This is for line 150-156.

In general using raw pointers are discouraged. I see you need this to check the
number of invocations later. The usual way is directly creating the smart
pointer and transfer ownership to manager. Later you can have a
CUManager::GetObservers() function to get back the observers and test them.

For example:
coordination_unit_manager()->RegisterObserver(base::MakeUnique<...>());
...
auto observers = coordination_unit_manager()->GetObservers();
EXPECT_EQ(2u, observers[0]->on_xyz());

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

Done.

+
+  // The CoordinationUnit types are intentionally different to make
+  // sure filtering, which is not disabled, does not occur.
+  CoordinationUnitID parent_cu_id(CoordinationUnitType::kWebContents,
+                                  std::string());
+  CoordinationUnitID child_cu_id(CoordinationUnitType::kFrame, std::string());
+
+  std::unique_ptr<CoordinationUnitImpl> parent_coordination_unit =
+      coordination_unit_factory::CreateCoordinationUnit(
+          parent_cu_id, service_context_ref_factory()->CreateRef());
+  std::unique_ptr<CoordinationUnitImpl> child_coordination_unit =
+      coordination_unit_factory::CreateCoordinationUnit(
+          child_cu_id, service_context_ref_factory()->CreateRef());
+
+  coordination_unit_manager()->NotifyObserversCoordinationUnitCreated(
+      parent_coordination_unit.get());
+  coordination_unit_manager()->NotifyObserversCoordinationUnitCreated(
+      child_coordination_unit.get());
+
+  parent_coordination_unit->AddChild(child_coordination_unit->id());
+  parent_coordination_unit->RemoveChild(child_coordination_unit->id());
+
+  parent_coordination_unit->SetProperty(mojom::PropertyType::kTest,
+                                        base::Value(42));
+
+  child_coordination_unit->WillBeDestroyed();
+  parent_coordination_unit->WillBeDestroyed();
+
+  EXPECT_EQ(1u, observer->on_child_added_listener_invocations());
+  EXPECT_EQ(1u, observer->on_child_removed_listener_invocations());
+  EXPECT_EQ(2u, observer->on_coordination_unit_created_invocations());

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

nit: it probably helps readability to do this check first.

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

Good suggestion. I moved the EXPECT_EQ directly after the code that should
trigger the change being checked as well.

+  EXPECT_EQ(1u, observer->on_parent_added_listener_invocations());
+  EXPECT_EQ(1u, observer->on_parent_removed_listener_invocations());
+  EXPECT_EQ(1u, observer->on_property_changed_listener_invocations());
+  EXPECT_EQ(2u, observer->on_will_be_destroyed_listener_invocations());
+}
+
+TEST_F(CoordinationUnitGraphObserverTest, CallbacksInvokedWithoutFilters) {

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

Do you mean "With" filters here? And "WithoutFilters" for the above test?

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

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

+  TestFilterAPICoordinationUnitGraphObserver* observer =

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

ditto. Do not use raw pointers here.

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

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

+      new TestFilterAPICoordinationUnitGraphObserver(
+          CoordinationUnitType::kFrame);
+  coordination_unit_manager()->RegisterObserver(
+      std::unique_ptr<TestFilterAPICoordinationUnitGraphObserver>(observer));
+
+  // The CoordinationUnit types are intentionally different to test
+  // that filtering is working correctly.
+  CoordinationUnitID process_cu_id(CoordinationUnitType::kProcess,
+                                   std::string());
+  CoordinationUnitID tab_cu_id(CoordinationUnitType::kWebContents,
+                               std::string());
+  CoordinationUnitID root_frame_cu_id(CoordinationUnitType::kFrame,
+                                      std::string());
+  CoordinationUnitID frame_cu_id(CoordinationUnitType::kFrame, std::string());
+  CoordinationUnitID navigation_cu_id(CoordinationUnitType::kNavigation,
+                                      std::string());
+
+  std::unique_ptr<CoordinationUnitImpl> process_coordination_unit =
+      coordination_unit_factory::CreateCoordinationUnit(
+          process_cu_id, service_context_ref_factory()->CreateRef());
+  std::unique_ptr<CoordinationUnitImpl> tab_coordination_unit =
+      coordination_unit_factory::CreateCoordinationUnit(
+          tab_cu_id, service_context_ref_factory()->CreateRef());
+  std::unique_ptr<CoordinationUnitImpl> root_frame_coordination_unit =
+      coordination_unit_factory::CreateCoordinationUnit(
+          root_frame_cu_id, service_context_ref_factory()->CreateRef());
+  std::unique_ptr<CoordinationUnitImpl> frame_coordination_unit =
+      coordination_unit_factory::CreateCoordinationUnit(
+          frame_cu_id, service_context_ref_factory()->CreateRef());
+  std::unique_ptr<CoordinationUnitImpl> navigation_coordination_unit =
+      coordination_unit_factory::CreateCoordinationUnit(
+          navigation_cu_id, service_context_ref_factory()->CreateRef());
+
+  // Should only invoke the OnCoordinationUnitCreated listener for
+  // the root_frame and frame CoordinationUnits because the observer
+  // filter is set to CoordinationUnitType::kFrame.
+  coordination_unit_manager()->NotifyObserversCoordinationUnitCreated(
+      process_coordination_unit.get());
+  coordination_unit_manager()->NotifyObserversCoordinationUnitCreated(
+      tab_coordination_unit.get());
+  coordination_unit_manager()->NotifyObserversCoordinationUnitCreated(
+      root_frame_coordination_unit.get());
+  coordination_unit_manager()->NotifyObserversCoordinationUnitCreated(
+      frame_coordination_unit.get());
+  coordination_unit_manager()->NotifyObserversCoordinationUnitCreated(
+      navigation_coordination_unit.get());
+
+  // Test AddChild filtering. The AddChild filter for the frame
+  // CoordinationUnit has been set to only execute when the child
+  // is a CoordinationUnitType::kFrame so the callback will execute.
+  root_frame_coordination_unit->AddChild(frame_coordination_unit->id());
+  // The child is CoordinationUnitType::kNavigation so the callback
+  // should not be executed. The navigation Coordination Unit is not
+  // actually a child of a frame in practice, but is being used here to
+  // check filtering on OnChildAdded.
+  root_frame_coordination_unit->AddChild(navigation_coordination_unit->id());
+
+  // Test AddParent filtering. The AddParent filter for the frame
+  // CoordinationUnit has been set to only execute when the parent
+  // is a CoordinationUnitType::kProcess so the callback will execute.
+  // Note that AddParent is called within Add child.

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

s/within Add child/as a consequence of AddChild.

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

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

+  process_coordination_unit->AddChild(root_frame_coordination_unit->id());
+  // The parent is CoordinationUnitType::kFrame so the callback
+  // should not be executed.
+  tab_coordination_unit->AddChild(root_frame_coordination_unit->id());
+
+  // Test RemoveChild filtering. The RemoveChild filter for the frame
+  // CoordinationUnit has been set to only execute when the child
+  // is a CoordinationUnitType::kFrame so the callback will execute.
+  root_frame_coordination_unit->RemoveChild(frame_coordination_unit->id());
+  // The child is CoordinationUnitType::kNavigation so the callback
+  // should not be executed. The navigation Coordination Unit is not
+  // actually a child of a frame in practice, but is being used here to
+  // check filtering on OnRemoveChild.
+  root_frame_coordination_unit->RemoveChild(navigation_coordination_unit->id());
+
+  // Test RemoveParent filtering. The RemoveParent filter for the frame
+  // CoordinationUnit has been set to only execute when the parent
+  // is a CoordinationUnitType::kProcess so the callback will execute.
+  // Note that RemoveParent is called within Remove child.
+  process_coordination_unit->RemoveChild(root_frame_coordination_unit->id());
+  // The parent is CoordinationUnitType::kFrame so the callback
+  // should not be executed.
+  tab_coordination_unit->RemoveChild(root_frame_coordination_unit->id());
+
+  // TODO(matthalp) Implement this another SetProperty once an additional
+  // mojom::PropertyType has been implemented so that the SetProperty
+  // filtering API can be tested.
+  root_frame_coordination_unit->SetProperty(mojom::PropertyType::kTest,
+                                            base::Value(42));
+
+  frame_coordination_unit->WillBeDestroyed();
+  navigation_coordination_unit->WillBeDestroyed();
+  process_coordination_unit->WillBeDestroyed();
+  root_frame_coordination_unit->WillBeDestroyed();
+  tab_coordination_unit->WillBeDestroyed();
+
+  EXPECT_EQ(1u, observer->on_child_added_listener_invocations());
+  EXPECT_EQ(1u, observer->on_child_removed_listener_invocations());
+  EXPECT_EQ(2u, observer->on_coordination_unit_created_invocations());

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

nit: it probably helps readability to do this check first.

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

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

+  EXPECT_EQ(1u, observer->on_parent_added_listener_invocations());
+  EXPECT_EQ(1u, observer->on_parent_removed_listener_invocations());
+  EXPECT_EQ(0u, observer->on_property_changed_listener_invocations());
+  EXPECT_EQ(2u, observer->on_will_be_destroyed_listener_invocations());
+}
+
+}  // namespace resource_coordinator