Make test URLFetcher implementations not derive from the URLFetcher implementation, since we want...

Make test URLFetcher implementations not derive from the URLFetcher implementation, since we want to hide that from chrome completely.

SetBackoffDelayForTesting moves from content::UrlFetcher to TestURLFetcher, now that the test objects derive from it.

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

[jam, 2011-10-26 21:23:03]

CCing Will: this is what we talked about

[willchan no longer on Chromium, 2011-10-26 21:24:42]

Sounds great. I defer the review to wtc.

On Wed, Oct 26, 2011 at 2:23 PM, <jam@chromium.org> wrote:

> CCing Will: this is what we talked about
>
>
http://codereview.chromium.**org/8395038/<http://codereview.chromium.org/8395...
>

[wtc, 2011-10-26 23:34:45]

Patch Set 7 LGTM.

I did a cursory review, so I'm counting on you to check this
carefully.  Thank you for working on this refactoring.

My main complaint is that the CL's commit message is terse.
It would be nice to describe the main changes in this CL.

http://codereview.chromium.org/8395038/diff/9023/content/common/net/url_fetch...
File content/common/net/url_fetcher.cc (right):

http://codereview.chromium.org/8395038/diff/9023/content/common/net/url_fetch...
content/common/net/url_fetcher.cc:467: static content::URLFetcherFactory*
g_factory = NULL;

Remove the "// static" comment on the previous line because
this is no longer a static data member.

http://codereview.chromium.org/8395038/diff/9023/content/public/common/url_fe...
File content/public/common/url_fetcher.h (left):

http://codereview.chromium.org/8395038/diff/9023/content/public/common/url_fe...
content/public/common/url_fetcher.h:162: virtual void
SetBackoffDelayForTesting(base::TimeDelta backoff_delay) = 0;

This change should be mentioned in the CL's commit message.
(I found that this method now becomes TestURLFetcher::set_backoff_delay.)

http://codereview.chromium.org/8395038/diff/9023/content/public/common/url_fe...
File content/public/common/url_fetcher.h (right):

http://codereview.chromium.org/8395038/diff/9023/content/public/common/url_fe...
content/public/common/url_fetcher.h:13: #include "base/memory/ref_counted.h"

Nit: list this header before "base/platform_file.h".

[jam, 2011-10-26 23:39:06]

http://codereview.chromium.org/8395038/diff/9023/content/public/common/url_fe...
File content/public/common/url_fetcher.h (left):

http://codereview.chromium.org/8395038/diff/9023/content/public/common/url_fe...
content/public/common/url_fetcher.h:162: virtual void
SetBackoffDelayForTesting(base::TimeDelta backoff_delay) = 0;
On 2011/10/26 23:34:46, wtc wrote:
> 
> This change should be mentioned in the CL's commit message.
> (I found that this method now becomes TestURLFetcher::set_backoff_delay.)

I had just added this method to the interface in my last cl, and now I moved it
back to set_backoff_delay now that tests don't derive from URLFetcher, so I
didn't think it was worth calling out

http://codereview.chromium.org/8395038/diff/9023/content/public/common/url_fe...
File content/public/common/url_fetcher.h (right):

http://codereview.chromium.org/8395038/diff/9023/content/public/common/url_fe...
content/public/common/url_fetcher.h:13: #include "base/memory/ref_counted.h"
On 2011/10/26 23:34:46, wtc wrote:
> 
> Nit: list this header before "base/platform_file.h".

Done.

[wtc, 2011-10-27 21:29:16]

Review comments on Patch Set 7.

I made another pass through this CL.  My main complaint
is the easy confusion of URLFetcherFactory and
content::URLFetcherFactory.  I leave this up to you.

http://codereview.chromium.org/8395038/diff/9023/content/common/net/url_fetch...
File content/common/net/url_fetcher.cc (right):

http://codereview.chromium.org/8395038/diff/9023/content/common/net/url_fetch...
content/common/net/url_fetcher.cc:467: static content::URLFetcherFactory*
g_factory = NULL;

Just wondering: why did you change this static member to a
global variable?

http://codereview.chromium.org/8395038/diff/9023/content/public/common/url_fe...
File content/public/common/url_fetcher.h (right):

http://codereview.chromium.org/8395038/diff/9023/content/public/common/url_fe...
content/public/common/url_fetcher.h:194: virtual const GURL& GetUrl() const = 0;

Nit: these two methods should be named GetOriginalURL
and GetURL, in keeping with the way "URL" is spelled in
URLFetcher.

http://codereview.chromium.org/8395038/diff/9023/content/test/test_url_fetche...
File content/test/test_url_fetcher_factory.cc (right):

http://codereview.chromium.org/8395038/diff/9023/content/test/test_url_fetche...
content/test/test_url_fetcher_factory.cc:31: content::URLFetcherDelegate* d)

The |request_type| argument is not used.  I am not sure if
we should remove the argument though.

http://codereview.chromium.org/8395038/diff/9023/content/test/test_url_fetche...
content/test/test_url_fetcher_factory.cc:238: success ?
net::URLRequestStatus::SUCCESS :net::URLRequestStatus::FAILED,

Add a space before net::URLRequestStatus::FAILED.  You may
need to fold the line.

http://codereview.chromium.org/8395038/diff/9023/content/test/test_url_fetche...
File content/test/test_url_fetcher_factory.h (right):

http://codereview.chromium.org/8395038/diff/9023/content/test/test_url_fetche...
content/test/test_url_fetcher_factory.h:32: // URLFetcher. TestURLFetcherFactory
is a URLFetcher::Factory that creates

There are still references to URLFetcher::Factory in the
comments in this file.  You may want to do a global search
for that.

http://codereview.chromium.org/8395038/diff/9023/content/test/test_url_fetche...
content/test/test_url_fetcher_factory.h:53: // might want to use the
FakeUrlFetcher and FakeUrlFetcherFactory classes

Typo: FakeUrlFetcherFactory => FakeURLFetcherFactory

FakeUrlFetcher may also be a typo.  I found a FakeURLFetcher
class defined in content/test/test_url_fetcher_factory.cc.

http://codereview.chromium.org/8395038/diff/9023/content/test/test_url_fetche...
content/test/test_url_fetcher_factory.h:272: class URLFetcherFactory : public
content::URLFetcherFactory {

Although the content namespace disambiguates the two
URLFetcherFactory classes, this seems like a bad idea.
Can we avoid using the same name?

One way to solve this problem is to turn content::URLFetcherFactory
back into a nested type content::URLFetcher::Factory.

[jam, 2011-10-27 22:34:41]

thanks for the thorough passover, fixes in
http://codereview.chromium.org/8416020/

http://codereview.chromium.org/8395038/diff/9023/content/common/net/url_fetch...
File content/common/net/url_fetcher.cc (right):

http://codereview.chromium.org/8395038/diff/9023/content/common/net/url_fetch...
content/common/net/url_fetcher.cc:467: static content::URLFetcherFactory*
g_factory = NULL;
On 2011/10/27 21:29:16, wtc wrote:
> 
> Just wondering: why did you change this static member to a
> global variable?

because now I need to use it from the static content::URLFetcher method

http://codereview.chromium.org/8395038/diff/9023/content/public/common/url_fe...
File content/public/common/url_fetcher.h (right):

http://codereview.chromium.org/8395038/diff/9023/content/public/common/url_fe...
content/public/common/url_fetcher.h:194: virtual const GURL& GetUrl() const = 0;
On 2011/10/27 21:29:16, wtc wrote:
> 
> Nit: these two methods should be named GetOriginalURL
> and GetURL, in keeping with the way "URL" is spelled in
> URLFetcher.

good point. done.

http://codereview.chromium.org/8395038/diff/9023/content/test/test_url_fetche...
File content/test/test_url_fetcher_factory.cc (right):

http://codereview.chromium.org/8395038/diff/9023/content/test/test_url_fetche...
content/test/test_url_fetcher_factory.cc:31: content::URLFetcherDelegate* d)
On 2011/10/27 21:29:16, wtc wrote:
> 
> The |request_type| argument is not used.  I am not sure if
> we should remove the argument though.

when doing this originally i wasn't sure if it's needed or not. now that we know
that we don't, removed.

http://codereview.chromium.org/8395038/diff/9023/content/test/test_url_fetche...
content/test/test_url_fetcher_factory.cc:238: success ?
net::URLRequestStatus::SUCCESS :net::URLRequestStatus::FAILED,
On 2011/10/27 21:29:16, wtc wrote:
> 
> Add a space before net::URLRequestStatus::FAILED.  You may
> need to fold the line.

Done.

http://codereview.chromium.org/8395038/diff/9023/content/test/test_url_fetche...
File content/test/test_url_fetcher_factory.h (right):

http://codereview.chromium.org/8395038/diff/9023/content/test/test_url_fetche...
content/test/test_url_fetcher_factory.h:32: // URLFetcher. TestURLFetcherFactory
is a URLFetcher::Factory that creates
On 2011/10/27 21:29:16, wtc wrote:
> 
> There are still references to URLFetcher::Factory in the
> comments in this file.  You may want to do a global search
> for that.

Done.

http://codereview.chromium.org/8395038/diff/9023/content/test/test_url_fetche...
content/test/test_url_fetcher_factory.h:53: // might want to use the
FakeUrlFetcher and FakeUrlFetcherFactory classes
On 2011/10/27 21:29:16, wtc wrote:
> 
> Typo: FakeUrlFetcherFactory => FakeURLFetcherFactory
> 
> FakeUrlFetcher may also be a typo.  I found a FakeURLFetcher
> class defined in content/test/test_url_fetcher_factory.cc.

Done.

http://codereview.chromium.org/8395038/diff/9023/content/test/test_url_fetche...
content/test/test_url_fetcher_factory.h:272: class URLFetcherFactory : public
content::URLFetcherFactory {
On 2011/10/27 21:29:16, wtc wrote:
> 
> Although the content namespace disambiguates the two
> URLFetcherFactory classes, this seems like a bad idea.
> Can we avoid using the same name?

sure, good point. i've renamed it to URLFetcherImplFactory to make what it does
obvious 
> 
> One way to solve this problem is to turn content::URLFetcherFactory
> back into a nested type content::URLFetcher::Factory.

the new convention, afaik, is to avoid nested classes, that's why i split it
off.