Support asynchronous patching operations in the component updater.

chrome/browser/component_updater/component_unpacker.h

Index: chrome/browser/component_updater/component_unpacker.h
diff --git a/chrome/browser/component_updater/component_unpacker.h b/chrome/browser/component_updater/component_unpacker.h
index c4dd8995710344b7d4d93884ff34ab5bb13a9e8e..42ccd1800e258834b432fdbabe1ce4a5ea60acd2 100644
--- a/chrome/browser/component_updater/component_unpacker.h
+++ b/chrome/browser/component_updater/component_unpacker.h
@@ -9,9 +9,12 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/json/json_file_value_serializer.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/sequenced_task_runner.h"
 
 namespace component_updater {
 
@@ -27,13 +30,39 @@ scoped_ptr<base::DictionaryValue> ReadManifest(
 // error the component specific installer will be invoked to proceed with
 // the component installation or update.
 //
-// This class should be used only by the component updater. It is inspired
+// This class should be used only by the component updater. It is inspired by
 // and overlaps with code in the extension's SandboxedUnpacker.
 // The main differences are:
 // - The public key hash is full SHA256.
 // - Does not use a sandboxed unpacker. A valid component is fully trusted.
 // - The manifest can have different attributes and resources are not
 //   transcoded.
+//
+// If the CRX is a delta CRX, the flow is:
+//   [ComponentUpdater]      [ComponentPatcher]
+//   Unpack
+//     \_ Verify
+//     \_ Unzip
+//     \_ BeginPatching ---> DifferentialUpdatePatch
+//                             ...
+//   DonePatching <----------- ...
+//     \_ Install
+//     \_ Finish
+//
+// For a full CRX, the flow is:
+//   [ComponentUpdater]
+//   Unpack
+//     \_ Verify
+//     \_ Unzip
+//     \_ BeginPatching

[Sorin Jianu, 2014/01/25 02:30:21]

could we do the same outflow as above, and say that the it's a nop instead of
DifferentialUpdatePatch ?

[waffles, 2014/01/28 21:06:31]

Discussed, we decided not to change it.

+//          |
+//          V
+//   DonePatching
+//     \_ Install
+//     \_ Finish
+//
+// In both cases, if there is an error at any point, the remaining steps will
+// be skipped and Finish will be called.
 class ComponentUnpacker {
  public:
   // Possible error conditions.
@@ -58,26 +87,66 @@ class ComponentUnpacker {
     kDeltaMissingExistingFile,
     kFingerprintWriteFailed,
   };
-  // Unpacks, verifies and calls the installer. |pk_hash| is the expected
-  // public key SHA256 hash. |path| is the current location of the CRX.
+
+  // Constructs an unpacker for a specific component unpacking operation.
+  // |pk_hash| is the expected/ public key SHA256 hash. |path| is the current
+  // location of the CRX.
   ComponentUnpacker(const std::vector<uint8>& pk_hash,
                     const base::FilePath& path,
                     const std::string& fingerprint,
                     ComponentPatcher* patcher,
-                    ComponentInstaller* installer);
-
-  // If something went wrong during unpacking or installer invocation, the
-  // destructor will delete the unpacked CRX files.
-  ~ComponentUnpacker();
+                    ComponentInstaller* installer,
+                    scoped_refptr<base::SequencedTaskRunner> task_runner);
 
-  Error error() const { return error_; }
+  virtual ~ComponentUnpacker();
 
-  int extended_error() const { return extended_error_; }
+  // Begins the actual unpacking of the files. May invoke a patcher if the
+  // package is a differential update. Call |callback| with the result.
+  void Unpack(
+      const base::Callback<void(Error, int)>& callback);
 
  private:
+  // Before unpacking, verify that the file is well-formed and correctly signed.

[Sorin Jianu, 2014/01/25 02:30:21]

We could say that verification is the first step, unzip is the second, etc.

[waffles, 2014/01/28 21:06:31]

Done.

+  bool Verify();
+
+  // The first step of unpacking is to unzip. Returns false if an error
+  // occurred as part of unzipping.
+  bool Unzip();
+
+  // The second step is to optionally patch files - this is a no-op for
+  // full (non-differential) updates. This step is asynchronous. Returns false

[Sorin Jianu, 2014/01/25 02:30:21]

I am thinking we can name it BeginPatchingAsync to self document it.

[waffles, 2014/01/28 21:06:31]

I'm not sure I like that idea so much, unless we start labeling all the async
calls with Async; we should be consistent.

Additionally, there is no BeginPatchingSync.

The fact that there is a Begin/Done for this function is supposed to cue the
reader that it is asynchronous. I explicitly call this out in the comment just
to be sure.

+  // if an error occurs.
+  bool BeginPatching();
+
+  // When patching is complete, DonePatching is called before moving on to step
+  // three.
+  void DonePatching(Error error, int extended_error);
+
+  // The third step is to install the component.
+  void Install();
+
+  // The final step is to do clean-up for things that can't be tidied as we go.
+  // If there is an error at any step, the remaining steps are skipped and
+  // and Finish is called.
+  // Finish is responsible for calling the callback provided in Start().
+  void Finish();

[Sorin Jianu, 2014/01/25 02:30:21]

Maybe call this function DoneUnpacking?

So far I've been using the terminology BeginFoo/EndFoo as opposed to
BeginFoo/DoneFoo. Just an idea.

[waffles, 2014/01/28 21:06:31]

I thought about this. There is already ComponentUpdateService::DoneUnpacking
which is passed as the callback, would Unpack() would become BeginUnpacking()?
Let's discuss.
Done. I have a slight preference for Done instead of End (since End is also a
verb), but it's best to be consistent.

+
+  // Returns a weak pointer to this object.
+  base::WeakPtr<ComponentUnpacker> GetWeakPtr();
+
+  std::vector<uint8> pk_hash_;
+  base::FilePath path_;
   base::FilePath unpack_path_;
+  base::FilePath unpack_diff_path_;
+  bool is_delta_;
+  std::string fingerprint_;
+  ComponentPatcher* patcher_;
+  ComponentInstaller* installer_;
+  base::Callback<void(Error, int)> callback_;
   Error error_;
-  int extended_error_;  // Provides additional error information.
+  int extended_error_;
+  base::WeakPtrFactory<ComponentUnpacker> ptr_factory_;
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;

[Sorin Jianu, 2014/01/25 02:30:21]

disallow copy and assign?

[waffles, 2014/01/28 21:06:31]

Done.

 };
 
 }  // namespace component_updater