Reporting: Plumb into RenderFrame via Mojo

content/browser/net/reporting_service_proxy.cc

Index: content/browser/net/reporting_service_proxy.cc
diff --git a/content/browser/net/reporting_service_proxy.cc b/content/browser/net/reporting_service_proxy.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0cb5856c9855fc093148b1f9793b328bea21a04f
--- /dev/null
+++ b/content/browser/net/reporting_service_proxy.cc
@@ -0,0 +1,89 @@
+// 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 "content/browser/net/reporting_service_proxy.h"
+
+#include <memory>
+#include <string>
+
+#include "base/memory/ref_counted.h"
+#include "base/values.h"
+#include "content/common/net/reporting.mojom.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/site_instance.h"
+#include "content/public/browser/storage_partition.h"
+#include "mojo/public/cpp/bindings/binding.h"
+#include "net/reporting/reporting_service.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "url/gurl.h"
+
+namespace content {
+
+namespace {
+
+void QueueReportOnIOThread(
+    scoped_refptr<net::URLRequestContextGetter> request_context_getter,
+    const GURL& url,
+    const std::string& group,
+    const std::string& type,
+    std::unique_ptr<const base::Value> body) {
+  net::URLRequestContext* request_context =
+      request_context_getter->GetURLRequestContext();
+  if (!request_context)
+    return;
+
+  net::ReportingService* reporting_service =
+      request_context->reporting_service();
+  if (!reporting_service)
+    return;
+
+  // TODO(juliatuttle): Get rid of this copy.
+  reporting_service->QueueReport(url, group, type, body->CreateDeepCopy());
+}
+
+class ReportingServiceProxyImpl : public mojom::ReportingServiceProxy {
+ public:
+  ReportingServiceProxyImpl(
+      mojom::ReportingServiceProxyRequest request,
+      scoped_refptr<net::URLRequestContextGetter> request_context_getter)
+      : binding_(this, std::move(request)),
+        request_context_getter_(request_context_getter) {}

[dcheng, 2017/05/19 13:42:55]

Nit: std::move() here

[Julia Tuttle, 2017/05/19 16:12:22]

Done.

[dcheng, 2017/05/22 09:30:38]

Followup nit: please #include <utility>

[Julia Tuttle, 2017/05/22 14:30:41]

Done.

+
+  // mojom::ReportingServiceProxy:
+  void QueueReport(const GURL& url,
+                   const std::string& group,
+                   const std::string& type,
+                   std::unique_ptr<base::Value> body) override {
+    // TODO(juliatuttle): Get rid of this intermediate step.
+    const base::Value* raw_const_body = body.release();
+
+    request_context_getter_->GetNetworkTaskRunner()->PostTask(
+        FROM_HERE,
+        base::BindOnce(&QueueReportOnIOThread, request_context_getter_, url,
+                       group, type, base::WrapUnique(raw_const_body)));

[dcheng, 2017/05/19 13:42:55]

How come this can't just std::move(body)? Does the parameter need to be a
std::unique_ptr<const base::Value>, or can it just be a plain old
std::unique_ptr<base::Value? That would let us just std::move() it all the way
through.

[Julia Tuttle, 2017/05/19 16:12:22]

Reporting deals in std::unique_ptr<const base::Value>, since report bodies never
change.

I *could* change it there, but I feel like there should be a neater cast for
this?

[dcheng, 2017/05/22 09:30:38]

I see. I think std::unique_ptr<const T> is quite rare and probably causes more
trouble than it's worth. Probably not worth investigating this particular point
in more detail now, as base::Value is migrating to become a value type.

[Julia Tuttle, 2017/05/22 14:30:41]

Oh, awesome to hear.

+  }
+
+ private:
+  mojo::Binding<mojom::ReportingServiceProxy> binding_;
+  scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
+};
+
+}  // namespace
+
+// static
+void CreateReportingServiceProxy(
+    scoped_refptr<SiteInstance> site_instance,

[dcheng, 2017/05/19 13:42:55]

Nit: Consider using base::RetainedRef and accepting this argument by raw
pointer, as there doesn't seem to be any ownership transfer in the body.

[Julia Tuttle, 2017/05/19 16:12:22]

Done.

+    const service_manager::BindSourceInfo& source_info,
+    mojom::ReportingServiceProxyRequest request) {
+  BrowserContext* browser_context = site_instance->GetBrowserContext();
+  StoragePartition* storage_partition =
+      BrowserContext::GetStoragePartition(browser_context, site_instance.get());
+  // TODO(juliatuttle): Make sure making our own scoped_refptr is okay.
+  scoped_refptr<net::URLRequestContextGetter> request_context_getter(
+      storage_partition->GetURLRequestContext());
+  new ReportingServiceProxyImpl(std::move(request),

[dcheng, 2017/05/19 13:42:55]

Should this be using mojo::MakeStrongBinding? I don't see anything that would
delete this, so I think it might be leaked.

[Julia Tuttle, 2017/05/19 16:12:22]

It was leaked! Thank you for teaching me the right way to do it :)

+                                std::move(request_context_getter));
+}
+}