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 903b495e115c42b0cd90021ee4f50751bc7ab788..4c252e7bd39b08a876a0c04f83489becc1182246 100644
--- a/chrome/browser/component_updater/component_unpacker.h
+++ b/chrome/browser/component_updater/component_unpacker.h
@@ -7,14 +7,40 @@
 
 #include <string>
 #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"
 
 class ComponentInstaller;
 class ComponentPatcher;
 
+namespace component_updater {
+
+// Possible error conditions.
+// Add only to the bottom of this enum; the order must be kept stable.
+enum Error {

[cpu_(ooo_6.6-7.5), 2013/11/20 02:24:52]

what is the advantage of moving this errors out of ComponentUnpacker?, as is in
this CL
component_updater::kInvalidParams does not tell me much, in particular the key
information that is part of unpacking.

[waffles, 2013/11/26 00:46:55]

(Done.) Good point. This was a casualty of this CL's many patchsets.

+  kNone,
+  kInvalidParams,
+  kInvalidFile,
+  kUnzipPathError,
+  kUnzipFailed,
+  kNoManifest,
+  kBadManifest,
+  kBadExtension,
+  kInvalidId,
+  kInstallerError,
+  kIoError,
+  kDeltaVerificationFailure,
+  kDeltaBadCommands,
+  kDeltaUnsupportedCommand,
+  kDeltaOperationFailure,
+  kDeltaPatchProcessFailure,
+  kDeltaMissingExistingFile,
+  kFingerprintWriteFailed,
+};
+
 // Deserializes the CRX manifest. The top level must be a dictionary.
 scoped_ptr<base::DictionaryValue> ReadManifest(
     const base::FilePath& unpack_path);
@@ -24,57 +50,97 @@ 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
+//     \_ Unzip
+//     \_ BeginPatching ---> DifferentialUpdatePatch
+//                             ...
+//   DonePatching <----------- ...
+//     \_ Install
+//     \_ Finish
+//
+// For a full CRX, the flow is:
+//   [ComponentUpdater]
+//   Unpack
+//     \_ Unzip
+//     \_ BeginPatching
+//          |
+//          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.
-  // Add only to the bottom of this enum; the order must be kept stable.
-  enum Error {
-    kNone,
-    kInvalidParams,
-    kInvalidFile,
-    kUnzipPathError,
-    kUnzipFailed,
-    kNoManifest,
-    kBadManifest,
-    kBadExtension,
-    kInvalidId,
-    kInstallerError,
-    kIoError,
-    kDeltaVerificationFailure,
-    kDeltaBadCommands,
-    kDeltaUnsupportedCommand,
-    kDeltaOperationFailure,
-    kDeltaPatchProcessFailure,
-    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

[Sorin Jianu, 2013/11/21 19:48:37]

line is too short.

[waffles, 2013/11/26 00:46:55]

Done.

+  // public key SHA256 hash. |path| is the current location of
+  // the CRX. When done, runs |callback| with the error and extra error code.

[Sorin Jianu, 2013/11/21 19:48:37]

what's done?

[waffles, 2013/11/26 00:46:55]

Done.

   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_; }
+  // Begin the actual unpacking of the files. May invoke a patcher if the

[Sorin Jianu, 2013/11/21 19:48:37]

// Verbs are usually in the 3rd person for function comments.

[waffles, 2013/11/26 00:46:55]

Done.

+  // package is a differential update. Call |callback| with the result.
+  void Unpack(
+      const base::Callback<void(component_updater::Error, int)>& callback);
 
  private:
+  // 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
+  // if an error occurs.
+  bool BeginPatching();
+
+  // When patching is complete, DonePatching is called before moving on to step
+  // three.
+  void DonePatching(component_updater::Error error, int extended_error);
+
+  // The third step is to install the component.
+  void Install();

[Sorin Jianu, 2013/11/21 19:48:37]

This part I've always found it strange, that the unpacker drives the install as
well. It acts as if it were ComponentInstaller actually.

[waffles, 2013/11/26 00:46:55]

I agree. Changing this is outside the scope of this CL.

+
+  // 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();
+
+  // Returns a weak pointer to this object.
+  base::WeakPtr<ComponentUnpacker> GetWeakPtr();
+
+  std::vector<uint8> pk_hash_;
+  base::FilePath path_;
   base::FilePath unpack_path_;
-  Error error_;
-  int extended_error_;  // Provides additional error information.
+  base::FilePath unpack_diff_path_;
+  bool delta_;

[Sorin Jianu, 2013/11/21 19:48:37]

rename to is_delta_ ?

[waffles, 2013/11/26 00:46:55]

Done.

+  std::string fingerprint_;
+  ComponentPatcher* patcher_;
+  ComponentInstaller* installer_;
+  base::Callback<void(Error, int)> callback_;
+  component_updater::Error error_;
+  int extended_error_;
+  base::WeakPtrFactory<ComponentUnpacker> ptr_factory_;
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;

[Sorin Jianu, 2013/11/22 00:09:21]

is SequencedTaskRunner declared anywhere?

[waffles, 2013/11/26 00:46:55]

Done.

 };
 
+}  // namespace component_updater

[Sorin Jianu, 2013/11/21 19:48:37]

needs a line after

[waffles, 2013/11/26 00:46:55]

Done.

 #endif  // CHROME_BROWSER_COMPONENT_UPDATER_COMPONENT_UNPACKER_H_