Moving MediaStreamSignaling logic into PeerConnection.

Moving MediaStreamSignaling logic into PeerConnection.

This needs to happen because in the future, m-lines will be offered
based on the set of RtpSenders/RtpReceivers, rather than the set of
tracks that MediaStreamSignaling knows about.

Besides that, MediaStreamSignaling was a "glue class" without
a clearly defined role, so it going away is good for other
reasons as well.

Committed: https://crrev.com/97c392935411398b506861601c82e31d95c591f0
Cr-Commit-Position: refs/heads/master@{#10268}

[Taylor Brandstetter, 2015-10-07 00:26:18]

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

[Taylor Brandstetter, 2015-10-07 00:26:19]

This may seem large, but 90% of it is just code being moved around. There
shouldn't be any behavioral changes in this CL.

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/datachann...
File talk/app/webrtc/datachannel.cc (right):

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/datachann...
talk/app/webrtc/datachannel.cc:401: void
DataChannel::OnStreamClosedRemotely(uint32 sid) {
Was previously MediaStreamSignaling::OnRemoteSctpDataChannelClosed.
The channel will be removed from the PeerConnection and sid released when it
calls SignalClosed so the behavior is the same.

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.cc (left):

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:345: }
The stuff previously done in mediastream_signaling->TearDown() will now happen
when WebRtcSession is destroyed and signals that its channels are closed.

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.h (right):

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.h:64: 
These methods are pulled out of GetOptionsForOffer/GetOptionsForAnswer, because
they're deterministic and this makes them easy to test.

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.h:377: rtc::scoped_ptr<WebRtcSession> session_;
Moved session_ down here so it will be destroyed first, since its destruction
triggers some signals we now listen to.

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/statscoll...
File talk/app/webrtc/statscollector.h (right):

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/statscoll...
talk/app/webrtc/statscollector.h:64: explicit StatsCollector(PeerConnection* pc,
WebRtcSession* session);
I don't really like passing the StatsCollector a PeerConnection pointer, but we
already pass it a WebRtcSession pointer... And later WebRtcSession will become
PeerConnection. Maybe in the future we should define a new interface that
StatsCollector depends on, distinct from PeerConnection and
PeerConnectionInterface?

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/statscoll...
File talk/app/webrtc/statscollector_unittest.cc (right):

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/statscoll...
talk/app/webrtc/statscollector_unittest.cc:510:
.WillRepeatedly(ReturnRef(data_channels_));
WillRepeatedly(ReturnNull()) appears in so many places (and same would be true
for WillRepeatedly(ReturnRef(data_channels_))) that it seemed sensible to just
do it here.

[pthatcher1, 2015-10-07 02:50:52]

I need to look at peerconnection.cc and .h (the big ones :), but I reviewed the
rest.

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/datachann...
File talk/app/webrtc/datachannel.cc (right):

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/datachann...
talk/app/webrtc/datachannel.cc:47: bool
SctpSidAllocator::AllocateSctpSid(rtc::SSLRole role, int* sid) {
Might as well remove "Sctp" from the method name and just call it AllocateSid,
ReserveSid, ReleaseSid, and IsSidAvailable.

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.h (right):

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.h:320: // called when the cricket::DataChannel
received a message indicating that a
called = Called

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.h:372: rtc::scoped_ptr<RemoteMediaStreamFactory>
remote_stream_factory_;
Can you comment the other things a little.  Like, where do local_streams_ and
remote_streams_ come from?  And what is the key of rtp_data_channels_?

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.h:377: rtc::scoped_ptr<WebRtcSession> session_;
On 2015/10/07 00:26:19, Taylor Brandstetter wrote:
> Moved session_ down here so it will be destroyed first, since its destruction
> triggers some signals we now listen to.

Can you put a comment in the code saying "This is last because ..."

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/statscoll...
File talk/app/webrtc/statscollector.h (right):

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/statscoll...
talk/app/webrtc/statscollector.h:64: explicit StatsCollector(PeerConnection* pc,
WebRtcSession* session);
On 2015/10/07 00:26:19, Taylor Brandstetter wrote:
> I don't really like passing the StatsCollector a PeerConnection pointer, but
we
> already pass it a WebRtcSession pointer... And later WebRtcSession will become
> PeerConnection. Maybe in the future we should define a new interface that
> StatsCollector depends on, distinct from PeerConnection and
> PeerConnectionInterface?

It makes sense to pass a PeerConnection in.  It's passing both a pc and a
session that's lame.  

Could we just make PeerConnection implement all of the methods that
StatsCollector needs from WebRtcSession by calling down to WebRtcSession inside
of it?  Then this test won't need to change again when WebRtcSession and
PeerConnection are merged.

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (left):

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:1731:
mediastream_signaling_->OnVideoChannelClose();
These used to trigger MediaStreamSignaling::RejectRemoteTracks which would end
tracks.  Do we not need that anymore?

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:1374: webrtc_data_channel,
&DataChannel::OnStreamClosedRemotely);
Can you make these all the same style?

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:1794: SignalDataChannelCreated();
Why don't we put the data channel in the signal?

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:1839: SignalDataChannelCreationRequested(label,
config);
I'd call this SignalDataChannelOpenMessage

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.h:276: sigslot::signal0<>
SignalDataChannelDestroyed;
Can you comment when these will be fired?

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.h:278: SignalDataChannelCreationRequested;
Can you comment what the parameters of this?

https://codereview.webrtc.org/1393563002/diff/80001/talk/libjingle_tests.gyp
File talk/libjingle_tests.gyp (left):

https://codereview.webrtc.org/1393563002/diff/80001/talk/libjingle_tests.gyp#...
talk/libjingle_tests.gyp:203: 'app/webrtc/mediastreamsignaling_unittest.cc',
Do we need to worry about the GN file also?

[pthatcher1, 2015-10-08 05:23:54]

OK, I've read through everything now.

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1542: void
PeerConnection::RejectRemoteTracks(cricket::MediaType media_type) {
Should this be called EndRemoteTracks?  All it does is set the state to ended.

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1857: kv.second->OnDataEngineClose();
Can we rename these so they are consistent with one another? 
OnTransportChannelCreated/OnDataEngineClose is a weird combo.  Looking at the
implementations of those methods, Activate/Close might be a good name pair.

[pthatcher1, 2015-10-08 22:05:36]

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/statscoll...
File talk/app/webrtc/statscollector.h (right):

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/statscoll...
talk/app/webrtc/statscollector.h:64: explicit StatsCollector(PeerConnection* pc,
WebRtcSession* session);
On 2015/10/07 02:50:51, pthatcher1 wrote:
> On 2015/10/07 00:26:19, Taylor Brandstetter wrote:
> > I don't really like passing the StatsCollector a PeerConnection pointer, but
> we
> > already pass it a WebRtcSession pointer... And later WebRtcSession will
become
> > PeerConnection. Maybe in the future we should define a new interface that
> > StatsCollector depends on, distinct from PeerConnection and
> > PeerConnectionInterface?
> 
> It makes sense to pass a PeerConnection in.  It's passing both a pc and a
> session that's lame.  
> 
> Could we just make PeerConnection implement all of the methods that
> StatsCollector needs from WebRtcSession by calling down to WebRtcSession
inside
> of it?  Then this test won't need to change again when WebRtcSession and
> PeerConnection are merged.

Or maybe pc_->session()?

[Taylor Brandstetter, 2015-10-09 19:54:09]

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/datachann...
File talk/app/webrtc/datachannel.cc (right):

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/datachann...
talk/app/webrtc/datachannel.cc:47: bool
SctpSidAllocator::AllocateSctpSid(rtc::SSLRole role, int* sid) {
On 2015/10/07 02:50:51, pthatcher1 wrote:
> Might as well remove "Sctp" from the method name and just call it AllocateSid,
> ReserveSid, ReleaseSid, and IsSidAvailable.

Done.

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1542: void
PeerConnection::RejectRemoteTracks(cricket::MediaType media_type) {
On 2015/10/08 05:23:54, pthatcher1 wrote:
> Should this be called EndRemoteTracks?  All it does is set the state to ended.

I agree; done. Funny enough, I think we already made a similar change in the
"video disable/reenable" CL that was rolled back.

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1857: kv.second->OnDataEngineClose();
On 2015/10/08 05:23:54, pthatcher1 wrote:
> Can we rename these so they are consistent with one another? 
> OnTransportChannelCreated/OnDataEngineClose is a weird combo.  Looking at the
> implementations of those methods, Activate/Close might be a good name pair.

Well, since the cricket::DataChannel actually IS being created (constructed) and
destroyed (destructed), I think Create/Destroy makes as much sense. And since
OnDataEngineClose is the only odd man out, I changed it to
OnTransportChannelDestroyed.

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.h (right):

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.h:320: // called when the cricket::DataChannel
received a message indicating that a
On 2015/10/07 02:50:51, pthatcher1 wrote:
> called = Called

Done.

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.h:372: rtc::scoped_ptr<RemoteMediaStreamFactory>
remote_stream_factory_;
On 2015/10/07 02:50:51, pthatcher1 wrote:
> Can you comment the other things a little.  Like, where do local_streams_ and
> remote_streams_ come from?  And what is the key of rtp_data_channels_?

Done.

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.h:377: rtc::scoped_ptr<WebRtcSession> session_;
On 2015/10/07 02:50:51, pthatcher1 wrote:
> On 2015/10/07 00:26:19, Taylor Brandstetter wrote:
> > Moved session_ down here so it will be destroyed first, since its
destruction
> > triggers some signals we now listen to.
> 
> Can you put a comment in the code saying "This is last because ..."

Done.

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/statscoll...
File talk/app/webrtc/statscollector.h (right):

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/statscoll...
talk/app/webrtc/statscollector.h:64: explicit StatsCollector(PeerConnection* pc,
WebRtcSession* session);
On 2015/10/08 22:05:36, pthatcher1 wrote:
> On 2015/10/07 02:50:51, pthatcher1 wrote:
> > On 2015/10/07 00:26:19, Taylor Brandstetter wrote:
> > > I don't really like passing the StatsCollector a PeerConnection pointer,
but
> > we
> > > already pass it a WebRtcSession pointer... And later WebRtcSession will
> become
> > > PeerConnection. Maybe in the future we should define a new interface that
> > > StatsCollector depends on, distinct from PeerConnection and
> > > PeerConnectionInterface?
> > 
> > It makes sense to pass a PeerConnection in.  It's passing both a pc and a
> > session that's lame.  
> > 
> > Could we just make PeerConnection implement all of the methods that
> > StatsCollector needs from WebRtcSession by calling down to WebRtcSession
> inside
> > of it?  Then this test won't need to change again when WebRtcSession and
> > PeerConnection are merged.
> 
> Or maybe pc_->session()?

Done.

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (left):

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:1731:
mediastream_signaling_->OnVideoChannelClose();
On 2015/10/07 02:50:51, pthatcher1 wrote:
> These used to trigger MediaStreamSignaling::RejectRemoteTracks which would end
> tracks.  Do we not need that anymore?

Now we listen to Signal[Video|Voice|Data]ChannelDestroyed, which will call
EndRemoteTracks.

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:1374: webrtc_data_channel,
&DataChannel::OnStreamClosedRemotely);
On 2015/10/07 02:50:51, pthatcher1 wrote:
> Can you make these all the same style?

Done (but the next git cl format will change it back unfortunately).

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:1794: SignalDataChannelCreated();
On 2015/10/07 02:50:51, pthatcher1 wrote:
> Why don't we put the data channel in the signal?

Because the higher level objects can (with Plan B SDP) assume that there's never
more than one VideoChannel, VoiceChannel or DataChannel.

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:1839: SignalDataChannelCreationRequested(label,
config);
On 2015/10/07 02:50:51, pthatcher1 wrote:
> I'd call this SignalDataChannelOpenMessage

Done.

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.h:276: sigslot::signal0<>
SignalDataChannelDestroyed;
On 2015/10/07 02:50:51, pthatcher1 wrote:
> Can you comment when these will be fired?

Done. I also added "SignalVoiceChannelCreated" and "SignalVideoChannelCreated"
for consistency, and since they'll be needed eventually.

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.h:278: SignalDataChannelCreationRequested;
On 2015/10/07 02:50:51, pthatcher1 wrote:
> Can you comment what the parameters of this?

Done.

https://codereview.webrtc.org/1393563002/diff/80001/talk/libjingle_tests.gyp
File talk/libjingle_tests.gyp (left):

https://codereview.webrtc.org/1393563002/diff/80001/talk/libjingle_tests.gyp#...
talk/libjingle_tests.gyp:203: 'app/webrtc/mediastreamsignaling_unittest.cc',
On 2015/10/07 02:50:52, pthatcher1 wrote:
> Do we need to worry about the GN file also?

If you mean the Chrome GN file, no; they don't build our unit tests.

[pthatcher1, 2015-10-09 21:12:26]

lgtm

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1857: kv.second->OnDataEngineClose();
On 2015/10/09 19:54:09, Taylor Brandstetter wrote:
> On 2015/10/08 05:23:54, pthatcher1 wrote:
> > Can we rename these so they are consistent with one another? 
> > OnTransportChannelCreated/OnDataEngineClose is a weird combo.  Looking at
the
> > implementations of those methods, Activate/Close might be a good name pair.
> 
> Well, since the cricket::DataChannel actually IS being created (constructed)
and
> destroyed (destructed), I think Create/Destroy makes as much sense. And since
> OnDataEngineClose is the only odd man out, I changed it to
> OnTransportChannelDestroyed.

Sounds good.

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1393563002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:1374: webrtc_data_channel,
&DataChannel::OnStreamClosedRemotely);
On 2015/10/09 19:54:09, Taylor Brandstetter wrote:
> On 2015/10/07 02:50:51, pthatcher1 wrote:
> > Can you make these all the same style?
> 
> Done (but the next git cl format will change it back unfortunately).

Oh, well go with git cl format, then, even if it's weird.

https://codereview.webrtc.org/1393563002/diff/100001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnection.h (right):

https://codereview.webrtc.org/1393563002/diff/100001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.h:391: rtc::scoped_ptr<StatsCollector> stats_;
Now session_ isn't at the bottom :).  Can you comment on why this is at the
bottom even below session_.

[Taylor Brandstetter, 2015-10-13 17:20:27]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-10-13 17:20:28]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1393563002/#ps160001 (title:
"Temporarily disabling one of the new tests. Will fix in future CL.")

[commit-bot: I haz the power, 2015-10-13 17:20:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1393563002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1393563002/160001

[commit-bot: I haz the power, 2015-10-13 17:23:08]

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

[commit-bot: I haz the power, 2015-10-13 17:23:09]

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

[Taylor Brandstetter, 2015-10-13 20:04:59]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-10-13 20:04:59]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1393563002/#ps180001 (title:
"Fixing copyright header")

[commit-bot: I haz the power, 2015-10-13 20:05:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1393563002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1393563002/180001

[commit-bot: I haz the power, 2015-10-13 20:23:46]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-10-13 20:23:55]

Patchset 10 (id:??) landed as
https://crrev.com/97c392935411398b506861601c82e31d95c591f0
Cr-Commit-Position: refs/heads/master@{#10268}

[Taylor Brandstetter, 2015-10-13 23:41:50]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.webrtc.org/1403633005/ by deadbeef@webrtc.org.

The reason for reverting is: Broke browser_tests on Mac. Still need to
investigate the cause..