Differential updates for components.

Differential updates for components. We are adding support for delivering delta updates for Chrome components. Initial platform support for the patcher is Windows only. The update response includes both the full update and, if available, the differential update. The differential update is tried first, then the full update, if needed.

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

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

[cpu_(ooo_6.6-7.5), 2013-05-29 20:06:44]

First off I haven't seen anything horrible yet so we are off to a good start.

Here are some comments, I think they apply beyond the few files I reviewed. So
for the things you fix consider the same case across the whole CL.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:403: // file IO to know your existing
component version.
remove this file from the CL. you can TBR=cpu this kind of changes in a
different CL.

In general we only want changes relevant in each Cl, because it helps us find
regressions when we have to bisect official builds.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
File chrome/browser/component_updater/component_patcher.cc (right):

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher.cc:33: if
(!operation.compare("copy")) {
this style is hard to read can we do if (operation == "copy") or is there some
perf reason?

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher.cc:139: if
(!command_args->GetString(kDeltaFrom, &input_rel_path) ||
In the same vein of error handling (see my comment in lin 243) I would like to
have, if it makes sense two different error codes (enums) here, possibly the
second one will be 'missing file' or something like that.

Consider also similar code in line 208, 209, etc.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher.cc:181: if
(!command_args->GetString(kDeltaPatch, &patch_rel_path) ||
comment of line 179 and 180 seem unnecessary.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher.cc:236: if (!commands.get())
{
not sure if we like braces for single line ifs again check the style on
component_updater_service.* for guidance.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher.cc:243: return
DELTA_BAD_COMMANDS;
note that here and and in 248 we return without setting *error, without having
look much further into the CL I question the value of 'error'. Unless specific
actions need to be taken consider removing it.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
File chrome/browser/component_updater/component_patcher.h (right):

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher.h:9: #include
"base/files/file_path.h"
I am guessing you can forward declare FilePath here.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher.h:13: enum DeltaUpdateResult
{
while the all caps constants in enums is a valid style, is best to use the same
style as in the other files in /component_updater/ for example 

Events {
  kManifestCheck,
  kComponentUpdated,
  kManifestError,
  kNetworkError,
  kUnpackError,
  kInstallerError
};

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher.h:47: // Applies a delta
patch to a file.
we can beef a bit this description, like explaining that takes |input_file| and
uses |patch_file| with ...e tc.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher.h:59:
DISALLOW_COPY_AND_ASSIGN(ComponentPatcher);
abstract base classes we normally don't put DISALLOW_COPY_AND_ASSIGN or ctors,
again refer to the style used in component_updater_service.h

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher.h:69: int* error) OVERRIDE;
use of override macro might require the compiler_specific.h include

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
File chrome/browser/component_updater/component_patcher_internal.h (right):

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher_internal.h:11: #include
"base/values.h"
I am guessing the you can fwd declare Value instead.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher_internal.h:41: base::FilePath
output_abs_path_;  // The relative location for the output.
prefer comments above each line. In this particular case the second comment
seems superfluous and misleading since the name has 'abs' and the comment says
relative.

Please give a pass to all the comments, we like more of a commentary on the
contract of the methods (like in lines 28 - 30) than on individual variables and
such.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher_internal.h:65: public:
since DeltaUpdateOp::ArgumentPos is in scope here having
DeltaUpdateOpCopy::ArgumentPos makes the code hard to read for me, is there any
particular advantage to this arrangement?

[cpu_(ooo_6.6-7.5), 2013-05-29 20:13:58]

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.h:21: class FilePath;
note filepath is fwd declared here.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.h:74: std::string
fingerprint;
if they have to provide the fingerprint, could this be something that the
ComponentInstaller can do? 

I imagine a scenario where we don't need the fingerprint until we have received
a reply from the server, or is that not how it works?

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.h:131: virtual GURL
PingUrl() = 0;
hmmm, I already receive complains about undue QPS from the CU, does this mean we
are going to double the traffic to our servers?

[waffles, 2013-06-13 20:55:04]

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:403: // file IO to know your existing
component version.
On 2013/05/29 20:06:44, cpu wrote:
> remove this file from the CL. you can TBR=cpu this kind of changes in a
> different CL.
> 
> In general we only want changes relevant in each Cl, because it helps us find
> regressions when we have to bisect official builds.

Done.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
File chrome/browser/component_updater/component_patcher.cc (right):

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher.cc:33: if
(!operation.compare("copy")) {
On 2013/05/29 20:06:44, cpu wrote:
> this style is hard to read can we do if (operation == "copy") or is there some
> perf reason?

Done.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher.cc:139: if
(!command_args->GetString(kDeltaFrom, &input_rel_path) ||
On 2013/05/29 20:06:44, cpu wrote:
> In the same vein of error handling (see my comment in lin 243) I would like to
> have, if it makes sense two different error codes (enums) here, possibly the
> second one will be 'missing file' or something like that.
> 
> Consider also similar code in line 208, 209, etc.

Done.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher.cc:181: if
(!command_args->GetString(kDeltaPatch, &patch_rel_path) ||
On 2013/05/29 20:06:44, cpu wrote:
> comment of line 179 and 180 seem unnecessary.

Done.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher.cc:236: if (!commands.get())
{
On 2013/05/29 20:06:44, cpu wrote:
> not sure if we like braces for single line ifs again check the style on
> component_updater_service.* for guidance.

Done with great fear.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher.cc:243: return
DELTA_BAD_COMMANDS;
On 2013/05/29 20:06:44, cpu wrote:
> note that here and and in 248 we return without setting *error, without having
> look much further into the CL I question the value of 'error'. Unless specific
> actions need to be taken consider removing it.

We would like the specific error code of bsdiff or courgette to be available to
ping the server with, but we don't want to duplicate courgette and bsdiff's list
of error codes in our DeltaResult, so we use error as a second field to record
that.

We should set *error=0 here, though, so I've done that.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
File chrome/browser/component_updater/component_patcher.h (right):

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher.h:9: #include
"base/files/file_path.h"
On 2013/05/29 20:06:44, cpu wrote:
> I am guessing you can forward declare FilePath here.

Done.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher.h:13: enum DeltaUpdateResult
{
On 2013/05/29 20:06:44, cpu wrote:
> while the all caps constants in enums is a valid style, is best to use the
same
> style as in the other files in /component_updater/ for example 
> 
> Events {
>   kManifestCheck,
>   kComponentUpdated,
>   kManifestError,
>   kNetworkError,
>   kUnpackError,
>   kInstallerError
> };

Done. We've unified this with the unpacker error codes, since they overlapped
anyways.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher.h:47: // Applies a delta
patch to a file.
On 2013/05/29 20:06:44, cpu wrote:
> we can beef a bit this description, like explaining that takes |input_file|
and
> uses |patch_file| with ...e tc.

Done.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher.h:59:
DISALLOW_COPY_AND_ASSIGN(ComponentPatcher);
On 2013/05/29 20:06:44, cpu wrote:
> abstract base classes we normally don't put DISALLOW_COPY_AND_ASSIGN or ctors,
> again refer to the style used in component_updater_service.h

Done.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher.h:69: int* error) OVERRIDE;
On 2013/05/29 20:06:44, cpu wrote:
> use of override macro might require the compiler_specific.h include

Done.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
File chrome/browser/component_updater/component_patcher_internal.h (right):

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher_internal.h:11: #include
"base/values.h"
On 2013/05/29 20:06:44, cpu wrote:
> I am guessing the you can fwd declare Value instead.

Done.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher_internal.h:41: base::FilePath
output_abs_path_;  // The relative location for the output.
On 2013/05/29 20:06:44, cpu wrote:
> prefer comments above each line. In this particular case the second comment
> seems superfluous and misleading since the name has 'abs' and the comment says
> relative.
> 
> Please give a pass to all the comments, we like more of a commentary on the
> contract of the methods (like in lines 28 - 30) than on individual variables
and
> such.

Done.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_patcher_internal.h:65: public:
On 2013/05/29 20:06:44, cpu wrote:
> since DeltaUpdateOp::ArgumentPos is in scope here having
> DeltaUpdateOpCopy::ArgumentPos makes the code hard to read for me, is there
any
> particular advantage to this arrangement?

Done.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.h:21: class FilePath;
On 2013/05/29 20:13:58, cpu wrote:
> note filepath is fwd declared here.

Done.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.h:74: std::string
fingerprint;
On 2013/05/29 20:13:58, cpu wrote:
> if they have to provide the fingerprint, could this be something that the
> ComponentInstaller can do? 
> 
> I imagine a scenario where we don't need the fingerprint until we have
received
> a reply from the server, or is that not how it works?

The fingerprint uniquely identifies the installed component (that is, it's a
stronger identifier than appid + version + operating system + etc); we must send
the current fingerprint to the server in the request so that the server knows
which package to give us the differential update from. The server then returns
the new fingerprint along with the response.

So, it feels like it belongs here with pk_hash and version, since it identifies
the currently installed package.

https://codereview.chromium.org/15908002/diff/64007/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.h:131: virtual GURL
PingUrl() = 0;
On 2013/05/29 20:13:58, cpu wrote:
> hmmm, I already receive complains about undue QPS from the CU, does this mean
we
> are going to double the traffic to our servers?
We've addressed this out of band. For the record, a summary is below.

There will be between 50% and 100% increase in average QPS from the component
updater.
There will be between 60% and 100% increase in peak QPS from the component
updater.
(Increases will be closer to the higher end of this range when multiple
components are releasing, and closer to/at the low end of this range on a daily
basis thanks to CRLSet's 12-hour update cycle.

The services that are going to receive this traffic have given the go-ahead.

[Sorin Jianu, 2013-06-14 22:26:55]

erikwright@: Erik, please take a look at chrome\browser\net\ changes for
clr_set_fetcher.*

asargent@: Anthony, please review the changes to the update manifest and the
overall idea of how to do differential updates for components:
chrome\common\extensions\ and extensions\common\

robershield@: Robert, please look at the installer changes and how we run the
patcher out of process using the setup.exe: chrome\installer\

sra@: Stephen, please review the changes to Courgette: courgette\. Also we have
added Courgette as a dependency for the Chrome unit_tests.

Thank you for reviewing.

[cpu_(ooo_6.6-7.5), 2013-06-14 23:05:49]

https://codereview.chromium.org/15908002/diff/101001/extensions/common/crx_fi...
File extensions/common/crx_file.cc (right):

https://codereview.chromium.org/15908002/diff/101001/extensions/common/crx_fi...
extensions/common/crx_file.cc:27: const char kCrxDiffFileHeaderMagic[] = "CrOD";
in what ways is the differential crx different from a non-differential one?

Also can you add a section to the design doc about the differential updates? I
gave Sorin edit access to it.

[cpu_(ooo_6.6-7.5), 2013-06-14 23:08:35]

can we change each component (nacl, cdm, swiftshader) in a different CL? we can
review them concurrently.

We can even do a different CL for each.

[cpu_(ooo_6.6-7.5), 2013-06-14 23:25:28]

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

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:79: return
ComponentUnpacker::kDeltaOperationFailure;
return kDeltaOperationUnsuported ?

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher_win.cc (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_win.cc:30: // in the
directory where chrome.dll. Returns the path to the setup.exe, if the
where chrome.dll lives?

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_win.cc:83:
launch_options.start_hidden = true;
setup does not launch other process while doing this? if so we need a job
object.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_win.cc:92: 
when do we close this process handle |ph| ?

[cpu_(ooo_6.6-7.5), 2013-06-14 23:28:18]

ignore my comment of splitting components into CLs for all the components that
just return false. For any that actually take advantage of differential my
comment stands.

[waffles, 2013-06-15 00:01:37]

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

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:79: return
ComponentUnpacker::kDeltaOperationFailure;
On 2013/06/14 23:25:28, cpu wrote:
> return kDeltaOperationUnsuported ?

Done; will be in the next patch set.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher_win.cc (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_win.cc:30: // in the
directory where chrome.dll. Returns the path to the setup.exe, if the
On 2013/06/14 23:25:28, cpu wrote:
> where chrome.dll lives?

Done; will be in the next patchset.

https://codereview.chromium.org/15908002/diff/101001/extensions/common/crx_fi...
File extensions/common/crx_file.cc (right):

https://codereview.chromium.org/15908002/diff/101001/extensions/common/crx_fi...
extensions/common/crx_file.cc:27: const char kCrxDiffFileHeaderMagic[] = "CrOD";
There are two major format differences:
1. CrOD instead of Cr24 in the file header.
2. No manifest.json file (commands.json instead).

We wanted to be able to distinguish a patchfile from a normal CRX because normal
extension CRXs should do something sane when you click-and-drag the file intro
Chrome to install it. I'd prefer we had something in the header of a "diff file"
to indicate that it's something that this operation can't be done with - that
way you don't have to unzip it to figure out what it is.

[Sorin Jianu, 2013-06-15 00:10:44]

We will also hook up the installers for diff aware components (pNacl) in a
different CL.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher_win.cc (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_win.cc:83:
launch_options.start_hidden = true;
On 2013/06/14 23:25:28, cpu wrote:
> setup does not launch other process while doing this? if so we need a job
> object.

setup.exe is the only process we create here, no other process is spawned from
here on.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_win.cc:92: 
On 2013/06/14 23:25:28, cpu wrote:
> when do we close this process handle |ph| ?

WaitForExitCode closes the handle.

[sra1, 2013-06-15 00:56:16]

lgtm

lgtm for courgette/*

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

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:22: kPatchTypeBsdiff,
One thing to consider:

Since Courgette ends with doing BSDiff(old+predicted,new), another way to get
BSDiff is to pass through to Courgette a list or other indication of formats it
is allowed to recognize.  If the list is empty, 'predicted' will be empty
(generated by a bsdiff between two empty sequences).

The overhead will be the Courgette header and a couple of additional BSDiff
headers, i.e. about 100 bytes.

[cpu_(ooo_6.6-7.5), 2013-06-15 02:40:36]

>> CRXs should do something sane when you click-and-drag the file intro

ah makes sense on my end. I was scared for a second there.

[robertshield, 2013-06-17 16:39:27]

chrome/installer comments, doing a full pass now

https://codereview.chromium.org/15908002/diff/101001/chrome/installer/setup/s...
File chrome/installer/setup/setup_util.cc (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.cc:38: const int kBsdiffErrorOffset = 600;
This now defines part of the contract for setup.exe's command line. All of the
rest of the possible return values are in util_constants.h. Please could we move
these values and the comment to util_constants.h?

https://codereview.chromium.org/15908002/diff/101001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.cc:111: static_cast<int>(patch_status) +
kCourgetteErrorOffset  : 0;
nit: only one space before :

https://codereview.chromium.org/15908002/diff/101001/chrome/installer/setup/s...
File chrome/installer/setup/setup_util.h (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.h:7: // deltas.
IMO, there's enough patch-related functionality in this file to make it worth
breaking the diff functions into a new pair of files. Perhaps
patch_helper.[h|cc]?

https://codereview.chromium.org/15908002/diff/101001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.h:36: // defined by the bsdiff code (see
mbspatch.h for the definitions of the codes).
please include the full path to mbspatch.h. It may get stale, but so may the
file name really, and it might help future readers find the file.

[erikwright (departed), 2013-06-17 16:59:58]

agl@, can you review chrome/browser/net/crl_set_fetcher.{cc,h}?

[agl, 2013-06-17 17:01:33]

LGTM for CRLSet changes.

[erikwright (departed), 2013-06-17 17:24:28]

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

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:19: enum PatchType {
This enum in the global namespace seems dangerous and collision prone.

I'd recommend making it a class member (maybe it belongs in ComponentPatcher?).

Furthermore, I would consider whether a component_updater namespace should be
used for everything in this directory. Or at least the non-component-specific
bits.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/test/component_patcher_mock.h (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/test/component_patcher_mock.h:24:
DISALLOW_COPY_AND_ASSIGN(MockComponentPatcher);
IWYU: include base/basictypes.h

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/test/component_patcher_unittest.cc
(right):

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/test/component_patcher_unittest.cc:27: class
DummyInstaller : public ComponentInstaller {
Please consider de-duping this class into a dummy_installer.cc/h for re-use
between unittests.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/test/component_unpacker_unittest.cc
(right):

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/test/component_unpacker_unittest.cc:58:
ComponentUnpackerOperationTest::ComponentUnpackerOperationTest() {
Consider whether a common test fixture base class should be extracted. I see
lots of similar code in this and the previous file.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/test/component_unpacker_unittest.cc:67:
file_util::Delete(unpack_dir_, true);
I think base/files/scoped_temp_dir.h would take care of this for you.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/test/component_updater_service_unittest.cc
(right):

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/test/component_updater_service_unittest.cc:192:
class PingChecker {
These classes are sufficiently complicated that I would suggest moving them to
their own file(s).

In fact, I would argue that they need unittests of their own since it will be
very hard for someone else to maintain them, otherwise.

[robertshield, 2013-06-17 17:46:59]

Some more comments.

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

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:58: std::string error;
error isn't used, pass NULL to Deserialize instead

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:64: return
static_cast<base::ListValue*>(root.release());
Could shorten the last five lines to:

return (root.get() && root->IsType(base::Value::TYPE_LIST)) ?
    static_cast<base::ListValue*>(root.release()) : NULL;

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:84:
DeltaUpdateOp::DeltaUpdateOp() {}
This is a relatively unconventional file structure for Chrome code. Typically
method definitions for a class defined in foo.h will be found in foo.cc.

This isn't universally true, but is so often enough that it should only be
deviated from if there's a strong reason (per-platform definitions, etc.).

Unless there's a good reason, please move the definitions of the classes
declared in component_patcher_internal.h to component_patcher_internal.cc. Also,
consider naming those files component_patcher_operations.[h|cc].

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:118: std::vector<uint8>
expected_hash(0);
remove the (0), it is redundant

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:121: return
ComponentUnpacker::kDeltaVerificationFailure;
nit: I find this code a little too dense, and would prefer a blank line after
each return statement (except the last). If you disagree vigorously, let me know
:-)

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:129: if
(memcmp(actual_hash, &expected_hash[0], crypto::kSHA256Length))
crypto::kSHA256Length -> arraysize(actual_hash)

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

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:25: // Applies a delta
patch to a file. Specifically, creates a file at
From reading the implementation, it looks like a delta patch is a bundle that
includes a commands.json file that describes the operation to be performed. It
would be great to have a somewhat longer overview comment describing at a high
level how this stuff works.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher_win.cc (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_win.cc:56: // Applies the
patch to the input file. Returns kNone if the pacth was
patch

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

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:212: fingerprint.size()))
{
indent each line that is a continuation of the same statement by 4: 

if (static_cast<int>(fingerprint.size()) !=
        file_util::WriteFile(
            unpack_path_.Append(FILE_PATH_LITERAL("manifest.fingerprint")),
            fingerprint.c_str(),
            fingerprint.size())) {

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

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.h:72: };
nit: two spaces before comment

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_configurator.cc (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_updater_configurator.cc:96:
pings_enabled_(false),
don't need these in the initializer list if they are unconditionally initialized
below.

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

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:413: struct
PingContext {
This struct appears rather empty. If it's here as a placeholder for future work,
please add a comment to that effect.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:649: // The
componet to add is |crxit| and the |query| string is modified with the
component

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/test/component_patcher_unittest.cc
(right):

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/test/component_patcher_unittest.cc:23: const
int kBsdiffErrorOffset = 600;
These should be taken from util_constants.h when the constants are moved there.

https://codereview.chromium.org/15908002/diff/101001/chrome/common/omaha_quer...
File chrome/common/omaha_query_params/omaha_query_params.h (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/common/omaha_quer...
chrome/common/omaha_query_params/omaha_query_params.h:26: // Returns the value
we use for the "prod=" parameter. Posible return values
Possible

https://codereview.chromium.org/15908002/diff/101001/chrome/installer/setup/s...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/installer/setup/s...
chrome/installer/setup/setup_main.cc:1500: }
we seem to have lost the "handled = false" else clause.

[cpu_(ooo_6.6-7.5), 2013-06-17 20:27:00]

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

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:91: int* error) {
After trying to like the int* error (and its friend extended_error_), I was
unable to. I am back to my comment in the initial CL I don't see it adding any
value and yet being present, passed about or assigned zero in almost all the new
code. Cognitive noise if you will. I strongly suggest you go without it on the
first release.

Is the only real place used in the return value of setup.exe?

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_configurator.cc (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_updater_configurator.cc:79: virtual
bool PingsEnabled() const OVERRIDE;
I feel we have duplicated state here, I propose that PingEnabled and PingUrl
both can be made into one, if PingUrl returns a valid url then pings are
enabled.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_updater_configurator.cc:108:
deltas_enabled_ = cmdline->HasSwitch(switches::kEnableComponentUpdateDeltas);
if deltas enabled are an option, it does not seem necessary to have #if win here

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_updater_configurator.cc:196: return
new ComponentPatcherWin();
this DI is nice but I do wonder if the configurator has the best information to
decide which patcher strategy to use. I can argue it both ways, maybe chrome
knows better, maybe the component updater knows better.

I guess for testing this is a convenient place, if we really like it here I
wonder if we should add a cmdline swith to force the crossplatform one on
windows.

[waffles, 2013-06-17 22:17:48]

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/test/component_patcher_mock.h (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/test/component_patcher_mock.h:24:
DISALLOW_COPY_AND_ASSIGN(MockComponentPatcher);
On 2013/06/17 17:24:28, erikwright wrote:
> IWYU: include base/basictypes.h

Done.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/test/component_patcher_unittest.cc
(right):

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/test/component_patcher_unittest.cc:27: class
DummyInstaller : public ComponentInstaller {
On 2013/06/17 17:24:28, erikwright wrote:
> Please consider de-duping this class into a dummy_installer.cc/h for re-use
> between unittests.
Done, although it didn't quite have the same behavior as the other one.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/test/component_unpacker_unittest.cc
(right):

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/test/component_unpacker_unittest.cc:58:
ComponentUnpackerOperationTest::ComponentUnpackerOperationTest() {
On 2013/06/17 17:24:28, erikwright wrote:
> Consider whether a common test fixture base class should be extracted. I see
> lots of similar code in this and the previous file.

Oops. Somehow this file escaped deletion. It's an earlier version of the other
file.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/test/component_unpacker_unittest.cc:67:
file_util::Delete(unpack_dir_, true);
On 2013/06/17 17:24:28, erikwright wrote:
> I think base/files/scoped_temp_dir.h would take care of this for you.

Applied in component_patcher_unittest.cc.

[Sorin Jianu, 2013-06-17 22:21:09]

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

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:58: std::string error;
On 2013/06/17 17:47:00, robertshield wrote:
> error isn't used, pass NULL to Deserialize instead

Done.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:64: return
static_cast<base::ListValue*>(root.release());
On 2013/06/17 17:47:00, robertshield wrote:
> Could shorten the last five lines to:
> 
> return (root.get() && root->IsType(base::Value::TYPE_LIST)) ?
>     static_cast<base::ListValue*>(root.release()) : NULL;

I prefer to keep it as is instead of the terse expression. Seems easier to read
but I can change it if needed.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:91: int* error) {
On 2013/06/17 20:27:01, cpu wrote:
> After trying to like the int* error (and its friend extended_error_), I was
> unable to. I am back to my comment in the initial CL I don't see it adding any
> value and yet being present, passed about or assigned zero in almost all the
new
> code. Cognitive noise if you will. I strongly suggest you go without it on the
> first release.
> 
> Is the only real place used in the return value of setup.exe?

The component patcher can give us valuable error information regarding why
deltas can fail, either due to Courgette or bsdiff errors. I know it sux to have
it but at the same time we also want to get a detailed understanding of
patching. Even though in the future we will change the way we run Courgette, we
would still want the errors in the pings. Part of the issue here is that we have
overlapping error ranges, so we use a category and an extended error
information, which is this error which I agree, it is hard to fall in love with
it.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:118: std::vector<uint8>
expected_hash(0);
On 2013/06/17 17:47:00, robertshield wrote:
> remove the (0), it is redundant

Done.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:121: return
ComponentUnpacker::kDeltaVerificationFailure;
On 2013/06/17 17:47:00, robertshield wrote:
> nit: I find this code a little too dense, and would prefer a blank line after
> each return statement (except the last). If you disagree vigorously, let me
know
> :-)

Done.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:129: if
(memcmp(actual_hash, &expected_hash[0], crypto::kSHA256Length))
On 2013/06/17 17:47:00, robertshield wrote:
> crypto::kSHA256Length -> arraysize(actual_hash)

replaced it with sizeof since we are doing a byte-wise memcmp.

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

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:19: enum PatchType {
On 2013/06/17 17:24:28, erikwright wrote:
> This enum in the global namespace seems dangerous and collision prone.
> 
> I'd recommend making it a class member (maybe it belongs in
ComponentPatcher?).
> 
> Furthermore, I would consider whether a component_updater namespace should be
> used for everything in this directory. Or at least the non-component-specific
> bits.

Done. We will introduce a namespace in a future CL.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher_win.cc (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_win.cc:56: // Applies the
patch to the input file. Returns kNone if the pacth was
On 2013/06/17 17:47:00, robertshield wrote:
> patch

Done.

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

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.cc:212: fingerprint.size()))
{
On 2013/06/17 17:47:00, robertshield wrote:
> indent each line that is a continuation of the same statement by 4: 
> 
> if (static_cast<int>(fingerprint.size()) !=
>         file_util::WriteFile(
>             unpack_path_.Append(FILE_PATH_LITERAL("manifest.fingerprint")),
>             fingerprint.c_str(),
>             fingerprint.size())) {

Done.

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

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_unpacker.h:72: };
On 2013/06/17 17:47:00, robertshield wrote:
> nit: two spaces before comment

Done.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_configurator.cc (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_updater_configurator.cc:96:
pings_enabled_(false),
On 2013/06/17 17:47:00, robertshield wrote:
> don't need these in the initializer list if they are unconditionally
initialized
> below.

We prefer to initialize any member in all cases even they get assign to later
on. Is that ok with you? I can change it but we are more afraid of the
initialization bugs and Heisenbugs when code changes later on than the runtime
cost of defensively initializing everything.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_updater_configurator.cc:108:
deltas_enabled_ = cmdline->HasSwitch(switches::kEnableComponentUpdateDeltas);
On 2013/06/17 20:27:01, cpu wrote:
> if deltas enabled are an option, it does not seem necessary to have #if win
here

People could turn on deltas on platforms that we don't support at the moment if
we remove the #if. Do we have a concern with that?

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_updater_configurator.cc:196: return
new ComponentPatcherWin();
On 2013/06/17 20:27:01, cpu wrote:
> this DI is nice but I do wonder if the configurator has the best information
to
> decide which patcher strategy to use. I can argue it both ways, maybe chrome
> knows better, maybe the component updater knows better.
> 
> I guess for testing this is a convenient place, if we really like it here I
> wonder if we should add a cmdline swith to force the crossplatform one on
> windows.

Right on, I needed this primarily for DI and this was the easier to hack it in.
It is the service locator pattern. WE hope that next iteration gets comes up
with an implementation of the cross-platform patcher and this code goes away
altogether.

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

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:413: struct
PingContext {
On 2013/06/17 17:47:00, robertshield wrote:
> This struct appears rather empty. If it's here as a placeholder for future
work,
> please add a comment to that effect.

Done.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:649: // The
componet to add is |crxit| and the |query| string is modified with the
On 2013/06/17 17:47:00, robertshield wrote:
> component

Done.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/test/component_patcher_mock.h (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/test/component_patcher_mock.h:24:
DISALLOW_COPY_AND_ASSIGN(MockComponentPatcher);
On 2013/06/17 17:24:28, erikwright wrote:
> IWYU: include base/basictypes.h

Done.

https://codereview.chromium.org/15908002/diff/101001/chrome/common/omaha_quer...
File chrome/common/omaha_query_params/omaha_query_params.h (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/common/omaha_quer...
chrome/common/omaha_query_params/omaha_query_params.h:26: // Returns the value
we use for the "prod=" parameter. Posible return values
On 2013/06/17 17:47:00, robertshield wrote:
> Possible

Done.

https://codereview.chromium.org/15908002/diff/101001/chrome/installer/setup/s...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/installer/setup/s...
chrome/installer/setup/setup_main.cc:1500: }
On 2013/06/17 17:47:00, robertshield wrote:
> we seem to have lost the "handled = false" else clause.

thank you!

https://codereview.chromium.org/15908002/diff/101001/chrome/installer/setup/s...
File chrome/installer/setup/setup_util.cc (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.cc:38: const int kBsdiffErrorOffset = 600;
On 2013/06/17 16:39:28, robertshield wrote:
> This now defines part of the contract for setup.exe's command line. All of the
> rest of the possible return values are in util_constants.h. Please could we
move
> these values and the comment to util_constants.h?

Done.

https://codereview.chromium.org/15908002/diff/101001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.cc:111: static_cast<int>(patch_status) +
kCourgetteErrorOffset  : 0;
On 2013/06/17 16:39:28, robertshield wrote:
> nit: only one space before :

Done.

https://codereview.chromium.org/15908002/diff/101001/chrome/installer/setup/s...
File chrome/installer/setup/setup_util.h (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.h:7: // deltas.
On 2013/06/17 16:39:28, robertshield wrote:
> IMO, there's enough patch-related functionality in this file to make it worth
> breaking the diff functions into a new pair of files. Perhaps
> patch_helper.[h|cc]?

My expectation is that the patching code will be hosted in a different way in
the next milestone. The patching code will be cross-platform at that time, so
dependencies on the setup.exe will likely be removed. If not, I will refactor
the code at that time. Will that work for you?

https://codereview.chromium.org/15908002/diff/101001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.h:36: // defined by the bsdiff code (see
mbspatch.h for the definitions of the codes).
On 2013/06/17 16:39:28, robertshield wrote:
> please include the full path to mbspatch.h. It may get stale, but so may the
> file name really, and it might help future readers find the file.

Done.

[waffles, 2013-06-17 23:51:07]

Changes will be in patchset 11.

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

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:22: kPatchTypeBsdiff,
On 2013/06/15 00:56:17, sra1 wrote:
> One thing to consider:
> 
> Since Courgette ends with doing BSDiff(old+predicted,new), another way to get
> BSDiff is to pass through to Courgette a list or other indication of formats
it
> is allowed to recognize.  If the list is empty, 'predicted' will be empty
> (generated by a bsdiff between two empty sequences).
> 
> The overhead will be the Courgette header and a couple of additional BSDiff
> headers, i.e. about 100 bytes.

That makes sense; but I think we prefer to keep it this way, at least for now;
being able to fall back on bsdiff without invoking courgette codepaths might be
useful for reliability reasons, and the smaller header is a bonus. Since we have
the --genbsdiff flag already, I think it makes sense to use it.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:25: // Applies a delta
patch to a file. Specifically, creates a file at
On 2013/06/17 17:47:00, robertshield wrote:
> From reading the implementation, it looks like a delta patch is a bundle that
> includes a commands.json file that describes the operation to be performed. It
> would be great to have a somewhat longer overview comment describing at a high
> level how this stuff works.

Done; the comment for DifferentialUpdatePatch has been improved.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_configurator.cc (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_updater_configurator.cc:79: virtual
bool PingsEnabled() const OVERRIDE;
On 2013/06/17 20:27:01, cpu wrote:
> I feel we have duplicated state here, I propose that PingEnabled and PingUrl
> both can be made into one, if PingUrl returns a valid url then pings are
> enabled.

Done.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/test/component_patcher_unittest.cc
(right):

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/test/component_patcher_unittest.cc:23: const
int kBsdiffErrorOffset = 600;
On 2013/06/17 17:47:00, robertshield wrote:
> These should be taken from util_constants.h when the constants are moved
there.

As per off-line discussion, we're leaving it and adding a comment.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/test/component_updater_service_unittest.cc
(right):

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/test/component_updater_service_unittest.cc:192:
class PingChecker {
On 2013/06/17 17:24:28, erikwright wrote:
> These classes are sufficiently complicated that I would suggest moving them to
> their own file(s).
> 
> In fact, I would argue that they need unittests of their own since it will be
> very hard for someone else to maintain them, otherwise.

I've moved the POST interceptors into a separate file, but kept the PingChecker
here, since it feels very closely tied with this code.

I haven't added unit tests for them.

[asargent_no_longer_on_chrome, 2013-06-18 00:08:00]

lgtm


nit: The CL description would be easier to read if line wrapped at 70-75 chars
or so.

https://codereview.chromium.org/15908002/diff/132001/chrome/common/extensions...
File chrome/common/extensions/update_manifest.cc (right):

https://codereview.chromium.org/15908002/diff/132001/chrome/common/extensions...
chrome/common/extensions/update_manifest.cc:201: result->package_fingerprint =
GetAttribute(updatecheck, "fp");
Consider mentioning this attribute in the comment in the .h file where we have
an example manifest xml file.

[waffles, 2013-06-18 00:36:25]

https://codereview.chromium.org/15908002/diff/132001/chrome/common/extensions...
File chrome/common/extensions/update_manifest.cc (right):

https://codereview.chromium.org/15908002/diff/132001/chrome/common/extensions...
chrome/common/extensions/update_manifest.cc:201: result->package_fingerprint =
GetAttribute(updatecheck, "fp");
On 2013/06/18 00:08:01, Antony Sargent wrote:
> Consider mentioning this attribute in the comment in the .h file where we have
> an example manifest xml file. 

Done.

[Sorin Jianu, 2013-06-18 03:05:13]

We addressed all the feedback so far, PTAL. Thank you!

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

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:84:
DeltaUpdateOp::DeltaUpdateOp() {}
On 2013/06/17 17:47:00, robertshield wrote:
> This is a relatively unconventional file structure for Chrome code. Typically
> method definitions for a class defined in foo.h will be found in foo.cc.
> 
> This isn't universally true, but is so often enough that it should only be
> deviated from if there's a strong reason (per-platform definitions, etc.).
> 
> Unless there's a good reason, please move the definitions of the classes
> declared in component_patcher_internal.h to component_patcher_internal.cc.
Also,
> consider naming those files component_patcher_operations.[h|cc].

Done. Introduced component_patcher_operation.[h|cc]

[robertshield, 2013-06-18 13:27:07]

lgtm

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

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:64: return
static_cast<base::ListValue*>(root.release());
On 2013/06/17 22:21:09, sorinj wrote:
> On 2013/06/17 17:47:00, robertshield wrote:
> > Could shorten the last five lines to:
> > 
> > return (root.get() && root->IsType(base::Value::TYPE_LIST)) ?
> >     static_cast<base::ListValue*>(root.release()) : NULL;
> 
> I prefer to keep it as is instead of the terse expression. Seems easier to
read
> but I can change it if needed.

How about a compromise:

if (!root.get()) || !root->IsType(base::Value::TYPE_LIST))
  return NULL;
return static_cast<base::ListValue*>(root.release());

The repeated "return NULL;" statements looked odd to me. Or you can leave it as
is, don't really mind :-)

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_configurator.cc (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_updater_configurator.cc:96:
pings_enabled_(false),
On 2013/06/17 22:21:09, sorinj wrote:
> On 2013/06/17 17:47:00, robertshield wrote:
> > don't need these in the initializer list if they are unconditionally
> initialized
> > below.
> 
> We prefer to initialize any member in all cases even they get assign to later
> on. Is that ok with you? I can change it but we are more afraid of the
> initialization bugs and Heisenbugs when code changes later on than the runtime
> cost of defensively initializing everything.

Sounds good. In that case, please initialize fast_update_ and out_of_process_ to
false as well.

https://codereview.chromium.org/15908002/diff/101001/chrome/installer/setup/s...
File chrome/installer/setup/setup_util.h (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.h:7: // deltas.
On 2013/06/17 22:21:09, sorinj wrote:
> On 2013/06/17 16:39:28, robertshield wrote:
> > IMO, there's enough patch-related functionality in this file to make it
worth
> > breaking the diff functions into a new pair of files. Perhaps
> > patch_helper.[h|cc]?
> 
> My expectation is that the patching code will be hosted in a different way in
> the next milestone. The patching code will be cross-platform at that time, so
> dependencies on the setup.exe will likely be removed. If not, I will refactor
> the code at that time. Will that work for you?

sgtm

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

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:56: typedef
ComponentUpdateService::Configurator Config;
This typedef appears to be used only in a couple of places below (whereas the
original type is used more often). I'd suggest getting rid of the typedef, since
it doesn't greatly enhance readability. If you are attached to it, please use it
everywhere.

[erikwright (departed), 2013-06-18 14:02:46]

LGTM.

[Sorin Jianu, 2013-06-18 17:00:11]

Thank you Robert! All done!

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

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.cc:64: return
static_cast<base::ListValue*>(root.release());
On 2013/06/18 13:27:07, robertshield wrote:
> On 2013/06/17 22:21:09, sorinj wrote:
> > On 2013/06/17 17:47:00, robertshield wrote:
> > > Could shorten the last five lines to:
> > > 
> > > return (root.get() && root->IsType(base::Value::TYPE_LIST)) ?
> > >     static_cast<base::ListValue*>(root.release()) : NULL;
> > 
> > I prefer to keep it as is instead of the terse expression. Seems easier to
> read
> > but I can change it if needed.
> 
> How about a compromise:
> 
> if (!root.get()) || !root->IsType(base::Value::TYPE_LIST))
>   return NULL;
> return static_cast<base::ListValue*>(root.release());
> 
> The repeated "return NULL;" statements looked odd to me. Or you can leave it
as
> is, don't really mind :-)

Thank you Robert. Given the choice, I decided to follow your first suggestion
and used the ?: operator.

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_configurator.cc (right):

https://codereview.chromium.org/15908002/diff/101001/chrome/browser/component...
chrome/browser/component_updater/component_updater_configurator.cc:96:
pings_enabled_(false),
On 2013/06/18 13:27:07, robertshield wrote:
> On 2013/06/17 22:21:09, sorinj wrote:
> > On 2013/06/17 17:47:00, robertshield wrote:
> > > don't need these in the initializer list if they are unconditionally
> > initialized
> > > below.
> > 
> > We prefer to initialize any member in all cases even they get assign to
later
> > on. Is that ok with you? I can change it but we are more afraid of the
> > initialization bugs and Heisenbugs when code changes later on than the
runtime
> > cost of defensively initializing everything.
> 
> Sounds good. In that case, please initialize fast_update_ and out_of_process_
to
> false as well.

Done.

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

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:56: typedef
ComponentUpdateService::Configurator Config;
On 2013/06/18 13:27:07, robertshield wrote:
> This typedef appears to be used only in a couple of places below (whereas the
> original type is used more often). I'd suggest getting rid of the typedef,
since
> it doesn't greatly enhance readability. If you are attached to it, please use
it
> everywhere. 

Done.

[cpu_(ooo_6.6-7.5), 2013-06-18 17:02:05]

sorry I couldn't get to this yesterday, here is enough to get you going.

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
File chrome/browser/component_updater/component_updater_service.cc (left):

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:205: CrxComponent
component;
In here we differ in philosophy. As you can see I took pains to minimize the per
item state, the new CL makes the per item state everything that you can possibly
care about.

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:453: // |to|
statatus and returns how many have been changed.
typo here :)

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

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:267: // True is a
completion ping has been queued for this component. If an update
"True is"?

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:270: bool
ping_queued;
maybe I got it wrong but you don't need this. If it CanPing() and it reached one
of the states, a ping will be made.

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:279: int
diff_size;
why do we need these sizes here? I failed to find where they are used. Also why
are they int? what does it mean to have a negative size?

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:294: int
diff_extra_code1;
sigh

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:510:
os_type_(chrome::VersionInfo().OSType()),
is this expensive to query, enough that we want to have in the object state?

You will find that one of my fixations is to minimize object state.

I am guessing that a lot of the new state is going to go somewhere once we
settle the item o line 986

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:985: crx->status =
CrxUpdateItem::kNoUpdate;
Use ChangeItemStatus

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:986:
config_->OnEvent(Configurator::kNetworkError, CrxIdtoUMAId(context->id));
and here we get to the fun part, as you can see there is an existing error
reporting framework. I don't see a compelling reason to have two of them.

The choice of moving the error reporting out of the core and into the
configurator was intentional. How errors are processed and reported is not
'core' to this code.

I am willing to let go of UMA, even though that is how chrome manages reporting
and use your reporting on the view that is omaha folks monitoring it, but in
turn I want all of it out into the configurator. 

Ideally the only code you will see here is config_=>OnEvent(....).

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:1096: VLOG(3) <<
"Sending ping. " << ping;
please remove all the VLOGing, instrument your local build if you want to printf
debug.

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.h:42: // Set
|installed_file| to the full path to the installed |file|. |file| is
I was expecting as input the version, but I guess that would introduce
complexity on the clients, lets at least mention that here.

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.h:55: // Only |name|
is optional. |pk_hash| is the SHA256 hash of the component's
"only name is optional"

[Sorin Jianu, 2013-06-18 20:46:38]

Thank you Carlos! PTAL.

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
File chrome/browser/component_updater/component_updater_service.cc (left):

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:205: CrxComponent
component;
On 2013/06/18 17:02:06, cpu wrote:
> In here we differ in philosophy. As you can see I took pains to minimize the
per
> item state, the new CL makes the per item state everything that you can
possibly
> care about.

I am sorry if we are breaking the existing design. CrxUpdateItem is more
stateful since we need to remember some more state information regarding to
delta updates. We are also introducing completion pings which include some of
the state information. 

In general, what is the concern that made you minimize the per item state
maintained in the CrxUpdateItem? I thought that this was the purpose of the
CrxUpdateItem: hold the update state of the object as it runs through the state
machine driven by the UpdateService.

We have agreed that more state is needed and the state has to go somewhere. I am
open to discuss this, we want to do what is right.

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:453: // |to|
statatus and returns how many have been changed.
On 2013/06/18 17:02:06, cpu wrote:
> typo here :)

What is the typo? I am sorry, we might have fixed this already, I can't see what
is wrong.

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

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:267: // True is a
completion ping has been queued for this component. If an update
On 2013/06/18 17:02:06, cpu wrote:
> "True is"?

Done.

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:270: bool
ping_queued;
On 2013/06/18 17:02:06, cpu wrote:
> maybe I got it wrong but you don't need this. If it CanPing() and it reached
one
> of the states, a ping will be made.

We removed the queuing of pings

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:279: int
diff_size;
On 2013/06/18 17:02:06, cpu wrote:
> why do we need these sizes here? I failed to find where they are used. Also
why
> are they int? what does it mean to have a negative size?

These are values that we parsed from the update response. In Omaha we use these
values for both validating the file integrity and for sending back quality of
service data.

We parse these values as ints. It does not mean sizes can be negative but we do
have a limit of how large the components can be. Is there a portable type in
Chromium for file sizes? What do you recommend?

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:294: int
diff_extra_code1;
On 2013/06/18 17:02:06, cpu wrote:
> sigh

I understand. The challenge we have here is that the amount of data we want to
return is more than what an integer can fit, so there has be be a way to hold
more error state, unless we don't want to return extended error information.
Which is so important to understand why things break in production.

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:510:
os_type_(chrome::VersionInfo().OSType()),
On 2013/06/18 17:02:06, cpu wrote:
> is this expensive to query, enough that we want to have in the object state?
> 
> You will find that one of my fixations is to minimize object state.
> 
> I am guessing that a lot of the new state is going to go somewhere once we
> settle the item o line 986

I agree that the state has to go somewhere or it has to be queried as needed.

Querying these members is not expensive, at least for |prod_id_| and |os_type_|.
My thinking is that since these members are used together, they are maintained
together as immutable members in the service.

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:985: crx->status =
CrxUpdateItem::kNoUpdate;
On 2013/06/18 17:02:06, cpu wrote:
> Use ChangeItemStatus

ChangeItemStatus changes the state of all items (from, to). We only want to
change the state for this item.

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:986:
config_->OnEvent(Configurator::kNetworkError, CrxIdtoUMAId(context->id));
On 2013/06/18 17:02:06, cpu wrote:
> and here we get to the fun part, as you can see there is an existing error
> reporting framework. I don't see a compelling reason to have two of them.
> 
> The choice of moving the error reporting out of the core and into the
> configurator was intentional. How errors are processed and reported is not
> 'core' to this code.
> 
> I am willing to let go of UMA, even though that is how chrome manages
reporting
> and use your reporting on the view that is omaha folks monitoring it, but in
> turn I want all of it out into the configurator. 
> 
> Ideally the only code you will see here is config_=>OnEvent(....).

Let's discuss this and see what is reasonable to do. Let's see what we want to
do and what we agree on so far:
* we want to send completion pings for all users to monitor the quality of the
update service
* we agree that Omaha will do this and we can do outside UMA, just as we do it
for Chrome updates.
* we agree that the error handling and reporting should be decoupled from the
rest of the code.

We are happy to follow your advice and move the error handling and the pinging
logic out in the configurator. The state that needs to be sent to the server
must be encapsulated in some object. Right now that object is the CrxUpdateItem.
At this time, I see three options:
1. Introduce a dependency in ChromeConfigurator on the CrxUpdateItem so that an
item can be passed as an argument to the OnEvent.
2. Pass the state information in some other kind of object which becomes an
argument to OnEvent.
2. Pass the state information as standalone parameters to OnEvent, as in:
config_->OnEvent(..., id, diff_error_code, error_code, version,
previous_version,....)
Which way would you recommend we go?

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:1096: VLOG(3) <<
"Sending ping. " << ping;
On 2013/06/18 17:02:06, cpu wrote:
> please remove all the VLOGing, instrument your local build if you want to
printf
> debug.

Done.

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.h:42: // Set
|installed_file| to the full path to the installed |file|. |file| is
On 2013/06/18 17:02:06, cpu wrote:
> I was expecting as input the version, but I guess that would introduce
> complexity on the clients, lets at least mention that here.

Done.

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.h:55: // Only |name|
is optional. |pk_hash| is the SHA256 hash of the component's
On 2013/06/18 17:02:06, cpu wrote:
> "only name is optional"

I am sorry I don't understand the comment.

[waffles, 2013-06-18 20:50:40]

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
File chrome/browser/component_updater/component_updater_service.cc (left):

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:453: // |to|
statatus and returns how many have been changed.
On 2013/06/18 20:46:39, sorinj wrote:
> On 2013/06/18 17:02:06, cpu wrote:
> > typo here :)
> 
> What is the typo? I am sorry, we might have fixed this already, I can't see
what
> is wrong.

Sorry for the confusion, I fixed that one. :)

[cpu_(ooo_6.6-7.5), 2013-06-18 22:02:35]

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

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:985: crx->status =
CrxUpdateItem::kNoUpdate;
On 2013/06/18 20:46:39, sorinj wrote:
> On 2013/06/18 17:02:06, cpu wrote:
> > Use ChangeItemStatus
> 
> ChangeItemStatus changes the state of all items (from, to). We only want to
> change the state for this item.

Correct. The way it is used like in line 802 is because there can be only one
component in kDownloading, this is important for the operation of the component
updater of having only one component in flight. It is sort of a self discipline
to use when it can be used.

[cpu_(ooo_6.6-7.5), 2013-06-18 22:07:33]

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.h:55: // Only |name|
is optional. |pk_hash| is the SHA256 hash of the component's
On 2013/06/18 20:46:39, sorinj wrote:
> On 2013/06/18 17:02:06, cpu wrote:
> > "only name is optional"
> 
> I am sorry I don't understand the comment.

If fingerprint optional? if so then comment needs to be updated.

[cpu_(ooo_6.6-7.5), 2013-06-18 22:18:24]

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

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:986:
config_->OnEvent(Configurator::kNetworkError, CrxIdtoUMAId(context->id));
On 2013/06/18 20:46:39, sorinj wrote:
> On 2013/06/18 17:02:06, cpu wrote:
> > and here we get to the fun part, as you can see there is an existing error
> > reporting framework. I don't see a compelling reason to have two of them.
> > 
> > The choice of moving the error reporting out of the core and into the
> > configurator was intentional. How errors are processed and reported is not
> > 'core' to this code.
> > 
> > I am willing to let go of UMA, even though that is how chrome manages
> reporting
> > and use your reporting on the view that is omaha folks monitoring it, but in
> > turn I want all of it out into the configurator. 
> > 
> > Ideally the only code you will see here is config_=>OnEvent(....).
> 
> Let's discuss this and see what is reasonable to do. Let's see what we want to
> do and what we agree on so far:
> * we want to send completion pings for all users to monitor the quality of the
> update service
> * we agree that Omaha will do this and we can do outside UMA, just as we do it
> for Chrome updates.
> * we agree that the error handling and reporting should be decoupled from the
> rest of the code.
> 
> We are happy to follow your advice and move the error handling and the pinging
> logic out in the configurator. The state that needs to be sent to the server
> must be encapsulated in some object. Right now that object is the
CrxUpdateItem.
> At this time, I see three options:
> 1. Introduce a dependency in ChromeConfigurator on the CrxUpdateItem so that
an
> item can be passed as an argument to the OnEvent.
> 2. Pass the state information in some other kind of object which becomes an
> argument to OnEvent.
> 2. Pass the state information as standalone parameters to OnEvent, as in:
> config_->OnEvent(..., id, diff_error_code, error_code, version,
> previous_version,....)
> Which way would you recommend we go? 

Neat to have agreement here.

I like 2a or 2b. If it was me I would try 2b and see how it looks, I would try
that first because then in the configurator I can maintain my own vector of
UpdateItemStatus or whatever name. 

Basically lets nuke the existing UMA reporting and lets do your with the least
coupling that we can do.

I would also be interesting to have this pinging decoupled from the update
scheduler, like say only once every 8 hours or so bulk reporting of the whole
thing.

[cpu_(ooo_6.6-7.5), 2013-06-18 22:32:36]

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

https://codereview.chromium.org/15908002/diff/214003/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:964:
base::Unretained(this)));
this postask is probably wrong. The design assumes ProcessPendingItems is only
called from the ScheduleNextRun. The idea here it to have only the timer drive
the state transition of the UI-thread bound items, which is pretty much all
updater state.

Use posttask here only to transition to a from the the file thread. See the long
comment starting at line 339 for more background.

I can see now why ChangeItemStatus was weird to use here. Hopefully now it makes
more sense.

https://codereview.chromium.org/15908002/diff/214003/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:1040:
BrowserThread::PostTask(
same comment here as of  964

[cpu_(ooo_6.6-7.5), 2013-06-18 22:45:52]

https://codereview.chromium.org/15908002/diff/214003/chrome/browser/component...
File chrome/browser/component_updater/component_updater_configurator.cc (right):

https://codereview.chromium.org/15908002/diff/214003/chrome/browser/component...
chrome/browser/component_updater/component_updater_configurator.cc:97:
pings_enabled_(false),
do we want pings disabled by default?

Is the plan to have all users ping, eventually? if so it is best to start in
'on' in canary since the smaller population has less change of DoS ing us.

https://codereview.chromium.org/15908002/diff/214003/chrome/browser/component...
chrome/browser/component_updater/component_updater_configurator.cc:107:
pings_enabled_ = cmdline->HasSwitch(switches::kEnableComponentUpdatePings);
probably best if we can use the existing scheme, it uses a single switch
--component-updater-debug= comma separated values here, instead of top level
chrome switches, maybe we can shorten it to just be component-updater

--component-updater=use-deltas,fast-update.

See lines 32, to 38.

[waffles, 2013-06-19 00:02:10]

https://codereview.chromium.org/15908002/diff/214003/chrome/browser/component...
File chrome/browser/component_updater/component_updater_configurator.cc (right):

https://codereview.chromium.org/15908002/diff/214003/chrome/browser/component...
chrome/browser/component_updater/component_updater_configurator.cc:97:
pings_enabled_(false),
On 2013/06/18 22:45:52, cpu wrote:
> do we want pings disabled by default?
> 
> Is the plan to have all users ping, eventually? if so it is best to start in
> 'on' in canary since the smaller population has less change of DoS ing us.

I'm planning on submitting another changelist almost immediately after this one
goes in to flip the flags to be on by default. I thought doing the flag flip in
a separate CL would be nice if it's necessary to revert. Is that a good plan?

https://codereview.chromium.org/15908002/diff/214003/chrome/browser/component...
chrome/browser/component_updater/component_updater_configurator.cc:107:
pings_enabled_ = cmdline->HasSwitch(switches::kEnableComponentUpdatePings);
On 2013/06/18 22:45:52, cpu wrote:
> probably best if we can use the existing scheme, it uses a single switch
> --component-updater-debug= comma separated values here, instead of top level
> chrome switches, maybe we can shorten it to just be component-updater
> 
> --component-updater=use-deltas,fast-update.
> 
> See lines 32, to 38.
> 

Hm, that's an interesting idea.

I have two concerns:
1) I'm worried about changing --component-updater-debug to something else
because I'm not sure what testing (automated or manual) relies on the flag, and
finding all the manual test playbooks seems like a daunting task.
2) My understanding was that the switches in the switches file are automatically
documented, whereas nested flags (fast-update and so on) seem pretty difficult
to automatically document - does this mean that having the highly-visible
standalone switches is preferred?

Let me know what you think.

[Sorin Jianu, 2013-06-19 00:29:10]

What is still pending is the ping decoupling work. The other items were done or
responded to.

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
File chrome/browser/component_updater/component_updater_service.cc (left):

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:453: // |to|
statatus and returns how many have been changed.
On 2013/06/18 20:50:41, waffles wrote:
> On 2013/06/18 20:46:39, sorinj wrote:
> > On 2013/06/18 17:02:06, cpu wrote:
> > > typo here :)
> > 
> > What is the typo? I am sorry, we might have fixed this already, I can't see
> what
> > is wrong.
> 
> Sorry for the confusion, I fixed that one. :)

Done.

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

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:985: crx->status =
CrxUpdateItem::kNoUpdate;
On 2013/06/18 22:02:36, cpu wrote:
> On 2013/06/18 20:46:39, sorinj wrote:
> > On 2013/06/18 17:02:06, cpu wrote:
> > > Use ChangeItemStatus
> > 
> > ChangeItemStatus changes the state of all items (from, to). We only want to
> > change the state for this item.
> 
> Correct. The way it is used like in line 802 is because there can be only one
> component in kDownloading, this is important for the operation of the
component
> updater of having only one component in flight. It is sort of a self
discipline
> to use when it can be used.
> 

Done by using ChangeItemStatus throughout this function.

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:986:
config_->OnEvent(Configurator::kNetworkError, CrxIdtoUMAId(context->id));
On 2013/06/18 22:18:25, cpu wrote:
> On 2013/06/18 20:46:39, sorinj wrote:
> > On 2013/06/18 17:02:06, cpu wrote:
> > > and here we get to the fun part, as you can see there is an existing error
> > > reporting framework. I don't see a compelling reason to have two of them.
> > > 
> > > The choice of moving the error reporting out of the core and into the
> > > configurator was intentional. How errors are processed and reported is not
> > > 'core' to this code.
> > > 
> > > I am willing to let go of UMA, even though that is how chrome manages
> > reporting
> > > and use your reporting on the view that is omaha folks monitoring it, but
in
> > > turn I want all of it out into the configurator. 
> > > 
> > > Ideally the only code you will see here is config_=>OnEvent(....).
> > 
> > Let's discuss this and see what is reasonable to do. Let's see what we want
to
> > do and what we agree on so far:
> > * we want to send completion pings for all users to monitor the quality of
the
> > update service
> > * we agree that Omaha will do this and we can do outside UMA, just as we do
it
> > for Chrome updates.
> > * we agree that the error handling and reporting should be decoupled from
the
> > rest of the code.
> > 
> > We are happy to follow your advice and move the error handling and the
pinging
> > logic out in the configurator. The state that needs to be sent to the server
> > must be encapsulated in some object. Right now that object is the
> CrxUpdateItem.
> > At this time, I see three options:
> > 1. Introduce a dependency in ChromeConfigurator on the CrxUpdateItem so that
> an
> > item can be passed as an argument to the OnEvent.
> > 2. Pass the state information in some other kind of object which becomes an
> > argument to OnEvent.
> > 2. Pass the state information as standalone parameters to OnEvent, as in:
> > config_->OnEvent(..., id, diff_error_code, error_code, version,
> > previous_version,....)
> > Which way would you recommend we go? 
> 
> Neat to have agreement here.
> 
> I like 2a or 2b. If it was me I would try 2b and see how it looks, I would try
> that first because then in the configurator I can maintain my own vector of
> UpdateItemStatus or whatever name. 
> 
> Basically lets nuke the existing UMA reporting and lets do your with the least
> coupling that we can do.
> 
> I would also be interesting to have this pinging decoupled from the update
> scheduler, like say only once every 8 hours or so bulk reporting of the whole
> thing.
> 
> 

I am mailing the comments and uploading the fixes I have so far except this one,
which I am working on it at the moment.

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/15908002/diff/194002/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.h:55: // Only |name|
is optional. |pk_hash| is the SHA256 hash of the component's
On 2013/06/18 22:07:34, cpu wrote:
> On 2013/06/18 20:46:39, sorinj wrote:
> > On 2013/06/18 17:02:06, cpu wrote:
> > > "only name is optional"
> > 
> > I am sorry I don't understand the comment.
> 
> If fingerprint optional? if so then comment needs to be updated.
> 

Done.

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

https://codereview.chromium.org/15908002/diff/214003/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:964:
base::Unretained(this)));
On 2013/06/18 22:32:37, cpu wrote:
> this postask is probably wrong. The design assumes ProcessPendingItems is only
> called from the ScheduleNextRun. The idea here it to have only the timer drive
> the state transition of the UI-thread bound items, which is pretty much all
> updater state.
> 
> Use posttask here only to transition to a from the the file thread. See the
long
> comment starting at line 339 for more background.
> 
> I can see now why ChangeItemStatus was weird to use here. Hopefully now it
makes
> more sense.

Used ScheduleNextRun(true); to loop back into the state machine and try the full
download.

https://codereview.chromium.org/15908002/diff/214003/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:1040:
BrowserThread::PostTask(
On 2013/06/18 22:32:37, cpu wrote:
> same comment here as of  964
Used ScheduleNextRun(true); to loop back into the state machine and try the full
install.

[grt (UTC plus 2), 2013-06-19 14:45:36]

drive-by chrome/installer comments

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/setup/s...
File chrome/installer/setup/setup_util.cc (right):

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.cc:96: if (src.empty() || patch.empty() ||
dest.empty()) {
nit: no braces for single-line conditionals like this

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.cc:106: if (exit_code) {
  LOG_IF(ERROR, exit_code) ...
(see comment below about always logging)

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.cc:120: if (src.empty() || patch.empty() ||
dest.empty()) {
nit: no braces for single-line conditionals like this

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.cc:127: if (exit_code) {
  LOG_IF(ERROR, exit_code) ...
(see comment below about always logging)

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.cc:172: VLOG(1) << "Failed to apply patch " <<
patch.value()
what do you think about surfacing this in the log always, rather than only when
--verbose-logging is specified? consider:
  LOG_IF(WARNING, binary_patch_status != OK)
or even ERROR if this should really never happen.

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/setup/s...
File chrome/installer/setup/setup_util.h (right):

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.h:44: // Applies a patch file to source file
using Courgette.
please document return value.

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.h:50: // Courgette's flavor of bsdiff.
please document return value.

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/util/ut...
File chrome/installer/util/util_constants.h (right):

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/util/ut...
chrome/installer/util/util_constants.h:184: extern const char kCourgette[];
consider moving kCourgette and kBsdiff out of namespace switches since they are
values to the --patch switch rather than switches themselves.

[Sorin Jianu, 2013-06-19 17:14:44]

Thank you Greg. All done.

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/setup/s...
File chrome/installer/setup/setup_util.cc (right):

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.cc:96: if (src.empty() || patch.empty() ||
dest.empty()) {
On 2013/06/19 14:45:36, grt wrote:
> nit: no braces for single-line conditionals like this

Done.

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.cc:106: if (exit_code) {
On 2013/06/19 14:45:36, grt wrote:
>   LOG_IF(ERROR, exit_code) ...
> (see comment below about always logging)

Done.

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.cc:120: if (src.empty() || patch.empty() ||
dest.empty()) {
On 2013/06/19 14:45:36, grt wrote:
> nit: no braces for single-line conditionals like this

Done.

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.cc:127: if (exit_code) {
On 2013/06/19 14:45:36, grt wrote:
>   LOG_IF(ERROR, exit_code) ...
> (see comment below about always logging)

Done.

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.cc:172: VLOG(1) << "Failed to apply patch " <<
patch.value()
On 2013/06/19 14:45:36, grt wrote:
> what do you think about surfacing this in the log always, rather than only
when
> --verbose-logging is specified? consider:
>   LOG_IF(WARNING, binary_patch_status != OK)
> or even ERROR if this should really never happen.

This looks like a logging bug, since I was logging the error all the time.
Fixed.

I have to know more about the logging policy in Chrome to give you an informed
answer. For the moment, I will mechanically follow your advice and change the
log statements.

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/setup/s...
File chrome/installer/setup/setup_util.h (right):

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.h:44: // Applies a patch file to source file
using Courgette.
On 2013/06/19 14:45:36, grt wrote:
> please document return value.

Done.

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.h:50: // Courgette's flavor of bsdiff.
On 2013/06/19 14:45:36, grt wrote:
> please document return value.

Done.

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/util/ut...
File chrome/installer/util/util_constants.h (right):

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/util/ut...
chrome/installer/util/util_constants.h:184: extern const char kCourgette[];
On 2013/06/19 14:45:36, grt wrote:
> consider moving kCourgette and kBsdiff out of namespace switches since they
are
> values to the --patch switch rather than switches themselves.

Done.

[grt (UTC plus 2), 2013-06-19 20:17:38]

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/setup/s...
File chrome/installer/setup/setup_util.cc (right):

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.cc:172: VLOG(1) << "Failed to apply patch " <<
patch.value()
On 2013/06/19 17:14:45, sorinj wrote:
> On 2013/06/19 14:45:36, grt wrote:
> > what do you think about surfacing this in the log always, rather than only
> when
> > --verbose-logging is specified? consider:
> >   LOG_IF(WARNING, binary_patch_status != OK)
> > or even ERROR if this should really never happen.
> 
> This looks like a logging bug, since I was logging the error all the time.
> Fixed.
> 
> I have to know more about the logging policy in Chrome to give you an informed
> answer. For the moment, I will mechanically follow your advice and change the
> log statements.

Do you think it's useful to find these log message after the fact (i.e., ask a
user to send in their chrome_installer.log file after they've reported things
not working properly), or only while a component is being updated (by way of
sawbuck)? VLOG(1) is good for the latter, but not for the former. If you want to
collect logs from customers, then I suggest harmonizing on either LOG at WARNING
if the conditions leading to them aren't surprising, or LOG at ERROR if they
truly are errors indicating something is wrong.

https://codereview.chromium.org/15908002/diff/233001/chrome/installer/setup/s...
File chrome/installer/setup/setup_util.h (right):

https://codereview.chromium.org/15908002/diff/233001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.h:44: // Applies a patch file to source file
using Courgette. Returns C_OK(1) in
is this correct? i had thought it returns a process exit code, which will be 0
if no error, or kCourgetteErrorOffset + a courgette error code.

https://codereview.chromium.org/15908002/diff/233001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.h:52: // Courgette's flavor of bsdiff. Returns
OK(0) in case of success, or another
similar comment to above

[Sorin Jianu, 2013-06-19 21:05:53]

Thank you Greg. Apparently, I can't comment on my own code. Fixed everything.
Please take a look at the logging changes in this patchset.

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/setup/s...
File chrome/installer/setup/setup_util.cc (right):

https://codereview.chromium.org/15908002/diff/226001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.cc:172: VLOG(1) << "Failed to apply patch " <<
patch.value()
On 2013/06/19 20:17:39, grt wrote:
> On 2013/06/19 17:14:45, sorinj wrote:
> > On 2013/06/19 14:45:36, grt wrote:
> > > what do you think about surfacing this in the log always, rather than only
> > when
> > > --verbose-logging is specified? consider:
> > >   LOG_IF(WARNING, binary_patch_status != OK)
> > > or even ERROR if this should really never happen.
> > 
> > This looks like a logging bug, since I was logging the error all the time.
> > Fixed.
> > 
> > I have to know more about the logging policy in Chrome to give you an
informed
> > answer. For the moment, I will mechanically follow your advice and change
the
> > log statements.
> 
> Do you think it's useful to find these log message after the fact (i.e., ask a
> user to send in their chrome_installer.log file after they've reported things
> not working properly), or only while a component is being updated (by way of
> sawbuck)? VLOG(1) is good for the latter, but not for the former. If you want
to
> collect logs from customers, then I suggest harmonizing on either LOG at
WARNING
> if the conditions leading to them aren't surprising, or LOG at ERROR if they
> truly are errors indicating something is wrong.

Thank you, please tell me if this is what you have in mind.

https://codereview.chromium.org/15908002/diff/233001/chrome/installer/setup/s...
File chrome/installer/setup/setup_util.h (right):

https://codereview.chromium.org/15908002/diff/233001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.h:44: // Applies a patch file to source file
using Courgette. Returns C_OK(1) in
On 2013/06/19 20:17:39, grt wrote:
> is this correct? i had thought it returns a process exit code, which will be 0
> if no error, or kCourgetteErrorOffset + a courgette error code.

Done.

https://codereview.chromium.org/15908002/diff/233001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.h:52: // Courgette's flavor of bsdiff. Returns
OK(0) in case of success, or another
On 2013/06/19 20:17:39, grt wrote:
> similar comment to above

Done.

[cpu_(ooo_6.6-7.5), 2013-06-20 00:13:26]

On 2013/06/19 00:02:10, waffles wrote:
>
https://codereview.chromium.org/15908002/diff/214003/chrome/browser/component...
> File chrome/browser/component_updater/component_updater_configurator.cc
(right):
> 
>
https://codereview.chromium.org/15908002/diff/214003/chrome/browser/component...
> chrome/browser/component_updater/component_updater_configurator.cc:97:
> pings_enabled_(false),
> On 2013/06/18 22:45:52, cpu wrote:
> > do we want pings disabled by default?
> > 
> > Is the plan to have all users ping, eventually? if so it is best to start in
> > 'on' in canary since the smaller population has less change of DoS ing us.
> 
> I'm planning on submitting another changelist almost immediately after this
one
> goes in to flip the flags to be on by default. I thought doing the flag flip
in
> a separate CL would be nice if it's necessary to revert. Is that a good plan?
> 
>
https://codereview.chromium.org/15908002/diff/214003/chrome/browser/component...
> chrome/browser/component_updater/component_updater_configurator.cc:107:
> pings_enabled_ = cmdline->HasSwitch(switches::kEnableComponentUpdatePings);
> On 2013/06/18 22:45:52, cpu wrote:
> > probably best if we can use the existing scheme, it uses a single switch
> > --component-updater-debug= comma separated values here, instead of top level
> > chrome switches, maybe we can shorten it to just be component-updater
> > 
> > --component-updater=use-deltas,fast-update.
> > 
> > See lines 32, to 38.
> > 
> 
> Hm, that's an interesting idea.
> 
> I have two concerns:
> 1) I'm worried about changing --component-updater-debug to something else
> because I'm not sure what testing (automated or manual) relies on the flag,
and
> finding all the manual test playbooks seems like a daunting task.

There is only one playbook and you have to work with them (QA) to help them
update it to test the new functionality.
I pointed QA to sorin so they should be contacting him.

There is no automated testing that uses the flags afaik. And none should be
made, anything
that uses flags can/should be tested with a custom configurator.

> 2) My understanding was that the switches in the switches file are
automatically
> documented, whereas nested flags (fast-update and so on) seem pretty difficult
> to automatically document - does this mean that having the highly-visible
> standalone switches is preferred?

The automated documentation only helps bloggers and tinkerers out there.

> 
> Let me know what you think.

[cpu_(ooo_6.6-7.5), 2013-06-20 00:19:35]

The CL is looking very good, I think we are in the final stretch. Do not
despair.

As for pings or no pings initially that is your call. Fine either way.

I also want to point out that there is no single loging style in the codebase
there are seas of code with many vlogs which I despise and there ares
significant pieces with only spartan CHECKs. In particular follow greg/robert's
advice on setup code and follow the style on CU in the CU files.

[cpu_(ooo_6.6-7.5), 2013-06-20 00:27:47]

https://codereview.chromium.org/15908002/diff/244001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher_win.cc (right):

https://codereview.chromium.org/15908002/diff/244001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_win.cc:89: int exit_code = 0;
I think I made a email comment here to sorin and it is better I put it here as
well. If chrome crashes or is shutdown we are going to live this orphan setup
process behind. It is probably best if we create a job object that kills it, use
JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE.

[waffles, 2013-06-20 04:53:32]

https://codereview.chromium.org/15908002/diff/214003/chrome/browser/component...
File chrome/browser/component_updater/component_updater_configurator.cc (right):

https://codereview.chromium.org/15908002/diff/214003/chrome/browser/component...
chrome/browser/component_updater/component_updater_configurator.cc:107:
pings_enabled_ = cmdline->HasSwitch(switches::kEnableComponentUpdatePings);
On 2013/06/19 00:02:11, waffles wrote:
> On 2013/06/18 22:45:52, cpu wrote:
> > probably best if we can use the existing scheme, it uses a single switch
> > --component-updater-debug= comma separated values here, instead of top level
> > chrome switches, maybe we can shorten it to just be component-updater
> > 
> > --component-updater=use-deltas,fast-update.
> > 
> > See lines 32, to 38.
> > 
> 
> Hm, that's an interesting idea.
> 
> I have two concerns:
> 1) I'm worried about changing --component-updater-debug to something else
> because I'm not sure what testing (automated or manual) relies on the flag,
and
> finding all the manual test playbooks seems like a daunting task.
> 2) My understanding was that the switches in the switches file are
automatically
> documented, whereas nested flags (fast-update and so on) seem pretty difficult
> to automatically document - does this mean that having the highly-visible
> standalone switches is preferred?
> 
> Let me know what you think.

Done.

[grt (UTC plus 2), 2013-06-20 14:04:14]

lgtm w/ final tweaks below. thanks.

https://codereview.chromium.org/15908002/diff/244001/chrome/installer/setup/s...
File chrome/installer/setup/setup_util.cc (right):

https://codereview.chromium.org/15908002/diff/244001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.cc:93: LOG(WARNING) << "Applying Courgette
patch " << patch.value() << " to file "
Seems like an "installer logging" style guide is in order. :-)

My inclination is for status messages like this to be VLOG(1), and for true
errors to be LOG(ERROR). That in mind, please change this, line 114, and line
133 back to VLOG(1). Apologies for the lack of clarity.

https://codereview.chromium.org/15908002/diff/244001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.cc:97: return
installer::PATCH_INVALID_ARGUMENTS;
nit: only indent two spaces

https://codereview.chromium.org/15908002/diff/244001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.cc:118: return
installer::PATCH_INVALID_ARGUMENTS;
nit: only indent two spaces

[grt (UTC plus 2), 2013-06-20 14:44:33]

lgtm

[Sorin Jianu, 2013-06-20 17:40:14]

https://codereview.chromium.org/15908002/diff/244001/chrome/installer/setup/s...
File chrome/installer/setup/setup_util.cc (right):

https://codereview.chromium.org/15908002/diff/244001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.cc:93: LOG(WARNING) << "Applying Courgette
patch " << patch.value() << " to file "
On 2013/06/20 14:04:14, grt wrote:
> Seems like an "installer logging" style guide is in order. :-)
> 
> My inclination is for status messages like this to be VLOG(1), and for true
> errors to be LOG(ERROR). That in mind, please change this, line 114, and line
> 133 back to VLOG(1). Apologies for the lack of clarity.

Done.

https://codereview.chromium.org/15908002/diff/244001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.cc:97: return
installer::PATCH_INVALID_ARGUMENTS;
On 2013/06/20 14:04:14, grt wrote:
> nit: only indent two spaces

Done.

https://codereview.chromium.org/15908002/diff/244001/chrome/installer/setup/s...
chrome/installer/setup/setup_util.cc:118: return
installer::PATCH_INVALID_ARGUMENTS;
On 2013/06/20 14:04:14, grt wrote:
> nit: only indent two spaces

Done.

[Sorin Jianu, 2013-06-20 19:39:22]

On 2013/06/20 17:40:14, sorinj wrote:
>
https://codereview.chromium.org/15908002/diff/244001/chrome/installer/setup/s...
> File chrome/installer/setup/setup_util.cc (right):
> 
>
https://codereview.chromium.org/15908002/diff/244001/chrome/installer/setup/s...
> chrome/installer/setup/setup_util.cc:93: LOG(WARNING) << "Applying Courgette
> patch " << patch.value() << " to file "
> On 2013/06/20 14:04:14, grt wrote:
> > Seems like an "installer logging" style guide is in order. :-)
> > 
> > My inclination is for status messages like this to be VLOG(1), and for true
> > errors to be LOG(ERROR). That in mind, please change this, line 114, and
line
> > 133 back to VLOG(1). Apologies for the lack of clarity.
> 
> Done.
> 
>
https://codereview.chromium.org/15908002/diff/244001/chrome/installer/setup/s...
> chrome/installer/setup/setup_util.cc:97: return
> installer::PATCH_INVALID_ARGUMENTS;
> On 2013/06/20 14:04:14, grt wrote:
> > nit: only indent two spaces
> 
> Done.
> 
>
https://codereview.chromium.org/15908002/diff/244001/chrome/installer/setup/s...
> chrome/installer/setup/setup_util.cc:118: return
> installer::PATCH_INVALID_ARGUMENTS;
> On 2013/06/20 14:04:14, grt wrote:
> > nit: only indent two spaces
> 
> Done.

We are all set, pings were removed, Carlos could you please take a look asap?

[sra1, 2013-06-20 20:11:37]

Courgette files still lgtm

[cpu_(ooo_6.6-7.5), 2013-06-20 20:40:33]

https://codereview.chromium.org/15908002/diff/276001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher_win.cc (right):

https://codereview.chromium.org/15908002/diff/276001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_win.cc:81: 
so no job object?

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

https://codereview.chromium.org/15908002/diff/276001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:266: GURL
diff_crx_url;
please put some comment somewhere in this file (here or elsewhere) explaining
what the two package hash mean and how they differ from the fingeprints. 

It is a bit unfortunate that is this code we have pk hash and package hash and
fingerprint. I think we should rename one of them. I suggest we rename pk_hash
to pubkey_hash or pkey_hash or something like that.

But before that, what is the value in this CL of the package hash? I see where
we get it but I don't see it put to use. Maybe it should follow the same fate as
the items of line 282 - 285?

https://codereview.chromium.org/15908002/diff/276001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:281: // The error
information for full and differential updates.
please remove all the members that are used only for logging or pinging that was
removed, I believe most of the ones below.

https://codereview.chromium.org/15908002/diff/276001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:629: item->next_fp
= "";
.clear()

[Sorin Jianu, 2013-06-20 21:34:54]

Fixed. PTAL. Thank you!

https://codereview.chromium.org/15908002/diff/276001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher_win.cc (right):

https://codereview.chromium.org/15908002/diff/276001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_win.cc:81: 
On 2013/06/20 20:40:34, cpu wrote:
> so no job object?

I prefer not to deal with jobs at this time due to the added complexity of
handling the jobs.

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

https://codereview.chromium.org/15908002/diff/276001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:266: GURL
diff_crx_url;
On 2013/06/20 20:40:34, cpu wrote:
> please put some comment somewhere in this file (here or elsewhere) explaining
> what the two package hash mean and how they differ from the fingeprints. 
> 
> It is a bit unfortunate that is this code we have pk hash and package hash and
> fingerprint. I think we should rename one of them. I suggest we rename pk_hash
> to pubkey_hash or pkey_hash or something like that.
> 
> But before that, what is the value in this CL of the package hash? I see where
> we get it but I don't see it put to use. Maybe it should follow the same fate
as
> the items of line 282 - 285?
> 

Members removed. At Omaha we like the idea of having checks to verify the data
integrity of the packet. Even if the CRX itself is tamper resistent, having a
checksum for the whole download helps validating we are actually getting what we
were supposed to get.

https://codereview.chromium.org/15908002/diff/276001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:281: // The error
information for full and differential updates.
On 2013/06/20 20:40:34, cpu wrote:
> please remove all the members that are used only for logging or pinging that
was
> removed, I believe most of the ones below.

I have removed the error members.

These members are very useful for debugging since they contain the result of the
update. We will have to introduce the ping code in the next CL and we will need
these data members. 

Please allow me to ask this question for the sake of understanding the current
design and avoid future friction. Why is there a concern with having members in
an object which purpose is to maintain the state of the crx as the crx is run by
the state machine?

https://codereview.chromium.org/15908002/diff/276001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:629: item->next_fp
= "";
On 2013/06/20 20:40:34, cpu wrote:
> .clear()

Done.

[cpu_(ooo_6.6-7.5), 2013-06-20 22:30:53]

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

https://codereview.chromium.org/15908002/diff/285001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:235: bool
is_update_available;
how is different from status = CrxUpdateItem::kCanUpdate

Looks like we can remove it as we already have this information.

[cpu_(ooo_6.6-7.5), 2013-06-20 22:32:11]

https://codereview.chromium.org/15908002/diff/276001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher_win.cc (right):

https://codereview.chromium.org/15908002/diff/276001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_win.cc:81: 
On 2013/06/20 21:34:55, sorinj wrote:
> On 2013/06/20 20:40:34, cpu wrote:
> > so no job object?
> 
> I prefer not to deal with jobs at this time due to the added complexity of
> handling the jobs.
> 

I think you might have a bad time testing this on the bots if we leave zombine
setups running. But we can address this on the next CL.

[cpu_(ooo_6.6-7.5), 2013-06-20 22:34:24]

https://codereview.chromium.org/15908002/diff/285001/chrome/browser/component...
File chrome/browser/component_updater/test/url_request_post_interceptor.cc
(right):

https://codereview.chromium.org/15908002/diff/285001/chrome/browser/component...
chrome/browser/component_updater/test/url_request_post_interceptor.cc:52: 
pingMockJob and related code, I think is not needed in this CL anymore.

[cpu_(ooo_6.6-7.5), 2013-06-20 22:39:22]

That is all I have!  please address the comment above.

lgtm

I started a set of trybots on patchset 25, please monitor for issues.

I can also CQ the final patchset. Let me know

[waffles, 2013-06-20 22:55:28]

lgtm

Not sure when I got added as a reviewer; but it LGTM.

[Sorin Jianu, 2013-06-20 23:51:58]

Thank you! All changes post LGTM uploaded.

https://codereview.chromium.org/15908002/diff/276001/chrome/browser/component...
File chrome/browser/component_updater/component_patcher_win.cc (right):

https://codereview.chromium.org/15908002/diff/276001/chrome/browser/component...
chrome/browser/component_updater/component_patcher_win.cc:81: 
On 2013/06/20 22:32:12, cpu wrote:
> On 2013/06/20 21:34:55, sorinj wrote:
> > On 2013/06/20 20:40:34, cpu wrote:
> > > so no job object?
> > 
> > I prefer not to deal with jobs at this time due to the added complexity of
> > handling the jobs.
> > 
> 
> I think you might have a bad time testing this on the bots if we leave zombine
> setups running. But we can address this on the next CL.
> 
> 

I agree. ty.

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

https://codereview.chromium.org/15908002/diff/285001/chrome/browser/component...
chrome/browser/component_updater/component_updater_service.cc:235: bool
is_update_available;
On 2013/06/20 22:30:54, cpu wrote:
> how is different from status = CrxUpdateItem::kCanUpdate
> 
> Looks like we can remove it as we already have this information.

removed.

https://codereview.chromium.org/15908002/diff/285001/chrome/browser/component...
File chrome/browser/component_updater/test/url_request_post_interceptor.cc
(right):

https://codereview.chromium.org/15908002/diff/285001/chrome/browser/component...
chrome/browser/component_updater/test/url_request_post_interceptor.cc:52: 
On 2013/06/20 22:34:25, cpu wrote:
> pingMockJob and related code, I think is not needed in this CL anymore.

Done.

[cpu_(ooo_6.6-7.5), 2013-06-21 00:06:34]

set CQ bit, I looked at trybots and I am not sure they are running the tests.
You might want to check that.

lgtm again

[commit-bot: I haz the power, 2013-06-21 00:06:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sorin@chromium.org/15908002/288001

[commit-bot: I haz the power, 2013-06-21 00:20:30]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[sra1, 2013-06-21 00:33:36]

lgtm

[sra%chromium.org, 2013-06-21 00:49:22]

lgtm

[dgarrett, 2013-06-21 00:53:29]

The new wrapper in Courgette looks handy.

LGTM

[commit-bot: I haz the power, 2013-06-21 00:54:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sorin@chromium.org/15908002/288001

[tommi (sloooow) - chröme, 2013-06-21 02:32:47]

Rubberstamp lgtm

[commit-bot: I haz the power, 2013-06-21 14:20:17]

Change committed as 207805

[Sorin Jianu, 2013-06-21 17:11:45]

Carlos, PTAL.

The patcher unit tests only run on Windows, to avoid a crash that happens on mac
debug while running the Courgette unit tests.

[commit-bot: I haz the power, 2013-06-21 17:25:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sorin@chromium.org/15908002/272004

[commit-bot: I haz the power, 2013-06-21 17:44:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sorin@chromium.org/15908002/152003

[cpu_(ooo_6.6-7.5), 2013-06-21 17:46:44]

lgtm

[commit-bot: I haz the power, 2013-06-21 20:41:39]

Change committed as 207917