memory-infra: Start disentangling tracing from memory-infra

base/trace_event/memory_tracing_frontend.cc

Index: base/trace_event/memory_tracing_frontend.cc
diff --git a/base/trace_event/memory_tracing_frontend.cc b/base/trace_event/memory_tracing_frontend.cc
new file mode 100644
index 0000000000000000000000000000000000000000..054f078d871ec78a6a23c2da7d0bbf1b49d2fbe1
--- /dev/null
+++ b/base/trace_event/memory_tracing_frontend.cc
@@ -0,0 +1,79 @@
+// 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 "base/trace_event/memory_tracing_frontend.h"
+
+namespace base {
+namespace trace_event {
+
+// static
+const char* const MemoryTracingFrontend::kTraceCategory =
+    TRACE_DISABLED_BY_DEFAULT("memory-infra");

[hjd, 2017/04/13 14:16:59]

^ I will remove this and move the constant in a follow up since it ends up being
a pretty noisy change.

[Primiano Tucci (use gerrit), 2017/04/13 15:22:37]

Yeah I agree. for the moment just leave it in MDM and we can move it later
separately. 

+
+// static
+MemoryTracingFrontend* g_instance = nullptr;
+void MemoryTracingFrontend::Initialize(MemoryDumpManager* mdm) {
+  DCHECK(!g_instance);
+  if (g_instance)
+    return;
+  g_instance = new MemoryTracingFrontend(TraceLog::GetInstance(), mdm);
+}

[hjd, 2017/04/13 14:16:59]

^ This seems very dubious but I'm not sure what the correct way to do this is.

[Primiano Tucci (use gerrit), 2017/04/13 15:22:37]

This is a bit of an odd pattern. I think it's cleaner and safer (Threading wise)
to just have  a conventional:
 static TMF* GetInstance() {
  static MemoryTracingFrontend* instance = new TMF();
  return instance;
}

and Initialize() being an instance (non-static) method

[hjd, 2017/04/13 16:32:18]

It adds a lifecycle to the object though which I was hoping to avoid :( Like it
would be possible to have a reference to a TracingFrontend but for none of the
methods to work since we hadn't Initialize'd it, this way we can remove the
possibility of error.

Maybe it would be better for the memory service client to create/hold on to an
instance? Rather than creating another pseudo singleton.

+
+MemoryTracingFrontend::MemoryTracingFrontend(
+    TraceLog* trace_log,

[Primiano Tucci (use gerrit), 2017/04/13 15:22:37]

both TraceLog and MDM are singletons. Why do you need to pass/inject them?

[hjd, 2017/04/13 16:32:18]

So this class can be tested in isolation. Otherwise we have to setup/teardown
(through a private api which isn't actually used in production since they're
both singletons) a ~1000 lines of multithreaded weirdness. It also makes this
test faster (no threads), more actionable (failures of MDM/TraceLog won't break
this) and simpler (don't have to understand MDM).

+    MemoryDumpManager* memory_dump_manager) {
+  enabled_ = false;
+  trace_log_ = trace_log;
+  memory_dump_manager_ = memory_dump_manager;
+
+  // DCHECK(memory_dump_manager_->IsInitialized());

[hjd, 2017/04/13 14:16:59]

Is it worth implementing IsInitialized for this DCHECK?

[Primiano Tucci (use gerrit), 2017/04/13 15:22:37]

I don't hink you need to rely on MDM being initialized (the definition of MDM
being "initialized" is unfortunataely very whacky)

[hjd, 2017/04/13 16:32:18]

I think I shouldn't call MDM->Enable() until after MDM->Initialize()?
MDM->Initialize() does stuff like:
delegate_ = std::move(delegate); and
MDM->Enable() does stuff like:
delegate_->IsCoordinator()

+
+  // If tracing was enabled before initializing MemoryDumpManager, we missed the
+  // OnTraceLogEnabled() event. Synthetize it so we can late-join the party.
+  // IsEnabled is called before adding observer to avoid calling
+  // OnTraceLogEnabled twice.
+  bool is_tracing_already_enabled = trace_log_->IsEnabled();
+  trace_log_->AddEnabledStateObserver(this);
+  if (is_tracing_already_enabled)
+    OnTraceLogEnabled();
+}
+
+MemoryTracingFrontend::~MemoryTracingFrontend() {
+  trace_log_->RemoveEnabledStateObserver(this);
+}
+
+void MemoryTracingFrontend::OnTraceLogEnabled() {
+  if (!IsMemoryInfraTracingEnabled())
+    return;
+
+  enabled_ = true;
+  memory_dump_manager_->Enable();
+}
+
+void MemoryTracingFrontend::OnTraceLogDisabled() {
+  if (!enabled_)
+    return;
+  memory_dump_manager_->Disable();
+  enabled_ = false;
+}
+
+bool MemoryTracingFrontend::IsMemoryInfraTracingEnabled() {
+  bool enabled;
+  TRACE_EVENT_CATEGORY_GROUP_ENABLED(kTraceCategory, &enabled);
+  return enabled;
+}
+
+// void MemoryTracingFrontend::RequestGlobalDump() {
+//  if (!enabled_)
+//    return;
+//
+//  auto dump_type = MemoryDumpType::EXPLICITLY_TRIGGERED;
+//  auto detail_level = MemoryDumpLevelOfDetail::DETAILED;
+//  auto callback = nullptr;
+//
+//  memory_dump_manager_->RequestGlobalDump(dump_type, detail_level, callback);
+//}

[hjd, 2017/04/13 14:16:59]

I will remove.

+
+}  // namespace trace_event
+}  // namespace base