Add LeakDetectorRemoteController as a Mojo service

chrome/browser/metrics/leak_detector_remote_controller.cc

Index: chrome/browser/metrics/leak_detector_remote_controller.cc
diff --git a/chrome/browser/metrics/leak_detector_remote_controller.cc b/chrome/browser/metrics/leak_detector_remote_controller.cc
new file mode 100644
index 0000000000000000000000000000000000000000..ba387f184d3070e589635b9578b0895c428d1ffd
--- /dev/null
+++ b/chrome/browser/metrics/leak_detector_remote_controller.cc
@@ -0,0 +1,56 @@
+// Copyright 2016 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 "chrome/browser/metrics/leak_detector_remote_controller.h"
+
+#include "content/public/browser/browser_thread.h"
+
+namespace metrics {
+
+LeakDetectorRemoteController::~LeakDetectorRemoteController() {}
+
+// static
+void LeakDetectorRemoteController::Create(LeakDetectorRemoteRequest request) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  new LeakDetectorRemoteController(std::move(request));
+}
+
+void LeakDetectorRemoteController::GetParams(
+    const LeakDetectorRemote::GetParamsCallback& cb) {

[Ilya Sherman, 2016/06/07 23:03:38]

nit: s/cb/callback

[Simon Que, 2016/06/08 01:14:18]

Done.

+  // If no controller exists, send an empty param protobuf. The remote caller
+  // should not initialize anything if the params are empty.

[Ilya Sherman, 2016/06/07 23:03:38]

Would it be reasonable to instead just DCHECK that a controller has been set by
this point?  Or is desirable for it to be possible for the controller to be
unset?

[Simon Que, 2016/06/08 01:14:18]

The intended operation is that the LocalController instance is set, but this
code allows the remote client to call this service smoothly even if this weren't
set. The remote client will see that |params.sampling_rate()| == 0 and refrain
from initializing the leak detector on the renderer process.

This allows us to check in all this code without adding any variation
config-based conditionals. Consider the alternative: There is no
LeakDetectorController / LocalController instance. The remote renderer process
doesn't know which variation is being used. It calls GetParams() and then this
function crashes.

+  MemoryLeakReportProto_Params params;
+  if (controller_) {
+    controller_->GetParams(&params);
+  }
+
+  // Serialize the params before sending it to the Mojo client.
+  std::string serialized_params;
+  params.SerializeToString(&serialized_params);
+
+  cb.Run(serialized_params);
+}
+
+void LeakDetectorRemoteController::SendLeakReports(
+    mojo::Array<mojo::String> reports) {
+  std::vector<MemoryLeakReportProto> report_protos;
+  report_protos.reserve(reports.size());
+
+  for (const mojo::String& serialized_report : reports) {
+    report_protos.push_back(MemoryLeakReportProto());
+    report_protos.back().ParseFromString(serialized_report);
+  }
+  DCHECK(controller_);
+  controller_->SendLeakReports(report_protos);
+}
+
+LeakDetectorRemoteController::LeakDetectorRemoteController(
+    LeakDetectorRemoteRequest request)
+    : binding_(this, std::move(request)) {}
+
+// static
+LeakDetectorRemoteController::LocalController*
+    LeakDetectorRemoteController::controller_ = nullptr;
+
+}  // namespace metrics

[Ilya Sherman, 2016/06/07 23:03:38]

It looks like this code could live within //components rather than within
//chrome -- is that true?  If so, why did you decide to implement it under
//chrome rather than //components?