Correct stats for RTCPeerConnectionStats.dataChannels[Opened/Closed].

Correct stats for RTCPeerConnectionStats.dataChannels[Opened/Closed].

DataChannel.SignalOpened and unittests added.
PeerConnection.SignalDataChannelCreated added and wired up to
RTCStatsCollector.OnDataChannelCreated on RTCStatsCollector
construction.
RTCStatsCollector.OnSignalOpened/Closed added and wired up on
OnDataChannelCreated.
rtcstatscollector_unittest.cc updated, faking that channels are opened
and closed.

I did not want to use DataChannelObserver because it is used for more
than state changes and there can only be one observer (unless code is
updated). Since DataChannel already had a SignalClosed it made sense to
add a SignalOpened.

Having OnSignalBlah in RTCStatsCollector is new in this CL but will
likely be needed to correctly handle RTPMediaStreamTracks being added
and detached independently of getStats. This CL establishes this
pattern.

(An integration test will be needed for this and all the other stats to
make sure everything is wired up correctly and test outside of a
mock/fake environment, but this is not news.)

BUG=chromium:636818, chromium:627816
Committed: https://crrev.com/82ebe02491d066697717ae386f886b752729e013
Cr-Commit-Position: refs/heads/master@{#15059}

[hbos, 2016-11-03 12:45:55]

Patchset #1 (id:1) has been deleted

[hbos, 2016-11-03 12:48:06]

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

[hbos, 2016-11-03 12:48:06]

Please take a look, hta and deadbeef

[hta-webrtc, 2016-11-03 13:49:58]

lgtm with commentary, most of which can be addressed by adding comments.

https://codereview.webrtc.org/2472113002/diff/20001/webrtc/api/rtcstatscollec...
File webrtc/api/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2472113002/diff/20001/webrtc/api/rtcstatscollec...
webrtc/api/rtcstatscollector.cc:670: ++data_channels_opened_;
Isn't this equvalent to

bool result = opened_data_channels_.insert(...
RTC_CHECK(result)

?

https://codereview.webrtc.org/2472113002/diff/20001/webrtc/api/rtcstatscollec...
webrtc/api/rtcstatscollector.cc:681: ++data_channels_closed_;
Is it possible to get into the state where a channel signals closed but not
open?

You might want to add a DCHECK to be sure - if it doesn't trigger ever, you can
remove the set of opened channels.

https://codereview.webrtc.org/2472113002/diff/20001/webrtc/api/rtcstatscollec...
File webrtc/api/rtcstatscollector.h (right):

https://codereview.webrtc.org/2472113002/diff/20001/webrtc/api/rtcstatscollec...
webrtc/api/rtcstatscollector.h:158: uint32_t data_channels_closed_;
This is actually the beginning of a new path in stats collecting, where you
allow the StatsCollector object to hold references to data between calls and
depend on them being updated correctly from the start of time to the end of
time.

You might want to flag that with a comment above; somewhere in the future, we
might consider factoring out all that kind of stuff into a (private?) class that
is separate from the statscollector itself.

But for these 2 counters, just making a note should be OK.

https://codereview.webrtc.org/2472113002/diff/20001/webrtc/api/rtcstatscollec...
webrtc/api/rtcstatscollector.h:160: std::set<DataChannel*>
opened_data_channels_;
Do we actually need this? I'd suggest not retaining pointers if we don't have a
clear use case for them - they're a temptation to sin.
If they're used as identifiers (and I think they are), make a clear comment that
that's what they are - they must NOT be followed.

It's tempting to cast them to int in order to limit the temptation.

[hbos, 2016-11-03 15:02:02]

Please take a look, deadbeef.

https://codereview.webrtc.org/2472113002/diff/20001/webrtc/api/rtcstatscollec...
File webrtc/api/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2472113002/diff/20001/webrtc/api/rtcstatscollec...
webrtc/api/rtcstatscollector.cc:670: ++data_channels_opened_;
On 2016/11/03 13:49:58, hta-webrtc wrote:
> Isn't this equvalent to
> 
> bool result = opened_data_channels_.insert(...
> RTC_CHECK(result)
> 
> ?

Done.

https://codereview.webrtc.org/2472113002/diff/20001/webrtc/api/rtcstatscollec...
webrtc/api/rtcstatscollector.cc:681: ++data_channels_closed_;
On 2016/11/03 13:49:58, hta-webrtc wrote:
> Is it possible to get into the state where a channel signals closed but not
> open?
> 
> You might want to add a DCHECK to be sure - if it doesn't trigger ever, you
can
> remove the set of opened channels.

From looking at the code I think this would happen if the channel's Close() is
called while it is in the connecting state, and I think it is perfectly legal to
do so and thus I should not DCHECK. That said, I have not spent the time to set
up a scenario where this happens.

https://codereview.webrtc.org/2472113002/diff/20001/webrtc/api/rtcstatscollec...
File webrtc/api/rtcstatscollector.h (right):

https://codereview.webrtc.org/2472113002/diff/20001/webrtc/api/rtcstatscollec...
webrtc/api/rtcstatscollector.h:158: uint32_t data_channels_closed_;
On 2016/11/03 13:49:58, hta-webrtc wrote:
> This is actually the beginning of a new path in stats collecting, where you
> allow the StatsCollector object to hold references to data between calls and
> depend on them being updated correctly from the start of time to the end of
> time.
> 
> You might want to flag that with a comment above; somewhere in the future, we
> might consider factoring out all that kind of stuff into a (private?) class
that
> is separate from the statscollector itself.
> 
> But for these 2 counters, just making a note should be OK.

Created struct InternalRecord and added a comment.

https://codereview.webrtc.org/2472113002/diff/20001/webrtc/api/rtcstatscollec...
webrtc/api/rtcstatscollector.h:160: std::set<DataChannel*>
opened_data_channels_;
On 2016/11/03 13:49:58, hta-webrtc wrote:
> Do we actually need this? I'd suggest not retaining pointers if we don't have
a
> clear use case for them - they're a temptation to sin.
> If they're used as identifiers (and I think they are), make a clear comment
that
> that's what they are - they must NOT be followed.
> 
> It's tempting to cast them to int in order to limit the temptation.
> 

Changed to std::set<uintptr_t>

[hbos, 2016-11-08 14:28:49]

Ping deadbeef

[Taylor Brandstetter, 2016-11-09 00:32:48]

Sorry for the delay in reviewing; I've been a little behind.

https://codereview.webrtc.org/2472113002/diff/20001/webrtc/api/rtcstatscollec...
File webrtc/api/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2472113002/diff/20001/webrtc/api/rtcstatscollec...
webrtc/api/rtcstatscollector.cc:681: ++data_channels_closed_;
On 2016/11/03 15:02:02, hbos wrote:
> On 2016/11/03 13:49:58, hta-webrtc wrote:
> > Is it possible to get into the state where a channel signals closed but not
> > open?
> > 
> > You might want to add a DCHECK to be sure - if it doesn't trigger ever, you
> can
> > remove the set of opened channels.
> 
> From looking at the code I think this would happen if the channel's Close() is
> called while it is in the connecting state, and I think it is perfectly legal
to
> do so and thus I should not DCHECK. That said, I have not spent the time to
set
> up a scenario where this happens.

Yes, this is definitely legal. Any kind of underlying SCTP error would cause
this.

https://codereview.webrtc.org/2472113002/diff/40001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/2472113002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:2083: &RTCStatsCollector::OnDataChannelOpened);
This is confusing to me since the slot is connected in a different file than
where it's defined, and it requires a friend relationship. I think it begins a
worrisome coupling between RTCStatsCollector and PeerConnection.

It would be better if:

- PeerConnection just kept track of the history itself and provided accessors.

- PeerConnection called a method on RTCStatsCollector called
"MonitorDataChannel" (or equivalent) which connected the signals.

- A step further: PeerConnection fires a "DataChannelCreated" signal, -which
RTCStatsCollector connects to. This in turn causes it to start monitoring the
new data channel.

I'd be happy with any of these, but I'd prefer to not have the friend
relationship and "ForTesting" methods. Since this is a pattern we'll likely need
for other things, it would be nice to start with a good precedent.

[hbos, 2016-11-11 09:52:02]

PTAL deadbeef

On 2016/11/09 00:32:48, Taylor Brandstetter wrote:
> Sorry for the delay in reviewing; I've been a little behind.

No problem, I'm sending a lot of CLs your way :)

https://codereview.webrtc.org/2472113002/diff/40001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/2472113002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:2083: &RTCStatsCollector::OnDataChannelOpened);
On 2016/11/09 00:32:48, Taylor Brandstetter wrote:
> This is confusing to me since the slot is connected in a different file than
> where it's defined, and it requires a friend relationship. I think it begins a
> worrisome coupling between RTCStatsCollector and PeerConnection.
> 
> It would be better if:
> 
> - PeerConnection just kept track of the history itself and provided accessors.
> 
> - PeerConnection called a method on RTCStatsCollector called
> "MonitorDataChannel" (or equivalent) which connected the signals.
> 
> - A step further: PeerConnection fires a "DataChannelCreated" signal, -which
> RTCStatsCollector connects to. This in turn causes it to start monitoring the
> new data channel.
> 
> I'd be happy with any of these, but I'd prefer to not have the friend
> relationship and "ForTesting" methods. Since this is a pattern we'll likely
need
> for other things, it would be nice to start with a good precedent.

Done.

I added a SignalDataChannelCreated to PeerConnection and wired it up.
I wanted to test the signal in peerconnection_unittest.cc, but I don't get how
you're supposed to test PeerConnection-specific things, the tests are handling
scoped_refptr<PeerConnectionInterface> and I don't want to introduce casting to
PeerConnection. (Why, there's already a peerconnectioninterface_unittest.cc?) I
also don't want to update the PeerConnectionInterface willy-nilly.

So right now only mocks are tested, ensuring RTCStatsCollector is working as
intended. I will soon start work on an integration test for getStats, testing
without mocks.

[hbos, 2016-11-11 09:52:15]

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

[commit-bot: I haz the power, 2016-11-11 09:52:17]

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

[hbos, 2016-11-11 09:57:19]

Description was changed from

==========
Correct stats for RTCPeerConnectionStats.dataChannels[Opened/Closed].

DataChannel.SignalOpened and unittests added.
RTCStatsCollector.OnSignalOpened/Closed added and wired up.
rtcstatscollector_unittest.cc updated, faking that channels are opened
and closed.

I did not want to use DataChannelObserver because it is used for more
than state changes and there can only be one observer (unless code is
updated). Since DataChannel already had a SignalClosed it made sense to
add a SignalOpened.

Having OnSignalBlah in RTCStatsCollector is new in this CL but will
likely be needed to correctly handle RTPMediaStreamTracks being added
and detached independently of getStats. This CL establishes this
pattern.

(An integration test will be needed for this and all the other stats to
make sure everything is wired up correctly and test outside of a
mock/fake environment, but this is not news.)

BUG=chromium:636818, chromium:627816
==========

to

==========
Correct stats for RTCPeerConnectionStats.dataChannels[Opened/Closed].

DataChannel.SignalOpened and unittests added.
PeerConnection.SignalDataChannelCreated added and wired up to
RTCStatsCollector.OnDataChannelCreated on RTCStatsCollector
construction.
RTCStatsCollector.OnSignalOpened/Closed added and wired up on
OnDataChannelCreated.
rtcstatscollector_unittest.cc updated, faking that channels are opened
and closed.

I did not want to use DataChannelObserver because it is used for more
than state changes and there can only be one observer (unless code is
updated). Since DataChannel already had a SignalClosed it made sense to
add a SignalOpened.

Having OnSignalBlah in RTCStatsCollector is new in this CL but will
likely be needed to correctly handle RTPMediaStreamTracks being added
and detached independently of getStats. This CL establishes this
pattern.

(An integration test will be needed for this and all the other stats to
make sure everything is wired up correctly and test outside of a
mock/fake environment, but this is not news.)

BUG=chromium:636818, chromium:627816
==========

[commit-bot: I haz the power, 2016-11-11 10:16:58]

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

[commit-bot: I haz the power, 2016-11-11 10:16:59]

Dry run: This issue passed the CQ dry run.

[Taylor Brandstetter, 2016-11-11 20:54:53]

lgtm

https://codereview.webrtc.org/2472113002/diff/40001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/2472113002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:2083: &RTCStatsCollector::OnDataChannelOpened);
On 2016/11/11 09:52:02, hbos wrote:
> On 2016/11/09 00:32:48, Taylor Brandstetter wrote:
> > This is confusing to me since the slot is connected in a different file than
> > where it's defined, and it requires a friend relationship. I think it begins
a
> > worrisome coupling between RTCStatsCollector and PeerConnection.
> > 
> > It would be better if:
> > 
> > - PeerConnection just kept track of the history itself and provided
accessors.
> > 
> > - PeerConnection called a method on RTCStatsCollector called
> > "MonitorDataChannel" (or equivalent) which connected the signals.
> > 
> > - A step further: PeerConnection fires a "DataChannelCreated" signal, -which
> > RTCStatsCollector connects to. This in turn causes it to start monitoring
the
> > new data channel.
> > 
> > I'd be happy with any of these, but I'd prefer to not have the friend
> > relationship and "ForTesting" methods. Since this is a pattern we'll likely
> need
> > for other things, it would be nice to start with a good precedent.
> 
> Done.
> 
> I added a SignalDataChannelCreated to PeerConnection and wired it up.
> I wanted to test the signal in peerconnection_unittest.cc, but I don't get how
> you're supposed to test PeerConnection-specific things, the tests are handling
> scoped_refptr<PeerConnectionInterface> and I don't want to introduce casting
to
> PeerConnection. (Why, there's already a peerconnectioninterface_unittest.cc?)
I
> also don't want to update the PeerConnectionInterface willy-nilly.
> 
> So right now only mocks are tested, ensuring RTCStatsCollector is working as
> intended. I will soon start work on an integration test for getStats, testing
> without mocks.

RTCStatsCollector already uses sctp_data_channels(), which is
PeerConnection-specific, so I don't think it's a new problem. We do need to
improve our PeerConnection test infrastructure so we can add these sorts of
tests more easily, but I think that's outside the scope of this CL.

[hbos, 2016-11-14 08:57:07]

The CQ bit was checked by hbos@webrtc.org

[hbos, 2016-11-14 08:57:08]

The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2472113002/#ps60001 (title:
"Addressed deadbeef's comments")

[commit-bot: I haz the power, 2016-11-14 08:57:17]

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, 2016-11-14 09:12:45]

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

[commit-bot: I haz the power, 2016-11-14 09:12:46]

Try jobs failed on following builders:
  win_clang_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/8226)

[hbos, 2016-11-14 09:18:34]

The CQ bit was checked by hbos@webrtc.org

[commit-bot: I haz the power, 2016-11-14 09:18:39]

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, 2016-11-14 09:41:12]

Description was changed from

==========
Correct stats for RTCPeerConnectionStats.dataChannels[Opened/Closed].

DataChannel.SignalOpened and unittests added.
PeerConnection.SignalDataChannelCreated added and wired up to
RTCStatsCollector.OnDataChannelCreated on RTCStatsCollector
construction.
RTCStatsCollector.OnSignalOpened/Closed added and wired up on
OnDataChannelCreated.
rtcstatscollector_unittest.cc updated, faking that channels are opened
and closed.

I did not want to use DataChannelObserver because it is used for more
than state changes and there can only be one observer (unless code is
updated). Since DataChannel already had a SignalClosed it made sense to
add a SignalOpened.

Having OnSignalBlah in RTCStatsCollector is new in this CL but will
likely be needed to correctly handle RTPMediaStreamTracks being added
and detached independently of getStats. This CL establishes this
pattern.

(An integration test will be needed for this and all the other stats to
make sure everything is wired up correctly and test outside of a
mock/fake environment, but this is not news.)

BUG=chromium:636818, chromium:627816
==========

to

==========
Correct stats for RTCPeerConnectionStats.dataChannels[Opened/Closed].

DataChannel.SignalOpened and unittests added.
PeerConnection.SignalDataChannelCreated added and wired up to
RTCStatsCollector.OnDataChannelCreated on RTCStatsCollector
construction.
RTCStatsCollector.OnSignalOpened/Closed added and wired up on
OnDataChannelCreated.
rtcstatscollector_unittest.cc updated, faking that channels are opened
and closed.

I did not want to use DataChannelObserver because it is used for more
than state changes and there can only be one observer (unless code is
updated). Since DataChannel already had a SignalClosed it made sense to
add a SignalOpened.

Having OnSignalBlah in RTCStatsCollector is new in this CL but will
likely be needed to correctly handle RTPMediaStreamTracks being added
and detached independently of getStats. This CL establishes this
pattern.

(An integration test will be needed for this and all the other stats to
make sure everything is wired up correctly and test outside of a
mock/fake environment, but this is not news.)

BUG=chromium:636818, chromium:627816
==========

[commit-bot: I haz the power, 2016-11-14 09:41:14]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-11-14 09:42:03]

Description was changed from

==========
Correct stats for RTCPeerConnectionStats.dataChannels[Opened/Closed].

DataChannel.SignalOpened and unittests added.
PeerConnection.SignalDataChannelCreated added and wired up to
RTCStatsCollector.OnDataChannelCreated on RTCStatsCollector
construction.
RTCStatsCollector.OnSignalOpened/Closed added and wired up on
OnDataChannelCreated.
rtcstatscollector_unittest.cc updated, faking that channels are opened
and closed.

I did not want to use DataChannelObserver because it is used for more
than state changes and there can only be one observer (unless code is
updated). Since DataChannel already had a SignalClosed it made sense to
add a SignalOpened.

Having OnSignalBlah in RTCStatsCollector is new in this CL but will
likely be needed to correctly handle RTPMediaStreamTracks being added
and detached independently of getStats. This CL establishes this
pattern.

(An integration test will be needed for this and all the other stats to
make sure everything is wired up correctly and test outside of a
mock/fake environment, but this is not news.)

BUG=chromium:636818, chromium:627816
==========

to

==========
Correct stats for RTCPeerConnectionStats.dataChannels[Opened/Closed].

DataChannel.SignalOpened and unittests added.
PeerConnection.SignalDataChannelCreated added and wired up to
RTCStatsCollector.OnDataChannelCreated on RTCStatsCollector
construction.
RTCStatsCollector.OnSignalOpened/Closed added and wired up on
OnDataChannelCreated.
rtcstatscollector_unittest.cc updated, faking that channels are opened
and closed.

I did not want to use DataChannelObserver because it is used for more
than state changes and there can only be one observer (unless code is
updated). Since DataChannel already had a SignalClosed it made sense to
add a SignalOpened.

Having OnSignalBlah in RTCStatsCollector is new in this CL but will
likely be needed to correctly handle RTPMediaStreamTracks being added
and detached independently of getStats. This CL establishes this
pattern.

(An integration test will be needed for this and all the other stats to
make sure everything is wired up correctly and test outside of a
mock/fake environment, but this is not news.)

BUG=chromium:636818, chromium:627816

Committed: https://crrev.com/82ebe02491d066697717ae386f886b752729e013
Cr-Commit-Position: refs/heads/master@{#15059}
==========

[commit-bot: I haz the power, 2016-11-14 09:42:04]

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