Use UtilityProcessHost to patch files.

chrome/browser/component_updater/component_patcher_operation.h

Index: chrome/browser/component_updater/component_patcher_operation.h
diff --git a/chrome/browser/component_updater/component_patcher_operation.h b/chrome/browser/component_updater/component_patcher_operation.h
index c9bc0b1690c3d62ccfb76499b3ddde8cf43ecf11..5153e604f56dc3d1dd168f4a685337b978706211 100644
--- a/chrome/browser/component_updater/component_patcher_operation.h
+++ b/chrome/browser/component_updater/component_patcher_operation.h
@@ -7,9 +7,13 @@
 
 #include <string>

[Sorin Jianu, 2014/02/03 20:57:57]

needs one line after the lib include.

[waffles, 2014/02/07 01:00:59]

Done.

 #include "base/basictypes.h"
+#include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/files/file_path.h"
+#include "base/memory/weak_ptr.h"
+#include "chrome/browser/component_updater/component_patcher.h"
 #include "chrome/browser/component_updater/component_unpacker.h"
+#include "content/public/browser/utility_process_host_client.h"
 
 namespace base {
 class DictionaryValue;
@@ -18,27 +22,28 @@ class DictionaryValue;
 namespace component_updater {
 
 class ComponentInstaller;
-class ComponentPatcher;
 
 class DeltaUpdateOp {
  public:
-
   DeltaUpdateOp();
   virtual ~DeltaUpdateOp();
 
-  // Parses, runs, and verifies the operation, returning an error code if an
-  // error is encountered, and DELTA_OK otherwise. In case of errors,
-  // extended error information can be returned in the |error| parameter.
-  ComponentUnpacker::Error Run(base::DictionaryValue* command_args,
-                               const base::FilePath& input_dir,
-                               const base::FilePath& unpack_dir,
-                               ComponentPatcher* patcher,
-                               ComponentInstaller* installer,
-                               int* error);
+  // Parses, runs, and verifies the operation. Calls |callback| with the
+  // result of the operateration. The callback is called using |task_runner|.

[Sorin Jianu, 2014/02/03 20:57:57]

operateration

[waffles, 2014/02/07 01:00:59]

Done. That's a fun word to say.

+  void Run(base::DictionaryValue* command_args,
+           const base::FilePath& input_dir,
+           const base::FilePath& unpack_dir,
+           ComponentInstaller* installer,
+           bool in_process,
+           const base::Callback<void(ComponentUnpacker::Error, int)>& callback,
+           scoped_refptr<base::SequencedTaskRunner> task_runner);
 
  protected:
+  base::WeakPtr<DeltaUpdateOp> GetWeakPtr();
+
   std::string output_sha256_;
   base::FilePath output_abs_path_;
+  bool in_process_;

[Sorin Jianu, 2014/02/03 20:57:57]

most likely, we want in_process_ to be immutable in the derived classes. Make it
private in the base class and provide read-only accessor? Not sure about the
remaining protected members.

In general, protected data members are evil.

[waffles, 2014/02/07 01:00:59]

Done. I'm a little resistant to making the remaining members accessible by
function. I like the symmetry if output_abs_path and input_abs_path and so
forth.

[Sorin Jianu, 2014/02/27 20:53:57]

Well, a nice symmetry would be that all of them would be private.

 
  private:
   ComponentUnpacker::Error CheckHash();
@@ -52,10 +57,19 @@ class DeltaUpdateOp {
       ComponentInstaller* installer) = 0;
 
   // Subclasses must override DoRun to actually perform the patching operation.
-  // DoRun returns DELTA_OK on success; any other code represents failure.
-  // Additional error information can be returned in the |error| parameter.
-  virtual ComponentUnpacker::Error DoRun(ComponentPatcher* patcher,
-                                         int* error) = 0;
+  // They must call the provided callback when they have completed their
+  // operations.
+  virtual void DoRun(
+      const base::Callback<void(ComponentUnpacker::Error, int)>& callback) = 0;

[Sorin Jianu, 2014/02/03 20:57:57]

I wonder if you could find a way to indicate in a stronger way that this
callback here is different than the callback parameter of the ctor, and the
callback_ member of this class. 

[waffles, 2014/02/07 01:00:59]

I've added a comment, but I'm not sure these details are relevant to the
subclass so it seems strange to say it here.

+
+  // Callback given to subclasses for when they complete their operation.
+  // Validates the output, and posts a task to the patching operation's
+  // callback.
+  void DoneRunning(ComponentUnpacker::Error error, int extended_error);
+
+  base::Callback<void(ComponentUnpacker::Error, int)> callback_;
+  base::WeakPtrFactory<DeltaUpdateOp> ptr_factory_;
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(DeltaUpdateOp);
 };
@@ -74,8 +88,9 @@ class DeltaUpdateOpCopy : public DeltaUpdateOp {
       const base::FilePath& input_dir,
       ComponentInstaller* installer) OVERRIDE;
 
-  virtual ComponentUnpacker::Error DoRun(ComponentPatcher* patcher,
-                                         int* error) OVERRIDE;
+  virtual void DoRun(
+      const base::Callback<void(ComponentUnpacker::Error, int)>& callback)

[Sorin Jianu, 2014/02/03 20:57:57]

maybe typedef the callback type to make the lines shorter in many places?

[waffles, 2014/02/07 01:00:59]

Done. I've never been a fan of typedefs in C++.

[Sorin Jianu, 2014/02/27 20:53:57]

Agreed, but what is the option? The callback library itself is full of typedefs.
It's a useful language feature in many cases.

+      OVERRIDE;
 
   base::FilePath input_abs_path_;
 
@@ -97,43 +112,44 @@ class DeltaUpdateOpCreate : public DeltaUpdateOp {
       const base::FilePath& input_dir,
       ComponentInstaller* installer) OVERRIDE;
 
-  virtual ComponentUnpacker::Error DoRun(ComponentPatcher* patcher,
-                                         int* error) OVERRIDE;
+  virtual void DoRun(
+      const base::Callback<void(ComponentUnpacker::Error, int)>& callback)
+      OVERRIDE;
 
   base::FilePath patch_abs_path_;
 
   DISALLOW_COPY_AND_ASSIGN(DeltaUpdateOpCreate);
 };
 
-// A 'bsdiff' operation takes an existing file on disk, and a bsdiff-
-// format patch file provided in the delta update package, and runs bsdiff
-// to construct an output file in the unpacking directory.
-class DeltaUpdateOpPatchBsdiff : public DeltaUpdateOp {
+// Both 'bsdiff' and 'courgette' operations take an existing file on disk,
+// and a bsdiff- or Courgette-format patch file provided in the delta update
+// package, and run bsdiff or Courgette to construct an output file in the
+// unpacking directory.
+class DeltaUpdateOpPatch : public DeltaUpdateOp {
  public:
-  DeltaUpdateOpPatchBsdiff();
+  explicit DeltaUpdateOpPatch(component_updater::PatchType patch_type);
 
- private:
-  // Overrides of DeltaUpdateOp.
-  virtual ComponentUnpacker::Error DoParseArguments(
-      base::DictionaryValue* command_args,
-      const base::FilePath& input_dir,
-      ComponentInstaller* installer) OVERRIDE;
+  virtual ~DeltaUpdateOpPatch();
 
-  virtual ComponentUnpacker::Error DoRun(ComponentPatcher* patcher,
-                                         int* error) OVERRIDE;
+  void DonePatching(bool success, int error_code);
 
-  base::FilePath patch_abs_path_;
-  base::FilePath input_abs_path_;
+  // An inner class is used so that DeltaUpdateOpPatch does not need to derive
+  // from UtilityProcessHostClient (because it is refcounted).
+  class PatcherBridge : public content::UtilityProcessHostClient {
+   public:
+    explicit PatcherBridge(DeltaUpdateOpPatch* op);
+    virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
+    virtual void OnProcessCrashed(int exit_code) OVERRIDE;
 
-  DISALLOW_COPY_AND_ASSIGN(DeltaUpdateOpPatchBsdiff);
-};
+   private:
+    virtual ~PatcherBridge();
+    void OnPatchSucceeded();
+    void OnPatchFailed(int error_code);
+    DeltaUpdateOpPatch* op_;
+    DISALLOW_COPY_AND_ASSIGN(PatcherBridge);
+  };
 
-// A 'courgette' operation takes an existing file on disk, and a Courgette-
-// format patch file provided in the delta update package, and runs Courgette
-// to construct an output file in the unpacking directory.
-class DeltaUpdateOpPatchCourgette : public DeltaUpdateOp {
- public:
-  DeltaUpdateOpPatchCourgette();
+  base::Callback<void(ComponentUnpacker::Error, int)>callback_;

[Sorin Jianu, 2014/02/03 20:57:57]

callback_ is declared as a public member.

[Sorin Jianu, 2014/02/03 20:57:57]

one space between the type and variable name?

[waffles, 2014/02/07 01:00:59]

Done.

[waffles, 2014/02/07 01:00:59]

Done.

 
  private:
   // Overrides of DeltaUpdateOp.
@@ -142,13 +158,18 @@ class DeltaUpdateOpPatchCourgette : public DeltaUpdateOp {
       const base::FilePath& input_dir,
       ComponentInstaller* installer) OVERRIDE;
 
-  virtual ComponentUnpacker::Error DoRun(ComponentPatcher* patcher,
-                                         int* error) OVERRIDE;
+  virtual void DoRun(
+      const base::Callback<void(ComponentUnpacker::Error, int)>& callback)
+      OVERRIDE;
+
+  void StartProcess();
 
   base::FilePath patch_abs_path_;
   base::FilePath input_abs_path_;
+  const component_updater::PatchType patch_type_;
+  scoped_refptr<PatcherBridge> bridge_;
 
-  DISALLOW_COPY_AND_ASSIGN(DeltaUpdateOpPatchCourgette);
+  DISALLOW_COPY_AND_ASSIGN(DeltaUpdateOpPatch);
 };
 
 // Factory function to create DeltaUpdateOp instances.