[GRC] Coordination Unit Graph Observer V3

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
index 7ad44f721747b13865f1859b7ebffe7e038bc11c..1183cfcd2396015c912f09edc67065c71983f4e1 100644
--- a/services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc
+++ b/services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc
@@ -25,24 +25,16 @@ namespace {
 class CoordinationUnitGraphObserverTest : public CoordinationUnitImplTestBase {
 };
 
-class TestBasicAPICoordinationUnitGraphObserver
-    : public CoordinationUnitGraphObserver {
+class TestCoordinationUnitGraphObserver : 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);
-  }
+  TestCoordinationUnitGraphObserver()
+      : 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() {
     return on_child_added_listener_invocations_;
@@ -66,47 +58,41 @@ class TestBasicAPICoordinationUnitGraphObserver
     return on_will_be_destroyed_listener_invocations_;
   }
 
-  void OnChildAddedEvent(
+  // Overridden from CoordinationUnitGraphObserver.
+  void OnCoordinationUnitCreated(
+      const CoordinationUnitImpl* coordination_unit) override {
+    ++on_coordination_unit_created_invocations_;
+  }
+  void OnChildAdded(
       const CoordinationUnitImpl* coordination_unit,
       const CoordinationUnitImpl* child_coordination_unit) override {
     ++on_child_added_listener_invocations_;
   }
-  void OnParentAddedEvent(
+  void OnParentAdded(
       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 {
+  void OnPropertyChanged(const CoordinationUnitImpl* coordination_unit,
+                         mojom::PropertyType property) override {
     ++on_property_changed_listener_invocations_;
   }
-  void OnChildRemovedEvent(
+  void OnChildRemoved(
       const CoordinationUnitImpl* coordination_unit,
       const CoordinationUnitImpl* former_child_coordination_unit) override {
     ++on_child_removed_listener_invocations_;
   }
-  void OnParentRemovedEvent(
+  void OnParentRemoved(
       const CoordinationUnitImpl* coordination_unit,
       const CoordinationUnitImpl* former_parent_coordination_unit) override {
     ++on_parent_removed_listener_invocations_;
   }
-  void OnWillBeDestroyedEvent(
+  void OnCoordinationUnitWillBeDestroyed(
       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) {}
-
+ private:
   size_t on_child_added_listener_invocations_;

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

IMHO, Let's use
child_added_count_/parent_added_count_/coodination_unit_created_count_/property_changed_count_/child_removed_count_/parent_removed_count/will_be_destroyed_count_

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

Done.

   size_t on_parent_added_listener_invocations_;
   size_t on_coordination_unit_created_invocations_;
@@ -116,49 +102,19 @@ class TestBasicAPICoordinationUnitGraphObserver
   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();
+  EXPECT_EQ(0u, coordination_unit_manager().observers().size());

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

nit: EXPECT_TRUE(.empty());

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

Agreed -- Done. Always better to use .empty() than size() == 0.

+  coordination_unit_manager().RegisterObserver(
+      base::MakeUnique<TestCoordinationUnitGraphObserver>());
 
-  // 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));
+  EXPECT_EQ(1u, coordination_unit_manager().observers().size());
+  TestCoordinationUnitGraphObserver* observer =
+      static_cast<TestCoordinationUnitGraphObserver*>(
+          coordination_unit_manager().observers()[0].get());
 
-  // 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 parent_cu_id(CoordinationUnitType::kFrame, std::string());
   CoordinationUnitID child_cu_id(CoordinationUnitType::kFrame, std::string());
 
   std::unique_ptr<CoordinationUnitImpl> parent_coordination_unit =
@@ -168,134 +124,28 @@ TEST_F(CoordinationUnitGraphObserverTest, CallbacksInvokedNoFilters) {
       coordination_unit_factory::CreateCoordinationUnit(
           child_cu_id, service_context_ref_factory()->CreateRef());
 
-  coordination_unit_manager()->NotifyObserversCoordinationUnitCreated(
+  coordination_unit_manager().OnCoordinationUnitCreated(
       parent_coordination_unit.get());
-  coordination_unit_manager()->NotifyObserversCoordinationUnitCreated(
+  coordination_unit_manager().OnCoordinationUnitCreated(
       child_coordination_unit.get());
+  EXPECT_EQ(2u, observer->on_coordination_unit_created_invocations());
 
   parent_coordination_unit->AddChild(child_coordination_unit->id());
+  EXPECT_EQ(1u, observer->on_child_added_listener_invocations());
+  EXPECT_EQ(1u, observer->on_parent_added_listener_invocations());
+
   parent_coordination_unit->RemoveChild(child_coordination_unit->id());
+  EXPECT_EQ(1u, observer->on_child_removed_listener_invocations());
+  EXPECT_EQ(1u, observer->on_parent_removed_listener_invocations());
 
   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());
-  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) {
-  TestFilterAPICoordinationUnitGraphObserver* observer =
-      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.
-  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());
-  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());
+  coordination_unit_manager().OnCoordinationUnitWillBeDestroyed(
+      child_coordination_unit.get());
+  coordination_unit_manager().OnCoordinationUnitWillBeDestroyed(
+      parent_coordination_unit.get());
   EXPECT_EQ(2u, observer->on_will_be_destroyed_listener_invocations());
 }