Refactor: simplify interface of SenderRtcpSession and CastTransport.

Refactor: simplify interface of SenderRtcpSession and CastTransport.
SenderRtcpSession constructor, CastTransport::InitializeAudio, and
CastTransport::InitializeVideo require caller inject one callback on
receiving every type of RTCP message. Defined the interface and reduced
the number of input parameters for these functions.

BUG=588275
Committed: https://crrev.com/3ea24de275a40219d274361afa0a21d018594395
Cr-Commit-Position: refs/heads/master@{#391319}

[xjz, 2016-04-12 01:10:17]

Patchset #1 (id:1) has been deleted

[xjz, 2016-04-12 05:07:58]

Patchset #3 (id:60001) has been deleted

[xjz, 2016-04-12 18:57:51]

Patchset #1 (id:20001) has been deleted

[xjz, 2016-04-12 18:58:04]

Patchset #1 (id:40001) has been deleted

[xjz, 2016-04-13 00:18:28]

Patchset #1 (id:80001) has been deleted

[xjz, 2016-04-13 00:18:58]

Patchset #1 (id:100001) has been deleted

[xjz, 2016-04-13 18:44:27]

Patchset #1 (id:120001) has been deleted

[xjz, 2016-04-13 18:44:43]

Patchset #1 (id:140001) has been deleted

[xjz, 2016-04-13 20:51:04]

xjz@chromium.org changed reviewers:
+ miu@chromium.org

[xjz, 2016-04-13 20:51:05]

PTAL

[miu, 2016-04-15 23:14:39]

Nice work!  Great to get rid of all these extra callbacks.  First round
comments:

https://codereview.chromium.org/1878883003/diff/160001/chrome/browser/media/c...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://codereview.chromium.org/1878883003/diff/160001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:65:
cast::CastTransportHostFilter* cast_transport_host_filter)
Should the third arg be of type base::WeakPtr<cast::CastTransportHostFilter>? 
The callbacks in the old code called Bind() with weak ptrs...

Or, maybe you confirmed we didn't need the weak pointers?  If so, please add a
comment that states something like, "CastTransportHostFilter must outlive all
RtcpClient instances."

https://codereview.chromium.org/1878883003/diff/160001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:87: int32_t channel_id_;
Please make these three members const.

https://codereview.chromium.org/1878883003/diff/160001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:189: void
CastTransportHostFilter::OnInitializeAudio(
side note: Notice how OnInitializeAudio() is identical to OnInitializeVideo()? 
And, CastTransport::InitializeAudio() is identical to
CastTransport::InitializeVideo()?  Not for this change, but it'd be great if we
could, in the long-term, replace all the separate "init/start audio" and
"init/start video" methods with a single "init/start rtp stream" that handles
either media type.

https://codereview.chromium.org/1878883003/diff/160001/chrome/renderer/media/...
File chrome/renderer/media/cast_transport_ipc.cc (right):

https://codereview.chromium.org/1878883003/diff/160001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_ipc.cc:43: clients_[config.ssrc] =
std::move(rtcp_client);
This was missing before, but consider adding: DCHECK(clients_.find(config.ssrc)
== clients_.end())

https://codereview.chromium.org/1878883003/diff/160001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_ipc.cc:50: clients_[config.ssrc] =
std::move(rtcp_client);
ditto here (maybe add DCHECK?)

https://codereview.chromium.org/1878883003/diff/160001/chrome/renderer/media/...
File chrome/renderer/media/cast_transport_ipc.h (right):

https://codereview.chromium.org/1878883003/diff/160001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_ipc.h:79: typedef std::map<uint32_t,
std::unique_ptr<media::cast::RtpSenderRtcpClient>>
nit: Since you're changing this line, please use the new, preferred C++ syntax:

  using ClientMap = std::map<uint32_t,
std::unique_ptr<media::cast::RtpSenderRtcpClient>>;

https://codereview.chromium.org/1878883003/diff/160001/media/cast/net/cast_tr...
File media/cast/net/cast_transport.h (right):

https://codereview.chromium.org/1878883003/diff/160001/media/cast/net/cast_tr...
media/cast/net/cast_transport.h:54: typedef
base::Callback<void(std::unique_ptr<std::vector<FrameEvent>>,
nit: ditto on "using" instead of "typedef" syntax here.

https://codereview.chromium.org/1878883003/diff/160001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_impl.cc (right):

https://codereview.chromium.org/1878883003/diff/160001/media/cast/net/cast_tr...
media/cast/net/cast_transport_impl.cc:70: uint32_t rtp_sender_ssrc_;
ditto: All const, please.

https://codereview.chromium.org/1878883003/diff/160001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_impl.h (right):

https://codereview.chromium.org/1878883003/diff/160001/media/cast/net/cast_tr...
media/cast/net/cast_transport_impl.h:115: void
OnReceivedLogMessage(EventMediaType media_type,
These don't need to be made public.  (See my comments for
Audio/Video/FrameSender below for details on how to do this.)

https://codereview.chromium.org/1878883003/diff/160001/media/cast/net/rtcp/se...
File media/cast/net/rtcp/sender_rtcp_session.h (right):

https://codereview.chromium.org/1878883003/diff/160001/media/cast/net/rtcp/se...
media/cast/net/rtcp/sender_rtcp_session.h:44: class Client {
This is identical to RtpSenderRtcpClient, except it adds one more method.  So,
consider just having one interface w/ all 4 methods.  Then, the name of the
interface can just be "RtcpClient" or maybe "RtcpObserver" would be more
descriptive?

https://codereview.chromium.org/1878883003/diff/160001/media/cast/sender/audi...
File media/cast/sender/audio_sender.cc (right):

https://codereview.chromium.org/1878883003/diff/160001/media/cast/sender/audi...
media/cast/sender/audio_sender.cc:22: class AudioSenderRtcpClient : public
RtpSenderRtcpClient {
This is duplicate code (same as in VideoSender).  So, can you just write it once
for FrameSender?  See comment below...

https://codereview.chromium.org/1878883003/diff/160001/media/cast/sender/fram...
File media/cast/sender/frame_sender.h (right):

https://codereview.chromium.org/1878883003/diff/160001/media/cast/sender/fram...
media/cast/sender/frame_sender.h:57: void OnReceivedCastFeedback(const
RtcpCastMessage& cast_feedback);
Instead of making these methods public, consider one of two options:

1. Have FrameSender extend and implement the RtpSenderRtcpClient interface
directly (i.e., no separate class).

-or-

2. You should give the RtpSenderRtcpClient subclass access to the protected
members of FrameSender.  This can be done by forward-declaring
FrameSenderRtcpClient as an inner class of FrameSender.  Meaning:

  class FrameSender {
    ...
   protected:
    class RtcpClient;
    ...
  };

Then, in frame_sender.cc:

class FrameSender::RtcpClient : public RtpSenderRtcpClient {
  ...code in this class has access to protected and private members of
FrameSender...
};

(And, note that you only need one of these; and not a separate one for
AudioSender vs VideoSender.)

IMO, #1 would be much simpler than #2.  Perhaps the same approach would work for
other classes involved in this change?

[xjz, 2016-04-19 17:01:45]

Patchset #2 (id:180001) has been deleted

[xjz, 2016-04-19 18:24:29]

Patchset #3 (id:220001) has been deleted

[xjz, 2016-04-19 18:58:49]

Patchset #2 (id:200001) has been deleted

[xjz, 2016-04-20 01:09:03]

Addressed comments. PTAL.

https://codereview.chromium.org/1878883003/diff/160001/chrome/browser/media/c...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://codereview.chromium.org/1878883003/diff/160001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:65:
cast::CastTransportHostFilter* cast_transport_host_filter)
On 2016/04/15 23:14:39, miu wrote:
> Should the third arg be of type base::WeakPtr<cast::CastTransportHostFilter>? 
> The callbacks in the old code called Bind() with weak ptrs...
> 
> Or, maybe you confirmed we didn't need the weak pointers?  If so, please add a
> comment that states something like, "CastTransportHostFilter must outlive all
> RtcpClient instances."

Done. Use weak ptr.

https://codereview.chromium.org/1878883003/diff/160001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:87: int32_t channel_id_;
On 2016/04/15 23:14:39, miu wrote:
> Please make these three members const.

Done.

https://codereview.chromium.org/1878883003/diff/160001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:189: void
CastTransportHostFilter::OnInitializeAudio(
On 2016/04/15 23:14:39, miu wrote:
> side note: Notice how OnInitializeAudio() is identical to OnInitializeVideo()?

> And, CastTransport::InitializeAudio() is identical to
> CastTransport::InitializeVideo()?  Not for this change, but it'd be great if
we
> could, in the long-term, replace all the separate "init/start audio" and
> "init/start video" methods with a single "init/start rtp stream" that handles
> either media type.

Sounds great! Add a todo comment.

https://codereview.chromium.org/1878883003/diff/160001/chrome/renderer/media/...
File chrome/renderer/media/cast_transport_ipc.cc (right):

https://codereview.chromium.org/1878883003/diff/160001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_ipc.cc:43: clients_[config.ssrc] =
std::move(rtcp_client);
On 2016/04/15 23:14:39, miu wrote:
> This was missing before, but consider adding:
DCHECK(clients_.find(config.ssrc)
> == clients_.end())

Done.

https://codereview.chromium.org/1878883003/diff/160001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_ipc.cc:50: clients_[config.ssrc] =
std::move(rtcp_client);
On 2016/04/15 23:14:39, miu wrote:
> ditto here (maybe add DCHECK?)

Done.

https://codereview.chromium.org/1878883003/diff/160001/chrome/renderer/media/...
File chrome/renderer/media/cast_transport_ipc.h (right):

https://codereview.chromium.org/1878883003/diff/160001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_ipc.h:79: typedef std::map<uint32_t,
std::unique_ptr<media::cast::RtpSenderRtcpClient>>
On 2016/04/15 23:14:39, miu wrote:
> nit: Since you're changing this line, please use the new, preferred C++
syntax:
> 
>   using ClientMap = std::map<uint32_t,
> std::unique_ptr<media::cast::RtpSenderRtcpClient>>;

Done.

https://codereview.chromium.org/1878883003/diff/160001/media/cast/net/cast_tr...
File media/cast/net/cast_transport.h (right):

https://codereview.chromium.org/1878883003/diff/160001/media/cast/net/cast_tr...
media/cast/net/cast_transport.h:54: typedef
base::Callback<void(std::unique_ptr<std::vector<FrameEvent>>,
On 2016/04/15 23:14:39, miu wrote:
> nit: ditto on "using" instead of "typedef" syntax here.

Done.

https://codereview.chromium.org/1878883003/diff/160001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_impl.cc (right):

https://codereview.chromium.org/1878883003/diff/160001/media/cast/net/cast_tr...
media/cast/net/cast_transport_impl.cc:70: uint32_t rtp_sender_ssrc_;
On 2016/04/15 23:14:39, miu wrote:
> ditto: All const, please.

Done.

https://codereview.chromium.org/1878883003/diff/160001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_impl.h (right):

https://codereview.chromium.org/1878883003/diff/160001/media/cast/net/cast_tr...
media/cast/net/cast_transport_impl.h:115: void
OnReceivedLogMessage(EventMediaType media_type,
On 2016/04/15 23:14:39, miu wrote:
> These don't need to be made public.  (See my comments for
> Audio/Video/FrameSender below for details on how to do this.)

Done.

https://codereview.chromium.org/1878883003/diff/160001/media/cast/net/rtcp/se...
File media/cast/net/rtcp/sender_rtcp_session.h (right):

https://codereview.chromium.org/1878883003/diff/160001/media/cast/net/rtcp/se...
media/cast/net/rtcp/sender_rtcp_session.h:44: class Client {
On 2016/04/15 23:14:39, miu wrote:
> This is identical to RtpSenderRtcpClient, except it adds one more method.  So,
> consider just having one interface w/ all 4 methods.  Then, the name of the
> interface can just be "RtcpClient" or maybe "RtcpObserver" would be more
> descriptive?

Done. Named the interface "SenderRtcpObserver".

https://codereview.chromium.org/1878883003/diff/160001/media/cast/sender/audi...
File media/cast/sender/audio_sender.cc (right):

https://codereview.chromium.org/1878883003/diff/160001/media/cast/sender/audi...
media/cast/sender/audio_sender.cc:22: class AudioSenderRtcpClient : public
RtpSenderRtcpClient {
On 2016/04/15 23:14:39, miu wrote:
> This is duplicate code (same as in VideoSender).  So, can you just write it
once
> for FrameSender?  See comment below...

Done.

https://codereview.chromium.org/1878883003/diff/160001/media/cast/sender/fram...
File media/cast/sender/frame_sender.h (right):

https://codereview.chromium.org/1878883003/diff/160001/media/cast/sender/fram...
media/cast/sender/frame_sender.h:57: void OnReceivedCastFeedback(const
RtcpCastMessage& cast_feedback);
On 2016/04/15 23:14:39, miu wrote:
> Instead of making these methods public, consider one of two options:
> 
> 1. Have FrameSender extend and implement the RtpSenderRtcpClient interface
> directly (i.e., no separate class).
> 
> -or-
> 
> 2. You should give the RtpSenderRtcpClient subclass access to the protected
> members of FrameSender.  This can be done by forward-declaring
> FrameSenderRtcpClient as an inner class of FrameSender.  Meaning:
> 
>   class FrameSender {
>     ...
>    protected:
>     class RtcpClient;
>     ...
>   };
> 
> Then, in frame_sender.cc:
> 
> class FrameSender::RtcpClient : public RtpSenderRtcpClient {
>   ...code in this class has access to protected and private members of
> FrameSender...
> };
> 
> (And, note that you only need one of these; and not a separate one for
> AudioSender vs VideoSender.)
> 
> IMO, #1 would be much simpler than #2.  Perhaps the same approach would work
for
> other classes involved in this change?

Done. Choose #2 for convenience.

[miu, 2016-04-25 23:23:54]

Looking good.  More comments:

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/cast_tr...
File media/cast/net/cast_transport.h (right):

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/cast_tr...
media/cast/net/cast_transport.h:73: virtual void OnReceiverLogReceived(const
RtcpReceiverLogMessage& log){};
nit: space before {}

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_impl.cc (right):

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/cast_tr...
media/cast/net/cast_transport_impl.cc:73:
cast_transport_impl_->OnReceivedCastMessage(rtp_sender_ssrc_, cast_message);
naming nit: OnCastMessageReceived versus OnReceivedCastMessage.  Let's pick one,
to be consistent.  ;)

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_impl_unittest.cc (right):

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/cast_tr...
media/cast/net/cast_transport_impl_unittest.cc:31: class RtcpObserver : public
SenderRtcpObserver {
naming nit: How about StubRtcpObserver?  (and a comment stating that the test
code calls into CastTransportImpl directly, instead of having SenderRtcpSession
call into it through this interface)

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/rtcp/rt...
File media/cast/net/rtcp/rtcp_unittest.cc (right):

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/rtcp/rt...
media/cast/net/rtcp/rtcp_unittest.cc:81: class RtcpObserver : public
SenderRtcpObserver {
Seems like you only need this class because SenderRtcpSession has to own the
SenderRtcpObserver.  Otherwise, RtcpTest could implement the SenderRtcpObserver
interface itself.  See comments in sender_rtcp_session.h for more about observer
object ownership...

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/rtcp/rt...
media/cast/net/rtcp/rtcp_unittest.cc:213: void
RtcpTest::RtcpObserver::OnCastMessageReceived(
Feel free to inline these methods in the class definition.  They're so simple,
IMHO, it'd be better for readability to have them there.

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/rtcp/se...
File media/cast/net/rtcp/sender_rtcp_session.h (right):

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/rtcp/se...
media/cast/net/rtcp/sender_rtcp_session.h:32: // from receiver and passes the
information to the Client:
s/the Client/a SenderRtcpObserver/

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/rtcp/se...
media/cast/net/rtcp/sender_rtcp_session.h:44:
SenderRtcpSession(std::unique_ptr<SenderRtcpObserver> observer,
The object ownership graph is a bit weird around this: First, CastTransportImpl
creates a CastTransportImpl::RtcpObserver that contains a raw pointer back to
the CastTransportImpl.  Then, it passes ownership of the observer to
SenderRtcpSession.

Generally, when the observer pattern is used, the observer is owned by the
client code, and it's the responsibility of client code to make sure the
observer outlives the observed (or use an Add/Remove observer scheme).  In this
case, it seems like CastTransportImpl should own the RtcpObserver it creates,
pass a raw pointer to the SenderRtcpSession ctor here, and then ensure the
RtcpObserver outlives the SenderRtcpSession.  It can easily do this since
CastTransportImpl owns the SenderRtcpSession instance.

Changing this also help cleans up issues I pointed out in other files...

[xjz, 2016-04-29 19:15:50]

Changes in PS#3: 
1. Addressed comments.
2. Some renames.
PTAL. Thanks!

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/cast_tr...
File media/cast/net/cast_transport.h (right):

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/cast_tr...
media/cast/net/cast_transport.h:73: virtual void OnReceiverLogReceived(const
RtcpReceiverLogMessage& log){};
On 2016/04/25 23:23:53, miu wrote:
> nit: space before {}

Done.

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_impl.cc (right):

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/cast_tr...
media/cast/net/cast_transport_impl.cc:73:
cast_transport_impl_->OnReceivedCastMessage(rtp_sender_ssrc_, cast_message);
On 2016/04/25 23:23:53, miu wrote:
> naming nit: OnCastMessageReceived versus OnReceivedCastMessage.  Let's pick
one,
> to be consistent.  ;)

Done.

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_impl_unittest.cc (right):

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/cast_tr...
media/cast/net/cast_transport_impl_unittest.cc:31: class RtcpObserver : public
SenderRtcpObserver {
On 2016/04/25 23:23:53, miu wrote:
> naming nit: How about StubRtcpObserver?  (and a comment stating that the test
> code calls into CastTransportImpl directly, instead of having
SenderRtcpSession
> call into it through this interface)

Renamed. I don't quite get this comment. CastTransportImpl will start the
SenderRtcpSession, which will handle the RTCP messages through this interface as
normal.

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/rtcp/rt...
File media/cast/net/rtcp/rtcp_unittest.cc (right):

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/rtcp/rt...
media/cast/net/rtcp/rtcp_unittest.cc:81: class RtcpObserver : public
SenderRtcpObserver {
On 2016/04/25 23:23:53, miu wrote:
> Seems like you only need this class because SenderRtcpSession has to own the
> SenderRtcpObserver.  Otherwise, RtcpTest could implement the
SenderRtcpObserver
> interface itself.  See comments in sender_rtcp_session.h for more about
observer
> object ownership...

Done.

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/rtcp/rt...
media/cast/net/rtcp/rtcp_unittest.cc:213: void
RtcpTest::RtcpObserver::OnCastMessageReceived(
On 2016/04/25 23:23:53, miu wrote:
> Feel free to inline these methods in the class definition.  They're so simple,
> IMHO, it'd be better for readability to have them there.

Done.

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/rtcp/se...
File media/cast/net/rtcp/sender_rtcp_session.h (right):

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/rtcp/se...
media/cast/net/rtcp/sender_rtcp_session.h:32: // from receiver and passes the
information to the Client:
On 2016/04/25 23:23:54, miu wrote:
> s/the Client/a SenderRtcpObserver/

Done.

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/rtcp/se...
media/cast/net/rtcp/sender_rtcp_session.h:44:
SenderRtcpSession(std::unique_ptr<SenderRtcpObserver> observer,
On 2016/04/25 23:23:54, miu wrote:
> The object ownership graph is a bit weird around this: First,
CastTransportImpl
> creates a CastTransportImpl::RtcpObserver that contains a raw pointer back to
> the CastTransportImpl.  Then, it passes ownership of the observer to
> SenderRtcpSession.
> 
> Generally, when the observer pattern is used, the observer is owned by the
> client code, and it's the responsibility of client code to make sure the
> observer outlives the observed (or use an Add/Remove observer scheme).  In
this
> case, it seems like CastTransportImpl should own the RtcpObserver it creates,
> pass a raw pointer to the SenderRtcpSession ctor here, and then ensure the
> RtcpObserver outlives the SenderRtcpSession.  It can easily do this since
> CastTransportImpl owns the SenderRtcpSession instance.
> 
> Changing this also help cleans up issues I pointed out in other files...

Done.

[miu, 2016-05-02 21:30:01]

lgtm

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_impl_unittest.cc (right):

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/cast_tr...
media/cast/net/cast_transport_impl_unittest.cc:31: class RtcpObserver : public
SenderRtcpObserver {
On 2016/04/29 19:15:49, xjz wrote:
> On 2016/04/25 23:23:53, miu wrote:
> > naming nit: How about StubRtcpObserver?  (and a comment stating that the
test
> > code calls into CastTransportImpl directly, instead of having
> SenderRtcpSession
> > call into it through this interface)
> 
> Renamed. I don't quite get this comment. CastTransportImpl will start the
> SenderRtcpSession, which will handle the RTCP messages through this interface
as
> normal. 

What I meant was that, for this test code, there are no raw RTCP packets being
passed to SenderRtcpSession (which would then call-back to CastTransportImpl).
Instead, the test code is calling CastTransportImpl directly. For example, on
line 235:

  transport_sender_->OnReceivedCastMessage(kVideoSsrc, cast_message);

So, this RtcpObserver here is just a "placeholder" that isn't used for anything
other than CastTransportSender not accepting a nullptr when calling
InitializeAudio/Video().

[xjz, 2016-05-03 17:45:14]

Patchset #4 (id:280001) has been deleted

[xjz, 2016-05-03 18:13:28]

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_impl_unittest.cc (right):

https://codereview.chromium.org/1878883003/diff/240001/media/cast/net/cast_tr...
media/cast/net/cast_transport_impl_unittest.cc:31: class RtcpObserver : public
SenderRtcpObserver {
On 2016/05/02 21:30:01, miu wrote:
> On 2016/04/29 19:15:49, xjz wrote:
> > On 2016/04/25 23:23:53, miu wrote:
> > > naming nit: How about StubRtcpObserver?  (and a comment stating that the
> test
> > > code calls into CastTransportImpl directly, instead of having
> > SenderRtcpSession
> > > call into it through this interface)
> > 
> > Renamed. I don't quite get this comment. CastTransportImpl will start the
> > SenderRtcpSession, which will handle the RTCP messages through this
interface
> as
> > normal. 
> 
> What I meant was that, for this test code, there are no raw RTCP packets being
> passed to SenderRtcpSession (which would then call-back to CastTransportImpl).
> Instead, the test code is calling CastTransportImpl directly. For example, on
> line 235:
> 
>   transport_sender_->OnReceivedCastMessage(kVideoSsrc, cast_message);
> 
> So, this RtcpObserver here is just a "placeholder" that isn't used for
anything
> other than CastTransportSender not accepting a nullptr when calling
> InitializeAudio/Video().

Got it. Thanks for the explanation.:)

[xjz, 2016-05-03 18:13:36]

The CQ bit was checked by xjz@chromium.org

[xjz, 2016-05-03 18:13:36]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org
Link to the patchset: https://codereview.chromium.org/1878883003/#ps300001
(title: "Rebase")

[commit-bot: I haz the power, 2016-05-03 18:14:01]

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

[commit-bot: I haz the power, 2016-05-03 19:10:49]

Committed patchset #4 (id:300001)

[commit-bot: I haz the power, 2016-05-03 19:12:40]

Description was changed from

==========
Refactor: simplify interface of SenderRtcpSession and CastTransport.
SenderRtcpSession constructor, CastTransport::InitializeAudio, and
CastTransport::InitializeVideo require caller inject one callback on
receiving every type of RTCP message. Defined the interface and reduced
the number of input parameters for these functions.

BUG=588275
==========

to

==========
Refactor: simplify interface of SenderRtcpSession and CastTransport.
SenderRtcpSession constructor, CastTransport::InitializeAudio, and
CastTransport::InitializeVideo require caller inject one callback on
receiving every type of RTCP message. Defined the interface and reduced
the number of input parameters for these functions.

BUG=588275

Committed: https://crrev.com/3ea24de275a40219d274361afa0a21d018594395
Cr-Commit-Position: refs/heads/master@{#391319}
==========

[commit-bot: I haz the power, 2016-05-03 19:12:42]

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