RTCIceCandidatePairStats.nominated collected.

RTCIceCandidatePairStats.nominated collected.

Connection::nominated() is updated to mean
(remote_nomination_ || acked_nomination_), which means both a
controlling and controlled agent can be said to be "nominated".
Previously this was (remote_nomination_ > 0) which only applies to the
controlling agent.

PortTest.TestNomination added to test nomination values and nomination
stat.

This value is surfaced through cricket::ConnectionInfo::nominated.
RTCStatsCollector uses this value in its collection of
RTCIceCandidatePairStats.

RTCStatsCollectorTest.CollectRTCIceCandidatePairStats updated to test
that ConnectionInfo::nominated is surfaced using mocks.
rtcstats_integrationtest.cc updated to expect nomination set without
using mocks.

Spec: https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatepairstats-nominated

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

[hbos, 2017-02-23 12:59:45]

Patchset #1 (id:1) has been deleted

[hbos, 2017-02-23 13:02:20]

Patchset #1 (id:20001) has been deleted

[hbos, 2017-02-23 13:02:24]

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

[commit-bot: I haz the power, 2017-02-23 13:02:32]

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-23 13:12:59]

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

[commit-bot: I haz the power, 2017-02-23 13:13:01]

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-23 13:17:28]

Description was changed from

==========
RTCIceCandidatePairStats.nominated collected.

In order to surface this value to the stats, nominated is added to
cricket::ConnectionInfo which is equivalent to cricket::Connection's
(remote_nomination_ || acked_nomination_).

RTCStatsCollector surfaces this value to the RTCIceCandidatePairStats
object.

PortTest.TestNomination added to test nomination values and nomination
stat.

RTCStatsCollectorTest.CollectRTCIceCandidatePairStats updated to test
that ConnectionInfo::nominated is surfaced.

rtcstats_integrationtest.cc updated to expect nomination set.

Spec: https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatepairstats-nominated

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

to

==========
----------------------------------------------------------------------
RTCIceCandidatePairStats.nominated collected.

Connection::nominated() is updated to mean
(remote_nomination_ || acked_nomination_), which means both a
controlling and controlled agent can be said to be "nominated".
Previously this was (remote_nomination_ > 0) which only applies to the
controlling agent.

PortTest.TestNomination added to test nomination values and nomination
stat.

This value is surfaced through cricket::ConnectionInfo::nominated.
RTCStatsCollector uses this value in its collection of
RTCIceCandidatePairStats.

RTCStatsCollectorTest.CollectRTCIceCandidatePairStats updated to test
that ConnectionInfo::nominated is surfaced using mocks.
rtcstats_integrationtest.cc updated to expect nomination set without
using mocks.

Spec: https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatepairstats-nominated

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

[hbos, 2017-02-23 13:17:37]

Description was changed from

==========
----------------------------------------------------------------------
RTCIceCandidatePairStats.nominated collected.

Connection::nominated() is updated to mean
(remote_nomination_ || acked_nomination_), which means both a
controlling and controlled agent can be said to be "nominated".
Previously this was (remote_nomination_ > 0) which only applies to the
controlling agent.

PortTest.TestNomination added to test nomination values and nomination
stat.

This value is surfaced through cricket::ConnectionInfo::nominated.
RTCStatsCollector uses this value in its collection of
RTCIceCandidatePairStats.

RTCStatsCollectorTest.CollectRTCIceCandidatePairStats updated to test
that ConnectionInfo::nominated is surfaced using mocks.
rtcstats_integrationtest.cc updated to expect nomination set without
using mocks.

Spec: https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatepairstats-nominated

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

to

==========
RTCIceCandidatePairStats.nominated collected.

Connection::nominated() is updated to mean
(remote_nomination_ || acked_nomination_), which means both a
controlling and controlled agent can be said to be "nominated".
Previously this was (remote_nomination_ > 0) which only applies to the
controlling agent.

PortTest.TestNomination added to test nomination values and nomination
stat.

This value is surfaced through cricket::ConnectionInfo::nominated.
RTCStatsCollector uses this value in its collection of
RTCIceCandidatePairStats.

RTCStatsCollectorTest.CollectRTCIceCandidatePairStats updated to test
that ConnectionInfo::nominated is surfaced using mocks.
rtcstats_integrationtest.cc updated to expect nomination set without
using mocks.

Spec: https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatepairstats-nominated

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

[hbos, 2017-02-23 13:17:47]

Patchset #1 (id:40001) has been deleted

[hbos, 2017-02-23 13:38:56]

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

[hbos, 2017-02-23 13:38:57]

Please take a look, honghaiz and deadbeef.

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

https://codereview.webrtc.org/2709293004/diff/60001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:534: bool nominated() const { return acked_nomination_ ||
remote_nomination_; }
According to https://tools.ietf.org/html/rfc5245#section-8 one or several pairs
may me nominated depending on if Regular or Aggressive Nomination is used.
Lacking background I'm having trouble understanding the RFC, or if we want a
controlling agent to be "nominated" if |acked_nomination_ > 0| or if
|nomination_ > 0| (before acked).

In my discussion with deadbeef we concluded to use |acked_nomination_|. I think
it makes sense to unify the stats' idea of "nominated" with
Connection::nominated(), which is currently only used for controlled agents, so
changing its meaning for controlling agents should be safe.

[commit-bot: I haz the power, 2017-02-23 13:51:05]

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

[commit-bot: I haz the power, 2017-02-23 13:51:06]

Dry run: This issue passed the CQ dry run.

[Taylor Brandstetter, 2017-02-24 05:38:31]

lgtm with minor comments

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

https://codereview.webrtc.org/2709293004/diff/60001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:534: bool nominated() const { return acked_nomination_ ||
remote_nomination_; }
On 2017/02/23 13:38:56, hbos wrote:
> According to https://tools.ietf.org/html/rfc5245#section-8 one or several
pairs
> may me nominated depending on if Regular or Aggressive Nomination is used.
> Lacking background I'm having trouble understanding the RFC, or if we want a
> controlling agent to be "nominated" if |acked_nomination_ > 0| or if
> |nomination_ > 0| (before acked).
> 
> In my discussion with deadbeef we concluded to use |acked_nomination_|. I
think
> it makes sense to unify the stats' idea of "nominated" with
> Connection::nominated(), which is currently only used for controlled agents,
so
> changing its meaning for controlling agents should be safe.

webrtc-stats refers to the "nominated" flag from ICE, and ICE definitely says
the nominated flag is only updated on a binding response. So this seems correct.

Could you add a comment above "nominated()" in the code, summarizing this?

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

https://codereview.webrtc.org/2709293004/diff/60001/webrtc/p2p/base/port_unit...
webrtc/p2p/base/port_unittest.cc:1817: ASSERT_EQ(rconn->nominated(),
rconn->stats().nominated);
These should probably be EXPECTs. Also in a few places below, wherever there's
an assertion that's central to the test.

[hbos, 2017-02-24 11:08:51]

Patchset #2 (id:80001) has been deleted

[hbos, 2017-02-24 11:08:59]

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

[commit-bot: I haz the power, 2017-02-24 11:09:09]

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 11:09:14]

Please take a look, honghaiz

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

https://codereview.webrtc.org/2709293004/diff/60001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:534: bool nominated() const { return acked_nomination_ ||
remote_nomination_; }
On 2017/02/24 05:38:31, Taylor Brandstetter wrote:
> On 2017/02/23 13:38:56, hbos wrote:
> > According to https://tools.ietf.org/html/rfc5245#section-8 one or several
> pairs
> > may me nominated depending on if Regular or Aggressive Nomination is used.
> > Lacking background I'm having trouble understanding the RFC, or if we want a
> > controlling agent to be "nominated" if |acked_nomination_ > 0| or if
> > |nomination_ > 0| (before acked).
> > 
> > In my discussion with deadbeef we concluded to use |acked_nomination_|. I
> think
> > it makes sense to unify the stats' idea of "nominated" with
> > Connection::nominated(), which is currently only used for controlled agents,
> so
> > changing its meaning for controlling agents should be safe.
> 
> webrtc-stats refers to the "nominated" flag from ICE, and ICE definitely says
> the nominated flag is only updated on a binding response. So this seems
correct.
> 
> Could you add a comment above "nominated()" in the code, summarizing this?

Done.

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

https://codereview.webrtc.org/2709293004/diff/60001/webrtc/p2p/base/port_unit...
webrtc/p2p/base/port_unittest.cc:1817: ASSERT_EQ(rconn->nominated(),
rconn->stats().nominated);
On 2017/02/24 05:38:31, Taylor Brandstetter wrote:
> These should probably be EXPECTs. Also in a few places below, wherever there's
> an assertion that's central to the test.

Done.

[commit-bot: I haz the power, 2017-02-24 11:30:39]

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

[commit-bot: I haz the power, 2017-02-24 11:30:40]

Dry run: Try jobs failed on following builders:
  linux_tsan2 on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/20172)

[Taylor Brandstetter, 2017-02-24 11:45:31]

Honghai has changed teams, so in general he won't be reviewing this code any
more (correct me if I'm wrong Honghai).

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

https://codereview.webrtc.org/2709293004/diff/100001/webrtc/p2p/base/port.h#n...
webrtc/p2p/base/port.h:540: // when receiving an acknowledgment ping in response
to this.
I'd say "when receiving a response to a nominating ping". People may interpret
"ping" to mean only a binding request, not a response.

[hbos, 2017-02-24 15:12:44]

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

[commit-bot: I haz the power, 2017-02-24 15:12:51]

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 15:16:45]

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

[hbos, 2017-02-24 15:16:46]

On 2017/02/24 11:45:31, Taylor Brandstetter wrote:
> Honghai has changed teams, so in general he won't be reviewing this code any
> more (correct me if I'm wrong Honghai).

In that case, please take a look pthatcher for webrtc/p2p/ changes.

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

https://codereview.webrtc.org/2709293004/diff/100001/webrtc/p2p/base/port.h#n...
webrtc/p2p/base/port.h:540: // when receiving an acknowledgment ping in response
to this.
On 2017/02/24 11:45:31, Taylor Brandstetter wrote:
> I'd say "when receiving a response to a nominating ping". People may interpret
> "ping" to mean only a binding request, not a response.

Done.

[commit-bot: I haz the power, 2017-02-24 15:45:53]

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

[commit-bot: I haz the power, 2017-02-24 15:45:56]

Dry run: Try jobs failed on following builders:
  linux_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/22444)

[Taylor Brandstetter, 2017-02-24 18:52:53]

On 2017/02/24 15:16:46, hbos wrote:
> On 2017/02/24 11:45:31, Taylor Brandstetter wrote:
> > Honghai has changed teams, so in general he won't be reviewing this code any
> > more (correct me if I'm wrong Honghai).
> 
> In that case, please take a look pthatcher for webrtc/p2p/ changes.

I'm a webrtc/p2p/ owner as well, so you can go ahead and submit this unless you
want an extra pair of eyes.

[hbos, 2017-02-27 09:18:02]

The CQ bit was checked by hbos@webrtc.org

[hbos, 2017-02-27 09:18:03]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2709293004/#ps120001 (title:
"Updated comment")

[commit-bot: I haz the power, 2017-02-27 09:18:09]

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-27 09:38:09]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488187082615650,
"parent_rev": "a2376e7db6b6c7b8fbe070fe7e277e476361d3ff", "commit_rev":
"92eaec610490ab5f7aa66fae1edf964df1d68345"}

[commit-bot: I haz the power, 2017-02-27 09:38:12]

Description was changed from

==========
RTCIceCandidatePairStats.nominated collected.

Connection::nominated() is updated to mean
(remote_nomination_ || acked_nomination_), which means both a
controlling and controlled agent can be said to be "nominated".
Previously this was (remote_nomination_ > 0) which only applies to the
controlling agent.

PortTest.TestNomination added to test nomination values and nomination
stat.

This value is surfaced through cricket::ConnectionInfo::nominated.
RTCStatsCollector uses this value in its collection of
RTCIceCandidatePairStats.

RTCStatsCollectorTest.CollectRTCIceCandidatePairStats updated to test
that ConnectionInfo::nominated is surfaced using mocks.
rtcstats_integrationtest.cc updated to expect nomination set without
using mocks.

Spec: https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatepairstats-nominated

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

to

==========
RTCIceCandidatePairStats.nominated collected.

Connection::nominated() is updated to mean
(remote_nomination_ || acked_nomination_), which means both a
controlling and controlled agent can be said to be "nominated".
Previously this was (remote_nomination_ > 0) which only applies to the
controlling agent.

PortTest.TestNomination added to test nomination values and nomination
stat.

This value is surfaced through cricket::ConnectionInfo::nominated.
RTCStatsCollector uses this value in its collection of
RTCIceCandidatePairStats.

RTCStatsCollectorTest.CollectRTCIceCandidatePairStats updated to test
that ConnectionInfo::nominated is surfaced using mocks.
rtcstats_integrationtest.cc updated to expect nomination set without
using mocks.

Spec: https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatepairstats-nominated

BUG=webrtc:7062, webrtc:7204

Review-Url: https://codereview.webrtc.org/2709293004
Cr-Commit-Position: refs/heads/master@{#16855}
Committed:
https://chromium.googlesource.com/external/webrtc/+/92eaec610490ab5f7aa66fae1...
==========

[commit-bot: I haz the power, 2017-02-27 09:38:13]

Committed patchset #3 (id:120001) as
https://chromium.googlesource.com/external/webrtc/+/92eaec610490ab5f7aa66fae1...