Implement FromDict/IntoDict for BackgroundTracingConfig.

content/browser/tracing/background_tracing_config.cc

+// Copyright 2015 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 "base/macros.h"
+#include "base/values.h"
+#include "content/public/browser/background_tracing_preemptive_config.h"
+#include "content/public/browser/background_tracing_reactive_config.h"
+
+namespace content {
+
+namespace {
+
+const char kConfigsKey[] = "configs";
+
+const char kConfigModeKey[] = "mode";
+const char kConfigModePreemptive[] = "preemptive";
+const char kConfigModeReactive[] = "reactive";
+
+const char kConfigCategoryKey[] = "category";
+const char kConfigCategoryBenchmark[] = "benchmark";
+const char kConfigCategoryBenchmarkDeep[] = "benchmark-deep";
+
+const char kConfigRuleKey[] = "rule";
+const char kConfigRuleMonitorNamed[] = "monitor_named";
+const char kConfigRuleTriggerNameKey[] = "trigger_name";
+const char kReactiveConfigRuleTraceOnTriggerOr10sOrTriggerOrFull[] =
+    "trace_on_trigger_or_10s_or_trigger_or_full";

[oystein (OOO til 10th of July), 2015/05/28 17:00:46]

Shouldn't this be trace_on_trigger_until_10s_or_trigger_or_full?

[shatch, 2015/05/28 19:17:15]

Ya was trying to shorten it since it was so long, but really no point since it's
still super long.

+
+const char kReactiveConfigTriggerNameKey[] = "trigger_name";
+
+}  // namespace
+
+std::string CategoryPresetToString(
+    BackgroundTracingConfig::CategoryPreset category_preset) {
+  switch (category_preset) {
+    case BackgroundTracingConfig::BENCHMARK:
+      return kConfigCategoryBenchmark;
+    case BackgroundTracingConfig::BENCHMARK_DEEP:
+      return kConfigCategoryBenchmarkDeep;
+      break;
+  }
+  CHECK(false);

[oystein (OOO til 10th of July), 2015/05/28 17:00:46]

NOTREACHED()?

[shatch, 2015/05/28 19:17:15]

Done.

+  return "";
+}
+
+bool IsStringValidCategoryPreset(const std::string& category_preset_string) {

[oystein (OOO til 10th of July), 2015/05/28 17:00:46]

Feels like IsStringValidCategoryPreset() and StringToCategoryPreset() should be
one function maybe? bool whatever(std::string* category).

[shatch, 2015/05/28 19:17:14]

Done.

+  if (category_preset_string == kConfigCategoryBenchmark ||
+      category_preset_string == kConfigCategoryBenchmarkDeep) {
+    return true;
+  }
+  return false;
+}
+
+BackgroundTracingConfig::CategoryPreset StringToCategoryPreset(
+    const std::string& category_preset_string) {
+  if (category_preset_string == kConfigCategoryBenchmark) {
+    return BackgroundTracingConfig::BENCHMARK;
+  } else if (category_preset_string == kConfigCategoryBenchmarkDeep) {
+    return BackgroundTracingConfig::BENCHMARK_DEEP;
+  }
+
+  CHECK(false);

[oystein (OOO til 10th of July), 2015/05/28 17:00:46]

NOTREACHED()?

[shatch, 2015/05/28 19:17:15]

Done.

+  return BackgroundTracingConfig::BENCHMARK;
+}
+
+BackgroundTracingReactiveConfig::RuleType StringToReactiveConfigRuleType(
+    const std::string& rule_type) {
+  if (rule_type == kReactiveConfigRuleTraceOnTriggerOr10sOrTriggerOrFull) {
+    return BackgroundTracingReactiveConfig::
+        TRACE_ON_MANUAL_TRIGGER_UNTIL_10S_OR_NEXT_TRIGGER_OR_FULL;
+  }
+  CHECK(false);

[oystein (OOO til 10th of July), 2015/05/28 17:00:46]

NOTREACHED()?

[shatch, 2015/05/28 19:17:15]

Done.

+  return BackgroundTracingReactiveConfig::
+      TRACE_ON_MANUAL_TRIGGER_UNTIL_10S_OR_NEXT_TRIGGER_OR_FULL;
+}
+
+scoped_ptr<BackgroundTracingPreemptiveConfig>
+BackgroundTracingPreemptiveConfig_FromDict(const base::DictionaryValue* dict) {
+  scoped_ptr<BackgroundTracingPreemptiveConfig> config(
+      new BackgroundTracingPreemptiveConfig());
+
+  std::string category_preset_string;
+  if (!dict->GetString(kConfigCategoryKey, &category_preset_string))
+    return NULL;
+
+  if (!IsStringValidCategoryPreset(category_preset_string))
+    return NULL;
+
+  config->category_preset = StringToCategoryPreset(category_preset_string);
+
+  const base::ListValue* configs_list = NULL;
+  if (!dict->GetList(kConfigsKey, &configs_list))
+    return NULL;
+
+  for (auto it = configs_list->begin(); it != configs_list->end(); ++it) {
+    const base::DictionaryValue* config_dict = NULL;
+    if (!(*it)->GetAsDictionary(&config_dict))
+      return NULL;
+
+    std::string type;
+    if (!config_dict->GetString(kConfigRuleKey, &type))
+      return NULL;
+
+    if (type == kConfigRuleMonitorNamed) {
+      std::string trigger_name;
+      if (!config_dict->GetString(kConfigRuleTriggerNameKey, &trigger_name))
+        return NULL;
+
+      BackgroundTracingPreemptiveConfig::MonitoringRule rule;
+      rule.type = BackgroundTracingPreemptiveConfig::
+          MONITOR_AND_DUMP_WHEN_TRIGGER_NAMED;
+      rule.named_trigger_info.trigger_name = trigger_name;
+
+      config->configs.push_back(rule);
+    } else {
+      // TODO(simonhatch): Implement UMA triggers.

[oystein (OOO til 10th of July), 2015/05/28 17:00:46]

Brainstorming for later CLs: Let's add UMAs that count up when a dict results in
a valid and activated scenario, and when we fail somewhere on the way.

+      return NULL;
+    }
+  }
+
+  return config.Pass();
+}
+
+scoped_ptr<BackgroundTracingReactiveConfig>
+BackgroundTracingReactiveConfig_FromDict(const base::DictionaryValue* dict) {
+  scoped_ptr<BackgroundTracingReactiveConfig> config(
+      new BackgroundTracingReactiveConfig());
+
+  const base::ListValue* configs_list = NULL;
+  if (!dict->GetList(kConfigsKey, &configs_list))
+    return NULL;
+
+  for (auto it = configs_list->begin(); it != configs_list->end(); ++it) {
+    const base::DictionaryValue* config_dict = NULL;
+    if (!(*it)->GetAsDictionary(&config_dict))
+      return NULL;
+
+    BackgroundTracingReactiveConfig::TracingRule rule;
+
+    std::string category_preset_string;
+    if (!config_dict->GetString(kConfigCategoryKey, &category_preset_string))
+      return NULL;
+
+    if (!IsStringValidCategoryPreset(category_preset_string))
+      return NULL;
+
+    rule.category_preset = StringToCategoryPreset(category_preset_string);
+
+    std::string type;
+    if (!config_dict->GetString(kConfigRuleKey, &type))
+      return NULL;
+
+    rule.type = StringToReactiveConfigRuleType(type);
+
+    std::string trigger_name;
+    if (!config_dict->GetString(kReactiveConfigTriggerNameKey, &trigger_name))
+      return NULL;
+
+    rule.trigger_name = trigger_name;
+
+    config->configs.push_back(rule);
+  }
+
+  return config.Pass();
+}
+
+bool BackgroundTracingPreemptiveConfig_IntoDict(
+    const BackgroundTracingPreemptiveConfig* config,
+    base::DictionaryValue* dict) {
+  dict->SetString(kConfigCategoryKey,
+                  CategoryPresetToString(config->category_preset));
+
+  base::ListValue* configs_list = new base::ListValue();

[oystein (OOO til 10th of July), 2015/05/28 17:00:47]

Is configs_list and config_dict leaking if we return false below, without
appending them to the dict? Maybe they could be scoped_ptrs that get released on
success instead?

[shatch, 2015/05/28 19:17:15]

Done.

+
+  for (size_t i = 0; i < config->configs.size(); ++i) {
+    base::DictionaryValue* config_dict = new base::DictionaryValue();
+
+    if (config->configs[i].type == BackgroundTracingPreemptiveConfig::
+                                       MONITOR_AND_DUMP_WHEN_TRIGGER_NAMED) {
+      config_dict->SetString(kConfigRuleKey, kConfigRuleMonitorNamed);
+      config_dict->SetString(
+          kConfigRuleTriggerNameKey,
+          config->configs[i].named_trigger_info.trigger_name.c_str());
+    } else {
+      // TODO(simonhatch): Implement UMA triggers.
+      return false;
+    }
+
+    configs_list->Append(config_dict);
+  }
+
+  dict->Set(kConfigsKey, configs_list);
+
+  return true;
+}
+
+bool BackgroundTracingReactiveConfig_IntoDict(
+    const BackgroundTracingReactiveConfig* config,
+    base::DictionaryValue* dict) {
+  base::ListValue* configs_list = new base::ListValue();
+
+  for (size_t i = 0; i < config->configs.size(); ++i) {
+    base::DictionaryValue* config_dict = new base::DictionaryValue();

[oystein (OOO til 10th of July), 2015/05/28 17:00:46]

Same leak comment as above

[shatch, 2015/05/28 19:17:15]

Done.

+
+    config_dict->SetString(
+        kConfigCategoryKey,
+        CategoryPresetToString(config->configs[i].category_preset));
+
+    switch (config->configs[i].type) {
+      case BackgroundTracingReactiveConfig::
+          TRACE_ON_MANUAL_TRIGGER_UNTIL_10S_OR_NEXT_TRIGGER_OR_FULL:
+        config_dict->SetString(
+            kConfigRuleKey,
+            kReactiveConfigRuleTraceOnTriggerOr10sOrTriggerOrFull);
+        break;
+      default:

[oystein (OOO til 10th of July), 2015/05/28 17:00:46]

Do we need the default?

[shatch, 2015/05/28 19:17:15]

Yeah to error out for any enums we don't support yet.

+        return false;
+    }
+
+    config_dict->SetString(kConfigRuleTriggerNameKey,
+                           config->configs[i].trigger_name.c_str());
+
+    configs_list->Append(config_dict);
+  }
+
+  dict->Set(kConfigsKey, configs_list);
+
+  return true;
+}
+
+scoped_ptr<BackgroundTracingConfig> BackgroundTracingConfig::FromDict(
+    const base::DictionaryValue* dict) {
+  if (!dict)

[oystein (OOO til 10th of July), 2015/05/28 17:00:46]

Can this ever really be NULL? Feels like it should be a DCHECK and checked on
the calling site.

[shatch, 2015/05/28 19:17:15]

Done.

+    return NULL;
+
+  std::string mode;
+  if (!dict->GetString(kConfigModeKey, &mode))
+    return NULL;
+
+  scoped_ptr<BackgroundTracingConfig> config;
+
+  if (mode == kConfigModePreemptive) {
+    config = BackgroundTracingPreemptiveConfig_FromDict(dict);
+  } else if (mode == kConfigModeReactive) {
+    config = BackgroundTracingReactiveConfig_FromDict(dict);
+  } else {
+    return NULL;
+  }
+
+  return config.Pass();
+}
+
+void BackgroundTracingConfig::IntoDict(const BackgroundTracingConfig* config,
+                                       base::DictionaryValue* dict) {
+  switch (config->mode) {

[oystein (OOO til 10th of July), 2015/05/28 17:00:46]

Switching on this and then static_casting() makes me a little bit nervous from a
security perspective.

Does the mode have to be a member variable? Can it be a pure virtual function
instead implemented in each subclass?

[shatch, 2015/05/28 19:17:15]

Chatted offline, maybe we can clean this up with a followup CL.

+    case PREEMPTIVE_TRACING_MODE:
+      dict->SetString(kConfigModeKey, kConfigModePreemptive);
+      if (!BackgroundTracingPreemptiveConfig_IntoDict(
+              static_cast<const BackgroundTracingPreemptiveConfig*>(config),
+              dict))
+        dict->Clear();
+      break;
+    case REACTIVE_TRACING_MODE:
+      dict->SetString(kConfigModeKey, kConfigModeReactive);
+      if (!BackgroundTracingReactiveConfig_IntoDict(
+              static_cast<const BackgroundTracingReactiveConfig*>(config),
+              dict))
+        dict->Clear();
+      break;
+  }
+}
+
+}  // namspace content