memory-infra: Finish moving memory_infra from TracingController

base/trace_event/memory_dump_manager_unittest.cc

Index: base/trace_event/memory_dump_manager_unittest.cc
diff --git a/base/trace_event/memory_dump_manager_unittest.cc b/base/trace_event/memory_dump_manager_unittest.cc
index e8c33af3e06b935a80911bf049ff3d6e8c3c4b01..e3d43fdc60dc8ce4e217dd70f94b6d515c96401d 100644
--- a/base/trace_event/memory_dump_manager_unittest.cc
+++ b/base/trace_event/memory_dump_manager_unittest.cc
@@ -118,7 +118,8 @@ void PostTaskAndWait(const tracked_objects::Location& from_here,
 // requests locally to the MemoryDumpManager instead of performing IPC dances.
 class MemoryDumpManagerDelegateForTesting : public MemoryDumpManagerDelegate {
  public:
-  MemoryDumpManagerDelegateForTesting() {
+  MemoryDumpManagerDelegateForTesting(bool is_coordinator)
+      : is_coordinator_(is_coordinator) {
     ON_CALL(*this, RequestGlobalMemoryDump(_, _))
         .WillByDefault(Invoke(
             this, &MemoryDumpManagerDelegateForTesting::CreateProcessDump));
@@ -128,13 +129,16 @@ class MemoryDumpManagerDelegateForTesting : public MemoryDumpManagerDelegate {
                void(const MemoryDumpRequestArgs& args,
                     const MemoryDumpCallback& callback));
 
-  uint64_t GetTracingProcessId() const override {
-    NOTREACHED();
-    return MemoryDumpManager::kInvalidTracingProcessId;
+  bool IsCoordinator() const override { return is_coordinator_; }
+  void SetIsCoordinator(bool is_coordinator) {
+    is_coordinator_ = is_coordinator;
   }
 
   // Promote the CreateProcessDump to public so it can be used by test fixtures.
   using MemoryDumpManagerDelegate::CreateProcessDump;
+
+ private:
+  bool is_coordinator_;
 };
 
 class MockMemoryDumpProvider : public MemoryDumpProvider {
@@ -219,13 +223,15 @@ class MemoryDumpManagerTest : public testing::Test {
     mdm_.reset(new MemoryDumpManager());
     MemoryDumpManager::SetInstanceForTesting(mdm_.get());
     ASSERT_EQ(mdm_.get(), MemoryDumpManager::GetInstance());
-    delegate_.reset(new MemoryDumpManagerDelegateForTesting);
+    delegate_for_mdm_.reset(new MemoryDumpManagerDelegateForTesting(false));
+    delegate_ = delegate_for_mdm_.get();

[oystein (OOO til 10th of July), 2017/02/16 20:50:00]

This is confusing to me, why are you storing the raw ptr of the delegate as well
as the unique_ptr?

[chiniforooshan, 2017/02/16 22:54:15]

It is because MDM assumes that the ownership of the delegate will be transferred
to it, which is OK because in the non-test scenarios only MDM uses the delegate
and it's OK to create a delegate and just pass it to MDM and forget about it.
But, in unittests we need the delegate outside MDM to verify its methods are
called correctly (i.e. EXPECT_CALLs) before and after MDM initialization.

I cannot just store a unique_ptr because after MDM initialization I'll loose
access to it. And, if I just store a raw pointer I have to take care of deleting
it if a a test case decides to not transfer ownership to MDM (i.e. doesn't call
mdm->Initialize). I know it's confusing, but is there a better way?

[oystein (OOO til 10th of July), 2017/02/16 23:19:49]

Can the MDM have a get_delegate_for_testing() which returns the raw ptr?

[chiniforooshan, 2017/02/17 17:20:42]

How? That function would return a MemoryDumpManagerDelegate and we need the
MemoryDumpManagerDelegateForTesting subclass in tests and I "cannot use
dynamic_cast with -fno-rtti".

   }
 
   void TearDown() override {
     MemoryDumpManager::SetInstanceForTesting(nullptr);
+    delegate_for_mdm_.reset();
+    delegate_ = nullptr;
     mdm_.reset();
-    delegate_.reset();
     message_loop_.reset();
     TraceLog::DeleteForTesting();
   }
@@ -247,7 +253,8 @@ class MemoryDumpManagerTest : public testing::Test {
  protected:
   void InitializeMemoryDumpManager(bool is_coordinator) {
     mdm_->set_dumper_registrations_ignored_for_testing(true);
-    mdm_->Initialize(delegate_.get(), is_coordinator);
+    delegate_for_mdm_->SetIsCoordinator(is_coordinator);

[oystein (OOO til 10th of July), 2017/02/16 20:50:00]

Can you just create the MemoryDumpManagerDelegateForTesting here, instead of
creating it in the destructor with is_coordinator = false and thus needing an
API for changing it?

[chiniforooshan, 2017/02/16 22:54:15]

No, because of the test case "InitializedAfterStartOfTracing". It does not call
InitializeMemoryDumpManager but uses the delegate:

EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(_, _)).Times(0);

[oystein (OOO til 10th of July), 2017/02/16 23:19:49]

Okay, though that line seems to be testing the test itself :). i.e. the
delegate_ hasn't gotten passed anywhere at this point, so it's effectively
testing that InitializeMemoryDumpManager() didn't get called by the test.

[chiniforooshan, 2017/02/17 17:20:42]

Agreed. Let me delete that EXPECT_CALL unless Primiano objects.

+    mdm_->Initialize(std::move(delegate_for_mdm_));
   }
 
   void RequestGlobalDumpAndWait(MemoryDumpType dump_type,
@@ -286,7 +293,10 @@ class MemoryDumpManagerTest : public testing::Test {
 
   const MemoryDumpProvider::Options kDefaultOptions;
   std::unique_ptr<MemoryDumpManager> mdm_;
-  std::unique_ptr<MemoryDumpManagerDelegateForTesting> delegate_;
+  // The delegate that is going to be passed to MDM.
+  std::unique_ptr<MemoryDumpManagerDelegateForTesting> delegate_for_mdm_;
+  // A pointer to the delegate that is owned by MDM, after MD initialization.
+  MemoryDumpManagerDelegateForTesting* delegate_;
   bool last_callback_success_;
 
  private: