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..8918e1a2308e31b15e1555a0b4fc7cd7508f052e 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,188 @@ using content::BrowserThread;
 
 const int kBurningBlockSize = 8 * 1024;  // 8 KiB
 
-void Operation::WriteStart() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  if (IsCancelled()) {
-    return;
+base::ScopedPlatformFileCloser Operation::OpenFile(
+    const base::FilePath& path,
+    const base::PlatformFileFlags mode) {
+  base::PlatformFileError result;
+  base::ScopedPlatformFileCloser file_closer(new base::PlatformFile());

[tbarzic, 2014/02/13 21:32:39]

will the created PlatformFile get deleted with |file_closer|, or will it just be
closed?

[Drew Haven, 2014/02/13 23:30:01]

Yeah, this doesn't match the semantics of scoped_ptr.  I'm going to avoid using
the ScopedPlatformFileCloser.  There's a cleaner implementation without it, I
think.

[tbarzic, 2014/02/13 23:42:46]

Yep, totally  agree :)

+
+  *file_closer = base::CreatePlatformFile(
+      path, base::PLATFORM_FILE_OPEN | mode, NULL, &result);
+
+  if (result != base::PLATFORM_FILE_OK) {
+    return base::ScopedPlatformFileCloser();
   }
 
-  if (image_path_.empty()) {
-    Error(error::kImageNotFound);
+  return file_closer.Pass();
+}
+
+void Operation::Write(const base::Closure& continuation) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  if (IsCancelled()) {
     return;
   }
 
-  DVLOG(1) << "Starting write of " << image_path_.value()
-           << " to " << storage_unit_id_;
-
   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 {
+  base::ScopedPlatformFileCloser source =
+      OpenFile(image_path_, base::PLATFORM_FILE_READ);
+  if (!source) {
     Error(error::kImageOpenError);
     return;
   }
 
-  BrowserThread::PostTask(
-    BrowserThread::FILE,
-    FROM_HERE,
-    base::Bind(&Operation::WriteChunk,
-               this,
-               base::Passed(&reader),
-               base::Passed(&writer),
-               0));
+  base::ScopedPlatformFileCloser target =
+      OpenFile(device_path_, base::PLATFORM_FILE_WRITE);
+  if (!target) {
+    Error(error::kImageOpenError);
+    return;
+  }
+
+  int64 total_size;
+  base::GetFileSize(image_path_, &total_size);
+
+  BrowserThread::PostTask(BrowserThread::FILE,
+                          FROM_HERE,
+                          base::Bind(&Operation::WriteChunk,
+                                     this,
+                                     base::Passed(&source),
+                                     base::Passed(&target),
+                                     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(base::ScopedPlatformFileCloser source,
+                           base::ScopedPlatformFileCloser target,
+                           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,
+                                         base::Passed(&source),
+                                         base::Passed(&target),
+                                         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_));
+  base::ScopedPlatformFileCloser image =
+      OpenFile(image_path_, base::PLATFORM_FILE_READ);
+  if (!image) {
+    Error(error::kImageOpenError);
+    return;
+  }
 
-  if (!base::GetFileSize(image_path_, &image_size)){
-    Error(error::kImageSizeError);
+  base::ScopedPlatformFileCloser device =
+      OpenFile(device_path_, base::PLATFORM_FILE_READ);
+  if (!device) {
+    Error(error::kDeviceOpenError);
     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);
+
+  BrowserThread::PostTask(BrowserThread::FILE,
+                          FROM_HERE,
+                          base::Bind(&Operation::VerifyWriteChunk,
+                                     this,
+                                     base::Passed(&image),
+                                     base::Passed(&device),
+                                     0,
+                                     total_size,
+                                     continuation));
 }
 
-void Operation::VerifyWriteCompare(
-    scoped_ptr<std::string> image_hash,
-    scoped_ptr<std::string> device_hash) {
-  DVLOG(1) << "Comparing hashes: " << *image_hash << " vs " << *device_hash;
-
-  if (*image_hash != *device_hash) {
-    Error(error::kVerificationFailed);
+void Operation::VerifyWriteChunk(base::ScopedPlatformFileCloser image,
+                                 base::ScopedPlatformFileCloser device,
+                                 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(
+      *image, bytes_processed, source_buffer.get(), kBurningBlockSize);
+
+  if (source_bytes_read > 0) {
+    int64 target_bytes_read = base::ReadPlatformFile(
+        *device, 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,
+                                         base::Passed(&image),
+                                         base::Passed(&device),
+                                         bytes_processed + source_bytes_read,
+                                         total_size,
+                                         continuation));
+    } else {
+      Error(error::kVerificationFailed);
+    }
+  } else if (source_bytes_read == 0) {
+    VerifyWriteComplete(continuation);
+  } else {  // len < 0
+    Error(error::kImageReadError);
+  }
+}
 
+void Operation::VerifyWriteComplete(const base::Closure& continuation) {
   SetProgress(kProgressComplete);
-
-  Finish();
+  continuation.Run();
 }
 
 }  // namespace image_writer