Add a component updater for the CLD2 data file.

Add a component updater for the CLD2 data file.

BUG=326023, 367239
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267181

[Andrew Hayden (chromium.org), 2014-04-03 15:25:06]

Although this isn't hooked up yet, I want to give folks a chance to get eyes-on.
Now that we can detect the presence of the CLD2 data file in the user data
directory, we can move on to the component updater step. This is significantly
less complicated than other installers because we do the heavy lifting of
mmap'ing and initializing CLD elsewhere already; what we're left with is
transferring the file and doing a single atomic rename into the expected
location within the user data directory.

So, here it is, in all its limited glory. I cribbed from the crl set fetcher,
copied the structure of the other installers in /chrome/component_updater, and
renamed. PTAL, more changes obviously required.

[waffles, 2014-04-03 16:31:23]

On 2014/04/03 15:25:06, Andrew Hayden wrote:
> Although this isn't hooked up yet, I want to give folks a chance to get
eyes-on.
> Now that we can detect the presence of the CLD2 data file in the user data
> directory, we can move on to the component updater step. This is significantly
> less complicated than other installers because we do the heavy lifting of
> mmap'ing and initializing CLD elsewhere already; what we're left with is
> transferring the file and doing a single atomic rename into the expected
> location within the user data directory.
> 
> So, here it is, in all its limited glory. I cribbed from the crl set fetcher,
> copied the structure of the other installers in /chrome/component_updater, and
> renamed. PTAL, more changes obviously required.

Have you considered using DefaultComponentInstaller / extending
ComponentInstallerTraits instead? Those classes are the gateway to differential
update support.

Also, note that on some systems base::Move may not be atomic.

[andrewhayden, 2014-04-03 18:04:30]

No, I wasn't aware of those. Is there an example you would consider
particularly correct that I should emulate? Thanks!


On 3 April 2014 17:31, <waffles@chromium.org> wrote:

> On 2014/04/03 15:25:06, Andrew Hayden wrote:
>
>> Although this isn't hooked up yet, I want to give folks a chance to get
>>
> eyes-on.
>
>> Now that we can detect the presence of the CLD2 data file in the user data
>> directory, we can move on to the component updater step. This is
>> significantly
>> less complicated than other installers because we do the heavy lifting of
>> mmap'ing and initializing CLD elsewhere already; what we're left with is
>> transferring the file and doing a single atomic rename into the expected
>> location within the user data directory.
>>
>
>  So, here it is, in all its limited glory. I cribbed from the crl set
>> fetcher,
>> copied the structure of the other installers in
>> /chrome/component_updater, and
>> renamed. PTAL, more changes obviously required.
>>
>
> Have you considered using DefaultComponentInstaller / extending
> ComponentInstallerTraits instead? Those classes are the gateway to
> differential
> update support.
>
> Also, note that on some systems base::Move may not be atomic.
>
> https://codereview.chromium.org/216923006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[waffles, 2014-04-03 18:14:43]

On 2014/04/03 18:04:30, andrewhayden wrote:
> No, I wasn't aware of those. Is there an example you would consider
> particularly correct that I should emulate? Thanks!
> 
> 
> On 3 April 2014 17:31, <mailto:waffles@chromium.org> wrote:
> 
> > On 2014/04/03 15:25:06, Andrew Hayden wrote:
> >
> >> Although this isn't hooked up yet, I want to give folks a chance to get
> >>
> > eyes-on.
> >
> >> Now that we can detect the presence of the CLD2 data file in the user data
> >> directory, we can move on to the component updater step. This is
> >> significantly
> >> less complicated than other installers because we do the heavy lifting of
> >> mmap'ing and initializing CLD elsewhere already; what we're left with is
> >> transferring the file and doing a single atomic rename into the expected
> >> location within the user data directory.
> >>
> >
> >  So, here it is, in all its limited glory. I cribbed from the crl set
> >> fetcher,
> >> copied the structure of the other installers in
> >> /chrome/component_updater, and
> >> renamed. PTAL, more changes obviously required.
> >>
> >
> > Have you considered using DefaultComponentInstaller / extending
> > ComponentInstallerTraits instead? Those classes are the gateway to
> > differential
> > update support.
> >
> > Also, note that on some systems base::Move may not be atomic.
> >
> > https://codereview.chromium.org/216923006/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Try widevine_cdm_component_installer.*. You can ignore almost everything in the
anonymous namespace in the .cc file - the important part is the implementation
of ComponentInstallerTraits and the RegisterWidevineCdmComponent().

Hopefully the comments in default_component_installer.h can help guide you -
based on the CL as it is now I think most of those methods will be simple
"return true"s.

[Andrew Hayden (chromium.org), 2014-04-04 08:47:49]

On 2014/04/03 18:14:43, waffles wrote:
> On 2014/04/03 18:04:30, andrewhayden wrote:
> > No, I wasn't aware of those. Is there an example you would consider
> > particularly correct that I should emulate? Thanks!
> > 
> > 
> > On 3 April 2014 17:31, <mailto:waffles@chromium.org> wrote:
> > 
> > > On 2014/04/03 15:25:06, Andrew Hayden wrote:
> > >
> > >> Although this isn't hooked up yet, I want to give folks a chance to get
> > >>
> > > eyes-on.
> > >
> > >> Now that we can detect the presence of the CLD2 data file in the user
data
> > >> directory, we can move on to the component updater step. This is
> > >> significantly
> > >> less complicated than other installers because we do the heavy lifting of
> > >> mmap'ing and initializing CLD elsewhere already; what we're left with is
> > >> transferring the file and doing a single atomic rename into the expected
> > >> location within the user data directory.
> > >>
> > >
> > >  So, here it is, in all its limited glory. I cribbed from the crl set
> > >> fetcher,
> > >> copied the structure of the other installers in
> > >> /chrome/component_updater, and
> > >> renamed. PTAL, more changes obviously required.
> > >>
> > >
> > > Have you considered using DefaultComponentInstaller / extending
> > > ComponentInstallerTraits instead? Those classes are the gateway to
> > > differential
> > > update support.
> > >
> > > Also, note that on some systems base::Move may not be atomic.
> > >
> > > https://codereview.chromium.org/216923006/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Try widevine_cdm_component_installer.*. You can ignore almost everything in
the
> anonymous namespace in the .cc file - the important part is the implementation
> of ComponentInstallerTraits and the RegisterWidevineCdmComponent().
> 
> Hopefully the comments in default_component_installer.h can help guide you -
> based on the CL as it is now I think most of those methods will be simple
> "return true"s.

Alright, I'll take a look.

Regarding the comment about non-atomic moves: is this true even when we're
talking about moving a file entirely within USER_DATA_DIR? This should be a
trivial rename on all remotely modern filesystems. Are there some out there that
we need to support that literally cannot rename a file without copying? If so,
I'll have to add a marker file to denote success; not a big deal, but obviously
another wrinkle to avoid if possible.

[waffles, 2014-04-04 16:12:39]

On 2014/04/04 08:47:49, Andrew Hayden wrote:
> On 2014/04/03 18:14:43, waffles wrote:
> > On 2014/04/03 18:04:30, andrewhayden wrote:
> > > No, I wasn't aware of those. Is there an example you would consider
> > > particularly correct that I should emulate? Thanks!
> > > 
> > > 
> > > On 3 April 2014 17:31, <mailto:waffles@chromium.org> wrote:
> > > 
> > > > On 2014/04/03 15:25:06, Andrew Hayden wrote:
> > > >
> > > >> Although this isn't hooked up yet, I want to give folks a chance to get
> > > >>
> > > > eyes-on.
> > > >
> > > >> Now that we can detect the presence of the CLD2 data file in the user
> data
> > > >> directory, we can move on to the component updater step. This is
> > > >> significantly
> > > >> less complicated than other installers because we do the heavy lifting
of
> > > >> mmap'ing and initializing CLD elsewhere already; what we're left with
is
> > > >> transferring the file and doing a single atomic rename into the
expected
> > > >> location within the user data directory.
> > > >>
> > > >
> > > >  So, here it is, in all its limited glory. I cribbed from the crl set
> > > >> fetcher,
> > > >> copied the structure of the other installers in
> > > >> /chrome/component_updater, and
> > > >> renamed. PTAL, more changes obviously required.
> > > >>
> > > >
> > > > Have you considered using DefaultComponentInstaller / extending
> > > > ComponentInstallerTraits instead? Those classes are the gateway to
> > > > differential
> > > > update support.
> > > >
> > > > Also, note that on some systems base::Move may not be atomic.
> > > >
> > > > https://codereview.chromium.org/216923006/
> > > >
> > > 
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > 
> > Try widevine_cdm_component_installer.*. You can ignore almost everything in
> the
> > anonymous namespace in the .cc file - the important part is the
implementation
> > of ComponentInstallerTraits and the RegisterWidevineCdmComponent().
> > 
> > Hopefully the comments in default_component_installer.h can help guide you -
> > based on the CL as it is now I think most of those methods will be simple
> > "return true"s.
> 
> Alright, I'll take a look.
> 
> Regarding the comment about non-atomic moves: is this true even when we're
> talking about moving a file entirely within USER_DATA_DIR? This should be a
> trivial rename on all remotely modern filesystems. Are there some out there
that
> we need to support that literally cannot rename a file without copying? If so,
> I'll have to add a marker file to denote success; not a big deal, but
obviously
> another wrinkle to avoid if possible.

It was the move from the temp directory to the final directory that prompted me
to make that comment (as an example, the temp directory could be on a different
physical disk partition). I think it's safe to assume that this issue does not
occur when using moves strictly within USER_DATA_DIR.

[Andrew Hayden (chromium.org), 2014-04-07 09:45:14]

On 2014/04/04 16:12:39, waffles wrote:
> On 2014/04/04 08:47:49, Andrew Hayden wrote:
> > On 2014/04/03 18:14:43, waffles wrote:
> > > On 2014/04/03 18:04:30, andrewhayden wrote:
> > > > No, I wasn't aware of those. Is there an example you would consider
> > > > particularly correct that I should emulate? Thanks!
> > > > 
> > > > 
> > > > On 3 April 2014 17:31, <mailto:waffles@chromium.org> wrote:
> > > > 
> > > > > On 2014/04/03 15:25:06, Andrew Hayden wrote:
> > > > >
> > > > >> Although this isn't hooked up yet, I want to give folks a chance to
get
> > > > >>
> > > > > eyes-on.
> > > > >
> > > > >> Now that we can detect the presence of the CLD2 data file in the user
> > data
> > > > >> directory, we can move on to the component updater step. This is
> > > > >> significantly
> > > > >> less complicated than other installers because we do the heavy
lifting
> of
> > > > >> mmap'ing and initializing CLD elsewhere already; what we're left with
> is
> > > > >> transferring the file and doing a single atomic rename into the
> expected
> > > > >> location within the user data directory.
> > > > >>
> > > > >
> > > > >  So, here it is, in all its limited glory. I cribbed from the crl set
> > > > >> fetcher,
> > > > >> copied the structure of the other installers in
> > > > >> /chrome/component_updater, and
> > > > >> renamed. PTAL, more changes obviously required.
> > > > >>
> > > > >
> > > > > Have you considered using DefaultComponentInstaller / extending
> > > > > ComponentInstallerTraits instead? Those classes are the gateway to
> > > > > differential
> > > > > update support.
> > > > >
> > > > > Also, note that on some systems base::Move may not be atomic.
> > > > >
> > > > > https://codereview.chromium.org/216923006/
> > > > >
> > > > 
> > > > To unsubscribe from this group and stop receiving emails from it, send
an
> > > email
> > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > > 
> > > Try widevine_cdm_component_installer.*. You can ignore almost everything
in
> > the
> > > anonymous namespace in the .cc file - the important part is the
> implementation
> > > of ComponentInstallerTraits and the RegisterWidevineCdmComponent().
> > > 
> > > Hopefully the comments in default_component_installer.h can help guide you
-
> > > based on the CL as it is now I think most of those methods will be simple
> > > "return true"s.
> > 
> > Alright, I'll take a look.
> > 
> > Regarding the comment about non-atomic moves: is this true even when we're
> > talking about moving a file entirely within USER_DATA_DIR? This should be a
> > trivial rename on all remotely modern filesystems. Are there some out there
> that
> > we need to support that literally cannot rename a file without copying? If
so,
> > I'll have to add a marker file to denote success; not a big deal, but
> obviously
> > another wrinkle to avoid if possible.
> 
> It was the move from the temp directory to the final directory that prompted
me
> to make that comment (as an example, the temp directory could be on a
different
> physical disk partition). I think it's safe to assume that this issue does not
> occur when using moves strictly within USER_DATA_DIR.

Right, that makes sense. OK, what I can do to ensure safety on all platforms is
this:
1. Copy to a temp file in the USER_DATA_DIR. This may not be atomic
2. Move temp file to final file name, in the same dir.

In my current code, not yet uploaded, I have a note that the first iteration of
change we should do is to allow the code to simply configure a runtime variable
with the location of the data file, then we don't have to worry about any of
this. I may actually go that path in our first iteration. At any rate, point
taken, and I'll make sure we don't end up in a race against the writer.

[andrewhayden, 2014-04-07 13:40:19]

Do you guys have any good documentation on testing components? I know I can
force a build that will talk to a QA server, but what about local unit
testing? Is there anything available?


On 7 April 2014 10:45, <andrewhayden@chromium.org> wrote:

> On 2014/04/04 16:12:39, waffles wrote:
>
>> On 2014/04/04 08:47:49, Andrew Hayden wrote:
>> > On 2014/04/03 18:14:43, waffles wrote:
>> > > On 2014/04/03 18:04:30, andrewhayden wrote:
>> > > > No, I wasn't aware of those. Is there an example you would consider
>> > > > particularly correct that I should emulate? Thanks!
>> > > >
>> > > >
>> > > > On 3 April 2014 17:31, <mailto:waffles@chromium.org> wrote:
>> > > >
>> > > > > On 2014/04/03 15:25:06, Andrew Hayden wrote:
>> > > > >
>> > > > >> Although this isn't hooked up yet, I want to give folks a chance
>> to
>>
> get
>
>> > > > >>
>> > > > > eyes-on.
>> > > > >
>> > > > >> Now that we can detect the presence of the CLD2 data file in the
>> user
>> > data
>> > > > >> directory, we can move on to the component updater step. This is
>> > > > >> significantly
>> > > > >> less complicated than other installers because we do the heavy
>>
> lifting
>
>> of
>> > > > >> mmap'ing and initializing CLD elsewhere already; what we're left
>> with
>> is
>> > > > >> transferring the file and doing a single atomic rename into the
>> expected
>> > > > >> location within the user data directory.
>> > > > >>
>> > > > >
>> > > > >  So, here it is, in all its limited glory. I cribbed from the crl
>> set
>> > > > >> fetcher,
>> > > > >> copied the structure of the other installers in
>> > > > >> /chrome/component_updater, and
>> > > > >> renamed. PTAL, more changes obviously required.
>> > > > >>
>> > > > >
>> > > > > Have you considered using DefaultComponentInstaller / extending
>> > > > > ComponentInstallerTraits instead? Those classes are the gateway to
>> > > > > differential
>> > > > > update support.
>> > > > >
>> > > > > Also, note that on some systems base::Move may not be atomic.
>> > > > >
>> > > > > https://codereview.chromium.org/216923006/
>> > > > >
>> > > >
>> > > > To unsubscribe from this group and stop receiving emails from it,
>> send
>>
> an
>
>> > > email
>> > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
>> > >
>> > > Try widevine_cdm_component_installer.*. You can ignore almost
>> everything
>>
> in
>
>> > the
>> > > anonymous namespace in the .cc file - the important part is the
>> implementation
>> > > of ComponentInstallerTraits and the RegisterWidevineCdmComponent().
>> > >
>> > > Hopefully the comments in default_component_installer.h can help
>> guide you
>>
> -
>
>> > > based on the CL as it is now I think most of those methods will be
>> simple
>> > > "return true"s.
>> >
>> > Alright, I'll take a look.
>> >
>> > Regarding the comment about non-atomic moves: is this true even when
>> we're
>> > talking about moving a file entirely within USER_DATA_DIR? This should
>> be a
>> > trivial rename on all remotely modern filesystems. Are there some out
>> there
>> that
>> > we need to support that literally cannot rename a file without copying?
>> If
>>
> so,
>
>> > I'll have to add a marker file to denote success; not a big deal, but
>> obviously
>> > another wrinkle to avoid if possible.
>>
>
>  It was the move from the temp directory to the final directory that
>> prompted
>>
> me
>
>> to make that comment (as an example, the temp directory could be on a
>>
> different
>
>> physical disk partition). I think it's safe to assume that this issue
>> does not
>> occur when using moves strictly within USER_DATA_DIR.
>>
>
> Right, that makes sense. OK, what I can do to ensure safety on all
> platforms is
> this:
> 1. Copy to a temp file in the USER_DATA_DIR. This may not be atomic
> 2. Move temp file to final file name, in the same dir.
>
> In my current code, not yet uploaded, I have a note that the first
> iteration of
> change we should do is to allow the code to simply configure a runtime
> variable
> with the location of the data file, then we don't have to worry about any
> of
> this. I may actually go that path in our first iteration. At any rate,
> point
> taken, and I'll make sure we don't end up in a race against the writer.
>
> https://codereview.chromium.org/216923006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sorin, 2014-04-07 16:18:26]

Andrew, at the moment we use just the usual gtest/gmock combo for local
tests. We thought about having browser tests but we did not go as far, and
I understand that browser tests are not encouraged anyway, as they tend to
be flaky.

Testing the component updater could be better, we have shipped code with
serious bugs due to not testing several timing elements, in both happy and
unhappy scenarios. We are open to hear ideas how to improve the testability
of the code.



On Mon, Apr 7, 2014 at 6:40 AM, Andrew Hayden <andrewhayden@google.com>wrote:

> Do you guys have any good documentation on testing components? I know I
> can force a build that will talk to a QA server, but what about local unit
> testing? Is there anything available?
>
>
> On 7 April 2014 10:45, <andrewhayden@chromium.org> wrote:
>
>> On 2014/04/04 16:12:39, waffles wrote:
>>
>>> On 2014/04/04 08:47:49, Andrew Hayden wrote:
>>> > On 2014/04/03 18:14:43, waffles wrote:
>>> > > On 2014/04/03 18:04:30, andrewhayden wrote:
>>> > > > No, I wasn't aware of those. Is there an example you would consider
>>> > > > particularly correct that I should emulate? Thanks!
>>> > > >
>>> > > >
>>> > > > On 3 April 2014 17:31, <mailto:waffles@chromium.org> wrote:
>>> > > >
>>> > > > > On 2014/04/03 15:25:06, Andrew Hayden wrote:
>>> > > > >
>>> > > > >> Although this isn't hooked up yet, I want to give folks a
>>> chance to
>>>
>> get
>>
>>> > > > >>
>>> > > > > eyes-on.
>>> > > > >
>>> > > > >> Now that we can detect the presence of the CLD2 data file in
>>> the user
>>> > data
>>> > > > >> directory, we can move on to the component updater step. This is
>>> > > > >> significantly
>>> > > > >> less complicated than other installers because we do the heavy
>>>
>> lifting
>>
>>> of
>>> > > > >> mmap'ing and initializing CLD elsewhere already; what we're
>>> left with
>>> is
>>> > > > >> transferring the file and doing a single atomic rename into the
>>> expected
>>> > > > >> location within the user data directory.
>>> > > > >>
>>> > > > >
>>> > > > >  So, here it is, in all its limited glory. I cribbed from the
>>> crl set
>>> > > > >> fetcher,
>>> > > > >> copied the structure of the other installers in
>>> > > > >> /chrome/component_updater, and
>>> > > > >> renamed. PTAL, more changes obviously required.
>>> > > > >>
>>> > > > >
>>> > > > > Have you considered using DefaultComponentInstaller / extending
>>> > > > > ComponentInstallerTraits instead? Those classes are the gateway
>>> to
>>> > > > > differential
>>> > > > > update support.
>>> > > > >
>>> > > > > Also, note that on some systems base::Move may not be atomic.
>>> > > > >
>>> > > > > https://codereview.chromium.org/216923006/
>>> > > > >
>>> > > >
>>> > > > To unsubscribe from this group and stop receiving emails from it,
>>> send
>>>
>> an
>>
>>> > > email
>>> > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
>>> > >
>>> > > Try widevine_cdm_component_installer.*. You can ignore almost
>>> everything
>>>
>> in
>>
>>> > the
>>> > > anonymous namespace in the .cc file - the important part is the
>>> implementation
>>> > > of ComponentInstallerTraits and the RegisterWidevineCdmComponent().
>>> > >
>>> > > Hopefully the comments in default_component_installer.h can help
>>> guide you
>>>
>> -
>>
>>> > > based on the CL as it is now I think most of those methods will be
>>> simple
>>> > > "return true"s.
>>> >
>>> > Alright, I'll take a look.
>>> >
>>> > Regarding the comment about non-atomic moves: is this true even when
>>> we're
>>> > talking about moving a file entirely within USER_DATA_DIR? This should
>>> be a
>>> > trivial rename on all remotely modern filesystems. Are there some out
>>> there
>>> that
>>> > we need to support that literally cannot rename a file without
>>> copying? If
>>>
>> so,
>>
>>> > I'll have to add a marker file to denote success; not a big deal, but
>>> obviously
>>> > another wrinkle to avoid if possible.
>>>
>>
>>  It was the move from the temp directory to the final directory that
>>> prompted
>>>
>> me
>>
>>> to make that comment (as an example, the temp directory could be on a
>>>
>> different
>>
>>> physical disk partition). I think it's safe to assume that this issue
>>> does not
>>> occur when using moves strictly within USER_DATA_DIR.
>>>
>>
>> Right, that makes sense. OK, what I can do to ensure safety on all
>> platforms is
>> this:
>> 1. Copy to a temp file in the USER_DATA_DIR. This may not be atomic
>> 2. Move temp file to final file name, in the same dir.
>>
>> In my current code, not yet uploaded, I have a note that the first
>> iteration of
>> change we should do is to allow the code to simply configure a runtime
>> variable
>> with the location of the data file, then we don't have to worry about any
>> of
>> this. I may actually go that path in our first iteration. At any rate,
>> point
>> taken, and I'll make sure we don't end up in a race against the writer.
>>
>> https://codereview.chromium.org/216923006/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sorin Jianu, 2014-04-07 17:01:17]

Andrew, is this code going to be rewritten to use ComponentInstallerTraits or
are we going to review it as is?

[andrewhayden, 2014-04-08 09:33:12]

I'm rewriting using ComponentInstallerTraits, but got sidetracked yesterday
by sheriffing duties and the fact that someone checked in a change that
broke dynamic mode. I've fixed the dynamic mode breakage, and was about to
start testing the traits version.

I'll have a new review up today. Thanks for the test info; What I'm looking
for specifically is to be able to test the CRX file locally and demonstrate
that it will load correctly at runtime.

-Andrew


On 7 April 2014 18:01, <sorin@chromium.org> wrote:

> Andrew, is this code going to be rewritten to use ComponentInstallerTraits
> or
> are we going to review it as is?
>
> https://codereview.chromium.org/216923006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[andrewhayden, 2014-04-16 20:07:12]

Sorry for the delay, but I've posted a new patchset:
https://codereview.chromium.org/216923006

Still obviously not done, but I believe this is the approach we're talking
about.

I'm still interested in figuring out how to test the CRX stuff locally.
Peter Beverloo tells me there are some app extension tests that manually
install CRX files; I've asked him for a link, but was kind of figuring
there would be some kind of framework. I'll keep your previous comments in
mind while looking into testing, but if there's any good examples out there
you think I would be wise to emulate in the interim, please let me know.

-Andrew


On 8 April 2014 10:33, Andrew Hayden <andrewhayden@google.com> wrote:

> I'm rewriting using ComponentInstallerTraits, but got sidetracked
> yesterday by sheriffing duties and the fact that someone checked in a
> change that broke dynamic mode. I've fixed the dynamic mode breakage, and
> was about to start testing the traits version.
>
> I'll have a new review up today. Thanks for the test info; What I'm
> looking for specifically is to be able to test the CRX file locally and
> demonstrate that it will load correctly at runtime.
>
> -Andrew
>
>
> On 7 April 2014 18:01, <sorin@chromium.org> wrote:
>
>> Andrew, is this code going to be rewritten to use
>> ComponentInstallerTraits or
>> are we going to review it as is?
>>
>> https://codereview.chromium.org/216923006/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Andrew Hayden (chromium.org), 2014-04-16 20:08:20]

https://codereview.chromium.org/216923006/diff/20001/chrome/browser/translate...
File chrome/browser/translate/translate_tab_helper.cc (right):

https://codereview.chromium.org/216923006/diff/20001/chrome/browser/translate...
chrome/browser/translate/translate_tab_helper.cc:39: #include
"chrome/common/chrome_constants.h"
This won't be in the final submit; it was necessary due to a breaking change
that has been patched but hasn't yet made it into my local LKGR.

[Andrew Hayden (chromium.org), 2014-04-17 14:46:14]

I think I'm ready to start testing now.

[Andrew Hayden (chromium.org), 2014-04-17 16:17:54]

With patchset 5, I think we're in a good place. Aside from the SHA2 hash, which
I'm still waiting for an answer on from Sorin et al, this should do the trick.
Proper integration with the existing translate system is now present; we don't
have to copy the CLD data file around, we just defer to the component updater if
CLD2_IS_COMPONENT.

PTAL.

[waffles, 2014-04-17 23:40:58]

On 2014/04/17 16:17:54, Andrew Hayden wrote:
> With patchset 5, I think we're in a good place. Aside from the SHA2 hash,
which
> I'm still waiting for an answer on from Sorin et al, this should do the trick.
> Proper integration with the existing translate system is now present; we don't
> have to copy the CLD data file around, we just defer to the component updater
if
> CLD2_IS_COMPONENT.
> 
> PTAL.

With respect to the component updater integration, this all looks pretty good to
me (modulo the hash, comment about the hash, etc). For my own education, under
what circumstances exactly is defined(CLD2_DYNAMIC_MODE) &&
defined(CLD2_IS_COMPONENT) true? Only on Android?

[Andrew Hayden (chromium.org), 2014-04-18 08:04:02]

On 2014/04/17 23:40:58, waffles wrote: 
> With respect to the component updater integration, this all looks pretty good
to
> me (modulo the hash, comment about the hash, etc). For my own education, under
> what circumstances exactly is defined(CLD2_DYNAMIC_MODE) &&
> defined(CLD2_IS_COMPONENT) true? Only on Android?

CLD2_DYNAMIC_MODE is currently 0 everywhere; it governs whether we load CLD's
data from a file, or whether we build it straight into the binary. This
parallels the mechanisms by which ICU data can be loaded. Moving the data out of
binary makes updates smaller, which is a win on mobile platforms. If we did
nothing but move the data out of the binary, we'd be in better shape than we are
today on many mobile devices. CLD2_IS_COMPONENT is new, and should be set by
platforms wishing to download the data via the component updater.

We built it this way so that each platform can pick the right solution, and to
decouple the decision on splitting the data out of the binary from the decision
on using component updater to acquire the bits.

My plan is to enable both flags on Android.

[Andrew Hayden (chromium.org), 2014-04-18 08:06:25]

On 2014/04/18 08:04:02, Andrew Hayden wrote:
> CLD2_DYNAMIC_MODE is currently 0 everywhere; it governs whether we load CLD's
> data from a file, or whether we build it straight into the binary. This
> parallels the mechanisms by which ICU data can be loaded. Moving the data out
of
> binary makes updates smaller, which is a win on mobile platforms. If we did
> nothing but move the data out of the binary, we'd be in better shape than we
are
> today on many mobile devices. CLD2_IS_COMPONENT is new, and should be set by
> platforms wishing to download the data via the component updater.
> 
> We built it this way so that each platform can pick the right solution, and to
> decouple the decision on splitting the data out of the binary from the
decision
> on using component updater to acquire the bits.
> 
> My plan is to enable both flags on Android.

PS - I should note that I don't plan to flip the flags on in this checkin. We're
making incremental progress towards flipping the switch; Before we do it for
real, there is work to be done on the testing side to ensure that we don't
perturb existing translation tests with this change. That is, I still have to
make sure that we can have the translate data available immediately upon browser
startup for all the existing tests that don't care about this change. That will
be yet another CL.

[Andrew Hayden (chromium.org), 2014-04-18 08:11:17]

https://codereview.chromium.org/216923006/diff/100001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/216923006/diff/100001/build/common.gypi#newco...
build/common.gypi:664: 'cld_version%': 2,
These bits should be left unchanged for this submit, as we can't turn this on
till we also upgrade the test infrastructure to be able to inject the data file
for all the tests that already exist that aren't related to this functionality
at all.

[Sorin Jianu, 2014-04-18 17:46:13]

Thank you Andrew!

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:412: #endif
Maybe keep the empty line above 413.

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/cld_component_installer.cc (right):

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:24: 
These lines of code don't have to be indented inside the anon namespace. At
least, I've never indented them.

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:28: // lock to guard
it.
Two ideas here. If the file path is guaranteed to be retrieved from the UI
thread only, perhaps we could avoid the complexity of locking. The
ComponentReady call runs on the UI thread.

Second idea is a suggestion to consider whether using g_browser_process to store
the CLD file path instead of the global member makes sense. We are not fond of
g_browser_process nor the globals. We are ok either way we go, after considering
all options.

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:35: base::FilePath*
value;
Can we use a value type here instead of a pointer type? Doing so can simplify
the setter and the getter slightly, as we don't have to allocate and deallocate
the member.

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:115:
std::vector<uint8>* hash) const {
does this fit on one line?

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:136: base::FilePath*
old = s_file_.Get().value;
Is there a reason why we don't just:
delete  s_file_.Get().value;
s_file_.Get().value = new base::FilePath(path);

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:138: if (old !=
NULL) delete old;
We can delete a null pointer, no need for the check.

[waffles, 2014-04-18 17:48:01]

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/cld_component_installer.cc (right):

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:44: // that's used
to sign generated CRL sets.
Hash + comment are wrong, as discussed.

I know you know this, but I want to leave a comment on this patchset for the
record/diffs.

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:126: new
CldComponentInstallerTraits);
We prefer "new CldComponentInstallerTraits())"

[Andrew Hayden (chromium.org), 2014-04-22 16:10:14]

Comments addressed! PTAL!

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:412: #endif
On 2014/04/18 17:46:14, Sorin Jianu wrote:
> Maybe keep the empty line above 413.

Done.

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/componen...
File chrome/browser/component_updater/cld_component_installer.cc (right):

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:24: 
On 2014/04/18 17:46:14, Sorin Jianu wrote:
> These lines of code don't have to be indented inside the anon namespace. At
> least, I've never indented them.

Done.

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:28: // lock to guard
it.
On 2014/04/18 17:46:14, Sorin Jianu wrote:
> Two ideas here. If the file path is guaranteed to be retrieved from the UI
> thread only, perhaps we could avoid the complexity of locking. The
> ComponentReady call runs on the UI thread.
> 
> Second idea is a suggestion to consider whether using g_browser_process to
store
> the CLD file path instead of the global member makes sense. We are not fond of
> g_browser_process nor the globals. We are ok either way we go, after
considering
> all options.

Access to the path takes place in the blocking pool, not the UI thread. This is
because we have to do some file access immediately, which is forbidden on the UI
thread in most cases. So I think we still need the lock.

I wasn't aware of g_browser_process, but after looking at it I don't feel like
it adds much value to move things there. That said there is precedent within for
the CRL updater and PNACL updater. I think I will defer to you guys on this: As
the owners of the component updater, would you prefer that I keep my global
handle in g_browser_process? If so, I'll move it; if not, I'm inclined to leave
it where it is.

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:35: base::FilePath*
value;
On 2014/04/18 17:46:14, Sorin Jianu wrote:
> Can we use a value type here instead of a pointer type? Doing so can simplify
> the setter and the getter slightly, as we don't have to allocate and
deallocate
> the member.

I didn't realize filepath was just a stringtype. Ha! OK, yeah, I guess we can
get away with a value type. I'll change this.

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:115:
std::vector<uint8>* hash) const {
On 2014/04/18 17:46:14, Sorin Jianu wrote:
> does this fit on one line?

........... maybe. :)

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:126: new
CldComponentInstallerTraits);
On 2014/04/18 17:48:02, waffles wrote:
> We prefer "new CldComponentInstallerTraits())"
Sorry, copy/paste cruft. Done.

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:136: base::FilePath*
old = s_file_.Get().value;
On 2014/04/18 17:46:14, Sorin Jianu wrote:
> Is there a reason why we don't just:
> delete  s_file_.Get().value;
> s_file_.Get().value = new base::FilePath(path);

Sorry, this was leftover from when I was trying to do this work without a lock.
We're not going to use a pointer anyhow, so no issue.

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:138: if (old !=
NULL) delete old;
On 2014/04/18 17:46:14, Sorin Jianu wrote:
> We can delete a null pointer, no need for the check.

My C++ is rusty, I thought this would be a segfault. We won't be using a pointer
anymore anyhow, so hooray.

https://codereview.chromium.org/216923006/diff/100001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:138: if (old !=
NULL) delete old;
On 2014/04/18 17:46:14, Sorin Jianu wrote:
> We can delete a null pointer, no need for the check.

Will go away when I stop using a pointer. Sorry, didn't know there as an
implicit null check in delete... my c++ is rusty.

[Andrew Hayden (chromium.org), 2014-04-22 16:11:14]

+jam/toyoshim at the behest of "git cl upload" OWNERS suggestions.

[Sorin Jianu, 2014-04-22 17:10:48]

Andrew, could you please take another look at the structure that wraps the path?
Do we need it?

Thank you!

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

https://codereview.chromium.org/216923006/diff/120001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:29:
base::LazyInstance<base::Lock> s_file_lock_ = LAZY_INSTANCE_INITIALIZER;
There should not be a trailing _.
Naming convention for globals, per coding style:

There are no special requirements for global variables, which should be rare in
any case, but if you use one, consider prefixing it with g_ or some other marker
to easily distinguish it from local variables.

https://codereview.chromium.org/216923006/diff/120001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:31: struct
CLDFilePathWrapper {
Why do we need the wrapper? Can we just say:
base::LazyInstance<base::FilePath>::Leaky s_file_

Also, we could consider more descriptive names for this identifier, which has a
larger visibility than most other identifiers in this module.

https://codereview.chromium.org/216923006/diff/120001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:45: const uint8
kPublicKeySHA256[32] = {
kPublicKeySHA256 and kCldManifestName could be members in the anon namespace
above. I think.

https://codereview.chromium.org/216923006/diff/120001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:139: base::FilePath
cached = s_file_.Get().value;
const base::FilePath cached

[jam, 2014-04-22 17:19:18]

lgtm

[Takashi Toyoshima, 2014-04-23 00:59:03]

LGTM for translate.
If you have a meta bug for component updater supporting CLD2, adding it to BUG
line would be nice.

[Andrew Hayden (chromium.org), 2014-04-24 16:56:18]

Here we go... this works on Android. I was able to watch the package download
from the QA server, get unpacked and verified, installed, and translation
started working. Pretty sweet.

I added some logging statements into the component updater guts that I believe
are useful, though I probably went a bit overboard. Sorin et al: Let me know if
you object to adding these in.

I was a bit confused to find that I had to manually specify the
"_platform_specific/all" directory; I figured that the contents of that
directory would be copied into the base_dir, but I was wrong apparently. Have I
done this correctly, hardcoding "_platform_specific/all" into the code? It seems
brittle to me, but perhaps there is a smarter way.

[Sorin Jianu, 2014-04-24 17:28:40]

Sorry, no idea about platforms all.

Any progress re: the last few nits we've pointed out about naming of the file
scope vars and the lazy wrapper?

[Andrew Hayden (chromium.org), 2014-04-25 16:10:27]

Var names and scopes fixed up, I believe this is ready to go. We will still have
to add UMA and get bots happy with copying the file to the right place before we
turn this on, though. I would like to do the change to allow on-demand updates
in a separate commit.

[Sorin Jianu, 2014-04-25 16:39:40]

Thank Andrew, a few more style guide issue (naming and indentation mainly).

https://codereview.chromium.org/216923006/diff/180001/chrome/browser/componen...
File chrome/browser/component_updater/cld_component_installer.cc (right):

https://codereview.chromium.org/216923006/diff/180001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:30:
base::LazyInstance<base::Lock> g_cld_file_lock_ = LAZY_INSTANCE_INITIALIZER;
I believe the coding style specifies that only the data members of classes have
trailing underscores. Therefore, the postfix for these variable names must not
end with '_'. I am not certain about the 'g_' prefix. The style guide specifies
that the globals could be prefixed with 'g_'. However, these variables, while
they have file scope, they are not globals, in the sense that they are not
visible to other compilation units. 

In conclusion, I would name them 'cld_file_lock' and 'cld_file' respectively (no
g_ not trailing _)

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

https://codereview.chromium.org/216923006/diff/180001/chrome/browser/componen...
chrome/browser/component_updater/component_updater_service.cc:311: <<
config_->InitialDelay() << " seconds";
style guide here and throughout the CL.

"subsequent lines should start with the << operator and should be aligned based
on the first << from the original "

https://codereview.chromium.org/216923006/diff/200001/chrome/browser/componen...
File chrome/browser/component_updater/cld_component_installer.cc (right):

https://codereview.chromium.org/216923006/diff/200001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:110: base::FilePath
expected_file = GetInstalledPath(install_dir);
could be const, sorry I did not see it before.

https://codereview.chromium.org/216923006/diff/200001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:112: bool result =
base::PathExists(expected_file);
const bool

https://codereview.chromium.org/216923006/diff/200001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:151: base::FilePath
cached = g_cld_file_.Get();
const base::FilePath

https://codereview.chromium.org/216923006/diff/200001/chrome/browser/componen...
File chrome/browser/component_updater/cld_component_installer.h (right):

https://codereview.chromium.org/216923006/diff/200001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.h:18: // once and was
valid when it was observed; if the method returns false, the
Small terminology neat. This is a function not a method. In C++, virtual
functions are sometimes called methods. People are somewhat loose with the
terminology and they extend the word method to cover any member functions. Few
would call a standalone function a method.

https://codereview.chromium.org/216923006/diff/200001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.h:19: // path parameter
is not modified. This method is completely threadsafe.
I think we could simply say: "The function is thread-safe".

[Andrew Hayden (chromium.org), 2014-04-25 17:07:20]

All nits addressed, PTAL again :)

https://codereview.chromium.org/216923006/diff/200001/chrome/browser/componen...
File chrome/browser/component_updater/cld_component_installer.cc (right):

https://codereview.chromium.org/216923006/diff/200001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:110: base::FilePath
expected_file = GetInstalledPath(install_dir);
On 2014/04/25 16:39:41, Sorin Jianu wrote:
> could be const, sorry I did not see it before.

Done.

https://codereview.chromium.org/216923006/diff/200001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:112: bool result =
base::PathExists(expected_file);
On 2014/04/25 16:39:41, Sorin Jianu wrote:
> const bool

Done.

https://codereview.chromium.org/216923006/diff/200001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.cc:151: base::FilePath
cached = g_cld_file_.Get();
On 2014/04/25 16:39:41, Sorin Jianu wrote:
> const base::FilePath

Done.

https://codereview.chromium.org/216923006/diff/200001/chrome/browser/componen...
File chrome/browser/component_updater/cld_component_installer.h (right):

https://codereview.chromium.org/216923006/diff/200001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.h:18: // once and was
valid when it was observed; if the method returns false, the
On 2014/04/25 16:39:41, Sorin Jianu wrote:
> Small terminology neat. This is a function not a method. In C++, virtual
> functions are sometimes called methods. People are somewhat loose with the
> terminology and they extend the word method to cover any member functions. Few
> would call a standalone function a method.

Done. Sorry, my C++ terminology has atrophied. Getting it back, one nit at a
time :)

https://codereview.chromium.org/216923006/diff/200001/chrome/browser/componen...
chrome/browser/component_updater/cld_component_installer.h:19: // path parameter
is not modified. This method is completely threadsafe.
On 2014/04/25 16:39:41, Sorin Jianu wrote:
> I think we could simply say: "The function is thread-safe".

Done.

[Andrew Hayden (chromium.org), 2014-04-25 17:08:07]

Whoops, missed the indents. Sigh.

[Andrew Hayden (chromium.org), 2014-04-25 17:11:32]

OK, for real this time, now with indented less-thans.

[Sorin Jianu, 2014-04-25 17:30:55]

lgtm

[Sorin Jianu, 2014-04-25 17:31:27]

On 2014/04/25 17:30:55, Sorin Jianu wrote:
> lgtm

Thank you Andrew!

[waffles, 2014-04-25 19:44:19]

lgtm

[Andrew Hayden (chromium.org), 2014-04-25 19:47:57]

Merge failures on the bots, so I'll rebase and try to land this on Monday.

[Andrew Hayden (chromium.org), 2014-04-28 13:21:38]

Interesting build failure on the windows bot only:
e:\b\build\slave\win\build\src\chrome\browser\component_updater\cld_component_installer.cc(91)
: error C2664: 'base::FilePath base::FilePath::Append(const base::FilePath &)
const' : cannot convert argument 1 from 'const char [19]' to 'const
base::FilePath::StringType &'
        Reason: cannot convert from 'const char [19]' to 'const
base::FilePath::StringType'
        No constructor could take the source type, or constructor overload
resolution was ambiguous
e:\b\build\slave\win\build\src\chrome\browser\component_updater\cld_component_installer.cc(92)
: error C2228: left of '.Append' must have class/struct/union
e:\b\build\slave\win\build\src\chrome\browser\component_updater\cld_component_installer.cc(93)
: error C2228: left of '.Append' must have class/struct/union

Should fix this before submitting, even though it won't be enabled on Windows.

[Sorin Jianu, 2014-04-28 16:16:42]

On 2014/04/28 13:21:38, Andrew Hayden wrote:
> Interesting build failure on the windows bot only:
>
e:\b\build\slave\win\build\src\chrome\browser\component_updater\cld_component_installer.cc(91)
> : error C2664: 'base::FilePath base::FilePath::Append(const base::FilePath &)
> const' : cannot convert argument 1 from 'const char [19]' to 'const
> base::FilePath::StringType &'
>         Reason: cannot convert from 'const char [19]' to 'const
> base::FilePath::StringType'
>         No constructor could take the source type, or constructor overload
> resolution was ambiguous
>
e:\b\build\slave\win\build\src\chrome\browser\component_updater\cld_component_installer.cc(92)
> : error C2228: left of '.Append' must have class/struct/union
>
e:\b\build\slave\win\build\src\chrome\browser\component_updater\cld_component_installer.cc(93)
> : error C2228: left of '.Append' must have class/struct/union
> 
> Should fix this before submitting, even though it won't be enabled on Windows.

You could the FILE_PATH_LITERAL macro to convert from a narrow string literal to
a wide string literal, since file paths are UTF-16 on Windows, hence the
conversion error.

[Andrew Hayden (chromium.org), 2014-04-29 11:45:54]

The CQ bit was checked by andrewhayden@chromium.org

[Andrew Hayden (chromium.org), 2014-04-29 11:46:16]

Thanks, Sorin. Done and uploaded, checking the submit box now.

[commit-bot: I haz the power, 2014-04-29 11:46:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/216923006/28...

[commit-bot: I haz the power, 2014-04-29 11:54:57]

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

[commit-bot: I haz the power, 2014-04-29 11:54:58]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium

[Andrew Hayden (chromium.org), 2014-04-29 12:23:36]

The CQ bit was checked by andrewhayden@chromium.org

[commit-bot: I haz the power, 2014-04-29 12:23:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/216923006/28...

[commit-bot: I haz the power, 2014-04-29 12:25:54]

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

[commit-bot: I haz the power, 2014-04-29 12:25:55]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium

[Andrew Hayden (chromium.org), 2014-04-29 12:48:49]

The CQ bit was checked by andrewhayden@chromium.org

[commit-bot: I haz the power, 2014-04-29 12:48:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/216923006/28...

[commit-bot: I haz the power, 2014-04-29 13:37:25]

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

[commit-bot: I haz the power, 2014-04-29 13:37:26]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[Andrew Hayden (chromium.org), 2014-04-29 13:40:45]

Mailed the chrome-troopers@ about the flaking tryjobs and the obviously
unrelated host lookup errors in the various network-accessing build steps.

[Andrew Hayden (chromium.org), 2014-04-29 13:41:31]

The CQ bit was checked by andrewhayden@chromium.org

[commit-bot: I haz the power, 2014-04-29 13:41:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/216923006/28...

[commit-bot: I haz the power, 2014-04-29 13:48:36]

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

[commit-bot: I haz the power, 2014-04-29 13:48:37]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium

[Andrew Hayden (chromium.org), 2014-04-29 13:50:17]

The CQ bit was checked by andrewhayden@chromium.org

[commit-bot: I haz the power, 2014-04-29 13:50:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/216923006/28...

[commit-bot: I haz the power, 2014-04-29 18:50:52]

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

[commit-bot: I haz the power, 2014-04-29 18:50:53]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium

[Andrew Hayden (chromium.org), 2014-04-30 11:23:23]

The CQ bit was checked by andrewhayden@chromium.org

[commit-bot: I haz the power, 2014-04-30 11:23:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/216923006/28...

[commit-bot: I haz the power, 2014-04-30 11:27:21]

Change committed as 267181