Extract the handling of the clean exit beacon from MetricsService. The extracted implementation sup…

Extract the handling of the clean exit beacon from MetricsService. The extracted implementation supports the use of a backup storage location and reports consistency between the two locations.

BUG=412398
Committed: https://crrev.com/65b58df31c75ef627cdff2a2ee377330fd86c2b4
Cr-Commit-Position: refs/heads/master@{#294498}

[erikwright (departed), 2014-09-09 17:23:04]

erikwright@chromium.org changed reviewers:
+ asvitkine@chromium.org

[erikwright (departed), 2014-09-09 17:23:05]

asvitkine: PTAL for components/metrics and histograms.xml

[erikwright (departed), 2014-09-09 18:54:37]

I extracted two base CLs, and now you can review everything, please, Alexei.

[Alexei Svitkine (slow), 2014-09-09 21:05:26]

I think this could be made much simpler by not going through
ChromeMetricsServiceClient.

All the registry APIs this uses is in base/, so can already be used directly
from the component.

The only thing you need to expose via the client interface is a getter that
returns PRODUCT_STRING_PATH, which is needed to construct the registry path.

[erikwright (departed), 2014-09-09 21:07:48]

On 2014/09/09 21:05:26, Alexei Svitkine wrote:
> I think this could be made much simpler by not going through
> ChromeMetricsServiceClient.
> 
> All the registry APIs this uses is in base/, so can already be used directly
> from the component.
> 
> The only thing you need to expose via the client interface is a getter that
> returns PRODUCT_STRING_PATH, which is needed to construct the registry path.

Sure. ChromeMetricsServiceClient is responsible for instantiating the
MetricsService, so presumably it would be responsible for providing the path to
use.

How would you prefer for CMSC to pass that parameter to MS? Constructor arg?
Getter on the client interface?

[Alexei Svitkine (slow), 2014-09-09 21:09:17]

Getter on the client interface SGTM.

On Tue, Sep 9, 2014 at 5:07 PM, <erikwright@chromium.org> wrote:

> On 2014/09/09 21:05:26, Alexei Svitkine wrote:
>
>> I think this could be made much simpler by not going through
>> ChromeMetricsServiceClient.
>>
>
>  All the registry APIs this uses is in base/, so can already be used
>> directly
>> from the component.
>>
>
>  The only thing you need to expose via the client interface is a getter
>> that
>> returns PRODUCT_STRING_PATH, which is needed to construct the registry
>> path.
>>
>
> Sure. ChromeMetricsServiceClient is responsible for instantiating the
> MetricsService, so presumably it would be responsible for providing the
> path to
> use.
>
> How would you prefer for CMSC to pass that parameter to MS? Constructor
> arg?
> Getter on the client interface?
>
> https://codereview.chromium.org/558683002/
>

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

[erikwright (departed), 2014-09-11 14:14:17]

PTAL.

[Alexei Svitkine (slow), 2014-09-11 15:23:28]

https://codereview.chromium.org/558683002/diff/60001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/558683002/diff/60001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:277: return
L"Software\\" PRODUCT_STRING_PATH L"\\StabilityMetrics";
Hmm, where is PRODUCT_STRING_PATH defined? In cs.chromium.org, I only see it
defined in some .cc files...

https://codereview.chromium.org/558683002/diff/60001/components/metrics/clean...
File components/metrics/clean_exit_beacon.cc (right):

https://codereview.chromium.org/558683002/diff/60001/components/metrics/clean...
components/metrics/clean_exit_beacon.cc:21:
"user_experience_metrics.stability.exited_cleanly";
What's the reason for moving the pref? I think we want to keep a central place
where all the metrics prefs are listed, so I prefer this is moved back.

https://codereview.chromium.org/558683002/diff/60001/components/metrics/clean...
components/metrics/clean_exit_beacon.cc:25: #if defined(OS_WIN)
There's too many ifdefs for my preference.

How about we make the client method available on all platforms, but return an
empty string on others.

We can also provide a default implementation that returns an empty string, so
that clients that don't care about Windows (e.g. Chromecast), don't need to
implement it.

Then, we can eliminate most of the ifdefs. We'd only need to keep them in the
implementation of this file, and even here, we can just put it around the body
of the code (rather than around the method signature itself).

https://codereview.chromium.org/558683002/diff/60001/components/metrics/clean...
File components/metrics/clean_exit_beacon.h (right):

https://codereview.chromium.org/558683002/diff/60001/components/metrics/clean...
components/metrics/clean_exit_beacon.h:10: #if defined(OS_WIN)
Nit: Remove ifdef. You don't have it in metrics_service_client.h anyway.

https://codereview.chromium.org/558683002/diff/60001/components/metrics/clean...
components/metrics/clean_exit_beacon.h:38: bool exited_cleanly() { return
initial_value_; }
Nit: const

https://codereview.chromium.org/558683002/diff/60001/components/metrics/metri...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/558683002/diff/60001/components/metrics/metri...
components/metrics/metrics_service.cc:523:
clean_exit_beacon_->WriteBeaconValue(false);
Shouldn't this be . instead of ->?

https://codereview.chromium.org/558683002/diff/60001/components/metrics/metri...
components/metrics/metrics_service.cc:574: if
(!clean_exit_beacon_.exited_cleanly()) {
Note: You need to sync/rebase on top of siggi's change that recently landed.

https://codereview.chromium.org/558683002/diff/60001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/558683002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:34749: +    State and the Windows
registry. They are normally expected to be identical.
Please mention when this is logged.

[erikwright (departed), 2014-09-11 15:39:14]

Please confirm that you want me to:

#ifdef the member declaration
#ifdef the constructor definition from the initializer list to the end
_not_ #ifdef the client getter
inline declare a default (empty) getter implementation

https://codereview.chromium.org/558683002/diff/60001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/558683002/diff/60001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:277: return
L"Software\\" PRODUCT_STRING_PATH L"\\StabilityMetrics";
On 2014/09/11 15:23:28, Alexei Svitkine wrote:
> Hmm, where is PRODUCT_STRING_PATH defined? In http://cs.chromium.org, I only
see it
> defined in some .cc files...

See https://codereview.chromium.org/558713002/ which I need to rebase.

https://codereview.chromium.org/558683002/diff/60001/components/metrics/clean...
File components/metrics/clean_exit_beacon.cc (right):

https://codereview.chromium.org/558683002/diff/60001/components/metrics/clean...
components/metrics/clean_exit_beacon.cc:21:
"user_experience_metrics.stability.exited_cleanly";
On 2014/09/11 15:23:28, Alexei Svitkine wrote:
> What's the reason for moving the pref? I think we want to keep a central place
> where all the metrics prefs are listed, so I prefer this is moved back.

Exposing the name of a preference permits violation of encapsulation. This code
encapsulates the access to this value, and it would be inappropriate for any
other code to directly read/write the value. Hiding the name of the pref
enforces that. This is a pattern that I've used elsewhere and I think should be
generally adopted any time it is not reasonable for arbitrary code to manipulate
a preference value.

https://codereview.chromium.org/558683002/diff/60001/components/metrics/clean...
components/metrics/clean_exit_beacon.cc:25: #if defined(OS_WIN)
On 2014/09/11 15:23:28, Alexei Svitkine wrote:
> There's too many ifdefs for my preference.
> 
> How about we make the client method available on all platforms, but return an
> empty string on others.
> 
> We can also provide a default implementation that returns an empty string, so
> that clients that don't care about Windows (e.g. Chromecast), don't need to
> implement it.

It seems a bit strange to declare and implement (even just by inheritance) a
method whose sole purpose is to return a Windows registry key on non-Windows
platforms. That is partially why I felt making the storage method (in addition
to the location) abstract was better.

> Then, we can eliminate most of the ifdefs. We'd only need to keep them in the
> implementation of this file, and even here, we can just put it around the body
> of the code (rather than around the method signature itself).

I'm not quite sure. Because you will end up with an unused member, which Clang
may warn about. Or else (if you ifdef the member declaration and assignment) you
will have an unused parameter (which I believe will not lead to a warning).

[Alexei Svitkine (slow), 2014-09-11 15:57:28]

"Please confirm that you want me to:

#ifdef the member declaration"

I think we don't need to ifdef it.

"#ifdef the constructor definition from the initializer list to the end"
I don't think we need to ifdef the init list, but instead can just ifdef the
part of the body that uses registry.

"_not_ #ifdef the client getter"
Correct.

"inline declare a default (empty) getter implementation"
This was my suggestion if we were to keep the accessor as it is in your current
CL. But my other suggestion (see below) is to expose an accessor for the product
name. If we do that, I don't think we need a default impl.

https://codereview.chromium.org/558683002/diff/60001/components/metrics/clean...
File components/metrics/clean_exit_beacon.cc (right):

https://codereview.chromium.org/558683002/diff/60001/components/metrics/clean...
components/metrics/clean_exit_beacon.cc:21:
"user_experience_metrics.stability.exited_cleanly";
On 2014/09/11 15:39:14, erikwright wrote:
> On 2014/09/11 15:23:28, Alexei Svitkine wrote:
> > What's the reason for moving the pref? I think we want to keep a central
place
> > where all the metrics prefs are listed, so I prefer this is moved back.
> 
> Exposing the name of a preference permits violation of encapsulation. This
code
> encapsulates the access to this value, and it would be inappropriate for any
> other code to directly read/write the value. Hiding the name of the pref
> enforces that. This is a pattern that I've used elsewhere and I think should
be
> generally adopted any time it is not reasonable for arbitrary code to
manipulate
> a preference value.

I agree in principle, but at the same time we lose the benefit of seeing all the
prefs in one place.

In practice, I think for this component, I'd rather keep all the names in one
place but have them private to the component (i.e. not used by anything outside
of the component). I think this is already mostly the case, though
chrome/common/pref_names.h does import the metrics pref names file, which we
should probably change.

I think if we wanted to do it the way you suggest, we should do it all at once
for all these prefs, so that there's consistency, rather than one-offs which
results in an inconsistent intermediate state. If we want to do it all at once,
it should be in a separate CL.

So for now, let's keep the name where it is and if you want, add a comment
saying that it should only be accessed through CleanExitBeacon.

https://codereview.chromium.org/558683002/diff/60001/components/metrics/clean...
components/metrics/clean_exit_beacon.cc:25: #if defined(OS_WIN)
On 2014/09/11 15:39:14, erikwright wrote:
> On 2014/09/11 15:23:28, Alexei Svitkine wrote:
> > There's too many ifdefs for my preference.
> > 
> > How about we make the client method available on all platforms, but return
an
> > empty string on others.
> > 
> > We can also provide a default implementation that returns an empty string,
so
> > that clients that don't care about Windows (e.g. Chromecast), don't need to
> > implement it.
> 
> It seems a bit strange to declare and implement (even just by inheritance) a
> method whose sole purpose is to return a Windows registry key on non-Windows
> platforms. That is partially why I felt making the storage method (in addition
> to the location) abstract was better.

My original thought would have been for the method to be an accessor for
PRODUCT_STRING_PATH. Then, it wouldn't need to be tied to Windows registry and
could also be re-used for other things later, if desired. Maybe let's do that
instead?

> 
> > Then, we can eliminate most of the ifdefs. We'd only need to keep them in
the
> > implementation of this file, and even here, we can just put it around the
body
> > of the code (rather than around the method signature itself).
> 
> I'm not quite sure. Because you will end up with an unused member, which Clang
> may warn about. Or else (if you ifdef the member declaration and assignment)
you
> will have an unused parameter (which I believe will not lead to a warning).

I don't think we have warnings about unused members.

[erikwright (departed), 2014-09-11 16:03:14]

https://codereview.chromium.org/558683002/diff/60001/components/metrics/clean...
File components/metrics/clean_exit_beacon.cc (right):

https://codereview.chromium.org/558683002/diff/60001/components/metrics/clean...
components/metrics/clean_exit_beacon.cc:25: #if defined(OS_WIN)
On 2014/09/11 15:57:28, Alexei Svitkine wrote:
> On 2014/09/11 15:39:14, erikwright wrote:
> > On 2014/09/11 15:23:28, Alexei Svitkine wrote:
> > > There's too many ifdefs for my preference.
> > > 
> > > How about we make the client method available on all platforms, but return
> an
> > > empty string on others.
> > > 
> > > We can also provide a default implementation that returns an empty string,
> so
> > > that clients that don't care about Windows (e.g. Chromecast), don't need
to
> > > implement it.
> > 
> > It seems a bit strange to declare and implement (even just by inheritance) a
> > method whose sole purpose is to return a Windows registry key on non-Windows
> > platforms. That is partially why I felt making the storage method (in
addition
> > to the location) abstract was better.
> 
> My original thought would have been for the method to be an accessor for
> PRODUCT_STRING_PATH. Then, it wouldn't need to be tied to Windows registry and
> could also be re-used for other things later, if desired. Maybe let's do that
> instead?

I think the location used for storage in the registry should be fully controlled
by the embedder, as it is desired that it be the same as all other registry
usage of the product. Remember you would need both the vendor and the product
name, and to know how to compose them (Google\Chrome or just Chromium) in order
to get the same result as elsewhere.

> > > Then, we can eliminate most of the ifdefs. We'd only need to keep them in
> the
> > > implementation of this file, and even here, we can just put it around the
> body
> > > of the code (rather than around the method signature itself).
> > 
> > I'm not quite sure. Because you will end up with an unused member, which
Clang
> > may warn about. Or else (if you ifdef the member declaration and assignment)
> you
> > will have an unused parameter (which I believe will not lead to a warning).
> 
> I don't think we have warnings about unused members.

[Alexei Svitkine (slow), 2014-09-11 17:07:19]

https://codereview.chromium.org/558683002/diff/60001/components/metrics/clean...
File components/metrics/clean_exit_beacon.cc (right):

https://codereview.chromium.org/558683002/diff/60001/components/metrics/clean...
components/metrics/clean_exit_beacon.cc:25: #if defined(OS_WIN)
On 2014/09/11 16:03:14, erikwright wrote:
> On 2014/09/11 15:57:28, Alexei Svitkine wrote:
> > On 2014/09/11 15:39:14, erikwright wrote:
> > > On 2014/09/11 15:23:28, Alexei Svitkine wrote:
> > > > There's too many ifdefs for my preference.
> > > > 
> > > > How about we make the client method available on all platforms, but
return
> > an
> > > > empty string on others.
> > > > 
> > > > We can also provide a default implementation that returns an empty
string,
> > so
> > > > that clients that don't care about Windows (e.g. Chromecast), don't need
> to
> > > > implement it.
> > > 
> > > It seems a bit strange to declare and implement (even just by inheritance)
a
> > > method whose sole purpose is to return a Windows registry key on
non-Windows
> > > platforms. That is partially why I felt making the storage method (in
> addition
> > > to the location) abstract was better.
> > 
> > My original thought would have been for the method to be an accessor for
> > PRODUCT_STRING_PATH. Then, it wouldn't need to be tied to Windows registry
and
> > could also be re-used for other things later, if desired. Maybe let's do
that
> > instead?
> 
> I think the location used for storage in the registry should be fully
controlled
> by the embedder, as it is desired that it be the same as all other registry
> usage of the product. Remember you would need both the vendor and the product
> name, and to know how to compose them (Google\Chrome or just Chromium) in
order
> to get the same result as elsewhere.

Okay, then let's go with the with my suggestion of just having a default impl of
that method that returns an empty string, which should minimize the amount of
ifdefs.

> 
> > > > Then, we can eliminate most of the ifdefs. We'd only need to keep them
in
> > the
> > > > implementation of this file, and even here, we can just put it around
the
> > body
> > > > of the code (rather than around the method signature itself).
> > > 
> > > I'm not quite sure. Because you will end up with an unused member, which
> Clang
> > > may warn about. Or else (if you ifdef the member declaration and
assignment)
> > you
> > > will have an unused parameter (which I believe will not lead to a
warning).
> > 
> > I don't think we have warnings about unused members.

[erikwright (departed), 2014-09-11 19:00:29]

PTAL.

https://codereview.chromium.org/558683002/diff/60001/components/metrics/clean...
File components/metrics/clean_exit_beacon.cc (right):

https://codereview.chromium.org/558683002/diff/60001/components/metrics/clean...
components/metrics/clean_exit_beacon.cc:21:
"user_experience_metrics.stability.exited_cleanly";
On 2014/09/11 15:57:28, Alexei Svitkine wrote:
> On 2014/09/11 15:39:14, erikwright wrote:
> > On 2014/09/11 15:23:28, Alexei Svitkine wrote:
> > > What's the reason for moving the pref? I think we want to keep a central
> place
> > > where all the metrics prefs are listed, so I prefer this is moved back.
> > 
> > Exposing the name of a preference permits violation of encapsulation. This
> code
> > encapsulates the access to this value, and it would be inappropriate for any
> > other code to directly read/write the value. Hiding the name of the pref
> > enforces that. This is a pattern that I've used elsewhere and I think should
> be
> > generally adopted any time it is not reasonable for arbitrary code to
> manipulate
> > a preference value.
> 
> I agree in principle, but at the same time we lose the benefit of seeing all
the
> prefs in one place.
> 
> In practice, I think for this component, I'd rather keep all the names in one
> place but have them private to the component (i.e. not used by anything
outside
> of the component). I think this is already mostly the case, though
> chrome/common/pref_names.h does import the metrics pref names file, which we
> should probably change.
> 
> I think if we wanted to do it the way you suggest, we should do it all at once
> for all these prefs, so that there's consistency, rather than one-offs which
> results in an inconsistent intermediate state. If we want to do it all at
once,
> it should be in a separate CL.
> 
> So for now, let's keep the name where it is and if you want, add a comment
> saying that it should only be accessed through CleanExitBeacon.

Done. I didn't add a comment, as it's not really any different from the other
constants in that file.

https://codereview.chromium.org/558683002/diff/60001/components/metrics/clean...
components/metrics/clean_exit_beacon.cc:25: #if defined(OS_WIN)
On 2014/09/11 17:07:19, Alexei Svitkine wrote:
> On 2014/09/11 16:03:14, erikwright wrote:
> > On 2014/09/11 15:57:28, Alexei Svitkine wrote:
> > > On 2014/09/11 15:39:14, erikwright wrote:
> > > > On 2014/09/11 15:23:28, Alexei Svitkine wrote:
> > > > > There's too many ifdefs for my preference.
> > > > > 
> > > > > How about we make the client method available on all platforms, but
> return
> > > an
> > > > > empty string on others.
> > > > > 
> > > > > We can also provide a default implementation that returns an empty
> string,
> > > so
> > > > > that clients that don't care about Windows (e.g. Chromecast), don't
need
> > to
> > > > > implement it.
> > > > 
> > > > It seems a bit strange to declare and implement (even just by
inheritance)
> a
> > > > method whose sole purpose is to return a Windows registry key on
> non-Windows
> > > > platforms. That is partially why I felt making the storage method (in
> > addition
> > > > to the location) abstract was better.
> > > 
> > > My original thought would have been for the method to be an accessor for
> > > PRODUCT_STRING_PATH. Then, it wouldn't need to be tied to Windows registry
> and
> > > could also be re-used for other things later, if desired. Maybe let's do
> that
> > > instead?
> > 
> > I think the location used for storage in the registry should be fully
> controlled
> > by the embedder, as it is desired that it be the same as all other registry
> > usage of the product. Remember you would need both the vendor and the
product
> > name, and to know how to compose them (Google\Chrome or just Chromium) in
> order
> > to get the same result as elsewhere.
> 
> Okay, then let's go with the with my suggestion of just having a default impl
of
> that method that returns an empty string, which should minimize the amount of
> ifdefs.
> 
> > 
> > > > > Then, we can eliminate most of the ifdefs. We'd only need to keep them
> in
> > > the
> > > > > implementation of this file, and even here, we can just put it around
> the
> > > body
> > > > > of the code (rather than around the method signature itself).
> > > > 
> > > > I'm not quite sure. Because you will end up with an unused member, which
> > Clang
> > > > may warn about. Or else (if you ifdef the member declaration and
> assignment)
> > > you
> > > > will have an unused parameter (which I believe will not lead to a
> warning).
> > > 
> > > I don't think we have warnings about unused members.
> 

Done.

https://codereview.chromium.org/558683002/diff/60001/components/metrics/clean...
File components/metrics/clean_exit_beacon.h (right):

https://codereview.chromium.org/558683002/diff/60001/components/metrics/clean...
components/metrics/clean_exit_beacon.h:10: #if defined(OS_WIN)
On 2014/09/11 15:23:28, Alexei Svitkine wrote:
> Nit: Remove ifdef. You don't have it in metrics_service_client.h anyway.

Done.

https://codereview.chromium.org/558683002/diff/60001/components/metrics/clean...
components/metrics/clean_exit_beacon.h:38: bool exited_cleanly() { return
initial_value_; }
On 2014/09/11 15:23:28, Alexei Svitkine wrote:
> Nit: const

Done.

https://codereview.chromium.org/558683002/diff/60001/components/metrics/metri...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/558683002/diff/60001/components/metrics/metri...
components/metrics/metrics_service.cc:523:
clean_exit_beacon_->WriteBeaconValue(false);
On 2014/09/11 15:23:28, Alexei Svitkine wrote:
> Shouldn't this be . instead of ->?

Done.

https://codereview.chromium.org/558683002/diff/60001/components/metrics/metri...
components/metrics/metrics_service.cc:574: if
(!clean_exit_beacon_.exited_cleanly()) {
On 2014/09/11 15:23:28, Alexei Svitkine wrote:
> Note: You need to sync/rebase on top of siggi's change that recently landed.

Done.

[Alexei Svitkine (slow), 2014-09-11 19:20:27]

https://codereview.chromium.org/558683002/diff/120001/components/metrics/clea...
File components/metrics/clean_exit_beacon.cc (right):

https://codereview.chromium.org/558683002/diff/120001/components/metrics/clea...
components/metrics/clean_exit_beacon.cc:15: #endif
Nit: Add a newline after this.

https://codereview.chromium.org/558683002/diff/120001/components/metrics/clea...
components/metrics/clean_exit_beacon.cc:26: #if defined(OS_WIN)
Nit: Add a newline before this.

https://codereview.chromium.org/558683002/diff/120001/components/metrics/clea...
components/metrics/clean_exit_beacon.cc:27: enum {
Add a comment explaining the order e.g. what does CLEAN_DIRTY mean).

https://codereview.chromium.org/558683002/diff/120001/components/metrics/clea...
components/metrics/clean_exit_beacon.cc:63: local_state_->CommitPendingWrite();
I don't think the previous code did this.

Do we need this functional change in this CL if we just want to measure the
beacon consistency?

[erikwright (departed), 2014-09-11 19:42:20]

PTAL.

https://codereview.chromium.org/558683002/diff/120001/components/metrics/clea...
File components/metrics/clean_exit_beacon.cc (right):

https://codereview.chromium.org/558683002/diff/120001/components/metrics/clea...
components/metrics/clean_exit_beacon.cc:15: #endif
On 2014/09/11 19:20:26, Alexei Svitkine wrote:
> Nit: Add a newline after this.

Done.

https://codereview.chromium.org/558683002/diff/120001/components/metrics/clea...
components/metrics/clean_exit_beacon.cc:26: #if defined(OS_WIN)
On 2014/09/11 19:20:26, Alexei Svitkine wrote:
> Nit: Add a newline before this.

Done.

https://codereview.chromium.org/558683002/diff/120001/components/metrics/clea...
components/metrics/clean_exit_beacon.cc:27: enum {
On 2014/09/11 19:20:26, Alexei Svitkine wrote:
> Add a comment explaining the order e.g. what does CLEAN_DIRTY mean).

Done.

https://codereview.chromium.org/558683002/diff/120001/components/metrics/clea...
components/metrics/clean_exit_beacon.cc:63: local_state_->CommitPendingWrite();
On 2014/09/11 19:20:26, Alexei Svitkine wrote:
> I don't think the previous code did this.
> 
> Do we need this functional change in this CL if we just want to measure the
> beacon consistency?

No, this is not needed for now.

[Alexei Svitkine (slow), 2014-09-11 19:49:12]

LGTM

[erikwright (departed), 2014-09-11 22:05:57]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2014-09-11 22:24:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/558683002/140001

[commit-bot: I haz the power, 2014-09-12 00:08:00]

Committed patchset #8 (id:140001) as 2c610dc2b5250278153a4dd8a9e96aefe52609fa

[commit-bot: I haz the power, 2014-09-12 00:22:20]

Patchset 8 (id:??) landed as
https://crrev.com/65b58df31c75ef627cdff2a2ee377330fd86c2b4
Cr-Commit-Position: refs/heads/master@{#294498}