Add a ProxyScriptFetcher class for doing asynch downloads of PAC scripts....

Add a ProxyScriptFetcher class for doing asynch downloads of PAC scripts.This object will be owned by ProxyService. It will be used to manage the fetching of PAC scripts (on the IO thread, using the primary URLRequestContext).BUG=74, 2764
(partial)

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

[eroman, 2008-12-08 18:58:08]

(depends on codereview 12938)

[wtc, 2008-12-09 02:38:18]

I have reviewed everything but the unit test.
I'll finish the review tomorrow.  Here are the
review comments for the rest of the files.

http://codereview.chromium.org/13251/diff/1/8
File net/base/load_flags.h (right):

http://codereview.chromium.org/13251/diff/1/8#newcode68
Line 68: // Do not resolve URLs using the configured proxy service. This
override is
Nit: "resolve URLs" => "resolve proxy".

http://codereview.chromium.org/13251/diff/1/8#newcode70
Line 70: LOAD_BYPASS_PROXY_SERVICE = 1 << 15,
Perhaps just LOAD_BYPASS_PROXY or LOAD_USE_DIRECT_CONNECTION?
ProxyService seems like an implementation detail to me.

http://codereview.chromium.org/13251/diff/1/7
File net/base/net_error_list.h (right):

http://codereview.chromium.org/13251/diff/1/7#newcode97
Line 97: NET_ERROR(PAC_BAD_STATUS_CODE, -116)
Perhaps PAC_STATUS_NOT_OK?  A non-200 status code can still
be a valid HTTP status code.

Please make sure -1xx is the right section for these two
error codes.

http://codereview.chromium.org/13251/diff/1/10
File net/proxy/proxy_script_fetcher.cc (right):

http://codereview.chromium.org/13251/diff/1/10#newcode22
Line 22: size_t kMaxResponseSize = 1048576;  // 1 megabyte
The k prefix should only be used for constants.

http://codereview.chromium.org/13251/diff/1/10#newcode26
Line 26: size_t kMaxDuration = 300;  // 5 minutes
The k prefix should only be used for constants.

http://codereview.chromium.org/13251/diff/1/10#newcode42
Line 42: } // namespace
Nit: run cpplint.py.

http://codereview.chromium.org/13251/diff/1/10#newcode55
Line 55: // ProxyScriptFetcher implementation
Nit: use the current convention in Chromium source tree for
this kind of comments.  The current convention is just a
one-liner:

  // ProxyScriptFetcher methods:

http://codereview.chromium.org/13251/diff/1/10#newcode126
Line 126: : url_request_context_(url_request_context),
Nit: I think the colon (:) only needs to be indented 4
spaces from the beginning of the line.

http://codereview.chromium.org/13251/diff/1/10#newcode128
Line 128: next_id_(1),
Nit: want to initialize this to 0 and do ++next_id_ in
ProxyScriptFetcherImpl::Fetch?

http://codereview.chromium.org/13251/diff/1/10#newcode176
Line 176: // Reset will free the URLRequest, which will cause cancellation.
Nit: "Reset" => "ResetCurRequestState"?

http://codereview.chromium.org/13251/diff/1/10#newcode200
Line 200: // OK, thanks for telling.
Does this mean we don't follow redirects?

http://codereview.chromium.org/13251/diff/1/10#newcode245
Line 245: } else {  // Error while reading.
Is num_bytes == 0 also an error?  Why doesn't that mean EOF?

http://codereview.chromium.org/13251/diff/1/10#newcode266
Line 266: // Read completed synchronously.
Does this mean "Read failed synchronously"?

http://codereview.chromium.org/13251/diff/1/11
File net/proxy/proxy_script_fetcher.h (right):

http://codereview.chromium.org/13251/diff/1/11#newcode5
Line 5: // ProxyScriptFetcher is an asynch interface for fetching a proxy auto
config
Nit: we should standardize on using "async" as the
abbreviation of asynchronous.

http://codereview.chromium.org/13251/diff/1/11#newcode6
Line 6: // script. It is specific to fetching a PAC script, in the constraints
it
Not sure what you meant by "in the constraints".

http://codereview.chromium.org/13251/diff/1/11#newcode20
Line 20: class ProxyScriptFetcher {
Nit: should we name this class PacScriptFetcher?

http://codereview.chromium.org/13251/diff/1/11#newcode23
Line 23: virtual ~ProxyScriptFetcher() { }
Nit: run cpplint.py.

http://codereview.chromium.org/13251/diff/1/11#newcode26
Line 26: virtual void Cancel() = 0;
Nit: list Cancel after Fetch.  It'll make the interface
easier to understand.  For example, I would have known that
only one fetch is allowed to be outstanding at a time.

http://codereview.chromium.org/13251/diff/1/11#newcode30
Line 30: // string of the response bytes. On failure, the response bytes is an
empty
Nit: in this line and the next line, change "response" to
"result".

http://codereview.chromium.org/13251/diff/1/11#newcode34
Line 34: //    ERR_TIMED_OUT        -- the file took too long to complete.
Nit: "file" => "fetch" or "file fetch"?

http://codereview.chromium.org/13251/diff/1/11#newcode38
Line 38: //    ERR_NOT_IMPLEMENTED  -- if the response required authentication.
Should we add an ERR_AUTH_REQUIRED error code?  I guess in
this case you want ERR_NOT_IMPLEMENTED because you will
eventually implement auth support?

http://codereview.chromium.org/13251/diff/1/11#newcode59
Line 59: static size_t OverrideTimeoutConstraintForUnittest(size_t new_timeout);
Nit: use base::TimeDelta instead of size_t?

Use "Set" instead of "Override" in these two method names?

http://codereview.chromium.org/13251/diff/1/11#newcode61
Line 61: // Sets the maximum-reponse size for a fetch to |new_size| (bytes).
Returns
"reponse" => "response"

http://codereview.chromium.org/13251/diff/1/9
File net/proxy/proxy_script_fetcher_unittest.cc (right):

http://codereview.chromium.org/13251/diff/1/9#newcode36
Line 36: delete proxy_service_;
This is the nth time I've seen this code sequence.  I think
we should define these two members as scoped_ptr in the
appropriate order.

[darin (slow to review), 2008-12-09 16:19:47]

Great stuff!

http://codereview.chromium.org/13251/diff/1/8
File net/base/load_flags.h (right):

http://codereview.chromium.org/13251/diff/1/8#newcode70
Line 70: LOAD_BYPASS_PROXY_SERVICE = 1 << 15,
LOAD_BYPASS_PROXY sounds good to me, but I'm probably biased since mozilla also
has a define like that ;-)

http://codereview.chromium.org/13251/diff/1/7
File net/base/net_error_list.h (right):

http://codereview.chromium.org/13251/diff/1/7#newcode94
Line 94: NET_ERROR(PAC_BAD_MIME_TYPE, -115)
is Firefox also picky about the server reported mime type?

http://codereview.chromium.org/13251/diff/1/10
File net/proxy/proxy_script_fetcher.cc (right):

http://codereview.chromium.org/13251/diff/1/10#newcode134
Line 134: CHECK(url_request_context);
nit: DCHECK should be fine

http://codereview.chromium.org/13251/diff/1/10#newcode146
Line 146: CHECK(!cur_request_.get());
nit: can these just be DCHECKs?

http://codereview.chromium.org/13251/diff/1/10#newcode182
Line 182: CHECK(request == cur_request_.get());
nit: can this just be a DCHECK?

http://codereview.chromium.org/13251/diff/1/10#newcode191
Line 191: CHECK(request == cur_request_.get());
nit: DCHECK?

http://codereview.chromium.org/13251/diff/1/10#newcode199
Line 199: CHECK(request == cur_request_.get());
nit: DCHECK?

http://codereview.chromium.org/13251/diff/1/10#newcode204
Line 204: CHECK(request == cur_request_.get());
nit: DCHECK?

http://codereview.chromium.org/13251/diff/1/10#newcode235
Line 235: CHECK(request == cur_request_.get());
nit: DCHECK? (same question applies to remaining CHECKs)

http://codereview.chromium.org/13251/diff/1/11
File net/proxy/proxy_script_fetcher.h (right):

http://codereview.chromium.org/13251/diff/1/11#newcode44
Line 44: virtual void Fetch(const GURL& url, std::string* bytes,
maybe Fetch should also return an |int| error code?

[wtc, 2008-12-09 21:06:56]

http://codereview.chromium.org/13251/diff/1/11
File net/proxy/proxy_script_fetcher.h (right):

http://codereview.chromium.org/13251/diff/1/11#newcode44
Line 44: virtual void Fetch(const GURL& url, std::string* bytes,
On 2008/12/09 16:19:47, darin wrote:
> maybe Fetch should also return an |int| error code?

I guess the assumption is that Fetch always completes
asynchronously?

[eroman, 2008-12-09 21:47:02]

http://codereview.chromium.org/13251/diff/1/8
File net/base/load_flags.h (right):

http://codereview.chromium.org/13251/diff/1/8#newcode68
Line 68: // Do not resolve URLs using the configured proxy service. This
override is
On 2008/12/09 02:38:18, wtc wrote:
> Nit: "resolve URLs" => "resolve proxy".

Done. Reworded the paragraph to:

// Do not resolve proxies. This override is used when downloading PAC files
// to avoid having a circular dependency.

http://codereview.chromium.org/13251/diff/1/8#newcode70
Line 70: LOAD_BYPASS_PROXY_SERVICE = 1 << 15,
On 2008/12/09 02:38:18, wtc wrote:
> Perhaps just LOAD_BYPASS_PROXY or LOAD_USE_DIRECT_CONNECTION?
> ProxyService seems like an implementation detail to me.

Done. Renamed to LOAD_BYPASS_PROXY.

http://codereview.chromium.org/13251/diff/1/7
File net/base/net_error_list.h (right):

http://codereview.chromium.org/13251/diff/1/7#newcode97
Line 97: NET_ERROR(PAC_BAD_STATUS_CODE, -116)
On 2008/12/09 02:38:18, wtc wrote:
> Perhaps PAC_STATUS_NOT_OK?  A non-200 status code can still
> be a valid HTTP status code.

Done. Renamed to PAC_STATUS_NOT_OK.
> 
> Please make sure -1xx is the right section for these two
> error codes.

Changed to a -3xx, although I am still hazy on what the groupings mean. (-1xx
however looks to be connection errors which is clearly wrong).

http://codereview.chromium.org/13251/diff/1/10
File net/proxy/proxy_script_fetcher.cc (right):

http://codereview.chromium.org/13251/diff/1/10#newcode22
Line 22: size_t kMaxResponseSize = 1048576;  // 1 megabyte
On 2008/12/09 02:38:18, wtc wrote:
> The k prefix should only be used for constants.

Done.

Renamed kMaxResponseSize --> max_response_size_bytes
Renamed kMaxDuration --> max_duration_ms

Note the reason these are not const, is to allow the unit-test to override them.

http://codereview.chromium.org/13251/diff/1/10#newcode26
Line 26: size_t kMaxDuration = 300;  // 5 minutes
On 2008/12/09 02:38:18, wtc wrote:
> The k prefix should only be used for constants.

Done, see comment above.

http://codereview.chromium.org/13251/diff/1/10#newcode42
Line 42: } // namespace
On 2008/12/09 02:38:18, wtc wrote:
> Nit: run cpplint.py.

Done.

http://codereview.chromium.org/13251/diff/1/10#newcode55
Line 55: // ProxyScriptFetcher implementation
On 2008/12/09 02:38:18, wtc wrote:
> Nit: use the current convention in Chromium source tree for
> this kind of comments.  The current convention is just a
> one-liner:
> 
>   // ProxyScriptFetcher methods:

Done.

http://codereview.chromium.org/13251/diff/1/10#newcode126
Line 126: : url_request_context_(url_request_context),
On 2008/12/09 02:38:18, wtc wrote:
> Nit: I think the colon (:) only needs to be indented 4
> spaces from the beginning of the line.
Done.

http://codereview.chromium.org/13251/diff/1/10#newcode128
Line 128: next_id_(1),
On 2008/12/09 02:38:18, wtc wrote:
> Nit: want to initialize this to 0 and do ++next_id_ in
> ProxyScriptFetcherImpl::Fetch?
> 

If I go with pre-increment, I would also want to change the name (since it is no
longer the "next id", but before-next-id). Perhaps "id_ = ++request_count_;" in
such case?

For now I have left it as is; let me know how to proceed.

http://codereview.chromium.org/13251/diff/1/10#newcode146
Line 146: CHECK(!cur_request_.get());
On 2008/12/09 16:19:47, darin wrote:
> nit: can these just be DCHECKs?

Done -- changed all the CHECK to DCHECK.

I have some style concerns on how we use DCHECK, but I will raise those later.

http://codereview.chromium.org/13251/diff/1/10#newcode176
Line 176: // Reset will free the URLRequest, which will cause cancellation.
On 2008/12/09 02:38:18, wtc wrote:
> Nit: "Reset" => "ResetCurRequestState"?

Done.

http://codereview.chromium.org/13251/diff/1/10#newcode200
Line 200: // OK, thanks for telling.
On 2008/12/09 02:38:18, wtc wrote:
> Does this mean we don't follow redirects?

We do follow redirects.

This delegate method is just for receiving notification of such a change. Even
though we don't we don't do anything in response, it must be implemented
(abstract).

I wander if we care about http:// redirecting to a file:// at this layer.
Probably not.

http://codereview.chromium.org/13251/diff/1/10#newcode245
Line 245: } else {  // Error while reading.
On 2008/12/09 02:38:18, wtc wrote:
> Is num_bytes == 0 also an error?  Why doesn't that mean EOF?

Changed the comment to read:

// Error while reading, or EOF

The logic should be correct -- when 0 bytes is read we decide the response is
completed.

http://codereview.chromium.org/13251/diff/1/10#newcode266
Line 266: // Read completed synchronously.
On 2008/12/09 02:38:18, wtc wrote:
> Does this mean "Read failed synchronously"?

Correct. Changed the comment to:

// Read failed synchronously.

http://codereview.chromium.org/13251/diff/1/11
File net/proxy/proxy_script_fetcher.h (right):

http://codereview.chromium.org/13251/diff/1/11#newcode5
Line 5: // ProxyScriptFetcher is an asynch interface for fetching a proxy auto
config
On 2008/12/09 02:38:18, wtc wrote:
> Nit: we should standardize on using "async" as the
> abbreviation of asynchronous.

Done.

http://codereview.chromium.org/13251/diff/1/11#newcode6
Line 6: // script. It is specific to fetching a PAC script, in the constraints
it
On 2008/12/09 02:38:18, wtc wrote:
> Not sure what you meant by "in the constraints".

Removed that wording.

http://codereview.chromium.org/13251/diff/1/11#newcode20
Line 20: class ProxyScriptFetcher {
On 2008/12/09 02:38:18, wtc wrote:
> Nit: should we name this class PacScriptFetcher?

I will do the rename later in the review process, to avoid clobbering the
current diffs.

(I got the name ProxyScriptFetcher from Darin's v8-proxy patch; PacScriptFetcher
good too)

http://codereview.chromium.org/13251/diff/1/11#newcode23
Line 23: virtual ~ProxyScriptFetcher() { }
On 2008/12/09 02:38:18, wtc wrote:
> Nit: run cpplint.py.

What is the problem on this line? (I ran cpplint but it didn't turn anything up
on this)

http://codereview.chromium.org/13251/diff/1/11#newcode26
Line 26: virtual void Cancel() = 0;
On 2008/12/09 02:38:18, wtc wrote:
> Nit: list Cancel after Fetch.  It'll make the interface
> easier to understand.  For example, I would have known that
> only one fetch is allowed to be outstanding at a time.

Done.

http://codereview.chromium.org/13251/diff/1/11#newcode30
Line 30: // string of the response bytes. On failure, the response bytes is an
empty
On 2008/12/09 02:38:18, wtc wrote:
> Nit: in this line and the next line, change "response" to
> "result".

Done.

http://codereview.chromium.org/13251/diff/1/11#newcode34
Line 34: //    ERR_TIMED_OUT        -- the file took too long to complete.
On 2008/12/09 02:38:18, wtc wrote:
> Nit: "file" => "fetch" or "file fetch"?

Done.

http://codereview.chromium.org/13251/diff/1/11#newcode38
Line 38: //    ERR_NOT_IMPLEMENTED  -- if the response required authentication.
On 2008/12/09 02:38:18, wtc wrote:
> Should we add an ERR_AUTH_REQUIRED error code?  I guess in
> this case you want ERR_NOT_IMPLEMENTED because you will
> eventually implement auth support?

I am hoping this is just temporary. Will leave as NOT_IMPLEMENTED for now.

http://codereview.chromium.org/13251/diff/1/11#newcode44
Line 44: virtual void Fetch(const GURL& url, std::string* bytes,
On 2008/12/09 16:19:47, darin wrote:
> maybe Fetch should also return an |int| error code?

Leaving as void for now to match URLRequest::Start()

I suspect I will change this API some when integrating into ProxyService.
Instead of a CompletionCallback, it might be cleaner to have a delegate. Then
ProxyService can implement PacScriptFetcher::Delegate and simply get a callback
with the (code, bytes).

http://codereview.chromium.org/13251/diff/1/11#newcode59
Line 59: static size_t OverrideTimeoutConstraintForUnittest(size_t new_timeout);
On 2008/12/09 02:38:18, wtc wrote:
> Nit: use base::TimeDelta instead of size_t?

Done, good improvement.
Also fixed the units issue.

> 
> Use "Set" instead of "Override" in these two method names?

Done.

http://codereview.chromium.org/13251/diff/1/11#newcode61
Line 61: // Sets the maximum-reponse size for a fetch to |new_size| (bytes).
Returns
On 2008/12/09 02:38:18, wtc wrote:
> "reponse" => "response"
> 

Done.

http://codereview.chromium.org/13251/diff/1/9
File net/proxy/proxy_script_fetcher_unittest.cc (right):

http://codereview.chromium.org/13251/diff/1/9#newcode36
Line 36: delete proxy_service_;
On 2008/12/09 02:38:18, wtc wrote:
> This is the nth time I've seen this code sequence.  I think
> we should define these two members as scoped_ptr in the
> appropriate order.

Agreed. I will see about refactoring that in a separate change, as it affects
other stuff too.

[wtc, 2008-12-09 22:02:52]

http://codereview.chromium.org/13251/diff/206/214
File net/proxy/proxy_script_fetcher_unittest.cc (right):

http://codereview.chromium.org/13251/diff/206/214#newcode11
Line 11: #include "base/path_service.h"
Nit: list "base/path_service.h" before the "net/*.h" headers.

http://codereview.chromium.org/13251/diff/206/214#newcode56
Line 56: // signalled, and the values read will have been written to |result_|.
Nit: "values" => "bytes"?

"|result_|" => "|fetch_result_|"

http://codereview.chromium.org/13251/diff/206/214#newcode96
Line 96: // Teardown the state in |io_thread_|.
Nit: "Teardown" => "Tear down".

http://codereview.chromium.org/13251/diff/206/214#newcode112
Line 112: // ProxyScriptFetcher.
Nit ^2: you may want to indicate that the URLRequestContext
and the ProxyScriptFetcher are part of thread_helper_.  It
took me a few seconds to figure that out.

http://codereview.chromium.org/13251/diff/206/214#newcode150
Line 150: // Get a file:// url relative to
net/data/proxy/proxy_script_fetcher_unittest
Nit: add period at end of sentence.

http://codereview.chromium.org/13251/diff/206/214#newcode160
Line 160: 
It would be nice to explain what you intend to accomplish by
repeating every test twice.  It's not obvious to me at all.

http://codereview.chromium.org/13251/diff/206/214#newcode164
Line 164: // Fetch a non-existant file (repeat twice).
Nit: "non-existant" => "non-existent".

http://codereview.chromium.org/13251/diff/206/214#newcode250
Line 250: // would ordinarily make it download -- should have no effect.
"make it download" => "make it a download"?

http://codereview.chromium.org/13251/diff/206/214#newcode268
Line 268: server.TestServerPage("files/large-pac.nsproxy"),
Nit: indent by 2?

http://codereview.chromium.org/13251/diff/206/214#newcode303
Line 303: // Try fetching a URL which takes 1.2 seconds. We should abort the
request
Important: please make sure this test is not flaky, that
it'll still pass on slow or heavily loaded machines.

[eroman, 2008-12-09 22:32:48]

http://codereview.chromium.org/13251/diff/1/7
File net/base/net_error_list.h (right):

http://codereview.chromium.org/13251/diff/1/7#newcode94
Line 94: NET_ERROR(PAC_BAD_MIME_TYPE, -115)
On 2008/12/09 16:19:47, darin wrote:
> is Firefox also picky about the server reported mime type?

I just tested this in other browsers:

--------------
Data
--------------
* None of {Firefox3, IE7, Safari 3.1, Opera 9.5}  enforce a proxy mime-type
* Only Firefox3 enforces 200 status code. The rest will happily execute scripts
that 500'ed.

I guess the documentation I found referencing netscape navigator being picky
with mimetypes was incorrect.

------------------
Test
------------------
* tried the following mime types: 
   text/html
   text/plain
   image/png
   application/javascript

* tried the following status codes
  500
  404
  200

------------------
Conclusion
-------------------

In general executing incorrectly typed data concerns me.
But it looks like we we shouldn't respect the mime-type to be compatible.

I think we could get away with respecting the status code though, since Firefox
looks to be doing it.

[wtc, 2008-12-10 01:46:40]

http://codereview.chromium.org/13251/diff/1/7
File net/base/net_error_list.h (right):

http://codereview.chromium.org/13251/diff/1/7#newcode97
Line 97: NET_ERROR(PAC_BAD_STATUS_CODE, -116)
On 2008/12/09 21:47:02, eroman wrote:
>
> Changed to a -3xx, although I am still hazy on what the groupings mean. (-1xx
> however looks to be connection errors which is clearly wrong).

We should discuss with Darin whether we still need to group
the error codes, and document the existing grouping.

http://codereview.chromium.org/13251/diff/1/10
File net/proxy/proxy_script_fetcher.cc (right):

http://codereview.chromium.org/13251/diff/1/10#newcode128
Line 128: next_id_(1),
On 2008/12/09 21:47:02, eroman wrote:
>
> If I go with pre-increment, I would also want to change the name (since it is
no
> longer the "next id", but before-next-id). Perhaps "id_ = ++request_count_;"
in
> such case?
> 
> For now I have left it as is; let me know how to proceed.

It's fine to leave it as is.

I think it's fine to pre-increment a variable named
next_id_.  To me, it's just like naming a parameter
int* result instead of int* result_ptr.  It's a little
ambiguous but an experienced programmer usually prefers
it over a more accurate but also more verbose name.

http://codereview.chromium.org/13251/diff/1/11
File net/proxy/proxy_script_fetcher.h (right):

http://codereview.chromium.org/13251/diff/1/11#newcode23
Line 23: virtual ~ProxyScriptFetcher() { }
On 2008/12/09 21:47:02, eroman wrote:
>
> What is the problem on this line? (I ran cpplint but it didn't turn anything
up
> on this)

Sorry, I thought cpplint.py complains about this.
This is the "No spaces inside empty braces" rule in
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Horizontal_Whi...

[wtc, 2008-12-10 01:58:09]

LGTM.

I feel rather strongly about changing base::TimeDelta back
to int or size_t.  See below.  You can do this in a
follow-up CL.

http://codereview.chromium.org/13251/diff/401/610
File net/proxy/proxy_script_fetcher.h (right):

http://codereview.chromium.org/13251/diff/401/610#newcode60
Line 60: const base::TimeDelta& timeout);
Now that I know that this 'timeout' eventually becomes the
"int delay_ms" argument to MessageLoop::PostDelayedTask,
I withdraw my suggestion of using the base::TimeDelta type.

I now think it should have the 'int' type (or size_t, if
you insist in spite of the static_cast<int>), and the unit
should be ms or sec.

http://codereview.chromium.org/13251/diff/401/610#newcode62
Line 62: // Sets the maximum-response size for a fetch to |size_bytes|. Returns
the
Nit: remove the "-" in "maximum-response".

[eroman, 2008-12-10 09:18:05]

Any further comments I will address in a subsequent CL. Committed.

http://codereview.chromium.org/13251/diff/1/10
File net/proxy/proxy_script_fetcher.cc (right):

http://codereview.chromium.org/13251/diff/1/10#newcode128
Line 128: next_id_(1),
On 2008/12/10 01:46:40, wtc wrote:
> On 2008/12/09 21:47:02, eroman wrote:
> >
> > If I go with pre-increment, I would also want to change the name (since it
is
> no
> > longer the "next id", but before-next-id). Perhaps "id_ = ++request_count_;"
> in
> > such case?
> > 
> > For now I have left it as is; let me know how to proceed.
> 
> It's fine to leave it as is.
> 
> I think it's fine to pre-increment a variable named
> next_id_.  To me, it's just like naming a parameter
> int* result instead of int* result_ptr.  It's a little
> ambiguous but an experienced programmer usually prefers
> it over a more accurate but also more verbose name.
> 
> 

Done -- pre-increment on next_id_.

http://codereview.chromium.org/13251/diff/1/11
File net/proxy/proxy_script_fetcher.h (right):

http://codereview.chromium.org/13251/diff/1/11#newcode23
Line 23: virtual ~ProxyScriptFetcher() { }
On 2008/12/10 01:46:40, wtc wrote:
> On 2008/12/09 21:47:02, eroman wrote:
> >
> > What is the problem on this line? (I ran cpplint but it didn't turn anything
> up
> > on this)
> 
> Sorry, I thought cpplint.py complains about this.
> This is the "No spaces inside empty braces" rule in
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Horizontal_Whi...
> 
> 

Done.

http://codereview.chromium.org/13251/diff/206/214
File net/proxy/proxy_script_fetcher_unittest.cc (right):

http://codereview.chromium.org/13251/diff/206/214#newcode11
Line 11: #include "base/path_service.h"
On 2008/12/09 22:02:52, wtc wrote:
> Nit: list "base/path_service.h" before the "net/*.h" headers.

Done.

http://codereview.chromium.org/13251/diff/206/214#newcode56
Line 56: // signalled, and the values read will have been written to |result_|.
On 2008/12/09 22:02:52, wtc wrote:
> Nit: "values" => "bytes"?
> 
> "|result_|" => "|fetch_result_|"

Done.

http://codereview.chromium.org/13251/diff/206/214#newcode96
Line 96: // Teardown the state in |io_thread_|.
On 2008/12/09 22:02:52, wtc wrote:
> Nit: "Teardown" => "Tear down".

Done.

http://codereview.chromium.org/13251/diff/206/214#newcode112
Line 112: // ProxyScriptFetcher.
On 2008/12/09 22:02:52, wtc wrote:
> Nit ^2: you may want to indicate that the URLRequestContext
> and the ProxyScriptFetcher are part of thread_helper_.  It
> took me a few seconds to figure that out.

Done.

http://codereview.chromium.org/13251/diff/206/214#newcode150
Line 150: // Get a file:// url relative to
net/data/proxy/proxy_script_fetcher_unittest
On 2008/12/09 22:02:52, wtc wrote:
> Nit: add period at end of sentence.

Done.

http://codereview.chromium.org/13251/diff/206/214#newcode160
Line 160: 
On 2008/12/09 22:02:52, wtc wrote:
> It would be nice to explain what you intend to accomplish by
> repeating every test twice.  It's not obvious to me at all.

I removed the repeat twice; it was prompted by paranoia.

http://codereview.chromium.org/13251/diff/206/214#newcode164
Line 164: // Fetch a non-existant file (repeat twice).
On 2008/12/09 22:02:52, wtc wrote:
> Nit: "non-existant" => "non-existent".

Done.

http://codereview.chromium.org/13251/diff/206/214#newcode250
Line 250: // would ordinarily make it download -- should have no effect.
On 2008/12/09 22:02:52, wtc wrote:
> "make it download" => "make it a download"?

Reworded differently.

http://codereview.chromium.org/13251/diff/206/214#newcode268
Line 268: server.TestServerPage("files/large-pac.nsproxy"),
On 2008/12/09 22:02:52, wtc wrote:
> Nit: indent by 2?

Done.

http://codereview.chromium.org/13251/diff/206/214#newcode303
Line 303: // Try fetching a URL which takes 1.2 seconds. We should abort the
request
On 2008/12/09 22:02:52, wtc wrote:
> Important: please make sure this test is not flaky, that
> it'll still pass on slow or heavily loaded machines.

Shouldn't be flaky. I have tested in purify which is much slower, seems ok. To
be safe I have put a bigger gap in the numbers. (500ms timeout, 1.2 second
latency)

http://codereview.chromium.org/13251/diff/401/610
File net/proxy/proxy_script_fetcher.h (right):

http://codereview.chromium.org/13251/diff/401/610#newcode60
Line 60: const base::TimeDelta& timeout);
On 2008/12/10 01:58:10, wtc wrote:
> Now that I know that this 'timeout' eventually becomes the
> "int delay_ms" argument to MessageLoop::PostDelayedTask,
> I withdraw my suggestion of using the base::TimeDelta type.
> 
> I now think it should have the 'int' type (or size_t, if
> you insist in spite of the static_cast<int>), and the unit
> should be ms or sec. 

Done.

Changed both constraints to be "int". (The reason I had the bytes limit as an
int was to avoid needing to cast away a unsigned to signed comparison).

http://codereview.chromium.org/13251/diff/401/610#newcode62
Line 62: // Sets the maximum-response size for a fetch to |size_bytes|. Returns
the
On 2008/12/10 01:58:10, wtc wrote:
> Nit: remove the "-" in "maximum-response".

Done.