Refactor net::BackoffEntry to not require subclassing

Refactor net::BackoffEntry to not require subclassing

Before this patch, net::BackoffEntry had a virtual ImplGetTimeNow method
that tests etc would override to change what time is considered "now".

As suggested by rsleevi in https://codereview.chromium.org/1023473003,
this patch removes that method, and instead makes net::BackoffEntry
accept a base::TickClock in the constructor, to allow overriding the
time without subclassing.

(this will allow future changes to net::BackoffEntry without the
fragile base class problem)

Accordingly, I've removed all subclasses of BackoffEntry, and made them
pass TickClocks instead; in most cases this has been a nice
simplification.

BUG=465399
TBR=stevenjb@chromium.org

Committed: https://crrev.com/dce40c39e0d44436283fbc43fa9b708153dd04e6
Cr-Commit-Position: refs/heads/master@{#325865}

[johnme, 2015-04-10 12:02:16]

johnme@chromium.org changed reviewers:
+ mmenke@chromium.org

[johnme, 2015-04-10 12:02:17]

mmenke: please review overall patch, and give owners approval for:
- net/
- chrome/browser/captive_portal/
- components/domain_reliability/

Thanks!

[mmenke, 2015-04-10 15:50:43]

https://codereview.chromium.org/1076853003/diff/80001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h
(right):

https://codereview.chromium.org/1076853003/diff/80001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:136:
mutable TestTickClock tick_clock_;
Why does this have to be mutable?

https://codereview.chromium.org/1076853003/diff/80001/net/base/backoff_entry_...
File net/base/backoff_entry_unittest.cc (right):

https://codereview.chromium.org/1076853003/diff/80001/net/base/backoff_entry_...
net/base/backoff_entry_unittest.cc:24: TimeTicks now_ticks_;
DISALLOW_COPY_AND_ASSIGN?  (+ include base/macros.h for it)

https://codereview.chromium.org/1076853003/diff/80001/net/url_request/url_req...
File net/url_request/url_request_throttler_test_support.h (right):

https://codereview.chromium.org/1076853003/diff/80001/net/url_request/url_req...
net/url_request/url_request_throttler_test_support.h:27: base::TimeTicks
now_ticks_;
DISALLOW_COPY_AND_ASSIGN?  (+ include base/macros.h for it)

[mmenke, 2015-04-10 15:52:13]

Also, thanks for doing this - I think the code looks cleaner with this in place.

[johnme, 2015-04-10 16:23:36]

Addressed review comments - thanks :)

https://codereview.chromium.org/1076853003/diff/80001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h
(right):

https://codereview.chromium.org/1076853003/diff/80001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:136:
mutable TestTickClock tick_clock_;
On 2015/04/10 15:50:43, mmenke wrote:
> Why does this have to be mutable?

It was because base::Clock::Now is non-const, hence TestTickClock::Now must be
non-const, but TestDataReductionProxyConfigServiceClient::Now must be const
since DataReductionProxyConfigServiceClient::Now is const.

I've made TestTickClock no longer inherit from base::Clock (I only added that
for symmetry), so Now can be const again and it no longer needs to be mutable.
However this is now a little asymmetric, with one method const and the other
non-const :-|

Alternatively, if you'd rather, I could make
DataReductionProxyConfigServiceClient::Now be non-const, as it's never currently
used in a const manner?

https://codereview.chromium.org/1076853003/diff/80001/net/base/backoff_entry_...
File net/base/backoff_entry_unittest.cc (right):

https://codereview.chromium.org/1076853003/diff/80001/net/base/backoff_entry_...
net/base/backoff_entry_unittest.cc:24: TimeTicks now_ticks_;
On 2015/04/10 15:50:43, mmenke wrote:
> DISALLOW_COPY_AND_ASSIGN?  (+ include base/macros.h for it)

Done.

https://codereview.chromium.org/1076853003/diff/80001/net/url_request/url_req...
File net/url_request/url_request_throttler_test_support.h (right):

https://codereview.chromium.org/1076853003/diff/80001/net/url_request/url_req...
net/url_request/url_request_throttler_test_support.h:27: base::TimeTicks
now_ticks_;
On 2015/04/10 15:50:43, mmenke wrote:
> DISALLOW_COPY_AND_ASSIGN?  (+ include base/macros.h for it)

Done.

[mmenke, 2015-04-10 16:26:37]

Still LGTM

https://codereview.chromium.org/1076853003/diff/80001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h
(right):

https://codereview.chromium.org/1076853003/diff/80001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:136:
mutable TestTickClock tick_clock_;
On 2015/04/10 16:23:36, johnme wrote:
> On 2015/04/10 15:50:43, mmenke wrote:
> > Why does this have to be mutable?
> 
> It was because base::Clock::Now is non-const, hence TestTickClock::Now must be
> non-const, but TestDataReductionProxyConfigServiceClient::Now must be const
> since DataReductionProxyConfigServiceClient::Now is const.
> 
> I've made TestTickClock no longer inherit from base::Clock (I only added that
> for symmetry), so Now can be const again and it no longer needs to be mutable.
> However this is now a little asymmetric, with one method const and the other
> non-const :-|
> 
> Alternatively, if you'd rather, I could make
> DataReductionProxyConfigServiceClient::Now be non-const, as it's never
currently
> used in a const manner?

I'd go ahead and make DataReductionProxyConfigServiceClient::Now non-const, for
simplicity, but don't have strong feelings on the subject.

[johnme, 2015-04-10 16:48:48]

On 2015/04/10 16:26:37, mmenke wrote:
> Still LGTM
> 
>
https://codereview.chromium.org/1076853003/diff/80001/components/data_reducti...
> File
> components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h
> (right):
> 
>
https://codereview.chromium.org/1076853003/diff/80001/components/data_reducti...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:136:
> mutable TestTickClock tick_clock_;
> On 2015/04/10 16:23:36, johnme wrote:
> > On 2015/04/10 15:50:43, mmenke wrote:
> > > Why does this have to be mutable?
> > 
> > It was because base::Clock::Now is non-const, hence TestTickClock::Now must
be
> > non-const, but TestDataReductionProxyConfigServiceClient::Now must be const
> > since DataReductionProxyConfigServiceClient::Now is const.
> > 
> > I've made TestTickClock no longer inherit from base::Clock (I only added
that
> > for symmetry), so Now can be const again and it no longer needs to be
mutable.
> > However this is now a little asymmetric, with one method const and the other
> > non-const :-|
> > 
> > Alternatively, if you'd rather, I could make
> > DataReductionProxyConfigServiceClient::Now be non-const, as it's never
> currently
> > used in a const manner?
> 
> I'd go ahead and make DataReductionProxyConfigServiceClient::Now non-const,
for
> simplicity, but don't have strong feelings on the subject.

Done, thanks :)

[johnme, 2015-04-10 17:03:55]

johnme@chromium.org changed reviewers:
+ sclittle@chromium.org, stevenjb@chromium.org, vabr@chromium.org,
zea@chromium.org

[johnme, 2015-04-10 17:03:55]

stevenjb: please give OWNERS approval for chromeos/network/ &
chrome/browser/chromeos/net/

sclittle: please give OWNERS approval for components/data_reduction_proxy/

zea: I've TBR'd you for
google_apis/gcm/engine/connection_factory_impl_unittest.cc as that change is so
simple.

vabr: I've TBR'd you for
components/password_manager/core/browser/affiliation_fetch_throttler.cc as that
change is so simple.

[vabr (Chromium), 2015-04-10 17:15:48]

components/password_manager/core/browser/affiliation_fetch_throttler.cc LGTM

[sclittle, 2015-04-10 17:56:02]

sclittle@chromium.org changed reviewers:
+ jeremyim@chromium.org

[sclittle, 2015-04-10 17:56:02]

jeremyim@ is more familiar with that part of the data_reduction_proxy code; I'm
tagging him in as a reviewer.

[jeremyim, 2015-04-10 18:27:08]

On 2015/04/10 17:56:02, sclittle wrote:
> jeremyim@ is more familiar with that part of the data_reduction_proxy code;
I'm
> tagging him in as a reviewer.

components/data_reduction_proxy LGTM

[Nicolas Zea, 2015-04-10 20:08:20]

gcm LGTM

[stevenjb, 2015-04-13 15:20:09]

stevenjb@chromium.org changed reviewers:
+ pneubeck@chromium.org, ygorshenin@chromium.org

[stevenjb, 2015-04-13 15:20:11]

https://codereview.chromium.org/1076853003/diff/120001/chromeos/network/porta...
File chromeos/network/portal_detector/network_portal_detector_strategy.h
(right):

https://codereview.chromium.org/1076853003/diff/120001/chromeos/network/porta...
chromeos/network/portal_detector/network_portal_detector_strategy.h:27: class
Delegate : public base::TickClock {
I'm not actually very familiar with this code, but having a Delegate derive from
TickClock seems strange. +pneubeck@, +ygorshenin@ who are more familiar with
this code.

[pneubeck (no reviews), 2015-04-14 10:27:05]

https://codereview.chromium.org/1076853003/diff/120001/chrome/browser/captive...
File chrome/browser/captive_portal/captive_portal_service.h (right):

https://codereview.chromium.org/1076853003/diff/120001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_service.h:30: private
base::TickClock {
as you've hidden TickClock::NowTicks below, TickClock should apparently not be
part of the _public_ interface of CaptivePortalService.

Thus it should be an implementation detail and you'd have to delegate to a
member instead of using inheritance.
RecheckTickClock would be exactly that internal implementation, but is currently
unused. Please, move CaptivePortalService::NowTicks to RecheckTickClock,
instantiate one RecheckTickClock member and pass that to the BackoffEntry.

Ideally, the |base::TimeTicks time_ticks_for_testing_;| part would be ripped out
of this class and instead a TickClock would be passed as dependency to this
Service.

https://codereview.chromium.org/1076853003/diff/120001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_service.h:85: //
GetCurrentTimeTicks function, for unit testing.
this function doesn't exist anymore.

https://codereview.chromium.org/1076853003/diff/120001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_service.h:86: class
RecheckTickClock;
unused

https://codereview.chromium.org/1076853003/diff/120001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_service.h:143: // base::TickClock
implementation.
visibility of inherited functions should usually not be changed.

https://codereview.chromium.org/1076853003/diff/120001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_service.h:158: void
set_time_ticks_for_testing(const base::TimeTicks& time_ticks) {
bad style. should be passed as TickClock dependency to the service.

https://codereview.chromium.org/1076853003/diff/120001/chromeos/network/porta...
File chromeos/network/portal_detector/network_portal_detector_strategy.h
(right):

https://codereview.chromium.org/1076853003/diff/120001/chromeos/network/porta...
chromeos/network/portal_detector/network_portal_detector_strategy.h:27: class
Delegate : public base::TickClock {
On 2015/04/13 15:20:10, stevenjb wrote:
> I'm not actually very familiar with this code, but having a Delegate derive
from
> TickClock seems strange. +pneubeck@, +ygorshenin@ who are more familiar with
> this code.

Looking more into the usage of this, I think it might be best to have a
  // The returned object lives (at least) until the destruction of the
delegate.
  base::TickClock* GetClock() = 0;

This allows one to pass _in_ an existing TickClock without having to know about
the actual TickClock interface.
E.g. NetworkPortalDetectorImpl::NowTicks should in turn take a TickClock for
testing and just pass it on, instead of maintaining a time_ticks_for_testing_
member.


As this is probably affecting more code than what the objective of this CL is,
feel free to go with your current implementation. Which I think is fine in this
corner case where TickClock has only a single method.

https://codereview.chromium.org/1076853003/diff/120001/chromeos/network/porta...
chromeos/network/portal_detector/network_portal_detector_strategy.h:38: //
TickClock implementation.
nit: not required to repeat abstract functions.
I'd just remove these two lines.

https://codereview.chromium.org/1076853003/diff/120001/chromeos/network/porta...
chromeos/network/portal_detector/network_portal_detector_strategy.h:44: static
scoped_ptr<PortalDetectorStrategy> CreateById(StrategyId id,
as you probably had to read/understand this code, please add a comment about the
expected lifetime of |delegate|.

https://codereview.chromium.org/1076853003/diff/120001/net/base/backoff_entry.cc
File net/base/backoff_entry.cc (right):

https://codereview.chromium.org/1076853003/diff/120001/net/base/backoff_entry...
net/base/backoff_entry.cc:112: // (meaning 0). It would also be reasonable to
reset it to GetTimeTicksNow().
"to null (meaning 0)" -> "to the uninitialized state" (or similar)

'meaning 0' is not helpful without reading the implementation of TimeTicks

https://codereview.chromium.org/1076853003/diff/120001/net/base/backoff_entry.h
File net/base/backoff_entry.h (right):

https://codereview.chromium.org/1076853003/diff/120001/net/base/backoff_entry...
net/base/backoff_entry.h:64: explicit BackoffEntry(const Policy* const policy);
arguments are usually not declared const.
(same for the next c'tor)

https://codereview.chromium.org/1076853003/diff/120001/net/base/backoff_entry...
net/base/backoff_entry.h:65: BackoffEntry(const Policy* const policy,
base::TickClock* clock);
this needs a comment about the lifetime of |clock| as well.

as there's a separate constructor with a single argument, you should probably
require |clock| to be not null.

https://codereview.chromium.org/1076853003/diff/120001/net/base/backoff_entry...
net/base/backoff_entry.h:114: base::TickClock* clock_;  // Not owned.
should this ever change during the lifetime of the BackoffEntry?
if not, it should be 
  base::TickClock* const clock_;

[johnme, 2015-04-20 15:52:45]

Addressed pneubeck's review comments - thanks!

https://codereview.chromium.org/1076853003/diff/120001/chrome/browser/captive...
File chrome/browser/captive_portal/captive_portal_service.h (right):

https://codereview.chromium.org/1076853003/diff/120001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_service.h:30: private
base::TickClock {
On 2015/04/14 10:27:04, pneubeck wrote:
> as you've hidden TickClock::NowTicks below, TickClock should apparently not be
> part of the _public_ interface of CaptivePortalService.
> 
> Thus it should be an implementation detail and you'd have to delegate to a
> member instead of using inheritance.
> RecheckTickClock would be exactly that internal implementation, but is
currently
> unused. Please, move CaptivePortalService::NowTicks to RecheckTickClock,
> instantiate one RecheckTickClock member and pass that to the BackoffEntry.
> 
> Ideally, the |base::TimeTicks time_ticks_for_testing_;| part would be ripped
out
> of this class and instead a TickClock would be passed as dependency to this
> Service.

Note that I was using private inheritance, so AFAIK keeping NowTicks private was
fine (indeed, the tests compiled and passed). But I've refactored this to take a
TickClock dependency, since I agree that that's cleaner.

I didn't need RecheckTickClock in the end, so I've deleted it.

https://codereview.chromium.org/1076853003/diff/120001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_service.h:85: //
GetCurrentTimeTicks function, for unit testing.
On 2015/04/14 10:27:04, pneubeck wrote:
> this function doesn't exist anymore.

Done (removed comment).

https://codereview.chromium.org/1076853003/diff/120001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_service.h:86: class
RecheckTickClock;
On 2015/04/14 10:27:04, pneubeck wrote:
> unused

Done (removed).

https://codereview.chromium.org/1076853003/diff/120001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_service.h:143: // base::TickClock
implementation.
On 2015/04/14 10:27:04, pneubeck wrote:
> visibility of inherited functions should usually not be changed.

Done (it was private inheritance, as explained above; but I've refactored this
to be has-a-TickClock instead of is-a-TickClock).

https://codereview.chromium.org/1076853003/diff/120001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_service.h:158: void
set_time_ticks_for_testing(const base::TimeTicks& time_ticks) {
On 2015/04/14 10:27:04, pneubeck wrote:
> bad style. should be passed as TickClock dependency to the service.

Done.

https://codereview.chromium.org/1076853003/diff/120001/chromeos/network/porta...
File chromeos/network/portal_detector/network_portal_detector_strategy.h
(right):

https://codereview.chromium.org/1076853003/diff/120001/chromeos/network/porta...
chromeos/network/portal_detector/network_portal_detector_strategy.h:27: class
Delegate : public base::TickClock {
On 2015/04/14 10:27:04, pneubeck wrote:
> On 2015/04/13 15:20:10, stevenjb wrote:
> > I'm not actually very familiar with this code, but having a Delegate derive
> from
> > TickClock seems strange. +pneubeck@, +ygorshenin@ who are more familiar with
> > this code.
> 
> Looking more into the usage of this, I think it might be best to have a
>   // The returned object lives (at least) until the destruction of the
delegate.
>   base::TickClock* GetClock() = 0;
> 
> This allows one to pass _in_ an existing TickClock without having to know
about
> the actual TickClock interface.
> E.g. NetworkPortalDetectorImpl::NowTicks should in turn take a TickClock for
> testing and just pass it on, instead of maintaining a time_ticks_for_testing_
> member.
> 
> 
> As this is probably affecting more code than what the objective of this CL is,
> feel free to go with your current implementation. Which I think is fine in
this
> corner case where TickClock has only a single method.

Acknowledged.

https://codereview.chromium.org/1076853003/diff/120001/chromeos/network/porta...
chromeos/network/portal_detector/network_portal_detector_strategy.h:38: //
TickClock implementation.
On 2015/04/14 10:27:04, pneubeck wrote:
> nit: not required to repeat abstract functions.
> I'd just remove these two lines.

Done.

https://codereview.chromium.org/1076853003/diff/120001/net/base/backoff_entry.cc
File net/base/backoff_entry.cc (right):

https://codereview.chromium.org/1076853003/diff/120001/net/base/backoff_entry...
net/base/backoff_entry.cc:112: // (meaning 0). It would also be reasonable to
reset it to GetTimeTicksNow().
On 2015/04/14 10:27:04, pneubeck wrote:
> "to null (meaning 0)" -> "to the uninitialized state" (or similar)
> 
> 'meaning 0' is not helpful without reading the implementation of TimeTicks

Done.

https://codereview.chromium.org/1076853003/diff/120001/net/base/backoff_entry.h
File net/base/backoff_entry.h (right):

https://codereview.chromium.org/1076853003/diff/120001/net/base/backoff_entry...
net/base/backoff_entry.h:64: explicit BackoffEntry(const Policy* const policy);
On 2015/04/14 10:27:04, pneubeck wrote:
> arguments are usually not declared const.
> (same for the next c'tor)

Done.

https://codereview.chromium.org/1076853003/diff/120001/net/base/backoff_entry...
net/base/backoff_entry.h:65: BackoffEntry(const Policy* const policy,
base::TickClock* clock);
On 2015/04/14 10:27:04, pneubeck wrote:
> this needs a comment about the lifetime of |clock| as well.
> 
> as there's a separate constructor with a single argument, you should probably
> require |clock| to be not null.

Added a comment. Requiring non-null |clock| seems a little awkward though, since
it requires callers such as CaptivePortalService::ResetBackoffEntry to
null-check the TickClocks they are passing through, and call a different
constructor accordingly (it also prevents using C++11 delegated constructors
here). Instead I commented that it's ok for clock to be null.

https://codereview.chromium.org/1076853003/diff/120001/net/base/backoff_entry...
net/base/backoff_entry.h:114: base::TickClock* clock_;  // Not owned.
On 2015/04/14 10:27:04, pneubeck wrote:
> should this ever change during the lifetime of the BackoffEntry?
> if not, it should be 
>   base::TickClock* const clock_;

Done.

[johnme, 2015-04-20 15:57:55]

Forgot to reply to a comment.

https://codereview.chromium.org/1076853003/diff/120001/chromeos/network/porta...
File chromeos/network/portal_detector/network_portal_detector_strategy.h
(right):

https://codereview.chromium.org/1076853003/diff/120001/chromeos/network/porta...
chromeos/network/portal_detector/network_portal_detector_strategy.h:44: static
scoped_ptr<PortalDetectorStrategy> CreateById(StrategyId id,
On 2015/04/14 10:27:04, pneubeck wrote:
> as you probably had to read/understand this code, please add a comment about
the
> expected lifetime of |delegate|.

Done.

[pneubeck (no reviews), 2015-04-20 16:35:50]

lgtm

https://codereview.chromium.org/1076853003/diff/120001/chrome/browser/captive...
File chrome/browser/captive_portal/captive_portal_service.h (right):

https://codereview.chromium.org/1076853003/diff/120001/chrome/browser/captive...
chrome/browser/captive_portal/captive_portal_service.h:30: private
base::TickClock {
On 2015/04/20 15:52:44, johnme wrote:
> On 2015/04/14 10:27:04, pneubeck wrote:
> > as you've hidden TickClock::NowTicks below, TickClock should apparently not
be
> > part of the _public_ interface of CaptivePortalService.
> > 
> > Thus it should be an implementation detail and you'd have to delegate to a
> > member instead of using inheritance.
> > RecheckTickClock would be exactly that internal implementation, but is
> currently
> > unused. Please, move CaptivePortalService::NowTicks to RecheckTickClock,
> > instantiate one RecheckTickClock member and pass that to the BackoffEntry.
> > 
> > Ideally, the |base::TimeTicks time_ticks_for_testing_;| part would be ripped
> out
> > of this class and instead a TickClock would be passed as dependency to this
> > Service.
> 
> Note that I was using private inheritance, so AFAIK keeping NowTicks private
was
> fine (indeed, the tests compiled and passed). But I've refactored this to take
a
> TickClock dependency, since I agree that that's cleaner.
> 
> I didn't need RecheckTickClock in the end, so I've deleted it.

Ha! I didn't even see that. Private inheritance is not allowed by the
styleguide:
"All inheritance should be public. If you want to do private inheritance, you
should be including an instance of the base class as a member instead."
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html

https://codereview.chromium.org/1076853003/diff/120001/net/base/backoff_entry.h
File net/base/backoff_entry.h (right):

https://codereview.chromium.org/1076853003/diff/120001/net/base/backoff_entry...
net/base/backoff_entry.h:65: BackoffEntry(const Policy* const policy,
base::TickClock* clock);
On 2015/04/20 15:52:44, johnme wrote:
> On 2015/04/14 10:27:04, pneubeck wrote:
> > this needs a comment about the lifetime of |clock| as well.
> > 
> > as there's a separate constructor with a single argument, you should
probably
> > require |clock| to be not null.
> 
> Added a comment. Requiring non-null |clock| seems a little awkward though,
since
> it requires callers such as CaptivePortalService::ResetBackoffEntry to
> null-check the TickClocks they are passing through, and call a different
> constructor accordingly (it also prevents using C++11 delegated constructors
> here). Instead I commented that it's ok for clock to be null.

Doesn't sound like a convincing argument to me (to adapt the interface to the
implementation; it should be the other way round). But I'm not an owner of this
code.

[johnme, 2015-04-20 17:00:29]

Thanks all for the reviews!

TBR'ing stevenjb@chromium.org since he delegated to pneubeck, who gave his lgtm.

[johnme, 2015-04-20 17:00:36]

The CQ bit was checked by johnme@chromium.org

[johnme, 2015-04-20 17:00:37]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
vabr@chromium.org, jeremyim@chromium.org, zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1076853003/#ps140001
(title: "Address pneubeck's review comments")

[commit-bot: I haz the power, 2015-04-20 17:01:41]

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

[commit-bot: I haz the power, 2015-04-20 17:06:27]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-04-20 17:07:13]

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