Merging BaseSession code into WebRtcSession.

Merging BaseSession code into WebRtcSession.

After the TransportController CL, BaseSession does little more than
hold a state and an error, and act as an intermediary for the
TransportController. So it doesn't make sense for it to be its own
class.

Committed: https://crrev.com/d59daf8023286d63a1b6c8af82eedb684181c1eb
Cr-Commit-Position: refs/heads/master@{#10281}

[Taylor Brandstetter, 2015-10-09 02:48:49]

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

[Taylor Brandstetter, 2015-10-09 02:48:50]

Besides fixing a bug (see below) and removing the extra copies of the local and
remote description, this CL is just moving code around.

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

https://codereview.webrtc.org/1397973002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession_unittest.cc:3519: TEST_F(WebRtcSessionTest,
SetSdpFailedOnSessionError) {
I moved this test into the end of TestSessionContentError, since now SetError is
private (as it should be).

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

https://codereview.webrtc.org/1397973002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession_unittest.cc:2789: // Verifies that if a local
offer is set, then remote candidates are received
This is a bug I found while working on the CL. In SetRemoteDescription,
CopySavedCandidates was being called in the wrong place, so it was doing
nothing.

[pthatcher1, 2015-10-09 03:28:57]

This is great.  I can't wait to see it combined with the elimination of
MediaStreamSignalling.

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

https://codereview.webrtc.org/1397973002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:1015: signaling_thread_->Post(this, MSG_STATE);
The whole point of this is to convert STATE_SENTACCEPT or STATE_RECEIVEDACCEPT
into STATE_INPROGRESS.    I'm pretty sure we could just skip to
STATE_INPROGRESS.

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

https://codereview.webrtc.org/1397973002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.h:156: STATE_DEINIT,             // session is
being destroyed
Not all of these are used any more, and almost all of them could use a name
update to reflect offer/answer instead of initiate/accept.  Can you delete the
unused ones and do a little renaming?

https://codereview.webrtc.org/1397973002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.h:190: bool initiator() const { return initiator_;
}
Maybe call it offerer.

https://codereview.webrtc.org/1397973002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.h:201: sigslot::signal2<WebRtcSession*, Error>
SignalError;
Is this still used?  I think we can just remove it.

https://codereview.webrtc.org/1397973002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.h:262: return local_desc_ ?
local_desc_->description() : nullptr;
base_ doesn't make sense as a prefix any more.  But I don't know what does. 
unwrapped?

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

https://codereview.webrtc.org/1397973002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession_unittest.cc:2812:
EXPECT_TRUE(session_->ProcessIceMessage(&ice_candidate));
I don't think that was what the copy of the candidates was for.  I don't think
we're supposed to process remote candidates until after we get the remote
description.  

I think the copying was so that if we got a new remote description without
candidates, it wouldn't blow away the candidates in the remote description.  I'm
not entirely sure why we care what candidates are in the remote description
(since we don't signal them and by then they are already passed down to the
transport), but maybe there was a good reason at some point.

[Taylor Brandstetter, 2015-10-09 23:02:07]

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

https://codereview.webrtc.org/1397973002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:1015: signaling_thread_->Post(this, MSG_STATE);
On 2015/10/09 03:28:57, pthatcher1 wrote:
> The whole point of this is to convert STATE_SENTACCEPT or STATE_RECEIVEDACCEPT
> into STATE_INPROGRESS.    I'm pretty sure we could just skip to
> STATE_INPROGRESS.

Done.

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

https://codereview.webrtc.org/1397973002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.h:156: STATE_DEINIT,             // session is
being destroyed
On 2015/10/09 03:28:57, pthatcher1 wrote:
> Not all of these are used any more, and almost all of them could use a name
> update to reflect offer/answer instead of initiate/accept.  Can you delete the
> unused ones and do a little renaming?

Done.

https://codereview.webrtc.org/1397973002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.h:190: bool initiator() const { return initiator_;
}
On 2015/10/09 03:28:57, pthatcher1 wrote:
> Maybe call it offerer.

Since there can be multiple offer/answer pairs, I'll call it "initial_offerer".

https://codereview.webrtc.org/1397973002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.h:201: sigslot::signal2<WebRtcSession*, Error>
SignalError;
On 2015/10/09 03:28:57, pthatcher1 wrote:
> Is this still used?  I think we can just remove it.

Done.

https://codereview.webrtc.org/1397973002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.h:262: return local_desc_ ?
local_desc_->description() : nullptr;
On 2015/10/09 03:28:57, pthatcher1 wrote:
> base_ doesn't make sense as a prefix any more.  But I don't know what does. 
> unwrapped?

Actually, the only reason I even kept these around was for the "if
SessionDescriptionInterface is null, don't dereference it" logic. But I'm
realizing that that's only needed in a couple places. So I'm just removing these
methods.

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

https://codereview.webrtc.org/1397973002/diff/20001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession_unittest.cc:2812:
EXPECT_TRUE(session_->ProcessIceMessage(&ice_candidate));
On 2015/10/09 03:28:57, pthatcher1 wrote:
> I don't think that was what the copy of the candidates was for.  I don't think
> we're supposed to process remote candidates until after we get the remote
> description.  
> 
> I think the copying was so that if we got a new remote description without
> candidates, it wouldn't blow away the candidates in the remote description. 
I'm
> not entirely sure why we care what candidates are in the remote description
> (since we don't signal them and by then they are already passed down to the
> transport), but maybe there was a good reason at some point.

We discussed this and got rid of saved_candidates_ altogether.

https://codereview.webrtc.org/1397973002/diff/40001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession_unittest.cc (left):

https://codereview.webrtc.org/1397973002/diff/40001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession_unittest.cc:1936: // Candidate should be allowed
to add before remote description.
It's odd that this test says "candidate can be added before remote description".
Because even though it's added, as I explained earlier, it wasn't being pushed
down to the transports.

[pthatcher1, 2015-10-10 00:07:27]

It looks good, except that I'd like to double-check with Justin and Harald that
we're OK with dropping candidates that come before the remote description.

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

https://codereview.webrtc.org/1397973002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:810: case WebRtcSession::STATE_TERMINATED:
STATE_CLOSED?

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

https://codereview.webrtc.org/1397973002/diff/40001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:778: void WebRtcSession::Terminate() {
Can we rename this Close()?

https://codereview.webrtc.org/1397973002/diff/40001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:1777: const IceCandidateInterface* candidate) {
In some of these places where you assume the remote_desc_ is non-null or the
local_desc_ is non-null, can you add an RTC_CHECK(remote_desc_) or
RTC_CHECK(local_desc_)?

I know the logic makes sense, but it's easy for logic to change, and an
RTC_CHECK is better than a null pointer deref.

https://codereview.webrtc.org/1397973002/diff/40001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession_unittest.cc (left):

https://codereview.webrtc.org/1397973002/diff/40001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession_unittest.cc:1936: // Candidate should be allowed
to add before remote description.
It's weird that we explicitly expected this behavior before.  Can you send a
quick email to Justin and Harald to double-check that we don't want to support
this?  I just want to make sure we don't break something unwittingly.

[Taylor Brandstetter, 2015-10-10 00:30:16]

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

https://codereview.webrtc.org/1397973002/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:810: case WebRtcSession::STATE_TERMINATED:
On 2015/10/10 00:07:26, pthatcher1 wrote:
> STATE_CLOSED?

Done.

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

https://codereview.webrtc.org/1397973002/diff/40001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:778: void WebRtcSession::Terminate() {
On 2015/10/10 00:07:27, pthatcher1 wrote:
> Can we rename this Close()?

Done.

https://codereview.webrtc.org/1397973002/diff/40001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:1777: const IceCandidateInterface* candidate) {
On 2015/10/10 00:07:27, pthatcher1 wrote:
> In some of these places where you assume the remote_desc_ is non-null or the
> local_desc_ is non-null, can you add an RTC_CHECK(remote_desc_) or
> RTC_CHECK(local_desc_)?
> 
> I know the logic makes sense, but it's easy for logic to change, and an
> RTC_CHECK is better than a null pointer deref.

In an earlier code review, tommi told me that RTC_CHECK'ing something that's
going to be dereferenced anyway is discouraged. So... who's right?

https://codereview.webrtc.org/1397973002/diff/40001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession_unittest.cc (left):

https://codereview.webrtc.org/1397973002/diff/40001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession_unittest.cc:1936: // Candidate should be allowed
to add before remote description.
On 2015/10/10 00:07:27, pthatcher1 wrote:
> It's weird that we explicitly expected this behavior before.  Can you send a
> quick email to Justin and Harald to double-check that we don't want to support
> this?  I just want to make sure we don't break something unwittingly.  

Email sent; CC'd you.

[pthatcher1, 2015-10-10 01:59:20]

lgtm

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

https://codereview.webrtc.org/1397973002/diff/40001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:1777: const IceCandidateInterface* candidate) {
On 2015/10/10 00:30:15, Taylor Brandstetter wrote:
> On 2015/10/10 00:07:27, pthatcher1 wrote:
> > In some of these places where you assume the remote_desc_ is non-null or the
> > local_desc_ is non-null, can you add an RTC_CHECK(remote_desc_) or
> > RTC_CHECK(local_desc_)?
> > 
> > I know the logic makes sense, but it's easy for logic to change, and an
> > RTC_CHECK is better than a null pointer deref.
> 
> In an earlier code review, tommi told me that RTC_CHECK'ing something that's
> going to be dereferenced anyway is discouraged. So... who's right?

I'll defer to Tommi on this one.

[Taylor Brandstetter, 2015-10-14 21:42:31]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-10-14 21:42:31]

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/1397973002/#ps80001 (title:
"Merging with master (MediaStreamSignaling removal affected WebRtcSession).")

[commit-bot: I haz the power, 2015-10-14 21:42:50]

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

[commit-bot: I haz the power, 2015-10-14 22:02:48]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-14 22:02:56]

Patchset 5 (id:??) landed as
https://crrev.com/d59daf8023286d63a1b6c8af82eedb684181c1eb
Cr-Commit-Position: refs/heads/master@{#10281}