[tracing] MemoryDumpManager uses TraceConfig to set the periodic dump timers

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 4d0372fd3243286fca9fbca439a89108970be143..6e00e066e76bc6e161b422ac8cd1c4bd091e2292 100644
--- a/base/trace_event/memory_dump_manager_unittest.cc
+++ b/base/trace_event/memory_dump_manager_unittest.cc
@@ -25,9 +25,33 @@ using testing::Return;
 namespace base {
 namespace trace_event {
 namespace {
-MemoryDumpArgs high_detail_args = {MemoryDumpArgs::LevelOfDetail::HIGH};
-MemoryDumpArgs low_detail_args = {MemoryDumpArgs::LevelOfDetail::LOW};
-}
+MemoryDumpArgs g_high_detail_args = {MemoryDumpArgs::LevelOfDetail::HIGH};
+MemoryDumpArgs g_low_detail_args = {MemoryDumpArgs::LevelOfDetail::LOW};
+
+const char kMemoryDumpTraceConfigString[] =
+  "{"
+    "\"included_categories\":["
+      "\"disabled-by-default-memory-infra\""

[Primiano Tucci (use gerrit), 2015/08/26 10:25:07]

I don't think you want the "disabled-by-default-memory-infra" here right?

+    "],"
+    "\"memory_dump_config\":{"
+      "\"triggers\":["
+        "{"
+          "\"level_of_detail\":\"low\","
+          "\"periodic_interval_ms\":50"
+        "},"
+        "{"
+          "\"level_of_detail\":\"high\","
+          "\"periodic_interval_ms\":100"
+        "},"
+        "{"
+          "\"level_of_detail\":\"high\","
+          "\"periodic_interval_ms\":150"
+        "}"
+      "]"
+    "}"
+  "}";
+
+}  // namespace
 
 // Testing MemoryDumpManagerDelegate which short-circuits dump requests locally
 // instead of performing IPC dances.
@@ -44,6 +68,11 @@ class MemoryDumpManagerDelegateForTesting : public MemoryDumpManagerDelegate {
   }
 };
 
+class CoordinatorMemoryDumpManagerDelegateForTesting

[Primiano Tucci (use gerrit), 2015/08/26 10:25:07]

Can you Just add a static public boolean to MemoryDumpManagerDelegateForTesting?
it's simpler to read.

So you don't need to change anything and just do

MemoryDumpManagerDelegateForTesting::is_coordinator_process = true below

+    : public MemoryDumpManagerDelegateForTesting {
+  bool IsCoordinatorProcess() const override { return true; }
+};
+
 class MemoryDumpManagerTest : public testing::Test {
  public:
   void SetUp() override {
@@ -53,7 +82,6 @@ class MemoryDumpManagerTest : public testing::Test {
     MemoryDumpManager::SetInstanceForTesting(mdm_.get());
     ASSERT_EQ(mdm_, MemoryDumpManager::GetInstance());
     MemoryDumpManager::GetInstance()->Initialize();
-    MemoryDumpManager::GetInstance()->SetDelegate(&delegate_);
   }
 
   void TearDown() override {
@@ -73,10 +101,19 @@ class MemoryDumpManagerTest : public testing::Test {
 
  protected:
   void EnableTracing(const char* category) {

[Primiano Tucci (use gerrit), 2015/08/26 10:25:07]

I think at this point you don't need the char* argument anymore and can make
this just EnableTracing()

+    delegate_.reset(new MemoryDumpManagerDelegateForTesting());
+    MemoryDumpManager::GetInstance()->SetDelegate(delegate_.get());
     TraceLog::GetInstance()->SetEnabled(
         TraceConfig(category, ""), TraceLog::RECORDING_MODE);
   }
 
+  void EnableTracingWithTriggers() {

[Primiano Tucci (use gerrit), 2015/08/26 10:25:07]

Rename this to EnableTracingWithTraceConfig(const char* trace_config)

And I'd remove the delegate_.reset(new
CoordinatorMemoryDumpManagerDelegateForTesting()).
Just in the test fixture below do:
MemoryDumpManagerDelegateForTesting::is_coordinator_process = true;

+    delegate_.reset(new CoordinatorMemoryDumpManagerDelegateForTesting());
+    MemoryDumpManager::GetInstance()->SetDelegate(delegate_.get());
+    TraceConfig tc(kMemoryDumpTraceConfigString);
+    TraceLog::GetInstance()->SetEnabled(tc, TraceLog::RECORDING_MODE);
+  }
+
   void DisableTracing() { TraceLog::GetInstance()->SetDisabled(); }
 
   scoped_ptr<MemoryDumpManager> mdm_;
@@ -84,7 +121,7 @@ class MemoryDumpManagerTest : public testing::Test {
 
  private:
   scoped_ptr<MessageLoop> message_loop_;
-  MemoryDumpManagerDelegateForTesting delegate_;
+  scoped_ptr<MemoryDumpManagerDelegateForTesting> delegate_;
 
   // We want our singleton torn down after each test.
   ShadowingAtExitManager at_exit_manager_;
@@ -93,20 +130,31 @@ class MemoryDumpManagerTest : public testing::Test {
 class MockDumpProvider : public MemoryDumpProvider {
  public:
   MockDumpProvider()
-      : dump_provider_to_register_or_unregister(nullptr),
-        last_session_state_(nullptr),
-        level_of_detail_(MemoryDumpArgs::LevelOfDetail::HIGH) {}
+      : MockDumpProvider(nullptr, MemoryDumpArgs::LevelOfDetail::HIGH, 0) {}
 
   // Ctor used by the RespectTaskRunnerAffinity test.
   explicit MockDumpProvider(
       const scoped_refptr<SingleThreadTaskRunner>& task_runner)
-      : last_session_state_(nullptr),
-        task_runner_(task_runner),
-        level_of_detail_(MemoryDumpArgs::LevelOfDetail::HIGH) {}
+      : MockDumpProvider(task_runner, MemoryDumpArgs::LevelOfDetail::HIGH, 0) {}
 
   // Ctor used by CheckMemoryDumpArgs test.
   explicit MockDumpProvider(const MemoryDumpArgs::LevelOfDetail level_of_detail)
-      : last_session_state_(nullptr), level_of_detail_(level_of_detail) {}
+      : MockDumpProvider(nullptr, level_of_detail, 0) {}
+
+  // Ctor used by TraceConfigTriggers test.
+  explicit MockDumpProvider(int dump_calls_to_disable_tracing)
+      : MockDumpProvider(nullptr,
+                         MemoryDumpArgs::LevelOfDetail::HIGH,
+                         dump_calls_to_disable_tracing) {}
+
+  MockDumpProvider(const scoped_refptr<SingleThreadTaskRunner>& task_runner,
+                   const MemoryDumpArgs::LevelOfDetail level_of_detail,
+                   int dump_calls_to_disable_tracing)
+      : dump_provider_to_register_or_unregister(nullptr),
+        last_session_state_(nullptr),
+        level_of_detail_(level_of_detail),
+        on_memory_dump_calls_(0),
+        dump_calls_to_disable_tracing_(dump_calls_to_disable_tracing) {}
 
   virtual ~MockDumpProvider() {}
 
@@ -153,6 +201,22 @@ class MockDumpProvider : public MemoryDumpProvider {
     return true;
   }
 
+  // OnMemoryDump() override for the TraceConfigTriggers test.
+  bool OnMemoryDump_TraceConfigTriggers(const MemoryDumpArgs& args,
+                                        ProcessMemoryDump* pmd) {
+    if (on_memory_dump_calls_ % 2 == 0 || on_memory_dump_calls_ % 3 == 0)

[Primiano Tucci (use gerrit), 2015/08/26 10:25:06]

I don't think you need all this logic, just use the gtest expectations below.

+      EXPECT_EQ(args.level_of_detail, MemoryDumpArgs::LevelOfDetail::HIGH);
+    else
+      EXPECT_EQ(args.level_of_detail, MemoryDumpArgs::LevelOfDetail::LOW);
+
+    on_memory_dump_calls_++;
+    if (on_memory_dump_calls_ == dump_calls_to_disable_tracing_) {
+      TraceLog::GetInstance()->SetDisabled();
+      MemoryDumpManager::GetInstance()->UnregisterDumpProvider(this);
+    }
+    return true;
+  }
+
   // Used by OnMemoryDump_(Un)RegisterExtraDumpProvider.
   MemoryDumpProvider* dump_provider_to_register_or_unregister;
 
@@ -160,6 +224,8 @@ class MockDumpProvider : public MemoryDumpProvider {
   MemoryDumpSessionState* last_session_state_;
   scoped_refptr<SingleThreadTaskRunner> task_runner_;
   const MemoryDumpArgs::LevelOfDetail level_of_detail_;
+  int on_memory_dump_calls_;
+  int dump_calls_to_disable_tracing_;
 };
 
 TEST_F(MemoryDumpManagerTest, SingleDumper) {
@@ -170,7 +236,7 @@ TEST_F(MemoryDumpManagerTest, SingleDumper) {
   EnableTracing("foo-and-bar-but-not-memory");
   EXPECT_CALL(mdp, OnMemoryDump(_, _)).Times(0);
   mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                          high_detail_args);
+                          g_high_detail_args);
   DisableTracing();
 
   // Now repeat enabling the memory category and check that the dumper is
@@ -179,7 +245,7 @@ TEST_F(MemoryDumpManagerTest, SingleDumper) {
   EXPECT_CALL(mdp, OnMemoryDump(_, _)).Times(3).WillRepeatedly(Return(true));
   for (int i = 0; i < 3; ++i)
     mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                            high_detail_args);
+                            g_high_detail_args);
   DisableTracing();
 
   mdm_->UnregisterDumpProvider(&mdp);
@@ -188,7 +254,7 @@ TEST_F(MemoryDumpManagerTest, SingleDumper) {
   EnableTracing(MemoryDumpManager::kTraceCategory);
   EXPECT_CALL(mdp, OnMemoryDump(_, _)).Times(0);
   mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                          high_detail_args);
+                          g_high_detail_args);
   TraceLog::GetInstance()->SetDisabled();
 }
 
@@ -205,7 +271,7 @@ TEST_F(MemoryDumpManagerTest, CheckMemoryDumpArgs) {
           Invoke(&mdp_high_detail,
                  &MockDumpProvider::OnMemoryDump_CheckMemoryDumpArgs));
   mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                          high_detail_args);
+                          g_high_detail_args);
   DisableTracing();
   mdm_->UnregisterDumpProvider(&mdp_high_detail);
 
@@ -221,7 +287,7 @@ TEST_F(MemoryDumpManagerTest, CheckMemoryDumpArgs) {
           Invoke(&mdp_low_detail,
                  &MockDumpProvider::OnMemoryDump_CheckMemoryDumpArgs));
   mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                          low_detail_args);
+                          g_low_detail_args);
   DisableTracing();
   mdm_->UnregisterDumpProvider(&mdp_low_detail);
 }
@@ -244,7 +310,7 @@ TEST_F(MemoryDumpManagerTest, SharedSessionState) {
 
   for (int i = 0; i < 2; ++i)
     mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                            high_detail_args);
+                            g_high_detail_args);
 
   DisableTracing();
 }
@@ -259,7 +325,7 @@ TEST_F(MemoryDumpManagerTest, MultipleDumpers) {
   EXPECT_CALL(mdp1, OnMemoryDump(_, _)).Times(1).WillRepeatedly(Return(true));
   EXPECT_CALL(mdp2, OnMemoryDump(_, _)).Times(0);
   mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                          high_detail_args);
+                          g_high_detail_args);
   DisableTracing();
 
   // Invert: enable mdp1 and disable mdp2.
@@ -269,7 +335,7 @@ TEST_F(MemoryDumpManagerTest, MultipleDumpers) {
   EXPECT_CALL(mdp1, OnMemoryDump(_, _)).Times(0);
   EXPECT_CALL(mdp2, OnMemoryDump(_, _)).Times(1).WillRepeatedly(Return(true));
   mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                          high_detail_args);
+                          g_high_detail_args);
   DisableTracing();
 
   // Enable both mdp1 and mdp2.
@@ -278,7 +344,7 @@ TEST_F(MemoryDumpManagerTest, MultipleDumpers) {
   EXPECT_CALL(mdp1, OnMemoryDump(_, _)).Times(1).WillRepeatedly(Return(true));
   EXPECT_CALL(mdp2, OnMemoryDump(_, _)).Times(1).WillRepeatedly(Return(true));
   mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                          high_detail_args);
+                          g_high_detail_args);
   DisableTracing();
 }
 
@@ -366,7 +432,7 @@ TEST_F(MemoryDumpManagerTest, RespectTaskRunnerAffinity) {
           Bind(&MemoryDumpManagerTest::DumpCallbackAdapter, Unretained(this),
                MessageLoop::current()->task_runner(), run_loop.QuitClosure());
       mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                              high_detail_args, callback);
+                              g_high_detail_args, callback);
       // This nested message loop (|run_loop|) will be quit if and only if
       // the RequestGlobalDump callback is invoked.
       run_loop.Run();
@@ -414,7 +480,7 @@ TEST_F(MemoryDumpManagerTest, DisableFailingDumpers) {
   for (int i = 0; i < 1 + MemoryDumpManager::kMaxConsecutiveFailuresCount;
        i++) {
     mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                            high_detail_args);
+                            g_high_detail_args);
   }
 
   DisableTracing();
@@ -445,7 +511,7 @@ TEST_F(MemoryDumpManagerTest, RegisterDumperWhileDumping) {
 
   for (int i = 0; i < 4; i++) {
     mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                            high_detail_args);
+                            g_high_detail_args);
   }
 
   DisableTracing();
@@ -476,7 +542,7 @@ TEST_F(MemoryDumpManagerTest, UnregisterDumperWhileDumping) {
 
   for (int i = 0; i < 4; i++) {
     mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                            high_detail_args);
+                            g_high_detail_args);
   }
 
   DisableTracing();
@@ -552,11 +618,31 @@ TEST_F(MemoryDumpManagerTest, CallbackCalledOnFailure) {
         Bind(&MemoryDumpManagerTest::DumpCallbackAdapter, Unretained(this),
              MessageLoop::current()->task_runner(), run_loop.QuitClosure());
     mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                            high_detail_args, callback);
+                            g_high_detail_args, callback);
     run_loop.Run();
   }
   EXPECT_FALSE(last_callback_success_);
 }
 
+TEST_F(MemoryDumpManagerTest, TraceConfigTriggers) {

[Primiano Tucci (use gerrit), 2015/08/26 10:25:06]

The name here (TraceConfigTriggers) should reflect what this fixture is testing.
Also please add a comment explaining what this test does.
I'd probably name it SchedulePeriodicDumpsFromTraceConfig

+  MockDumpProvider mdp;
+  mdm_->RegisterDumpProvider(&mdp);
+  const int kTestDumpCount = 10;
+  RunLoop run_loop;
+  EnableTracingWithTriggers();
+  EXPECT_CALL(mdp, OnMemoryDump(_, _))
+      .Times(kTestDumpCount)
+      .WillRepeatedly(
+          Invoke(&mdp, &MockDumpProvider::OnMemoryDump_TraceConfigTriggers));
+
+  // 50 ms is the shortest interval given in the kMemoryDumpTraceConfigString.

[Primiano Tucci (use gerrit), 2015/08/26 10:25:06]

Please don't have tests based on timings as they are extremely flaky.
I'd just make this test as follows:
Make the dump provider post the QuitClosure on the Nth dump, and just check that
you receive the right amount of light and heavy dumps

+  // kTestDumpCount * 50 is time taken for kTestDumpCount "OnMemoryDump" calls.
+  const uint32 test_runtime = kTestDumpCount * 50 + 25;
+  MessageLoop::current()->task_runner()->PostDelayedTask(
+      FROM_HERE, run_loop.QuitClosure(),
+      base::TimeDelta::FromMilliseconds(test_runtime));
+  run_loop.Run();
+}

[Primiano Tucci (use gerrit), 2015/08/26 10:25:07]

Also you should have a test to make sure that if you use the old-style
traceconfig ctor you still get the periodic dumps.

[ssid, 2015/08/28 12:34:48]

This test is not needed here since, all we test here is if we get the correct
TraceConfig and follow it. This test was added in TraceConfig change.

+
 }  // namespace trace_event
 }  // namespace base