Cast: IPC glue between cast library transport and encoders.

IPC glue between cast library transport and encoders.

This CL lets the cast library run the packetizing and encryption in the browser and the video and audio encoding in the renderer. The encoded data is sent over IPC to the browser.

BUG=301920
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251592

[mikhal1, 2014-01-23 21:04:38]

https://codereview.chromium.org/138753004/diff/30001/chrome/browser/media/cas...
File chrome/browser/media/cast_net_host_filter.cc (right):

https://codereview.chromium.org/138753004/diff/30001/chrome/browser/media/cas...
chrome/browser/media/cast_net_host_filter.cc:3: // found in the LICENSE file.
add new line

https://codereview.chromium.org/138753004/diff/30001/chrome/browser/media/cas...
chrome/browser/media/cast_net_host_filter.cc:30: return *message_was_ok =
handled;
This is initially set to true and never modified. Is that intentional?

https://codereview.chromium.org/138753004/diff/30001/chrome/browser/media/cas...
chrome/browser/media/cast_net_host_filter.cc:104: if (ptr) {
Shouldn't there be an error msg in case you didn't find the channel? Here and in
the rest of the functions.

https://codereview.chromium.org/138753004/diff/30001/chrome/browser/media/cas...
File chrome/browser/media/cast_net_host_filter.h (right):

https://codereview.chromium.org/138753004/diff/30001/chrome/browser/media/cas...
chrome/browser/media/cast_net_host_filter.h:3: // found in the LICENSE file.
add new line

https://codereview.chromium.org/138753004/diff/30001/chrome/browser/media/cas...
chrome/browser/media/cast_net_host_filter.h:36: // BrowserMessageFilter
implementation
missing "." at the end of a comment. Here and below.

https://codereview.chromium.org/138753004/diff/30001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_ipc_helper.cc (right):

https://codereview.chromium.org/138753004/diff/30001/chrome/renderer/media/ca...
chrome/renderer/media/cast_ipc_helper.cc:25: return handled;
Same comment as above regarding handled.

https://codereview.chromium.org/138753004/diff/30001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_ipc_helper.h (right):

https://codereview.chromium.org/138753004/diff/30001/chrome/renderer/media/ca...
chrome/renderer/media/cast_ipc_helper.h:3: // found in the LICENSE file.
new line

https://codereview.chromium.org/138753004/diff/30001/content/common/cast_mess...
File content/common/cast_messages_data.h (right):

https://codereview.chromium.org/138753004/diff/30001/content/common/cast_mess...
content/common/cast_messages_data.h:12: struct SendRtcpFromRtpSenderData {
default constructor/destructor missing

[hubbe, 2014-02-03 22:58:59]

Lots of changes, actually works now.
(Both simple unit tests and mirring to cast_receiver_app)

https://codereview.chromium.org/138753004/diff/30001/chrome/browser/media/cas...
File chrome/browser/media/cast_net_host_filter.cc (right):

https://codereview.chromium.org/138753004/diff/30001/chrome/browser/media/cas...
chrome/browser/media/cast_net_host_filter.cc:3: // found in the LICENSE file.
On 2014/01/23 21:04:38, mikhal1 wrote:
> add new line

Done.

https://codereview.chromium.org/138753004/diff/30001/chrome/browser/media/cas...
chrome/browser/media/cast_net_host_filter.cc:30: return *message_was_ok =
handled;
On 2014/01/23 21:04:38, mikhal1 wrote:
> This is initially set to true and never modified. Is that intentional?

I had seen this in other handlers, but misunderstood how it worked, I think it
should be better now.

https://codereview.chromium.org/138753004/diff/30001/chrome/browser/media/cas...
chrome/browser/media/cast_net_host_filter.cc:104: if (ptr) {
On 2014/01/23 21:04:38, mikhal1 wrote:
> Shouldn't there be an error msg in case you didn't find the channel? Here and
in
> the rest of the functions.

Done.

https://codereview.chromium.org/138753004/diff/30001/chrome/browser/media/cas...
File chrome/browser/media/cast_net_host_filter.h (right):

https://codereview.chromium.org/138753004/diff/30001/chrome/browser/media/cas...
chrome/browser/media/cast_net_host_filter.h:3: // found in the LICENSE file.
On 2014/01/23 21:04:38, mikhal1 wrote:
> add new line

Done.

https://codereview.chromium.org/138753004/diff/30001/chrome/browser/media/cas...
chrome/browser/media/cast_net_host_filter.h:36: // BrowserMessageFilter
implementation
On 2014/01/23 21:04:38, mikhal1 wrote:
> missing "." at the end of a comment. Here and below.

Done.

https://codereview.chromium.org/138753004/diff/30001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_ipc_helper.cc (right):

https://codereview.chromium.org/138753004/diff/30001/chrome/renderer/media/ca...
chrome/renderer/media/cast_ipc_helper.cc:25: return handled;
On 2014/01/23 21:04:38, mikhal1 wrote:
> Same comment as above regarding handled.

Done.

https://codereview.chromium.org/138753004/diff/30001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_ipc_helper.h (right):

https://codereview.chromium.org/138753004/diff/30001/chrome/renderer/media/ca...
chrome/renderer/media/cast_ipc_helper.h:3: // found in the LICENSE file.
On 2014/01/23 21:04:38, mikhal1 wrote:
> new line

Done.

https://codereview.chromium.org/138753004/diff/30001/content/common/cast_mess...
File content/common/cast_messages_data.h (right):

https://codereview.chromium.org/138753004/diff/30001/content/common/cast_mess...
content/common/cast_messages_data.h:12: struct SendRtcpFromRtpSenderData {
On 2014/01/23 21:04:38, mikhal1 wrote:
> default constructor/destructor missing

In that case I should probably put it in cast_transport_config.{h,cc} instead.

[hubbe, 2014-02-05 00:09:24]

On 2014/02/03 22:58:59, hubbe wrote:
> Lots of changes, actually works now.
> (Both simple unit tests and mirring to cast_receiver_app)
> 
>
https://codereview.chromium.org/138753004/diff/30001/chrome/browser/media/cas...
> File chrome/browser/media/cast_net_host_filter.cc (right):
> 
>
https://codereview.chromium.org/138753004/diff/30001/chrome/browser/media/cas...
> chrome/browser/media/cast_net_host_filter.cc:3: // found in the LICENSE file.
> On 2014/01/23 21:04:38, mikhal1 wrote:
> > add new line
> 
> Done.
> 
>
https://codereview.chromium.org/138753004/diff/30001/chrome/browser/media/cas...
> chrome/browser/media/cast_net_host_filter.cc:30: return *message_was_ok =
> handled;
> On 2014/01/23 21:04:38, mikhal1 wrote:
> > This is initially set to true and never modified. Is that intentional?
> 
> I had seen this in other handlers, but misunderstood how it worked, I think it
> should be better now.
> 
>
https://codereview.chromium.org/138753004/diff/30001/chrome/browser/media/cas...
> chrome/browser/media/cast_net_host_filter.cc:104: if (ptr) {
> On 2014/01/23 21:04:38, mikhal1 wrote:
> > Shouldn't there be an error msg in case you didn't find the channel? Here
and
> in
> > the rest of the functions.
> 
> Done.
> 
>
https://codereview.chromium.org/138753004/diff/30001/chrome/browser/media/cas...
> File chrome/browser/media/cast_net_host_filter.h (right):
> 
>
https://codereview.chromium.org/138753004/diff/30001/chrome/browser/media/cas...
> chrome/browser/media/cast_net_host_filter.h:3: // found in the LICENSE file.
> On 2014/01/23 21:04:38, mikhal1 wrote:
> > add new line
> 
> Done.
> 
>
https://codereview.chromium.org/138753004/diff/30001/chrome/browser/media/cas...
> chrome/browser/media/cast_net_host_filter.h:36: // BrowserMessageFilter
> implementation
> On 2014/01/23 21:04:38, mikhal1 wrote:
> > missing "." at the end of a comment. Here and below.
> 
> Done.
> 
>
https://codereview.chromium.org/138753004/diff/30001/chrome/renderer/media/ca...
> File chrome/renderer/media/cast_ipc_helper.cc (right):
> 
>
https://codereview.chromium.org/138753004/diff/30001/chrome/renderer/media/ca...
> chrome/renderer/media/cast_ipc_helper.cc:25: return handled;
> On 2014/01/23 21:04:38, mikhal1 wrote:
> > Same comment as above regarding handled.
> 
> Done.
> 
>
https://codereview.chromium.org/138753004/diff/30001/chrome/renderer/media/ca...
> File chrome/renderer/media/cast_ipc_helper.h (right):
> 
>
https://codereview.chromium.org/138753004/diff/30001/chrome/renderer/media/ca...
> chrome/renderer/media/cast_ipc_helper.h:3: // found in the LICENSE file.
> On 2014/01/23 21:04:38, mikhal1 wrote:
> > new line
> 
> Done.
> 
>
https://codereview.chromium.org/138753004/diff/30001/content/common/cast_mess...
> File content/common/cast_messages_data.h (right):
> 
>
https://codereview.chromium.org/138753004/diff/30001/content/common/cast_mess...
> content/common/cast_messages_data.h:12: struct SendRtcpFromRtpSenderData {
> On 2014/01/23 21:04:38, mikhal1 wrote:
> > default constructor/destructor missing
> 
> In that case I should probably put it in cast_transport_config.{h,cc} instead.

Ping?

[mikhal1, 2014-02-05 00:53:04]

First set of comments - mainly nits. 
Will take another look later.
-Mikhal

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

https://codereview.chromium.org/138753004/diff/250001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.cc:46: 
nit: rm new line

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

https://codereview.chromium.org/138753004/diff/250001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.h:60: // Clock used by Cast
transport.
and who owns it?

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
File chrome/renderer/media/cast_session_delegate.h (right):

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.h:53: // will begin.
You may want to look at Patrik's cl where he's rewriting this code.

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
File chrome/renderer/media/cast_transport_sender_ipc.cc (right):

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:27:
IPC_BEGIN_MESSAGE_MAP_EX(CastIPCDispatcher, message, msg_is_good)
Can will msg_is_good update?

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:45: channel_ = NULL;
What's the difference between removing a filter and closing a channel? Seems to
do the same thing.

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:96: if (
CastIPCDispatcher::global_instance_) {
nit:remove space after (

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:114: void
CastTransportSenderIPC::SetPacketReceiver(
Consider renaming the function

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
File chrome/renderer/media/cast_transport_sender_ipc.h (right):

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.h:27: explicit
CastTransportSenderIPC(
remove explicit

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.h:32: //
media::cast::transport::CastNetSender implementation
CastNetSender->CastTransportSender

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
File chrome/renderer/media/cast_udp_transport.cc (right):

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
chrome/renderer/media/cast_udp_transport.cc:18: net::IPEndPoint
null_endpoint(net::IPAddressNumber(4, 0), 2346);
why null? Please add a comment.

[hubbe, 2014-02-05 01:11:44]

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

https://codereview.chromium.org/138753004/diff/250001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.cc:46: 
On 2014/02/05 00:53:05, mikhal1 wrote:
> nit: rm new line

Done.

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

https://codereview.chromium.org/138753004/diff/250001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.h:60: // Clock used by Cast
transport.
On 2014/02/05 00:53:05, mikhal1 wrote:
> and who owns it?

It's not a pointer or reference, I think it's pretty clear who owns it.

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
File chrome/renderer/media/cast_session_delegate.h (right):

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.h:53: // will begin.
On 2014/02/05 00:53:05, mikhal1 wrote:
> You may want to look at Patrik's cl where he's rewriting this code.

Yes, probably.

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
File chrome/renderer/media/cast_transport_sender_ipc.cc (right):

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:27:
IPC_BEGIN_MESSAGE_MAP_EX(CastIPCDispatcher, message, msg_is_good)
On 2014/02/05 00:53:05, mikhal1 wrote:
> Can will msg_is_good update?

Yes, I think so.

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:45: channel_ = NULL;
On 2014/02/05 00:53:05, mikhal1 wrote:
> What's the difference between removing a filter and closing a channel? Seems
to
> do the same thing.

Not much. Although, filter removal will never happen I think.
It's just part of the API that I need to implement.

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:96: if (
CastIPCDispatcher::global_instance_) {
On 2014/02/05 00:53:05, mikhal1 wrote:
> nit:remove space after (

Done.

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:114: void
CastTransportSenderIPC::SetPacketReceiver(
On 2014/02/05 00:53:05, mikhal1 wrote:
> Consider renaming the function

I could, but not in this CL, as I would have to change it everywhere.

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
File chrome/renderer/media/cast_transport_sender_ipc.h (right):

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.h:27: explicit
CastTransportSenderIPC(
On 2014/02/05 00:53:05, mikhal1 wrote:
> remove explicit

Done.

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.h:32: //
media::cast::transport::CastNetSender implementation
On 2014/02/05 00:53:05, mikhal1 wrote:
> CastNetSender->CastTransportSender

Done.

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
File chrome/renderer/media/cast_udp_transport.cc (right):

https://codereview.chromium.org/138753004/diff/250001/chrome/renderer/media/c...
chrome/renderer/media/cast_udp_transport.cc:18: net::IPEndPoint
null_endpoint(net::IPAddressNumber(4, 0), 2346);
On 2014/02/05 00:53:05, mikhal1 wrote:
> why null? Please add a comment.

Comment added.

[mikhal1, 2014-02-05 21:09:42]

lgtm + 1 comment

https://chromiumcodereview.appspot.com/138753004/diff/470001/chrome/renderer/...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://chromiumcodereview.appspot.com/138753004/diff/470001/chrome/renderer/...
chrome/renderer/media/cast_session_delegate.cc:83: if (!audio_configured_ ||
!video_configured_ || !transport_configured_)
We must have the transport configured to be able to send packets and either
audio or video or both.

[hubbe, 2014-02-05 21:14:53]

https://chromiumcodereview.appspot.com/138753004/diff/470001/chrome/renderer/...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://chromiumcodereview.appspot.com/138753004/diff/470001/chrome/renderer/...
chrome/renderer/media/cast_session_delegate.cc:83: if (!audio_configured_ ||
!video_configured_ || !transport_configured_)
On 2014/02/05 21:09:43, mikhal1 wrote:
> We must have the transport configured to be able to send packets and either
> audio or video or both.

Until Patrik's changes are checked in, the code only supports using audio *and*
video.

[hubbe, 2014-02-06 22:42:30]

Adding OWNERS and security reviewers, git cl owners came up with:

jam:
  chrome/browser/media/DEPS
  chrome/browser/media/cast_transport_host_filter.cc
  chrome/browser/media/cast_transport_host_filter.h
  chrome/browser/media/cast_transport_host_filter_unittest.cc
  chrome/renderer/chrome_content_renderer_client.cc
  chrome/renderer/chrome_content_renderer_client.h
  chrome/renderer/extensions/cast_streaming_native_handler.cc
  chrome/renderer/media/DEPS
  chrome/renderer/media/cast_session.cc
  chrome/renderer/media/cast_session.h
  chrome/renderer/media/cast_session_delegate.cc
  chrome/renderer/media/cast_session_delegate.h
  chrome/renderer/media/cast_transport_sender_ipc.cc
  chrome/renderer/media/cast_transport_sender_ipc.h
  chrome/renderer/media/cast_udp_transport.cc
  content/common/DEPS
  content/common/content_message_generator.h
cevans:
  content/common/DEPS
  content/common/cast_messages.h
  content/common/content_message_generator.h
  ipc/ipc_message_start.h
scherkus:
  chrome/browser/media/DEPS
  chrome/browser/media/cast_transport_host_filter.cc
  chrome/browser/media/cast_transport_host_filter.h
  chrome/browser/media/cast_transport_host_filter_unittest.cc
  chrome/renderer/media/DEPS
  chrome/renderer/media/cast_session.cc
  chrome/renderer/media/cast_session.h
  chrome/renderer/media/cast_session_delegate.cc
  chrome/renderer/media/cast_session_delegate.h
  chrome/renderer/media/cast_transport_sender_ipc.cc
  chrome/renderer/media/cast_transport_sender_ipc.h
  chrome/renderer/media/cast_udp_transport.cc
  media/cast/audio_sender/audio_sender.cc
  media/cast/cast_receiver.gyp
  media/cast/cast_sender.gyp
  media/cast/rtcp/rtcp_defines.h
  media/cast/transport/cast_transport.gyp
  media/cast/transport/cast_transport_config.cc
  media/cast/transport/cast_transport_config.h
  media/cast/transport/cast_transport_defines.h
  media/cast/transport/cast_transport_sender.h
  media/cast/transport/rtcp/rtcp_builder.cc
  media/cast/video_sender/video_sender.cc

[scherkus (not reviewing), 2014-02-07 00:11:04]

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

https://codereview.chromium.org/138753004/diff/700001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.h:72: #endif  //
CHROME_BROWSER_MEDIA_CAST_TRANSPORT_HOST_FILTER_H_
nit: extra blank line before the #endif

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/extensi...
File chrome/renderer/extensions/cast_streaming_native_handler.cc (right):

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.cc:19: #include
"content/public/renderer/render_view.h"
are these #includes still needed?

I didn't spot any other changes in this .cc file that would require them

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
File chrome/renderer/media/cast_session.h (right):

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_session.h:29: namespace content{
space before {

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_session.h:35: namespace cast{
space before {

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_session.h:37: };
don't need ;

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.cc:32:
DCHECK(io_message_loop_proxy_.get());
don't need .get()

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.cc:88: // TODO(hubbe):
receiver_endpoint, local_endpoint
what is this TODO referring to? unlike the TODO below about aes_key and
aes_iv_mask (which I assume is a TODO to remind you to set these), you are
setting local_endpoint and receiver_endpoint on lines 89-90

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.cc:99: // TODO(Hubbe): aes_key,
aes_iv_mask
nit: s/Hubbe/hubbe/ ?

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.cc:103:
base::Bind(&IgnoreStatusCB)));
these sorts of things sadden me

by putting in the callback ignoring code at the endpoint in the renderer, we
have to introduce and maintain the following plumbing:
 - CastIPCDispatcher::OnNotifyStatusChange()
 - CastMsg_NotifyStatusChange IPC message
 - CastTransportHostFilter::NotifyStatusChange()

... only to throw away the result. You'd be amazed at how this type of
well-intentioned code leads to cruft!

Also as far as I can tell, it looks like the status returned is always false
because we're not setting aes_key/aes_iv_mask (the TransportEncryptionHandler
object that eventually is created seems to want properly sized keys)

It'd be nice to at least have a justification of some sort (perhaps by expanding
the TODO about setting aes_key,aes_iv_mask?) or nuking the code in this CL until
things are properly hooked up (e.g., binding the IngoreStatusCB inside of
CastTransportHostFilter vs. CastIPCDispatcher?)

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
File chrome/renderer/media/cast_session_delegate.h (right):

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.h:37: };
don't need ;

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
File chrome/renderer/media/cast_transport_sender_ipc.cc (right):

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:34: LOG(ERROR) << "Failed to
parse incoming message.";
avoid LOG(ERROR)

heck, there's nothing you can really do with msg_is_good anyway so you can use
IPC_BEGIN_MESSAGE_MAP

(IIRC the IPC_BEGIN_MESSAGE_MAP_EX version is used in the browser process to
terminate bad renderer processes, but you don't really need to worry about that
here)

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:54: 
remove extra bit o' whitespace

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:115:
CastIPCDispatcher::global_instance_->id_map_.Remove(channel_id_);
over indented

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:119: bool
CastTransportSenderIPC::Send(IPC::Message *message) {
this method is bool, yet you ignore the result every time you call it :)

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:185: // Retrieves video RTP
statistics.
nuke comment?

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:194: 
nuke all this extra whitespace

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
File chrome/renderer/media/cast_transport_sender_ipc.h (right):

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.h:15: namespace cast {
IIRC we prefer chrome/ code to be in the global namespace:
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/pcgYLSJsii8...

also these types of namespaces are generally discouraged since the types inside
of the cast namespace contain a CastXXX prefix, so it ends up being redundant

it also isn't clear to future readers which cast:: is which (i.e., the one under
media/ or the one under chrome/?)
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/h11LgwA5vgg/qJb4V...

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.h:32: //
media::cast::transport::CastTransportSender implementation
comments end with periods

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.h:51: // Retransmission request.
nit: this looks duplicated from the CastTransportSender .h -- I'd just nuke it

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.h:67: friend class
CastIPCDispatcher;
do these two classes really need to be friends?

why not expose public methods on both instead of poking at each others privates?

something like:

class CastIPCDispatcher {
  // Returns global instance if created, NULL otherwise.
  static CastIPCDispatcher* Get();

  int AddTransport(CastTransportSenderIPC* sender);
  void RemoveTransport(int channel_id);

  void Send(IPC::Message* msg) {
    if (!channel_ || !channel_->Send(msg))
      delete msg;
  }

  // etc...
};


alternatively you can make CastTransportSenderIPC a private inner class of
CastIPCDispatcher and completely hide the existence of CastTransportSenderIPC
entirely (i.e., CastIPCDispatcher would create them and return
media::cast::transport::CastTransportSender*), but that might be going a bit
overboard

WDYT?

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.h:78: class CastIPCDispatcher :
public IPC::ChannelProxy::MessageFilter {
any reason this needs to be inside this .h vs its own .h?

it's also a bit confusing that the implementation of this class in the .cc is
first despite the definition in the .h being second ...

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
File chrome/renderer/media/cast_udp_transport.cc (right):

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_udp_transport.cc:19: // TODO(Hubbe): Figure out how
to not use a fixed port.
s/Hubbe/hubbe/

[hubbe, 2014-02-07 00:52:21]

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

https://codereview.chromium.org/138753004/diff/700001/chrome/browser/media/ca...
chrome/browser/media/cast_transport_host_filter.h:72: #endif  //
CHROME_BROWSER_MEDIA_CAST_TRANSPORT_HOST_FILTER_H_
On 2014/02/07 00:11:06, scherkus wrote:
> nit: extra blank line before the #endif

Done.

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/extensi...
File chrome/renderer/extensions/cast_streaming_native_handler.cc (right):

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/extensi...
chrome/renderer/extensions/cast_streaming_native_handler.cc:19: #include
"content/public/renderer/render_view.h"
On 2014/02/07 00:11:06, scherkus wrote:
> are these #includes still needed?
> 
> I didn't spot any other changes in this .cc file that would require them

Nope, gone.

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
File chrome/renderer/media/cast_session.h (right):

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_session.h:29: namespace content{
On 2014/02/07 00:11:06, scherkus wrote:
> space before {

Done.

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_session.h:35: namespace cast{
On 2014/02/07 00:11:06, scherkus wrote:
> space before {

Done.

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_session.h:37: };
On 2014/02/07 00:11:06, scherkus wrote:
> don't need ;

Done.

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.cc:32:
DCHECK(io_message_loop_proxy_.get());
On 2014/02/07 00:11:06, scherkus wrote:
> don't need .get()

Done.

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.cc:88: // TODO(hubbe):
receiver_endpoint, local_endpoint
On 2014/02/07 00:11:06, scherkus wrote:
> what is this TODO referring to? unlike the TODO below about aes_key and
> aes_iv_mask (which I assume is a TODO to remind you to set these), you are
> setting local_endpoint and receiver_endpoint on lines 89-90

Yep, this TODO has been removed, sorry about that.

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.cc:99: // TODO(Hubbe): aes_key,
aes_iv_mask
On 2014/02/07 00:11:06, scherkus wrote:
> nit: s/Hubbe/hubbe/ ?

Done.

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.cc:103:
base::Bind(&IgnoreStatusCB)));
On 2014/02/07 00:11:06, scherkus wrote:
> these sorts of things sadden me
> 
> by putting in the callback ignoring code at the endpoint in the renderer, we
> have to introduce and maintain the following plumbing:
>  - CastIPCDispatcher::OnNotifyStatusChange()
>  - CastMsg_NotifyStatusChange IPC message
>  - CastTransportHostFilter::NotifyStatusChange()
> 
> ... only to throw away the result. You'd be amazed at how this type of
> well-intentioned code leads to cruft!
> 
> Also as far as I can tell, it looks like the status returned is always false
> because we're not setting aes_key/aes_iv_mask (the TransportEncryptionHandler
> object that eventually is created seems to want properly sized keys)
> 
> It'd be nice to at least have a justification of some sort (perhaps by
expanding
> the TODO about setting aes_key,aes_iv_mask?) or nuking the code in this CL
until
> things are properly hooked up (e.g., binding the IngoreStatusCB inside of
> CastTransportHostFilter vs. CastIPCDispatcher?)

I would gladly hook it up, but I don't know how.
I've added another TODO for it, and I fully intend to hook this up to it's
rightful place as soon as I figure out what that is.
pwestin is working on another CL which also adds async callbacks after startup,
so hopefully these two callbacks can actually go to the same place in the
javascript, but that place doesn't exist yet as far as I know.

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
File chrome/renderer/media/cast_session_delegate.h (right):

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.h:37: };
On 2014/02/07 00:11:06, scherkus wrote:
> don't need ;

Gone (the whole block actually)

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
File chrome/renderer/media/cast_transport_sender_ipc.cc (right):

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:34: LOG(ERROR) << "Failed to
parse incoming message.";
On 2014/02/07 00:11:06, scherkus wrote:
> avoid LOG(ERROR)
> 
> heck, there's nothing you can really do with msg_is_good anyway so you can use
> IPC_BEGIN_MESSAGE_MAP
> 
> (IIRC the IPC_BEGIN_MESSAGE_MAP_EX version is used in the browser process to
> terminate bad renderer processes, but you don't really need to worry about
that
> here)

That's what I get for following documentation I guess...
Done.

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:54: 
On 2014/02/07 00:11:06, scherkus wrote:
> remove extra bit o' whitespace

Done.

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:115:
CastIPCDispatcher::global_instance_->id_map_.Remove(channel_id_);
On 2014/02/07 00:11:06, scherkus wrote:
> over indented

Done.

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:119: bool
CastTransportSenderIPC::Send(IPC::Message *message) {
On 2014/02/07 00:11:06, scherkus wrote:
> this method is bool, yet you ignore the result every time you call it :)

Making it void.

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:185: // Retrieves video RTP
statistics.
On 2014/02/07 00:11:06, scherkus wrote:
> nuke comment?

Done.

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.cc:194: 
On 2014/02/07 00:11:06, scherkus wrote:
> nuke all this extra whitespace

Done.

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
File chrome/renderer/media/cast_transport_sender_ipc.h (right):

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.h:15: namespace cast {
On 2014/02/07 00:11:06, scherkus wrote:
> IIRC we prefer chrome/ code to be in the global namespace:
>
https://groups.google.com/a/chromium.org/forum/#%21msg/chromium-dev/pcgYLSJsi...
> 
> also these types of namespaces are generally discouraged since the types
inside
> of the cast namespace contain a CastXXX prefix, so it ends up being redundant
> 
> it also isn't clear to future readers which cast:: is which (i.e., the one
under
> media/ or the one under chrome/?)
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/h11LgwA5vgg/qJb4V...

Done.

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.h:32: //
media::cast::transport::CastTransportSender implementation
On 2014/02/07 00:11:06, scherkus wrote:
> comments end with periods

Done.

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.h:51: // Retransmission request.
On 2014/02/07 00:11:06, scherkus wrote:
> nit: this looks duplicated from the CastTransportSender .h -- I'd just nuke it

Done.

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.h:67: friend class
CastIPCDispatcher;
On 2014/02/07 00:11:06, scherkus wrote:
> do these two classes really need to be friends?
> 
> why not expose public methods on both instead of poking at each others
privates?
> 
> something like:
> 
> class CastIPCDispatcher {
>   // Returns global instance if created, NULL otherwise.
>   static CastIPCDispatcher* Get();
> 
>   int AddTransport(CastTransportSenderIPC* sender);
>   void RemoveTransport(int channel_id);
> 
>   void Send(IPC::Message* msg) {
>     if (!channel_ || !channel_->Send(msg))
>       delete msg;
>   }
> 
>   // etc...
> };
> 
> 
> alternatively you can make CastTransportSenderIPC a private inner class of
> CastIPCDispatcher and completely hide the existence of CastTransportSenderIPC
> entirely (i.e., CastIPCDispatcher would create them and return
> media::cast::transport::CastTransportSender*), but that might be going a bit
> overboard
> 
> WDYT?


Generally I think of these two classes as one unit.
Adding helper functions may or may not be nicer, but without the friendship
relationship these helper functions would be exposed to a lot of people who
don't need them.
If the two classes go in separate h/cc files, then it's probably clearer to get
rid of the friendship and just use public methods.

Actually, my preferred way to write this would be to only expose factory methods
in this file and put the classes in the .cc file. That way public methods are
only
exposed in one file. Although, that's not usually how it's done in chrome, so
I'm not sure if that is a good idea or not.

PS: Not arguing to avoid work, just checking which way to go with it before i
change it. :)

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.h:78: class CastIPCDispatcher :
public IPC::ChannelProxy::MessageFilter {
On 2014/02/07 00:11:06, scherkus wrote:
> any reason this needs to be inside this .h vs its own .h?
> 
> it's also a bit confusing that the implementation of this class in the .cc is
> first despite the definition in the .h being second ...

See comment above (factory methods)

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
File chrome/renderer/media/cast_udp_transport.cc (right):

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_udp_transport.cc:19: // TODO(Hubbe): Figure out how
to not use a fixed port.
On 2014/02/07 00:11:06, scherkus wrote:
> s/Hubbe/hubbe/

Done.

[scherkus (not reviewing), 2014-02-07 01:15:28]

passing this off to acolwell@ for media-ish reviews (I'm going OOO)

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
File chrome/renderer/media/cast_transport_sender_ipc.h (right):

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.h:67: friend class
CastIPCDispatcher;
On 2014/02/07 00:52:22, hubbe wrote:
> On 2014/02/07 00:11:06, scherkus wrote:
> > do these two classes really need to be friends?
> > 
> > why not expose public methods on both instead of poking at each others
> privates?
> > 
> > something like:
> > 
> > class CastIPCDispatcher {
> >   // Returns global instance if created, NULL otherwise.
> >   static CastIPCDispatcher* Get();
> > 
> >   int AddTransport(CastTransportSenderIPC* sender);
> >   void RemoveTransport(int channel_id);
> > 
> >   void Send(IPC::Message* msg) {
> >     if (!channel_ || !channel_->Send(msg))
> >       delete msg;
> >   }
> > 
> >   // etc...
> > };
> > 
> > 
> > alternatively you can make CastTransportSenderIPC a private inner class of
> > CastIPCDispatcher and completely hide the existence of
CastTransportSenderIPC
> > entirely (i.e., CastIPCDispatcher would create them and return
> > media::cast::transport::CastTransportSender*), but that might be going a bit
> > overboard
> > 
> > WDYT?
> 
> 
> Generally I think of these two classes as one unit.
> Adding helper functions may or may not be nicer, but without the friendship
> relationship these helper functions would be exposed to a lot of people who
> don't need them.
> If the two classes go in separate h/cc files, then it's probably clearer to
get
> rid of the friendship and just use public methods.
> 
> Actually, my preferred way to write this would be to only expose factory
methods
> in this file and put the classes in the .cc file. That way public methods are
> only
> exposed in one file. Although, that's not usually how it's done in chrome, so
> I'm not sure if that is a good idea or not.

Not entirely sure what you mean by factory methods ... but typically chrome is
OK exposing extra public methods and classes *within a module* (i.e., chrome/,
content/, net/)

My $0.02: it can be infuriating enough dealing with abstraction layers between
modules, hence the tendency to avoid additional abstractions within a module and
just use the types you declare and don't worry about over exposing public
methods


Anyway I don't know where this leaves us and I'm going on vacation right now.
I'd either:
  a) go the full two-file-no-friend split
  b) go the hidden internal class route and make them friendly

I'll get acolwell@ to review subsequent changes.

[hubbe, 2014-02-07 22:27:46]

For your convenience, git cl owners came up with this division, but feel free to
review the whole thing.

acolwell:
  chrome/browser/media/DEPS
  chrome/browser/media/cast_transport_host_filter.cc
  chrome/browser/media/cast_transport_host_filter.h
  chrome/browser/media/cast_transport_host_filter_unittest.cc
  chrome/renderer/media/DEPS
  chrome/renderer/media/cast_session.cc
  chrome/renderer/media/cast_session.h
  chrome/renderer/media/cast_session_delegate.cc
  chrome/renderer/media/cast_session_delegate.h
  chrome/renderer/media/cast_transport_sender_ipc.cc
  chrome/renderer/media/cast_transport_sender_ipc.h
  chrome/renderer/media/cast_udp_transport.cc
  media/cast/audio_sender/audio_sender.cc
  media/cast/cast_receiver.gyp
  media/cast/cast_sender.gyp
  media/cast/rtcp/rtcp_defines.h
  media/cast/transport/cast_transport.gyp
  media/cast/transport/cast_transport_config.cc
  media/cast/transport/cast_transport_config.h
  media/cast/transport/cast_transport_defines.h
  media/cast/transport/cast_transport_sender.h
  media/cast/transport/rtcp/rtcp_builder.cc
  media/cast/video_sender/video_sender.cc
cevans:
  chrome/common/cast_messages.h
  content/common/content_param_traits.h
  ipc/ipc_message_start.h
sky:
  chrome/browser/media/DEPS
  chrome/browser/media/cast_transport_host_filter.cc
  chrome/browser/media/cast_transport_host_filter.h
  chrome/browser/media/cast_transport_host_filter_unittest.cc
  chrome/common/common_message_generator.h
  chrome/renderer/chrome_content_renderer_client.cc
  chrome/renderer/chrome_content_renderer_client.h
  chrome/renderer/media/DEPS
  chrome/renderer/media/cast_session.cc
  chrome/renderer/media/cast_session.h
  chrome/renderer/media/cast_session_delegate.cc
  chrome/renderer/media/cast_session_delegate.h
  chrome/renderer/media/cast_transport_sender_ipc.cc
  chrome/renderer/media/cast_transport_sender_ipc.h
  chrome/renderer/media/cast_udp_transport.cc

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
File chrome/renderer/media/cast_transport_sender_ipc.h (right):

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...
chrome/renderer/media/cast_transport_sender_ipc.h:67: friend class
CastIPCDispatcher;
On 2014/02/07 01:15:29, scherkus (OOO Fri 7-9) wrote:
> On 2014/02/07 00:52:22, hubbe wrote:
> > On 2014/02/07 00:11:06, scherkus wrote:
> > > do these two classes really need to be friends?
> > > 
> > > why not expose public methods on both instead of poking at each others
> > privates?
> > > 
> > > something like:
> > > 
> > > class CastIPCDispatcher {
> > >   // Returns global instance if created, NULL otherwise.
> > >   static CastIPCDispatcher* Get();
> > > 
> > >   int AddTransport(CastTransportSenderIPC* sender);
> > >   void RemoveTransport(int channel_id);
> > > 
> > >   void Send(IPC::Message* msg) {
> > >     if (!channel_ || !channel_->Send(msg))
> > >       delete msg;
> > >   }
> > > 
> > >   // etc...
> > > };
> > > 
> > > 
> > > alternatively you can make CastTransportSenderIPC a private inner class of
> > > CastIPCDispatcher and completely hide the existence of
> CastTransportSenderIPC
> > > entirely (i.e., CastIPCDispatcher would create them and return
> > > media::cast::transport::CastTransportSender*), but that might be going a
bit
> > > overboard
> > > 
> > > WDYT?
> > 
> > 
> > Generally I think of these two classes as one unit.
> > Adding helper functions may or may not be nicer, but without the friendship
> > relationship these helper functions would be exposed to a lot of people who
> > don't need them.
> > If the two classes go in separate h/cc files, then it's probably clearer to
> get
> > rid of the friendship and just use public methods.
> > 
> > Actually, my preferred way to write this would be to only expose factory
> methods
> > in this file and put the classes in the .cc file. That way public methods
are
> > only
> > exposed in one file. Although, that's not usually how it's done in chrome,
so
> > I'm not sure if that is a good idea or not.
> 
> Not entirely sure what you mean by factory methods ... but typically chrome is
> OK exposing extra public methods and classes *within a module* (i.e., chrome/,
> content/, net/)
> 
> My $0.02: it can be infuriating enough dealing with abstraction layers between
> modules, hence the tendency to avoid additional abstractions within a module
and
> just use the types you declare and don't worry about over exposing public
> methods
> 
> 
> Anyway I don't know where this leaves us and I'm going on vacation right now.
> I'd either:
>   a) go the full two-file-no-friend split
>   b) go the hidden internal class route and make them friendly
> 
> I'll get acolwell@ to review subsequent changes.

Ok, I started working on (a), but it ended up ugly (in my opinion) because it
just had too many methods that looks something like:

void FOO(some arguments) {
  other_object->FOO(some_arguments);
}

Repeat for FOO, BAR, BAZ, etc. and with the inherent duplication between .h and
.cc, it just became too much for me.  So instead I decided to show you what I
meant by using "factory methods". (I really meant "factory functions" though...)

Feel free to tell me how fugly it is and then I'll go back to (a) or (b) again.
:)

[hubbe, 2014-02-07 22:46:28]

On 2014/02/07 22:27:46, hubbe wrote:
> For your convenience, git cl owners came up with this division, but feel free
to
> review the whole thing.

Re-generated list:
acolwell:
  chrome/browser/media/DEPS
  chrome/browser/media/cast_transport_host_filter.cc
  chrome/browser/media/cast_transport_host_filter.h
  chrome/browser/media/cast_transport_host_filter_unittest.cc
  chrome/renderer/media/cast_session.cc
  chrome/renderer/media/cast_session.h
  chrome/renderer/media/cast_session_delegate.cc
  chrome/renderer/media/cast_session_delegate.h
  chrome/renderer/media/cast_transport_sender_ipc.cc
  chrome/renderer/media/cast_transport_sender_ipc.h
  chrome/renderer/media/cast_udp_transport.cc
  media/cast/audio_sender/audio_sender.cc
  media/cast/cast_receiver.gyp
  media/cast/cast_sender.gyp
  media/cast/rtcp/rtcp_defines.h
  media/cast/transport/cast_transport.gyp
  media/cast/transport/cast_transport_config.cc
  media/cast/transport/cast_transport_config.h
  media/cast/transport/cast_transport_defines.h
  media/cast/transport/cast_transport_sender.h
  media/cast/transport/rtcp/rtcp_builder.cc
  media/cast/video_sender/video_sender.cc
cevans:
  chrome/common/cast_messages.h
  ipc/ipc_message_start.h
sky:
  chrome/browser/media/DEPS
  chrome/browser/media/cast_transport_host_filter.cc
  chrome/browser/media/cast_transport_host_filter.h
  chrome/browser/media/cast_transport_host_filter_unittest.cc
  chrome/common/DEPS
  chrome/common/common_message_generator.h
  chrome/common/common_param_traits.cc
  chrome/common/common_param_traits.h
  chrome/renderer/chrome_content_renderer_client.cc
  chrome/renderer/chrome_content_renderer_client.h
  chrome/renderer/media/cast_session.cc
  chrome/renderer/media/cast_session.h
  chrome/renderer/media/cast_session_delegate.cc
  chrome/renderer/media/cast_session_delegate.h
  chrome/renderer/media/cast_transport_sender_ipc.cc
  chrome/renderer/media/cast_transport_sender_ipc.h
  chrome/renderer/media/cast_udp_transport.cc

[hubbe, 2014-02-08 00:22:09]

On 2014/02/07 22:46:28, hubbe wrote:
> On 2014/02/07 22:27:46, hubbe wrote:
> > For your convenience, git cl owners came up with this division, but feel
free
> to
> > review the whole thing.
> 
> Re-generated list:
> acolwell:
>   chrome/browser/media/DEPS
>   chrome/browser/media/cast_transport_host_filter.cc
>   chrome/browser/media/cast_transport_host_filter.h
>   chrome/browser/media/cast_transport_host_filter_unittest.cc
>   chrome/renderer/media/cast_session.cc
>   chrome/renderer/media/cast_session.h
>   chrome/renderer/media/cast_session_delegate.cc
>   chrome/renderer/media/cast_session_delegate.h
>   chrome/renderer/media/cast_transport_sender_ipc.cc
>   chrome/renderer/media/cast_transport_sender_ipc.h
>   chrome/renderer/media/cast_udp_transport.cc
>   media/cast/audio_sender/audio_sender.cc
>   media/cast/cast_receiver.gyp
>   media/cast/cast_sender.gyp
>   media/cast/rtcp/rtcp_defines.h
>   media/cast/transport/cast_transport.gyp
>   media/cast/transport/cast_transport_config.cc
>   media/cast/transport/cast_transport_config.h
>   media/cast/transport/cast_transport_defines.h
>   media/cast/transport/cast_transport_sender.h
>   media/cast/transport/rtcp/rtcp_builder.cc
>   media/cast/video_sender/video_sender.cc
> cevans:
>   chrome/common/cast_messages.h
>   ipc/ipc_message_start.h
> sky:
>   chrome/browser/media/DEPS
>   chrome/browser/media/cast_transport_host_filter.cc
>   chrome/browser/media/cast_transport_host_filter.h
>   chrome/browser/media/cast_transport_host_filter_unittest.cc
>   chrome/common/DEPS
>   chrome/common/common_message_generator.h
>   chrome/common/common_param_traits.cc
>   chrome/common/common_param_traits.h
>   chrome/renderer/chrome_content_renderer_client.cc
>   chrome/renderer/chrome_content_renderer_client.h
>   chrome/renderer/media/cast_session.cc
>   chrome/renderer/media/cast_session.h
>   chrome/renderer/media/cast_session_delegate.cc
>   chrome/renderer/media/cast_session_delegate.h
>   chrome/renderer/media/cast_transport_sender_ipc.cc
>   chrome/renderer/media/cast_transport_sender_ipc.h
>   chrome/renderer/media/cast_udp_transport.cc


Ok, so I have a problem: As suggested I moved cast_messages.h to chrome/, but
then I couldn't access ParamTraits<net::IPEndPoint> any more. I'm trying moving
ParamTraits<net::IPEndPoint> to ip_endpoint.h, but that doesn't seem to work
either.  Where *should* it be?

[hubbe, 2014-02-10 18:50:14]

Adding joi for content/public changes.

[sky, 2014-02-10 21:56:44]

There are better local OWNERS, meaning pick and owner from chrome/browser/media
and chrome/renderer/media. Additionally the message changes have specific
reviewers. I believe that leaves the following files for me to review:
chrome/common/DEPS
chrome/renderer/chrome_content_renderer_client.*
Those seem fine, one question below though.

https://codereview.chromium.org/138753004/diff/1280001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/138753004/diff/1280001/chrome/chrome_browser....
chrome/chrome_browser.gypi:52:
'../media/cast/transport/cast_transport.gyp:cast_transport',
nit: sort.

https://codereview.chromium.org/138753004/diff/1280001/chrome/chrome_renderer...
File chrome/chrome_renderer.gypi (right):

https://codereview.chromium.org/138753004/diff/1280001/chrome/chrome_renderer...
chrome/chrome_renderer.gypi:12: 'common',
Don't you also need to add a dependency from common onto media?

[hubbe, 2014-02-10 22:35:55]

-cevans, +palmer

[hubbe, 2014-02-10 22:35:56]

-cevans, +palmer

[hubbe, 2014-02-10 22:50:06]

-joi
+avi

Sky: I also need you to look at chrome/common/common/message_generator.h

https://codereview.chromium.org/138753004/diff/1280001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/138753004/diff/1280001/chrome/chrome_browser....
chrome/chrome_browser.gypi:52:
'../media/cast/transport/cast_transport.gyp:cast_transport',
On 2014/02/10 21:56:44, sky wrote:
> nit: sort.

Done.

https://codereview.chromium.org/138753004/diff/1280001/chrome/chrome_renderer...
File chrome/chrome_renderer.gypi (right):

https://codereview.chromium.org/138753004/diff/1280001/chrome/chrome_renderer...
chrome/chrome_renderer.gypi:12: 'common',
On 2014/02/10 21:56:44, sky wrote:
> Don't you also need to add a dependency from common onto media?

Common is shared between renderer and browser?
It seems to compile right now, and renderer already have a dep on cast library
(which has a dep on cast_transport)

[hubbe, 2014-02-10 22:55:28]

New OWNER list update:

Sky:
 chrome/common/DEPS
 chrome/common/common_message_generator.h
 chrome/renderer/chrome_content_renderer_client.cc
 chrome/renderer/chrome_content_renderer_client.h
palmer:
  chrome/common/cast_messages.h
  ipc/ipc_message_start.h
avi:
  content/common/content_param_traits.cc
  content/common/content_param_traits.h
  content/public/common/common_param_traits.cc
  content/public/common/common_param_traits.h

acolwell: Everything else.

[Avi (use Gerrit), 2014-02-10 22:59:53]

content/common/content_param_traits.cc
  content/common/content_param_traits.h
  content/public/common/common_param_traits.cc
  content/public/common/common_param_traits.h

LGTM

[sky, 2014-02-10 23:17:11]

Said files LGTM

[palmer, 2014-02-11 00:50:03]

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

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:95: void
CastTransportHostFilter::OnDelete(
Nit: This could fit on one line.

https://codereview.chromium.org/138753004/diff/1360001/chrome/common/cast_mes...
File chrome/common/cast_messages.h (right):

https://codereview.chromium.org/138753004/diff/1360001/chrome/common/cast_mes...
chrome/common/cast_messages.h:1: // Copyright (c) 2013 The Chromium Authors. All
rights reserved.
2014

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

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:20: // renderer less direct
control over the UDP sockets.
Well, only honest renderers. Compromised renderers don't have to go through this
filtering layer, they can say whatever they want to the browser via the given
IPC channels.


How abstract are the IPC messages? Forgive me, but I'm unfamiliar with this
code; how much control over the exact packet headers/contents do renderers have?

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:63: data.c_name =
data.c_name;
Bug? Typo? Should be data.c_name = c_name?

https://codereview.chromium.org/138753004/diff/1360001/media/cast/transport/c...
File media/cast/transport/cast_transport_config.h (right):

https://codereview.chromium.org/138753004/diff/1360001/media/cast/transport/c...
media/cast/transport/cast_transport_config.h:165: std::string c_name;
What is the intended structure of this string?

Can the recipient validate that structure somehow?

[acolwell GONE FROM CHROMIUM, 2014-02-11 00:57:36]

Here is my first round of comments.

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

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
nit: s/(c) 2013/2014/

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:63:
media::cast::transport::CastTransportSender *sender =
nit: * should be next to the type not the variable name.

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:77:
sender->SetPacketReceiver(
Why aren't these passed into the CreateCastTransportSender()?

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:97:
media::cast::transport::CastTransportSender *sender =
nit: * should be next to the type not the variable name.

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:102: LOG(ERROR) <<
"CastTransportHostFilter::Delete called "
nit: s/LOG(ERROR)/DVLOG(1)/ here and elsewhere. It is unlikely these errors
really need to be displayed in release builds.

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:111:
media::cast::transport::CastTransportSender *sender =
ditto

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:126:
media::cast::transport::CastTransportSender *sender =
ditto

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:143:
media::cast::transport::CastTransportSender *sender =
ditto

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:163:
media::cast::transport::CastTransportSender *sender =
ditto

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
File chrome/browser/media/cast_transport_host_filter.h (right):

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
nit: s/(c) 2013/2014/

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
File chrome/browser/media/cast_transport_host_filter_unittest.cc (right):

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter_unittest.cc:50: // Now do it
wrong.
nit: Please make the comment a little more explicit about why this is wrong. Are
we testing that only one object is allowed at a time or are we verifying that
multiple objects with the same channel_id can't be new & deleted.

Is this just intended to be a "we didn't crash and we didn't leak memory" test
case?

https://codereview.chromium.org/138753004/diff/1360001/chrome/common/cast_mes...
File chrome/common/cast_messages.h (right):

https://codereview.chromium.org/138753004/diff/1360001/chrome/common/cast_mes...
chrome/common/cast_messages.h:1: // Copyright (c) 2013 The Chromium Authors. All
rights reserved.
nit: s/(c) 2013/2014/

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
File chrome/renderer/media/cast_session.h (right):

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_session.h:36: class CastIPCDispatcher;
remove. It doesn't look like this is necessary

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_session_delegate.cc:103:
base::Bind(&IgnoreStatusCB)));
Remove the callback from the method signature until you are actually ready to
add code to handle the callback.

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

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
nit: s/(c) //

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:93: friend class
CastIPCDispatcher;
Do not use friend. Just have the the IPCDispatcher call public methods on this
object. That way all code that accesses the internal state of this object is in
this class.

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:94: void Send(IPC::Message
*message);
nit: s/ */* /

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:152: if (ptr) {
nit: Everything in this block should be in a CastTransportSenderIPC method.

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:173:
ptr->status_callback_.Run(status);
Please remove this an any related IPC code until you actually have code that
responds to it. There is no point in sending the messages if no one is
listening.

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:186: if (ptr) {
ditto

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:196: friend class
CastTransportSenderIPC;
Remove friend declaration

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:215:
CastTransportSenderIPC::CastTransportSenderIPC(
Put class declarations in the .h, remove inline implementations, and group
methods from the same classes together.

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:220: channel_id_ =
CastIPCDispatcher::global_instance_->id_map_.Add(this);
Don't access internal state like this. Use public methods.

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
File chrome/renderer/media/cast_transport_sender_ipc.h (right):

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
nit: s/(c) 2013/2014/

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.h:12:
media::cast::transport::CastTransportSender* NewCastTransportSenderIPC(
There is only one implementation for each of these factory methods which calls
into question their utility. Please change the code to use constructors on the
actual objects instead of hiding behind these method signatures. You can always
add factory methods when there is more than one implementation.

[hubbe, 2014-02-12 00:54:23]

PTAL

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

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2014/02/11 00:57:38, acolwell wrote:
> nit: s/(c) 2013/2014/

Done.

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:63:
media::cast::transport::CastTransportSender *sender =
On 2014/02/11 00:57:38, acolwell wrote:
> nit: * should be next to the type not the variable name.

Done.

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:77:
sender->SetPacketReceiver(
On 2014/02/11 00:57:38, acolwell wrote:
> Why aren't these passed into the CreateCastTransportSender()?

Good question, when I asked the same thing, the answer was because the object
they go to (CastSender) doesn't exist yet...

I think it should be fixed, but it is probably outside the scope of this CL.

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:95: void
CastTransportHostFilter::OnDelete(
On 2014/02/11 00:50:04, Chromium Palmer wrote:
> Nit: This could fit on one line.

Done.

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:97:
media::cast::transport::CastTransportSender *sender =
On 2014/02/11 00:57:38, acolwell wrote:
> nit: * should be next to the type not the variable name.

Done.

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:102: LOG(ERROR) <<
"CastTransportHostFilter::Delete called "
On 2014/02/11 00:57:38, acolwell wrote:
> nit: s/LOG(ERROR)/DVLOG(1)/ here and elsewhere. It is unlikely these errors
> really need to be displayed in release builds.

Done.

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:111:
media::cast::transport::CastTransportSender *sender =
On 2014/02/11 00:57:38, acolwell wrote:
> ditto

Done.

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:126:
media::cast::transport::CastTransportSender *sender =
On 2014/02/11 00:57:38, acolwell wrote:
> ditto

Done.

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:143:
media::cast::transport::CastTransportSender *sender =
On 2014/02/11 00:57:38, acolwell wrote:
> ditto

Done.

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:163:
media::cast::transport::CastTransportSender *sender =
On 2014/02/11 00:57:38, acolwell wrote:
> ditto

Done.

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
File chrome/browser/media/cast_transport_host_filter.h (right):

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2014/02/11 00:57:38, acolwell wrote:
> nit: s/(c) 2013/2014/

Done.

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
File chrome/browser/media/cast_transport_host_filter_unittest.cc (right):

https://codereview.chromium.org/138753004/diff/1360001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter_unittest.cc:50: // Now do it
wrong.
On 2014/02/11 00:57:38, acolwell wrote:
> nit: Please make the comment a little more explicit about why this is wrong.
Are
> we testing that only one object is allowed at a time or are we verifying that
> multiple objects with the same channel_id can't be new & deleted.
> 
> Is this just intended to be a "we didn't crash and we didn't leak memory" test
> case?

Done.

https://codereview.chromium.org/138753004/diff/1360001/chrome/common/cast_mes...
File chrome/common/cast_messages.h (right):

https://codereview.chromium.org/138753004/diff/1360001/chrome/common/cast_mes...
chrome/common/cast_messages.h:1: // Copyright (c) 2013 The Chromium Authors. All
rights reserved.
On 2014/02/11 00:50:04, Chromium Palmer wrote:
> 2014

Done.

https://codereview.chromium.org/138753004/diff/1360001/chrome/common/cast_mes...
chrome/common/cast_messages.h:1: // Copyright (c) 2013 The Chromium Authors. All
rights reserved.
On 2014/02/11 00:57:38, acolwell wrote:
> nit: s/(c) 2013/2014/

Done.

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
File chrome/renderer/media/cast_session.h (right):

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_session.h:36: class CastIPCDispatcher;
On 2014/02/11 00:57:38, acolwell wrote:
> remove. It doesn't look like this is necessary

Gone.

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_session_delegate.cc:103:
base::Bind(&IgnoreStatusCB)));
On 2014/02/11 00:57:38, acolwell wrote:
> Remove the callback from the method signature until you are actually ready to
> add code to handle the callback.

But if I do that, I still need an IgnoreStatusCB lower down.
Or, I could change it all the way to remove the status message from
cast_messages.h, or I could go all the way to the cast library and remove the
callback....

I'm going to argue that the right way to do this is to stop calling the function
IgnoreStatusCB, because that's not actually my intention. Instead I'm calling it
StatusNotificationCB and adding a todo for handling the callback.

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

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2014/02/11 00:57:38, acolwell wrote:
> nit: s/(c) //

Done.

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:20: // renderer less direct
control over the UDP sockets.
On 2014/02/11 00:50:04, Chromium Palmer wrote:
> Well, only honest renderers. Compromised renderers don't have to go through
this
> filtering layer, they can say whatever they want to the browser via the given
> IPC channels.
> 
> 
> How abstract are the IPC messages? Forgive me, but I'm unfamiliar with this
> code; how much control over the exact packet headers/contents do renderers
have?

Well, a compromised renderer can possibly use p2p socket or the pepper api
sockets
to circumvent these filters, but those should hopefully have their own security
solutions. (p2p socket requires that the STUN protocol is used for instance.)

The IPC messages are on the level of "here is an encoded frame", which are then
chopped up into packets and encrypted by the browser.

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:63: data.c_name =
data.c_name;
On 2014/02/11 00:50:04, Chromium Palmer wrote:
> Bug? Typo? Should be data.c_name = c_name?

Done.

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:93: friend class
CastIPCDispatcher;
On 2014/02/11 00:57:38, acolwell wrote:
> Do not use friend. Just have the the IPCDispatcher call public methods on this
> object. That way all code that accesses the internal state of this object is
in
> this class.

Done.

PS:

I really don't like having code like:

foo() { x->foo(); }
bar() { x->bar(); }
gazonk() { x->gazonk(); }
etc.etc.

I really wish I could decode the "channel" part of the IPC message, route the
message to the right object and then do the OnMessageReceived() part there....
Is that possible?

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:94: void Send(IPC::Message
*message);
On 2014/02/11 00:57:38, acolwell wrote:
> nit: s/ */* /

Done.

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:152: if (ptr) {
On 2014/02/11 00:57:38, acolwell wrote:
> nit: Everything in this block should be in a CastTransportSenderIPC method.

Done.

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:173:
ptr->status_callback_.Run(status);
On 2014/02/11 00:57:38, acolwell wrote:
> Please remove this an any related IPC code until you actually have code that
> responds to it. There is no point in sending the messages if no one is
> listening.

If I do that I'll need a separate security review when I add another message.
Seems counter-productive.

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:186: if (ptr) {
On 2014/02/11 00:57:38, acolwell wrote:
> ditto

Done.

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:196: friend class
CastTransportSenderIPC;
On 2014/02/11 00:57:38, acolwell wrote:
> Remove friend declaration

Done.

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:215:
CastTransportSenderIPC::CastTransportSenderIPC(
On 2014/02/11 00:57:38, acolwell wrote:
> Put class declarations in the .h, remove inline implementations, and group
> methods from the same classes together.

Done.

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:220: channel_id_ =
CastIPCDispatcher::global_instance_->id_map_.Add(this);
On 2014/02/11 00:57:38, acolwell wrote:
> Don't access internal state like this. Use public methods.

Done.

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
File chrome/renderer/media/cast_transport_sender_ipc.h (right):

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2014/02/11 00:57:38, acolwell wrote:
> nit: s/(c) 2013/2014/

Done.

https://codereview.chromium.org/138753004/diff/1360001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.h:12:
media::cast::transport::CastTransportSender* NewCastTransportSenderIPC(
On 2014/02/11 00:57:38, acolwell wrote:
> There is only one implementation for each of these factory methods which calls
> into question their utility. Please change the code to use constructors on the
> actual objects instead of hiding behind these method signatures. You can
always
> add factory methods when there is more than one implementation.

The point of using factory method is to speed up compilation.
Anyways, I've changed it use more common chrome parlance.
In conjunction with removing the friendships, I now have to write each
method name 4 times. But it should look like chrome code now.

https://codereview.chromium.org/138753004/diff/1360001/media/cast/transport/c...
File media/cast/transport/cast_transport_config.h (right):

https://codereview.chromium.org/138753004/diff/1360001/media/cast/transport/c...
media/cast/transport/cast_transport_config.h:165: std::string c_name;
On 2014/02/11 00:50:04, Chromium Palmer wrote:
> What is the intended structure of this string?
> 
> Can the recipient validate that structure somehow?

Apparently, the CNAME comes from RFC 3550 page 46, and should be on the form
user@host or just host if no user is available. It is generally treated as an
opaque identifier and is never decoded anywhere.

[acolwell GONE FROM CHROMIUM, 2014-02-13 19:28:57]

lgtm % nits

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

https://codereview.chromium.org/138753004/diff/1920001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
nit: ditto

https://codereview.chromium.org/138753004/diff/1920001/chrome/browser/media/c...
File chrome/browser/media/cast_transport_host_filter.h (right):

https://codereview.chromium.org/138753004/diff/1920001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
nit: Remove "(c) ". New files aren't supposed to have it.

https://codereview.chromium.org/138753004/diff/1920001/chrome/browser/media/c...
File chrome/browser/media/cast_transport_host_filter_unittest.cc (right):

https://codereview.chromium.org/138753004/diff/1920001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter_unittest.cc:83: CastHostMsg_New
new_msg(42, config);
nit: Please declare const int kChannelId = 42 here and use the constant name
below. That makes it easier to quicky determine the significance of this value.

https://codereview.chromium.org/138753004/diff/1920001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter_unittest.cc:90: audio_frame.data
= std::string(47 * 2 * 2, 'q');
nit: This is an odd number. Please add a comment indicating its significance.

https://codereview.chromium.org/138753004/diff/1920001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter_unittest.cc:100:
video_frame.data = std::string(4711, 'p');
ditto

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
File chrome/renderer/media/cast_ipc_dispatcher.cc (right):

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
chrome/renderer/media/cast_ipc_dispatcher.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
nit: Remove "(c) ". New files are not supposed to have it.

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
chrome/renderer/media/cast_ipc_dispatcher.cc:11: CastIPCDispatcher*
CastIPCDispatcher::global_instance_ = NULL;
This global static makes me nervous. Is this how other IPCDispatchers in the
code base address the problem you are running into?

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
chrome/renderer/media/cast_ipc_dispatcher.cc:77: CastTransportSenderIPC* ptr =
id_map_.Lookup(channel_id);
nit: s/ptr/sender/ here and below just to make the name a little more
descriptive.

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
File chrome/renderer/media/cast_ipc_dispatcher.h (right):

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
chrome/renderer/media/cast_ipc_dispatcher.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
nit: s/(c) 2013/2014/

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
chrome/renderer/media/cast_session_delegate.cc:126: // TODO(hubbe): Status
callback is not supposed to be ignored.
nit: Remove comment since you have a TODO in StatusNotificationCB?

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

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:14: 
nit: I believe this should be inside a namespace.

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:17: const
media::cast::transport::CastTransportStatusCallback& status_cb)
Technically this class doesn't need to know about the dispatcher. If you passed
in a channel_id, a callback for Send, a Closure for remove, and used a
WeakPtrFactory in the dispatcher, this code wouldn't need to know any details
about dispatcher lifetime and the dispatcher would not need to expose public
methods. If a factory method was added to the dispatcher to create these
objects, then the rest of the code wouldn't need to know this class even exists.


I'll leave it up to you if you'd like to make this change. It would provide a
better data hiding like you were trying to achieve before.

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
File chrome/renderer/media/cast_transport_sender_ipc.h (right):

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
nit: Remove "(c) ".

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.h:11: 
nit: I believe this needs to be inside a namespace.

https://codereview.chromium.org/138753004/diff/1920001/media/cast/transport/c...
File media/cast/transport/cast_transport_sender.h (right):

https://codereview.chromium.org/138753004/diff/1920001/media/cast/transport/c...
media/cast/transport/cast_transport_sender.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
nit: You shouldn't need to update this, since it isn't a new file.

[palmer, 2014-02-13 20:27:44]

"""
> What is the intended structure of this string?
> 
> Can the recipient validate that structure somehow?

Apparently, the CNAME comes from RFC 3550 page 46, and should be on the form
user@host or just host if no user is available. It is generally treated as an
opaque identifier and is never decoded anywhere.
"""

OK, that sounds good. How about hashing the string (with e.g. SHA-1) then?
Benefits: Fixed size; and enforces the "is opaque" property (so it won't be
decoded/parsed later, by other code, by someone else, in 6 months).

[hubbe, 2014-02-13 23:33:47]

On 2014/02/13 20:27:44, Chromium Palmer wrote:
> """
> > What is the intended structure of this string?
> > 
> > Can the recipient validate that structure somehow?
> 
> Apparently, the CNAME comes from RFC 3550 page 46, and should be on the form
> user@host or just host if no user is available. It is generally treated as an
> opaque identifier and is never decoded anywhere.
> """
> 
> OK, that sounds good. How about hashing the string (with e.g. SHA-1) then?
> Benefits: Fixed size; and enforces the "is opaque" property (so it won't be
> decoded/parsed later, by other code, by someone else, in 6 months).

Actually, Let me rephrase: We generate this string and send it out, we don't
actually know
what the other side does with it. The RFC says we SHOULD generate the string on
a particular
format, so we should probably stick with that.

[hubbe, 2014-02-14 00:03:22]

PTAL

(Having some trouble with uploads, let me know if anything seems to be missing.)

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

https://codereview.chromium.org/138753004/diff/1920001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2014/02/13 19:28:58, acolwell wrote:
> nit: ditto

Done.

https://codereview.chromium.org/138753004/diff/1920001/chrome/browser/media/c...
File chrome/browser/media/cast_transport_host_filter.h (right):

https://codereview.chromium.org/138753004/diff/1920001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2014/02/13 19:28:58, acolwell wrote:
> nit: Remove "(c) ". New files aren't supposed to have it.

Done.

https://codereview.chromium.org/138753004/diff/1920001/chrome/browser/media/c...
File chrome/browser/media/cast_transport_host_filter_unittest.cc (right):

https://codereview.chromium.org/138753004/diff/1920001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter_unittest.cc:83: CastHostMsg_New
new_msg(42, config);
On 2014/02/13 19:28:58, acolwell wrote:
> nit: Please declare const int kChannelId = 42 here and use the constant name
> below. That makes it easier to quicky determine the significance of this
value.

Done.

https://codereview.chromium.org/138753004/diff/1920001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter_unittest.cc:90: audio_frame.data
= std::string(47 * 2 * 2, 'q');
On 2014/02/13 19:28:58, acolwell wrote:
> nit: This is an odd number. Please add a comment indicating its significance.

Used constants to clarify.

https://codereview.chromium.org/138753004/diff/1920001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter_unittest.cc:100:
video_frame.data = std::string(4711, 'p');
On 2014/02/13 19:28:58, acolwell wrote:
> ditto

Done.

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
File chrome/renderer/media/cast_ipc_dispatcher.cc (right):

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
chrome/renderer/media/cast_ipc_dispatcher.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2014/02/13 19:28:58, acolwell wrote:
> nit: Remove "(c) ". New files are not supposed to have it.

Done.

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
chrome/renderer/media/cast_ipc_dispatcher.cc:11: CastIPCDispatcher*
CastIPCDispatcher::global_instance_ = NULL;
On 2014/02/13 19:28:58, acolwell wrote:
> This global static makes me nervous. Is this how other IPCDispatchers in the
> code base address the problem you are running into?

No, most filters use RenderThread::Current()->get_something() but
RenderThread::Current() doesn't work on the IO thread, and it just seemed really
redundant to keep a pointer to the ipc dispatcher in each
CastTransportSenderIPC.

I'm open to other suggestions.

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
chrome/renderer/media/cast_ipc_dispatcher.cc:77: CastTransportSenderIPC* ptr =
id_map_.Lookup(channel_id);
On 2014/02/13 19:28:58, acolwell wrote:
> nit: s/ptr/sender/ here and below just to make the name a little more
> descriptive.

Done.

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
File chrome/renderer/media/cast_ipc_dispatcher.h (right):

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
chrome/renderer/media/cast_ipc_dispatcher.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2014/02/13 19:28:58, acolwell wrote:
> nit: s/(c) 2013/2014/

Done.

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
chrome/renderer/media/cast_session_delegate.cc:126: // TODO(hubbe): Status
callback is not supposed to be ignored.
On 2014/02/13 19:28:58, acolwell wrote:
> nit: Remove comment since you have a TODO in StatusNotificationCB?

Done.

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

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:14: 
On 2014/02/13 19:28:58, acolwell wrote:
> nit: I believe this should be inside a namespace.

It was, scherkus said not to, see:

https://codereview.chromium.org/138753004/diff/700001/chrome/renderer/media/c...

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:17: const
media::cast::transport::CastTransportStatusCallback& status_cb)
On 2014/02/13 19:28:58, acolwell wrote:
> Technically this class doesn't need to know about the dispatcher. If you
passed
> in a channel_id, a callback for Send, a Closure for remove, and used a
> WeakPtrFactory in the dispatcher, this code wouldn't need to know any details
> about dispatcher lifetime and the dispatcher would not need to expose public
> methods. If a factory method was added to the dispatcher to create these
> objects, then the rest of the code wouldn't need to know this class even
exists.
> 
> 
> I'll leave it up to you if you'd like to make this change. It would provide a
> better data hiding like you were trying to achieve before.

I think I'll pass. It doesn't seem to add that much I think.

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
File chrome/renderer/media/cast_transport_sender_ipc.h (right):

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2014/02/13 19:28:58, acolwell wrote:
> nit: Remove "(c) ".

Done.

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.h:11: 
On 2014/02/13 19:28:58, acolwell wrote:
> nit: I believe this needs to be inside a namespace.

See other comment.

https://codereview.chromium.org/138753004/diff/1920001/media/cast/transport/c...
File media/cast/transport/cast_transport_sender.h (right):

https://codereview.chromium.org/138753004/diff/1920001/media/cast/transport/c...
media/cast/transport/cast_transport_sender.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2014/02/13 19:28:58, acolwell wrote:
> nit: You shouldn't need to update this, since it isn't a new file.

Ops, fixed.

[acolwell GONE FROM CHROMIUM, 2014-02-14 01:47:35]

please reupload the patch. The base files don't appear to be present.

[acolwell GONE FROM CHROMIUM, 2014-02-14 02:17:17]

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
File chrome/renderer/media/cast_ipc_dispatcher.cc (right):

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
chrome/renderer/media/cast_ipc_dispatcher.cc:11: CastIPCDispatcher*
CastIPCDispatcher::global_instance_ = NULL;
On 2014/02/14 00:03:24, hubbe wrote:
> On 2014/02/13 19:28:58, acolwell wrote:
> > This global static makes me nervous. Is this how other IPCDispatchers in the
> > code base address the problem you are running into?
> 
> No, most filters use RenderThread::Current()->get_something() but
> RenderThread::Current() doesn't work on the IO thread, and it just seemed
really
> redundant to keep a pointer to the ipc dispatcher in each
> CastTransportSenderIPC.
> 
> I'm open to other suggestions.

This response actually makes me more nervous.

not lgtm. After looking at the code again it appears that you are accessing
|global_instance_| from multiple threads w/o a lock. IIUC the dispatcher is
created and destroyed on the renderer thread but calls to Add/RemoveSender and
Send() are occuring on the IO thread. I believe this means you have a race on
dispatcher destruction here.

I'm surprised that there isn't another example in the codebase that you could
derive inspiration from. Perhaps someone else on the reviewer list more familiar
w/ IPC techniques could point you in the right direction.

[hubbe, 2014-02-14 19:34:45]

PTAL

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
File chrome/renderer/media/cast_ipc_dispatcher.cc (right):

https://codereview.chromium.org/138753004/diff/1920001/chrome/renderer/media/...
chrome/renderer/media/cast_ipc_dispatcher.cc:11: CastIPCDispatcher*
CastIPCDispatcher::global_instance_ = NULL;
On 2014/02/14 02:17:18, acolwell wrote:
> On 2014/02/14 00:03:24, hubbe wrote:
> > On 2014/02/13 19:28:58, acolwell wrote:
> > > This global static makes me nervous. Is this how other IPCDispatchers in
the
> > > code base address the problem you are running into?
> > 
> > No, most filters use RenderThread::Current()->get_something() but
> > RenderThread::Current() doesn't work on the IO thread, and it just seemed
> really
> > redundant to keep a pointer to the ipc dispatcher in each
> > CastTransportSenderIPC.
> > 
> > I'm open to other suggestions.
> 
> This response actually makes me more nervous.
> 
> not lgtm. After looking at the code again it appears that you are accessing
> |global_instance_| from multiple threads w/o a lock. IIUC the dispatcher is
> created and destroyed on the renderer thread but calls to Add/RemoveSender and
> Send() are occuring on the IO thread. I believe this means you have a race on
> dispatcher destruction here.
> 
> I'm surprised that there isn't another example in the codebase that you could
> derive inspiration from. Perhaps someone else on the reviewer list more
familiar
> w/ IPC techniques could point you in the right direction.
> 
> 

I thought about it and changed it so that the global variable is handled in the
safe way.
It is now only accessed from the IO thread (except in one case, see the code)

[acolwell GONE FROM CHROMIUM, 2014-02-14 20:45:19]

Almost there.

https://codereview.chromium.org/138753004/diff/2230001/chrome/renderer/media/...
File chrome/renderer/media/cast_ipc_dispatcher.cc (right):

https://codereview.chromium.org/138753004/diff/2230001/chrome/renderer/media/...
chrome/renderer/media/cast_ipc_dispatcher.cc:26: // Option 2: Something went
wrong before we got added as a filter, and
How are we supposed to hit this case? The pointer only gets set when the filter
is added right?

https://codereview.chromium.org/138753004/diff/2230001/chrome/renderer/media/...
chrome/renderer/media/cast_ipc_dispatcher.cc:29: global_instance_ = NULL;
This still global pointer business still feels wrong. Please just put a
scoped_refptr in the Senders so you are guaranteed that this object outlives the
senders. It isn't redundant. It properly reflects the shared ownership of this
object.

[hubbe, 2014-02-14 20:58:05]

https://codereview.chromium.org/138753004/diff/2230001/chrome/renderer/media/...
File chrome/renderer/media/cast_ipc_dispatcher.cc (right):

https://codereview.chromium.org/138753004/diff/2230001/chrome/renderer/media/...
chrome/renderer/media/cast_ipc_dispatcher.cc:26: // Option 2: Something went
wrong before we got added as a filter, and
On 2014/02/14 20:45:20, acolwell wrote:
> How are we supposed to hit this case? The pointer only gets set when the
filter
> is added right?

Actually, the more I think about it, the more I think this case can't happen.
When the object is created, it immediately posted onto the IO thread, and if
something went wrong, the ref will be freed on the IO thread.

https://codereview.chromium.org/138753004/diff/2230001/chrome/renderer/media/...
chrome/renderer/media/cast_ipc_dispatcher.cc:29: global_instance_ = NULL;
On 2014/02/14 20:45:20, acolwell wrote:
> This still global pointer business still feels wrong. Please just put a
> scoped_refptr in the Senders so you are guaranteed that this object outlives
the
> senders. It isn't redundant. It properly reflects the shared ownership of this
> object.

I can do that, but I'm not sure that solves anything, because I still have to
*find* the CastIPCDispatcher somehow. It just moves the problem to the
CastTransportSenderIPC constructor, doesn't it?

[acolwell GONE FROM CHROMIUM, 2014-02-14 23:09:26]

After our offline conversation, I'm fine with these changes. lgtm

https://codereview.chromium.org/138753004/diff/2560001/chrome/renderer/media/...
File chrome/renderer/media/cast_ipc_dispatcher.cc (right):

https://codereview.chromium.org/138753004/diff/2560001/chrome/renderer/media/...
chrome/renderer/media/cast_ipc_dispatcher.cc:23: global_instance_ = NULL;
nit: I believe you should be able to change this to a DCHECK(!global_instance_)
because I'm pretty sure if you get an OnFilterAdded() call, you'll get a
OnFilterRemoved() call.

[hubbe, 2014-02-14 23:13:09]

Palmer, you're up...

https://codereview.chromium.org/138753004/diff/2560001/chrome/renderer/media/...
File chrome/renderer/media/cast_ipc_dispatcher.cc (right):

https://codereview.chromium.org/138753004/diff/2560001/chrome/renderer/media/...
chrome/renderer/media/cast_ipc_dispatcher.cc:23: global_instance_ = NULL;
On 2014/02/14 23:09:28, acolwell wrote:
> nit: I believe you should be able to change this to a
DCHECK(!global_instance_)
> because I'm pretty sure if you get an OnFilterAdded() call, you'll get a
> OnFilterRemoved() call.

Done.

[hubbe, 2014-02-14 23:39:56]

https://codereview.chromium.org/138753004/diff/2560001/chrome/renderer/media/...
File chrome/renderer/media/cast_ipc_dispatcher.cc (right):

https://codereview.chromium.org/138753004/diff/2560001/chrome/renderer/media/...
chrome/renderer/media/cast_ipc_dispatcher.cc:23: global_instance_ = NULL;
On 2014/02/14 23:09:28, acolwell wrote:
> nit: I believe you should be able to change this to a
DCHECK(!global_instance_)
> because I'm pretty sure if you get an OnFilterAdded() call, you'll get a
> OnFilterRemoved() call.

Actually, that seems to be not true, at least in my tests....
Putting it back to global_instance_ = NULL;

[palmer, 2014-02-14 23:52:59]

IPC security LGTM but you might want to avoid code duplication before committing
(see comment).

https://codereview.chromium.org/138753004/diff/2610001/media/cast/transport/r...
File media/cast/transport/rtcp/rtcp_builder.cc (right):

https://codereview.chromium.org/138753004/diff/2610001/media/cast/transport/r...
media/cast/transport/rtcp/rtcp_builder.cc:103: if (start_size + 12 +
c_name_.length() > kMaxIpPacketSize) return false;
To avoid duplicating the check logic, why not do:

if (start_size + 12 + c_name_.length() > kMaxIpPacketSize) {
  DCHECK() << "Not enough buffer space";
  return false;
}

[hubbe, 2014-02-15 00:00:17]

https://codereview.chromium.org/138753004/diff/2610001/media/cast/transport/r...
File media/cast/transport/rtcp/rtcp_builder.cc (right):

https://codereview.chromium.org/138753004/diff/2610001/media/cast/transport/r...
media/cast/transport/rtcp/rtcp_builder.cc:103: if (start_size + 12 +
c_name_.length() > kMaxIpPacketSize) return false;
On 2014/02/14 23:53:01, Chromium Palmer wrote:
> To avoid duplicating the check logic, why not do:
> 
> if (start_size + 12 + c_name_.length() > kMaxIpPacketSize) {
>   DCHECK() << "Not enough buffer space";
>   return false;
> }

Good idea (except DCHECK() doesn't work, but DLOG(FATAL) does)

[hubbe, 2014-02-15 00:00:39]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-02-15 00:17:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/138753004/2650001

[hubbe, 2014-02-15 00:19:27]

The CQ bit was unchecked by hubbe@chromium.org

[hubbe, 2014-02-15 00:19:27]

The CQ bit was checked by hubbe@chromium.org

[acolwell GONE FROM CHROMIUM, 2014-02-15 00:47:53]

On 2014/02/14 23:39:56, hubbe wrote:
>
https://codereview.chromium.org/138753004/diff/2560001/chrome/renderer/media/...
> File chrome/renderer/media/cast_ipc_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/138753004/diff/2560001/chrome/renderer/media/...
> chrome/renderer/media/cast_ipc_dispatcher.cc:23: global_instance_ = NULL;
> On 2014/02/14 23:09:28, acolwell wrote:
> > nit: I believe you should be able to change this to a
> DCHECK(!global_instance_)
> > because I'm pretty sure if you get an OnFilterAdded() call, you'll get a
> > OnFilterRemoved() call.
> 
> Actually, that seems to be not true, at least in my tests....
> Putting it back to global_instance_ = NULL;

It would be good to understand exactly why OnFilterRemoved() isn't getting
called. We might be missing something important. Once the filter is added,
doesn't the RenderThreadImpl's channel_ essentially own the filter at this
point? It looks like when the channel_'s destructor runs it would cause
ChannelProxy::Close() to occur, which I believe ultimately should cause
OnFilterRemoved() to get called. What is actually happening that is allowing the
dispatcher to get destroyed w/o OnFilterRemoved() being called?

[hubbe, 2014-02-15 00:50:48]

On 2014/02/15 00:47:53, acolwell wrote:
> On 2014/02/14 23:39:56, hubbe wrote:
> >
>
https://codereview.chromium.org/138753004/diff/2560001/chrome/renderer/media/...
> > File chrome/renderer/media/cast_ipc_dispatcher.cc (right):
> > 
> >
>
https://codereview.chromium.org/138753004/diff/2560001/chrome/renderer/media/...
> > chrome/renderer/media/cast_ipc_dispatcher.cc:23: global_instance_ = NULL;
> > On 2014/02/14 23:09:28, acolwell wrote:
> > > nit: I believe you should be able to change this to a
> > DCHECK(!global_instance_)
> > > because I'm pretty sure if you get an OnFilterAdded() call, you'll get a
> > > OnFilterRemoved() call.
> > 
> > Actually, that seems to be not true, at least in my tests....
> > Putting it back to global_instance_ = NULL;
> 
> It would be good to understand exactly why OnFilterRemoved() isn't getting
> called. We might be missing something important. Once the filter is added,
> doesn't the RenderThreadImpl's channel_ essentially own the filter at this
> point? It looks like when the channel_'s destructor runs it would cause
> ChannelProxy::Close() to occur, which I believe ultimately should cause
> OnFilterRemoved() to get called. What is actually happening that is allowing
the
> dispatcher to get destroyed w/o OnFilterRemoved() being called?


It looks like it happens in a test, where MockRenderThread owns the filter:

[26193:26193:0214/153625:169723404190:FATAL:cast_ipc_dispatcher.cc(23)] Check
failed: !global_instance_.
 [0x00000051a0c3] __interceptor_backtrace
 [0x00000d7f6c82] base::debug::StackTrace::StackTrace()
 [0x00000db06e19] logging::LogMessage::~LogMessage()
 [0x00000d2179a7] CastIPCDispatcher::~CastIPCDispatcher()
 [0x00000d217be2] CastIPCDispatcher::~CastIPCDispatcher()
 [0x00000cb8e02d] base::RefCountedThreadSafe<>::DeleteInternal()
 [0x00000cb8de2a] base::DefaultRefCountedThreadSafeTraits<>::Destruct()
 [0x00000cb8dca5] base::RefCountedThreadSafe<>::Release()
 [0x000019cb4a81] scoped_refptr<>::~scoped_refptr()
 [0x000019cc59fa] std::_Destroy<>()
 [0x000019cc5832] std::_Destroy_aux<>::__destroy<>()
 [0x000019cc55dc] std::_Destroy<>()
 [0x000019cc4ea4] std::_Destroy<>()
 [0x000019ce03c8] std::__cxx1998::vector<>::~vector()
 [0x000019cb1369] std::__debug::vector<>::~vector()
 [0x000040316438] content::MockRenderThread::~MockRenderThread()
 [0x00000d5830e5] ChromeMockRenderThread::~ChromeMockRenderThread()
 [0x00000d583272] ChromeMockRenderThread::~ChromeMockRenderThread()
 [0x00000407664d] base::DefaultDeleter<>::operator()()
 [0x0000403392c4] base::internal::scoped_ptr_impl<>::~scoped_ptr_impl()
 [0x00004033365a] scoped_ptr<>::~scoped_ptr()
 [0x0000403275de] content::RenderViewTest::~RenderViewTest()
 [0x000004075172] ChromeRenderViewTest::~ChromeRenderViewTest()
 [0x000003f05b7d]
chrome::CastSessionBrowserTest_CreateAndDestroy_Test::~CastSessionBrowserTest_CreateAndDestroy_Test()
 [0x000003f05d12]
chrome::CastSessionBrowserTest_CreateAndDestroy_Test::~CastSessionBrowserTest_CreateAndDestroy_Test()
 [0x000011b6e03d] testing::Test::DeleteSelf_()
 [0x000011c0975a] testing::internal::HandleSehExceptionsInMethodIfSupported<>()
 [0x000011b6cb36] testing::internal::HandleExceptionsInMethodIfSupported<>()
 [0x000011b386aa] testing::TestInfo::Run()
 [0x000011b3a967] testing::TestCase::Run()
 [0x000011b52631] testing::internal::UnitTestImpl::RunAllTests()
 [0x000011bc76aa] testing::internal::HandleSehExceptionsInMethodIfSupported<>()
 [0x000011b78c99] testing::internal::HandleExceptionsInMethodIfSupported<>()
 [0x000011b510a7] testing::UnitTest::Run()
 [0x00000e31b0b1] RUN_ALL_TESTS()
 [0x00000e3145b4] base::TestSuite::Run()
 [0x00000d5d5d43] (anonymous
namespace)::ChromeTestLauncherDelegate::RunTestSuite()
 [0x00004038daec] content::LaunchTests()
 [0x00000d5d5659] LaunchChromeTests()
 [0x000004074d16] main
 [0x7fcc57b2876d] __libc_start_main
 [0x0000005567fd] <unknown>

[acolwell GONE FROM CHROMIUM, 2014-02-15 02:36:32]

On 2014/02/15 00:50:48, hubbe wrote:
> On 2014/02/15 00:47:53, acolwell wrote:
> > On 2014/02/14 23:39:56, hubbe wrote:
> > >
> >
>
https://codereview.chromium.org/138753004/diff/2560001/chrome/renderer/media/...
> > > File chrome/renderer/media/cast_ipc_dispatcher.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/138753004/diff/2560001/chrome/renderer/media/...
> > > chrome/renderer/media/cast_ipc_dispatcher.cc:23: global_instance_ = NULL;
> > > On 2014/02/14 23:09:28, acolwell wrote:
> > > > nit: I believe you should be able to change this to a
> > > DCHECK(!global_instance_)
> > > > because I'm pretty sure if you get an OnFilterAdded() call, you'll get a
> > > > OnFilterRemoved() call.
> > > 
> > > Actually, that seems to be not true, at least in my tests....
> > > Putting it back to global_instance_ = NULL;
> > 
> > It would be good to understand exactly why OnFilterRemoved() isn't getting
> > called. We might be missing something important. Once the filter is added,
> > doesn't the RenderThreadImpl's channel_ essentially own the filter at this
> > point? It looks like when the channel_'s destructor runs it would cause
> > ChannelProxy::Close() to occur, which I believe ultimately should cause
> > OnFilterRemoved() to get called. What is actually happening that is allowing
> the
> > dispatcher to get destroyed w/o OnFilterRemoved() being called?
> 
> 
> It looks like it happens in a test, where MockRenderThread owns the filter:
> 
> [26193:26193:0214/153625:169723404190:FATAL:cast_ipc_dispatcher.cc(23)] Check
> failed: !global_instance_.
>  [0x00000051a0c3] __interceptor_backtrace
>  [0x00000d7f6c82] base::debug::StackTrace::StackTrace()
>  [0x00000db06e19] logging::LogMessage::~LogMessage()
>  [0x00000d2179a7] CastIPCDispatcher::~CastIPCDispatcher()
>  [0x00000d217be2] CastIPCDispatcher::~CastIPCDispatcher()
>  [0x00000cb8e02d] base::RefCountedThreadSafe<>::DeleteInternal()
>  [0x00000cb8de2a] base::DefaultRefCountedThreadSafeTraits<>::Destruct()
>  [0x00000cb8dca5] base::RefCountedThreadSafe<>::Release()
>  [0x000019cb4a81] scoped_refptr<>::~scoped_refptr()
>  [0x000019cc59fa] std::_Destroy<>()
>  [0x000019cc5832] std::_Destroy_aux<>::__destroy<>()
>  [0x000019cc55dc] std::_Destroy<>()
>  [0x000019cc4ea4] std::_Destroy<>()
>  [0x000019ce03c8] std::__cxx1998::vector<>::~vector()
>  [0x000019cb1369] std::__debug::vector<>::~vector()
>  [0x000040316438] content::MockRenderThread::~MockRenderThread()
>  [0x00000d5830e5] ChromeMockRenderThread::~ChromeMockRenderThread()
>  [0x00000d583272] ChromeMockRenderThread::~ChromeMockRenderThread()
>  [0x00000407664d] base::DefaultDeleter<>::operator()()
>  [0x0000403392c4] base::internal::scoped_ptr_impl<>::~scoped_ptr_impl()
>  [0x00004033365a] scoped_ptr<>::~scoped_ptr()
>  [0x0000403275de] content::RenderViewTest::~RenderViewTest()
>  [0x000004075172] ChromeRenderViewTest::~ChromeRenderViewTest()
>  [0x000003f05b7d]
>
chrome::CastSessionBrowserTest_CreateAndDestroy_Test::~CastSessionBrowserTest_CreateAndDestroy_Test()
>  [0x000003f05d12]
>
chrome::CastSessionBrowserTest_CreateAndDestroy_Test::~CastSessionBrowserTest_CreateAndDestroy_Test()
>  [0x000011b6e03d] testing::Test::DeleteSelf_()
>  [0x000011c0975a]
testing::internal::HandleSehExceptionsInMethodIfSupported<>()
>  [0x000011b6cb36] testing::internal::HandleExceptionsInMethodIfSupported<>()
>  [0x000011b386aa] testing::TestInfo::Run()
>  [0x000011b3a967] testing::TestCase::Run()
>  [0x000011b52631] testing::internal::UnitTestImpl::RunAllTests()
>  [0x000011bc76aa]
testing::internal::HandleSehExceptionsInMethodIfSupported<>()
>  [0x000011b78c99] testing::internal::HandleExceptionsInMethodIfSupported<>()
>  [0x000011b510a7] testing::UnitTest::Run()
>  [0x00000e31b0b1] RUN_ALL_TESTS()
>  [0x00000e3145b4] base::TestSuite::Run()
>  [0x00000d5d5d43] (anonymous
> namespace)::ChromeTestLauncherDelegate::RunTestSuite()
>  [0x00004038daec] content::LaunchTests()
>  [0x00000d5d5659] LaunchChromeTests()
>  [0x000004074d16] main
>  [0x7fcc57b2876d] __libc_start_main
>  [0x0000005567fd] <unknown>

This looks like the MockRenderThread is not sufficiently emulating the real
RenderThread's behavior. We should probably fix the mock object instead of
making your code more permissive.

[commit-bot: I haz the power, 2014-02-15 04:17:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-15 04:17:21]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Alpha Left Google, 2014-02-15 18:31:05]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-02-15 18:31:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/138753004/2650001

[commit-bot: I haz the power, 2014-02-15 19:45:19]

Change committed as 251592