DnsQuery: changed raw function pointer to callback.

DnsQuery: changed raw function pointer to callback.

BUG=60149
TEST=net_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=90844

[agayev, 2011-06-27 21:52:30]

brettw, darin: please review base/ change

cbentzel, willchan: the rest.

I have a few more of classes that need PRNG functor, instead of redefining it
over and over, I think it would be good to have it once in base/rand_util.h.

Thanks.

[brettw, 2011-06-27 22:24:48]

http://codereview.chromium.org/7276014/diff/1/base/rand_util.h
File base/rand_util.h (right):

http://codereview.chromium.org/7276014/diff/1/base/rand_util.h#newcode23
base/rand_util.h:23: typedef base::Callback<int(int, int)> RandIntCallback;
I don't understand why this is here. It's not necessary for any part of this
file, and introduces new header file dependencies. If you want a typedef, I'd
make it inside your Dns class.

[agayev, 2011-06-27 23:53:52]

http://codereview.chromium.org/7276014/diff/1/base/rand_util.h
File base/rand_util.h (right):

http://codereview.chromium.org/7276014/diff/1/base/rand_util.h#newcode23
base/rand_util.h:23: typedef base::Callback<int(int, int)> RandIntCallback;
On 2011/06/27 22:24:48, brettw wrote:
> I don't understand why this is here. It's not necessary for any part of this
> file, and introduces new header file dependencies. If you want a typedef, I'd
> make it inside your Dns class.

I thought callback for a RandInt is general enough to belong here.  Its client
is not just Dns class.  Should I redefine it every time I need it?

[willchan no longer on Chromium, 2011-06-28 09:58:23]

http://codereview.chromium.org/7276014/diff/1/base/rand_util.h
File base/rand_util.h (right):

http://codereview.chromium.org/7276014/diff/1/base/rand_util.h#newcode23
base/rand_util.h:23: typedef base::Callback<int(int, int)> RandIntCallback;
On 2011/06/27 23:53:52, agayev wrote:
> On 2011/06/27 22:24:48, brettw wrote:
> > I don't understand why this is here. It's not necessary for any part of this
> > file, and introduces new header file dependencies. If you want a typedef,
I'd
> > make it inside your Dns class.
> 
> I thought callback for a RandInt is general enough to belong here.  Its client
> is not just Dns class.  Should I redefine it every time I need it?

This looks to be an ease of use vs dependency bloat tradeoff. It's not clear to
me, since callback.h will probably already be included in most places. But we've
recently been fighting to trim our dependencies, so I think I'd err on the side
of burdening the clients with having to define it themselves.

http://codereview.chromium.org/7276014/diff/1/net/base/dns_query_unittest.cc
File net/base/dns_query_unittest.cc (right):

http://codereview.chromium.org/7276014/diff/1/net/base/dns_query_unittest.cc#...
net/base/dns_query_unittest.cc:7: #include "net/base/dns_query.h"
This header should go first.

http://codereview.chromium.org/7276014/diff/1/net/base/dns_response_unittest.cc
File net/base/dns_response_unittest.cc (right):

http://codereview.chromium.org/7276014/diff/1/net/base/dns_response_unittest....
net/base/dns_response_unittest.cc:7: #include "net/base/dns_response.h"
This header should go first.

[cbentzel, 2011-06-28 10:03:42]

Could you add a net/base/random_callback.h which the DNS related
headers include?

On Tue, Jun 28, 2011 at 5:58 AM,  <willchan@chromium.org> wrote:
>
> http://codereview.chromium.org/7276014/diff/1/base/rand_util.h
> File base/rand_util.h (right):
>
> http://codereview.chromium.org/7276014/diff/1/base/rand_util.h#newcode23
> base/rand_util.h:23: typedef base::Callback<int(int, int)>
> RandIntCallback;
> On 2011/06/27 23:53:52, agayev wrote:
>>
>> On 2011/06/27 22:24:48, brettw wrote:
>> > I don't understand why this is here. It's not necessary for any part
>
> of this
>>
>> > file, and introduces new header file dependencies. If you want a
>
> typedef, I'd
>>
>> > make it inside your Dns class.
>
>> I thought callback for a RandInt is general enough to belong here.
>
> Its client
>>
>> is not just Dns class.  Should I redefine it every time I need it?
>
> This looks to be an ease of use vs dependency bloat tradeoff. It's not
> clear to me, since callback.h will probably already be included in most
> places. But we've recently been fighting to trim our dependencies, so I
> think I'd err on the side of burdening the clients with having to define
> it themselves.
>
> http://codereview.chromium.org/7276014/diff/1/net/base/dns_query_unittest.cc
> File net/base/dns_query_unittest.cc (right):
>
>
http://codereview.chromium.org/7276014/diff/1/net/base/dns_query_unittest.cc#...
> net/base/dns_query_unittest.cc:7: #include "net/base/dns_query.h"
> This header should go first.
>
>
http://codereview.chromium.org/7276014/diff/1/net/base/dns_response_unittest.cc
> File net/base/dns_response_unittest.cc (right):
>
>
http://codereview.chromium.org/7276014/diff/1/net/base/dns_response_unittest....
> net/base/dns_response_unittest.cc:7: #include "net/base/dns_response.h"
> This header should go first.
>
> http://codereview.chromium.org/7276014/
>

[agayev, 2011-06-28 14:37:23]

cbentzel: another client of RandInt callback is going to be UDP socket.  Can it
still go to net/base/rand_callback.h?

http://codereview.chromium.org/7276014/diff/1/net/base/dns_query_unittest.cc
File net/base/dns_query_unittest.cc (right):

http://codereview.chromium.org/7276014/diff/1/net/base/dns_query_unittest.cc#...
net/base/dns_query_unittest.cc:7: #include "net/base/dns_query.h"
On 2011/06/28 09:58:23, willchan wrote:
> This header should go first.

Done.

http://codereview.chromium.org/7276014/diff/1/net/base/dns_response_unittest.cc
File net/base/dns_response_unittest.cc (right):

http://codereview.chromium.org/7276014/diff/1/net/base/dns_response_unittest....
net/base/dns_response_unittest.cc:7: #include "net/base/dns_response.h"
On 2011/06/28 09:58:23, willchan wrote:
> This header should go first.

Done.

[willchan no longer on Chromium, 2011-06-28 14:39:21]

On Tue, Jun 28, 2011 at 5:37 PM,  <agayev@chromium.org> wrote:
> cbentzel: another client of RandInt callback is going to be UDP socket.  Can
> it
> still go to net/base/rand_callback.h?

Yes, that's reasonable. net/udp should move to net/socket/udp and
net/socket/udp is allowed to depend on net/base.

>
>
> http://codereview.chromium.org/7276014/diff/1/net/base/dns_query_unittest.cc
> File net/base/dns_query_unittest.cc (right):
>
>
http://codereview.chromium.org/7276014/diff/1/net/base/dns_query_unittest.cc#...
> net/base/dns_query_unittest.cc:7: #include "net/base/dns_query.h"
> On 2011/06/28 09:58:23, willchan wrote:
>>
>> This header should go first.
>
> Done.
>
>
http://codereview.chromium.org/7276014/diff/1/net/base/dns_response_unittest.cc
> File net/base/dns_response_unittest.cc (right):
>
>
http://codereview.chromium.org/7276014/diff/1/net/base/dns_response_unittest....
> net/base/dns_response_unittest.cc:7: #include "net/base/dns_response.h"
> On 2011/06/28 09:58:23, willchan wrote:
>>
>> This header should go first.
>
> Done.
>
> http://codereview.chromium.org/7276014/
>

[agayev, 2011-06-28 14:59:11]

On 2011/06/28 14:39:21, willchan wrote:
> On Tue, Jun 28, 2011 at 5:37 PM,  <mailto:agayev@chromium.org> wrote:
> > cbentzel: another client of RandInt callback is going to be UDP socket.
&nbsp;Can
> > it
> > still go to net/base/rand_callback.h?
> 
> Yes, that's reasonable. net/udp should move to net/socket/udp and
> net/socket/udp is allowed to depend on net/base.

Done.

> 
> >
> >
> > http://codereview.chromium.org/7276014/diff/1/net/base/dns_query_unittest.cc
> > File net/base/dns_query_unittest.cc (right):
> >
> >
>
http://codereview.chromium.org/7276014/diff/1/net/base/dns_query_unittest.cc#...
> > net/base/dns_query_unittest.cc:7: #include "net/base/dns_query.h"
> > On 2011/06/28 09:58:23, willchan wrote:
> >>
> >> This header should go first.
> >
> > Done.
> >
> >
>
http://codereview.chromium.org/7276014/diff/1/net/base/dns_response_unittest.cc
> > File net/base/dns_response_unittest.cc (right):
> >
> >
>
http://codereview.chromium.org/7276014/diff/1/net/base/dns_response_unittest....
> > net/base/dns_response_unittest.cc:7: #include "net/base/dns_response.h"
> > On 2011/06/28 09:58:23, willchan wrote:
> >>
> >> This header should go first.
> >
> > Done.
> >
> > http://codereview.chromium.org/7276014/
> >

[brettw, 2011-06-28 16:09:51]

http://codereview.chromium.org/7276014/diff/1/base/rand_util.h
File base/rand_util.h (right):

http://codereview.chromium.org/7276014/diff/1/base/rand_util.h#newcode23
base/rand_util.h:23: typedef base::Callback<int(int, int)> RandIntCallback;
I might define it at the top of your DNS class so users can refer to it as
DNSQuery::RandomCallback if they need it.

[agayev, 2011-06-28 16:22:20]

http://codereview.chromium.org/7276014/diff/1/base/rand_util.h
File base/rand_util.h (right):

http://codereview.chromium.org/7276014/diff/1/base/rand_util.h#newcode23
base/rand_util.h:23: typedef base::Callback<int(int, int)> RandIntCallback;
On 2011/06/28 16:09:52, brettw wrote:
> I might define it at the top of your DNS class so users can refer to it as
> DNSQuery::RandomCallback if they need it.

It's not specific to DNS.  For example, UDPSocket class stores the same callback
as a member (to generate random port numbers when clients call Connect.)  Having
DNSQuery::RandomCallback as a type of a UDPSocket class member wouldn't be the
right thing, would it?

Anyway, I've moved the typedef to net/base/rand_callback.h.

[cbentzel, 2011-06-28 19:08:18]

LGTM

http://codereview.chromium.org/7276014/diff/1006/net/base/dns_query.h
File net/base/dns_query.h (right):

http://codereview.chromium.org/7276014/diff/1006/net/base/dns_query.h#newcode11
net/base/dns_query.h:11: #include "base/rand_util.h"
You don't need this include anymore

[willchan no longer on Chromium, 2011-06-28 19:10:47]

LGTM

http://codereview.chromium.org/7276014/diff/1006/net/base/dns_query.cc
File net/base/dns_query.cc (right):

http://codereview.chromium.org/7276014/diff/1006/net/base/dns_query.cc#newcode62
net/base/dns_query.cc:62: DnsQuery::DnsQuery(const DnsQuery& rhs)
I just happened to notice this. Copy constructors are frowned upon in the Google
Style Guide. Not going to block this change, and perhaps you've evaluated the
pros and cons already, but I'd advise using a Copy() or CopyFrom() instead.

http://codereview.chromium.org/7276014/diff/1006/net/base/dns_query.h
File net/base/dns_query.h (right):

http://codereview.chromium.org/7276014/diff/1006/net/base/dns_query.h#newcode11
net/base/dns_query.h:11: #include "base/rand_util.h"
Is this header still necessary?

[agayev, 2011-06-28 19:49:55]

http://codereview.chromium.org/7276014/diff/1006/net/base/dns_query.cc
File net/base/dns_query.cc (right):

http://codereview.chromium.org/7276014/diff/1006/net/base/dns_query.cc#newcode62
net/base/dns_query.cc:62: DnsQuery::DnsQuery(const DnsQuery& rhs)
On 2011/06/28 19:10:47, willchan wrote:
> I just happened to notice this. Copy constructors are frowned upon in the
Google
> Style Guide. Not going to block this change, and perhaps you've evaluated the
> pros and cons already, but I'd advise using a Copy() or CopyFrom() instead.

I wasn't aware of that, although this copy constructor is private, cannot be
accidentally invoked, I've also declared an assignment operator so this is just
like DISALLOW_COPY_AND_ASSIGN.

Anyway, I will just change CloneWithNewId to do whole cloning operation in a
followup CL, given that it does not change the interface, and add an explicit
DISALLOW_COPY_AND_ASSIGN to be consistent with the rest, since I really need
this to go in for my work to continue.

http://codereview.chromium.org/7276014/diff/1006/net/base/dns_query.h
File net/base/dns_query.h (right):

http://codereview.chromium.org/7276014/diff/1006/net/base/dns_query.h#newcode11
net/base/dns_query.h:11: #include "base/rand_util.h"
On 2011/06/28 19:08:18, cbentzel wrote:
> You don't need this include anymore

Done.

[willchan no longer on Chromium, 2011-06-28 20:06:42]

http://codereview.chromium.org/7276014/diff/1006/net/base/dns_query.cc
File net/base/dns_query.cc (right):

http://codereview.chromium.org/7276014/diff/1006/net/base/dns_query.cc#newcode62
net/base/dns_query.cc:62: DnsQuery::DnsQuery(const DnsQuery& rhs)
On 2011/06/28 19:49:55, agayev wrote:
> On 2011/06/28 19:10:47, willchan wrote:
> > I just happened to notice this. Copy constructors are frowned upon in the
> Google
> > Style Guide. Not going to block this change, and perhaps you've evaluated
the
> > pros and cons already, but I'd advise using a Copy() or CopyFrom() instead.
> 
> I wasn't aware of that, although this copy constructor is private, cannot be
> accidentally invoked, I've also declared an assignment operator so this is
just
> like DISALLOW_COPY_AND_ASSIGN.
> 
> Anyway, I will just change CloneWithNewId to do whole cloning operation in a
> followup CL, given that it does not change the interface, and add an explicit
> DISALLOW_COPY_AND_ASSIGN to be consistent with the rest, since I really need
> this to go in for my work to continue.

Yeah, a followup CL would be great. No need to block this one, I hope I
mentioned that earlier.