[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..968b0e38df28b03485a1b625257a5e3a42520e69
--- /dev/null
+++ b/services/resource_coordinator/coordination_unit/coordination_unit_graph_observer_unittest.cc
@@ -0,0 +1,315 @@
+// 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 "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:
+  explicit TestBasicAPICoordinationUnitGraphObserver(
+      CoordinationUnitType filter)

[Zhen Wang, 2017/06/15 20:07:52]

If this does not test filter, this class should not have constructor taking in
filter.

If this is only used for subclass purpose, put it in protected.

[matthalp, 2017/06/16 00:01:27]

Done. CoordinationUnitGraphObserver::filter_ is now a protected member.

+      : CoordinationUnitGraphObserver(filter),
+        add_child_listener_invocations_(0u),
+        add_parent_listener_invocations_(0u),
+        coordination_unit_created_invocations_(0u),
+        property_changed_listener_invocations_(0u),
+        remove_child_listener_invocations_(0u),
+        remove_parent_listener_invocations_(0u),
+        will_be_destroyed_listener_invocations_(0u) {}
+  TestBasicAPICoordinationUnitGraphObserver()
+      : TestBasicAPICoordinationUnitGraphObserver(
+            CoordinationUnitType::kInvalidType) {}
+
+  void CoordinationUnitCreated(
+      CoordinationUnitImpl* coordination_unit) override {
+    ++coordination_unit_created_invocations_;
+
+    coordination_unit->on_add_child_event_listeners().AddListener(base::Bind(
+        &TestBasicAPICoordinationUnitGraphObserver::AddChildListenerInvoked,
+        base::Unretained(this)));
+    coordination_unit->on_add_parent_event_listeners().AddListener(base::Bind(
+        &TestBasicAPICoordinationUnitGraphObserver::AddParentListenerInvoked,
+        base::Unretained(this)));
+    coordination_unit->on_property_changed_event_listeners().AddListener(
+        base::Bind(&TestBasicAPICoordinationUnitGraphObserver::
+                       PropertyChangedListenerInvoked,
+                   base::Unretained(this)));
+    coordination_unit->on_remove_child_event_listeners().AddListener(base::Bind(
+        &TestBasicAPICoordinationUnitGraphObserver::RemoveChildListenerInvoked,
+        base::Unretained(this)));
+    coordination_unit->on_remove_parent_event_listeners().AddListener(
+        base::Bind(&TestBasicAPICoordinationUnitGraphObserver::
+                       RemoveParentListenerInvoked,
+                   base::Unretained(this)));
+    coordination_unit->on_will_be_destroyed_event_listeners().AddListener(
+        base::Bind(&TestBasicAPICoordinationUnitGraphObserver::
+                       WillBeDestroyedListenerInvoked,
+                   base::Unretained(this)));
+  }
+  void AddChildListenerInvoked(

[Zhen Wang, 2017/06/15 20:07:53]

Should these callbacks be put into protected section?

And they need to be renamed to On*() style.

[matthalp, 2017/06/16 00:01:27]

I attempted to make the callbacks protected, but the compiler did not like it
(anonymous namespace issues).

I will also rename the callbacks to use the On*() style.

+      const CoordinationUnitImpl* coordination_unit,
+      const CoordinationUnitImpl* child_coordination_unit) {
+    ++add_child_listener_invocations_;
+  }
+  void AddParentListenerInvoked(
+      const CoordinationUnitImpl* coordination_unit,
+      const CoordinationUnitImpl* parent_coordination_unit) {
+    ++add_parent_listener_invocations_;
+  }
+  void PropertyChangedListenerInvoked(
+      const CoordinationUnitImpl* coordination_unit,
+      mojom::PropertyType property) {
+    ++property_changed_listener_invocations_;
+  }
+  void RemoveChildListenerInvoked(
+      const CoordinationUnitImpl* coordination_unit,
+      const CoordinationUnitImpl* former_child_coordination_unit) {
+    ++remove_child_listener_invocations_;
+  }
+  void RemoveParentListenerInvoked(
+      const CoordinationUnitImpl* coordination_unit,
+      const CoordinationUnitImpl* former_parent_coordination_unit) {
+    ++remove_parent_listener_invocations_;
+  }
+  void WillBeDestroyedListenerInvoked(
+      const CoordinationUnitImpl* coordination_unit) {
+    ++will_be_destroyed_listener_invocations_;
+  }
+
+  unsigned int add_child_listener_invocations() {
+    return add_child_listener_invocations_;
+  }
+  unsigned int add_parent_listener_invocations() {
+    return add_parent_listener_invocations_;
+  }
+  unsigned int coordination_unit_created_invocations() {
+    return coordination_unit_created_invocations_;
+  }
+  unsigned int property_changed_listener_invocations() {
+    return property_changed_listener_invocations_;
+  }
+  unsigned int remove_child_listener_invocations() {
+    return remove_child_listener_invocations_;
+  }
+  unsigned int remove_parent_listener_invocations() {
+    return remove_parent_listener_invocations_;
+  }
+  unsigned int will_be_destroyed_listener_invocations() {
+    return will_be_destroyed_listener_invocations_;
+  }
+
+ protected:
+  unsigned int add_child_listener_invocations_;

[Zhen Wang, 2017/06/15 20:07:53]

Use size_t for these:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[matthalp, 2017/06/16 00:01:27]

Done.

+  unsigned int add_parent_listener_invocations_;
+  unsigned int coordination_unit_created_invocations_;
+  unsigned int property_changed_listener_invocations_;
+  unsigned int remove_child_listener_invocations_;
+  unsigned int remove_parent_listener_invocations_;
+  unsigned int will_be_destroyed_listener_invocations_;
+};
+
+class TestFilterAPICoordinationUnitGraphObserver
+    : public TestBasicAPICoordinationUnitGraphObserver {
+ public:
+  explicit TestFilterAPICoordinationUnitGraphObserver(
+      CoordinationUnitType filter)
+      : TestBasicAPICoordinationUnitGraphObserver(filter) {}
+
+  void CoordinationUnitCreated(
+      CoordinationUnitImpl* coordination_unit) override {
+    ++coordination_unit_created_invocations_;
+
+    coordination_unit->on_add_child_event_listeners().AddListener(
+        base::Bind(
+            &TestBasicAPICoordinationUnitGraphObserver::AddChildListenerInvoked,
+            base::Unretained(this)),
+        CoordinationUnitType::kFrame);
+    coordination_unit->on_add_parent_event_listeners().AddListener(
+        base::Bind(&TestBasicAPICoordinationUnitGraphObserver::
+                       AddParentListenerInvoked,
+                   base::Unretained(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->on_property_changed_event_listeners().AddListener(
+        base::Bind(&TestBasicAPICoordinationUnitGraphObserver::
+                       PropertyChangedListenerInvoked,
+                   base::Unretained(this)),
+        static_cast<mojom::PropertyType>(1));
+    coordination_unit->on_remove_child_event_listeners().AddListener(
+        base::Bind(&TestBasicAPICoordinationUnitGraphObserver::
+                       RemoveChildListenerInvoked,
+                   base::Unretained(this)),
+        CoordinationUnitType::kNavigation);
+    coordination_unit->on_remove_parent_event_listeners().AddListener(
+        base::Bind(&TestBasicAPICoordinationUnitGraphObserver::
+                       RemoveParentListenerInvoked,
+                   base::Unretained(this)),
+        CoordinationUnitType::kWebContents);
+    coordination_unit->on_will_be_destroyed_event_listeners().AddListener(
+        base::Bind(&TestBasicAPICoordinationUnitGraphObserver::
+                       WillBeDestroyedListenerInvoked,
+                   base::Unretained(this)));
+  }
+};
+
+}  // namespace
+
+TEST_F(CoordinationUnitGraphObserverTest, CallbacksInvokedNoFilters) {

[Zhen Wang, 2017/06/15 20:07:53]

nit: s/CallbacksInvokedNoFilters/CallbacksInvokedWithoutFilters

[matthalp, 2017/06/16 00:01:27]

Done.

+  // The observer will be deleted after this test executes when the
+  // CoordinationUnitManager object goes out of scope and destructs.
+  TestBasicAPICoordinationUnitGraphObserver* observer =

[Zhen Wang, 2017/06/15 20:07:53]

Use smart pointers and pass it to manager with std::move. With that, you do not
even need the comment above because people will understand manager "owns"
observers and will do the corresponding cleanup.
https://www.chromium.org/developers/smart-pointer-guidelines

[matthalp, 2017/06/16 00:01:27]

Done.

+      new TestBasicAPICoordinationUnitGraphObserver();
+  coordination_unit_manager()->RegisterObserver(observer);
+
+  // 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->add_child_listener_invocations());
+  EXPECT_EQ(1u, observer->add_parent_listener_invocations());
+  EXPECT_EQ(2u, observer->coordination_unit_created_invocations());
+  EXPECT_EQ(1u, observer->property_changed_listener_invocations());
+  EXPECT_EQ(1u, observer->remove_child_listener_invocations());
+  EXPECT_EQ(1u, observer->remove_parent_listener_invocations());
+  EXPECT_EQ(2u, observer->will_be_destroyed_listener_invocations());
+}
+
+TEST_F(CoordinationUnitGraphObserverTest, CallbacksInvokedWithFilters) {
+  TestFilterAPICoordinationUnitGraphObserver* observer =

[Zhen Wang, 2017/06/15 20:07:52]

ditto: use smart pointer.

[matthalp, 2017/06/16 00:01:27]

Done.

+      new TestFilterAPICoordinationUnitGraphObserver(
+          CoordinationUnitType::kFrame);
+  coordination_unit_manager()->RegisterObserver(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 not invoke the CoordinationUnitCreated listener

[Zhen Wang, 2017/06/15 20:07:52]

Combine the comments here to one: Shoudl invoke the CoordinationUnitCreated
listener for frame CU only. 

[matthalp, 2017/06/16 00:01:27]

Done.

+  coordination_unit_manager()->NotifyObserversCoordinationUnitCreated(
+      process_coordination_unit.get());
+  // Should not invoke the CoordinationUnitCreated listener
+  coordination_unit_manager()->NotifyObserversCoordinationUnitCreated(
+      tab_coordination_unit.get());
+  // Should invoke the CoordinationUnitCreated listener
+  coordination_unit_manager()->NotifyObserversCoordinationUnitCreated(
+      root_frame_coordination_unit.get());
+  // Should not invoke the CoordinationUnitCreated listener
+  coordination_unit_manager()->NotifyObserversCoordinationUnitCreated(
+      frame_coordination_unit.get());

[Zhen Wang, 2017/06/15 20:07:52]

The comment says it does not invoke the listener. Why not?

[matthalp, 2017/06/16 00:01:27]

Done -- added comment at the top of this set of function invocations.

+  // Should not invoke the CoordinationUnitCreated listener
+  coordination_unit_manager()->NotifyObserversCoordinationUnitCreated(
+      navigation_coordination_unit.get());
+
+  // Should invoke the observer's AddChild listener.
+  root_frame_coordination_unit->AddChild(frame_coordination_unit->id());
+  // The navigation Coordination Unit is not actually a child of a frame
+  // in practice, but is being used here to check filtering on AddChild.
+  // Should not invoke the observer's AddChild listener.
+  root_frame_coordination_unit->AddChild(navigation_coordination_unit->id());
+  // Should invoke the observer's AddParent listener.

[Zhen Wang, 2017/06/15 20:07:52]

Why AddParent? Typo?

[matthalp, 2017/06/16 00:01:27]

This is not a typo, but the comment has been improved to make it more clear:

By calling AddChild on the process CU, the AddParent is called on the frame CU,
whose filter is set to only look for CoordinationUnitType::kProcess parents.

+  process_coordination_unit->AddChild(root_frame_coordination_unit->id());
+  // Should not invoke the observer's AddParent listener.

[Zhen Wang, 2017/06/15 20:07:52]

Why AddParent? Typo?

[matthalp, 2017/06/16 00:01:27]

See comment above.

+  tab_coordination_unit->AddChild(root_frame_coordination_unit->id());
+
+  // Should not invoke the observer's RemoveChild listener.
+  root_frame_coordination_unit->RemoveChild(frame_coordination_unit->id());
+  // The navigation Coordination Unit is not actually a child of a frame
+  // in practice, but is being used here to check filtering on AddChild.
+  // Should invoke the observer's RemoveChild listener.
+  root_frame_coordination_unit->RemoveChild(navigation_coordination_unit->id());
+  // Should not invoke the observer's RemoveParent listener.

[Zhen Wang, 2017/06/15 20:07:53]

Why RemoveParent? Typo?

[matthalp, 2017/06/16 00:01:27]

See comment above.

+  process_coordination_unit->RemoveChild(root_frame_coordination_unit->id());
+  // Should invoke the observer's RemoveParent listener.

[Zhen Wang, 2017/06/15 20:07:53]

Why RemoveParent? Typo?

[matthalp, 2017/06/16 00:01:27]

See comment above.

+  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));
+
+  process_coordination_unit->WillBeDestroyed();
+  tab_coordination_unit->WillBeDestroyed();
+  root_frame_coordination_unit->WillBeDestroyed();
+  frame_coordination_unit->WillBeDestroyed();
+  navigation_coordination_unit->WillBeDestroyed();
+
+  EXPECT_EQ(1u, observer->add_child_listener_invocations());
+  EXPECT_EQ(1u, observer->add_parent_listener_invocations());
+  EXPECT_EQ(2u, observer->coordination_unit_created_invocations());
+  EXPECT_EQ(0u, observer->property_changed_listener_invocations());
+  EXPECT_EQ(1u, observer->remove_child_listener_invocations());
+  EXPECT_EQ(1u, observer->remove_parent_listener_invocations());
+  EXPECT_EQ(2u, observer->will_be_destroyed_listener_invocations());
+}
+
+}  // namespace resource_coordinator