Domain Reliability: Mark beacons from previous networks.

Domain Reliability: Mark beacons from previous networks.

To help Domain Reliability collectors know whether the client's IP
address is actually the address from which they experienced network
issues, label beacons with '"network_changed": true' if the client's
network connection changes between logging and reporting the beacon.

BUG=

Committed: https://crrev.com/743f4ae91ce9fb5fc523edd0acc4978c7ba5db85
Cr-Commit-Position: refs/heads/master@{#302655}

[Deprecated (see juliatuttle), 2014-10-30 18:18:29]

ttuttle@chromium.org changed reviewers:
+ davidben@chromium.org

[Deprecated (see juliatuttle), 2014-10-30 18:18:29]

ttuttle@chromium.org changed required reviewers:
+ davidben@chromium.org

[Deprecated (see juliatuttle), 2014-10-30 18:18:30]

PTAL, davidben.

[davidben, 2014-10-30 21:18:00]

https://codereview.chromium.org/691053003/diff/1/components/domain_reliabilit...
File components/domain_reliability/beacon.cc (right):

https://codereview.chromium.org/691053003/diff/1/components/domain_reliabilit...
components/domain_reliability/beacon.cc:44:
beacon_value->SetBoolean("network_changed", true);
If the beacon came up because the network changed while we were fetching the
resource and then that aborted the request, would it end up on the correct side?
Do we want to compare with start_time instead?

https://codereview.chromium.org/691053003/diff/1/components/domain_reliabilit...
File components/domain_reliability/context.h (right):

https://codereview.chromium.org/691053003/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:114: const base::TimeTicks&
last_network_change_time_;
It's really not obvious, certainly not from the constructor, that these guys are
all aliasing the parent's base::TimeTicks. (I thought at first there was no way
this code worked.) I don't think we use const references that way. Could it be
switched to a pointer and also documented? (Or perhaps it gets plumbed through
somewhere else?)

https://codereview.chromium.org/691053003/diff/1/components/domain_reliabilit...
File components/domain_reliability/context_unittest.cc (right):

https://codereview.chromium.org/691053003/diff/1/components/domain_reliabilit...
components/domain_reliability/context_unittest.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
This probably wants tests (not sure if here or elsewhere) to test that
NetworkChangeObserver behaves as expected. I'm not sure it does right now. There
seems to be a test API to simulate network changes.

https://codereview.chromium.org/691053003/diff/1/components/domain_reliabilit...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/691053003/diff/1/components/domain_reliabilit...
components/domain_reliability/monitor.cc:55:
net::NetworkChangeNotifier::AddNetworkChangeObserver(this);
I think this should be in InitURLRequestContext, otherwise the notifications
won't come on the right thread. And then maybe the destructor checks
|moved_to_network_thread_| because RemoveNetworkChangeObserver needs to be
called on the same thread.

[Deprecated (see juliatuttle), 2014-10-31 22:00:50]

Patchset #3 (id:40001) has been deleted

[Deprecated (see juliatuttle), 2014-10-31 22:01:02]

PTAL, davidben.

https://codereview.chromium.org/691053003/diff/1/components/domain_reliabilit...
File components/domain_reliability/beacon.cc (right):

https://codereview.chromium.org/691053003/diff/1/components/domain_reliabilit...
components/domain_reliability/beacon.cc:44:
beacon_value->SetBoolean("network_changed", true);
On 2014/10/30 21:17:59, David Benjamin wrote:
> If the beacon came up because the network changed while we were fetching the
> resource and then that aborted the request, would it end up on the correct
side?
> Do we want to compare with start_time instead?

Done.

https://codereview.chromium.org/691053003/diff/1/components/domain_reliabilit...
File components/domain_reliability/context.h (right):

https://codereview.chromium.org/691053003/diff/1/components/domain_reliabilit...
components/domain_reliability/context.h:114: const base::TimeTicks&
last_network_change_time_;
On 2014/10/30 21:18:00, David Benjamin wrote:
> It's really not obvious, certainly not from the constructor, that these guys
are
> all aliasing the parent's base::TimeTicks. (I thought at first there was no
way
> this code worked.) I don't think we use const references that way. Could it be
> switched to a pointer and also documented? (Or perhaps it gets plumbed through
> somewhere else?)

I'll make it a pointer.

https://codereview.chromium.org/691053003/diff/1/components/domain_reliabilit...
File components/domain_reliability/context_unittest.cc (right):

https://codereview.chromium.org/691053003/diff/1/components/domain_reliabilit...
components/domain_reliability/context_unittest.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2014/10/30 21:18:00, David Benjamin wrote:
> This probably wants tests (not sure if here or elsewhere) to test that
> NetworkChangeObserver behaves as expected. I'm not sure it does right now.
There
> seems to be a test API to simulate network changes.

The Context itself just bases it on the (shared) last_network_change_time, so I
added a test against that here.

[Deprecated (see juliatuttle), 2014-11-03 14:36:57]

PTAL, davidben.

[davidben, 2014-11-03 19:25:34]

Hrm. Try jobs seem to be failing. I think this might be because URLRequest isn't
using the mocked time, so they don't end up properly comparable.

https://codereview.chromium.org/691053003/diff/20001/components/domain_reliab...
File components/domain_reliability/context.h (right):

https://codereview.chromium.org/691053003/diff/20001/components/domain_reliab...
components/domain_reliability/context.h:39: const base::TimeTicks*
last_network_change_time,
Could you add a comment somewhere documenting why this is a pointer?

https://codereview.chromium.org/691053003/diff/20001/components/domain_reliab...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/691053003/diff/20001/components/domain_reliab...
components/domain_reliability/monitor.cc:152: last_network_change_time_ =
time_->NowTicks();
Perhaps a test in monitor_unittest.cc to provide coverage over this codepath?
You could call NotifyObserversOfNetworkChangeForTests to simulate a few of
these.

[Deprecated (see juliatuttle), 2014-11-04 17:56:52]

PTAL, davidben.

https://codereview.chromium.org/691053003/diff/20001/components/domain_reliab...
File components/domain_reliability/context.h (right):

https://codereview.chromium.org/691053003/diff/20001/components/domain_reliab...
components/domain_reliability/context.h:39: const base::TimeTicks*
last_network_change_time,
On 2014/11/03 19:25:33, David Benjamin wrote:
> Could you add a comment somewhere documenting why this is a pointer?

Done (well, to the member variable).

https://codereview.chromium.org/691053003/diff/20001/components/domain_reliab...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/691053003/diff/20001/components/domain_reliab...
components/domain_reliability/monitor.cc:152: last_network_change_time_ =
time_->NowTicks();
On 2014/11/03 19:25:34, David Benjamin wrote:
> Perhaps a test in monitor_unittest.cc to provide coverage over this codepath?
> You could call NotifyObserversOfNetworkChangeForTests to simulate a few of
> these.

Hmm. I don't have an easy way to test the effects of that, short of duplicating
the context unittests at the monitor layer. (I could add
GetLastNetworkChangeTimeForTesting, but that's getting weird.)

[davidben, 2014-11-04 19:12:35]

lgtm

https://codereview.chromium.org/691053003/diff/20001/components/domain_reliab...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/691053003/diff/20001/components/domain_reliab...
components/domain_reliability/monitor.cc:152: last_network_change_time_ =
time_->NowTicks();
On 2014/11/04 17:56:52, ttuttle wrote:
> On 2014/11/03 19:25:34, David Benjamin wrote:
> > Perhaps a test in monitor_unittest.cc to provide coverage over this
codepath?
> > You could call NotifyObserversOfNetworkChangeForTests to simulate a few of
> > these.
> 
> Hmm. I don't have an easy way to test the effects of that, short of
duplicating
> the context unittests at the monitor layer. (I could add
> GetLastNetworkChangeTimeForTesting, but that's getting weird.)

Mm. I dunno, it seems worth having an integration test or two to ensure that
this assembly is hooked up together right. (Contexts all alias the right
last_network_change_time_ which gets updated, etc.)

[Deprecated (see juliatuttle), 2014-11-04 19:15:57]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2014-11-04 19:17:22]

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

[commit-bot: I haz the power, 2014-11-04 20:22:40]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2014-11-04 20:24:11]

Patchset 3 (id:??) landed as
https://crrev.com/743f4ae91ce9fb5fc523edd0acc4978c7ba5db85
Cr-Commit-Position: refs/heads/master@{#302655}