Significantly cleans up the ImageWriter Operation class and subclasses.

chrome/browser/extensions/api/image_writer_private/operation.h

 // Copyright 2013 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 CHROME_BROWSER_EXTENSIONS_API_IMAGE_WRITER_PRIVATE_OPERATION_H_
 #define CHROME_BROWSER_EXTENSIONS_API_IMAGE_WRITER_PRIVATE_OPERATION_H_
 
 #include "base/callback.h"
+#include "base/files/scoped_platform_file_closer.h"
+#include "base/files/scoped_temp_dir.h"
 #include "base/md5.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/memory/weak_ptr.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "base/timer/timer.h"
-#include "chrome/browser/extensions/api/image_writer_private/image_writer_utils.h"
 #include "chrome/common/extensions/api/image_writer_private.h"
 #include "third_party/zlib/google/zip_reader.h"
 
 namespace image_writer_api = extensions::api::image_writer_private;
 
 namespace base {
 class FilePath;
 }  // namespace base
 
 namespace extensions {
@@ skipping 14 matching lines @@
 // for advancing to the next stage and other UI interaction.  The Run phase does
 // the work on the FILE thread and calls SendProgress or Error as appropriate.
 class Operation : public base::RefCountedThreadSafe<Operation> {
  public:
   typedef base::Callback<void(bool, const std::string&)> StartWriteCallback;
   typedef base::Callback<void(bool, const std::string&)> CancelWriteCallback;
   typedef std::string ExtensionId;
 
   Operation(base::WeakPtr<OperationManager> manager,
             const ExtensionId& extension_id,
-            const std::string& storage_unit_id);
+            const std::string& device_path);
 
   // Starts the operation.
-  virtual void Start() = 0;
+  virtual void Start();
 
   // Cancel the operation. This must be called to clean up internal state and
   // cause the the operation to actually stop.  It will not be destroyed until
   // all callbacks have completed.
   void Cancel();
 
   // Aborts the operation, cancelling it and generating an error.
   void Abort();
 
   // Informational getters.
   int GetProgress();
   image_writer_api::Stage GetStage();
 
  protected:
   virtual ~Operation();
 
+  // Unzips the current file if it ends in ".zip".  The current_file will be set
+  // to the unzipped file.
+  void Unzip(const base::Closure& continuation);
+
+  // Writes the current file to device_path.
+  void Write(const base::Closure& continuation);
+
+  // Verifies that the current file and device_path contents match.
+  void VerifyWrite(const base::Closure& continuation);
+
+  // Completes the operation.
+  void Finish();
+
   // Generates an error.
   // |error_message| is used to create an OnWriteError event which is
   // sent to the extension
   virtual void Error(const std::string& error_message);
 
   // Set |progress_| and send an event.  Progress should be in the interval
   // [0,100]
   void SetProgress(int progress);
   // Change to a new |stage_| and set |progress_| to zero.  Triggers a progress
   // event.
   void SetStage(image_writer_api::Stage stage);
 
   // Can be queried to safely determine if the operation has been cancelled.
   bool IsCancelled();
 
   // Adds a callback that will be called during clean-up, whether the operation
   // is aborted, encounters and error, or finishes successfully.  These
   // functions will be run on the FILE thread.
-  void AddCleanUpFunction(base::Closure);
-
-  void UnzipStart(scoped_ptr<base::FilePath> zip_file);
-  void WriteStart();
-  void VerifyWriteStart();
-  void Finish();
+  void AddCleanUpFunction(const base::Closure& callback);
 
   // If |file_size| is non-zero, only |file_size| bytes will be read from file,
   // otherwise the entire file will be read.
   // |progress_scale| is a percentage to which the progress will be scale, e.g.
   // a scale of 50 means it will increment from 0 to 50 over the course of the
   // sum.  |progress_offset| is an percentage that will be added to the progress
   // of the MD5 sum before updating |progress_| but after scaling.
   void GetMD5SumOfFile(
-      scoped_ptr<base::FilePath> file,
+      const base::FilePath file,
       int64 file_size,
       int progress_offset,
       int progress_scale,
-      const base::Callback<void(scoped_ptr<std::string>)>& callback);
+      const base::Callback<void(const std::string&)>& callback);
 
   base::WeakPtr<OperationManager> manager_;
   const ExtensionId extension_id_;
 
   base::FilePath image_path_;
-  const std::string storage_unit_id_;
+  base::FilePath device_path_;
 
-  // Whether or not to run the final verification step.
-  bool verify_write_;
+  // Temporary directory to store files as we go.
+  base::ScopedTempDir temp_dir_;
 
  private:
   friend class base::RefCountedThreadSafe<Operation>;
 
   // TODO(haven): Clean up these switches. http://crbug.com/292956
 #if defined(OS_LINUX) && !defined(CHROMEOS)
+  bool OpenPlatformFiles();
+
   void WriteRun();
-  void WriteChunk(scoped_ptr<image_writer_utils::ImageReader> reader,
-                  scoped_ptr<image_writer_utils::ImageWriter> writer,
-                  int64 bytes_written);
-  bool WriteCleanUp(scoped_ptr<image_writer_utils::ImageReader> reader,
-                    scoped_ptr<image_writer_utils::ImageWriter> writer);
-  void WriteComplete();
+  void WriteChunk(const int64& bytes_written,
+                  const int64& total_size,
+                  const base::Closure& continuation);
+  void WriteComplete(const base::Closure& continuation);
 
-  void VerifyWriteStage2(scoped_ptr<std::string> image_hash);
-  void VerifyWriteCompare(scoped_ptr<std::string> image_hash,
-                          scoped_ptr<std::string> device_hash);
+  void VerifyWriteChunk(const int64& bytes_written,
+                        const int64& total_size,
+                        const base::Closure& continuation);
+  void VerifyWriteComplete(const base::Closure& continuation);
+
+  base::PlatformFile source_;
+  base::ScopedPlatformFileCloser source_closer_;

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

will operation that uses this always be deleted on FILE/worker thread

though, it may be better to explicitly Close files when writing is done than
having the closer inn the operation scope.
You could also create a class {base::PlatformFile;
base::ScopedPlatformFileCloser} to pass to WriteChunk tasks?

Hm, I'm fine with whatever you prefer..

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

I tried something new, passing around the ScopedPlatformFileClosers and
referencing the file from them.  Also, this way they'll always go out of scope
in one of those functions that are always on the FILE thread.

I was wondering earlier if there was an easier way to do my thread boundries so
that I could avoid these scoped_refptrs and other threading issues.  But I'm
leaving that for another CL as nothing jumped out at me as easy to implement.

+  base::PlatformFile target_;
+  base::ScopedPlatformFileCloser target_closer_;
 #endif
 
 #if defined(OS_CHROMEOS)
-  void StartWriteOnUIThread();
+  void StartWriteOnUIThread(const base::Closure& continuation);
 
-  void OnBurnFinished(const std::string& target_path,
+  void OnBurnFinished(const base::Closure& continuation,
+                      const std::string& target_path,
                       bool success,
                       const std::string& error);
   void OnBurnProgress(const std::string& target_path,
                       int64 num_bytes_burnt,
                       int64 total_size);
   void OnBurnError();
 #endif
 
   // Incrementally calculates the MD5 sum of a file.
-  void MD5Chunk(scoped_ptr<image_writer_utils::ImageReader> reader,
+  void MD5Chunk(base::PlatformFile file,
                 int64 bytes_processed,
                 int64 bytes_total,
                 int progress_offset,
                 int progress_scale,
-                const base::Callback<void(scoped_ptr<std::string>)>& callback);
+                const base::Callback<void(const std::string&)>& callback);
 
   // Callbacks for zip::ZipReader.
-  void OnUnzipSuccess();
+  void OnUnzipSuccess(const base::Closure& continuation);
   void OnUnzipFailure();
   void OnUnzipProgress(int64 total_bytes, int64 progress_bytes);
 
   // Runs all cleanup functions.
   void CleanUp();
 
   // |stage_| and |progress_| are owned by the FILE thread, use |SetStage| and
   // |SetProgress| to update.  Progress should be in the interval [0,100]
   image_writer_api::Stage stage_;
   int progress_;
 
   // MD5 contexts don't play well with smart pointers.  Just going to allocate
   // memory here.  This requires that we only do one MD5 sum at a time.
   base::MD5Context md5_context_;
 
   // Zip reader for unzip operations.
   zip::ZipReader zip_reader_;
 
   // CleanUp operations that must be run.  All these functions are run on the
   // FILE thread.
   std::vector<base::Closure> cleanup_functions_;
 };
 
 }  // namespace image_writer
 }  // namespace extensions
 
 #endif  // CHROME_BROWSER_EXTENSIONS_API_IMAGE_WRITER_PRIVATE_OPERATION_H_