Isolates generic DnsClient from AsyncHostResolver.

Isolates generic DnsClient from AsyncHostResolver. 

DnsClient provides a generic DNS client that allows fetching resource records. 
DnsClient is very lightweight and does not support aggregation, queuing or 
prioritization of requests. 

This is the first CL in a series to merge AsyncHostResolver into 
HostResolverImpl. 

Also introduces general-purpose BigEndianReader/Writer. 

BUG=90881
TEST=./net_unittests


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

[szym, 2011-11-30 21:37:12]

This is now ready for review. PTAL.

DnsClient uses new-style Callback to finish the request and the lifetime of the
request is determined by the lifetime of the DnsClient::Request object.
DnsResponse offers new DnsRecordParser which is designed to fit the
DnsRRResolver as well.

DnsSession is a ref-counted state shared between requests that currently hosts
parameters, but eventually would host adaptive timeout and TCP/UDP socket pools.
(A UDP socket pool might necessary if we confirm that NAT port exhaustion is a
real problem.)

Note that the changes to AsyncHostResolver are mainly to compile and pass the
tests. Only the code in AsyncHostResolver::OnRequestComplete will eventually
move to HostResolverImpl.

[cbentzel, 2011-11-30 22:04:45]

Matt can be the primary reviewer. I'll try to take a look, but may not get
as detailed.

On Wed, Nov 30, 2011 at 4:37 PM, <szym@chromium.org> wrote:

> Reviewers: Matt Menke, cbentzel,
>
> Message:
> This is now ready for review. PTAL.
>
> DnsClient uses new-style Callback to finish the request and the lifetime
> of the
> request is determined by the lifetime of the DnsClient::Request object.
> DnsResponse offers new DnsRecordParser which is designed to fit the
> DnsRRResolver as well.
>
> DnsSession is a ref-counted state shared between requests that currently
> hosts
> parameters, but eventually would host adaptive timeout and TCP/UDP socket
> pools.
> (A UDP socket pool might necessary if we confirm that NAT port exhaustion
> is a
> real problem.)
>
> Note that the changes to AsyncHostResolver are mainly to compile and pass
> the
> tests. Only the code in AsyncHostResolver::**OnRequestComplete will
> eventually
> move to HostResolverImpl.
>
>
> Description:
> Isolates generic DnsClient from AsyncHostResolver.
>
> DnsClient provides a generic DNS client that allows fetching resource
> records.
> DnsClient is very lightweight and does not support aggregation, queuing or
> prioritization of requests.
>
> This is the first CL in a series to merge AsyncHostResolver into
> HostResolverImpl.
>
> Also introduces general-purpose BigEndianReader/Writer.
>
> BUG=90881 [TBD]
> TEST=./net_unittests
>
>
> Please review this at
http://codereview.chromium.**org/8762001/<http://codereview.chromium.org/8762...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  A net/base/bigendian.h
>  A net/base/bigendian.cc
>  A net/base/bigendian_unittest.cc
>  M net/base/dns_util.h
>  M net/base/dns_util.cc
>  M net/dns/async_host_resolver.h
>  M net/dns/async_host_resolver.cc
>  M net/dns/async_host_resolver_**unittest.cc
>  A net/dns/dns_client.h
>  A net/dns/dns_client.cc
>  A net/dns/dns_client_unittest.cc
>  A net/dns/dns_protocol.h
>  M net/dns/dns_query.h
>  M net/dns/dns_query.cc
>  M net/dns/dns_query_unittest.cc
>  M net/dns/dns_response.h
>  M net/dns/dns_response.cc
>  M net/dns/dns_response_unittest.**cc
>  A net/dns/dns_session.h
>  A net/dns/dns_session.cc
>  M net/dns/dns_test_util.h
>  M net/dns/dns_test_util.cc
>  M net/dns/dns_transaction.h
>  M net/dns/dns_transaction.cc
>  M net/dns/dns_transaction_**unittest.cc
>  M net/net.gyp
>
>
>

[szym, 2011-11-30 22:36:09]

Thanks, Chris. 

Matt, I've sprinkled this code with TODOs as I anticipated to have to come back
to many of the listed issues. In particular, I am considering to reduce
DnsTransaction to a single request/reply exchange on a single socket and leave
retransmissions and choosing the socket to DnsClientImpl::RequestImpl. As I
mentioned, I predict we will need to pool sockets (both TCP and UDP). In that
case a DnsConnection would need to parse the transaction ids from the responses
and lookup the DnsTransaction that issued the query. Notably, TCP requests do
not need to be retransmitted unless the connection breaks.


On 2011/11/30 22:04:45, cbentzel wrote:
> Matt can be the primary reviewer. I'll try to take a look, but may not get
> as detailed.

[mmenke, 2011-12-02 00:53:55]

The code's basically fine.  Just a bunch of style nits, push back as needed.

Still have yet to review a couple of the unit tests, and one or two other files.

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

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.cc#newcode15
net/base/bigendian.cc:15: ptr_+= len;
nit:  There should be a space between _ and + in all of these.

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.h
File net/base/bigendian.h (right):

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.h#newcode56
net/base/bigendian.h:56: int remaining() const { return end_ - ptr_; }
Could you add a couple linebreaks here, to increase readability?  Up to you just
where to put them, but I'd suggest below the constructor, here, and below copy.

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.h#newcode58
net/base/bigendian.h:58: bool Copy(void* out, size_t len);
I believe this can be const.

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.h#newcode58
net/base/bigendian.h:58: bool Copy(void* out, size_t len);
Think a comment that states that all of these functions increment ptr_ by the
number of bytes read would be good.  It's a simple class, but people shouldn't
have to read the code to know what it does.  Same goes for the Writer.

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.h#newcode60
net/base/bigendian.h:60: bool Piece(base::StringPiece* out, size_t len);
Google style is to use verbs for function names.  Since these are more than
simple accessors, I suggest just ReadPiece, "ReadU8", etc.

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.h#newcode61
net/base/bigendian.h:61: bool U8(uint8* v);
Google style discourages overly abbreviated variable names.  I suggest you just
write out "value".

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.h#newcode64
net/base/bigendian.h:64: private:
nit:  Google style is to put a blank line before this
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_...).
 It excludes small classes, but think this one is large enough that it increases
readability.

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.h#newcode70
net/base/bigendian.h:70: const char* end_;
Not vital, but could make both |end_| pointers could be char* const, to be a
little clearer.

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.h#newcode81
net/base/bigendian.h:81: bool Copy(const void* buf, size_t len);
Think the name of this function is a little odd.  U8, U16, and U32 all copy data
as well.  WriteBytes, maybe?  I'm open to other ideas.  Can even live with it as
is, but the name just seems a little odd.

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.h#newcode85
net/base/bigendian.h:85: private:
nit:  Missing blank line.

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

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian_unittest.cc#...
net/base/bigendian_unittest.cc:22: EXPECT_TRUE(r.Copy(buf, sizeof(buf)));
I suggest adding linebreaks before each function that reads from or writes to
the BigEndianReader's ptr_ (Skip(), Copy(), U8(), etc) to increase readability,
but up to you.

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian_unittest.cc#...
net/base/bigendian_unittest.cc:30: EXPECT_EQ(0x0708090au, u32);
nit:  I suggest you capitalize all your hexadecimal characters.  "au" just looks
weird.

http://codereview.chromium.org/8762001/diff/1/net/dns/async_host_resolver.cc
File net/dns/async_host_resolver.cc (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/async_host_resolver.cc#...
net/dns/async_host_resolver.cc:353: AddressList addrlist;
nit:  Should be an underscore between words:  addr_list.

http://codereview.chromium.org/8762001/diff/1/net/dns/async_host_resolver.cc#...
net/dns/async_host_resolver.cc:358: // TODO(szym): add stricter checking of
names, aliases and address lengths
Nit:  Capitalize "add", add period.

http://codereview.chromium.org/8762001/diff/1/net/dns/async_host_resolver.cc#...
net/dns/async_host_resolver.cc:362: record.rdata.size() == kIPv6AddressSize))
Chrome style is to use braces on if statements, when the condition or statement
block runs onto multiple lines.

http://codereview.chromium.org/8762001/diff/1/net/dns/async_host_resolver.h
File net/dns/async_host_resolver.h (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/async_host_resolver.h#n...
net/dns/async_host_resolver.h:29: size_t max_pending_requests_,
Know this was already here, but while you're modifying this file anyways, could
you please remove the ending underscore?  Thanks.

http://codereview.chromium.org/8762001/diff/1/net/dns/async_host_resolver.h#n...
net/dns/async_host_resolver.h:51: const DnsResponse* transaction);
OVERRIDE

http://codereview.chromium.org/8762001/diff/1/net/dns/async_host_resolver.h#n...
net/dns/async_host_resolver.h:115: TransactionList dns_requests_;
I think overload requests to both mean requests to the AsyncHostResolver and
requests to the DnsClient is a bit confusing.  You might want to either find
another name for one or the other, or add an adjective or something.

Maybe dns_client_requests_?  I though transactions_ was ok, too, though if you
feel that name is inaccurate, you should probably rename TransactionList as
well.

http://codereview.chromium.org/8762001/diff/1/net/dns/async_host_resolver_uni...
File net/dns/async_host_resolver_unittest.cc (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/async_host_resolver_uni...
net/dns/async_host_resolver_unittest.cc:63: : Request(qname, qtype, callback),
started_(false), client_(client) {
nit:  4 space indent.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client.cc
File net/dns/dns_client.cc (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client.cc#newcode20
net/dns/dns_client.cc:20: : qname_(qname.data(), qname.size()),
nit:  Indent 4 spaces.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client.cc#newcode30
net/dns/dns_client.cc:30: class RequestImpl : public Request {
Think you should either not make this an inner class, or not inline the
functions.  While it's perfectly in a decent text editor with nice C++ coloring,
Rietveld tends to make things look hideous, and as the complexity of this class
grows as you intend, I think it'll just make everyone's life easier.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client.cc#newcode90
net/dns/dns_client.cc:90: 
nit:  Only one blank line needed

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client.h
File net/dns/dns_client.h (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client.h#newcode70
net/dns/dns_client.h:70: };
Could you plut a linebreak here, to visually separate the innerclass from the
DnsClient's destructor?

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc
File net/dns/dns_client_unittest.cc (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc#...
net/dns/dns_client_unittest.cc:23: namespace {
If you don't need to make the test the friend of the classes you're testing, as
appears to be the case here, you can just stick the entire test in the anonymous
namespace.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc#...
net/dns/dns_client_unittest.cc:30: struct TestRequest {
This is sufficiently big and complicated that I think it should be a class.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc#...
net/dns/dns_client_unittest.cc:30: struct TestRequest {
"TestRequest" seems like a misnomer, since it's acting more like a HostResolver,
rather than a DnsClient::Request.  TestRequestHandler or TestRequestHelper,
maybe?  I'm open to other ideas.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc#...
net/dns/dns_client_unittest.cc:61: DCHECK(request.get() != NULL);
ASSERT_TRUE(request.get()) is preferred in unit tests.  Generally more useful on
buildbots, particularly release ones.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc#...
net/dns/dns_client_unittest.cc:84: void
OnRequestCompleteCancel(DnsClient::Request* req,
I find this function name a little vague.  It could also mean a request
"completed" by being cancelled.  I suggest you either add a comment, or rename
it to "CancelRequestOnComplete" or "CancelOnRequestComplete" or something.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc#...
net/dns/dns_client_unittest.cc:152: DCHECK(rv);
EXPECT_EQ(OK, rv);

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc#...
net/dns/dns_client_unittest.cc:166: DCHECK(!transaction_ids_.empty());
EXPECT_FALSE(transaction_ids_.empty());

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc#...
net/dns/dns_client_unittest.cc:169: DCHECK(id >= min && id <= max);
EXPECT_GE(id, min);
EXPECT_LE(id, max);

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc#...
net/dns/dns_client_unittest.cc:182: // wait until result
nit:  Wait until result.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc#...
net/dns/dns_client_unittest.cc:242: requests_[i].MakeRequest(client_.get());
nit:  Too much indentation.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc#...
net/dns/dns_client_unittest.cc:252: 
Think it might be nice to have a test for a simulated failure, either due to bad
DNS data, or a network error.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_protocol.h
File net/dns/dns_protocol.h (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_protocol.h#newcode13
net/dns/dns_protocol.h:13: namespace net {
nit:  Google style is to have an empty line here.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_protocol.h#newcode75
net/dns/dns_protocol.h:75: // Used internally in DnsQuery and DnsResponseParser
nit:  Missing period.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_protocol.h#newcode117
net/dns/dns_protocol.h:117: }  // namespace dns_protocol
nit:  add a blank line.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_query.cc
File net/dns/dns_query.cc (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_query.cc#newcode24
net/dns/dns_query.cc:24: + sizeof(uint16)                   // QTYPE
While this style is allowed by the Google style guide, it tends to be a little
more error prone, and it's not that common in chrome/ or net/.

The following is valid, though incorrect, code:

whatever = 5;
    + 6;

I believe Clang would catch it, but best to be safe, and the problem is even
worse with a semi-colon optional language like Javascript.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_query.cc#newcode29
net/dns/dns_query.cc:29:
reinterpret_cast<dns_protocol::Header*>(io_buffer_->data());
Google style is to avoid meaningless variable names.  You can just go with
header, writer, reader, etc.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_query.cc#newcode35
net/dns/dns_query.cc:35: // write question section after the header
nit:  Capitalized "Write", add period.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_query.cc#newcode64
net/dns/dns_query.cc:64: + qname_size_, &type);
Suggest you move your operators onto the previous line here as well.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_query.cc#newcode79
net/dns/dns_query.cc:79:
reinterpret_cast<dns_protocol::Header*>(io_buffer_->data());
Weird number of spaces here.  Should be indented 4 more than the previous line.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_query.h
File net/dns/dns_query.h (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_query.h#newcode12
net/dns/dns_query.h:12: #include "net/dns/dns_protocol.h"
Don't think we need this one.

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

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_query_unittest.cc#n...
net/dns/dns_query_unittest.cc:41:
EXPECT_EQ(static_cast<int>(sizeof(query_data)), q1.io_buffer()->size());
nit:  Not terribly important, but might want to make this an ASSERT_EQ, since if
they're not equal, the next line could be bad.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_query_unittest.cc#n...
net/dns/dns_query_unittest.cc:47: EXPECT_EQ(0, q1.question().compare(question));
Why not:

EXPECT_EQ(question, q1.question())?

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_response.cc
File net/dns/dns_response.cc (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_response.cc#newcode58
net/dns/dns_response.cc:58: seen+= sizeof(uint16);
nit:  space before +=

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_response.cc#newcode64
net/dns/dns_response.cc:64: offset&= dns_protocol::kOffsetMask;
nit:  space before &=

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_response.cc#newcode88
net/dns/dns_response.cc:88: seen+= 1 + label_len;
nit:  Space before +=

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_session.cc
File net/dns/dns_session.cc (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_session.cc#newcode20
net/dns/dns_session.cc:20: : config_(config),
nit:  4 spaces.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_session.h
File net/dns/dns_session.h (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_session.h#newcode51
net/dns/dns_session.h:51: DnsConfig config_;
It looks to me like your intention is to create a new DnsClient/DnsSession when
the DnsConfig changes.  If this is the case, I'd go ahead and make this const,
and update the DnsSession class description to mention that it must be recreated
when the config changes.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_transaction.cc#newc...
net/dns/dns_transaction.cc:255: return ERR_DNS_SERVER_REQUIRES_TCP;
nit:  Should use two space indent.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_transaction.cc#newc...
net/dns/dns_transaction.cc:258: response_->rcode() !=
dns_protocol::kRcodeNXDOMAIN)
nit:  Use braces here.

http://codereview.chromium.org/8762001/diff/7001/net/base/bigendian.h
File net/base/bigendian.h (right):

http://codereview.chromium.org/8762001/diff/7001/net/base/bigendian.h#newcode5
net/base/bigendian.h:5: #ifndef NET_BASE_BIGENDIAN_H_
nit:  This should be named big_endian.h

http://codereview.chromium.org/8762001/diff/7001/net/dns/dns_protocol.h
File net/dns/dns_protocol.h (right):

http://codereview.chromium.org/8762001/diff/7001/net/dns/dns_protocol.h#newco...
net/dns/dns_protocol.h:14: namespace dns_protocol {
nit:  Should be a blank line before the inner namespace.

http://codereview.chromium.org/8762001/diff/7001/net/dns/dns_protocol.h#newco...
net/dns/dns_protocol.h:117: }  // namespace dns_protocol
nit: Blank line needed.

http://codereview.chromium.org/8762001/diff/7001/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

http://codereview.chromium.org/8762001/diff/7001/net/dns/dns_transaction.cc#n...
net/dns/dns_transaction.cc:217: return ERR_DNS_SERVER_FAILED;
This is a weird error message in this case, since it's a actually a client
error.

[mmenke, 2011-12-02 01:19:56]

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.h
File net/base/bigendian.h (right):

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.h#newcode56
net/base/bigendian.h:56: int remaining() const { return end_ - ptr_; }
On 2011/12/02 00:53:55, Matt Menke wrote:
> Could you add a couple linebreaks here, to increase readability?  Up to you
just
> where to put them, but I'd suggest below the constructor, here, and below
copy.

Oops.  That should be above Copy.

[szym, 2011-12-05 23:06:28]

Thanks for the review. Given the results of my experiments with NAT overload, I
expect to rework the relationship of DnsTransaction <-> DnsSession <-> DnsClient
so that we reuse one UDP socket, but I'd rather split it out as another CL.

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

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.cc#newcode15
net/base/bigendian.cc:15: ptr_+= len;
On 2011/12/02 00:53:55, Matt Menke wrote:
> nit:  There should be a space between _ and + in all of these.

Done.

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.h
File net/base/bigendian.h (right):

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.h#newcode56
net/base/bigendian.h:56: int remaining() const { return end_ - ptr_; }
On 2011/12/02 00:53:55, Matt Menke wrote:
> Could you add a couple linebreaks here, to increase readability?  Up to you
just
> where to put them, but I'd suggest below the constructor, here, and below
copy.

Done.

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.h#newcode58
net/base/bigendian.h:58: bool Copy(void* out, size_t len);
On 2011/12/02 00:53:55, Matt Menke wrote:
> Think a comment that states that all of these functions increment ptr_ by the
> number of bytes read would be good.  It's a simple class, but people shouldn't
> have to read the code to know what it does.  Same goes for the Writer.

Done.

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.h#newcode60
net/base/bigendian.h:60: bool Piece(base::StringPiece* out, size_t len);
On 2011/12/02 00:53:55, Matt Menke wrote:
> Google style is to use verbs for function names.  Since these are more than
> simple accessors, I suggest just ReadPiece, "ReadU8", etc.

Done.

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.h#newcode61
net/base/bigendian.h:61: bool U8(uint8* v);
On 2011/12/02 00:53:55, Matt Menke wrote:
> Google style discourages overly abbreviated variable names.  I suggest you
just
> write out "value".

Done.

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.h#newcode64
net/base/bigendian.h:64: private:
On 2011/12/02 00:53:55, Matt Menke wrote:
> nit:  Google style is to put a blank line before this
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_...).
>  It excludes small classes, but think this one is large enough that it
increases
> readability.

Done.

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.h#newcode70
net/base/bigendian.h:70: const char* end_;
Left it non-const to allow the compiler to generate operator=.

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.h#newcode81
net/base/bigendian.h:81: bool Copy(const void* buf, size_t len);
On 2011/12/02 00:53:55, Matt Menke wrote:
> Think the name of this function is a little odd.  U8, U16, and U32 all copy
data
> as well.  WriteBytes, maybe?  I'm open to other ideas.  Can even live with it
as
> is, but the name just seems a little odd.

Done. I like WriteBytes, although it's meant to write any POD.

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian.h#newcode85
net/base/bigendian.h:85: private:
On 2011/12/02 00:53:55, Matt Menke wrote:
> nit:  Missing blank line.

Done.

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

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian_unittest.cc#...
net/base/bigendian_unittest.cc:22: EXPECT_TRUE(r.Copy(buf, sizeof(buf)));
On 2011/12/02 00:53:55, Matt Menke wrote:
> I suggest adding linebreaks before each function that reads from or writes to
> the BigEndianReader's ptr_ (Skip(), Copy(), U8(), etc) to increase
readability,
> but up to you.

Done.

http://codereview.chromium.org/8762001/diff/1/net/base/bigendian_unittest.cc#...
net/base/bigendian_unittest.cc:30: EXPECT_EQ(0x0708090au, u32);
On 2011/12/02 00:53:55, Matt Menke wrote:
> nit:  I suggest you capitalize all your hexadecimal characters.  "au" just
looks
> weird.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/async_host_resolver.cc
File net/dns/async_host_resolver.cc (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/async_host_resolver.cc#...
net/dns/async_host_resolver.cc:353: AddressList addrlist;
On 2011/12/02 00:53:55, Matt Menke wrote:
> nit:  Should be an underscore between words:  addr_list.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/async_host_resolver.cc#...
net/dns/async_host_resolver.cc:358: // TODO(szym): add stricter checking of
names, aliases and address lengths
On 2011/12/02 00:53:55, Matt Menke wrote:
> Nit:  Capitalize "add", add period.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/async_host_resolver.cc#...
net/dns/async_host_resolver.cc:362: record.rdata.size() == kIPv6AddressSize))
On 2011/12/02 00:53:55, Matt Menke wrote:
> Chrome style is to use braces on if statements, when the condition or
statement
> block runs onto multiple lines.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/async_host_resolver.h
File net/dns/async_host_resolver.h (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/async_host_resolver.h#n...
net/dns/async_host_resolver.h:29: size_t max_pending_requests_,
On 2011/12/02 00:53:55, Matt Menke wrote:
> Know this was already here, but while you're modifying this file anyways,
could
> you please remove the ending underscore?  Thanks.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/async_host_resolver.h#n...
net/dns/async_host_resolver.h:51: const DnsResponse* transaction);
On 2011/12/02 00:53:55, Matt Menke wrote:
> OVERRIDE

DnsTransaction no longer defines a Delegate interface. This is just a
base::Callback, so no OVERRIDE.

http://codereview.chromium.org/8762001/diff/1/net/dns/async_host_resolver_uni...
File net/dns/async_host_resolver_unittest.cc (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/async_host_resolver_uni...
net/dns/async_host_resolver_unittest.cc:63: : Request(qname, qtype, callback),
started_(false), client_(client) {
On 2011/12/02 00:53:55, Matt Menke wrote:
> nit:  4 space indent.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client.cc
File net/dns/dns_client.cc (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client.cc#newcode20
net/dns/dns_client.cc:20: : qname_(qname.data(), qname.size()),
On 2011/12/02 00:53:55, Matt Menke wrote:
> nit:  Indent 4 spaces.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client.h
File net/dns/dns_client.h (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client.h#newcode70
net/dns/dns_client.h:70: };
On 2011/12/02 00:53:55, Matt Menke wrote:
> Could you plut a linebreak here, to visually separate the innerclass from the
> DnsClient's destructor?

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc
File net/dns/dns_client_unittest.cc (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc#...
net/dns/dns_client_unittest.cc:23: namespace {
On 2011/12/02 00:53:55, Matt Menke wrote:
> If you don't need to make the test the friend of the classes you're testing,
as
> appears to be the case here, you can just stick the entire test in the
anonymous
> namespace.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc#...
net/dns/dns_client_unittest.cc:30: struct TestRequest {
On 2011/12/02 00:53:55, Matt Menke wrote:
> "TestRequest" seems like a misnomer, since it's acting more like a
HostResolver,
> rather than a DnsClient::Request.  TestRequestHandler or TestRequestHelper,
> maybe?  I'm open to other ideas.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc#...
net/dns/dns_client_unittest.cc:61: DCHECK(request.get() != NULL);
On 2011/12/02 00:53:55, Matt Menke wrote:
> ASSERT_TRUE(request.get()) is preferred in unit tests.  Generally more useful
on
> buildbots, particularly release ones.

Done, although ASSERT_* does not work in functions returning non-void (e.g.,
GetNextId).

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc#...
net/dns/dns_client_unittest.cc:84: void
OnRequestCompleteCancel(DnsClient::Request* req,
On 2011/12/02 00:53:55, Matt Menke wrote:
> I find this function name a little vague.  It could also mean a request
> "completed" by being cancelled.  I suggest you either add a comment, or rename
> it to "CancelRequestOnComplete" or "CancelOnRequestComplete" or something.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc#...
net/dns/dns_client_unittest.cc:152: DCHECK(rv);
On 2011/12/02 00:53:55, Matt Menke wrote:
> EXPECT_EQ(OK, rv);

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc#...
net/dns/dns_client_unittest.cc:166: DCHECK(!transaction_ids_.empty());
On 2011/12/02 00:53:55, Matt Menke wrote:
> EXPECT_FALSE(transaction_ids_.empty());

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc#...
net/dns/dns_client_unittest.cc:169: DCHECK(id >= min && id <= max);
On 2011/12/02 00:53:55, Matt Menke wrote:
> EXPECT_GE(id, min);
> EXPECT_LE(id, max);

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc#...
net/dns/dns_client_unittest.cc:182: // wait until result
On 2011/12/02 00:53:55, Matt Menke wrote:
> nit:  Wait until result.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc#...
net/dns/dns_client_unittest.cc:242: requests_[i].MakeRequest(client_.get());
On 2011/12/02 00:53:55, Matt Menke wrote:
> nit:  Too much indentation.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_client_unittest.cc#...
net/dns/dns_client_unittest.cc:252: 
On 2011/12/02 00:53:55, Matt Menke wrote:
> Think it might be nice to have a test for a simulated failure, either due to
bad
> DNS data, or a network error.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_protocol.h
File net/dns/dns_protocol.h (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_protocol.h#newcode13
net/dns/dns_protocol.h:13: namespace net {
On 2011/12/02 00:53:55, Matt Menke wrote:
> nit:  Google style is to have an empty line here.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_protocol.h#newcode75
net/dns/dns_protocol.h:75: // Used internally in DnsQuery and DnsResponseParser
On 2011/12/02 00:53:55, Matt Menke wrote:
> nit:  Missing period.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_protocol.h#newcode117
net/dns/dns_protocol.h:117: }  // namespace dns_protocol
On 2011/12/02 00:53:55, Matt Menke wrote:
> nit:  add a blank line.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_query.cc
File net/dns/dns_query.cc (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_query.cc#newcode24
net/dns/dns_query.cc:24: + sizeof(uint16)                   // QTYPE
On 2011/12/02 00:53:55, Matt Menke wrote:
> While this style is allowed by the Google style guide, it tends to be a little
> more error prone, and it's not that common in chrome/ or net/.
> 
> The following is valid, though incorrect, code:
> 
> whatever = 5;
>     + 6;
> 
> I believe Clang would catch it, but best to be safe, and the problem is even
> worse with a semi-colon optional language like Javascript.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_query.cc#newcode29
net/dns/dns_query.cc:29:
reinterpret_cast<dns_protocol::Header*>(io_buffer_->data());
On 2011/12/02 00:53:55, Matt Menke wrote:
> Google style is to avoid meaningless variable names.  You can just go with
> header, writer, reader, etc.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_query.cc#newcode29
net/dns/dns_query.cc:29:
reinterpret_cast<dns_protocol::Header*>(io_buffer_->data());
On 2011/12/02 00:53:55, Matt Menke wrote:
> Google style is to avoid meaningless variable names.  You can just go with
> header, writer, reader, etc.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_query.cc#newcode35
net/dns/dns_query.cc:35: // write question section after the header
On 2011/12/02 00:53:55, Matt Menke wrote:
> nit:  Capitalized "Write", add period.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_query.cc#newcode64
net/dns/dns_query.cc:64: + qname_size_, &type);
On 2011/12/02 00:53:55, Matt Menke wrote:
> Suggest you move your operators onto the previous line here as well.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_query.cc#newcode79
net/dns/dns_query.cc:79:
reinterpret_cast<dns_protocol::Header*>(io_buffer_->data());
On 2011/12/02 00:53:55, Matt Menke wrote:
> Weird number of spaces here.  Should be indented 4 more than the previous
line.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_query.h
File net/dns/dns_query.h (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_query.h#newcode12
net/dns/dns_query.h:12: #include "net/dns/dns_protocol.h"
On 2011/12/02 00:53:55, Matt Menke wrote:
> Don't think we need this one.

Done.

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

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_query_unittest.cc#n...
net/dns/dns_query_unittest.cc:41:
EXPECT_EQ(static_cast<int>(sizeof(query_data)), q1.io_buffer()->size());
On 2011/12/02 00:53:55, Matt Menke wrote:
> nit:  Not terribly important, but might want to make this an ASSERT_EQ, since
if
> they're not equal, the next line could be bad.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_query_unittest.cc#n...
net/dns/dns_query_unittest.cc:47: EXPECT_EQ(0, q1.question().compare(question));
On 2011/12/02 00:53:55, Matt Menke wrote:
> Why not:
> 
> EXPECT_EQ(question, q1.question())?

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_response.cc
File net/dns/dns_response.cc (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_response.cc#newcode58
net/dns/dns_response.cc:58: seen+= sizeof(uint16);
On 2011/12/02 00:53:55, Matt Menke wrote:
> nit:  space before +=

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_response.cc#newcode64
net/dns/dns_response.cc:64: offset&= dns_protocol::kOffsetMask;
On 2011/12/02 00:53:55, Matt Menke wrote:
> nit:  space before &=

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_response.cc#newcode88
net/dns/dns_response.cc:88: seen+= 1 + label_len;
On 2011/12/02 00:53:55, Matt Menke wrote:
> nit:  Space before +=

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_session.cc
File net/dns/dns_session.cc (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_session.cc#newcode20
net/dns/dns_session.cc:20: : config_(config),
On 2011/12/02 00:53:55, Matt Menke wrote:
> nit:  4 spaces.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_session.h
File net/dns/dns_session.h (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_session.h#newcode51
net/dns/dns_session.h:51: DnsConfig config_;
On 2011/12/02 00:53:55, Matt Menke wrote:
> It looks to me like your intention is to create a new DnsClient/DnsSession
when
> the DnsConfig changes.  If this is the case, I'd go ahead and make this const,
> and update the DnsSession class description to mention that it must be
recreated
> when the config changes.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_transaction.cc#newc...
net/dns/dns_transaction.cc:255: return ERR_DNS_SERVER_REQUIRES_TCP;
On 2011/12/02 00:53:56, Matt Menke wrote:
> nit:  Should use two space indent.

Done.

http://codereview.chromium.org/8762001/diff/1/net/dns/dns_transaction.cc#newc...
net/dns/dns_transaction.cc:258: response_->rcode() !=
dns_protocol::kRcodeNXDOMAIN)
On 2011/12/02 00:53:56, Matt Menke wrote:
> nit:  Use braces here.

Done.

http://codereview.chromium.org/8762001/diff/7001/net/base/bigendian.h
File net/base/bigendian.h (right):

http://codereview.chromium.org/8762001/diff/7001/net/base/bigendian.h#newcode5
net/base/bigendian.h:5: #ifndef NET_BASE_BIGENDIAN_H_
On 2011/12/02 00:53:56, Matt Menke wrote:
> nit:  This should be named big_endian.h

Done.

http://codereview.chromium.org/8762001/diff/7001/net/dns/dns_protocol.h
File net/dns/dns_protocol.h (right):

http://codereview.chromium.org/8762001/diff/7001/net/dns/dns_protocol.h#newco...
net/dns/dns_protocol.h:14: namespace dns_protocol {
On 2011/12/02 00:53:56, Matt Menke wrote:
> nit:  Should be a blank line before the inner namespace.

Done.

http://codereview.chromium.org/8762001/diff/7001/net/dns/dns_protocol.h#newco...
net/dns/dns_protocol.h:117: }  // namespace dns_protocol
On 2011/12/02 00:53:56, Matt Menke wrote:
> nit: Blank line needed.

Done.

http://codereview.chromium.org/8762001/diff/7001/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

http://codereview.chromium.org/8762001/diff/7001/net/dns/dns_transaction.cc#n...
net/dns/dns_transaction.cc:217: return ERR_DNS_SERVER_FAILED;
On 2011/12/02 00:53:56, Matt Menke wrote:
> This is a weird error message in this case, since it's a actually a client
> error.

Changed to ERR_MSG_TOO_BIG, although we should rather expect
DatagramClientSocket::Write to return that first.

[szym, 2011-12-06 14:32:32]

Trybots passed. Ready for another round.

[mmenke, 2011-12-06 18:08:39]

Want to give the CL a final once over before signing off on it, but looking
good.

http://codereview.chromium.org/8762001/diff/1/net/dns/async_host_resolver.h
File net/dns/async_host_resolver.h (right):

http://codereview.chromium.org/8762001/diff/1/net/dns/async_host_resolver.h#n...
net/dns/async_host_resolver.h:51: const DnsResponse* transaction);
On 2011/12/05 23:06:28, szym wrote:
> On 2011/12/02 00:53:55, Matt Menke wrote:
> > OVERRIDE
> 
> DnsTransaction no longer defines a Delegate interface. This is just a
> base::Callback, so no OVERRIDE.
> 

Ah, right.  Don't think it needs to be virtual, either, then, as nothing
inherits from AsyncHostResolver.

http://codereview.chromium.org/8762001/diff/23001/net/dns/async_host_resolver.h
File net/dns/async_host_resolver.h (right):

http://codereview.chromium.org/8762001/diff/23001/net/dns/async_host_resolver...
net/dns/async_host_resolver.h:51: const DnsResponse* transaction);
nit:  Fix indentation.

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_client.cc
File net/dns/dns_client.cc (right):

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_client.cc#newco...
net/dns/dns_client.cc:90: 
nit:  Only one blank line needed.

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_client_unittest.cc
File net/dns/dns_client_unittest.cc (right):

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_client_unittest...
net/dns/dns_client_unittest.cc:109: void SetUp() OVERRIDE {
virtual

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_client_unittest...
net/dns/dns_client_unittest.cc:176: void TearDown() OVERRIDE {
virtual

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_query.cc
File net/dns/dns_query.cc (right):

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_query.cc#newcode27
net/dns/dns_query.cc:27: + question_size);
nit:  Move '+' onto previous line.

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_response.h
File net/dns/dns_response.h (right):

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_response.h#newc...
net/dns/dns_response.h:42: bool IsValid() const { return packet_ != NULL; }
Should add a comment that this validates the parser, not the data it's parsing.

Also, are this and the uninitialized constructor even necessary?

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_response_unitte...
File net/dns/dns_response_unittest.cc (right):

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_response_unitte...
net/dns/dns_response_unittest.cc:43: DnsRecordParser it(data, sizeof(data), 0);
This variable name is a little weird.

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_response_unitte...
net/dns/dns_response_unittest.cc:48: // check that the last "." is never stored
nit:  All these comments that are sentences should be capitalized and have
periods, as per Google style guide.

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_response_unitte...
net/dns/dns_response_unittest.cc:79: 0xc0, 0x08,
I believe these should be:

0xc0, 0x08,
0xc0, 0x06,

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_response_unitte...
net/dns/dns_response_unittest.cc:143: // Test truncated record
nit:  period.

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_response_unitte...
net/dns/dns_response_unittest.cc:190: 0xc0, 0x35,         // NAME is a pointer
to name in Question section.
Mind fixing this comment?  That looks to be in the Answer 1 section to me.

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_response_unitte...
net/dns/dns_response_unittest.cc:203: // reject too short
nit:  Convert to sentence / capitalize / period, as per Google style guidelines.
 Goes for the other comments in this test, too.

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_transaction_uni...
File net/dns/dns_transaction_unittest.cc (right):

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_transaction_uni...
net/dns/dns_transaction_unittest.cc:33: void SetUp() OVERRIDE {
virtual

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_transaction_uni...
net/dns/dns_transaction_unittest.cc:77: std::string qname_;
Think these should still be private.  Just use accessors for any you need to
read from.  Makes which are accessed/modified by individual tests a little
clearer.

[szym, 2011-12-06 19:05:51]

Thanks again for review. In particular, for catching that loopy name test.

http://codereview.chromium.org/8762001/diff/23001/net/dns/async_host_resolver.h
File net/dns/async_host_resolver.h (right):

http://codereview.chromium.org/8762001/diff/23001/net/dns/async_host_resolver...
net/dns/async_host_resolver.h:51: const DnsResponse* transaction);
On 2011/12/06 18:08:39, Matt Menke wrote:
> nit:  Fix indentation.

Done.

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_client.cc
File net/dns/dns_client.cc (right):

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_client.cc#newco...
net/dns/dns_client.cc:90: 
On 2011/12/06 18:08:39, Matt Menke wrote:
> nit:  Only one blank line needed.

Done.

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_client_unittest.cc
File net/dns/dns_client_unittest.cc (right):

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_client_unittest...
net/dns/dns_client_unittest.cc:109: void SetUp() OVERRIDE {
On 2011/12/06 18:08:39, Matt Menke wrote:
> virtual

Done.

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_client_unittest...
net/dns/dns_client_unittest.cc:176: void TearDown() OVERRIDE {
On 2011/12/06 18:08:39, Matt Menke wrote:
> virtual

Done.

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_query.cc
File net/dns/dns_query.cc (right):

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_query.cc#newcode27
net/dns/dns_query.cc:27: + question_size);
On 2011/12/06 18:08:39, Matt Menke wrote:
> nit:  Move '+' onto previous line.

Done.

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_response.h
File net/dns/dns_response.h (right):

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_response.h#newc...
net/dns/dns_response.h:42: bool IsValid() const { return packet_ != NULL; }
On 2011/12/06 18:08:39, Matt Menke wrote:
> Should add a comment that this validates the parser, not the data it's
parsing.

Done.

> Also, are this and the uninitialized constructor even necessary?

It makes DnsResponse::Parser() a bit easier to implement. Could remove it if
it's distracting.

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_response_unitte...
File net/dns/dns_response_unittest.cc (right):

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_response_unitte...
net/dns/dns_response_unittest.cc:43: DnsRecordParser it(data, sizeof(data), 0);
On 2011/12/06 18:08:39, Matt Menke wrote:
> This variable name is a little weird.

Done.

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_response_unitte...
net/dns/dns_response_unittest.cc:48: // check that the last "." is never stored
On 2011/12/06 18:08:39, Matt Menke wrote:
> nit:  All these comments that are sentences should be capitalized and have
> periods, as per Google style guide.

Done.

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_response_unitte...
net/dns/dns_response_unittest.cc:79: 0xc0, 0x08,
On 2011/12/06 18:08:39, Matt Menke wrote:
> I believe these should be:
> 
> 0xc0, 0x08,
> 0xc0, 0x06,

Good catch! Done.

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_response_unitte...
net/dns/dns_response_unittest.cc:143: // Test truncated record
On 2011/12/06 18:08:39, Matt Menke wrote:
> nit:  period.

Done.

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_response_unitte...
net/dns/dns_response_unittest.cc:190: 0xc0, 0x35,         // NAME is a pointer
to name in Question section.
On 2011/12/06 18:08:39, Matt Menke wrote:
> Mind fixing this comment?  That looks to be in the Answer 1 section to me.

Done.

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_response_unitte...
net/dns/dns_response_unittest.cc:203: // reject too short
On 2011/12/06 18:08:39, Matt Menke wrote:
> nit:  Convert to sentence / capitalize / period, as per Google style
guidelines.
>  Goes for the other comments in this test, too.

Done.

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_transaction_uni...
File net/dns/dns_transaction_unittest.cc (right):

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_transaction_uni...
net/dns/dns_transaction_unittest.cc:33: void SetUp() OVERRIDE {
On 2011/12/06 18:08:39, Matt Menke wrote:
> virtual

Done.

http://codereview.chromium.org/8762001/diff/23001/net/dns/dns_transaction_uni...
net/dns/dns_transaction_unittest.cc:77: std::string qname_;
On 2011/12/06 18:08:39, Matt Menke wrote:
> Think these should still be private.  Just use accessors for any you need to
> read from.  Makes which are accessed/modified by individual tests a little
> clearer.

Done.

[mmenke, 2011-12-06 20:43:01]

LGTM, just a couple nits.

http://codereview.chromium.org/8762001/diff/10004/net/base/big_endian.h
File net/base/big_endian.h (right):

http://codereview.chromium.org/8762001/diff/10004/net/base/big_endian.h#newcode9
net/base/big_endian.h:9: #include "base/string_piece.h"
Can you just forward declare base::StringPiece?

http://codereview.chromium.org/8762001/diff/10004/net/dns/async_host_resolver.h
File net/dns/async_host_resolver.h (right):

http://codereview.chromium.org/8762001/diff/10004/net/dns/async_host_resolver...
net/dns/async_host_resolver.h:51: const DnsResponse* transaction);
You probably missed the comment, but virtual doesn't seem to be needed here.

http://codereview.chromium.org/8762001/diff/10004/net/dns/async_host_resolver...
File net/dns/async_host_resolver_unittest.cc (right):

http://codereview.chromium.org/8762001/diff/10004/net/dns/async_host_resolver...
net/dns/async_host_resolver_unittest.cc:66: int Start() OVERRIDE {
virtual

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_client.cc
File net/dns/dns_client.cc (right):

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_client.cc#newco...
net/dns/dns_client.cc:42: int Start() OVERRIDE {
virtual

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_client.h
File net/dns/dns_client.h (right):

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_client.h#newcode14
net/dns/dns_client.h:14: #include "base/string_piece.h"
Can you just use a forward declaration instead of including this?

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_client.h#newcode16
net/dns/dns_client.h:16: #include "net/dns/dns_config_service.h"
Don't think this is needed.

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_query.cc
File net/dns/dns_query.cc (right):

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_query.cc#newcode50
net/dns/dns_query.cc:50: const dns_protocol::Header* h =
nit:  Rename variable to header or something.

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_query.h
File net/dns/dns_query.h (right):

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_query.h#newcode12
net/dns/dns_query.h:12: #include "net/base/io_buffer.h"
Looks like you forward declare IOBufferWithSize, so I don't think this is
needed.

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_response.cc
File net/dns/dns_response.cc (right):

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:25: DCHECK(offset <= length);
nit:  Use DCHECK_LE here and in ParseName, where applicable.

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:152: question.size()))
nit:  Add parentheses.

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_response.h
File net/dns/dns_response.h (right):

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_response.h#newc...
net/dns/dns_response.h:15: #include "net/base/net_util.h"
Think you can just forward declare IPAddressList.

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_response.h#newc...
net/dns/dns_response.h:16: #include "net/dns/dns_protocol.h"
Can we just forward declare dns_protocol::Header?

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_response.h#newc...
net/dns/dns_response.h:18: namespace net{
Mind adding a space here?

[szym, 2011-12-06 21:06:43]

Thanks!

http://codereview.chromium.org/8762001/diff/10004/net/base/big_endian.h
File net/base/big_endian.h (right):

http://codereview.chromium.org/8762001/diff/10004/net/base/big_endian.h#newcode9
net/base/big_endian.h:9: #include "base/string_piece.h"
On 2011/12/06 20:43:01, Matt Menke wrote:
> Can you just forward declare base::StringPiece?

Done.

http://codereview.chromium.org/8762001/diff/10004/net/dns/async_host_resolver.h
File net/dns/async_host_resolver.h (right):

http://codereview.chromium.org/8762001/diff/10004/net/dns/async_host_resolver...
net/dns/async_host_resolver.h:51: const DnsResponse* transaction);
On 2011/12/06 20:43:01, Matt Menke wrote:
> You probably missed the comment, but virtual doesn't seem to be needed here.

Done.

http://codereview.chromium.org/8762001/diff/10004/net/dns/async_host_resolver...
File net/dns/async_host_resolver_unittest.cc (right):

http://codereview.chromium.org/8762001/diff/10004/net/dns/async_host_resolver...
net/dns/async_host_resolver_unittest.cc:66: int Start() OVERRIDE {
On 2011/12/06 20:43:01, Matt Menke wrote:
> virtual

Done.

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_client.cc
File net/dns/dns_client.cc (right):

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_client.cc#newco...
net/dns/dns_client.cc:42: int Start() OVERRIDE {
On 2011/12/06 20:43:01, Matt Menke wrote:
> virtual

Done.

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_client.h
File net/dns/dns_client.h (right):

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_client.h#newcode14
net/dns/dns_client.h:14: #include "base/string_piece.h"
On 2011/12/06 20:43:01, Matt Menke wrote:
> Can you just use a forward declaration instead of including this?

Done.

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_client.h#newcode16
net/dns/dns_client.h:16: #include "net/dns/dns_config_service.h"
On 2011/12/06 20:43:01, Matt Menke wrote:
> Don't think this is needed.

Done.

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_query.cc
File net/dns/dns_query.cc (right):

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_query.cc#newcode50
net/dns/dns_query.cc:50: const dns_protocol::Header* h =
On 2011/12/06 20:43:01, Matt Menke wrote:
> nit:  Rename variable to header or something.

Done. (I thought I caught all of them.)

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_query.h
File net/dns/dns_query.h (right):

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_query.h#newcode12
net/dns/dns_query.h:12: #include "net/base/io_buffer.h"
On 2011/12/06 20:43:01, Matt Menke wrote:
> Looks like you forward declare IOBufferWithSize, so I don't think this is
> needed.

Done.

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_response.cc
File net/dns/dns_response.cc (right):

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:25: DCHECK(offset <= length);
On 2011/12/06 20:43:01, Matt Menke wrote:
> nit:  Use DCHECK_LE here and in ParseName, where applicable.

Done.

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_response.cc#new...
net/dns/dns_response.cc:152: question.size()))
On 2011/12/06 20:43:01, Matt Menke wrote:
> nit:  Add parentheses.

Done.

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_response.h
File net/dns/dns_response.h (right):

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_response.h#newc...
net/dns/dns_response.h:15: #include "net/base/net_util.h"
On 2011/12/06 20:43:01, Matt Menke wrote:
> Think you can just forward declare IPAddressList.

Can't forward declare typedef :(

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_response.h#newc...
net/dns/dns_response.h:16: #include "net/dns/dns_protocol.h"
On 2011/12/06 20:43:01, Matt Menke wrote:
> Can we just forward declare dns_protocol::Header?

Done.

http://codereview.chromium.org/8762001/diff/10004/net/dns/dns_response.h#newc...
net/dns/dns_response.h:18: namespace net{
On 2011/12/06 20:43:01, Matt Menke wrote:
> Mind adding a space here?

Done.

[commit-bot: I haz the power, 2011-12-06 21:13:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/8762001/32004

[commit-bot: I haz the power, 2011-12-06 21:26:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/8762001/30003

[commit-bot: I haz the power, 2011-12-06 21:26:42]

Can't process patch for file net/dns/dns_session.h.
File's status is None, patchset upload is incomplete.

[commit-bot: I haz the power, 2011-12-06 21:30:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/8762001/30004

[commit-bot: I haz the power, 2011-12-06 23:33:00]

Change committed as 113282