[tracing] Add memory dump config to TraceConfig

base/trace_event/trace_config.cc

Index: base/trace_event/trace_config.cc
diff --git a/base/trace_event/trace_config.cc b/base/trace_event/trace_config.cc
index 974e40fe4b79e8fc362b0664f91d0c1bc0b941d2..77e75d6e1181f433b14f41a72ef6b402e4beba34 100644
--- a/base/trace_event/trace_config.cc
+++ b/base/trace_event/trace_config.cc
@@ -10,6 +10,7 @@
 #include "base/strings/string_split.h"
 #include "base/strings/string_tokenizer.h"
 #include "base/strings/stringprintf.h"
+#include "base/trace_event/memory_dump_manager.h"
 #include "base/trace_event/trace_event.h"
 
 namespace base {
@@ -37,6 +38,23 @@ const char kSyntheticDelaysParam[] = "synthetic_delays";
 
 const char kSyntheticDelayCategoryFilterPrefix[] = "DELAY(";
 
+// String parameters that is used to parse memory dump config in trace config
+// string.
+const char kMemoryDumpConfigParam[] = "memory_dump_config";
+const char kTriggersParam[] = "triggers";
+const char kPeriodicIntervalParam[] = "periodic_interval_ms";
+const char kDumpTypeParam[] = "dump_type";
+const char kHeavyDumpTypeParam[] = "heavy";
+const char kLightDumpTypeParam[] = "light";
+
+// Default configuration of memory dumps.
+const TraceConfig::MemoryDumpConfig kDefaultHeavyMemoryDumpConfig = {
+    2000,  // periodic_interval_ms
+    MemoryDumpArgs::LevelOfDetail::HIGH};
+const TraceConfig::MemoryDumpConfig kDefaultLightMemoryDumpConfig = {
+    250,  // periodic_interval_ms
+    MemoryDumpArgs::LevelOfDetail::LOW};
+
 }  // namespace
 
 TraceConfig::TraceConfig() {
@@ -82,11 +100,11 @@ TraceConfig::TraceConfig(const TraceConfig& tc)
       enable_sampling_(tc.enable_sampling_),
       enable_systrace_(tc.enable_systrace_),
       enable_argument_filter_(tc.enable_argument_filter_),
+      memory_dump_config_list_(tc.memory_dump_config_list_),
       included_categories_(tc.included_categories_),
       disabled_categories_(tc.disabled_categories_),
       excluded_categories_(tc.excluded_categories_),
-      synthetic_delays_(tc.synthetic_delays_) {
-}
+      synthetic_delays_(tc.synthetic_delays_) {}
 
 TraceConfig::~TraceConfig() {
 }
@@ -99,6 +117,7 @@ TraceConfig& TraceConfig::operator=(const TraceConfig& rhs) {
   enable_sampling_ = rhs.enable_sampling_;
   enable_systrace_ = rhs.enable_systrace_;
   enable_argument_filter_ = rhs.enable_argument_filter_;
+  memory_dump_config_list_ = rhs.memory_dump_config_list_;
   included_categories_ = rhs.included_categories_;
   disabled_categories_ = rhs.disabled_categories_;
   excluded_categories_ = rhs.excluded_categories_;
@@ -207,6 +226,10 @@ void TraceConfig::Merge(const TraceConfig& config) {
     included_categories_.clear();
   }
 
+  memory_dump_config_list_.insert(memory_dump_config_list_.end(),
+                                  config.memory_dump_config_list_.begin(),
+                                  config.memory_dump_config_list_.end());
+
   disabled_categories_.insert(disabled_categories_.end(),
                               config.disabled_categories_.begin(),
                               config.disabled_categories_.end());
@@ -227,6 +250,7 @@ void TraceConfig::Clear() {
   disabled_categories_.clear();
   excluded_categories_.clear();
   synthetic_delays_.clear();
+  memory_dump_config_list_.clear();
 }
 
 void TraceConfig::InitializeDefault() {
@@ -279,14 +303,21 @@ void TraceConfig::InitializeFromConfigString(const std::string& config_string) {
   else
     enable_argument_filter_ = enable_argument_filter;
 
-
-  base::ListValue* category_list = NULL;
+  base::ListValue* category_list = nullptr;
   if (dict->GetList(kIncludedCategoriesParam, &category_list))
     SetCategoriesFromIncludedList(*category_list);
   if (dict->GetList(kExcludedCategoriesParam, &category_list))
     SetCategoriesFromExcludedList(*category_list);
   if (dict->GetList(kSyntheticDelaysParam, &category_list))
     SetSyntheticDelaysFromList(*category_list);
+
+  if (!IsCategoryEnabled(MemoryDumpManager::kTraceCategory))
+    return;

[Primiano Tucci (use gerrit), 2015/08/24 11:58:35]

probably in this case you might not want an early return, better an if.
It looks likely that sometbody might add code below. if that is the case, this
early return might subtly break things.

[ssid, 2015/08/24 15:29:30]

Done.

+
+  base::DictionaryValue* memory_dump_config = nullptr;
+  if (dict->GetDictionary(kMemoryDumpConfigParam, &memory_dump_config)) {
+    SetMemoryDumpConfigList(*memory_dump_config);
+  }
 }
 
 void TraceConfig::InitializeFromStrings(
@@ -353,6 +384,12 @@ void TraceConfig::InitializeFromStrings(
       }
     }
   }
+
+  if (IsCategoryEnabled(MemoryDumpManager::kTraceCategory)) {
+    memory_dump_config_list_.clear();
+    memory_dump_config_list_.push_back(kDefaultHeavyMemoryDumpConfig);
+    memory_dump_config_list_.push_back(kDefaultLightMemoryDumpConfig);
+  }
 }
 
 void TraceConfig::SetCategoriesFromIncludedList(
@@ -412,6 +449,47 @@ void TraceConfig::AddCategoryToDict(base::DictionaryValue& dict,
   dict.Set(param, list.Pass());
 }
 
+void TraceConfig::SetMemoryDumpConfigList(
+    base::DictionaryValue& memory_dump_config) {

[Primiano Tucci (use gerrit), 2015/08/24 11:58:35]

+const

[ssid, 2015/08/24 15:29:30]

Done.

+  base::ListValue* trigger_list = nullptr;
+  if (!memory_dump_config.GetList(kTriggersParam, &trigger_list) ||
+      trigger_list->GetSize() == 0) {
+    // If triggers are not set, use the default config.

[Primiano Tucci (use gerrit), 2015/08/24 11:58:36]

Don't you want to just disable the periodic dumps if they are empty?

[ssid, 2015/08/24 15:29:30]

yes, I forgot that part, changed.

+    memory_dump_config_list_.push_back(kDefaultHeavyMemoryDumpConfig);
+    memory_dump_config_list_.push_back(kDefaultLightMemoryDumpConfig);
+    return;
+  }
+
+  for (size_t i = 0; i < trigger_list->GetSize(); ++i) {
+    base::DictionaryValue* trigger = nullptr;
+    if (!trigger_list->GetDictionary(i, &trigger))
+      continue;
+
+    MemoryDumpConfig dump_config;
+    std::string dump_type;
+    int interval;

[Primiano Tucci (use gerrit), 2015/08/24 11:58:35]

= 0;

[ssid, 2015/08/24 15:29:30]

Done.

+
+    if (!trigger->GetInteger(kPeriodicIntervalParam, &interval)) {
+      continue;
+    }
+    DCHECK_GT(interval, 0);
+    dump_config.periodic_interval_ms = static_cast<uint32>(interval);
+
+    if (trigger->GetString(kDumpTypeParam, &dump_type)) {
+      if (dump_type == kHeavyDumpTypeParam) {
+        dump_config.dump_level_of_detail = MemoryDumpArgs::LevelOfDetail::HIGH;
+      } else if (dump_type == kLightDumpTypeParam) {
+        dump_config.dump_level_of_detail = MemoryDumpArgs::LevelOfDetail::LOW;
+      }
+    } else {
+      // If dump type is not specified use light.
+      dump_config.dump_level_of_detail = MemoryDumpArgs::LevelOfDetail::LOW;

[Primiano Tucci (use gerrit), 2015/08/24 11:58:36]

I'd probably rework this by making LOW the default assignment (before the if of
line 478) and then assign high only if the dump_type is not null and == "heavy"

This should be backwards compatible, if in future we introduce level="foo" and
foo does not exist in this version of chrome, level of detail here will not be
left unassigned

[ssid, 2015/08/24 15:29:30]

Done.

+    }
+
+    memory_dump_config_list_.push_back(dump_config);
+  }
+}
+
 void TraceConfig::ToDict(base::DictionaryValue& dict) const {
   switch (record_mode_) {
     case RECORD_UNTIL_FULL:
@@ -452,6 +530,35 @@ void TraceConfig::ToDict(base::DictionaryValue& dict) const {
   AddCategoryToDict(dict, kIncludedCategoriesParam, categories);
   AddCategoryToDict(dict, kExcludedCategoriesParam, excluded_categories_);
   AddCategoryToDict(dict, kSyntheticDelaysParam, synthetic_delays_);
+
+  if (!IsCategoryEnabled(MemoryDumpManager::kTraceCategory) ||
+      memory_dump_config_list_.size() == 0) {
+    return;

[Primiano Tucci (use gerrit), 2015/08/24 11:58:35]

same reason of above here, not sure if I'f I'd do the early return here.

[ssid, 2015/08/24 15:29:30]

Done.

+  }
+
+  scoped_ptr<base::DictionaryValue> memory_dump_config(
+      new base::DictionaryValue());
+  scoped_ptr<base::ListValue> triggers_list(new base::ListValue());
+  for (const MemoryDumpConfig& config : memory_dump_config_list_) {
+    scoped_ptr<base::DictionaryValue> trigger_dict(new base::DictionaryValue());
+    trigger_dict->SetInteger(kPeriodicIntervalParam,
+                             static_cast<int>(config.periodic_interval_ms));
+
+    switch (config.dump_level_of_detail) {
+      case MemoryDumpArgs::LevelOfDetail::LOW:
+        trigger_dict->SetString(kDumpTypeParam, kLightDumpTypeParam);
+        break;
+      case MemoryDumpArgs::LevelOfDetail::HIGH:
+        trigger_dict->SetString(kDumpTypeParam, kHeavyDumpTypeParam);
+        break;
+      default:
+        NOTREACHED();

[Primiano Tucci (use gerrit), 2015/08/24 11:58:35]

I think that you don't need the default: case here, as per our code style:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Loops_and_Swi...

* If not conditional on an enumerated value, switch statements should always
have a default case (in the case of an enumerated value, the compiler will warn
you if any values are not handled).

[ssid, 2015/08/24 15:29:31]

Yes, but I added the default case since the other enums in this file have
default case. Just added to be consistent with them.

+    }
+    triggers_list->Append(trigger_dict.Pass());
+  }
+
+  memory_dump_config->Set(kTriggersParam, triggers_list.Pass());
+  dict.Set(kMemoryDumpConfigParam, memory_dump_config.Pass());
 }
 
 std::string TraceConfig::ToTraceOptionsString() const {