Remove usage of VoEBase::AssociateSendChannel() from WVoMC.

Remove usage of VoEBase::AssociateSendChannel() from WVoMC.

- Functionality now implemented in AudioReceiveStream and Call.
- Added some missing function to MockChannelProxy.

BUG=webrtc:4690
Committed: https://crrev.com/7602aabdc0414f30756b2414e8623e41478a7503
Cr-Commit-Position: refs/heads/master@{#15072}

[the sun, 2016-11-10 08:31:30]

Description was changed from

==========
Remove usage of VoEBase::AssociateSendChannel() from WVoMC.

- Functionality now implemented in AudioReceiveStream and Call.

BUG=webrtc:4690
==========

to

==========
Remove usage of VoEBase::AssociateSendChannel() from WVoMC.

- Functionality now implemented in AudioReceiveStream and Call.
- Added some missing function to MockChannelProxy.

BUG=webrtc:4690
==========

[the sun, 2016-11-10 11:42:17]

solenberg@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[kwiberg-webrtc, 2016-11-11 13:47:40]

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/audio/audio_receiv...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.cc:147: AssociateSendStream(nullptr);
Or just channel_proxy_->DisassociateSendChannel();, so that it's easier to see
what it does.

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.cc:238: VoiceEngineImpl* voe_impl =
static_cast<VoiceEngineImpl*>(voice_engine());
Is this static_cast guaranteed to be safe? And so, would it be possible to
convince the type system of that fact so that we could avoid the cast?

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:432: uint32_t ssrc = audio_send_stream->config().rtp.ssrc;
const?

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:436: RTC_DCHECK(num_deleted == 1);
RTC_DCHECK_EQ

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/call/call_unittest.cc
File webrtc/call/call_unittest.cc (right):

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/call/call_unittest...
webrtc/call/call_unittest.cc:130: // our expectations.
Bad grammar.

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/call/call_unittest...
webrtc/call/call_unittest.cc:177: // our expectations.
Same bad grammar here.

[the sun, 2016-11-11 20:03:40]

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/audio/audio_receiv...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.cc:147: AssociateSendStream(nullptr);
On 2016/11/11 13:47:39, kwiberg-webrtc wrote:
> Or just channel_proxy_->DisassociateSendChannel();, so that it's easier to see
> what it does.

Yes, thanks.

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.cc:238: VoiceEngineImpl* voe_impl =
static_cast<VoiceEngineImpl*>(voice_engine());
On 2016/11/11 13:47:39, kwiberg-webrtc wrote:
> Is this static_cast guaranteed to be safe? And so, would it be possible to
> convince the type system of that fact so that we could avoid the cast?

I don't see how, without exposing VoiceEngineImpl in AudioState. We need to cast
because of the GetChannelProxy() method, which is on VoiceEngineImpl but not on
VoiceEngine. What we do know is that a VoiceEngine* will always point to an
instance of VoiceEngineImpl so the cast is safe (the same pattern is used in the
ctor for both AudioSendStream and AudioReceiveStream.

Do you have any persuasive arguments I could throw on the type system?

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:432: uint32_t ssrc = audio_send_stream->config().rtp.ssrc;
On 2016/11/11 13:47:40, kwiberg-webrtc wrote:
> const?

Hmm, why here, in particular, and not on every local variable that's not really
variable?

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:436: RTC_DCHECK(num_deleted == 1);
On 2016/11/11 13:47:39, kwiberg-webrtc wrote:
> RTC_DCHECK_EQ

Done.

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/call/call_unittest.cc
File webrtc/call/call_unittest.cc (right):

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/call/call_unittest...
webrtc/call/call_unittest.cc:130: // our expectations.
On 2016/11/11 13:47:40, kwiberg-webrtc wrote:
> Bad grammar.

Done.

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/call/call_unittest...
webrtc/call/call_unittest.cc:177: // our expectations.
On 2016/11/11 13:47:40, kwiberg-webrtc wrote:
> Same bad grammar here.

Done.

[kwiberg-webrtc, 2016-11-13 20:17:02]

lgtm

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/audio/audio_receiv...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.cc:238: VoiceEngineImpl* voe_impl =
static_cast<VoiceEngineImpl*>(voice_engine());
On 2016/11/11 20:03:39, the sun wrote:
> On 2016/11/11 13:47:39, kwiberg-webrtc wrote:
> > Is this static_cast guaranteed to be safe? And so, would it be possible to
> > convince the type system of that fact so that we could avoid the cast?
> 
> I don't see how, without exposing VoiceEngineImpl in AudioState. We need to
cast
> because of the GetChannelProxy() method, which is on VoiceEngineImpl but not
on
> VoiceEngine. What we do know is that a VoiceEngine* will always point to an
> instance of VoiceEngineImpl so the cast is safe (the same pattern is used in
the
> ctor for both AudioSendStream and AudioReceiveStream.
> 
> Do you have any persuasive arguments I could throw on the type system?

The only way to make this statically safe (since you can't verify that a class
has only one subclass) is to not do down casts, which means you can't do up
casts either---in other words, you'd need to pass it around as the most specific
type you need. Meaning either you'd need to expose the Impl class, or move
GetChannelProxy to VoiceEngine. (The latter sounds more appealing.)

This makes me a bit suspicious, though---if you depend on VoiceEngineImpl being
the only subclass of the abstract class VoiceEngine, why not just turn
VoiceEngine into a concrete, final class?

I realize that whatever the answers are to my questions, getting rid of the down
cast is going to be outside the scope of this CL...

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:432: uint32_t ssrc = audio_send_stream->config().rtp.ssrc;
On 2016/11/11 20:03:39, the sun wrote:
> On 2016/11/11 13:47:40, kwiberg-webrtc wrote:
> > const?
> 
> Hmm, why here, in particular, and not on every local variable that's not
really
> variable?

I try to complain in pretty much all such cases, yes. But I probably miss a few.

[the sun, 2016-11-14 18:58:06]

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/audio/audio_receiv...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.cc:238: VoiceEngineImpl* voe_impl =
static_cast<VoiceEngineImpl*>(voice_engine());
On 2016/11/13 20:17:02, kwiberg-webrtc wrote:
> On 2016/11/11 20:03:39, the sun wrote:
> > On 2016/11/11 13:47:39, kwiberg-webrtc wrote:
> > > Is this static_cast guaranteed to be safe? And so, would it be possible to
> > > convince the type system of that fact so that we could avoid the cast?
> > 
> > I don't see how, without exposing VoiceEngineImpl in AudioState. We need to
> cast
> > because of the GetChannelProxy() method, which is on VoiceEngineImpl but not
> on
> > VoiceEngine. What we do know is that a VoiceEngine* will always point to an
> > instance of VoiceEngineImpl so the cast is safe (the same pattern is used in
> the
> > ctor for both AudioSendStream and AudioReceiveStream.
> > 
> > Do you have any persuasive arguments I could throw on the type system?
> 
> The only way to make this statically safe (since you can't verify that a class
> has only one subclass) is to not do down casts, which means you can't do up
> casts either---in other words, you'd need to pass it around as the most
specific
> type you need. Meaning either you'd need to expose the Impl class, or move
> GetChannelProxy to VoiceEngine. (The latter sounds more appealing.)

Well, no, we don't want to expose that method in the public API.

> This makes me a bit suspicious, though---if you depend on VoiceEngineImpl
being
> the only subclass of the abstract class VoiceEngine, why not just turn
> VoiceEngine into a concrete, final class?

Not only that, but we also depend on VoiceEngineImpl to inherit from all the
VoENnn interface classes. See e.g. VoEBase::GetInterface(). I consider this part
of the legacy API and will not change it now.

> I realize that whatever the answers are to my questions, getting rid of the
down
> cast is going to be outside the scope of this CL...

Indeed. :)

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:432: uint32_t ssrc = audio_send_stream->config().rtp.ssrc;
On 2016/11/13 20:17:02, kwiberg-webrtc wrote:
> On 2016/11/11 20:03:39, the sun wrote:
> > On 2016/11/11 13:47:40, kwiberg-webrtc wrote:
> > > const?
> > 
> > Hmm, why here, in particular, and not on every local variable that's not
> really
> > variable?
> 
> I try to complain in pretty much all such cases, yes. But I probably miss a
few.

In the name of consistency with surrounding code I will not change it here. Does
the style guide favor const in this case? I've not found that a useful practice
- I tried it once but thought there was just lot of verbosity for no real gain
(same as when using const for primitive type arguments).

[the sun, 2016-11-14 19:07:25]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2016-11-14 19:07:25]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2461523002/#ps60001 (title:
"rebase")

[commit-bot: I haz the power, 2016-11-14 19:07:34]

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 19:30:11]

Description was changed from

==========
Remove usage of VoEBase::AssociateSendChannel() from WVoMC.

- Functionality now implemented in AudioReceiveStream and Call.
- Added some missing function to MockChannelProxy.

BUG=webrtc:4690
==========

to

==========
Remove usage of VoEBase::AssociateSendChannel() from WVoMC.

- Functionality now implemented in AudioReceiveStream and Call.
- Added some missing function to MockChannelProxy.

BUG=webrtc:4690
==========

[commit-bot: I haz the power, 2016-11-14 19:30:12]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-14 19:33:27]

Description was changed from

==========
Remove usage of VoEBase::AssociateSendChannel() from WVoMC.

- Functionality now implemented in AudioReceiveStream and Call.
- Added some missing function to MockChannelProxy.

BUG=webrtc:4690
==========

to

==========
Remove usage of VoEBase::AssociateSendChannel() from WVoMC.

- Functionality now implemented in AudioReceiveStream and Call.
- Added some missing function to MockChannelProxy.

BUG=webrtc:4690

Committed: https://crrev.com/7602aabdc0414f30756b2414e8623e41478a7503
Cr-Commit-Position: refs/heads/master@{#15072}
==========

[commit-bot: I haz the power, 2016-11-14 19:33:28]

Patchset 4 (id:??) landed as
https://crrev.com/7602aabdc0414f30756b2414e8623e41478a7503
Cr-Commit-Position: refs/heads/master@{#15072}

[kwiberg-webrtc, 2016-11-15 05:05:02]

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2461523002/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:432: uint32_t ssrc = audio_send_stream->config().rtp.ssrc;
On 2016/11/14 18:58:06, the sun wrote:
> On 2016/11/13 20:17:02, kwiberg-webrtc wrote:
> > On 2016/11/11 20:03:39, the sun wrote:
> > > On 2016/11/11 13:47:40, kwiberg-webrtc wrote:
> > > > const?
> > > 
> > > Hmm, why here, in particular, and not on every local variable that's not
> > really
> > > variable?
> > 
> > I try to complain in pretty much all such cases, yes. But I probably miss a
> few.
> 
> In the name of consistency with surrounding code I will not change it here.
Does
> the style guide favor const in this case? I've not found that a useful
practice
> - I tried it once but thought there was just lot of verbosity for no real gain
> (same as when using const for primitive type arguments).

I don't think the style guide takes a position on this issue. But I do know that
in many cases, it makes the code easier to read, because you know right away
that the variable isn't being modified by the code that follows. That's why I
recommend people to say const; if it isn't immediately obvious to me when
reviewing the code whether a variable is being modified or not, "const" would
have helped.