Enqueue mDns requests if send on socket in progress.

Enqueue mDns requests if send on socket in progress.

BUG=415753
Committed: https://crrev.com/2085974efdf70fb9e1abf047ce59eb6cecd050ab
Cr-Commit-Position: refs/heads/master@{#296094}

[Vitaly Buka (NO REVIEWS), 2014-09-19 07:22:18]

vitalybuka@chromium.org changed reviewers:
+ sergeyu@chromium.org, wtc@chromium.org

[Sergey Ulanov, 2014-09-19 20:40:19]

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

https://codereview.chromium.org/581813004/diff/60001/net/dns/mdns_client_impl...
net/dns/mdns_client_impl.cc:122: send_in_progress_ = false;
|rv| is ignored here. I'm not sure how important it is to return correct result
from MDnsConnection::Send(). Maybe move error check from MDnsConnection::Send()
here and make  MDnsConnection::SocketHandler::Send() return void? Or maybe call
connection_->OnError() when rv != net::OK?
I'm fine with just logging here and adding TODO to fix it later.

[Vitaly Buka (NO REVIEWS), 2014-09-19 21:01:45]

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

https://codereview.chromium.org/581813004/diff/60001/net/dns/mdns_client_impl...
net/dns/mdns_client_impl.cc:122: send_in_progress_ = false;
On 2014/09/19 20:40:19, Sergey Ulanov wrote:
> |rv| is ignored here. I'm not sure how important it is to return correct
result
> from MDnsConnection::Send(). Maybe move error check from
MDnsConnection::Send()
> here and make  MDnsConnection::SocketHandler::Send() return void? Or maybe
call
> connection_->OnError() when rv != net::OK?
> I'm fine with just logging here and adding TODO to fix it later.

Done.

[Vitaly Buka (NO REVIEWS), 2014-09-19 21:04:46]

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

https://codereview.chromium.org/581813004/diff/80001/net/dns/mdns_client_impl...
net/dns/mdns_client_impl.cc:125: if (rv == ERR_IO_PENDING)
Looks like in both implementations ERR_IO_PENDING check is not needed.

[Sergey Ulanov, 2014-09-19 21:11:27]

lgtm, but i'm not an owner

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

https://codereview.chromium.org/581813004/diff/80001/net/dns/mdns_client_impl...
net/dns/mdns_client_impl.cc:117: return;
Maybe remove return and use else below.

https://codereview.chromium.org/581813004/diff/80001/net/dns/mdns_client_impl...
net/dns/mdns_client_impl.cc:129: connection_->OnError(this, rv);
Is OnConnectionError() handler allowed to delete MDnsConnection? if it is then
the code below will crash in that case.
Currently the only implementation of MDnsConnection::Delegate I can find ignores
this call
(https://code.google.com/p/chromium/codesearch#chromium/src/net/dns/mdns_clien...),
so it's not a problem right now, but it may become problem in the future.

[Vitaly Buka (NO REVIEWS), 2014-09-19 21:33:27]

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

https://codereview.chromium.org/581813004/diff/80001/net/dns/mdns_client_impl...
net/dns/mdns_client_impl.cc:117: return;
On 2014/09/19 21:11:27, Sergey Ulanov wrote:
> Maybe remove return and use else below.

Done.

https://codereview.chromium.org/581813004/diff/80001/net/dns/mdns_client_impl...
net/dns/mdns_client_impl.cc:129: connection_->OnError(this, rv);
On 2014/09/19 21:11:27, Sergey Ulanov wrote:
> Is OnConnectionError() handler allowed to delete MDnsConnection? if it is then
> the code below will crash in that case.
> Currently the only implementation of MDnsConnection::Delegate I can find
ignores
> this call
>
(https://code.google.com/p/chromium/codesearch#chromium/src/net/dns/mdns_clien...),
> so it's not a problem right now, but it may become problem in the future. 

Done.

[Vitaly Buka (NO REVIEWS), 2014-09-19 21:48:15]

vitalybuka@chromium.org changed reviewers:
+ rch@chromium.org

[Vitaly Buka (NO REVIEWS), 2014-09-19 21:48:16]

+rch

[Ryan Hamilton, 2014-09-22 19:26:44]

The code looks good, but I'm surprised not to see a unit test of the new logic.

https://codereview.chromium.org/581813004/diff/140001/net/dns/mdns_client_imp...
File net/dns/mdns_client_impl.cc (right):

https://codereview.chromium.org/581813004/diff/140001/net/dns/mdns_client_imp...
net/dns/mdns_client_impl.cc:173: int socket =
Do I read this correctly to say that this is actually just an index? Since it's
only used in the VLOG (which typically won't fire) I wonder if it's worth moving
this expression into the VLOG << block so it doesn't run unless VLOG is enabled?

https://codereview.chromium.org/581813004/diff/140001/net/dns/mdns_client_uni...
File net/dns/mdns_client_unittest.cc (right):

https://codereview.chromium.org/581813004/diff/140001/net/dns/mdns_client_uni...
net/dns/mdns_client_unittest.cc:1184: 
I don't see any tests of the new behavior. Specifically of queueing up multiple
requests and then sending them all when one completes. Seems like this would be
a good idea?

[Vitaly Buka (NO REVIEWS), 2014-09-22 22:53:49]

https://codereview.chromium.org/581813004/diff/140001/net/dns/mdns_client_imp...
File net/dns/mdns_client_impl.cc (right):

https://codereview.chromium.org/581813004/diff/140001/net/dns/mdns_client_imp...
net/dns/mdns_client_impl.cc:173: int socket =
On 2014/09/22 19:26:44, Ryan Hamilton wrote:
> Do I read this correctly to say that this is actually just an index? Since
it's
> only used in the VLOG (which typically won't fire) I wonder if it's worth
moving
> this expression into the VLOG << block so it doesn't run unless VLOG is
enabled?

Done.

https://codereview.chromium.org/581813004/diff/140001/net/dns/mdns_client_uni...
File net/dns/mdns_client_unittest.cc (right):

https://codereview.chromium.org/581813004/diff/140001/net/dns/mdns_client_uni...
net/dns/mdns_client_unittest.cc:1184: 
On 2014/09/22 19:26:44, Ryan Hamilton wrote:
> I don't see any tests of the new behavior. Specifically of queueing up
multiple
> requests and then sending them all when one completes. Seems like this would
be
> a good idea?

Done.

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

https://codereview.chromium.org/581813004/diff/160001/net/dns/mdns_client_imp...
net/dns/mdns_client_impl.h:55: void Send(const scoped_refptr<IOBuffer>& buffer,
unsigned size);
swithing back to IOBuffer, because comment in IOBufferWithSize asks to use
IOBuffer.

[Ryan Hamilton, 2014-09-22 23:19:57]

lgtm

https://codereview.chromium.org/581813004/diff/220001/net/dns/mdns_client_uni...
File net/dns/mdns_client_unittest.cc (right):

https://codereview.chromium.org/581813004/diff/220001/net/dns/mdns_client_uni...
net/dns/mdns_client_unittest.cc:1115: TEST_F(MDnsConnectionTest,
ReceiveSynchronous) {
nit: Should these tests be "ConnectionTest" or "ClientTest". Based on the file
name I would think "ClientTest" but you would know better.

https://codereview.chromium.org/581813004/diff/220001/net/dns/mdns_client_uni...
net/dns/mdns_client_unittest.cc:1207: .WillOnce(DoAll(SaveArg<2>(&callback),
Return(ERR_IO_PENDING)));
Wow, clever!

[Vitaly Buka (NO REVIEWS), 2014-09-22 23:33:02]

https://codereview.chromium.org/581813004/diff/220001/net/dns/mdns_client_uni...
File net/dns/mdns_client_unittest.cc (right):

https://codereview.chromium.org/581813004/diff/220001/net/dns/mdns_client_uni...
net/dns/mdns_client_unittest.cc:1115: TEST_F(MDnsConnectionTest,
ReceiveSynchronous) {
I guess most content of the file should be in mdns_client_impl_unittest.cc

I am not sure, but I'd rather expect test name is derived from class name.
MdnsConnection lives in mdns_client_impl.h. Probably it's not a good place.
If you don't mind will not change that in this CL.


On 2014/09/22 23:19:56, Ryan Hamilton wrote:
> nit: Should these tests be "ConnectionTest" or "ClientTest". Based on the file
> name I would think "ClientTest" but you would know better.

[Vitaly Buka (NO REVIEWS), 2014-09-22 23:33:06]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-09-22 23:34:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/581813004/220001

[Ryan Hamilton, 2014-09-22 23:34:50]

https://codereview.chromium.org/581813004/diff/220001/net/dns/mdns_client_uni...
File net/dns/mdns_client_unittest.cc (right):

https://codereview.chromium.org/581813004/diff/220001/net/dns/mdns_client_uni...
net/dns/mdns_client_unittest.cc:1115: TEST_F(MDnsConnectionTest,
ReceiveSynchronous) {
On 2014/09/22 23:33:02, Vitaly Buka wrote:
> I guess most content of the file should be in mdns_client_impl_unittest.cc
> 
> I am not sure, but I'd rather expect test name is derived from class name.
> MdnsConnection lives in mdns_client_impl.h. Probably it's not a good place.
> If you don't mind will not change that in this CL.

Yeah, no worries.

[commit-bot: I haz the power, 2014-09-22 23:42:37]

Committed patchset #12 (id:220001) as b304a74102f89669a88ca22c91baa182a828a5de

[commit-bot: I haz the power, 2014-09-22 23:43:08]

Patchset 12 (id:??) landed as
https://crrev.com/2085974efdf70fb9e1abf047ce59eb6cecd050ab
Cr-Commit-Position: refs/heads/master@{#296094}