Refactored to make MDnsSocketFactory return array of sockets.

Refactored to make MDnsSocketFactory return array of sockets.
MDnsClinet will just listen and send packet to those sockets without knowing address family or interface.

BUG=297690
TBR=thestig, for c/tools/

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

[szym, 2013-11-26 20:37:31]

https://codereview.chromium.org/87693002/diff/1/net/dns/mdns_client_impl.cc
File net/dns/mdns_client_impl.cc (right):

https://codereview.chromium.org/87693002/diff/1/net/dns/mdns_client_impl.cc#n...
net/dns/mdns_client_impl.cc:205: return socket.Pass();
Needs PassAs to compile.

[Vitaly Buka (NO REVIEWS), 2013-11-26 20:52:29]

https://codereview.chromium.org/87693002/diff/1/net/dns/mdns_client_impl.cc
File net/dns/mdns_client_impl.cc (right):

https://codereview.chromium.org/87693002/diff/1/net/dns/mdns_client_impl.cc#n...
net/dns/mdns_client_impl.cc:205: return socket.Pass();
On 2013/11/26 20:37:31, szym wrote:
> Needs PassAs to compile.

Done.

[szym, 2013-11-27 00:28:08]

https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_impl.cc
File net/dns/mdns_client_impl.cc (right):

https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.cc:93: socket_handlers_.push_back(
Empty sockets (which failed to bind in CreateSockets) should be filtered out
here.

https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_impl.h
File net/dns/mdns_client_impl.h (right):

https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.h:30: class Socket {
When compared to the more direct route of using DatagramServerSocket, wrapping
UDPServerSocket in a SocketImpl adds one virtual dispatch. 

What's the benefit of going this way?

[Vitaly Buka (NO REVIEWS), 2013-11-27 00:45:44]

https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_impl.cc
File net/dns/mdns_client_impl.cc (right):

https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.cc:93: socket_handlers_.push_back(
CreateSockets should filter. That was initial idea, but somehow I missed that
part. i'll put that at line 191

On 2013/11/27 00:28:09, szym wrote:
> Empty sockets (which failed to bind in CreateSockets) should be filtered out
> here.

https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_impl.h
File net/dns/mdns_client_impl.h (right):

https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.h:30: class Socket {
1. Cleaner tests because no need to mock unnecessary functionality.
2. Maybe we will listen in browser, and handle in sandbox, so it will be easy to
have IPC proxy for this one then for entire DatagramServerSocket

3. And most important: To setup MDns with array of sockets, we need to keep also
IPEndPoint for each socket. So I need to have some wrapper anyway or have
CreateSockets(vector<pair<Socket, IPEndPoint>* sockets)
So if we go with wrapper, it should be cleaner this way.

On 2013/11/27 00:28:09, szym wrote:
> When compared to the more direct route of using DatagramServerSocket, wrapping
> UDPServerSocket in a SocketImpl adds one virtual dispatch. 
> 
> What's the benefit of going this way?

[Vitaly Buka (NO REVIEWS), 2013-11-27 00:48:02]

On 2013/11/27 00:45:44, Vitaly Buka wrote:
>
https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_impl.cc
> File net/dns/mdns_client_impl.cc (right):
> 
>
https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_impl....
> net/dns/mdns_client_impl.cc:93: socket_handlers_.push_back(
> CreateSockets should filter. That was initial idea, but somehow I missed that
> part. i'll put that at line 191
> 
> On 2013/11/27 00:28:09, szym wrote:
> > Empty sockets (which failed to bind in CreateSockets) should be filtered out
> > here.
> 
> https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_impl.h
> File net/dns/mdns_client_impl.h (right):
> 
>
https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_impl....
> net/dns/mdns_client_impl.h:30: class Socket {
> 1. Cleaner tests because no need to mock unnecessary functionality.
> 2. Maybe we will listen in browser, and handle in sandbox, so it will be easy
to
> have IPC proxy for this one then for entire DatagramServerSocket
> 
> 3. And most important: To setup MDns with array of sockets, we need to keep
also
> IPEndPoint for each socket. So I need to have some wrapper anyway or have
> CreateSockets(vector<pair<Socket, IPEndPoint>* sockets)
> So if we go with wrapper, it should be cleaner this way.
> 
> On 2013/11/27 00:28:09, szym wrote:
> > When compared to the more direct route of using DatagramServerSocket,
wrapping
> > UDPServerSocket in a SocketImpl adds one virtual dispatch. 
> > 
> > What's the benefit of going this way?

Also regarding this patch.
It looks like there is not value in SocketFactory here, maybe I should kill it
and just pass array of Sockets?

[szym, 2013-11-27 02:25:27]

https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_impl.h
File net/dns/mdns_client_impl.h (right):

https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.h:30: class Socket {
On 2013/11/27 00:45:45, Vitaly Buka wrote:
> 1. Cleaner tests because no need to mock unnecessary functionality.

True, but less _test_ code is not a reason to slow _down_ production code.

> 2. Maybe we will listen in browser, and handle in sandbox, so it will be easy
to
> have IPC proxy for this one then for entire DatagramServerSocket
> 
> 3. And most important: To setup MDns with array of sockets, we need to keep
also
> IPEndPoint for each socket. So I need to have some wrapper anyway or have
> CreateSockets(vector<pair<Socket, IPEndPoint>* sockets)
> So if we go with wrapper, it should be cleaner this way.

Aren't the Sockets already pre-bound by CreateSockets? If CreateSockets returned
pre-bound DatagramServerSockets, then there's no need to return IPEndPoint.

> 
> On 2013/11/27 00:28:09, szym wrote:
> > When compared to the more direct route of using DatagramServerSocket,
wrapping
> > UDPServerSocket in a SocketImpl adds one virtual dispatch. 
> > 
> > What's the benefit of going this way?
>

[Vitaly Buka (NO REVIEWS), 2013-11-27 06:29:31]

On 2013/11/27 02:25:27, szym wrote:
> https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_impl.h
> File net/dns/mdns_client_impl.h (right):
> 
>
https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_impl....
> net/dns/mdns_client_impl.h:30: class Socket {
> On 2013/11/27 00:45:45, Vitaly Buka wrote:
> > 1. Cleaner tests because no need to mock unnecessary functionality.
> 
> True, but less _test_ code is not a reason to slow _down_ production code.
It's not main reason. And it's just additions virtual call, per packet. Take
into account that we also have one IPC call per packet.

> 
> > 2. Maybe we will listen in browser, and handle in sandbox, so it will be
easy
> to
> > have IPC proxy for this one then for entire DatagramServerSocket

this "maybe" does not deserve attention.

> > 
> > 3. And most important: To setup MDns with array of sockets, we need to keep
> also
> > IPEndPoint for each socket. So I need to have some wrapper anyway or have
> > CreateSockets(vector<pair<Socket, IPEndPoint>* sockets)
> > So if we go with wrapper, it should be cleaner this way.
> 
> Aren't the Sockets already pre-bound by CreateSockets? If CreateSockets
returned
> pre-bound DatagramServerSockets, then there's no need to return IPEndPoint.

It's pre-bound, but SendTo needs same IPEndpoint so CreateSockets will have to
return it some how.
Actually we need just address family to restore IPEndpoint and this data exists
in UDP sockets on all platforms.
We just need to expose it in DatagramServerSockets.

> 
> > 
> > On 2013/11/27 00:28:09, szym wrote:
> > > When compared to the more direct route of using DatagramServerSocket,
> wrapping
> > > UDPServerSocket in a SocketImpl adds one virtual dispatch. 
> > > 
> > > What's the benefit of going this way?
> >

[Vitaly Buka (NO REVIEWS), 2013-11-27 06:35:42]

On 2013/11/27 06:29:31, Vitaly Buka wrote:
> On 2013/11/27 02:25:27, szym wrote:
> >
https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_impl.h
> > File net/dns/mdns_client_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_impl....
> > net/dns/mdns_client_impl.h:30: class Socket {
> > On 2013/11/27 00:45:45, Vitaly Buka wrote:
> > > 1. Cleaner tests because no need to mock unnecessary functionality.
> > 
> > True, but less _test_ code is not a reason to slow _down_ production code.
> It's not main reason. And it's just additions virtual call, per packet. Take
> into account that we also have one IPC call per packet.
> 
> > 
> > > 2. Maybe we will listen in browser, and handle in sandbox, so it will be
> easy
> > to
> > > have IPC proxy for this one then for entire DatagramServerSocket
> 
> this "maybe" does not deserve attention.
> 
> > > 
> > > 3. And most important: To setup MDns with array of sockets, we need to
keep
> > also
> > > IPEndPoint for each socket. So I need to have some wrapper anyway or have
> > > CreateSockets(vector<pair<Socket, IPEndPoint>* sockets)
> > > So if we go with wrapper, it should be cleaner this way.
> > 
> > Aren't the Sockets already pre-bound by CreateSockets? If CreateSockets
> returned
> > pre-bound DatagramServerSockets, then there's no need to return IPEndPoint.
> 
> It's pre-bound, but SendTo needs same IPEndpoint so CreateSockets will have to
> return it some how.
> Actually we need just address family to restore IPEndpoint and this data
exists
> in UDP sockets on all platforms.
> We just need to expose it in DatagramServerSockets.

Never mind, I can get address family from DatagramServerSockets::GetLocalAddress

> 
> > 
> > > 
> > > On 2013/11/27 00:28:09, szym wrote:
> > > > When compared to the more direct route of using DatagramServerSocket,
> > wrapping
> > > > UDPServerSocket in a SocketImpl adds one virtual dispatch. 
> > > > 
> > > > What's the benefit of going this way?
> > >

[szym, 2013-11-27 07:33:55]

On Tue, Nov 26, 2013 at 10:35 PM, <vitalybuka@chromium.org> wrote:

> On 2013/11/27 06:29:31, Vitaly Buka wrote:
>
>> On 2013/11/27 02:25:27, szym wrote:
>> >
>>
> https://codereview.chromium.org/87693002/diff/20001/net/
> dns/mdns_client_impl.h
>
>> > File net/dns/mdns_client_impl.h (right):
>> >
>> >
>>
>
> https://codereview.chromium.org/87693002/diff/20001/net/
> dns/mdns_client_impl.h#newcode30
>
>> > net/dns/mdns_client_impl.h:30: class Socket {
>> > On 2013/11/27 00:45:45, Vitaly Buka wrote:
>> > > 1. Cleaner tests because no need to mock unnecessary functionality.
>> >
>> > True, but less _test_ code is not a reason to slow _down_ production
>> code.
>> It's not main reason. And it's just additions virtual call, per packet.
>> Take
>> into account that we also have one IPC call per packet.
>>
>
>  >
>> > > 2. Maybe we will listen in browser, and handle in sandbox, so it will
>> be
>> easy
>> > to
>> > > have IPC proxy for this one then for entire DatagramServerSocket
>>
>
>  this "maybe" does not deserve attention.
>>
>
>  > >
>> > > 3. And most important: To setup MDns with array of sockets, we need to
>>
> keep
>
>> > also
>> > > IPEndPoint for each socket. So I need to have some wrapper anyway or
>> have
>> > > CreateSockets(vector<pair<Socket, IPEndPoint>* sockets)
>> > > So if we go with wrapper, it should be cleaner this way.
>> >
>> > Aren't the Sockets already pre-bound by CreateSockets? If CreateSockets
>> returned
>> > pre-bound DatagramServerSockets, then there's no need to return
>> IPEndPoint.
>>
>
>  It's pre-bound, but SendTo needs same IPEndpoint so CreateSockets will
>> have to
>> return it some how.
>> Actually we need just address family to restore IPEndpoint and this data
>>
> exists
>
>> in UDP sockets on all platforms.
>> We just need to expose it in DatagramServerSockets.
>>
>
> Never mind, I can get address family from DatagramServerSockets::
> GetLocalAddress
>
>
>
Ah, I missed the fact that you need to SendTo(address). A
DatagramClientSocket can be Connect'ed so that you don't need to remember
the destination address, but it seems DatagramServerSocket does not have
this.

GetLocalAddress is getsockname (a syscall) though, so at this point it'd
make sense to extend DataServerSocket, but that means you need the virtual
dispatch anyway.

Given that, I'd say the Socket interface is just fine.


>
>  >
>> > >
>> > > On 2013/11/27 00:28:09, szym wrote:
>> > > > When compared to the more direct route of using
>> DatagramServerSocket,
>> > wrapping
>> > > > UDPServerSocket in a SocketImpl adds one virtual dispatch.
>> > > >
>> > > > What's the benefit of going this way?
>> > >
>>
>
>
> https://codereview.chromium.org/87693002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[szym, 2013-11-27 07:37:44]

https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_impl.h
File net/dns/mdns_client_impl.h (right):

https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.h:38: virtual int SendTo(IOBuffer* buf,
Does not take an address, so this should be |Send| not |SendTo|.

https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_unitt...
File net/dns/mdns_client_unittest.cc (right):

https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_unitt...
net/dns/mdns_client_unittest.cc:1030: }
To avoid weird bugs, either sockets->clear() or DCHECK(sockets->empty()) at the
beginning of the function.

[Vitaly Buka (NO REVIEWS), 2013-11-27 08:45:37]

https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_impl.h
File net/dns/mdns_client_impl.h (right):

https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_impl....
net/dns/mdns_client_impl.h:38: virtual int SendTo(IOBuffer* buf,
This one does not, that was a point, make implementation to care about address.

https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_unitt...
File net/dns/mdns_client_unittest.cc (right):

https://codereview.chromium.org/87693002/diff/20001/net/dns/mdns_client_unitt...
net/dns/mdns_client_unittest.cc:1030: }
On 2013/11/27 07:37:44, szym wrote:
> To avoid weird bugs, either sockets->clear() or DCHECK(sockets->empty()) at
the
> beginning of the function.

Done.

[Vitaly Buka (NO REVIEWS), 2013-11-27 08:48:34]

In this patch I switched back to DatagramServerSockets

[szym, 2013-11-27 09:14:03]

lgtm

[commit-bot: I haz the power, 2013-11-27 20:04:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/87693002/160001

[commit-bot: I haz the power, 2013-11-28 01:31:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/87693002/160001

[commit-bot: I haz the power, 2013-11-28 02:07:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/87693002/160001

[commit-bot: I haz the power, 2013-11-28 02:37:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/87693002/160001

[commit-bot: I haz the power, 2013-11-28 03:06:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/87693002/160001

[commit-bot: I haz the power, 2013-11-28 03:33:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/87693002/160001

[commit-bot: I haz the power, 2013-11-28 03:57:25]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-11-29 21:48:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/87693002/160001

[commit-bot: I haz the power, 2013-11-29 22:05:33]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-12-02 00:07:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/87693002/160001

[commit-bot: I haz the power, 2013-12-02 06:26:33]

Change committed as 238042

[Lei Zhang, 2013-12-02 18:21:01]

lgtm