Break the dependency of DnsQuery and DnsResponse from AddressList

Break the dependency of DnsQuery and DnsResponse from AddressList

BUG=None
TEST=net_unittests --gtest_filter="DnsQuery*:DnsResponse*"

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

[agayev, 2011-06-08 20:48:36]

This patch makes DnsQuery and DnsResponse classes more abstract by breaking the
AddressList dependecy.

When implementing the higher-level class it became obvious that we should detect
invalid hostnames as early as possible and return synchronously from Resolve(),
which we had to do by calculating corresponding DNS name format; since we
already had it calculated, it didn't make sense to recalculate it in DnsQuery
constructor -- thus, with this patch, DnsQuery constructor takes a hostname in
DNS format; it also has a nice side effect that one cannot construct an invalid
DnsQuery (DCHECK() in the constructor acts as a type check) hence no more ugly
IsValid().  Also got rid of some redundant information like keeping qtype and
hostname as both fields and in the io_buffer.

Please review.  Thanks.

[cbentzel, 2011-06-09 01:16:06]

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

http://codereview.chromium.org/6995088/diff/2001/net/base/dns_query.cc#newcode58
net/base/dns_query.cc:58: DnsQuery::DnsQuery(const DnsQuery& rhs) {
Use initializer arguments rather than assignment where appropriate - maybe only
dns_name_size_

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

http://codereview.chromium.org/6995088/diff/2001/net/base/dns_query_unittest....
net/base/dns_query_unittest.cc:98: EXPECT_FALSE(q1.id() == q2.id());
I still don't like this test because it _will_ fail on some run.

[agayev, 2011-06-09 14:23:02]

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

http://codereview.chromium.org/6995088/diff/2001/net/base/dns_query.cc#newcode58
net/base/dns_query.cc:58: DnsQuery::DnsQuery(const DnsQuery& rhs) {
On 2011/06/09 01:16:06, cbentzel wrote:
> Use initializer arguments rather than assignment where appropriate - maybe
only
> dns_name_size_

Done.

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

http://codereview.chromium.org/6995088/diff/2001/net/base/dns_query_unittest....
net/base/dns_query_unittest.cc:98: EXPECT_FALSE(q1.id() == q2.id());
On 2011/06/09 01:16:06, cbentzel wrote:
> I still don't like this test because it _will_ fail on some run. 

We don't have an API for setting a seed to get a deterministic behavior.  Should
I get rid of them?

How about generating queries in a loop of 100 times and breaking out first time
they are different?  Probability of generating same ids 100 times in a row is
virtually 0.

[agl, 2011-06-09 14:29:04]

LGTM when you've fixed RandomIdTest.

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

http://codereview.chromium.org/6995088/diff/2001/net/base/dns_query.h#newcode41
net/base/dns_query.h:41: char* question_data() const;
const char? If the method is const then I suspect that the returned data should
also be.

[agayev, 2011-06-09 14:57:26]

Also fixed the RandomIdTest.

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

http://codereview.chromium.org/6995088/diff/2001/net/base/dns_query.h#newcode41
net/base/dns_query.h:41: char* question_data() const;
On 2011/06/09 14:29:04, agl wrote:
> const char? If the method is const then I suspect that the returned data
should
> also be.

Done.

[cbentzel, 2011-06-09 15:24:25]

LGTM

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

http://codereview.chromium.org/6995088/diff/8/net/base/dns_query.h#newcode16
net/base/dns_query.h:16: // Represents on-the-wire DNS query string as an
object.
Perhaps "DNS query message" instead of "DNS query string"?

http://codereview.chromium.org/6995088/diff/8/net/base/dns_query.h#newcode44
net/base/dns_query.h:44: IOBufferWithSize* io_buffer() const { return
io_buffer_; }
This could be const IOBufferWithSize* - but you'd have to add const char* data()
const to IOBuffer.

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

http://codereview.chromium.org/6995088/diff/8/net/base/dns_query_unittest.cc#...
net/base/dns_query_unittest.cc:94: // probability of collision, to avoid a flaky
test.
OK, I guess I can live with a failure probability of 1 in 2^32000 :)

[agayev, 2011-06-09 15:35:18]

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

http://codereview.chromium.org/6995088/diff/8/net/base/dns_query.h#newcode16
net/base/dns_query.h:16: // Represents on-the-wire DNS query string as an
object.
On 2011/06/09 15:24:25, cbentzel wrote:
> Perhaps "DNS query message" instead of "DNS query string"? 

Done.

http://codereview.chromium.org/6995088/diff/8/net/base/dns_query.h#newcode44
net/base/dns_query.h:44: IOBufferWithSize* io_buffer() const { return
io_buffer_; }
On 2011/06/09 15:24:25, cbentzel wrote:
> This could be const IOBufferWithSize* - but you'd have to add const char*
data()
> const to IOBuffer. 

In another CL perhaps.