Separating internal and external methods of RtpSender/RtpReceiver.

Separating internal and external methods of RtpSender/RtpReceiver.

This moves the implementation specific methods to separate classes
(RtpSenderInternal/RtpReceiverInternal) so that the interface classes
represent the interface that external applications can rely on.

The reason this wasn't done earlier was that PeerConnection needed
to store proxy pointers, but also needed to access implementation-
specific methods on the underlying objects. This is now possible
by using "RtpSenderProxyWithInternal<RtpSenderInternal>", which is a proxy
that implements RtpSenderInterface but also provides direct access
to an RtpSenderInternal.

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

[Taylor Brandstetter, 2016-06-01 20:00:29]

Description was changed from

==========
Refactoring of RtpSenderInterface/RtpReceiverInterface with new proxy class
capabilities.
==========

to

==========
Refactoring of RtpSenderInterface/RtpReceiverInterface.

This moves the implementation specific methods to separate classes
(RtpSenderInternal/RtpReceiverInternal) so that the interface classes
represent the interface that external applications can rely on.

The reason this wasn't done earlier was that PeerConnection needed
to store proxy pointers, but also needed to access implementation-
specific methods on the underlying objects. This is now possible
by using "RtpSenderProxyEx<RtpSenderInternal>", which is a proxy
that implements RtpSenderInterface but also provides direct access
to an RtpSenderInternal.
==========

[Taylor Brandstetter, 2016-06-01 20:04:02]

Description was changed from

==========
Refactoring of RtpSenderInterface/RtpReceiverInterface.

This moves the implementation specific methods to separate classes
(RtpSenderInternal/RtpReceiverInternal) so that the interface classes
represent the interface that external applications can rely on.

The reason this wasn't done earlier was that PeerConnection needed
to store proxy pointers, but also needed to access implementation-
specific methods on the underlying objects. This is now possible
by using "RtpSenderProxyEx<RtpSenderInternal>", which is a proxy
that implements RtpSenderInterface but also provides direct access
to an RtpSenderInternal.
==========

to

==========
Separating internal and external methods of RtpSender/RtpReceiver.

This moves the implementation specific methods to separate classes
(RtpSenderInternal/RtpReceiverInternal) so that the interface classes
represent the interface that external applications can rely on.

The reason this wasn't done earlier was that PeerConnection needed
to store proxy pointers, but also needed to access implementation-
specific methods on the underlying objects. This is now possible
by using "RtpSenderProxyEx<RtpSenderInternal>", which is a proxy
that implements RtpSenderInterface but also provides direct access
to an RtpSenderInternal.
==========

[Taylor Brandstetter, 2016-06-01 20:04:24]

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

[Taylor Brandstetter, 2016-06-01 20:04:26]

https://codereview.webrtc.org/2023373002/diff/1/webrtc/api/proxy.h
File webrtc/api/proxy.h (right):

https://codereview.webrtc.org/2023373002/diff/1/webrtc/api/proxy.h#newcode311
webrtc/api/proxy.h:311: class c##ProxyEx;                                  \
I know that "ProxyEx" is a terrible name; what do you suggest? "ProxyTo<Thing>"?
"ProxyWithInner<Thing>"?

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/rtpreceiver.h
File webrtc/api/rtpreceiver.h (right):

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/rtpreceiver.h#...
webrtc/api/rtpreceiver.h:30: class RtpReceiverInternal : public
RtpReceiverInterface {
I called it "RtpReceiverInternal" because both "RtpReceiver" and
"RtpReceiverImpl" are taken... What should we do about this, long term? I know
we're trying to merge the "rtc", "cricket" and "webrtc" namespaces, but should
we think about having nested namespaces within "webrtc"?

[pthatcher1, 2016-06-01 22:18:57]

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/peerconnection.h
File webrtc/api/peerconnection.h (right):

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection.h:342:
rtc::scoped_refptr<RtpReceiverProxyEx<RtpReceiverInternal>>>::iterator
I think I need to chat with you next to a whiteboard to understand what you're
trying to accomplish here.

[Taylor Brandstetter, 2016-06-01 23:13:02]

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/peerconnection.h
File webrtc/api/peerconnection.h (right):

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection.h:342:
rtc::scoped_refptr<RtpReceiverProxyEx<RtpReceiverInternal>>>::iterator
On 2016/06/01 22:18:57, pthatcher1 wrote:
> I think I need to chat with you next to a whiteboard to understand what you're
> trying to accomplish here. 

It's an iterator to a refptr to a RtpReceiverInterface proxy that exposes an
internal RtpReceiverInternal. Clear as mud, right? :)

[pthatcher1, 2016-06-03 00:23:50]

I think I understand it and like it.  I just want to change the names a bit to
make it more clear what's going on.

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:362: sender->get()->stream_id());
would it make sense to call it internal() instead of get()?

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/peerconnection.h
File webrtc/api/peerconnection.h (right):

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection.h:337: RtpSenderInternal* FindSenderById(const
std::string& id);
Can you call this RtpSenderInternalInterface?

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection.h:339:
std::vector<rtc::scoped_refptr<RtpSenderProxyEx<RtpSenderInternal>>>::iterator
How about RtpSenderInternalProxy?

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/proxy.h
File webrtc/api/proxy.h (right):

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/proxy.h#newcod...
webrtc/api/proxy.h:334: template <class INNER_CLASS>                            
             \
Would it make sense to call this INTERNAL_CLASS?

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/proxy.h#newcod...
webrtc/api/proxy.h:341: c##ProxyEx(rtc::Thread* signaling_thread,               
           \
Would it make senes for this to be InternalProxy?

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/proxy.h#newcod...
webrtc/api/proxy.h:441: ;
Should we undo the reformatting here?

[Taylor Brandstetter, 2016-06-03 22:48:39]

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:362: sender->get()->stream_id());
On 2016/06/03 00:23:50, pthatcher1 wrote:
> would it make sense to call it internal() instead of get()?

Done.

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/peerconnection.h
File webrtc/api/peerconnection.h (right):

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection.h:337: RtpSenderInternal* FindSenderById(const
std::string& id);
On 2016/06/03 00:23:50, pthatcher1 wrote:
> Can you call this RtpSenderInternalInterface?

I'll probably move code to it in a future CL, in which case it won't just be an
interface.

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection.h:339:
std::vector<rtc::scoped_refptr<RtpSenderProxyEx<RtpSenderInternal>>>::iterator
On 2016/06/03 00:23:50, pthatcher1 wrote:
> How about RtpSenderInternalProxy?

So RtpSenderInternalProxy<RtpSenderInternal>? That works for this, but it's not
very generalized. What if there's "FooInterface" and "FooBar"/"FooBaz" that
implement it. Would we have "FooInternalProxy<FooBar>"? I still prefer my
earlier suggestions (FooProxyTo<FooBar>, FooProxyWithInternal<FooBar>...). What
do you think?

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/proxy.h
File webrtc/api/proxy.h (right):

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/proxy.h#newcod...
webrtc/api/proxy.h:334: template <class INNER_CLASS>                            
             \
On 2016/06/03 00:23:50, pthatcher1 wrote:
> Would it make sense to call this INTERNAL_CLASS?

Sure.

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/proxy.h#newcod...
webrtc/api/proxy.h:341: c##ProxyEx(rtc::Thread* signaling_thread,               
           \
On 2016/06/03 00:23:50, pthatcher1 wrote:
> Would it make senes for this to be InternalProxy?

See my other comment. That name implies to me that the inner object always has
"Internal" in its name.

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/proxy.h#newcod...
webrtc/api/proxy.h:441: ;
On 2016/06/03 00:23:50, pthatcher1 wrote:
> Should we undo the reformatting here?

This is just what "git cl format" did (yep, it's smart enough to format macros,
surprised me too). I can undo it if you like though.

[pthatcher1, 2016-06-03 23:05:49]

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/peerconnection.h
File webrtc/api/peerconnection.h (right):

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection.h:337: RtpSenderInternal* FindSenderById(const
std::string& id);
On 2016/06/03 22:48:39, Taylor Brandstetter wrote:
> On 2016/06/03 00:23:50, pthatcher1 wrote:
> > Can you call this RtpSenderInternalInterface?
> 
> I'll probably move code to it in a future CL, in which case it won't just be
an
> interface.

The purpose of that would be to share code between AudioRtpSender and
VideoRtpSender, right?  So why not RtpSenderBase or BaseRtpSender?

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection.h:339:
std::vector<rtc::scoped_refptr<RtpSenderProxyEx<RtpSenderInternal>>>::iterator
On 2016/06/03 22:48:39, Taylor Brandstetter wrote:
> On 2016/06/03 00:23:50, pthatcher1 wrote:
> > How about RtpSenderInternalProxy?
> 
> So RtpSenderInternalProxy<RtpSenderInternal>? That works for this, but it's
not
> very generalized. What if there's "FooInterface" and "FooBar"/"FooBaz" that
> implement it. Would we have "FooInternalProxy<FooBar>"? I still prefer my
> earlier suggestions (FooProxyTo<FooBar>, FooProxyWithInternal<FooBar>...).
What
> do you think?

I think FooInternalProxy is a good shortening of FooProxyWithInternal and just
as general.  So in the general case it would be FooInternalProxy<FooBar>.  In
this specific case, FooInternalProxy<RtpSenderBase>

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/proxy.h
File webrtc/api/proxy.h (right):

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/proxy.h#newcod...
webrtc/api/proxy.h:341: c##ProxyEx(rtc::Thread* signaling_thread,               
           \
On 2016/06/03 22:48:39, Taylor Brandstetter wrote:
> On 2016/06/03 00:23:50, pthatcher1 wrote:
> > Would it make senes for this to be InternalProxy?
> 
> See my other comment. That name implies to me that the inner object always has
> "Internal" in its name.

I think it just implies that you can get to the "internal" thing via internal().
 I don't think it means anything about the name of the thing.  

For example, I'd be happy if we had RtpSenderBase instead of RtpSenderInternal.

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/proxy.h#newcod...
webrtc/api/proxy.h:441: ;
On 2016/06/03 22:48:39, Taylor Brandstetter wrote:
> On 2016/06/03 00:23:50, pthatcher1 wrote:
> > Should we undo the reformatting here?
> 
> This is just what "git cl format" did (yep, it's smart enough to format
macros,
> surprised me too). I can undo it if you like though.

It looked better before, so please undo it.

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/rtpreceiver.h
File webrtc/api/rtpreceiver.h (right):

https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/rtpreceiver.h#...
webrtc/api/rtpreceiver.h:30: class RtpReceiverInternal : public
RtpReceiverInterface {
On 2016/06/01 20:04:26, Taylor Brandstetter wrote:
> I called it "RtpReceiverInternal" because both "RtpReceiver" and
> "RtpReceiverImpl" are taken... What should we do about this, long term? I know
> we're trying to merge the "rtc", "cricket" and "webrtc" namespaces, but should
> we think about having nested namespaces within "webrtc"?

How about RtpSenderBase/RtpReceiverBase?

[pthatcher1, 2016-06-03 23:24:55]

lgtm, assuming a name of ProxyWithInternal<RtpSenderBase>

On 2016/06/03 23:05:49, pthatcher1 wrote:
>
https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/peerconnection.h
> File webrtc/api/peerconnection.h (right):
> 
>
https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/peerconnection...
> webrtc/api/peerconnection.h:337: RtpSenderInternal* FindSenderById(const
> std::string& id);
> On 2016/06/03 22:48:39, Taylor Brandstetter wrote:
> > On 2016/06/03 00:23:50, pthatcher1 wrote:
> > > Can you call this RtpSenderInternalInterface?
> > 
> > I'll probably move code to it in a future CL, in which case it won't just be
> an
> > interface.
> 
> The purpose of that would be to share code between AudioRtpSender and
> VideoRtpSender, right?  So why not RtpSenderBase or BaseRtpSender?
> 
>
https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/peerconnection...
> webrtc/api/peerconnection.h:339:
> std::vector<rtc::scoped_refptr<RtpSenderProxyEx<RtpSenderInternal>>>::iterator
> On 2016/06/03 22:48:39, Taylor Brandstetter wrote:
> > On 2016/06/03 00:23:50, pthatcher1 wrote:
> > > How about RtpSenderInternalProxy?
> > 
> > So RtpSenderInternalProxy<RtpSenderInternal>? That works for this, but it's
> not
> > very generalized. What if there's "FooInterface" and "FooBar"/"FooBaz" that
> > implement it. Would we have "FooInternalProxy<FooBar>"? I still prefer my
> > earlier suggestions (FooProxyTo<FooBar>, FooProxyWithInternal<FooBar>...).
> What
> > do you think?
> 
> I think FooInternalProxy is a good shortening of FooProxyWithInternal and just
> as general.  So in the general case it would be FooInternalProxy<FooBar>.  In
> this specific case, FooInternalProxy<RtpSenderBase>
> 
> https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/proxy.h
> File webrtc/api/proxy.h (right):
> 
>
https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/proxy.h#newcod...
> webrtc/api/proxy.h:341: c##ProxyEx(rtc::Thread* signaling_thread,             
 
>            \
> On 2016/06/03 22:48:39, Taylor Brandstetter wrote:
> > On 2016/06/03 00:23:50, pthatcher1 wrote:
> > > Would it make senes for this to be InternalProxy?
> > 
> > See my other comment. That name implies to me that the inner object always
has
> > "Internal" in its name.
> 
> I think it just implies that you can get to the "internal" thing via
internal().
>  I don't think it means anything about the name of the thing.  
> 
> For example, I'd be happy if we had RtpSenderBase instead of
RtpSenderInternal.
> 
>
https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/proxy.h#newcod...
> webrtc/api/proxy.h:441: ;
> On 2016/06/03 22:48:39, Taylor Brandstetter wrote:
> > On 2016/06/03 00:23:50, pthatcher1 wrote:
> > > Should we undo the reformatting here?
> > 
> > This is just what "git cl format" did (yep, it's smart enough to format
> macros,
> > surprised me too). I can undo it if you like though.
> 
> It looked better before, so please undo it.
> 
> https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/rtpreceiver.h
> File webrtc/api/rtpreceiver.h (right):
> 
>
https://codereview.webrtc.org/2023373002/diff/20001/webrtc/api/rtpreceiver.h#...
> webrtc/api/rtpreceiver.h:30: class RtpReceiverInternal : public
> RtpReceiverInterface {
> On 2016/06/01 20:04:26, Taylor Brandstetter wrote:
> > I called it "RtpReceiverInternal" because both "RtpReceiver" and
> > "RtpReceiverImpl" are taken... What should we do about this, long term? I
know
> > we're trying to merge the "rtc", "cricket" and "webrtc" namespaces, but
should
> > we think about having nested namespaces within "webrtc"?
> 
> How about RtpSenderBase/RtpReceiverBase?

m

[Taylor Brandstetter, 2016-06-06 19:33:42]

Description was changed from

==========
Separating internal and external methods of RtpSender/RtpReceiver.

This moves the implementation specific methods to separate classes
(RtpSenderInternal/RtpReceiverInternal) so that the interface classes
represent the interface that external applications can rely on.

The reason this wasn't done earlier was that PeerConnection needed
to store proxy pointers, but also needed to access implementation-
specific methods on the underlying objects. This is now possible
by using "RtpSenderProxyEx<RtpSenderInternal>", which is a proxy
that implements RtpSenderInterface but also provides direct access
to an RtpSenderInternal.
==========

to

==========
Separating internal and external methods of RtpSender/RtpReceiver.

This moves the implementation specific methods to separate classes
(RtpSenderInternal/RtpReceiverInternal) so that the interface classes
represent the interface that external applications can rely on.

The reason this wasn't done earlier was that PeerConnection needed
to store proxy pointers, but also needed to access implementation-
specific methods on the underlying objects. This is now possible
by using "RtpSenderProxyWithInternal<RtpSenderInternal>", which is a proxy
that implements RtpSenderInterface but also provides direct access
to an RtpSenderInternal.
==========

[Taylor Brandstetter, 2016-06-06 19:49:29]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-06-06 19:49:30]

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/2023373002/#ps60001 (title:
"Renaming "ProxyTo<X>" to "ProxyWithInternal<X>"")

[commit-bot: I haz the power, 2016-06-06 19:49:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2023373002/60001

[commit-bot: I haz the power, 2016-06-06 21:27:42]

Description was changed from

==========
Separating internal and external methods of RtpSender/RtpReceiver.

This moves the implementation specific methods to separate classes
(RtpSenderInternal/RtpReceiverInternal) so that the interface classes
represent the interface that external applications can rely on.

The reason this wasn't done earlier was that PeerConnection needed
to store proxy pointers, but also needed to access implementation-
specific methods on the underlying objects. This is now possible
by using "RtpSenderProxyWithInternal<RtpSenderInternal>", which is a proxy
that implements RtpSenderInterface but also provides direct access
to an RtpSenderInternal.
==========

to

==========
Separating internal and external methods of RtpSender/RtpReceiver.

This moves the implementation specific methods to separate classes
(RtpSenderInternal/RtpReceiverInternal) so that the interface classes
represent the interface that external applications can rely on.

The reason this wasn't done earlier was that PeerConnection needed
to store proxy pointers, but also needed to access implementation-
specific methods on the underlying objects. This is now possible
by using "RtpSenderProxyWithInternal<RtpSenderInternal>", which is a proxy
that implements RtpSenderInterface but also provides direct access
to an RtpSenderInternal.
==========

[commit-bot: I haz the power, 2016-06-06 21:27:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-06 21:27:52]

Description was changed from

==========
Separating internal and external methods of RtpSender/RtpReceiver.

This moves the implementation specific methods to separate classes
(RtpSenderInternal/RtpReceiverInternal) so that the interface classes
represent the interface that external applications can rely on.

The reason this wasn't done earlier was that PeerConnection needed
to store proxy pointers, but also needed to access implementation-
specific methods on the underlying objects. This is now possible
by using "RtpSenderProxyWithInternal<RtpSenderInternal>", which is a proxy
that implements RtpSenderInterface but also provides direct access
to an RtpSenderInternal.
==========

to

==========
Separating internal and external methods of RtpSender/RtpReceiver.

This moves the implementation specific methods to separate classes
(RtpSenderInternal/RtpReceiverInternal) so that the interface classes
represent the interface that external applications can rely on.

The reason this wasn't done earlier was that PeerConnection needed
to store proxy pointers, but also needed to access implementation-
specific methods on the underlying objects. This is now possible
by using "RtpSenderProxyWithInternal<RtpSenderInternal>", which is a proxy
that implements RtpSenderInterface but also provides direct access
to an RtpSenderInternal.

Committed: https://crrev.com/a601f5c86310c078c21161947f526f2d1e65c9cc
Cr-Commit-Position: refs/heads/master@{#13056}
==========

[commit-bot: I haz the power, 2016-06-06 21:27:53]

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