Componentize component_updater: Decouple in-process DeltaUpdateOp from out-of-process version.

Componentize component_updater: Decouple in-process DeltaUpdateOp from out-of-process version.

The in-process DeltaUpdateOps don't require content/ and can be eventually used on iOS, whereas the out-of-process version needs to stay in chrome/.

Part 1d in design doc here: https://docs.google.com/document/d/1F76yNZCnPnGzgNXhI-sCFKlXulwx_s_OKTGbJS5NhbA/edit?usp=sharing

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

[tommycli, 2014-07-24 20:31:16]

PTAL: This enables the eventual move of the in-process patcher operations to
components/

[tommycli, 2014-07-24 20:34:24]

On 2014/07/24 20:31:16, tommycli wrote:
> PTAL: This enables the eventual move of the in-process patcher operations to
> components/

BTW, the insertions/deletions count is wrong because I turned similarity down to
allow for comparison. The real numbers are:

 16 files changed, 375 insertions(+), 314 deletions(-)

[blundell, 2014-07-25 07:40:42]

Hi Tommy,

Is there a design outlining the strategy of the changes here?

Thanks,

Colin

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

https://codereview.chromium.org/420503002/diff/40001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:63: DeltaUpdateOpFactory*
delta_update_op_factory,
If this object takes ownership of the factory, it should be passed in as a
scoped_ptr.

[tommycli, 2014-07-25 18:08:47]

Overall strategy in Design doc section 1d:
https://docs.google.com/document/d/1F76yNZCnPnGzgNXhI-sCFKlXulwx_s_OKTGbJS5Nh...


Mostly based on a conversation we had over IM a few weeks back.

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

https://codereview.chromium.org/420503002/diff/40001/chrome/browser/component...
chrome/browser/component_updater/component_patcher.h:63: DeltaUpdateOpFactory*
delta_update_op_factory,
On 2014/07/25 07:40:42, blundell (OOO until August 6) wrote:
> If this object takes ownership of the factory, it should be passed in as a
> scoped_ptr.

I changed all the usages of this class to use scoped_ptr

[Sorin Jianu, 2014-07-28 17:08:08]

Thank you Tommy. I'd like to offer an idea about how to deal with the op
factory. I don't have a strong opinion about this idea, it is ok to drop it if
it is not working.

I would prefer if the functions that create DeltaUpdateOpFactory were unified. I
think that the original code, with a bool |in_process_| was correct. I would
want 
component_patcher_operation.h to declare a function
CreateDeltaUpdateOpFactory(bool in_process), which will be implemented in such a
way that both in-process and out of process factories can be created in Chrome
but only the in-process factory ca

[Sorin Jianu, 2014-07-28 17:14:48]

Thank you Tommy. I'd like to offer an idea about how to deal with the op
factory. I don't have a strong opinion about this idea, it is ok to drop it if
it is not working.

I would prefer if the functions that create DeltaUpdateOpFactory were unified. I
think that the original code, with a bool |in_process_| was correct. I would
want 
component_patcher_operation.h to declare a function
CreateDeltaUpdateOpFactory(bool in_process), which will be implemented in such a
way that both in-process and out of process factories can be created in Chrome
but only the in-process factory can be created on iOS. This way, the caller does
not have to include different header files for different factories, and it
allows us to switch the type of factory in the Chrome component updater easily
(if we need to).

In other words, the interface to component patcher operaration should be
component_patcher_operation.h plus bool in_process, and that is what the clients
should be calling. The in-process/out-of-process distinction is only relevant to
the implementor of the factories.

[tommycli, 2014-07-28 18:09:13]

On 2014/07/28 17:14:48, Sorin Jianu wrote:
> Thank you Tommy. I'd like to offer an idea about how to deal with the op
> factory. I don't have a strong opinion about this idea, it is ok to drop it if
> it is not working.
> 
> I would prefer if the functions that create DeltaUpdateOpFactory were unified.
I
> think that the original code, with a bool |in_process_| was correct. I would
> want 
> component_patcher_operation.h to declare a function
> CreateDeltaUpdateOpFactory(bool in_process), which will be implemented in such
a
> way that both in-process and out of process factories can be created in Chrome
> but only the in-process factory can be created on iOS. This way, the caller
does
> not have to include different header files for different factories, and it
> allows us to switch the type of factory in the Chrome component updater easily
> (if we need to).
> 
> In other words, the interface to component patcher operaration should be
> component_patcher_operation.h plus bool in_process, and that is what the
clients
> should be calling. The in-process/out-of-process distinction is only relevant
to
> the implementor of the factories.

I replaced the 'bool InProcess()' method in the configurator with a method that
calls the appropriate factory creation method. Are you recommending that we put
the 'bool InProcess()' method back? Would the body of
CreateDeltaUpdateOpFactory(bool in_process) be implemented using #ifdef
statements?

Thanks!

[tommycli, 2014-07-28 18:11:15]

On 2014/07/28 18:09:13, tommycli wrote:
> On 2014/07/28 17:14:48, Sorin Jianu wrote:
> > Thank you Tommy. I'd like to offer an idea about how to deal with the op
> > factory. I don't have a strong opinion about this idea, it is ok to drop it
if
> > it is not working.
> > 
> > I would prefer if the functions that create DeltaUpdateOpFactory were
unified.
> I
> > think that the original code, with a bool |in_process_| was correct. I would
> > want 
> > component_patcher_operation.h to declare a function
> > CreateDeltaUpdateOpFactory(bool in_process), which will be implemented in
such
> a
> > way that both in-process and out of process factories can be created in
Chrome
> > but only the in-process factory can be created on iOS. This way, the caller
> does
> > not have to include different header files for different factories, and it
> > allows us to switch the type of factory in the Chrome component updater
easily
> > (if we need to).
> > 
> > In other words, the interface to component patcher operaration should be
> > component_patcher_operation.h plus bool in_process, and that is what the
> clients
> > should be calling. The in-process/out-of-process distinction is only
relevant
> to
> > the implementor of the factories.
> 
> I replaced the 'bool InProcess()' method in the configurator with a method
that
> calls the appropriate factory creation method. Are you recommending that we
put
> the 'bool InProcess()' method back? Would the body of
> CreateDeltaUpdateOpFactory(bool in_process) be implemented using #ifdef
> statements?
> 
> Thanks!

I split the CreateDeltaUpdateOpFactory(bool in_process) into two methods because
the in-process on can be in components/component_updater, and the out-of-process
one needs to be in chrome/.

[Sorin Jianu, 2014-07-28 18:16:37]

Tommy, I was saying that Chrome needs the code for both in-process and
out-of-process patching.

[tommycli, 2014-07-28 18:27:37]

On 2014/07/28 18:16:37, Sorin Jianu wrote:
> Tommy, I was saying that Chrome needs the code for both in-process and
> out-of-process patching.

Oh... I think I understand you now. I will give that a shot and get back to you.
Thanks!

[Sorin Jianu, 2014-07-28 18:32:31]

On 2014/07/28 18:09:13, tommycli wrote:
> On 2014/07/28 17:14:48, Sorin Jianu wrote:
> > Thank you Tommy. I'd like to offer an idea about how to deal with the op
> > factory. I don't have a strong opinion about this idea, it is ok to drop it
if
> > it is not working.
> > 
> > I would prefer if the functions that create DeltaUpdateOpFactory were
unified.
> I
> > think that the original code, with a bool |in_process_| was correct. I would
> > want 
> > component_patcher_operation.h to declare a function
> > CreateDeltaUpdateOpFactory(bool in_process), which will be implemented in
such
> a
> > way that both in-process and out of process factories can be created in
Chrome
> > but only the in-process factory can be created on iOS. This way, the caller
> does
> > not have to include different header files for different factories, and it
> > allows us to switch the type of factory in the Chrome component updater
easily
> > (if we need to).
> > 
> > In other words, the interface to component patcher operaration should be
> > component_patcher_operation.h plus bool in_process, and that is what the
> clients
> > should be calling. The in-process/out-of-process distinction is only
relevant
> to
> > the implementor of the factories.
> 
> I replaced the 'bool InProcess()' method in the configurator with a method
that
> calls the appropriate factory creation method. Are you recommending that we
put
> the 'bool InProcess()' method back? Would the body of
> CreateDeltaUpdateOpFactory(bool in_process) be implemented using #ifdef
> statements?
> 
> Thanks!

I would imagine that the iOS implementation would provide a factory creation
function that only returns the in-proc op factory.
And consequently, the configurator for iOs would call 

As I was saying, I don't have a strong opinion of what I've suggested. My
concern is that with the current CL, details about in-process vs. out-of process
spill out to callers, as seen by them including different header files.

waffles@ wrote this code, his opinion matters more than mine.

[tommycli, 2014-07-28 20:04:19]

sorin: I made some changes based on your suggestion. See if this looks better to
you. Thanks!

[waffles, 2014-07-29 23:54:09]

My major concern with this code is that it seems wrong to inject actual delta
patch operations (or the factory for them) into the component updater service.
Delta update operations are a feature of the component updater itself - they
just happen to depend upon out-of-process support. What we should instead be
injecting is the out-of-process support, or rather, some method by which we do
some risky, asynchronous operation. (The injector can decide if that is actually
out-of-process or not.)

To me, it's highly similar to the recent threading changes. Rather than inject a
ComponentUnpacker, we inject a TaskRunner. It would be nice to accomplish
something similar here, though I think the challenges are greater.

Tommy, do you think there is a way to accomplish such a thing?

[tommycli, 2014-07-30 00:26:28]

On 2014/07/29 23:54:09, waffles wrote:
> My major concern with this code is that it seems wrong to inject actual delta
> patch operations (or the factory for them) into the component updater service.
> Delta update operations are a feature of the component updater itself - they
> just happen to depend upon out-of-process support. What we should instead be
> injecting is the out-of-process support, or rather, some method by which we do
> some risky, asynchronous operation. (The injector can decide if that is
actually
> out-of-process or not.)
> 
> To me, it's highly similar to the recent threading changes. Rather than inject
a
> ComponentUnpacker, we inject a TaskRunner. It would be nice to accomplish
> something similar here, though I think the challenges are greater.
> 
> Tommy, do you think there is a way to accomplish such a thing?

So you want to keep all the DeltaUpdateOp knowledge in one place, and pass in a
a generic RiskyOperationRunner... This RiskyOperationRunner would have to be
able to run an arbitrary base::Callback on the utility process.

Am I understanding you correctly?

Currently, the utility process accepts a fixed set of messages and runs a fixed
set of corresponding operations [1]. I agree that it's a leaky abstraction and
it would be cool if the utility process could run arbitrary operations...

I'm not sure if anything like this currently exists. I will have to think about
this overnight.

Let me know if you come up with any insights.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/utility/chr...

[tommycli, 2014-07-30 18:02:53]

On 2014/07/30 00:26:28, tommycli wrote:
> On 2014/07/29 23:54:09, waffles wrote:
> > My major concern with this code is that it seems wrong to inject actual
delta
> > patch operations (or the factory for them) into the component updater
service.
> > Delta update operations are a feature of the component updater itself - they
> > just happen to depend upon out-of-process support. What we should instead be
> > injecting is the out-of-process support, or rather, some method by which we
do
> > some risky, asynchronous operation. (The injector can decide if that is
> actually
> > out-of-process or not.)
> > 
> > To me, it's highly similar to the recent threading changes. Rather than
inject
> a
> > ComponentUnpacker, we inject a TaskRunner. It would be nice to accomplish
> > something similar here, though I think the challenges are greater.
> > 
> > Tommy, do you think there is a way to accomplish such a thing?
> 
> So you want to keep all the DeltaUpdateOp knowledge in one place, and pass in
a
> a generic RiskyOperationRunner... This RiskyOperationRunner would have to be
> able to run an arbitrary base::Callback on the utility process.
> 
> Am I understanding you correctly?
> 
> Currently, the utility process accepts a fixed set of messages and runs a
fixed
> set of corresponding operations [1]. I agree that it's a leaky abstraction and
> it would be cool if the utility process could run arbitrary operations...
> 
> I'm not sure if anything like this currently exists. I will have to think
about
> this overnight.
> 
> Let me know if you come up with any insights.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/utility/chr...

waffles: Okay I thought about it.

There's currently no way to run an arbitrary callback on the utility process.
All utility process operations have an explicit IPC message and a handler in
ChromeContentUtilityClient. So there's no way to inject something that just has
knowledge of out of process operations.

Whatever we inject needs to have knowledge of how to run a DeltaUpdateOpPatch
out of process.

Currently I'm injecting a DeltaUpdateOpFactory that has knowledge of how to do
all the DeltaUpdateOps. If that bothers you, I could narrow the scope to have it
only be a DeltaUpdateOpPatchFactory, but I don't think I can narrow the scope
beyond that.

What do you think?

[waffles, 2014-07-30 22:46:11]

On 2014/07/30 18:02:53, tommycli wrote:
> On 2014/07/30 00:26:28, tommycli wrote:
> > On 2014/07/29 23:54:09, waffles wrote:
> > > My major concern with this code is that it seems wrong to inject actual
> delta
> > > patch operations (or the factory for them) into the component updater
> service.
> > > Delta update operations are a feature of the component updater itself -
they
> > > just happen to depend upon out-of-process support. What we should instead
be
> > > injecting is the out-of-process support, or rather, some method by which
we
> do
> > > some risky, asynchronous operation. (The injector can decide if that is
> > actually
> > > out-of-process or not.)
> > > 
> > > To me, it's highly similar to the recent threading changes. Rather than
> inject
> > a
> > > ComponentUnpacker, we inject a TaskRunner. It would be nice to accomplish
> > > something similar here, though I think the challenges are greater.
> > > 
> > > Tommy, do you think there is a way to accomplish such a thing?
> > 
> > So you want to keep all the DeltaUpdateOp knowledge in one place, and pass
in
> a
> > a generic RiskyOperationRunner... This RiskyOperationRunner would have to be
> > able to run an arbitrary base::Callback on the utility process.
> > 
> > Am I understanding you correctly?
> > 
> > Currently, the utility process accepts a fixed set of messages and runs a
> fixed
> > set of corresponding operations [1]. I agree that it's a leaky abstraction
and
> > it would be cool if the utility process could run arbitrary operations...
> > 
> > I'm not sure if anything like this currently exists. I will have to think
> about
> > this overnight.
> > 
> > Let me know if you come up with any insights.
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/utility/chr...
> 
> waffles: Okay I thought about it.
> 
> There's currently no way to run an arbitrary callback on the utility process.
> All utility process operations have an explicit IPC message and a handler in
> ChromeContentUtilityClient. So there's no way to inject something that just
has
> knowledge of out of process operations.
> 
> Whatever we inject needs to have knowledge of how to run a DeltaUpdateOpPatch
> out of process.
> 
> Currently I'm injecting a DeltaUpdateOpFactory that has knowledge of how to do
> all the DeltaUpdateOps. If that bothers you, I could narrow the scope to have
it
> only be a DeltaUpdateOpPatchFactory, but I don't think I can narrow the scope
> beyond that.
> 
> What do you think?

Yeah, I agree that there's no good way to run an arbitrary callback on the
utility process.

Maybe we should inject an (optional) OutOfProcessFilePatcher instead of a
DeltaUpdateOpFactory.

I'm wondering about the merits of injecting an object that can look up the
appropriate IPC messages, know how to contain the UtilityProcessHostClient, and
handle all that (basically, any dependency the DeltaUpdateOpPatchOutOfProcess
has on non-component code). Obviously, because there's no good way to run an
arbitrary callback, it would have to know a little about the delta update
operation; e.g. it has to know enough to know which IPC messages to use and what
responses to expect. But, it doesn't have to know details like how to parse the
arguments, which error code offsets to apply, etc. So it might wind up having
some method like f(x, y) where |x| is kCourgette or kBsdiff and |y| is a
callback to call with an error/success code.

On the other hand, there isn't much to a DeltaUpdateOpPatchOutOfProcess that
isn't in an OutOfProcessFilePatcher (just the error code offsets and argument
parsing, I think) so maybe there's not a lot of practical gain here. But then at
least the code left in /chrome more closely matches the code we have to leave in
/chrome/utility anyways. (That is, we've made a generic out-of-process file
patcher, and it need not be used in context of a DeltaUpdateOp. Meanwhile,
initializers of the CUS don't know that it does DeltaUpdateOps, just that it can
make use of an OutOfProcessFilePatcher if one is provided.)

I hope you followed my meandering train of thought. To summarize: I prefer to
optionally inject a "generic" OutOfProcessFilePatcher object instead of a
DeltaUpdateOpFactory.  Or maybe it should be a mandatory injection of a generic
FilePatcher object, and the caller decides if it is in or out of process. What
do you think?

[tommycli, 2014-07-31 19:53:52]

waffles: I switched some stuff up based on your feedback. See if you like this
new class structure better.

Thanks!

[waffles, 2014-07-31 21:54:27]

Yes, I like this much better, thank you.

Sorin, any comments?

https://codereview.chromium.org/420503002/diff/120001/chrome/browser/componen...
File
chrome/browser/component_updater/component_patcher_operation_out_of_process.cc
(right):

https://codereview.chromium.org/420503002/diff/120001/chrome/browser/componen...
chrome/browser/component_updater/component_patcher_operation_out_of_process.cc:21:
class DeltaUpdateOpPatchHost : public content::UtilityProcessHostClient {
Can we move this to an anonymous namespace?

https://codereview.chromium.org/420503002/diff/120001/chrome/browser/componen...
chrome/browser/component_updater/component_patcher_operation_out_of_process.cc:92:
host_ = new DeltaUpdateOpPatchHost(callback, task_runner);
I would rename DeltaUpdateOpPatchHost to just PatchHost or something like that -
I like that in theory this code could be invoked outside the context of a delta
update operation.

https://codereview.chromium.org/420503002/diff/120001/chrome/browser/componen...
File
chrome/browser/component_updater/component_patcher_operation_out_of_process.h
(right):

https://codereview.chromium.org/420503002/diff/120001/chrome/browser/componen...
chrome/browser/component_updater/component_patcher_operation_out_of_process.h:34:
scoped_refptr<DeltaUpdateOpPatchHost> host_;
Can we relax this to a scoped_refptr<content::UtilityProcessHostClient>?

https://codereview.chromium.org/420503002/diff/120001/chrome/browser/componen...
File chrome/browser/component_updater/test/component_patcher_unittest.cc
(right):

https://codereview.chromium.org/420503002/diff/120001/chrome/browser/componen...
chrome/browser/component_updater/test/component_patcher_unittest.cc:156:
CreateDeltaUpdateOpFactory(true /* in_process */));
Am I correct in thinking that this true no longer needs to be here?

[Sorin Jianu, 2014-07-31 23:29:15]

lgtm

Thank you Tommy. I am ok with this from a design and style standpoint.

Are you running lint and format over this code?

https://codereview.chromium.org/420503002/diff/120001/chrome/browser/componen...
File chrome/browser/component_updater/component_patcher_operation.h (right):

https://codereview.chromium.org/420503002/diff/120001/chrome/browser/componen...
chrome/browser/component_updater/component_patcher_operation.h:152: explicit
DeltaUpdateOpPatch(
In general, explicit is only meaningful for ctors with one params, although that
has changed in C++ 11 and up. Did you mean it this way?

[tommycli, 2014-08-04 20:25:14]

sorin/waffles: here you go

sorin: i just ran git cl format and lint. Thanks!

https://codereview.chromium.org/420503002/diff/120001/chrome/browser/componen...
File chrome/browser/component_updater/component_patcher_operation.h (right):

https://codereview.chromium.org/420503002/diff/120001/chrome/browser/componen...
chrome/browser/component_updater/component_patcher_operation.h:152: explicit
DeltaUpdateOpPatch(
On 2014/07/31 23:29:15, Sorin Jianu wrote:
> In general, explicit is only meaningful for ctors with one params, although
that
> has changed in C++ 11 and up. Did you mean it this way?

No, I just forgot to change this. Thanks for assuming the best though haha

https://codereview.chromium.org/420503002/diff/120001/chrome/browser/componen...
File
chrome/browser/component_updater/component_patcher_operation_out_of_process.cc
(right):

https://codereview.chromium.org/420503002/diff/120001/chrome/browser/componen...
chrome/browser/component_updater/component_patcher_operation_out_of_process.cc:21:
class DeltaUpdateOpPatchHost : public content::UtilityProcessHostClient {
On 2014/07/31 21:54:26, waffles wrote:
> Can we move this to an anonymous namespace?

I couldn't, as this is forward declared. I tried fwd declaring it in an
anonymous namespace, but 'git cl lint' complained.

https://codereview.chromium.org/420503002/diff/120001/chrome/browser/componen...
chrome/browser/component_updater/component_patcher_operation_out_of_process.cc:92:
host_ = new DeltaUpdateOpPatchHost(callback, task_runner);
On 2014/07/31 21:54:26, waffles wrote:
> I would rename DeltaUpdateOpPatchHost to just PatchHost or something like that
-
> I like that in theory this code could be invoked outside the context of a
delta
> update operation.

Done.

https://codereview.chromium.org/420503002/diff/120001/chrome/browser/componen...
File
chrome/browser/component_updater/component_patcher_operation_out_of_process.h
(right):

https://codereview.chromium.org/420503002/diff/120001/chrome/browser/componen...
chrome/browser/component_updater/component_patcher_operation_out_of_process.h:34:
scoped_refptr<DeltaUpdateOpPatchHost> host_;
On 2014/07/31 21:54:26, waffles wrote:
> Can we relax this to a scoped_refptr<content::UtilityProcessHostClient>?

This didn't work, as I needed to Bind to a PatchHost::StartProcess, which was
not in the base class.

https://codereview.chromium.org/420503002/diff/120001/chrome/browser/componen...
File chrome/browser/component_updater/test/component_patcher_unittest.cc
(right):

https://codereview.chromium.org/420503002/diff/120001/chrome/browser/componen...
chrome/browser/component_updater/test/component_patcher_unittest.cc:156:
CreateDeltaUpdateOpFactory(true /* in_process */));
On 2014/07/31 21:54:27, waffles wrote:
> Am I correct in thinking that this true no longer needs to be here?

Done.

[tommycli, 2014-08-04 20:27:40]

tsepez: chrome/common/*_messages.h changes. Thanks!

[Tom Sepez, 2014-08-04 20:40:08]

Messages LGTM.

https://codereview.chromium.org/420503002/diff/160001/chrome/browser/componen...
File chrome/browser/component_updater/component_patcher_operation.h (right):

https://codereview.chromium.org/420503002/diff/160001/chrome/browser/componen...
chrome/browser/component_updater/component_patcher_operation.h:23: const char
kOp[] = "op";
nit: usually you want
extern const char kOp[];
in the header file, with the actual string occurring in a .cc file.

[tommycli, 2014-08-04 21:07:25]

https://codereview.chromium.org/420503002/diff/160001/chrome/browser/componen...
File chrome/browser/component_updater/component_patcher_operation.h (right):

https://codereview.chromium.org/420503002/diff/160001/chrome/browser/componen...
chrome/browser/component_updater/component_patcher_operation.h:23: const char
kOp[] = "op";
On 2014/08/04 20:40:08, Tom Sepez wrote:
> nit: usually you want
> extern const char kOp[];
> in the header file, with the actual string occurring in a .cc file.

Done.

[waffles, 2014-08-04 21:14:55]

lgtm

[waffles, 2014-08-04 21:18:31]

On 2014/08/04 21:14:55, waffles wrote:
> lgtm

Thank you Tommy, this all looks good to me. :)

[tommycli, 2014-08-05 00:04:09]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-08-05 00:06:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tommycli@chromium.org/420503002/180001

[commit-bot: I haz the power, 2014-08-05 01:16:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-05 01:20:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-05 01:20:07]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tommycli, 2014-08-05 01:23:33]

Thestig: chrome_utility_client review needed. Thanks!

[Lei Zhang, 2014-08-05 01:30:15]

lgtm

[tommycli, 2014-08-05 05:11:42]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-08-05 05:13:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tommycli@chromium.org/420503002/180001

[commit-bot: I haz the power, 2014-08-05 05:35:03]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-05 07:52:45]

Change committed as 287484

[blundell, 2014-08-06 08:52:36]

LGTM