Split DiskCacheBasedSSLHostInfo unit tests to its own

Split DiskCacheBasedSSLHostInfo unit tests to its own
file. I'll be adding more unit tests so it's a good time
to do this.

BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111217

[gavinp, 2011-11-18 14:26:22]

Is this upload OK for this cl?  disk_cache_based_ssl_host_info_unittest.cc
doesn't exist, yet this CL shows a 5180 line deletion from it.  I'm very
confused, and reading the raw diffs hasn't helped this time.

http://codereview.chromium.org/8588011/diff/5002/net/http/mock_http_cache.cc
File net/http/mock_http_cache.cc (right):

http://codereview.chromium.org/8588011/diff/5002/net/http/mock_http_cache.cc#...
net/http/mock_http_cache.cc:284: void
MockDiskEntry::RunCallback(net::OldCompletionCallback* callback, int result) {
80 chars.

http://codereview.chromium.org/8588011/diff/5002/net/http/mock_http_cache.cc#...
net/http/mock_http_cache.cc:307: net::OldCompletionCallback* callback,
80 chars.

http://codereview.chromium.org/8588011/diff/5002/net/http/mock_http_cache.h
File net/http/mock_http_cache.h (right):

http://codereview.chromium.org/8588011/diff/5002/net/http/mock_http_cache.h#n...
net/http/mock_http_cache.h:115: std::vector<std::pair<std::string, std::string>
>* stats) OVERRIDE;
Nit:

#include <utility>
#include <vector>

[gavinp, 2011-11-18 14:29:04]

It looks like all of mock_http_cache.cc, mock_http_cache.h and
disk_cache_based_ssl_host_info._unittest.cc are copies fo
http_cache_unittest.cc.

[rvargas (doing something else), 2011-11-18 19:12:21]

On 2011/11/18 14:29:04, gavinp wrote:
> It looks like all of mock_http_cache.cc, mock_http_cache.h and
> disk_cache_based_ssl_host_info._unittest.cc are copies fo
> http_cache_unittest.cc.

Yes, that's correct. I should not be adding or removing any code here, just
splitting http_cache_unittest.cc in four parts (svn copy). After that, code just
moves around, but I tried to keep that to a minimum to make the review as easy
as possible.

[rvargas (doing something else), 2011-11-18 19:24:58]

http://codereview.chromium.org/8588011/diff/5002/net/http/mock_http_cache.cc
File net/http/mock_http_cache.cc (right):

http://codereview.chromium.org/8588011/diff/5002/net/http/mock_http_cache.cc#...
net/http/mock_http_cache.cc:284: void
MockDiskEntry::RunCallback(net::OldCompletionCallback* callback, int result) {
On 2011/11/18 14:26:22, gavinp wrote:
> 80 chars.

This will go away when OldCompletionCallback is replaced with CompletionCallback
(the first part of that change didn't fix formatting)

http://codereview.chromium.org/8588011/diff/5002/net/http/mock_http_cache.h
File net/http/mock_http_cache.h (right):

http://codereview.chromium.org/8588011/diff/5002/net/http/mock_http_cache.h#n...
net/http/mock_http_cache.h:115: std::vector<std::pair<std::string, std::string>
>* stats) OVERRIDE;
On 2011/11/18 14:26:22, gavinp wrote:
> Nit:
> 
> #include <utility>
> #include <vector>

I know that's what lint wants, but historically we've tried to minimize the
number of includes (especially on header files) to keep compiling as fast as
possible. My general rule is to remove all I can until it fails to compile.
(note that our rule of keeping at least one cc file that has one of our h files
as the first include makes sure that we don't miss anything).

[gavinp, 2011-11-22 18:33:48]

Thanks for your patience, I just needed to track this big file being split up.

http://codereview.chromium.org/8588011/diff/5002/net/http/mock_http_cache.cc
File net/http/mock_http_cache.cc (right):

http://codereview.chromium.org/8588011/diff/5002/net/http/mock_http_cache.cc#...
net/http/mock_http_cache.cc:284: void
MockDiskEntry::RunCallback(net::OldCompletionCallback* callback, int result) {
SGTM!  Just CC me or add me as a reviewer when that comes down the line, please.

On 2011/11/18 19:24:58, rvargas wrote:
> On 2011/11/18 14:26:22, gavinp wrote:
> > 80 chars.
> 
> This will go away when OldCompletionCallback is replaced with
CompletionCallback
> (the first part of that change didn't fix formatting)

http://codereview.chromium.org/8588011/diff/5002/net/http/mock_http_cache.h
File net/http/mock_http_cache.h (right):

http://codereview.chromium.org/8588011/diff/5002/net/http/mock_http_cache.h#n...
net/http/mock_http_cache.h:115: std::vector<std::pair<std::string, std::string>
>* stats) OVERRIDE;
I don't think that's a good rule in 2011; compilers have avoided double-includes
for quite some time; both #pragma once and automatic include guard detection are
provided by most of our compilers already.

I think it'd be best to defer to lint here; including what you use makes future
edits easier.

On 2011/11/18 19:24:58, rvargas wrote:
> On 2011/11/18 14:26:22, gavinp wrote:
> > Nit:
> > 
> > #include <utility>
> > #include <vector>
> 
> I know that's what lint wants, but historically we've tried to minimize the
> number of includes (especially on header files) to keep compiling as fast as
> possible. My general rule is to remove all I can until it fails to compile.
> (note that our rule of keeping at least one cc file that has one of our h
files
> as the first include makes sure that we don't miss anything).

[rvargas (doing something else), 2011-11-22 18:58:10]

http://codereview.chromium.org/8588011/diff/5002/net/http/mock_http_cache.cc
File net/http/mock_http_cache.cc (right):

http://codereview.chromium.org/8588011/diff/5002/net/http/mock_http_cache.cc#...
net/http/mock_http_cache.cc:284: void
MockDiskEntry::RunCallback(net::OldCompletionCallback* callback, int result) {
I'm not the one working on that change. Will is doing that, and I have no idea
when he plans on going back to the old name, I just know that he intentionally
didn't fix the line overflows because find/replace was going to fix that at some
point.

On 2011/11/22 18:33:48, gavinp wrote:
> SGTM!  Just CC me or add me as a reviewer when that comes down the line,
please.
> 
> On 2011/11/18 19:24:58, rvargas wrote:
> > On 2011/11/18 14:26:22, gavinp wrote:
> > > 80 chars.
> > 
> > This will go away when OldCompletionCallback is replaced with
> CompletionCallback
> > (the first part of that change didn't fix formatting)
>

http://codereview.chromium.org/8588011/diff/5002/net/http/mock_http_cache.h
File net/http/mock_http_cache.h (right):

http://codereview.chromium.org/8588011/diff/5002/net/http/mock_http_cache.h#n...
net/http/mock_http_cache.h:115: std::vector<std::pair<std::string, std::string>
>* stats) OVERRIDE;
Looking at this particular case (without delving into what lint should do or
not), we are talking about the disk cache interface. There is no way this file
can lose the requirement to include disk_cache.h without any serious
repurposing, so I see no value in requiring this file to include things that are
required for the interface declaration. That is not the most common pattern in
our code.

As for what a compiler can or cannot do, remember that as recently as one year
ago we were measuring the effect of adding pragma once to all the files given
the speedup provided on some platforms
(https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr...).

Of course, I can add the two files if you insist, I just don't agree that there
is any advantage in doing so (and there is a potential downside given that we
really don't control the compilers used to build net).

[gavinp, 2011-11-22 19:59:29]

http://codereview.chromium.org/8588011/diff/5002/net/http/mock_http_cache.h
File net/http/mock_http_cache.h (right):

http://codereview.chromium.org/8588011/diff/5002/net/http/mock_http_cache.h#n...
net/http/mock_http_cache.h:115: std::vector<std::pair<std::string, std::string>
>* stats) OVERRIDE;
I remember that thread well.  But IWYU was specifically spoken about in
chromium-dev just last week:
https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr...

The consensus was that we should do it.  I'm going to have to argue for
consistency over the speedup on platforms that don't have #pragma once system
libraries.  :(

On 2011/11/22 18:58:10, rvargas wrote:
> Looking at this particular case (without delving into what lint should do or
> not), we are talking about the disk cache interface. There is no way this file
> can lose the requirement to include disk_cache.h without any serious
> repurposing, so I see no value in requiring this file to include things that
are
> required for the interface declaration. That is not the most common pattern in
> our code.
> 
> As for what a compiler can or cannot do, remember that as recently as one year
> ago we were measuring the effect of adding pragma once to all the files given
> the speedup provided on some platforms
>
(https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr...).
> 
> Of course, I can add the two files if you insist, I just don't agree that
there
> is any advantage in doing so (and there is a potential downside given that we
> really don't control the compilers used to build net).

[gavinp, 2011-11-22 20:18:29]

LGTM, and I'm writing a followup to chromium-dev to talk about transitivity and
IWYU.

http://codereview.chromium.org/8588011/diff/5002/net/http/mock_http_cache.h
File net/http/mock_http_cache.h (right):

http://codereview.chromium.org/8588011/diff/5002/net/http/mock_http_cache.h#n...
net/http/mock_http_cache.h:115: std::vector<std::pair<std::string, std::string>
>* stats) OVERRIDE;
Annnd... I appear to have missed the point.  I've reflected on this more; since
this is an OVERRIDE, it's compile-time asserted that the ancestor class has the
same method defined, and so just as you don't need to explicitly include your
ancestor's ancestor, you are good here.

My apologies for the tooth-pulling that it seemed like to convince me of this. 
I do not believe that classes used in OVERRIDE methods need tickle an IWYU rule
now.

On 2011/11/22 19:59:29, gavinp wrote:
> I remember that thread well.  But IWYU was specifically spoken about in
> chromium-dev just last week:
>
https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr...
> 
> The consensus was that we should do it.  I'm going to have to argue for
> consistency over the speedup on platforms that don't have #pragma once system
> libraries.  :(
> 
> On 2011/11/22 18:58:10, rvargas wrote:
> > Looking at this particular case (without delving into what lint should do or
> > not), we are talking about the disk cache interface. There is no way this
file
> > can lose the requirement to include disk_cache.h without any serious
> > repurposing, so I see no value in requiring this file to include things that
> are
> > required for the interface declaration. That is not the most common pattern
in
> > our code.
> > 
> > As for what a compiler can or cannot do, remember that as recently as one
year
> > ago we were measuring the effect of adding pragma once to all the files
given
> > the speedup provided on some platforms
> >
>
(https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr...).
> > 
> > Of course, I can add the two files if you insist, I just don't agree that
> there
> > is any advantage in doing so (and there is a potential downside given that
we
> > really don't control the compilers used to build net).
>