P2P <-> cast library integration v0.1

This CL enables the cast library to use the P2P socket library to send UDP packets.

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

[Alpha Left Google, 2013-11-09 00:30:18]

Looks pretty good.

Please add bug number 301920 and fill in more details in the CL description.

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_se...
File chrome/renderer/media/cast_session.h (right):

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_se...
chrome/renderer/media/cast_session.h:35: virtual content::P2PSocketClient*
create() = 0;
Return type should be scoped_ptr<T>.

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_se...
chrome/renderer/media/cast_session.h:45: void SetSocketFactory(SocketFactory*
socket_factory,
Use scoped_ptr<> for type of socket_factory. This means passing the ownership.

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_se...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_se...
chrome/renderer/media/cast_session_delegate.cc:44: if (!socket_factory_) {
I'd just do a DCHECK. We should never be here without a socket factory.

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_se...
chrome/renderer/media/cast_session_delegate.cc:111: // Ignored
nit: Period at the end of sentence.

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_se...
chrome/renderer/media/cast_session_delegate.cc:115: // TODO(hubbe): Report this
back to the user
nit: Period at the end of sentence.

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_se...
chrome/renderer/media/cast_session_delegate.cc:118: void
CastSessionDelegate::OnDataReceived(const net::IPEndPoint& address,
Should we verify the packet is coming from the right address?

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_se...
chrome/renderer/media/cast_session_delegate.cc:120: uint8 *packet_copy = new
uint8[data.size()];
Use scoped_ptr<uint8[]>.

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_se...
chrome/renderer/media/cast_session_delegate.cc:125: packet_copy));
You can do a base::Passed(packet_copy) if packet_copy is a scoped_ptr.

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_ud...
File chrome/renderer/media/cast_udp_transport.cc (right):

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_ud...
chrome/renderer/media/cast_udp_transport.cc:13: class CastUDPSocketFactory :
public CastSession::SocketFactory {
nit: CastUdpSocketFactory to be consistent with CastUdpTransport.

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_ud...
chrome/renderer/media/cast_udp_transport.cc:16: content::P2PSocketClient *socket
= content::CreateP2PSocket();
This should be scoped_ptr<content::P2PSocketClient>.

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_ud...
chrome/renderer/media/cast_udp_transport.cc:22: return socket;
return socket.Pass() if socket is a scoped_ptr.

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_ud...
chrome/renderer/media/cast_udp_transport.cc:33: void
CastUdpTransport::Start(const net::HostPortPair& remote_address) {
Might as well just change the type to net::IPAddressNumber in this change. It's
not a lot of affected code locations.

[hubbe, 2013-11-26 20:24:29]

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_se...
File chrome/renderer/media/cast_session.h (right):

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_se...
chrome/renderer/media/cast_session.h:35: virtual content::P2PSocketClient*
create() = 0;
On 2013/11/09 00:30:18, Alpha wrote:
> Return type should be scoped_ptr<T>.

scoped_refptr<>

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_se...
chrome/renderer/media/cast_session.h:45: void SetSocketFactory(SocketFactory*
socket_factory,
On 2013/11/09 00:30:18, Alpha wrote:
> Use scoped_ptr<> for type of socket_factory. This means passing the ownership.

Done.

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_se...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_se...
chrome/renderer/media/cast_session_delegate.cc:44: if (!socket_factory_) {
On 2013/11/09 00:30:18, Alpha wrote:
> I'd just do a DCHECK. We should never be here without a socket factory.

Isn't this function triggered by some javascript?
What prevents the user from calling it without a socket factory?

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_se...
chrome/renderer/media/cast_session_delegate.cc:111: // Ignored
On 2013/11/09 00:30:18, Alpha wrote:
> nit: Period at the end of sentence.

Done.

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_se...
chrome/renderer/media/cast_session_delegate.cc:115: // TODO(hubbe): Report this
back to the user
On 2013/11/09 00:30:18, Alpha wrote:
> nit: Period at the end of sentence.

Done.

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_se...
chrome/renderer/media/cast_session_delegate.cc:118: void
CastSessionDelegate::OnDataReceived(const net::IPEndPoint& address,
On 2013/11/09 00:30:18, Alpha wrote:
> Should we verify the packet is coming from the right address?

Good question.
Will each connection have its own socket? Or could they potentially be reused?
Also, perhaps we should be checking the destination in SendPacket as well.

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_se...
chrome/renderer/media/cast_session_delegate.cc:120: uint8 *packet_copy = new
uint8[data.size()];
On 2013/11/09 00:30:18, Alpha wrote:
> Use scoped_ptr<uint8[]>.

see below

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_se...
chrome/renderer/media/cast_session_delegate.cc:125: packet_copy));
On 2013/11/09 00:30:18, Alpha wrote:
> You can do a base::Passed(packet_copy) if packet_copy is a scoped_ptr. 

I'm not sure it's going to be that easy.
I also use packet_copy two lines up, and if I do:

   packet_copy.get(),,,,packet_copy.Pass()

without a semi-colon in between, then the call order is undefined and get() may
end up returning a null pointer.

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_ud...
File chrome/renderer/media/cast_udp_transport.cc (right):

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_ud...
chrome/renderer/media/cast_udp_transport.cc:13: class CastUDPSocketFactory :
public CastSession::SocketFactory {
On 2013/11/09 00:30:18, Alpha wrote:
> nit: CastUdpSocketFactory to be consistent with CastUdpTransport.

Done.

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_ud...
chrome/renderer/media/cast_udp_transport.cc:16: content::P2PSocketClient *socket
= content::CreateP2PSocket();
On 2013/11/09 00:30:18, Alpha wrote:
> This should be scoped_ptr<content::P2PSocketClient>.

scoped_refptr actually

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_ud...
chrome/renderer/media/cast_udp_transport.cc:22: return socket;
On 2013/11/09 00:30:18, Alpha wrote:
> return socket.Pass() if socket is a scoped_ptr.

using scoped_refptr, no need for Pass

https://codereview.chromium.org/66293003/diff/1/chrome/renderer/media/cast_ud...
chrome/renderer/media/cast_udp_transport.cc:33: void
CastUdpTransport::Start(const net::HostPortPair& remote_address) {
On 2013/11/09 00:30:18, Alpha wrote:
> Might as well just change the type to net::IPAddressNumber in this change.
It's
> not a lot of affected code locations.

Done.

[Alpha Left Google, 2013-12-03 23:03:44]

LGTM after merge.

https://codereview.chromium.org/66293003/diff/110001/chrome/renderer/extensio...
File chrome/renderer/extensions/webrtc_native_handler.cc (right):

https://codereview.chromium.org/66293003/diff/110001/chrome/renderer/extensio...
chrome/renderer/extensions/webrtc_native_handler.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
This file has been renamed to cast_streaming_native_handler.cc, please merge.

[hubbe, 2013-12-07 00:44:55]

Adding owners

[Sergey Ulanov, 2013-12-07 01:15:28]

lgtm with nits

https://codereview.chromium.org/66293003/diff/190001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_session.cc (right):

https://codereview.chromium.org/66293003/diff/190001/chrome/renderer/media/ca...
chrome/renderer/media/cast_session.cc:23:
CastSession::SocketFactory::~SocketFactory() {
nit: for abstract interfaces destructor can be declared inline.

https://codereview.chromium.org/66293003/diff/190001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_session.h (right):

https://codereview.chromium.org/66293003/diff/190001/chrome/renderer/media/ca...
chrome/renderer/media/cast_session.h:26: };
nit: remove ;, add comment as for other namespaces above

https://codereview.chromium.org/66293003/diff/190001/chrome/renderer/media/ca...
chrome/renderer/media/cast_session.h:41: class SocketFactory {
maybe P2PSocketFactory - these are not regular sockets.

https://codereview.chromium.org/66293003/diff/190001/chrome/renderer/media/ca...
chrome/renderer/media/cast_session.h:46: virtual
scoped_refptr<content::P2PSocketClient> create() = 0;
Create()

https://codereview.chromium.org/66293003/diff/190001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://codereview.chromium.org/66293003/diff/190001/chrome/renderer/media/ca...
chrome/renderer/media/cast_session_delegate.cc:126: // We don't support server
sockets.
Do you ever use TCP sockets for cast? If not, then I think it's better to say
here that this method is not expected to be called for UDP sockets.

[not at google - send to devlin, 2013-12-09 21:53:38]

extensions lgtm

[hubbe, 2013-12-10 18:25:57]

https://codereview.chromium.org/66293003/diff/190001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_session.cc (right):

https://codereview.chromium.org/66293003/diff/190001/chrome/renderer/media/ca...
chrome/renderer/media/cast_session.cc:23:
CastSession::SocketFactory::~SocketFactory() {
On 2013/12/07 01:15:29, Sergey Ulanov wrote:
> nit: for abstract interfaces destructor can be declared inline.

Has this changed recently?
One of the compilers complained last time I tried this.

https://codereview.chromium.org/66293003/diff/190001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_session.h (right):

https://codereview.chromium.org/66293003/diff/190001/chrome/renderer/media/ca...
chrome/renderer/media/cast_session.h:26: };
On 2013/12/07 01:15:29, Sergey Ulanov wrote:
> nit: remove ;, add comment as for other namespaces above

Done.

https://codereview.chromium.org/66293003/diff/190001/chrome/renderer/media/ca...
chrome/renderer/media/cast_session.h:41: class SocketFactory {
On 2013/12/07 01:15:29, Sergey Ulanov wrote:
> maybe P2PSocketFactory - these are not regular sockets.

Done.

https://codereview.chromium.org/66293003/diff/190001/chrome/renderer/media/ca...
chrome/renderer/media/cast_session.h:46: virtual
scoped_refptr<content::P2PSocketClient> create() = 0;
On 2013/12/07 01:15:29, Sergey Ulanov wrote:
> Create()

Done.

https://codereview.chromium.org/66293003/diff/190001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://codereview.chromium.org/66293003/diff/190001/chrome/renderer/media/ca...
chrome/renderer/media/cast_session_delegate.cc:126: // We don't support server
sockets.
On 2013/12/07 01:15:29, Sergey Ulanov wrote:
> Do you ever use TCP sockets for cast? If not, then I think it's better to say
> here that this method is not expected to be called for UDP sockets.

Done.

[hubbe, 2013-12-10 18:26:45]

Ping Joi, you're the only one left.

[Jói, 2013-12-10 19:29:01]

//content/public LGTM.

[commit-bot: I haz the power, 2013-12-10 20:33:42]

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

[commit-bot: I haz the power, 2013-12-10 22:29:19]

Change committed as 239891