Support asynchronous patching operations in the component updater.

Support asynchronous patching operations in the component updater.

First part of a 2-part changelist.
The second part is https://codereview.chromium.org/25909005/

BUG=304879
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248144

[waffles, 2013-10-07 17:52:07]

Hi Sorin, please take a look at these two CLs for cross-platform patching.

This CL introduces callbacks into the flow, which is necessary if we want to
make the patching operation asynchronous.

[waffles, 2013-10-24 00:11:06]

+Carlos

[cpu_(ooo_6.6-7.5), 2013-10-25 23:31:35]

waffles, can you flesh out the bug with the approach we are taking, e.g using
the utility process, etc?

[cpu_(ooo_6.6-7.5), 2013-10-25 23:59:26]

maybe I am looking at the CL before is ready to be looked at?

https://codereview.chromium.org/25883006/diff/6001/chrome/browser/component_u...
File chrome/browser/component_updater/component_unpacker.cc (right):

https://codereview.chromium.org/25883006/diff/6001/chrome/browser/component_u...
chrome/browser/component_updater/component_unpacker.cc:121: //   transcoded.
can we get a bit of an ascii flow chart here? to me the flow got harder to
understand with all the new helpers

https://codereview.chromium.org/25883006/diff/6001/chrome/browser/component_u...
chrome/browser/component_updater/component_unpacker.cc:262:
base::DeleteFile(unpack_diff_path_, true);
don't get the logic of lines 261-262, deleting with empty pack?

https://codereview.chromium.org/25883006/diff/6001/chrome/browser/component_u...
chrome/browser/component_updater/component_unpacker.cc:264:
base::DeleteFile(unpack_path_, true);
same

https://codereview.chromium.org/25883006/diff/6001/chrome/browser/component_u...
chrome/browser/component_updater/component_unpacker.cc:317: }
we are in the file thread, aren't we?

[waffles, 2013-10-28 22:13:22]

Thanks for the comments, Sorin, Carlos; please take another look.

Sorin:
I've tried out explicit ownership of the ComponentUnpacker in the CUS. I'm not
super-satisfied with the way it turned out; in fact it has a few surprising
gotchas: e.g. CrxUpdateService::DoneUnpacking can't reset the unpacker until it
posts the delayed task, because the unpacker has the last remaining reference to
the callback that holds the arguments of DoneUnpacking. (So I suppose
DoneUnpacking should take params by copy, not reference.)

On the other hand, it did help get rid of a few of those awkward callbacks. Let
me know what you think.

https://codereview.chromium.org/25883006/diff/6001/chrome/browser/component_u...
File chrome/browser/component_updater/component_unpacker.cc (right):

https://codereview.chromium.org/25883006/diff/6001/chrome/browser/component_u...
chrome/browser/component_updater/component_unpacker.cc:121: //   transcoded.
On 2013/10/25 23:59:26, cpu wrote:
> can we get a bit of an ascii flow chart here? to me the flow got harder to
> understand with all the new helpers

Done.

https://codereview.chromium.org/25883006/diff/6001/chrome/browser/component_u...
chrome/browser/component_updater/component_unpacker.cc:262:
base::DeleteFile(unpack_diff_path_, true);
On 2013/10/25 23:59:26, cpu wrote:
> don't get the logic of lines 261-262, deleting with empty pack?

Bug; thanks.

https://codereview.chromium.org/25883006/diff/6001/chrome/browser/component_u...
chrome/browser/component_updater/component_unpacker.cc:264:
base::DeleteFile(unpack_path_, true);
On 2013/10/25 23:59:26, cpu wrote:
> same

Done.

https://codereview.chromium.org/25883006/diff/6001/chrome/browser/component_u...
chrome/browser/component_updater/component_unpacker.cc:317: }
On 2013/10/25 23:59:26, cpu wrote:
> we are in the file thread, aren't we?

This is something I'm struggling with in this code.
I want to guarantee that Finish() is called exactly once. This is my fear about
calling DifferentialUpdatePatch directly:

DifferentialUpdatePatch gets an error and calls the provided callback directly,
supplying the error. DonePatching detects the error, calls Finish. Eventually,
Finish returns, DonePatching returns, DifferentialUpdatePatch returns, and then
BeginPatching returns; since we had an error, we detect it, and call Finish().

I don't like that BeginPatching returns /after/ DonePatching; so I was using a
posted task (to the same thread) to guarantee that it doesn't (see
SequentialTaskRunner).

But, since this won't really work if we move to the blocking thread pool, maybe
I should just give up and make Finish idempotent? That seems hard to guarantee
without using a mutex. Another alternative is to track the failure location in
stack frames; this is what I have done with the next patch set.

[cpu_(ooo_6.6-7.5), 2013-10-28 22:22:50]

Thanks that last explanation was extremely helpful.

We do use PostTask for that purpose sometimes. A post task solution is preferred
to any mutex solution all other things being the same.

[waffles, 2013-11-12 18:07:29]

Hi Sorin, Carlos; please take another look at this first part of the two-part CL
to support using UtilityProcessHost to patch components.

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

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
File chrome/browser/component_updater/component_unpacker.h (right):

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.h:23: enum Error {
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.

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

Thank you Josh!

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher.h (right):

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:54: virtual
component_updater::Error Patch(PatchType patch_type,
it would be nice to define what we can inside the component_updater namespace
and reduce some of this syntactic clutter.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
File chrome/browser/component_updater/component_unpacker.cc (right):

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:145: // Else,
DonePatching will be called asynchronously.
Consider rewriting so the comment does not look like code. It is sufficient to
make this a function comment imho.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:150: error_ =
component_updater::kInvalidParams;
do we want to handle pk_hash_.empty() here or let the code at 176 handle it?

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:170: uint8 hash[32];
not initialized.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:179: base::FilePath*
destination;
not initialized.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:180: if (delta_)
can use ?:

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:183: destination =
&unpack_path_;
Better use a reference and pass the address of that reference to the function,
to avoid a naked pointer in the code.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:189: file.Close(); //
TODO(waffles): Is it safe to close the file? It could change.
TODO is not clear

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:272: return
ptr_factory_.GetWeakPtr();
why do we call this a ptr_factory_ ?

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
File chrome/browser/component_updater/component_unpacker.h (right):

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.h:87: // |pk_hash| is the
expected
line is too short.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.h:89: // the CRX. When done,
runs |callback| with the error and extra error code.
what's done?

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.h:99: // Begin the actual
unpacking of the files. May invoke a patcher if the
// Verbs are usually in the 3rd person for function comments.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.h:119: void Install();
This part I've always found it strange, that the unpacker drives the install as
well. It acts as if it were ComponentInstaller actually.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.h:134: bool delta_;
rename to is_delta_ ?

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.h:145: }  // namespace
component_updater
needs a line after

[Sorin Jianu, 2013-11-22 00:09:20]

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
File chrome/browser/component_updater/component_unpacker.h (right):

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.h:142:
scoped_refptr<base::SequencedTaskRunner> task_runner_;
is SequencedTaskRunner declared anywhere?

[waffles, 2013-11-26 00:46:54]

Thanks! PTAL.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher.h (right):

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:54: virtual
component_updater::Error Patch(PatchType patch_type,
On 2013/11/21 19:48:37, Sorin Jianu wrote:
> it would be nice to define what we can inside the component_updater namespace
> and reduce some of this syntactic clutter.

Done.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
File chrome/browser/component_updater/component_unpacker.cc (right):

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:145: // Else,
DonePatching will be called asynchronously.
On 2013/11/21 19:48:37, Sorin Jianu wrote:
> Consider rewriting so the comment does not look like code. It is sufficient to
> make this a function comment imho.

Done.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:150: error_ =
component_updater::kInvalidParams;
On 2013/11/21 19:48:37, Sorin Jianu wrote:
> do we want to handle pk_hash_.empty() here or let the code at 176 handle it?

I preferred this, since pk_hash_ is learned from our parameters (e.g. Chrome
code), and therefore is "InvalidParams", while at 176 it could be that the hash
in the CRX file is wrong.

This is splitting error codes very finely, I know.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:170: uint8 hash[32];
On 2013/11/21 19:48:37, Sorin Jianu wrote:
> not initialized.

Done.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:179: base::FilePath*
destination;
On 2013/11/21 19:48:37, Sorin Jianu wrote:
> not initialized.

Done.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:180: if (delta_)
On 2013/11/21 19:48:37, Sorin Jianu wrote:
> can use ?:

Done.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:183: destination =
&unpack_path_;
On 2013/11/21 19:48:37, Sorin Jianu wrote:
> Better use a reference and pass the address of that reference to the function,
> to avoid a naked pointer in the code.

Done.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:189: file.Close(); //
TODO(waffles): Is it safe to close the file? It could change.
On 2013/11/21 19:48:37, Sorin Jianu wrote:
> TODO is not clear

Removed it. It's not clear that there can be a solution here. TODO reasoning:
Unzip() can fail on some platforms if the file is open, so we must close it. But
if we close it, it may be replaced before we Unzip it.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:272: return
ptr_factory_.GetWeakPtr();
On 2013/11/21 19:48:37, Sorin Jianu wrote:
> why do we call this a ptr_factory_ ?

Because it is a base::WeakPtrFactory<ComponentUnpacker>.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
File chrome/browser/component_updater/component_unpacker.h (right):

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.h:23: enum Error {
On 2013/11/20 02:24:52, cpu wrote:
> 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.
> 

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

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.h:87: // |pk_hash| is the
expected
On 2013/11/21 19:48:37, Sorin Jianu wrote:
> line is too short.

Done.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.h:89: // the CRX. When done,
runs |callback| with the error and extra error code.
On 2013/11/21 19:48:37, Sorin Jianu wrote:
> what's done?

Done.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.h:99: // Begin the actual
unpacking of the files. May invoke a patcher if the
On 2013/11/21 19:48:37, Sorin Jianu wrote:
> // Verbs are usually in the 3rd person for function comments.

Done.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.h:119: void Install();
On 2013/11/21 19:48:37, Sorin Jianu wrote:
> This part I've always found it strange, that the unpacker drives the install
as
> well. It acts as if it were ComponentInstaller actually.

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

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.h:134: bool delta_;
On 2013/11/21 19:48:37, Sorin Jianu wrote:
> rename to is_delta_ ?

Done.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.h:142:
scoped_refptr<base::SequencedTaskRunner> task_runner_;
On 2013/11/22 00:09:21, Sorin Jianu wrote:
> is SequencedTaskRunner declared anywhere?

Done.

https://codereview.chromium.org/25883006/diff/296001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.h:145: }  // namespace
component_updater
On 2013/11/21 19:48:37, Sorin Jianu wrote:
> needs a line after

Done.

[Sorin Jianu, 2013-11-27 00:34:00]

lgtm

Thank you Josh!

https://codereview.chromium.org/25883006/diff/476001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher_operation.h (right):

https://codereview.chromium.org/25883006/diff/476001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:35:
component_updater::ComponentUnpacker::Error Run(
namespace needed here and below?

https://codereview.chromium.org/25883006/diff/476001/chrome/browser/component...
File chrome/browser/component_updater/component_unpacker.cc (right):

https://codereview.chromium.org/25883006/diff/476001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:186: file.Close();
Is |file| used after we have validated it? If not,perhaps we could close it
sooner than later. We could also encapsulate the whole file verification in a
function. The idea is that we have this stray Close call in a middle of a
function.

https://codereview.chromium.org/25883006/diff/476001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:194: if (is_delta_) {  //
Package is a diff package.
comment is not needed.

[waffles, 2013-11-27 01:26:42]

Thanks!

https://codereview.chromium.org/25883006/diff/476001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher_operation.h (right):

https://codereview.chromium.org/25883006/diff/476001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:35:
component_updater::ComponentUnpacker::Error Run(
On 2013/11/27 00:34:01, Sorin Jianu wrote:
> namespace needed here and below?

Done.

https://codereview.chromium.org/25883006/diff/476001/chrome/browser/component...
File chrome/browser/component_updater/component_unpacker.cc (right):

https://codereview.chromium.org/25883006/diff/476001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:186: file.Close();
On 2013/11/27 00:34:01, Sorin Jianu wrote:
> Is |file| used after we have validated it? If not,perhaps we could close it
> sooner than later. We could also encapsulate the whole file verification in a
> function. The idea is that we have this stray Close call in a middle of a
> function.
Done.

https://codereview.chromium.org/25883006/diff/476001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:194: if (is_delta_) {  //
Package is a diff package.
On 2013/11/27 00:34:01, Sorin Jianu wrote:
> comment is not needed.

Done.

[waffles, 2014-01-24 20:41:38]

Hi Carlos, Sorin;

Please take another look at this CL.

Carlos, if you'd rather just defer to Sorin, that's OK with me.

[Sorin Jianu, 2014-01-25 02:30:20]

Thank you Josh, please read the comments with full stomach, they are all
nitpicks except one where it is possible a return statement is missing.

I have the feeling that there are small sections of the code that are run as pre
or post conditions for functions that could be encapsulated in the respective
functions, to streamline the code at the call sites.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
File chrome/browser/component_updater/component_patcher.cc (right):

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_patcher.cc:61: if (!commands.get()) {
I wonder if some amount of code duplication here can be eliminated if we
delegate the implementation to a function DifferentialUpdatePatchInternal that
returns an std::pair and this function becomes:

void DifferentialUpdatePatch(... {
  std::pair<> result = DifferentialUpdatePatchInternal(...);
  callback.Run(...);
  return;
}

It's a small deal but it enforces the call back is called in all cases.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
File chrome/browser/component_updater/component_unpacker.cc (right):

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.cc:138: void
ComponentUnpacker::Unpack(
We could have a function like:

bool UnpackInternal(...) {
if (!Verify())
  return false;
if (!Unzip())
  return false;
....
}

And then:
void ComponentUnpacker::Unpack(...) {
  if(!UnpackInternal())
    Finish();
}

Small fry...

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.cc:141: if (!Verify()) {
I knew it Verify would be the first step.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.cc:196: error_ =
kUnzipFailed;
is the code missing a return false here?

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.cc:203: // We want a
different temp directory to put the patch output files into.
I am in the definitely pet peeve territory here but I have nothing else but to
nit pick. I've been avoiding comments in the first person plural since I've come
to Google at the suggestion of Mark Mentovai. Sometimes is wierd to formulate
the comment but in this case it would sound like:
// Use a different temp directory for the patch output files.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.cc:210: FROM_HERE,
base::Bind(&DifferentialUpdatePatch,
Can this function create the unpack_path and return it to the caller? It seems
something that can be encapsulated inside this function. And then, BeginPatching
becomes nice and simple: if this call that else call nop.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.cc:236: if
(!unpack_diff_path_.empty()) {
Arguably, we can have DifferentialUpdatePatch delete the patch files upon
return. Then DonePatching becomes prettier and there would be no need for the
comment.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.cc:245: // Write the
fingerprint to disk.
Is the semantics of the Install that it copies/installs everything in the
unpack_path_ as the manifest.fingerprint would have been part of the original
CRX?

If yes, would it be more appropriate to have writing the fingerprint as a
separate step before Install()?

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.cc:254: if (error_ == kNone)
{
in which case error_ != kNone would be true at this point?

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
File chrome/browser/component_updater/component_unpacker.h (right):

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.h:57: //     \_
BeginPatching
could we do the same outflow as above, and say that the it's a nop instead of
DifferentialUpdatePatch ?

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.h:109: // Before unpacking,
verify that the file is well-formed and correctly signed.
We could say that verification is the first step, unzip is the second, etc.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.h:117: // full
(non-differential) updates. This step is asynchronous. Returns false
I am thinking we can name it BeginPatchingAsync to self document it.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.h:132: void Finish();
Maybe call this function DoneUnpacking?

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

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.h:149:
scoped_refptr<base::SequencedTaskRunner> task_runner_;
disallow copy and assign?

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_updater_service.cc:855: // Do some
cleanup before we skip back to the UI thread.
This comment is more an implementation comment than a function comment. Might
want to remove it completely.

[waffles, 2014-01-28 21:06:30]

Thanks! PTAL.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
File chrome/browser/component_updater/component_patcher.cc (right):

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_patcher.cc:61: if (!commands.get()) {
On 2014/01/25 02:30:21, Sorin Jianu wrote:
> I wonder if some amount of code duplication here can be eliminated if we
> delegate the implementation to a function DifferentialUpdatePatchInternal that
> returns an std::pair and this function becomes:
> 
> void DifferentialUpdatePatch(... {
>   std::pair<> result = DifferentialUpdatePatchInternal(...);
>   callback.Run(...);
>   return;
> }
> 
> It's a small deal but it enforces the call back is called in all cases.

Yes, but I'll have to immediately undo that change in the next CL, since once
operation->Run() becomes asynchronous, DifferentialUpdatePatchInternal would
become asynchronous as well.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
File chrome/browser/component_updater/component_unpacker.cc (right):

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.cc:138: void
ComponentUnpacker::Unpack(
On 2014/01/25 02:30:21, Sorin Jianu wrote:
> We could have a function like:
> 
> bool UnpackInternal(...) {
> if (!Verify())
>   return false;
> if (!Unzip())
>   return false;
> ....
> }
> 
> And then:
> void ComponentUnpacker::Unpack(...) {
>   if(!UnpackInternal())
>     Finish();
> }
> 
> Small fry...
> 
> 

Done.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.cc:141: if (!Verify()) {
On 2014/01/25 02:30:21, Sorin Jianu wrote:
> I knew it Verify would be the first step.

Acknowledged.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.cc:196: error_ =
kUnzipFailed;
On 2014/01/25 02:30:21, Sorin Jianu wrote:
> is the code missing a return false here?

Yes, thanks. Done.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.cc:203: // We want a
different temp directory to put the patch output files into.
On 2014/01/25 02:30:21, Sorin Jianu wrote:
> I am in the definitely pet peeve territory here but I have nothing else but to
> nit pick. I've been avoiding comments in the first person plural since I've
come
> to Google at the suggestion of Mark Mentovai. Sometimes is wierd to formulate
> the comment but in this case it would sound like:
> // Use a different temp directory for the patch output files.

Done. I agree, this one slipped in.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.cc:210: FROM_HERE,
base::Bind(&DifferentialUpdatePatch,
On 2014/01/25 02:30:21, Sorin Jianu wrote:
> Can this function create the unpack_path and return it to the caller? It seems
> something that can be encapsulated inside this function. And then,
BeginPatching
> becomes nice and simple: if this call that else call nop.

Let's discuss. I like Unpacker being responsible for all the paths, though I
agree that such a change is possible. Do you just want this function to be
simplified?

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.cc:236: if
(!unpack_diff_path_.empty()) {
On 2014/01/25 02:30:21, Sorin Jianu wrote:
> Arguably, we can have DifferentialUpdatePatch delete the patch files upon
> return. Then DonePatching becomes prettier and there would be no need for the
> comment.

We could do that; I like having the ComponentUnpacker responsible for deleting
the path it creates, however.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.cc:245: // Write the
fingerprint to disk.
On 2014/01/25 02:30:21, Sorin Jianu wrote:
> Is the semantics of the Install that it copies/installs everything in the
> unpack_path_ as the manifest.fingerprint would have been part of the original
> CRX?
> 
> If yes, would it be more appropriate to have writing the fingerprint as a
> separate step before Install()?

Personally I consider manifest.fingerprint a piece of metadata that we happen to
store alongside the contents of the CRX. I don't mind making an additional step
for it, though, if you prefer that.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.cc:254: if (error_ == kNone)
{
On 2014/01/25 02:30:21, Sorin Jianu wrote:
> in which case error_ != kNone would be true at this point?
No cases, as far as I can tell. I've removed it.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
File chrome/browser/component_updater/component_unpacker.h (right):

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.h:57: //     \_
BeginPatching
On 2014/01/25 02:30:21, Sorin Jianu wrote:
> could we do the same outflow as above, and say that the it's a nop instead of
> DifferentialUpdatePatch ?

Discussed, we decided not to change it.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.h:109: // Before unpacking,
verify that the file is well-formed and correctly signed.
On 2014/01/25 02:30:21, Sorin Jianu wrote:
> We could say that verification is the first step, unzip is the second, etc.

Done.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.h:117: // full
(non-differential) updates. This step is asynchronous. Returns false
On 2014/01/25 02:30:21, Sorin Jianu wrote:
> I am thinking we can name it BeginPatchingAsync to self document it.

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.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.h:132: void Finish();
On 2014/01/25 02:30:21, Sorin Jianu wrote:
> Maybe call this function DoneUnpacking?

I thought about this. There is already ComponentUpdateService::DoneUnpacking
which is passed as the callback, would Unpack() would become BeginUnpacking()?
Let's discuss.


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

Done. I have a slight preference for Done instead of End (since End is also a
verb), but it's best to be consistent.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.h:149:
scoped_refptr<base::SequencedTaskRunner> task_runner_;
On 2014/01/25 02:30:21, Sorin Jianu wrote:
> disallow copy and assign?

Done.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_updater_service.cc:855: // Do some
cleanup before we skip back to the UI thread.
On 2014/01/25 02:30:21, Sorin Jianu wrote:
> This comment is more an implementation comment than a function comment. Might
> want to remove it completely.

Done.

[Sorin Jianu, 2014-01-31 00:32:15]

Thank you!

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
File chrome/browser/component_updater/component_unpacker.cc (right):

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.cc:210: FROM_HERE,
base::Bind(&DifferentialUpdatePatch,
On 2014/01/28 21:06:31, waffles wrote:
> On 2014/01/25 02:30:21, Sorin Jianu wrote:
> > Can this function create the unpack_path and return it to the caller? It
seems
> > something that can be encapsulated inside this function. And then,
> BeginPatching
> > becomes nice and simple: if this call that else call nop.
> 
> Let's discuss. I like Unpacker being responsible for all the paths, though I
> agree that such a change is possible. Do you just want this function to be
> simplified?

I'll leave it up to you to structure the code. In general, I agree with a
certain symmetry when resources are acquired and released. At the same time, the
non-blocking aspect of the code can result in split ownership since it makes
certain c++ idioms impractical, such as scoping of resources.

I can certainly see how DifferentialUpdatePatch can create the path just like a
function can allocate memory and return it tp the caller.

Streamlining functions is a good thing.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.cc:245: // Write the
fingerprint to disk.
On 2014/01/28 21:06:31, waffles wrote:
> On 2014/01/25 02:30:21, Sorin Jianu wrote:
> > Is the semantics of the Install that it copies/installs everything in the
> > unpack_path_ as the manifest.fingerprint would have been part of the
original
> > CRX?
> > 
> > If yes, would it be more appropriate to have writing the fingerprint as a
> > separate step before Install()?
> 
> Personally I consider manifest.fingerprint a piece of metadata that we happen
to
> store alongside the contents of the CRX. I don't mind making an additional
step
> for it, though, if you prefer that.

I have no preference, I am just offering an idea.

https://codereview.chromium.org/25883006/diff/1326001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.cc:254: if (error_ == kNone)
{
On 2014/01/28 21:06:31, waffles wrote:
> On 2014/01/25 02:30:21, Sorin Jianu wrote:
> > in which case error_ != kNone would be true at this point?
> No cases, as far as I can tell. I've removed it.

Seems a good place to assert that errors were handled as a preconditions to
calling this function.

https://codereview.chromium.org/25883006/diff/1446001/chrome/browser/componen...
File chrome/browser/component_updater/component_unpacker.cc (right):

https://codereview.chromium.org/25883006/diff/1446001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.cc:141: // EndPatching()
will be called asynchronously.
Is the comment correct wrt to EndPatching? Could it be BeginPatching instead?

Either way, from a logical standpoint, the comment appears to be unclear, since
we can see where Finish call happens but we do not see the EndPatching call.

https://codereview.chromium.org/25883006/diff/1446001/chrome/browser/componen...
File chrome/browser/component_updater/component_unpacker.h (right):

https://codereview.chromium.org/25883006/diff/1446001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.h:104: // package is a
differential update. Call |callback| with the result.
Calls

[waffles, 2014-01-31 02:54:47]

Thanks!

https://codereview.chromium.org/25883006/diff/1446001/chrome/browser/componen...
File chrome/browser/component_updater/component_unpacker.cc (right):

https://codereview.chromium.org/25883006/diff/1446001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.cc:141: // EndPatching()
will be called asynchronously.
On 2014/01/31 00:32:16, Sorin Jianu wrote:
> Is the comment correct wrt to EndPatching? Could it be BeginPatching instead?
> 
> Either way, from a logical standpoint, the comment appears to be unclear,
since
> we can see where Finish call happens but we do not see the EndPatching call.

Done.

https://codereview.chromium.org/25883006/diff/1446001/chrome/browser/componen...
File chrome/browser/component_updater/component_unpacker.h (right):

https://codereview.chromium.org/25883006/diff/1446001/chrome/browser/componen...
chrome/browser/component_updater/component_unpacker.h:104: // package is a
differential update. Call |callback| with the result.
On 2014/01/31 00:32:16, Sorin Jianu wrote:
> Calls

Done.

[Sorin Jianu, 2014-01-31 03:00:56]

lgtm

Thank you!

[commit-bot: I haz the power, 2014-01-31 03:01:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/waffles@chromium.org/25883006/1466001

[commit-bot: I haz the power, 2014-01-31 06:48:59]

Change committed as 248144

[commit-bot: I haz the power, 2014-01-31 06:49:02]

CQ bit was unchecked on CL. Ignoring.