Refactor: simplify interface of SenderRtcpSession and CastTransport.

chrome/browser/media/cast_transport_host_filter.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/media/cast_transport_host_filter.h"
 
 #include "base/memory/ptr_util.h"
 #include "base/thread_task_runner_handle.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/common/cast_messages.h"
@@ skipping 40 matching lines @@
   cast_transport_host_filter_->Send(
       new CastMsg_RawEvents(channel_id_, *packet_events, *frame_events));
 }
 
 void TransportClient::ProcessRtpPacket(
     std::unique_ptr<media::cast::Packet> packet) {
   cast_transport_host_filter_->Send(
       new CastMsg_ReceivedPacket(channel_id_, *packet));
 }
 
+class RtcpClient : public media::cast::RtpSenderRtcpClient {
+ public:
+  RtcpClient(int32_t channel_id,
+             uint32_t rtp_sender_ssrc,
+             cast::CastTransportHostFilter* cast_transport_host_filter)

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

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."

[xjz, 2016/04/20 01:09:02]

Done. Use weak ptr.

+      : channel_id_(channel_id),
+        rtp_sender_ssrc_(rtp_sender_ssrc),
+        cast_transport_host_filter_(cast_transport_host_filter) {}
+
+  void OnCastMessageReceived(
+      const media::cast::RtcpCastMessage& cast_message) override {
+    cast_transport_host_filter_->Send(new CastMsg_RtcpCastMessage(
+        channel_id_, rtp_sender_ssrc_, cast_message));
+  }
+
+  void OnRttReceived(base::TimeDelta round_trip_time) override {
+    cast_transport_host_filter_->Send(
+        new CastMsg_Rtt(channel_id_, rtp_sender_ssrc_, round_trip_time));
+  }
+
+  void OnPliReceived() override {
+    cast_transport_host_filter_->Send(
+        new CastMsg_Pli(channel_id_, rtp_sender_ssrc_));
+  }
+
+ private:
+  int32_t channel_id_;

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

Please make these three members const.

[xjz, 2016/04/20 01:09:02]

Done.

+  uint32_t rtp_sender_ssrc_;
+  cast::CastTransportHostFilter* cast_transport_host_filter_;
+
+  DISALLOW_COPY_AND_ASSIGN(RtcpClient);
+};
+
 }  // namespace
 
 namespace cast {
 
 CastTransportHostFilter::CastTransportHostFilter()
     : BrowserMessageFilter(CastMsgStart),
       weak_factory_(this) {}
 
 CastTransportHostFilter::~CastTransportHostFilter() {}
 
@@ skipping 25 matching lines @@
     IPC_MESSAGE_HANDLER(CastHostMsg_AddRtcpEvents, OnAddRtcpEvents)
     IPC_MESSAGE_HANDLER(CastHostMsg_AddRtpReceiverReport,
                         OnAddRtpReceiverReport)
     IPC_MESSAGE_HANDLER(CastHostMsg_SendRtcpFromRtpReceiver,
                         OnSendRtcpFromRtpReceiver)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
-void CastTransportHostFilter::SendRtt(int32_t channel_id,
-                                      uint32_t rtp_sender_ssrc,
-                                      base::TimeDelta rtt) {
-  Send(new CastMsg_Rtt(channel_id, rtp_sender_ssrc, rtt));
-}
-
-void CastTransportHostFilter::SendCastMessage(
-    int32_t channel_id,
-    uint32_t rtp_sender_ssrc,
-    const media::cast::RtcpCastMessage& cast_message) {
-  Send(new CastMsg_RtcpCastMessage(channel_id, rtp_sender_ssrc, cast_message));
-}
-
-void CastTransportHostFilter::SendReceivedPli(int32_t channel_id,
-                                              uint32_t rtp_sender_ssrc) {
-  Send(new CastMsg_Pli(channel_id, rtp_sender_ssrc));
-}
-
 void CastTransportHostFilter::OnNew(int32_t channel_id,
                                     const net::IPEndPoint& local_end_point,
                                     const net::IPEndPoint& remote_end_point,
                                     const base::DictionaryValue& options) {
   if (!power_save_blocker_) {
     DVLOG(1) << ("Preventing the application from being suspended while one or "
                  "more transports are active for Cast Streaming.");
     power_save_blocker_ = content::PowerSaveBlocker::Create(
         content::PowerSaveBlocker::kPowerSaveBlockPreventAppSuspension,
         content::PowerSaveBlocker::kReasonOther,
@@ skipping 30 matching lines @@
   }
 
   if (id_map_.IsEmpty()) {
     DVLOG_IF(1, power_save_blocker_) <<
         ("Releasing the block on application suspension since no transports "
          "are active anymore for Cast Streaming.");
     power_save_blocker_.reset();
   }
 }
 
 void CastTransportHostFilter::OnInitializeAudio(

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

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.

[xjz, 2016/04/20 01:09:02]

Sounds great! Add a todo comment.

     int32_t channel_id,
     const media::cast::CastTransportRtpConfig& config) {
   media::cast::CastTransport* sender = id_map_.Lookup(channel_id);
   if (sender) {
-    sender->InitializeAudio(
-        config, base::Bind(&CastTransportHostFilter::SendCastMessage,
-                           weak_factory_.GetWeakPtr(), channel_id, config.ssrc),
-        base::Bind(&CastTransportHostFilter::SendRtt,
-                   weak_factory_.GetWeakPtr(), channel_id, config.ssrc),
-        base::Bind(&CastTransportHostFilter::SendReceivedPli,
-                   weak_factory_.GetWeakPtr(), channel_id, config.ssrc));
+    sender->InitializeAudio(config, base::WrapUnique(new RtcpClient(
+                                        channel_id, config.ssrc, this)));
   } else {
     DVLOG(1)
         << "CastTransportHostFilter::OnInitializeAudio on non-existing channel";
   }
 }
 
 void CastTransportHostFilter::OnInitializeVideo(
     int32_t channel_id,
     const media::cast::CastTransportRtpConfig& config) {
   media::cast::CastTransport* sender = id_map_.Lookup(channel_id);
   if (sender) {
-    sender->InitializeVideo(
-        config, base::Bind(&CastTransportHostFilter::SendCastMessage,
-                           weak_factory_.GetWeakPtr(), channel_id, config.ssrc),
-        base::Bind(&CastTransportHostFilter::SendRtt,
-                   weak_factory_.GetWeakPtr(), channel_id, config.ssrc),
-        base::Bind(&CastTransportHostFilter::SendReceivedPli,
-                   weak_factory_.GetWeakPtr(), channel_id, config.ssrc));
+    sender->InitializeVideo(config, base::WrapUnique(new RtcpClient(
+                                        channel_id, config.ssrc, this)));
   } else {
     DVLOG(1)
         << "CastTransportHostFilter::OnInitializeVideo on non-existing channel";
   }
 }
 
 void CastTransportHostFilter::OnInsertFrame(
     int32_t channel_id,
     uint32_t ssrc,
     const media::cast::EncodedFrame& frame) {
@@ skipping 129 matching lines @@
   if (sender) {
     sender->SendRtcpFromRtpReceiver();
   } else {
     DVLOG(1)
         << "CastTransportHostFilter::OnSendRtcpFromRtpReceiver "
         << "on non-existing channel";
   }
 }
 
 }  // namespace cast