Use UtilityProcessHost to patch files.

Use UtilityProcessHost to patch files.

Second part of a 2-part changelist.
The first part is https://codereview.chromium.org/25883006/

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

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

[waffles, 2013-10-07 17:58:27]

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

This CL switches from the setup.exe to a ProcessUtilityHost, and makes the
patching operation non-blocking.

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

+Carlos

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

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

[Sorin Jianu, 2014-02-03 20:57:56]

Thank you Josh!

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

https://codereview.chromium.org/25909005/diff/186001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:84:
base::Callback<void(ComponentUnpacker::Error, int)> callback_;
A typedef would be handy to make the code prettier @70, depending on personal
taste.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/DEPS#newc...
chrome/browser/DEPS:34: "+courgette",
As discuss, we need to get buy in from people to add Courgette here. I expect
questions about the size cost to be asked at the minimum.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher.cc (right):

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:76:
callback_.Run(ComponentUnpacker::kNone, 0);
Why is this callback called directly and the ones below it are called in the
thread pool?

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:108: } else if
(next_command_ == commands_->end()) {
This is the if statement that could be eliminated, possibly. Since PatchNextFile
does not seem to increment next_command_, and next_command_ is valid throughout
the PatchNextFile function, then it must not be at the end in this function.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:111: next_command_++;
could use preincrement.

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

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:51: // The constructor
takes an unpacked differential CRX (|input_dir|) and a
This comment appears to be a function comment instead of a class comment.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:56: // encountered.
Calls |callback| when finished, with the error codes if any errors were
encountered.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:62: bool in_process,
might want to document |in_process|.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:67: // Starts patching
files. This method returns immediately, after posting a
Technically, in C++ only virtual member functions are called "methods" although
people are loose with the terminology in many cases.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:80: base::FilePath
input_dir_;
Can some of these members be declared const? The ones initialized in the ctor?

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher_operation.cc (right):

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.cc:44: // These
error ranges are overlapping, so we use an offset to disambiguate
The integer offset disambiguates between overlapping error ranges.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.cc:51:
DeltaUpdateOp* CreateDeltaUpdateOp(base::DictionaryValue* command) {
can the argument be const?

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.cc:109:
ComponentUnpacker::Error error, int extended_error) {
does this line fit on the line above?

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.cc:109:
ComponentUnpacker::Error error, int extended_error) {
Coding style nit:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

The style guide is:
void DeltaUpdateOp::DoneRunning(ComponentUnpacker::Error error, 
                                int extended_error) {

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.cc:113: FROM_HERE,
params vertical align as above?

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.cc:193:
component_updater::PatchType patch_type) : patch_type_(patch_type) {
Formatting of function params and initializers.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.cc:225:
base::Bind(&DeltaUpdateOpPatch::StartProcess, base::Unretained(this)));
Can we have a race here due to ownership issues, not sure, just asking.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.cc:237:
DonePatching(false, result);
I've noticed that is most cases DonePatching is called in case of errors and the
callback is called directly in the success cases. DonePatching also supports the
success case, and that is called from the bridge. Is is worth unifying these
cases and call DonePatching as a completion function in all cases. 

Throughout the CL, being consistent when we call a callback directly or a
function would help ease the readability and the maintenance cost.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.cc:282: int offset
= patch_type_ == component_updater::kPatchTypeCourgette
we have the switch on the patch_type_ in three different functions, would it be
worth introducing polymorphism to handle this?

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

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:8: #include
<string>
needs one line after the lib include.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:32: // result of
the operateration. The callback is called using |task_runner|.
operateration

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:46: bool
in_process_;
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.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:63: const
base::Callback<void(ComponentUnpacker::Error, int)>& callback) = 0;
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.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:92: const
base::Callback<void(ComponentUnpacker::Error, int)>& callback)
maybe typedef the callback type to make the lines shorter in many places?

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:152:
base::Callback<void(ComponentUnpacker::Error, int)>callback_;
callback_ is declared as a public member.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:152:
base::Callback<void(ComponentUnpacker::Error, int)>callback_;
one space between the type and variable name?

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

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.h:149: bool in_process_;
const?

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:863: // Reset the
unpacker last, otherwise we free our own arguments.
What happens if the unpacker is destroyed sooner?

https://codereview.chromium.org/25909005/diff/206001/chrome/common/chrome_uti...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/25909005/diff/206001/chrome/common/chrome_uti...
chrome/common/chrome_utility_messages.h:172: // Tell the utility process to
patch the given |input_file| using |patch_file|
Tells

[waffles, 2014-02-07 01:00:57]

Thanks! I've tried to restructure and cleanup the callback inconsistencies.
Everything is now ref-counted, as we discussed. PTAL.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/DEPS#newc...
chrome/browser/DEPS:34: "+courgette",
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> As discuss, we need to get buy in from people to add Courgette here. I expect
> questions about the size cost to be asked at the minimum.

Agreed.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher.cc (right):

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:76:
callback_.Run(ComponentUnpacker::kNone, 0);
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> Why is this callback called directly and the ones below it are called in the
> thread pool?

Done.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:108: } else if
(next_command_ == commands_->end()) {
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> This is the if statement that could be eliminated, possibly. Since
PatchNextFile
> does not seem to increment next_command_, and next_command_ is valid
throughout
> the PatchNextFile function, then it must not be at the end in this function.

Done.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:111: next_command_++;
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> could use preincrement.

Done.

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

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:51: // The constructor
takes an unpacked differential CRX (|input_dir|) and a
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> This comment appears to be a function comment instead of a class comment.

Done.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:56: // encountered.
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> Calls |callback| when finished, with the error codes if any errors were
> encountered.

Done.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:62: bool in_process,
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> might want to document |in_process|.

Done.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:67: // Starts patching
files. This method returns immediately, after posting a
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> Technically, in C++ only virtual member functions are called "methods"
although
> people are loose with the terminology in many cases.

Could you provide me with a reference on this? All explanations I've found so
far equate member functions with methods, whether or not they are virtual.

A quick search of a (working draft) of the standard seems to indicate that the
standard doesn't mention "method", instead preferring "member function".

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:80: base::FilePath
input_dir_;
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> Can some of these members be declared const? The ones initialized in the ctor?

Done.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher_operation.cc (right):

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.cc:44: // These
error ranges are overlapping, so we use an offset to disambiguate
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> The integer offset disambiguates between overlapping error ranges.

Done.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.cc:51:
DeltaUpdateOp* CreateDeltaUpdateOp(base::DictionaryValue* command) {
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> can the argument be const?

Done.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.cc:109:
ComponentUnpacker::Error error, int extended_error) {
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> does this line fit on the line above?

Not the whole line, no.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.cc:109:
ComponentUnpacker::Error error, int extended_error) {
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> Coding style nit:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> 
> The style guide is:
> void DeltaUpdateOp::DoneRunning(ComponentUnpacker::Error error, 
>                                 int extended_error) {

Done.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.cc:113: FROM_HERE,
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> params vertical align as above?

Done.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.cc:193:
component_updater::PatchType patch_type) : patch_type_(patch_type) {
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> Formatting of function params and initializers.

Done.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.cc:225:
base::Bind(&DeltaUpdateOpPatch::StartProcess, base::Unretained(this)));
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> Can we have a race here due to ownership issues, not sure, just asking.

Yes - this should be resolved now that this class is refcounted.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.cc:237:
DonePatching(false, result);
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> I've noticed that is most cases DonePatching is called in case of errors and
the
> callback is called directly in the success cases. DonePatching also supports
the
> success case, and that is called from the bridge. Is is worth unifying these
> cases and call DonePatching as a completion function in all cases. 
> 
> Throughout the CL, being consistent when we call a callback directly or a
> function would help ease the readability and the maintenance cost.

This should all be much more organized now.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.cc:282: int offset
= patch_type_ == component_updater::kPatchTypeCourgette
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> we have the switch on the patch_type_ in three different functions, would it
be
> worth introducing polymorphism to handle this?

I've tried out a using a traits class - let me know what you think.

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

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:8: #include
<string>
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> needs one line after the lib include.

Done.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:32: // result of
the operateration. The callback is called using |task_runner|.
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> operateration

Done. That's a fun word to say.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:46: bool
in_process_;
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> 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.

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.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:63: const
base::Callback<void(ComponentUnpacker::Error, int)>& callback) = 0;
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> 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. 

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.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:92: const
base::Callback<void(ComponentUnpacker::Error, int)>& callback)
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> maybe typedef the callback type to make the lines shorter in many places?

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

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:152:
base::Callback<void(ComponentUnpacker::Error, int)>callback_;
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> one space between the type and variable name?

Done.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:152:
base::Callback<void(ComponentUnpacker::Error, int)>callback_;
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> callback_ is declared as a public member.

Done.

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

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.h:149: bool in_process_;
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> const?

Done.

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:863: // Reset the
unpacker last, otherwise we free our own arguments.
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> What happens if the unpacker is destroyed sooner?

The problem is that the unpacker owns the EndUnpacking callback, which owns
component_id, crx_path.

If EndUnpacking destroys the unpacker, the unpacker destroys the callback, while
the callback code is running.

This is a pretty awful situation.

One possible solution is to pass a component unpacker function as the callback
function, and make the component unpacker refcounted so that the callback
maintains a reference to the component unpacker. That component unpacker
function could then call CrxUpdateService::EndUnpacking directly. This seems
really circuitous, though.

https://codereview.chromium.org/25909005/diff/206001/chrome/common/chrome_uti...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/25909005/diff/206001/chrome/common/chrome_uti...
chrome/common/chrome_utility_messages.h:172: // Tell the utility process to
patch the given |input_file| using |patch_file|
On 2014/02/03 20:57:57, Sorin Jianu wrote:
> Tells

Done.

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

Thank you Josh, I don't feel strong about any of the issues below, if any
comments are useful, please follow through them.

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

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:67: // Starts patching
files. This method returns immediately, after posting a
On 2014/02/07 01:00:59, waffles wrote:
> On 2014/02/03 20:57:57, Sorin Jianu wrote:
> > Technically, in C++ only virtual member functions are called "methods"
> although
> > people are loose with the terminology in many cases.
> 
> Could you provide me with a reference on this? All explanations I've found so
> far equate member functions with methods, whether or not they are virtual.
> 
> A quick search of a (working draft) of the standard seems to indicate that the
> standard doesn't mention "method", instead preferring "member function".

http://books.google.com/books?id=0klsAQAAQBAJ&printsec=frontcover&source=gbs_...

Look on the definition on the page 586.

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

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:46: bool
in_process_;
On 2014/02/07 01:00:59, waffles wrote:
> On 2014/02/03 20:57:57, Sorin Jianu wrote:
> > 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.
> 
> 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.

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

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:92: const
base::Callback<void(ComponentUnpacker::Error, int)>& callback)
On 2014/02/07 01:00:59, waffles wrote:
> On 2014/02/03 20:57:57, Sorin Jianu wrote:
> > maybe typedef the callback type to make the lines shorter in many places?
> 
> Done. I've never been a fan of typedefs in C++.

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

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/25909005/diff/206001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:863: // Reset the
unpacker last, otherwise we free our own arguments.
On 2014/02/07 01:00:59, waffles wrote:
> On 2014/02/03 20:57:57, Sorin Jianu wrote:
> > What happens if the unpacker is destroyed sooner?
> 
> The problem is that the unpacker owns the EndUnpacking callback, which owns
> component_id, crx_path.
> 
> If EndUnpacking destroys the unpacker, the unpacker destroys the callback,
while
> the callback code is running.
> 
> This is a pretty awful situation.
> 
> One possible solution is to pass a component unpacker function as the callback
> function, and make the component unpacker refcounted so that the callback
> maintains a reference to the component unpacker. That component unpacker
> function could then call CrxUpdateService::EndUnpacking directly. This seems
> really circuitous, though.

Thank you. I would suggest rewording the comment as:
// Reset the unpacker last, since it owns the callback which is currently
running.
 
It also made me think about how the other objects in the service are released
(or not).

https://codereview.chromium.org/25909005/diff/536001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher.cc (right):

https://codereview.chromium.org/25909005/diff/536001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:84: base::DictionaryValue*
command_args =
can this be a pointer to const type?

https://codereview.chromium.org/25909005/diff/536001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:104: if (error !=
ComponentUnpacker::kNone) {
This is definitely a matter of personal taste. I prefer dealing with the happy
case on the if branch of an if-else.

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

https://codereview.chromium.org/25909005/diff/536001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:70: const
base::Callback<void(ComponentUnpacker::Error, int)>& callback);
A typedef could help making the code prettier throughout.

https://codereview.chromium.org/25909005/diff/536001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:90:
scoped_ptr<base::ListValue> commands_;
do we need values.h?

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

https://codereview.chromium.org/25909005/diff/536001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:42: typedef
base::Callback<void(ComponentUnpacker::Error, int)> Callback;
This type could be a typedef in ComponentUnpacker. This typedef here already has
a dependency on the ComponentUnpacker and this compilation unit is including the
header for that type. The idea is that there are too many of the callback types.
At the first glance, they all serve the same function: get called with an
unpacking error and an int.

https://codereview.chromium.org/25909005/diff/536001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:46: bool
InProcess();
Is there a way to remove some of the protected members here. For example,
InProcess could be public. This is not exposing any implementation detail since
in_process is an argument that the caller already knows about.

InProcess can be const.

https://codereview.chromium.org/25909005/diff/536001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:130: class
DeltaUpdateOpPatchTraits {
Let's rename that Traits to something else, with the idea that the terminology
for traits in C++ is relevant to type traits. A couple of related patterns here
are Bridge or Strategy.

https://codereview.chromium.org/25909005/diff/536001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:134: // Returns
an integer to add to error codes to disambiguate their source.
can any of these be const?

https://codereview.chromium.org/25909005/diff/536001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:161: virtual bool
OnMessageReceived(const IPC::Message& message) OVERRIDE;
Can we make these two virtuals private and add the boilerplate comment
//Overrides of...

[waffles, 2014-02-28 00:52:42]

Thanks! Please take another look. Once you're satisfied, I'll ad dgarrett@ and
thestig@ as reviewers.

https://codereview.chromium.org/25909005/diff/536001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher.cc (right):

https://codereview.chromium.org/25909005/diff/536001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:84: base::DictionaryValue*
command_args =
On 2014/02/27 20:53:57, Sorin Jianu wrote:
> can this be a pointer to const type? 

Done.

https://codereview.chromium.org/25909005/diff/536001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:104: if (error !=
ComponentUnpacker::kNone) {
On 2014/02/27 20:53:57, Sorin Jianu wrote:
> This is definitely a matter of personal taste. I prefer dealing with the happy
> case on the if branch of an if-else.

That's interesting. Elsewhere, we've used the pattern of having the failures be
higher in the function - I felt this was more consistent (e.g. could replace
with if(...){ DonePatching(...) return; } ++next_command; PatchNextFile();.

I kind of like it this way, too, since this is how I think about loops -
DonePatching() is like a break and ++next_commmand_ and PatchNextFile is the
final bumping up of the iterator.

Left it as it is for now.

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

https://codereview.chromium.org/25909005/diff/536001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:70: const
base::Callback<void(ComponentUnpacker::Error, int)>& callback);
On 2014/02/27 20:53:57, Sorin Jianu wrote:
> A typedef could help making the code prettier throughout.

Done.

https://codereview.chromium.org/25909005/diff/536001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:90:
scoped_ptr<base::ListValue> commands_;
On 2014/02/27 20:53:57, Sorin Jianu wrote:
> do we need values.h?

Yes thank you.

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

https://codereview.chromium.org/25909005/diff/536001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:42: typedef
base::Callback<void(ComponentUnpacker::Error, int)> Callback;
On 2014/02/27 20:53:57, Sorin Jianu wrote:
> This type could be a typedef in ComponentUnpacker. This typedef here already
has
> a dependency on the ComponentUnpacker and this compilation unit is including
the
> header for that type. The idea is that there are too many of the callback
types.
> At the first glance, they all serve the same function: get called with an
> unpacking error and an int.

Done.

https://codereview.chromium.org/25909005/diff/536001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:46: bool
InProcess();
On 2014/02/27 20:53:57, Sorin Jianu wrote:
> Is there a way to remove some of the protected members here. For example,
> InProcess could be public. This is not exposing any implementation detail
since
> in_process is an argument that the caller already knows about.
> 
> InProcess can be const.

Let's leave this one to a later CL if you don't mind tolerating it for a little
while longer.

https://codereview.chromium.org/25909005/diff/536001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:130: class
DeltaUpdateOpPatchTraits {
On 2014/02/27 20:53:57, Sorin Jianu wrote:
> Let's rename that Traits to something else, with the idea that the terminology
> for traits in C++ is relevant to type traits. A couple of related patterns
here
> are Bridge or Strategy.

Done.

https://codereview.chromium.org/25909005/diff/536001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:134: // Returns
an integer to add to error codes to disambiguate their source.
On 2014/02/27 20:53:57, Sorin Jianu wrote:
> can any of these be const?

Done.

https://codereview.chromium.org/25909005/diff/536001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.h:161: virtual bool
OnMessageReceived(const IPC::Message& message) OVERRIDE;
On 2014/02/27 20:53:57, Sorin Jianu wrote:
> Can we make these two virtuals private and add the boilerplate comment
> //Overrides of...

Done.

[Sorin Jianu, 2014-02-28 01:53:53]

lgtm

Thank you!

https://codereview.chromium.org/25909005/diff/536001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher.cc (right):

https://codereview.chromium.org/25909005/diff/536001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:104: if (error !=
ComponentUnpacker::kNone) {
On 2014/02/28 00:52:43, waffles wrote:
> On 2014/02/27 20:53:57, Sorin Jianu wrote:
> > This is definitely a matter of personal taste. I prefer dealing with the
happy
> > case on the if branch of an if-else.
> 
> That's interesting. Elsewhere, we've used the pattern of having the failures
be
> higher in the function - I felt this was more consistent (e.g. could replace
> with if(...){ DonePatching(...) return; } ++next_command; PatchNextFile();.
> 
> I kind of like it this way, too, since this is how I think about loops -
> DonePatching() is like a break and ++next_commmand_ and PatchNextFile is the
> final bumping up of the iterator.
> 
> Left it as it is for now.

It's ok either way, and a matter of taste.
One of the idioms is dealing with the errors and use an early return.
if (error)
  return error;
bar();

This code is not using that idiom but an if-else statement. I am ok with the
code as is, although I would have reversed the condition of the branch. Not an
issue at all.

https://codereview.chromium.org/25909005/diff/556001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher_operation.cc (right):

https://codereview.chromium.org/25909005/diff/556001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.cc:278:
scoped_ptr<DeltaUpdateOpPatchStrategy> traits) {
need to rename the traits variables throughout.

[waffles, 2014-02-28 17:14:08]

dgarrett, thestig: You are needed for OWNERS approvals.

dgarrett@: please approve or challenge linking courgette into chrome.dll. FYI,
my completely unscientific process of building "release" Chrome Windows w/ and
w/o courgette linked in to chrome changes the (uncompressed) size of chrome.dll
by exactly 54 kilobytes (31496704 bytes (with) vs 31441408 bytes (without)).

thestig@: For OWNERS of chrome/ and chrome/utility. sorin@ is OWNER of
chrome/browser/component_updater.

If anyone on this CL thinks there's anyone else I should get an LGTM from,
please let me know soon.

https://codereview.chromium.org/25909005/diff/556001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher_operation.cc (right):

https://codereview.chromium.org/25909005/diff/556001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_operation.cc:278:
scoped_ptr<DeltaUpdateOpPatchStrategy> traits) {
On 2014/02/28 01:53:53, Sorin Jianu wrote:
> need to rename the traits variables throughout.

Done.

[waffles, 2014-02-28 17:16:40]

(actually adding dgarrett@, thestig@ this time)

dgarrett@, thestig@: You are needed for OWNERS approvals.

dgarrett@: please approve or challenge linking courgette into chrome.dll. FYI,
my completely unscientific process of building "release" Chrome Windows w/ and
w/o courgette linked in to chrome changes the (uncompressed) size of chrome.dll
by exactly 54 kilobytes (31496704 bytes (with) vs 31441408 bytes (without)).

thestig@: For OWNERS of chrome/ and chrome/utility. sorin@ is OWNER of
chrome/browser/component_updater.

If anyone on this CL thinks there's anyone else I should get an LGTM from,
please let me know soon.

[waffles, 2014-02-28 17:18:46]

+sky@ since thestig is marked as OOO.

[sky, 2014-02-28 17:59:03]

LGTM - you're going to need a security reviewer for the message change

https://codereview.chromium.org/25909005/diff/566001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/25909005/diff/566001/chrome/browser/DEPS#newc...
chrome/browser/DEPS:38: "+courgette",
nit: sort.

[waffles, 2014-03-01 00:19:30]

https://codereview.chromium.org/25909005/diff/566001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/25909005/diff/566001/chrome/browser/DEPS#newc...
chrome/browser/DEPS:38: "+courgette",
On 2014/02/28 17:59:04, sky wrote:
> nit: sort.

Done, thanks.
:-/ Bad merge resolution, I think.

[waffles, 2014-03-20 18:18:10]

+cdn@ for IPC message security review.

thestig, dgarrett: ping. :)

[Lei Zhang, 2014-03-20 18:50:00]

On 2014/03/20 18:18:10, waffles wrote:
> +cdn@ for IPC message security review.
> 
> thestig, dgarrett: ping. :)

I thought sky@ reviewed on my behalf while I was OOO. Do you still want me to
review this? If so, any part in particular? All?

[waffles, 2014-03-20 18:53:38]

On 2014/03/20 18:50:00, Lei Zhang wrote:
> On 2014/03/20 18:18:10, waffles wrote:
> > +cdn@ for IPC message security review.
> > 
> > thestig, dgarrett: ping. :)
> 
> I thought sky@ reviewed on my behalf while I was OOO. Do you still want me to
> review this? If so, any part in particular? All?

Ah, whoops, you're right. Now I can't even remember my own changelist history.
Nevermind (although if you want to take a look, you're welcome to).

[dgarrett-goog, 2014-03-20 19:05:45]

LGTM the Courgette usage.

But this CL is the first time I've looked at Chrome's update process outside of
Courgette.

[Cris Neckar, 2014-03-24 18:04:34]

IPC changes LGTM

[waffles, 2014-03-24 18:07:38]

On 2014/03/20 19:05:45, dgarrett-goog wrote:
> LGTM the Courgette usage.
> 
> But this CL is the first time I've looked at Chrome's update process outside
of
> Courgette.

Don, could I also get an LGTM from your @chromium? I think I'll need it to pass
the OWNERS check.

[dgarrett, 2014-03-24 18:21:54]

On 2014/03/24 18:07:38, waffles wrote:
> On 2014/03/20 19:05:45, dgarrett-goog wrote:
> > LGTM the Courgette usage.
> > 
> > But this CL is the first time I've looked at Chrome's update process outside
> of
> > Courgette.
> 
> Don, could I also get an LGTM from your @chromium? I think I'll need it to
pass
> the OWNERS check.

I keep doing that in this tool. Sorry.

[dgarrett, 2014-03-26 00:51:18]

On 2014/03/24 18:21:54, dgarrett wrote:
> On 2014/03/24 18:07:38, waffles wrote:
> > On 2014/03/20 19:05:45, dgarrett-goog wrote:
> > > LGTM the Courgette usage.
> > > 
> > > But this CL is the first time I've looked at Chrome's update process
outside
> > of
> > > Courgette.
> > 
> > Don, could I also get an LGTM from your @chromium? I think I'll need it to
> pass
> > the OWNERS check.
> 
> I keep doing that in this tool. Sorry.

LGTM

[waffles, 2014-03-26 19:33:28]

The CQ bit was checked by waffles@chromium.org

[commit-bot: I haz the power, 2014-03-26 19:35:35]

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

[commit-bot: I haz the power, 2014-03-26 22:35:23]

Change committed as 259726

[erikchen, 2014-03-26 23:43:06]

A revert of this CL has been created in
https://codereview.chromium.org/213923002/ by erikchen@chromium.org.

The reason for reverting is: linux asan failure

http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%2BLSan%20Te....

[waffles, 2014-03-28 17:11:17]

The CQ bit was checked by waffles@chromium.org

[commit-bot: I haz the power, 2014-03-28 17:12:17]

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

[commit-bot: I haz the power, 2014-03-28 19:42:02]

Change committed as 260243