Add the UtilityProcessMojoClient class and convert SafeJsonParser

content/public/browser/utility_process_mojo_client.h

+// 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.
+
+#ifndef CONTENT_PUBLIC_BROWSER_UTILITY_PROCESS_MOJO_CLIENT_H_
+#define CONTENT_PUBLIC_BROWSER_UTILITY_PROCESS_MOJO_CLIENT_H_
+
+#include "base/callback.h"

[Anand Mistry (off Chromium), 2016/06/10 11:13:55]

#include "base/logging.h" for DCHECK

[Patrick Monette, 2016/06/10 18:13:24]

Done.

+#include "base/macros.h"
+#include "base/strings/string16.h"
+#include "base/threading/thread_checker.h"
+#include "content/public/browser/utility_process_mojo_client_impl.h"
+#include "mojo/public/cpp/bindings/interface_ptr.h"
+
+namespace content {
+
+// Implements a client to a mojo service running on a utility process. Takes
+// care of starting the utility process and connecting to the remote mojo
+// service. The utility process is terminated in the destructor.
+// Note: This class is not thread-safe. It is bound to the
+// SingleThreadTaskRunner it is created on.
+template <class MojoInterface>
+class UtilityProcessMojoClient : public UtilityProcessMojoClientImpl {

[Anand Mistry (off Chromium), 2016/06/10 11:13:54]

It's kinda weird to derive from the impl. Since there are no virtual functions
and the only public function in impl is disable_sandbox() (which can be wrapped
trivially), I think the impl can be a private member instead of the base class.

[Patrick Monette, 2016/06/10 18:13:23]

Done.

+ public:
+  explicit UtilityProcessMojoClient(const base::string16& process_name)
+      : UtilityProcessMojoClientImpl(process_name) {}
+
+  // Sets an error handler that will be called when an error occurs.
+  void set_error_handler(const base::Closure& on_error_callback) {

[grt (UTC plus 2), 2016/06/09 14:48:58]

specifying an error handler seems like it will be the common case. how about
making on_error_callback a 2nd param to the ctor? callers can pass in an empty
closure if they really don't care.

[Anand Mistry (off Chromium), 2016/06/10 11:13:55]

+1. I think we want to ensure a valid callback is always specified. The callback
is how a process failure (launch or crash) is propagated and I think we want to
ensure that case is always handled. Several of the non-mojo utility process
things I've looked at haven't handled failure correctly (including both safe
json and image decoder, now mojo), and I think it would be good to ensure this
is fixed going forward.

[Patrick Monette, 2016/06/10 18:13:23]

Done. I'll make the callback mandatory.

+    on_error_callback_ = on_error_callback;
+  }
+
+  void Start() {
+    DCHECK(thread_checker_.CalledOnValidThread());

[Anand Mistry (off Chromium), 2016/06/10 11:13:54]

DCHECK(!start_called_);

[Patrick Monette, 2016/06/10 18:13:24]

Done.

+
+    start_called_ = true;
+
+    mojo::InterfaceRequest<MojoInterface> req = mojo::GetProxy(&service_);
+
+    service_.set_connection_error_handler(base::Bind(
+        &UtilityProcessMojoClient::OnConnectionError, base::Unretained(this)));
+    StartImpl(MojoInterface::Name_, req.PassMessagePipe());
+  }
+
+  // Returns the Mojo service used to make calls to the utility process.
+  MojoInterface* service() WARN_UNUSED_RESULT {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    DCHECK(start_called_);
+
+    return service_.get();
+  }
+
+ private:
+  void OnConnectionError() { on_error_callback_.Run(); }

[Anand Mistry (off Chromium), 2016/06/10 11:13:54]

If you don't make the error handler mandatory like I suggest above, you'll need
to check to see if this is null before running. Running a null base::Closure
will segfault.

[Patrick Monette, 2016/06/10 18:13:23]

Acknowledged.

+
+  // Enforce calling Start() before getting the service.
+  bool start_called_ = false;
+
+  // Called when a connection error happens or if the process didn't start.
+  base::Closure on_error_callback_;
+
+  mojo::InterfacePtr<MojoInterface> service_;
+
+  // Checks that this class is always accessed from the same thread.
+  base::ThreadChecker thread_checker_;

[grt (UTC plus 2), 2016/06/09 14:48:58]

how about getting rid of this and instead move the one in the Impl class up into
that class's "protected:" block so it can be used here, too?

[Anand Mistry (off Chromium), 2016/06/10 11:13:54]

Or get rid of the impl's thread checker. I don't think it's needed there.

[Patrick Monette, 2016/06/10 18:13:24]

Got rid of impl's thread checker.

+
+  DISALLOW_COPY_AND_ASSIGN(UtilityProcessMojoClient);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_PUBLIC_BROWSER_UTILITY_PROCESS_MOJO_CLIENT_H_