Simplify interface for media/cast: CastTransportSenderImpl

Simplify interface for media/cast: CastTransportSenderImpl

Defined an interface to reduce the 11-arg constructor down to 4 args.

BUG=557477
Committed: https://crrev.com/74f752173cf8084d9f96073eaa88236f0ac54b79
Cr-Commit-Position: refs/heads/master@{#377148}

[xjz, 2015-12-10 23:52:13]

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

[xjz, 2015-12-10 23:52:13]

miu: PTAL this proposed interface change first. Thanks!

[Irfan, 2015-12-11 17:33:25]

https://codereview.chromium.org/1515023002/diff/1/media/cast/net/cast_transpo...
File media/cast/net/cast_transport_sender.h (left):

https://codereview.chromium.org/1515023002/diff/1/media/cast/net/cast_transpo...
media/cast/net/cast_transport_sender.h:66: const BulkRawEventsCallback&
raw_events_callback,
my feeling here is that only the callbacks should be combined as an interface
(status, packet reception, raw event logs). Combining everything makes it harder
to read.

[xjz, 2015-12-11 18:30:06]

On 2015/12/11 17:33:25, Irfan wrote:
>
https://codereview.chromium.org/1515023002/diff/1/media/cast/net/cast_transpo...
> File media/cast/net/cast_transport_sender.h (left):
> 
>
https://codereview.chromium.org/1515023002/diff/1/media/cast/net/cast_transpo...
> media/cast/net/cast_transport_sender.h:66: const BulkRawEventsCallback&
> raw_events_callback,
> my feeling here is that only the callbacks should be combined as an interface
> (status, packet reception, raw event logs). Combining everything makes it
harder
> to read.

I did only include three callbacks in the interface at first. 
However, in that way we only saved 2 args. 
And we sacrificed the flexibility of using callbacks. 
Each caller has to implement the interface.
Does that worth?

[Irfan, 2015-12-11 18:56:05]

https://codereview.chromium.org/1515023002/diff/1/media/cast/net/cast_transpo...
File media/cast/net/cast_transport_sender.h (right):

https://codereview.chromium.org/1515023002/diff/1/media/cast/net/cast_transpo...
media/cast/net/cast_transport_sender.h:79: base::TickClock* clock,
Perhaps the UDP transport fields can be combined (local, remote, netlog &
transport options) separately.

Combining unrelated fields shifts the readability issue IMO

[miu, 2015-12-12 01:30:45]

https://codereview.chromium.org/1515023002/diff/1/media/cast/net/cast_transpo...
File media/cast/net/cast_transport_sender.h (right):

https://codereview.chromium.org/1515023002/diff/1/media/cast/net/cast_transpo...
media/cast/net/cast_transport_sender.h:79: base::TickClock* clock,
On 2015/12/11 18:56:04, Irfan wrote:
> Perhaps the UDP transport fields can be combined (local, remote, netlog &
> transport options) separately.
> 
> Combining unrelated fields shifts the readability issue IMO

Agreed.  Thinking about this, a structure I'd propose is:

class CastTransportSender {
 public:
  class Client {
   public:
    virtual void OnStatusChange(CastTransportStatus status) = 0;
    virtual void OnReceivedLoggingEvents(scoped_ptr<vector<FrameEvent>>,
                                         scoped_ptr<vector<PacketEvent>>) = 0;
    virtual bool OnReceviedPacket(scoped_ptr<Packet> packet) = 0;
   protected:
    virtual ~Client();
  };

  struct CreateParams {
    CreateParams();

    net::NetLog* net_log;
    base::TickClock* clock;
    net::IPEndPoint local_end_point;
    net::IPEndPoint remote_end_point;
    Client* client;
    base::TimeDelta logging_flush_interval;
    base::DictionaryValue optional_config;
  };

  static scoped_ptr<CastTransportSender> Create(const CreateParams& params);

  ...
};

The "CreateParams inner struct" pattern is used in a lot of other places in
Chromium where tons of arguments are passed-in to construct an instance. 
Example:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

Some of the weirdness here is around CastTransportSender being used for the cast
receiver as well.  So, this class is really a CastTransportReceiver too.  IMO,
this class (and its impl, and IPC messages, and so on) should probably be
renamed to just CastTransport to make it clearer that it's just a way of moving
things from one endpoint to the other.  That's probably something you'll want to
do in a separate change.

[xjz, 2015-12-15 18:40:13]

Thanks, Irfan and Yuri! PTAL. Changed the interface as suggested. Add Impl.
Still no tests yet.

https://codereview.chromium.org/1515023002/diff/1/media/cast/net/cast_transpo...
File media/cast/net/cast_transport_sender.h (left):

https://codereview.chromium.org/1515023002/diff/1/media/cast/net/cast_transpo...
media/cast/net/cast_transport_sender.h:66: const BulkRawEventsCallback&
raw_events_callback,
On 2015/12/11 17:33:25, Irfan wrote:
> my feeling here is that only the callbacks should be combined as an interface
> (status, packet reception, raw event logs). Combining everything makes it
harder
> to read.

Yuri suggested this too. Changed.

https://codereview.chromium.org/1515023002/diff/1/media/cast/net/cast_transpo...
File media/cast/net/cast_transport_sender.h (right):

https://codereview.chromium.org/1515023002/diff/1/media/cast/net/cast_transpo...
media/cast/net/cast_transport_sender.h:79: base::TickClock* clock,
On 2015/12/12 01:30:45, miu wrote:
> On 2015/12/11 18:56:04, Irfan wrote:
> > Perhaps the UDP transport fields can be combined (local, remote, netlog &
> > transport options) separately.
> > 
> > Combining unrelated fields shifts the readability issue IMO
> 
> Agreed.  Thinking about this, a structure I'd propose is:
> 
> class CastTransportSender {
>  public:
>   class Client {
>    public:
>     virtual void OnStatusChange(CastTransportStatus status) = 0;
>     virtual void OnReceivedLoggingEvents(scoped_ptr<vector<FrameEvent>>,
>                                          scoped_ptr<vector<PacketEvent>>) = 0;
>     virtual bool OnReceviedPacket(scoped_ptr<Packet> packet) = 0;
>    protected:
>     virtual ~Client();
>   };
> 
>   struct CreateParams {
>     CreateParams();
> 
>     net::NetLog* net_log;
>     base::TickClock* clock;
>     net::IPEndPoint local_end_point;
>     net::IPEndPoint remote_end_point;
>     Client* client;
>     base::TimeDelta logging_flush_interval;
>     base::DictionaryValue optional_config;
>   };
> 
>   static scoped_ptr<CastTransportSender> Create(const CreateParams& params);
> 
>   ...
> };
> 
> The "CreateParams inner struct" pattern is used in a lot of other places in
> Chromium where tons of arguments are passed-in to construct an instance. 
> Example:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

Thanks, Yuri! I'll take your suggestion.

> 
> Some of the weirdness here is around CastTransportSender being used for the
cast
> receiver as well.  So, this class is really a CastTransportReceiver too.  IMO,
> this class (and its impl, and IPC messages, and so on) should probably be
> renamed to just CastTransport to make it clearer that it's just a way of
moving
> things from one endpoint to the other.  That's probably something you'll want
to
> do in a separate change.

Will do that after.

[Irfan, 2015-12-15 22:23:13]

Think the commit message needs an update.

Perhaps add someone like wez@ if you need a quicker turn-around with miu@ being
away.

https://codereview.chromium.org/1515023002/diff/20001/chrome/browser/media/ca...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://codereview.chromium.org/1515023002/diff/20001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.cc:128: TransportClient
client(channel_id, this);
I think we will want to make CastTransportHostFilter itself a a Client by
extending CastTransportSender::Client and having callbacks like previously
(instead of having separate class) and then pass "this" instead of &client.

https://codereview.chromium.org/1515023002/diff/20001/media/cast/net/cast_tra...
File media/cast/net/cast_transport_sender.h (right):

https://codereview.chromium.org/1515023002/diff/20001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender.h:59: // Interface to create a cast
transport sender / receiver.
Perhaps the name of the class should be CastTransportEndPoint

https://codereview.chromium.org/1515023002/diff/20001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender.h:62: virtual void
OnStatusChange(CastTransportStatus status) = 0;
Comment suggestion: "Audio and video transport status change is reported on this
callback."

https://codereview.chromium.org/1515023002/diff/20001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender.h:63: // Raw events call back. This will be
involked every
s/involked/invoked

Suggestion: "Raw events will be invoked on this callback every
|logging_flush_interval|. If the user is not interested in raw events,
|logging_flush_interval| should be set to |base::TimeDelta|."

https://codereview.chromium.org/1515023002/diff/20001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender.h:70: // Return false if user is provided a
no-op function.
It appears the return status is only used for logging "stale packet received" in
the code and that log detail itself appears to be incorrect ?

Think this should be void

https://codereview.chromium.org/1515023002/diff/20001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender.h:71: virtual bool
OnReceivedPackets(scoped_ptr<Packet> packet) = 0;
There seems to be a comment in the old code related to this somewhere later:

 // Called with incoming packets. (Unless they match the	
 // channels created by Initialize{Audio,Video}.

Not sure I follow the unless part - has that been behavior been captured ?

https://codereview.chromium.org/1515023002/diff/20001/media/cast/net/udp_tran...
File media/cast/net/udp_transport.cc (right):

https://codereview.chromium.org/1515023002/diff/20001/media/cast/net/udp_tran...
media/cast/net/udp_transport.cc:38: UdpTransport::UdpTransport(const
CastTransportSender::CreateParams& params,
Do not think CastTransportSender::CreateParams should be used here, it is
leaking additional information meant only for CastTransportSender.

Perhaps leave this as is ?

[xjz, 2015-12-16 00:29:14]

Patchset #3 (id:40001) has been deleted

[xjz, 2015-12-16 18:04:31]

Patchset #3 (id:60001) has been deleted

[xjz, 2015-12-16 18:11:34]

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

[xjz, 2015-12-16 18:11:35]

PTAL. Still NO tests yet.

https://codereview.chromium.org/1515023002/diff/20001/chrome/browser/media/ca...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://codereview.chromium.org/1515023002/diff/20001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.cc:128: TransportClient
client(channel_id, this);
On 2015/12/15 22:23:13, Irfan wrote:
> I think we will want to make CastTransportHostFilter itself a a Client by
> extending CastTransportSender::Client and having callbacks like previously
> (instead of having separate class) and then pass "this" instead of &client.
> 

Done.

https://codereview.chromium.org/1515023002/diff/20001/media/cast/net/cast_tra...
File media/cast/net/cast_transport_sender.h (right):

https://codereview.chromium.org/1515023002/diff/20001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender.h:59: // Interface to create a cast
transport sender / receiver.
On 2015/12/15 22:23:13, Irfan wrote:
> Perhaps the name of the class should be CastTransportEndPoint

Rename it to CastTransportClient.

https://codereview.chromium.org/1515023002/diff/20001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender.h:62: virtual void
OnStatusChange(CastTransportStatus status) = 0;
On 2015/12/15 22:23:13, Irfan wrote:
> Comment suggestion: "Audio and video transport status change is reported on
this
> callback."

Done.

https://codereview.chromium.org/1515023002/diff/20001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender.h:63: // Raw events call back. This will be
involked every
On 2015/12/15 22:23:13, Irfan wrote:
> s/involked/invoked
> 
> Suggestion: "Raw events will be invoked on this callback every
> |logging_flush_interval|. If the user is not interested in raw events,
> |logging_flush_interval| should be set to |base::TimeDelta|."

Done.

https://codereview.chromium.org/1515023002/diff/20001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender.h:70: // Return false if user is provided a
no-op function.
On 2015/12/15 22:23:13, Irfan wrote:
> It appears the return status is only used for logging "stale packet received"
in
> the code and that log detail itself appears to be incorrect ?
> 
> Think this should be void

Done.

https://codereview.chromium.org/1515023002/diff/20001/media/cast/net/cast_tra...
media/cast/net/cast_transport_sender.h:71: virtual bool
OnReceivedPackets(scoped_ptr<Packet> packet) = 0;
On 2015/12/15 22:23:13, Irfan wrote:
> There seems to be a comment in the old code related to this somewhere later:
> 
>  // Called with incoming packets. (Unless they match the	
>  // channels created by Initialize{Audio,Video}.
> 
> Not sure I follow the unless part - has that been behavior been captured ?

This would only happen on errors.

https://codereview.chromium.org/1515023002/diff/20001/media/cast/net/udp_tran...
File media/cast/net/udp_transport.cc (right):

https://codereview.chromium.org/1515023002/diff/20001/media/cast/net/udp_tran...
media/cast/net/udp_transport.cc:38: UdpTransport::UdpTransport(const
CastTransportSender::CreateParams& params,
On 2015/12/15 22:23:13, Irfan wrote:
> Do not think CastTransportSender::CreateParams should be used here, it is
> leaking additional information meant only for CastTransportSender.
> 
> Perhaps leave this as is ?

Done. Left |UdpTransport| as is.

[Irfan, 2015-12-16 19:44:34]

https://codereview.chromium.org/1515023002/diff/80001/chrome/browser/media/ca...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://codereview.chromium.org/1515023002/diff/80001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.cc:102: channel_id_ =
channel_id;
as we discussed, this creates a problem since we want to be able to manage
several channel ids in CastTransportHostFilter.

Two options:

- Push channel id to CastTransportSender and let it manage it. I agree with you
that channel id is an abstraction that CastTransportSender should probably not
know about.

- Go back to your approach of abstracting channel id in a separate class of its
own and pass ownership to CastTransportSender to manage the life time

https://codereview.chromium.org/1515023002/diff/80001/chrome/browser/media/ca...
File chrome/browser/media/cast_transport_host_filter.h (right):

https://codereview.chromium.org/1515023002/diff/80001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.h:40: // Override
CastTransportSender::CastTransportClient.
ha, my suggestion of "CastTransportEndPoint" was for CastTransportSender.

I am ok if you want to retain CastTransportClient here

[xjz, 2015-12-16 21:58:01]

PTAL. Still NO tests.

https://codereview.chromium.org/1515023002/diff/80001/chrome/browser/media/ca...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://codereview.chromium.org/1515023002/diff/80001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.cc:102: channel_id_ =
channel_id;
On 2015/12/16 19:44:34, Irfan wrote:
> as we discussed, this creates a problem since we want to be able to manage
> several channel ids in CastTransportHostFilter.
> 
> Two options:
> 
> - Push channel id to CastTransportSender and let it manage it. I agree with
you
> that channel id is an abstraction that CastTransportSender should probably not
> know about.
> 
> - Go back to your approach of abstracting channel id in a separate class of
its
> own and pass ownership to CastTransportSender to manage the life time

I prefer the second approach, unless there are reasons to let the sender manager
the channel id. And it is more flexible to have its own class. For example, the
client might want to add some other parameters in the future.

https://codereview.chromium.org/1515023002/diff/80001/chrome/browser/media/ca...
File chrome/browser/media/cast_transport_host_filter.h (right):

https://codereview.chromium.org/1515023002/diff/80001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.h:40: // Override
CastTransportSender::CastTransportClient.
On 2015/12/16 19:44:34, Irfan wrote:
> ha, my suggestion of "CastTransportEndPoint" was for CastTransportSender.
> 
> I am ok if you want to retain CastTransportClient here

The rename of "CastTransportSender" might in a later CL as Yuri suggested. I am
OK with either "Client" or "CastTransportClient". Rename it back to "Client" to
make it simpler for now :).

[Irfan, 2015-12-16 22:37:04]

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

https://codereview.chromium.org/1515023002/diff/100001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:23:
base::WeakPtr<cast::CastTransportHostFilter> cast_transport_host_filter)
If you use WeakPtr, I think you will want to validate the pointer is valid
before using in the get() calls below.

OTOH, a raw pointer seems sufficient given that CastTransportHostFilter is
always expected to be valid during the life time of a TransportClient.

https://codereview.chromium.org/1515023002/diff/100001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:126: new
TransportClient(channel_id, weak_factory_.GetWeakPtr()));
I think what we want to do is pass a raw pointer to CastTransportSender after
allocating on heap and have scoped_ptr be only created in CastTransportSender to
own the lifetime of that pointer.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender_impl.h (right):

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:128: const CreateParams&
transport_params_;
If we only pass raw pointer on client, we can get rid of these and track a
scoped_ptr on the Client

[imcheng, 2015-12-17 00:18:11]

Overall looks fine. I agree with Irfan's comments. Also have some comments about
CreateParams struct and what goes in there.

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

https://codereview.chromium.org/1515023002/diff/100001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:23:
base::WeakPtr<cast::CastTransportHostFilter> cast_transport_host_filter)
On 2015/12/16 22:37:04, Irfan wrote:
> If you use WeakPtr, I think you will want to validate the pointer is valid
> before using in the get() calls below.
> 
> OTOH, a raw pointer seems sufficient given that CastTransportHostFilter is
> always expected to be valid during the life time of a TransportClient.

+1 on raw pointer. Looks like this class is ultimately owned by
CastTransportHostFilter.

https://codereview.chromium.org/1515023002/diff/100001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:27: void
OnStatusChange(media::cast::CastTransportStatus status) final;
nit on naming: I would probably name these OnStatusChanged,
OnLoggingEventsReceived, OnPacketsReceived.

https://codereview.chromium.org/1515023002/diff/100001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:34: int32 channel_id_;
const int32 channel_id_;

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender.h (right):

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:59: // Interface to create a cast
transport sender / receiver.
I don't think this comment is not accurate. If I understood correctly, this
interface is used for receiving status updates and/or out-of-band data (is this
accurate?) from CastTransportSender.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:65: // Raw events will be invoked on this
callback every
This comment should probably go in CreateParams. Also looks like the code checks
if it is greater than base::TimeDelta() rather than equals?

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:67: // |logging_flush_interval| should be
set to |base::TimeDelta|.
s/base::TimeDelta/base::TimeDelta()

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:69: scoped_ptr<std::vector<FrameEvent>>,
Add names for the input args.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:76: virtual ~Client();
nit: Place the destructor before the other pure virtual methods.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:99: CreateParams(
I feel some of these arguments do not belong in this struct. To me, it would
make more sense to have only "pure data" in this class - such as |local_addr|,
|remote_addr|, |transport_logging_flush_interval|, and |options|. The others are
pointers to other data structures that are probably fine to pass into
media::cast::CastTransportSender::Create().

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:116: scoped_ptr<base::DictionaryValue>
optional_config;
If this is optional, can it be set via a setter() so there's one fewer input
argument in the constructor? It's up to you, because I do see there are several
places downstream where it is assumed to be non-null, and adding null checks may
not end up worth the trouble.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:117: const
scoped_refptr<base::SingleThreadTaskRunner>& transport_task_runner;
Did you mean scoped_refptr<base::SingleThreadTaskRunner> transport_task_runner;
? Holding a reference in a member variable is generally a bad idea...

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender_impl.cc (right):

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.cc:74:
CastTransportSender::Client::~Client() {}
Can this be inlined into cast_transport_sender.h?

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.cc:365:
DCHECK(transport_params_.client);
If this DCHECK and other needed? If client is null, then the code will segfault
anyways. Since we own the code that sets |client|, we shouldn't need the DCHECK.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender_impl.h (right):

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:108: // If |logging_flush_interval|
is not |base::TimeDelta|, calls it with events
s/base::TimeDelta|base::TimeDelta()

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:128: const CreateParams&
transport_params_;
On 2015/12/16 22:37:04, Irfan wrote:
> If we only pass raw pointer on client, we can get rid of these and track a
> scoped_ptr on the Client

Also, this shouldn't be a reference.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:130: const
scoped_refptr<base::SingleThreadTaskRunner>& transport_task_runner_;
This shouldn't be a reference.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:131: base::TimeDelta
logging_flush_interval_;
const base::TimeDelta

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:143: scoped_ptr<PacedSender> pacer_;
Why this change?

[xjz, 2015-12-17 22:54:56]

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

[xjz, 2015-12-17 22:54:58]

PTAL. Still NO tests.

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

https://codereview.chromium.org/1515023002/diff/100001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:23:
base::WeakPtr<cast::CastTransportHostFilter> cast_transport_host_filter)
On 2015/12/16 22:37:04, Irfan wrote:
> If you use WeakPtr, I think you will want to validate the pointer is valid
> before using in the get() calls below.
> 
> OTOH, a raw pointer seems sufficient given that CastTransportHostFilter is
> always expected to be valid during the life time of a TransportClient.

Done. Use raw pointer.

https://codereview.chromium.org/1515023002/diff/100001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:27: void
OnStatusChange(media::cast::CastTransportStatus status) final;
On 2015/12/17 00:18:10, imcheng1 wrote:
> nit on naming: I would probably name these OnStatusChanged,
> OnLoggingEventsReceived, OnPacketsReceived.

Done.

https://codereview.chromium.org/1515023002/diff/100001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:34: int32 channel_id_;
On 2015/12/17 00:18:10, imcheng1 wrote:
> const int32 channel_id_;

Done.

https://codereview.chromium.org/1515023002/diff/100001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:126: new
TransportClient(channel_id, weak_factory_.GetWeakPtr()));
On 2015/12/16 22:37:04, Irfan wrote:
> I think what we want to do is pass a raw pointer to CastTransportSender after
> allocating on heap and have scoped_ptr be only created in CastTransportSender
to
> own the lifetime of that pointer.

This is correct but might not be good as commented somewhere else. I prefer pass
it separately as Derek suggested.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender.h (right):

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:59: // Interface to create a cast
transport sender / receiver.
On 2015/12/17 00:18:10, imcheng1 wrote:
> I don't think this comment is not accurate. If I understood correctly, this
> interface is used for receiving status updates and/or out-of-band data (is
this
> accurate?) from CastTransportSender.

Done.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:65: // Raw events will be invoked on this
callback every
On 2015/12/17 00:18:10, imcheng1 wrote:
> This comment should probably go in CreateParams. Also looks like the code
checks
> if it is greater than base::TimeDelta() rather than equals?

Add comments too in CreateParams. Yes, the code checks if it is greater than
|base::TimeDelta()|. Modified the comment.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:67: // |logging_flush_interval| should be
set to |base::TimeDelta|.
On 2015/12/17 00:18:10, imcheng1 wrote:
> s/base::TimeDelta/base::TimeDelta()

Done.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:69: scoped_ptr<std::vector<FrameEvent>>,
On 2015/12/17 00:18:10, imcheng1 wrote:
> Add names for the input args.

Done.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:76: virtual ~Client();
On 2015/12/17 00:18:10, imcheng1 wrote:
> nit: Place the destructor before the other pure virtual methods.

Done.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:99: CreateParams(
On 2015/12/17 00:18:10, imcheng1 wrote:
> I feel some of these arguments do not belong in this struct. To me, it would
> make more sense to have only "pure data" in this class - such as |local_addr|,
> |remote_addr|, |transport_logging_flush_interval|, and |options|. The others
are
> pointers to other data structures that are probably fine to pass into
> media::cast::CastTransportSender::Create().

There are only three pure data. |options| is also a pointer to other data
structure. I moved the |client| pointer out since it is the interface of all
callbacks. For other parameters, I think it is OK to put them inside this
structure since all of them are parameters used to create the cast transport. If
this structure is not preferred, I will prefer leave other parameters as they
are. It dose not sgtm to just group |local_addr|, |group_addr| and
|logging_flush_interval|.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:116: scoped_ptr<base::DictionaryValue>
optional_config;
On 2015/12/17 00:18:10, imcheng1 wrote:
> If this is optional, can it be set via a setter() so there's one fewer input
> argument in the constructor? It's up to you, because I do see there are
several
> places downstream where it is assumed to be non-null, and adding null checks
may
> not end up worth the trouble.

It is only used inside the constructor. I prefer leave it as an input parameter.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:117: const
scoped_refptr<base::SingleThreadTaskRunner>& transport_task_runner;
On 2015/12/17 00:18:10, imcheng1 wrote:
> Did you mean scoped_refptr<base::SingleThreadTaskRunner>
transport_task_runner;
> ? Holding a reference in a member variable is generally a bad idea...

Done.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender_impl.cc (right):

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.cc:74:
CastTransportSender::Client::~Client() {}
On 2015/12/17 00:18:11, imcheng1 wrote:
> Can this be inlined into cast_transport_sender.h?

Done.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.cc:365:
DCHECK(transport_params_.client);
On 2015/12/17 00:18:11, imcheng1 wrote:
> If this DCHECK and other needed? If client is null, then the code will
segfault
> anyways. Since we own the code that sets |client|, we shouldn't need the
DCHECK.

Done. Removed DCHECKs.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender_impl.h (right):

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:108: // If |logging_flush_interval|
is not |base::TimeDelta|, calls it with events
On 2015/12/17 00:18:11, imcheng1 wrote:
> s/base::TimeDelta|base::TimeDelta()

Done.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:128: const CreateParams&
transport_params_;
On 2015/12/17 00:18:11, imcheng1 wrote:
> On 2015/12/16 22:37:04, Irfan wrote:
> > If we only pass raw pointer on client, we can get rid of these and track a
> > scoped_ptr on the Client
> 
> Also, this shouldn't be a reference.

Removed.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:128: const CreateParams&
transport_params_;
On 2015/12/16 22:37:04, Irfan wrote:
> If we only pass raw pointer on client, we can get rid of these and track a
> scoped_ptr on the Client

I think we could do this, but might not be a good way. If for some reson this
pointer is not successfully parsed to the scoped_ptr, this pointer will not be
deleted. I prefer take it out of the CreateParams structure and pass it
separately as Derek suggested.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:130: const
scoped_refptr<base::SingleThreadTaskRunner>& transport_task_runner_;
On 2015/12/17 00:18:11, imcheng1 wrote:
> This shouldn't be a reference.

Done.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:131: base::TimeDelta
logging_flush_interval_;
On 2015/12/17 00:18:11, imcheng1 wrote:
> const base::TimeDelta

Done.

https://codereview.chromium.org/1515023002/diff/100001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:143: scoped_ptr<PacedSender> pacer_;
On 2015/12/17 00:18:11, imcheng1 wrote:
> Why this change?

It cannot be initialized in the initialization list after the interface change.
So I changed it to scoped_ptr to initialize it inside the constructor instead.

[Wez, 2015-12-18 00:31:19]

https://codereview.chromium.org/1515023002/diff/120001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender.h (right):

https://codereview.chromium.org/1515023002/diff/120001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:105: const
scoped_refptr<base::SingleThreadTaskRunner>& task_runner);
As discussed offline, having a CreateParams struct reduces the number of
parameters to Create() but really just moves the complexity somewhere else.

Alternative patterns you might consider:
1. Just accept that Create() takes a huge number of parameters. :)
2. Pass in interfaces and passed/owned objects directly to Create, with a
smaller struct for the data-only parameters, as Derek suggested.
3. Create() with a minimal set of parameters, or no parameters and have the
caller supply additional parameters via setters on the resulting
CastTransportSender. This has the advantage that you can group parameters into
more logical groups, potentially, but means more boilerplate setter code.

#3 works especially well if there are meaningful defaults for particular
parameters - so you Create() with the required ones and then optionally set the
remaining ones, or run with the defaults.

Given the way that these parameters break down I'd personally move back to
Create(lots-of-params) with a separate set_transport_options() setter on
CastTransportSender, I think.

nit: Please document the object-lifetime requirements of |net_log| and |clock| -
it looks like the implementation assumes that the caller keeps those alive until
the sender has been destroyed?

[xjz, 2016-01-12 22:17:40]

PTAL. Still NO unit tests code changes in.

https://codereview.chromium.org/1515023002/diff/120001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender.h (right):

https://codereview.chromium.org/1515023002/diff/120001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:105: const
scoped_refptr<base::SingleThreadTaskRunner>& task_runner);
On 2015/12/18 00:31:19, Wez wrote:
> As discussed offline, having a CreateParams struct reduces the number of
> parameters to Create() but really just moves the complexity somewhere else.
> 
> Alternative patterns you might consider:
> 1. Just accept that Create() takes a huge number of parameters. :)
> 2. Pass in interfaces and passed/owned objects directly to Create, with a
> smaller struct for the data-only parameters, as Derek suggested.
> 3. Create() with a minimal set of parameters, or no parameters and have the
> caller supply additional parameters via setters on the resulting
> CastTransportSender. This has the advantage that you can group parameters into
> more logical groups, potentially, but means more boilerplate setter code.
> 
> #3 works especially well if there are meaningful defaults for particular
> parameters - so you Create() with the required ones and then optionally set
the
> remaining ones, or run with the defaults.
> 
> Given the way that these parameters break down I'd personally move back to
> Create(lots-of-params) with a separate set_transport_options() setter on
> CastTransportSender, I think.
> 
> nit: Please document the object-lifetime requirements of |net_log| and |clock|
-
> it looks like the implementation assumes that the caller keeps those alive
until
> the sender has been destroyed?

Yes, |net_log| and |clock| are owned by the caller. Added comments.

After considering all the comments, I made the following changes to the
interface:
1. Still use the interface instead of callbacks.
2. Group |net_log|, |local_end_point| and |remote_end_point| together, as they
are only used to create udp transport.
3. Leave other parameters as they are.

[miu, 2016-01-16 02:28:42]

I like this code structure much better.  The UDPTrasport params are now grouped
nicely, and the callbacks replaced with a simple 3-method interface.

Comments:

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

https://codereview.chromium.org/1515023002/diff/140001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:34:
cast::CastTransportHostFilter* cast_transport_host_filter_;
nit (add const): cast::CastTransportHostFilter* const
cast_transport_host_filter_;

https://codereview.chromium.org/1515023002/diff/140001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender.h (right):

https://codereview.chromium.org/1515023002/diff/140001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:60: // Interface used for receiving
status updates, raw events, and out-of-band
s/out-of-band incoming/RTP/

See comment below...

https://codereview.chromium.org/1515023002/diff/140001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:69: // |logging_flush_interval| if it is
greater than |base::TimeDelta()|.
Suggest replacing "every |logging_flush_interval| if it is greater than
|base::TimeDelta()|" with "periodically, according to the configured logging
flush interval passed to CastTransportSender::Create()."

https://codereview.chromium.org/1515023002/diff/140001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:76: virtual void
OnPacketsReceived(scoped_ptr<Packet> packet) = 0;
This is how an RTP packet is delivered.  So, this method should probably be
named HandleRtpPacket() or ProcessRtpPacket().

Also, please adjust the comment accordingly (i.e., just say "Called to pass RTP
packets to the Client" or some such).

FYI--This method only applies for a Cast Receiver.  Unfortunately, the
CastTransportSender class is used for the receiver code too, so it is horribly
mis-named.  http://crbug.com/569259

https://codereview.chromium.org/1515023002/diff/140001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender_impl.h (right):

https://codereview.chromium.org/1515023002/diff/140001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:117: // If |logging_flush_interval|
is not |base::TimeDelta|, calls it with events
nit: It would be sufficient to just say, "If |logging_flush_interval_| is set,
this is called at approximate periodic intervals."

https://codereview.chromium.org/1515023002/diff/140001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:134: // Used as status callback to
create udp transport.
s/udp transport/UdpTransport/

https://codereview.chromium.org/1515023002/diff/140001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:137: scoped_ptr<Client>
transport_client_;
Please preserve ordering (same order here as args).  So, |clock_| should go
before |trasport_client_|.

Also, please add const: const scoped_ptr<Client> transport_client_;

https://codereview.chromium.org/1515023002/diff/140001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:138: base::TickClock* clock_;  //
Not owned by this class.
Please add const here too: base::TickClock* const clock;

https://codereview.chromium.org/1515023002/diff/140001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:152: scoped_ptr<PacedSender> pacer_;
I don't see why this was changed.  Unless I'm missing something, please revert
to avoid the extra indirection.

[xjz, 2016-01-21 19:06:51]

Patchset #7 (id:160001) has been deleted

[xjz, 2016-01-21 19:16:28]

miu@: PTAL on this new version of the interface (still no change on test codes).
Main changes:
1. Move the create of UdpTransport out. 
2. Use PacketSender as the argument. It could be either UdpTransport or external
transport.
3. Use three setters to set options for UdpTransport, PacedSender, and Wifi
separately.

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

https://codereview.chromium.org/1515023002/diff/140001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:34:
cast::CastTransportHostFilter* cast_transport_host_filter_;
On 2016/01/16 02:28:42, miu wrote:
> nit (add const): cast::CastTransportHostFilter* const
> cast_transport_host_filter_;

Done.

https://codereview.chromium.org/1515023002/diff/140001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender.h (right):

https://codereview.chromium.org/1515023002/diff/140001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:60: // Interface used for receiving
status updates, raw events, and out-of-band
On 2016/01/16 02:28:42, miu wrote:
> s/out-of-band incoming/RTP/
> 
> See comment below...

Done.

https://codereview.chromium.org/1515023002/diff/140001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:69: // |logging_flush_interval| if it is
greater than |base::TimeDelta()|.
On 2016/01/16 02:28:42, miu wrote:
> Suggest replacing "every |logging_flush_interval| if it is greater than
> |base::TimeDelta()|" with "periodically, according to the configured logging
> flush interval passed to CastTransportSender::Create()."

Done.

https://codereview.chromium.org/1515023002/diff/140001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:76: virtual void
OnPacketsReceived(scoped_ptr<Packet> packet) = 0;
On 2016/01/16 02:28:42, miu wrote:
> This is how an RTP packet is delivered.  So, this method should probably be
> named HandleRtpPacket() or ProcessRtpPacket().
> 
> Also, please adjust the comment accordingly (i.e., just say "Called to pass
RTP
> packets to the Client" or some such).
> 
> FYI--This method only applies for a Cast Receiver.  Unfortunately, the
> CastTransportSender class is used for the receiver code too, so it is horribly
> mis-named.  http://crbug.com/569259

Done. Named it ProcessRtpPacket().

https://codereview.chromium.org/1515023002/diff/140001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender_impl.h (right):

https://codereview.chromium.org/1515023002/diff/140001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:117: // If |logging_flush_interval|
is not |base::TimeDelta|, calls it with events
On 2016/01/16 02:28:42, miu wrote:
> nit: It would be sufficient to just say, "If |logging_flush_interval_| is set,
> this is called at approximate periodic intervals."

Done.

https://codereview.chromium.org/1515023002/diff/140001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:134: // Used as status callback to
create udp transport.
On 2016/01/16 02:28:42, miu wrote:
> s/udp transport/UdpTransport/

Done.

https://codereview.chromium.org/1515023002/diff/140001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:137: scoped_ptr<Client>
transport_client_;
On 2016/01/16 02:28:42, miu wrote:
> Please preserve ordering (same order here as args).  So, |clock_| should go
> before |trasport_client_|.
> 
> Also, please add const: const scoped_ptr<Client> transport_client_;

Done.

https://codereview.chromium.org/1515023002/diff/140001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:138: base::TickClock* clock_;  //
Not owned by this class.
On 2016/01/16 02:28:42, miu wrote:
> Please add const here too: base::TickClock* const clock;

Done.

https://codereview.chromium.org/1515023002/diff/140001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:152: scoped_ptr<PacedSender> pacer_;
On 2016/01/16 02:28:42, miu wrote:
> I don't see why this was changed.  Unless I'm missing something, please revert
> to avoid the extra indirection.

This was changed because this can only be initialized after initializing
|transport_| and has to be inside the constructor. It is now reverted since in
the new version the |transport_| is created outside.

[miu, 2016-01-27 02:18:04]

This looks absolutely great!  The code factoring and structure is much cleaner,
with functionality properly encapsulated.  :)

Good to go for updating the unit tests.  I just have these nit comments for
Patch Set 7:

https://codereview.chromium.org/1515023002/diff/180001/chrome/renderer/media/...
File chrome/renderer/media/cast_transport_sender_ipc.cc (right):

https://codereview.chromium.org/1515023002/diff/180001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:206: return nullptr;
Please add a NOTREACHED() statement here with a comment that "access to the
PacedSender is only needed browser-side, for setting networking options."

https://codereview.chromium.org/1515023002/diff/180001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender_impl.cc (right):

https://codereview.chromium.org/1515023002/diff/180001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.cc:62: DCHECK(clock);
Since the whole class assumes |transport_client_|, |transport_| and
|transport_task_runner_| are never null, please add null-checking DCHECKs here.

https://codereview.chromium.org/1515023002/diff/180001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender_impl.h (right):

https://codereview.chromium.org/1515023002/diff/180001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:173:
scoped_ptr<net::ScopedWifiOptions> wifi_options_autoreset_;
Please add a comment like: "While non-null, global WiFi behavior modifications
are in effect.  This is used, for example, to turn off WiFi scanning that tends
to interfere with the reliability of UDP packet transmission."

[xjz, 2016-01-28 18:04:16]

Patchset #8 (id:200001) has been deleted

[xjz, 2016-01-29 00:01:55]

Patchset #8 (id:220001) has been deleted

[xjz, 2016-01-29 20:21:11]

Patchset #8 (id:240001) has been deleted

[xjz, 2016-01-29 22:09:23]

Patchset #8 (id:260001) has been deleted

[xjz, 2016-01-30 01:19:26]

PTAL. Add changes on tests.

https://codereview.chromium.org/1515023002/diff/180001/chrome/renderer/media/...
File chrome/renderer/media/cast_transport_sender_ipc.cc (right):

https://codereview.chromium.org/1515023002/diff/180001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:206: return nullptr;
On 2016/01/27 02:18:04, miu wrote:
> Please add a NOTREACHED() statement here with a comment that "access to the
> PacedSender is only needed browser-side, for setting networking options."

Removed this function. Set the options directly through CastTransportSender
instead.

https://codereview.chromium.org/1515023002/diff/180001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender_impl.cc (right):

https://codereview.chromium.org/1515023002/diff/180001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.cc:62: DCHECK(clock);
On 2016/01/27 02:18:04, miu wrote:
> Since the whole class assumes |transport_client_|, |transport_| and
> |transport_task_runner_| are never null, please add null-checking DCHECKs
here.

Done.

https://codereview.chromium.org/1515023002/diff/180001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender_impl.h (right):

https://codereview.chromium.org/1515023002/diff/180001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:173:
scoped_ptr<net::ScopedWifiOptions> wifi_options_autoreset_;
On 2016/01/27 02:18:04, miu wrote:
> Please add a comment like: "While non-null, global WiFi behavior modifications
> are in effect.  This is used, for example, to turn off WiFi scanning that
tends
> to interfere with the reliability of UDP packet transmission."

Done.

[miu, 2016-02-04 20:23:28]

Patch Set 8 doesn't look like Patch Set 7 + unit tests.  I'm not sure that
you've uploaded the right change here (spot checked a few places that seem
wrong).

[xjz, 2016-02-05 00:04:29]

On 2016/02/04 20:23:28, miu wrote:
> Patch Set 8 doesn't look like Patch Set 7 + unit tests.  I'm not sure that
> you've uploaded the right change here (spot checked a few places that seem
> wrong).

Here are changes from Patch Set 7 other than unit tests:
1. Moved the handling of udp options from CastTransportHostFilter to
UdpTransport.
2. Combined and moved the handling of paced sender options and wifi options 
from CastTransportHostFilter to CastTransportSenderImpl.
3. Removed GetPacedSender() since the option is now handled inside
CastTransportSenderImpl.

These changes were made to avoid duplicate codes handling options when called 
CastTransportSender with options.

[miu, 2016-02-22 22:36:23]

Description was changed from

==========
Simlify interface for media/cast: CastTransportSenderImpl

Defined an interface to reduce the 11-arg constructor down to 4 args.

BUG=557477
==========

to

==========
Simplify interface for media/cast: CastTransportSenderImpl

Defined an interface to reduce the 11-arg constructor down to 4 args.

BUG=557477
==========

[miu, 2016-02-22 22:38:48]

Looking good.  Mostly little things:

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_config.h (right):

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/cast_tr...
media/cast/net/cast_transport_config.h:141: virtual void StartReceiving(
1. These methods don't belong in a "PacketSender" interface.  Looking at what
they're here for, perhaps this class should be renamed to something like, say,
PacketTransporter?

2. Please add comments for these methods.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender.h (right):

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:66: // Audio and Video transport status
change is reported on this callback.
nit: Add newline above this line.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:142: // Possible keys are:
nit: I'd suggest moving lines 142-153 to cast_transport_sender_impl.h (where the
override of the SetOptions() method is declared) since this information is
specific to that implementation.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender_impl_unittest.cc (right):

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl_unittest.cc:29: 
Please remove this extra newline; or else add another one to above line 26 to
even-out the spacing.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl_unittest.cc:121:
TransportClient(CastTransportSenderImplTest* cast_transport_sender_impl_test)
Need 'explicit' keyword here.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl_unittest.cc:128: if
(cast_transport_sender_impl_test_)
How about:

  CHECK(cast_transport_sender_impl_test_);

...since this method shouldn't be called if the logging_flush_interval is zero.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/udp_tra...
File media/cast/net/udp_transport.cc (right):

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/udp_tra...
media/cast/net/udp_transport.cc:24: const int kMaxPacketSize = 1500;
Let's delete this, since it's identical to kMaxIpPacketSize.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/udp_tra...
media/cast/net/udp_transport.cc:61: 
nit: Need extra newline above line 24 as well.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/test/cast_b...
File media/cast/test/cast_benchmarks.cc (right):

https://codereview.chromium.org/1515023002/diff/280001/media/cast/test/cast_b...
media/cast/test/cast_benchmarks.cc:442: TransportClient(RunOneBenchmark*
run_one_benchmark)
Need 'explicit' keyword here.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/test/sender.cc
File media/cast/test/sender.cc (right):

https://codereview.chromium.org/1515023002/diff/280001/media/cast/test/sender...
media/cast/test/sender.cc:178: void
ProcessRtpPacket(scoped_ptr<media::cast::Packet> packet) final{};
Please run `git cl format` to fix these kinds of whitespace issues.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/test/utilit...
File media/cast/test/utility/in_process_receiver.cc (right):

https://codereview.chromium.org/1515023002/diff/280001/media/cast/test/utilit...
media/cast/test/utility/in_process_receiver.cc:29:
TransportClient(InProcessReceiver* in_process_receiver)
Need 'explicit' keyword here.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/test/utilit...
media/cast/test/utility/in_process_receiver.cc:114: NULL,
cast_environment_->GetTaskRunner(CastEnvironment::MAIN),
nit: Please change NULLs to nullptr whenever you change a line of code.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/test/utilit...
File media/cast/test/utility/in_process_receiver.h (right):

https://codereview.chromium.org/1515023002/diff/280001/media/cast/test/utilit...
media/cast/test/utility/in_process_receiver.h:67: void
ReceivePacket(scoped_ptr<Packet> packet);
Don't make this method public.  Instead, make the TransportClient class an inner
class of InProcessReceiver so that it is allowed to call the private method.

[xjz, 2016-02-23 19:26:40]

Patchset #9 (id:300001) has been deleted

[xjz, 2016-02-23 21:47:52]

Patchset #9 (id:320001) has been deleted

[xjz, 2016-02-23 21:51:47]

PTAL

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_config.h (right):

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/cast_tr...
media/cast/net/cast_transport_config.h:141: virtual void StartReceiving(
On 2016/02/22 22:38:48, miu wrote:
> 1. These methods don't belong in a "PacketSender" interface.  Looking at what
> they're here for, perhaps this class should be renamed to something like, say,
> PacketTransporter?

Agree. Will rename it in a following CL together with the renaming of
CastTransportSender. Added a TODO comment here for now.

> 
> 2. Please add comments for these methods.

Done.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender.h (right):

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:66: // Audio and Video transport status
change is reported on this callback.
On 2016/02/22 22:38:48, miu wrote:
> nit: Add newline above this line.

Done.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender.h:142: // Possible keys are:
On 2016/02/22 22:38:48, miu wrote:
> nit: I'd suggest moving lines 142-153 to cast_transport_sender_impl.h (where
the
> override of the SetOptions() method is declared) since this information is
> specific to that implementation.

Done.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender_impl_unittest.cc (right):

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl_unittest.cc:29: 
On 2016/02/22 22:38:48, miu wrote:
> Please remove this extra newline; or else add another one to above line 26 to
> even-out the spacing.

Done.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl_unittest.cc:121:
TransportClient(CastTransportSenderImplTest* cast_transport_sender_impl_test)
On 2016/02/22 22:38:48, miu wrote:
> Need 'explicit' keyword here.

Done.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl_unittest.cc:128: if
(cast_transport_sender_impl_test_)
On 2016/02/22 22:38:48, miu wrote:
> How about:
> 
>   CHECK(cast_transport_sender_impl_test_);
> 
> ...since this method shouldn't be called if the logging_flush_interval is
zero.

Done.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/udp_tra...
File media/cast/net/udp_transport.cc (right):

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/udp_tra...
media/cast/net/udp_transport.cc:24: const int kMaxPacketSize = 1500;
On 2016/02/22 22:38:48, miu wrote:
> Let's delete this, since it's identical to kMaxIpPacketSize.

Done.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/net/udp_tra...
media/cast/net/udp_transport.cc:61: 
On 2016/02/22 22:38:48, miu wrote:
> nit: Need extra newline above line 24 as well.

Done.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/test/cast_b...
File media/cast/test/cast_benchmarks.cc (right):

https://codereview.chromium.org/1515023002/diff/280001/media/cast/test/cast_b...
media/cast/test/cast_benchmarks.cc:442: TransportClient(RunOneBenchmark*
run_one_benchmark)
On 2016/02/22 22:38:48, miu wrote:
> Need 'explicit' keyword here.

Done.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/test/sender.cc
File media/cast/test/sender.cc (right):

https://codereview.chromium.org/1515023002/diff/280001/media/cast/test/sender...
media/cast/test/sender.cc:178: void
ProcessRtpPacket(scoped_ptr<media::cast::Packet> packet) final{};
On 2016/02/22 22:38:48, miu wrote:
> Please run `git cl format` to fix these kinds of whitespace issues.

Done.
This is what "git cl format" results with the ending ";"... Should have removed
that.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/test/utilit...
File media/cast/test/utility/in_process_receiver.cc (right):

https://codereview.chromium.org/1515023002/diff/280001/media/cast/test/utilit...
media/cast/test/utility/in_process_receiver.cc:29:
TransportClient(InProcessReceiver* in_process_receiver)
On 2016/02/22 22:38:48, miu wrote:
> Need 'explicit' keyword here.
Done.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/test/utilit...
media/cast/test/utility/in_process_receiver.cc:114: NULL,
cast_environment_->GetTaskRunner(CastEnvironment::MAIN),
On 2016/02/22 22:38:48, miu wrote:
> nit: Please change NULLs to nullptr whenever you change a line of code.

Done.

https://codereview.chromium.org/1515023002/diff/280001/media/cast/test/utilit...
File media/cast/test/utility/in_process_receiver.h (right):

https://codereview.chromium.org/1515023002/diff/280001/media/cast/test/utilit...
media/cast/test/utility/in_process_receiver.h:67: void
ReceivePacket(scoped_ptr<Packet> packet);
On 2016/02/22 22:38:48, miu wrote:
> Don't make this method public.  Instead, make the TransportClient class an
inner
> class of InProcessReceiver so that it is allowed to call the private method.

Done.

[miu, 2016-02-23 23:36:00]

lgtm!  Ship it!  :)

[xjz, 2016-02-24 00:02:11]

The CQ bit was checked by xjz@chromium.org

[commit-bot: I haz the power, 2016-02-24 00:05:53]

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

[commit-bot: I haz the power, 2016-02-24 00:20:24]

Description was changed from

==========
Simplify interface for media/cast: CastTransportSenderImpl

Defined an interface to reduce the 11-arg constructor down to 4 args.

BUG=557477
==========

to

==========
Simplify interface for media/cast: CastTransportSenderImpl

Defined an interface to reduce the 11-arg constructor down to 4 args.

BUG=557477
==========

[commit-bot: I haz the power, 2016-02-24 00:20:26]

Committed patchset #9 (id:340001)

[commit-bot: I haz the power, 2016-02-24 00:21:41]

Description was changed from

==========
Simplify interface for media/cast: CastTransportSenderImpl

Defined an interface to reduce the 11-arg constructor down to 4 args.

BUG=557477
==========

to

==========
Simplify interface for media/cast: CastTransportSenderImpl

Defined an interface to reduce the 11-arg constructor down to 4 args.

BUG=557477

Committed: https://crrev.com/74f752173cf8084d9f96073eaa88236f0ac54b79
Cr-Commit-Position: refs/heads/master@{#377148}
==========

[commit-bot: I haz the power, 2016-02-24 00:21:43]

Patchset 9 (id:??) landed as
https://crrev.com/74f752173cf8084d9f96073eaa88236f0ac54b79
Cr-Commit-Position: refs/heads/master@{#377148}