RTCIceCandidatePairStats.[total/current]RoundTripTime collected.

RTCIceCandidatePairStats.[total/current]RoundTripTime collected.

Collected in accordance with the spec:
https://w3c.github.io/webrtc-stats/#candidatepair-dict*

totalRoundTripTime is collected as the sum of rtt measurements, it was
previously not collected.
currentRoundTripTime is collected as the latest rtt measurement, it
was previously collected as a smoothed value, which was incorrect.

Connection is updated to collect these values which are surfaced
through ConnectionInfo.

BUG=webrtc:7062, webrtc:7204
Review-Url: https://codereview.webrtc.org/2719523002
Cr-Commit-Position: refs/heads/master@{#16905}
Committed: https://chromium.googlesource.com/external/webrtc/+/bf8d3e572c41f5740eda0871a31d382736d8509c

[hbos, 2017-02-24 16:39:11]

Patchset #1 (id:1) has been deleted

[hbos, 2017-02-24 16:42:11]

The CQ bit was checked by hbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 16:42:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[hbos, 2017-02-24 16:47:44]

hbos@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, pthatcher@webrtc.org

[hbos, 2017-02-24 16:47:45]

Please take a look, deadbeef and pthatcher.

[commit-bot: I haz the power, 2017-02-24 17:22:27]

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

[commit-bot: I haz the power, 2017-02-24 17:22:28]

Dry run: This issue passed the CQ dry run.

[Taylor Brandstetter, 2017-02-25 01:31:29]

https://codereview.webrtc.org/2719523002/diff/20001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2719523002/diff/20001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:704: rtc::Optional<uint64_t> total_round_trip_time_ms_;
Doesn't need to be rtc::Optional, since 0 is a sensible value for "no round
trips complete yet".

https://codereview.webrtc.org/2719523002/diff/20001/webrtc/p2p/base/port_unit...
File webrtc/p2p/base/port_unittest.cc (right):

https://codereview.webrtc.org/2719523002/diff/20001/webrtc/p2p/base/port_unit...
webrtc/p2p/base/port_unittest.cc:1810: lconn->ReceivedPingResponse(10, "id");
ReceivedPingResponse is something that *should* be private... It would be
preferable to test an actual binding request/response with delay, simulating the
delay with set_delay_mean and UpdateDelayDistribution on the
VirtualSocketServer, and simulating time with a ScopedFakeClock as used in some
other tests.

[pthatcher1, 2017-02-27 18:07:21]

https://codereview.webrtc.org/2719523002/diff/20001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2719523002/diff/20001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:704: rtc::Optional<uint64_t> total_round_trip_time_ms_;
On 2017/02/25 01:31:29, Taylor Brandstetter wrote:
> Doesn't need to be rtc::Optional, since 0 is a sensible value for "no round
> trips complete yet".

I wish spec were clear what is should be.  But I agree that as currently
written, it seems like 0 is the right answer and not "nothing".

https://codereview.webrtc.org/2719523002/diff/20001/webrtc/p2p/base/port_unit...
File webrtc/p2p/base/port_unittest.cc (right):

https://codereview.webrtc.org/2719523002/diff/20001/webrtc/p2p/base/port_unit...
webrtc/p2p/base/port_unittest.cc:1810: lconn->ReceivedPingResponse(10, "id");
On 2017/02/25 01:31:29, Taylor Brandstetter wrote:
> ReceivedPingResponse is something that *should* be private... It would be
> preferable to test an actual binding request/response with delay, simulating
the
> delay with set_delay_mean and UpdateDelayDistribution on the
> VirtualSocketServer, and simulating time with a ScopedFakeClock as used in
some
> other tests.

I agree.  TestHandleStunMessage is an example of how to do build responses, and
TestHandleStunBindingIndication an example of how to use SignalReadPacket.

[hbos, 2017-02-27 19:42:32]

Patchset #3 (id:60001) has been deleted

[hbos, 2017-02-27 19:42:39]

The CQ bit was checked by hbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-27 19:42:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[hbos, 2017-02-27 19:51:34]

PTAL deadbeef & pthatcher.

https://codereview.webrtc.org/2719523002/diff/20001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2719523002/diff/20001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:704: rtc::Optional<uint64_t> total_round_trip_time_ms_;
On 2017/02/27 18:07:21, pthatcher1 wrote:
> On 2017/02/25 01:31:29, Taylor Brandstetter wrote:
> > Doesn't need to be rtc::Optional, since 0 is a sensible value for "no round
> > trips complete yet".
> 
> I wish spec were clear what is should be.  But I agree that as currently
> written, it seems like 0 is the right answer and not "nothing".

I agree 0 makes the most sense, done.

https://codereview.webrtc.org/2719523002/diff/20001/webrtc/p2p/base/port_unit...
File webrtc/p2p/base/port_unittest.cc (right):

https://codereview.webrtc.org/2719523002/diff/20001/webrtc/p2p/base/port_unit...
webrtc/p2p/base/port_unittest.cc:1810: lconn->ReceivedPingResponse(10, "id");
On 2017/02/27 18:07:21, pthatcher1 wrote:
> On 2017/02/25 01:31:29, Taylor Brandstetter wrote:
> > ReceivedPingResponse is something that *should* be private... It would be
> > preferable to test an actual binding request/response with delay, simulating
> the
> > delay with set_delay_mean and UpdateDelayDistribution on the
> > VirtualSocketServer, and simulating time with a ScopedFakeClock as used in
> some
> > other tests.
> 
> I agree.  TestHandleStunMessage is an example of how to do build responses,
and
> TestHandleStunBindingIndication an example of how to use SignalReadPacket.

I updated the test to use Ping much like the real scenario and ScopedFakeClock
to pass time. Passing messages between lconn and rconn using TestPort's
last_stun_buf/OnReadPacket without having to touch ReceivedPingResponse.

[commit-bot: I haz the power, 2017-02-27 20:40:49]

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

[commit-bot: I haz the power, 2017-02-27 20:40:50]

Dry run: This issue passed the CQ dry run.

[Taylor Brandstetter, 2017-02-27 22:21:09]

lgtm

https://codereview.webrtc.org/2719523002/diff/80001/webrtc/p2p/base/port_unit...
File webrtc/p2p/base/port_unittest.cc (right):

https://codereview.webrtc.org/2719523002/diff/80001/webrtc/p2p/base/port_unit...
webrtc/p2p/base/port_unittest.cc:231:
rconn->OnReadPacket(lport->last_stun_buf()->data<char>(),
OnReadPacket is also something that should be private, but this is still much
better, and consistent with the other tests, so it looks good to me.

https://codereview.webrtc.org/2719523002/diff/80001/webrtc/p2p/base/port_unit...
webrtc/p2p/base/port_unittest.cc:1885:
EXPECT_EQ(lconn->stats().total_round_trip_time_ms, 0u);
nit: The expected value (0) goes before the measured value. Same with below
EXPECT_EQs

[pthatcher1, 2017-02-27 22:30:41]

lgtm

[hbos, 2017-02-28 13:59:08]

https://codereview.webrtc.org/2719523002/diff/80001/webrtc/p2p/base/port_unit...
File webrtc/p2p/base/port_unittest.cc (right):

https://codereview.webrtc.org/2719523002/diff/80001/webrtc/p2p/base/port_unit...
webrtc/p2p/base/port_unittest.cc:231:
rconn->OnReadPacket(lport->last_stun_buf()->data<char>(),
On 2017/02/27 22:21:09, Taylor Brandstetter wrote:
> OnReadPacket is also something that should be private, but this is still much
> better, and consistent with the other tests, so it looks good to me.

Acknowledged.

https://codereview.webrtc.org/2719523002/diff/80001/webrtc/p2p/base/port_unit...
webrtc/p2p/base/port_unittest.cc:1885:
EXPECT_EQ(lconn->stats().total_round_trip_time_ms, 0u);
On 2017/02/27 22:21:09, Taylor Brandstetter wrote:
> nit: The expected value (0) goes before the measured value. Same with below
> EXPECT_EQs

Done.

[hbos, 2017-02-28 13:59:15]

The CQ bit was checked by hbos@webrtc.org

[hbos, 2017-02-28 13:59:15]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2719523002/#ps100001 (title:
"EXPECT_EQ(expected, actual)")

[commit-bot: I haz the power, 2017-02-28 13:59:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-28 14:34:47]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488290355155620,
"parent_rev": "4e836828f436082179e8d9a4fbf57b7a1a0a9a18", "commit_rev":
"bf8d3e572c41f5740eda0871a31d382736d8509c"}

[commit-bot: I haz the power, 2017-02-28 14:34:51]

Description was changed from

==========
RTCIceCandidatePairStats.[total/current]RoundTripTime collected.

Collected in accordance with the spec:
https://w3c.github.io/webrtc-stats/#candidatepair-dict*

totalRoundTripTime is collected as the sum of rtt measurements, it was
previously not collected.
currentRoundTripTime is collected as the latest rtt measurement, it
was previously collected as a smoothed value, which was incorrect.

Connection is updated to collect these values which are surfaced
through ConnectionInfo.

BUG=webrtc:7062, webrtc:7204
==========

to

==========
RTCIceCandidatePairStats.[total/current]RoundTripTime collected.

Collected in accordance with the spec:
https://w3c.github.io/webrtc-stats/#candidatepair-dict*

totalRoundTripTime is collected as the sum of rtt measurements, it was
previously not collected.
currentRoundTripTime is collected as the latest rtt measurement, it
was previously collected as a smoothed value, which was incorrect.

Connection is updated to collect these values which are surfaced
through ConnectionInfo.

BUG=webrtc:7062, webrtc:7204

Review-Url: https://codereview.webrtc.org/2719523002
Cr-Commit-Position: refs/heads/master@{#16905}
Committed:
https://chromium.googlesource.com/external/webrtc/+/bf8d3e572c41f5740eda0871a...
==========

[commit-bot: I haz the power, 2017-02-28 14:34:51]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/bf8d3e572c41f5740eda0871a...