Convert utility process ImageWriter IPC to mojo

chrome/utility/image_writer/image_writer_handler.cc

Index: chrome/utility/image_writer/image_writer_handler.cc
diff --git a/chrome/utility/image_writer/image_writer_handler.cc b/chrome/utility/image_writer/image_writer_handler.cc
index 3adf42e9fc274e54cce29daeba719d659e22b3d6..426015c11bf24ab2f93e99d7ebb0b7e2b95175f6 100644
--- a/chrome/utility/image_writer/image_writer_handler.cc
+++ b/chrome/utility/image_writer/image_writer_handler.cc
@@ -4,55 +4,48 @@
 
 #include "chrome/utility/image_writer/image_writer_handler.h"
 
+#include "base/bind.h"
 #include "base/files/file_path.h"
-#include "chrome/common/extensions/chrome_utility_extensions_messages.h"
+#include "base/optional.h"
+#include "chrome/common/extensions/removable_storage_writer.mojom.h"
 #include "chrome/utility/image_writer/error_messages.h"
-#include "content/public/utility/utility_thread.h"
 
-namespace image_writer {
-
-ImageWriterHandler::ImageWriterHandler() {}
-ImageWriterHandler::~ImageWriterHandler() {}
+namespace {
 
-void ImageWriterHandler::SendSucceeded() {
-  Send(new ChromeUtilityHostMsg_ImageWriter_Succeeded());
-  content::UtilityThread::Get()->ReleaseProcessIfNeeded();
+bool IsTestDevice(const base::FilePath& device) {
+  return device.AsUTF8Unsafe() ==
+         extensions::mojom::RemovableStorageWriter::kTestDevice;

[dcheng, 2017/02/06 05:52:02]

Seeing this makes me kind of feel that this should just define this as a
FILE_PATH_LITERAL and leave it outside the mojom; it seems like it should be
possible to do this with a simple comparison without constructing temporaries,
etc.

(It makes me slightly sad to have testing code closely interleaved into
production interfaces... for this sort of thing, does it make sense to use a
test interface for the browser test?)

[Noel Gordon, 2017/02/07 11:31:09]

I don't really mind either way, but other reviewers prefer it in the mojom,
maybe because in code search you can search for

  [ extensions::mojom::RemovableStorageWriter ]

and find all the related stuff. Both ways have merits I suppose.
Not sure why constructing temporaries would matter.  The file we write is a
ChromeOS image, they are 2GByte or more in size based on my testing, and the
image write operation takes order 10 minutes on my USB stick.
Makes me even more sad to see production interfaces with no tests.  Testing when
a child process is involved is hard, and I have invent things like kTestDevice
just to have the child and browser process co-ordinate during a test.

for this sort of thing, does it make sense to use a
kTestDevice is kinda that, but is a compromise.  Adding test-only API to mojo
interfaces is an idea I've had also, but the idea got no traction in off-line
discussion.

[dcheng, 2017/02/08 09:31:18]

It's true that it's a fairly minor cost, but it does add up -- creating and
destroying a temporary std::string is surprisingly heavy in terms of binary size
(which does matter somewhat, even if this is never hit).
I was kind of thinking that we would just substitute a completely separate mojo
interface when we're in test mode. I don't actually know how we would accomplish
that though. Basically, it's not clear to me how scalable this test mode
approach will be. If it's just a few interfaces, maybe it's not worth thinking
too much about.

 }
 
-void ImageWriterHandler::SendCancelled() {
-  Send(new ChromeUtilityHostMsg_ImageWriter_Cancelled());
-  content::UtilityThread::Get()->ReleaseProcessIfNeeded();
+base::FilePath MakeTestDevicePath(const base::FilePath& image) {
+  return image.ReplaceExtension(FILE_PATH_LITERAL("out"));
 }
 
-void ImageWriterHandler::SendFailed(const std::string& message) {
-  Send(new ChromeUtilityHostMsg_ImageWriter_Failed(message));
-  content::UtilityThread::Get()->ReleaseProcessIfNeeded();
-}
+}  // namespace
 
-void ImageWriterHandler::SendProgress(int64_t progress) {
-  Send(new ChromeUtilityHostMsg_ImageWriter_Progress(progress));
-}
+namespace image_writer {
 
-void ImageWriterHandler::Send(IPC::Message* msg) {
-  content::UtilityThread::Get()->Send(msg);
-}
+ImageWriterHandler::ImageWriterHandler() = default;
 
-bool ImageWriterHandler::OnMessageReceived(const IPC::Message& message) {
-  bool handled = true;
-  IPC_BEGIN_MESSAGE_MAP(ImageWriterHandler, message)
-  IPC_MESSAGE_HANDLER(ChromeUtilityMsg_ImageWriter_Write, OnWriteStart)
-  IPC_MESSAGE_HANDLER(ChromeUtilityMsg_ImageWriter_Verify, OnVerifyStart)
-  IPC_MESSAGE_HANDLER(ChromeUtilityMsg_ImageWriter_Cancel, OnCancel)
-  IPC_MESSAGE_UNHANDLED(handled = false)
-  IPC_END_MESSAGE_MAP()
-  return handled;
-}
+ImageWriterHandler::~ImageWriterHandler() = default;
+
+void ImageWriterHandler::Write(
+    const base::FilePath& image,
+    const base::FilePath& device,
+    extensions::mojom::RemovableStorageWriterClientPtr client) {
+  client_ = std::move(client);
+  client_.set_connection_error_handler(
+      base::Bind(&ImageWriterHandler::Cancel, base::Unretained(this)));
+
+  base::FilePath target_device = device;
+  const bool test_mode = IsTestDevice(device);
+  if (test_mode)
+    target_device = MakeTestDevicePath(image);
 
-void ImageWriterHandler::OnWriteStart(const base::FilePath& image,
-                                      const base::FilePath& device) {
+  // http://crbug.com/352442
   if (!image_writer_.get() || image != image_writer_->GetImagePath() ||
-      device != image_writer_->GetDevicePath()) {
-    image_writer_.reset(new ImageWriter(this, image, device));
+      target_device != image_writer_->GetDevicePath()) {
+    image_writer_.reset(new ImageWriter(this, image, target_device));
   }
 
   if (image_writer_->IsRunning()) {
@@ -60,6 +53,11 @@ void ImageWriterHandler::OnWriteStart(const base::FilePath& image,
     return;
   }
 
+  if (test_mode) {
+    image_writer_->Write();
+    return;
+  }
+
   if (!image_writer_->IsValidDevice()) {
     SendFailed(error::kInvalidDevice);
     return;
@@ -69,11 +67,23 @@ void ImageWriterHandler::OnWriteStart(const base::FilePath& image,
       base::Bind(&ImageWriter::Write, image_writer_->AsWeakPtr()));
 }
 
-void ImageWriterHandler::OnVerifyStart(const base::FilePath& image,
-                                       const base::FilePath& device) {
+void ImageWriterHandler::Verify(
+    const base::FilePath& image,
+    const base::FilePath& device,
+    extensions::mojom::RemovableStorageWriterClientPtr client) {
+  client_ = std::move(client);
+  client_.set_connection_error_handler(
+      base::Bind(&ImageWriterHandler::Cancel, base::Unretained(this)));
+
+  base::FilePath target_device = device;
+  const bool test_mode = IsTestDevice(device);
+  if (test_mode)
+    target_device = MakeTestDevicePath(image);
+
+  // http://crbug.com/352442
   if (!image_writer_.get() || image != image_writer_->GetImagePath() ||
-      device != image_writer_->GetDevicePath()) {
-    image_writer_.reset(new ImageWriter(this, image, device));
+      target_device != image_writer_->GetDevicePath()) {
+    image_writer_.reset(new ImageWriter(this, image, target_device));
   }
 
   if (image_writer_->IsRunning()) {
@@ -81,6 +91,11 @@ void ImageWriterHandler::OnVerifyStart(const base::FilePath& image,
     return;
   }
 
+  if (test_mode) {
+    image_writer_->Verify();
+    return;
+  }
+
   if (!image_writer_->IsValidDevice()) {
     SendFailed(error::kInvalidDevice);
     return;
@@ -89,12 +104,24 @@ void ImageWriterHandler::OnVerifyStart(const base::FilePath& image,
   image_writer_->Verify();
 }
 
-void ImageWriterHandler::OnCancel() {
-  if (image_writer_.get()) {
+void ImageWriterHandler::SendProgress(int64_t progress) {
+  client_->Progress(progress);
+}
+
+void ImageWriterHandler::SendSucceeded() {
+  client_->Complete(base::nullopt);
+  client_.reset();
+}
+
+void ImageWriterHandler::SendFailed(const std::string& error) {
+  client_->Complete(error);
+  client_.reset();
+}
+
+void ImageWriterHandler::Cancel() {
+  if (image_writer_.get())

[dcheng, 2017/02/06 05:52:02]

Nit: no .get()

[Noel Gordon, 2017/02/07 11:31:09]

Done.

     image_writer_->Cancel();
-  } else {
-    SendCancelled();
-  }
+  client_.reset();
 }
 
 }  // namespace image_writer