Significantly cleans up the ImageWriter Operation class and subclasses.

chrome/browser/extensions/api/image_writer_private/operation_linux.cc

Index: chrome/browser/extensions/api/image_writer_private/operation_linux.cc
diff --git a/chrome/browser/extensions/api/image_writer_private/operation_linux.cc b/chrome/browser/extensions/api/image_writer_private/operation_linux.cc
index df34834b6e8579852ed0fe2c7bcbc586ed073f04..8f9c319aaa8c8f8a2649ecd27495349d56e0d175 100644
--- a/chrome/browser/extensions/api/image_writer_private/operation_linux.cc
+++ b/chrome/browser/extensions/api/image_writer_private/operation_linux.cc
@@ -18,195 +18,174 @@ using content::BrowserThread;
 
 const int kBurningBlockSize = 8 * 1024;  // 8 KiB
 
-void Operation::WriteStart() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  if (IsCancelled()) {
-    return;
+bool Operation::OpenPlatformFiles() {
+  base::PlatformFileError result;
+
+  if (source_ == base::kInvalidPlatformFileValue) {
+    source_ = base::CreatePlatformFile(
+        image_path_,
+        base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_READ,
+        NULL,
+        &result);
+
+    if (result != base::PLATFORM_FILE_OK) {
+      DLOG(ERROR) << "Failed to open source(" << result
+                  << "): " << image_path_.value();
+      Error(error::kImageOpenError);
+      return false;
+    }
+
+    source_closer_.reset(&source_);
   }
 
-  if (image_path_.empty()) {
-    Error(error::kImageNotFound);
-    return;
+  if (target_ == base::kInvalidPlatformFileValue) {
+    target_ = base::CreatePlatformFile(device_path_,
+                                       base::PLATFORM_FILE_OPEN |
+                                           base::PLATFORM_FILE_WRITE |
+                                           base::PLATFORM_FILE_READ,
+                                       NULL,
+                                       &result);
+
+    if (result != base::PLATFORM_FILE_OK) {
+      DLOG(ERROR) << "Failed to open target(" << result
+                  << "): " << device_path_.value();
+      Error(error::kDeviceOpenError);
+      return false;
+    }
+
+    target_closer_.reset(&target_);
   }
 
-  DVLOG(1) << "Starting write of " << image_path_.value()
-           << " to " << storage_unit_id_;
+  return true;
+}
+
+void Operation::Write(const base::Closure& continuation) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  if (IsCancelled()) {
+    return;
+  }
 
   SetStage(image_writer_api::STAGE_WRITE);
 
   // TODO (haven): Unmount partitions before writing. http://crbug.com/284834
 
-  scoped_ptr<image_writer_utils::ImageReader> reader(
-      new image_writer_utils::ImageReader());
-  scoped_ptr<image_writer_utils::ImageWriter> writer(
-      new image_writer_utils::ImageWriter());
-  base::FilePath storage_path(storage_unit_id_);
-
-  if (reader->Open(image_path_)) {
-    if (!writer->Open(storage_path)) {
-      reader->Close();
-      Error(error::kDeviceOpenError);
-      return;
-    }
-  } else {
-    Error(error::kImageOpenError);
+  if (!OpenPlatformFiles())
     return;
-  }
+
+  int64 total_size;
+  base::GetFileSize(image_path_, &total_size);
 
   BrowserThread::PostTask(
-    BrowserThread::FILE,
-    FROM_HERE,
-    base::Bind(&Operation::WriteChunk,
-               this,
-               base::Passed(&reader),
-               base::Passed(&writer),
-               0));
+      BrowserThread::FILE,
+      FROM_HERE,
+      base::Bind(&Operation::WriteChunk, this, 0, total_size, continuation));
 }
 
-void Operation::WriteChunk(
-    scoped_ptr<image_writer_utils::ImageReader> reader,
-    scoped_ptr<image_writer_utils::ImageWriter> writer,
-    int64 bytes_written) {
+void Operation::WriteChunk(const int64& bytes_written,
+                           const int64& total_size,
+                           const base::Closure& continuation) {
   if (IsCancelled()) {
-    WriteCleanUp(reader.Pass(), writer.Pass());
     return;
   }
 
-  char buffer[kBurningBlockSize];
-  int64 image_size = reader->GetSize();
-  int len = reader->Read(buffer, kBurningBlockSize);
+  scoped_ptr<char[]> buffer(new char[kBurningBlockSize]);
+  int64 len = base::ReadPlatformFile(
+      source_, bytes_written, buffer.get(), kBurningBlockSize);
 
   if (len > 0) {
-    if (writer->Write(buffer, len) == len) {
-      int percent_prev = kProgressComplete * bytes_written / image_size;
-      int percent_curr = kProgressComplete * (bytes_written + len) / image_size;
-
-      if (percent_curr > percent_prev) {
-        SetProgress(percent_curr);
-      }
-
-      BrowserThread::PostTask(
-        BrowserThread::FILE,
-        FROM_HERE,
-        base::Bind(&Operation::WriteChunk,
-                   this,
-                   base::Passed(&reader),
-                   base::Passed(&writer),
-                   bytes_written + len));
+    if (base::WritePlatformFile(target_, bytes_written, buffer.get(), len) ==
+        len) {
+      int percent_curr = kProgressComplete * (bytes_written + len) / total_size;
+
+      SetProgress(percent_curr);
+
+      BrowserThread::PostTask(BrowserThread::FILE,
+                              FROM_HERE,
+                              base::Bind(&Operation::WriteChunk,
+                                         this,
+                                         bytes_written + len,
+                                         total_size,
+                                         continuation));
     } else {
-      WriteCleanUp(reader.Pass(), writer.Pass());
       Error(error::kDeviceWriteError);
     }
   } else if (len == 0) {
-    if (bytes_written == image_size) {
-      if (WriteCleanUp(reader.Pass(), writer.Pass())) {
-        BrowserThread::PostTask(
-          BrowserThread::FILE,
-          FROM_HERE,
-          base::Bind(&Operation::WriteComplete,
-                     this));
-      }
-    } else {
-      WriteCleanUp(reader.Pass(), writer.Pass());
-      Error(error::kImageReadError);
-    }
+    WriteComplete(continuation);
   } else { // len < 0
-    WriteCleanUp(reader.Pass(), writer.Pass());
     Error(error::kImageReadError);
   }
 }
 
-bool Operation::WriteCleanUp(
-    scoped_ptr<image_writer_utils::ImageReader> reader,
-    scoped_ptr<image_writer_utils::ImageWriter> writer) {
-
-  bool success = true;
-  if (!reader->Close()) {
-    Error(error::kImageCloseError);
-    success = false;
-  }
-
-  if (!writer->Close()) {
-    Error(error::kDeviceCloseError);
-    success = false;
-  }
-  return success;
-}
-
-void Operation::WriteComplete() {
-
-  DVLOG(2) << "Completed write of " << image_path_.value();
+void Operation::WriteComplete(const base::Closure& continuation) {
   SetProgress(kProgressComplete);
-
-  if (verify_write_) {
-    BrowserThread::PostTask(BrowserThread::FILE,
-                            FROM_HERE,
-                            base::Bind(&Operation::VerifyWriteStart, this));
-  } else {
-    BrowserThread::PostTask(BrowserThread::FILE,
-                            FROM_HERE,
-                            base::Bind(&Operation::Finish, this));
-  }
+  continuation.Run();
 }
 
-void Operation::VerifyWriteStart() {
+void Operation::VerifyWrite(const base::Closure& continuation) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   if (IsCancelled()) {
     return;
   }
 
-  DVLOG(1) << "Starting verification stage.";
-
   SetStage(image_writer_api::STAGE_VERIFYWRITE);
 
-  scoped_ptr<base::FilePath> image_path(new base::FilePath(image_path_));
-
-  GetMD5SumOfFile(
-      image_path.Pass(),
-      -1,
-      0, // progress_offset
-      50, // progress_scale
-      base::Bind(&Operation::VerifyWriteStage2,
-                 this));
-}
-
-void Operation::VerifyWriteStage2(
-    scoped_ptr<std::string> image_hash) {
-  DVLOG(1) << "Building MD5 sum of device: " << storage_unit_id_;
-
-  int64 image_size;
-  scoped_ptr<base::FilePath> device_path(new base::FilePath(storage_unit_id_));
-
-  if (!base::GetFileSize(image_path_, &image_size)){
-    Error(error::kImageSizeError);
+  if (!OpenPlatformFiles())

[tbarzic, 2014/02/13 06:58:52]

Is this needed?
I think you should either reset files after write is done (and remove source_ ==
base::kInvalidPlatformFileValue  from OpenPlatformFile) or expect the files to
be opened here (and possibly fail the operation if they are not)

[Drew Haven, 2014/02/13 20:48:10]

So this was to save us closing and reopening the files when we go from the write
to the verify phases.  This way they only get opened once and get closed by the
scoped closer however the operation terminates.  It also shares the code between
the write and verify functions. Granted, I'm not very experienced with using a
scoped closer.

The operation will be "failed" if the files fail to open because
OpenPlatformFiles actually throws the error, but that's not clear the way this
is written.

I'll split the two functions up and it's probably best to close and flush the
files anyway.  See what you think of this new change.

     return;
-  }
 
-  GetMD5SumOfFile(
-      device_path.Pass(),
-      image_size,
-      50, // progress_offset
-      50, // progress_scale
-      base::Bind(&Operation::VerifyWriteCompare,
-                 this,
-                 base::Passed(&image_hash)));
-}
+  int64 total_size;
+  base::GetFileSize(image_path_, &total_size);
 
-void Operation::VerifyWriteCompare(
-    scoped_ptr<std::string> image_hash,
-    scoped_ptr<std::string> device_hash) {
-  DVLOG(1) << "Comparing hashes: " << *image_hash << " vs " << *device_hash;
+  BrowserThread::PostTask(
+      BrowserThread::FILE,
+      FROM_HERE,
+      base::Bind(
+          &Operation::VerifyWriteChunk, this, 0, total_size, continuation));
+}
 
-  if (*image_hash != *device_hash) {
-    Error(error::kVerificationFailed);
+void Operation::VerifyWriteChunk(const int64& bytes_processed,
+                                 const int64& total_size,
+                                 const base::Closure& continuation) {
+  if (IsCancelled()) {
     return;
   }
 
-  DVLOG(2) << "Completed write verification of " << image_path_.value();
+  scoped_ptr<char[]> source_buffer(new char[kBurningBlockSize]);
+  scoped_ptr<char[]> target_buffer(new char[kBurningBlockSize]);
+
+  int64 source_bytes_read = base::ReadPlatformFile(
+      source_, bytes_processed, source_buffer.get(), kBurningBlockSize);
+
+  if (source_bytes_read > 0) {
+    int64 target_bytes_read = base::ReadPlatformFile(
+        target_, bytes_processed, target_buffer.get(), source_bytes_read);
+    if (source_bytes_read == target_bytes_read &&
+        memcmp(source_buffer.get(), target_buffer.get(), source_bytes_read) ==
+            0) {
+      int percent_curr = kProgressComplete *
+                         (bytes_processed + source_bytes_read) / total_size;
+
+      SetProgress(percent_curr);
+
+      BrowserThread::PostTask(BrowserThread::FILE,
+                              FROM_HERE,
+                              base::Bind(&Operation::VerifyWriteChunk,
+                                         this,
+                                         bytes_processed + source_bytes_read,
+                                         total_size,
+                                         continuation));
+    } else {
+      Error(error::kVerificationFailed);
+    }
+  } else if (source_bytes_read == 0) {

[tbarzic, 2014/02/13 06:58:52]

what if target file is not completely read at this point (e.g. if it somehow
ended up being larger)?

[Drew Haven, 2014/02/13 20:48:10]

So, the target file should be larger because it's the file representing the
entire disk, so we only want to verify up to the point where the source ends.

+    VerifyWriteComplete(continuation);
+  } else {  // len < 0
+    Error(error::kImageReadError);
+  }
+}
 
+void Operation::VerifyWriteComplete(const base::Closure& continuation) {
   SetProgress(kProgressComplete);
-
-  Finish();
+  continuation.Run();
 }
 
 }  // namespace image_writer