Allow URLFetcher to save results in a file. Have CRX updates use this capability.

Allow URLFetcher to save results in a file. Have CRX updates use this capability.

Darin, this is based on your recommendation from http://codereview.chromium.org/6904057/ .  


BUG=82568
TEST=URLFetcher*Test.*:ExtensionUpdaterTest.*

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

[Sam Kerner (Chrome), 2011-05-13 18:22:06]

Darin,
    I am still working on tests for this change, but I would appreciate a quick
sanity-check that what I am implementing is what you had in mind.

Sam

[darin (slow to review), 2011-05-16 04:57:04]

sorry, most of my comments are about how sad the URLFetcher interface is.

http://codereview.chromium.org/6969067/diff/1005/chrome/common/net/url_fetcher.h
File chrome/common/net/url_fetcher.h (right):

http://codereview.chromium.org/6969067/diff/1005/chrome/common/net/url_fetche...
chrome/common/net/url_fetcher.h:99: // instance is destroyed.
I don't understand why you need these additional methods.

OnURLFetchComplete has a way to report errors via the URLRequestStatus
parameter.  It seems like you want to be able to report errors that are not
covered by the network stack.

Instead of creating a new method, another approach would be to construct your
own URLRequestStatus when there is a failure writing to the file.  However, if
it is helpful to discern errors originating from the network stack from those
originating from URLFetcher itself, then maybe we'd want some way to tell them
apart.

Back to my comment about this interface needing TLC:  it seems like URLFetcher
has a number of methods that can be queried once OnURLFetchComplete is called,
and it also passes a number of response properties as parameters.  In the
original design, the only results from URLFetcher came in the form of parameters
to OnURLFetchComplete.  But, now we have a mix, and that makes things more
confusing than they should need to be.  It seems like a good idea to make
OnURLFetchComplete only take a single parameter, the URLFetcher.  Then, all of
the other parameters can just be properties of the URLFetcher object.  I suspect
this would make things much cleaner.

http://codereview.chromium.org/6969067/diff/1005/chrome/common/net/url_fetche...
chrome/common/net/url_fetcher.h:226: void
set_response_destination(ResponseDestinationType response_destination) {
It seems a bit verbose to use an enum here.  Maybe you could just have a method
named SaveResponseToTemporaryFile?

<rant>Man, people have really been abusing this class, dumping all sorts of
methods in here in such a disorganized fashion.  It is hard to tell what methods
should be called before or after Start.  This file needs some major TLC. 
Non-inline methods using unix_hacker() naming convention!  /ugh/</rant>

[Sam Kerner (Chrome), 2011-05-17 04:09:34]

Comments addressed.  Ready for review.

http://codereview.chromium.org/6969067/diff/1005/chrome/common/net/url_fetcher.h
File chrome/common/net/url_fetcher.h (right):

http://codereview.chromium.org/6969067/diff/1005/chrome/common/net/url_fetche...
chrome/common/net/url_fetcher.h:99: // instance is destroyed.
On 2011/05/16 04:57:04, darin wrote:
> I don't understand why you need these additional methods.

One of them (the new OnURLFetchComplete() ) will replace the existing
OnURLFetchComplete() method.  Updating every user will make the change harder to
review, so I plan to do the update in a follow-on CL.

> OnURLFetchComplete has a way to report errors via the URLRequestStatus
> parameter.  It seems like you want to be able to report errors that are not
> covered by the network stack.
> 
> Instead of creating a new method, another approach would be to construct your
> own URLRequestStatus when there is a failure writing to the file.  However, if
> it is helpful to discern errors originating from the network stack from those
> originating from URLFetcher itself, then maybe we'd want some way to tell them
> apart.

That was the reason I created a new callback OnFileWriteError():  File system
errors don't fit in a class representing network request status.  What http
response code should be passed to OnURLFetchComplete() if the response was not
started because the temp file could not be created?


> Back to my comment about this interface needing TLC:  it seems like URLFetcher
> has a number of methods that can be queried once OnURLFetchComplete is called,
> and it also passes a number of response properties as parameters.  In the
> original design, the only results from URLFetcher came in the form of
parameters
> to OnURLFetchComplete.  But, now we have a mix, and that makes things more
> confusing than they should need to be.  It seems like a good idea to make
> OnURLFetchComplete only take a single parameter, the URLFetcher.  Then, all of
> the other parameters can just be properties of the URLFetcher object.  I
suspect
> this would make things much cleaner.

I made the new version of OnURLFetchComplete() work this way.  Also added a
function that gives the file system error, if there was one.

http://codereview.chromium.org/6969067/diff/1005/chrome/common/net/url_fetche...
chrome/common/net/url_fetcher.h:226: void
set_response_destination(ResponseDestinationType response_destination) {
On 2011/05/16 04:57:04, darin wrote:
> It seems a bit verbose to use an enum here.  Maybe you could just have a
method
> named SaveResponseToTemporaryFile?

Done.  Made the enum private.

> <rant>Man, people have really been abusing this class, dumping all sorts of
> methods in here in such a disorganized fashion.  It is hard to tell what
methods
> should be called before or after Start.  This file needs some major TLC. 
> Non-inline methods using unix_hacker() naming convention!  /ugh/</rant>

[darin (slow to review), 2011-05-17 16:23:31]

On Mon, May 16, 2011 at 9:09 PM, <skerner@chromium.org> wrote:

> Comments addressed.  Ready for review.
>
>
>
>
>
http://codereview.chromium.org/6969067/diff/1005/chrome/common/net/url_fetcher.h
> File chrome/common/net/url_fetcher.h (right):
>
>
>
http://codereview.chromium.org/6969067/diff/1005/chrome/common/net/url_fetche...
> chrome/common/net/url_fetcher.h:99: // instance is destroyed.
> On 2011/05/16 04:57:04, darin wrote:
>
>> I don't understand why you need these additional methods.
>>
>
> One of them (the new OnURLFetchComplete() ) will replace the existing
> OnURLFetchComplete() method.  Updating every user will make the change
> harder to review, so I plan to do the update in a follow-on CL.
>
>
>  OnURLFetchComplete has a way to report errors via the URLRequestStatus
>> parameter.  It seems like you want to be able to report errors that
>>
> are not
>
>> covered by the network stack.
>>
>
>  Instead of creating a new method, another approach would be to
>>
> construct your
>
>> own URLRequestStatus when there is a failure writing to the file.
>>
> However, if
>
>> it is helpful to discern errors originating from the network stack
>>
> from those
>
>> originating from URLFetcher itself, then maybe we'd want some way to
>>
> tell them
>
>> apart.
>>
>
> That was the reason I created a new callback OnFileWriteError():  File
> system errors don't fit in a class representing network request status.
> What http response code should be passed to OnURLFetchComplete() if the
> response was not started because the temp file could not be created?
>
>
URLRequestStatus does not convey http response codes.  It conveys error
codes defined in net/base/net_error_list.h.

Of course net_error_list.h does not cover all of PlatformFileError, and it
probably shouldn't be changed to do so.



>
>
>  Back to my comment about this interface needing TLC:  it seems like
>>
> URLFetcher
>
>> has a number of methods that can be queried once OnURLFetchComplete is
>>
> called,
>
>> and it also passes a number of response properties as parameters.  In
>>
> the
>
>> original design, the only results from URLFetcher came in the form of
>>
> parameters
>
>> to OnURLFetchComplete.  But, now we have a mix, and that makes things
>>
> more
>
>> confusing than they should need to be.  It seems like a good idea to
>>
> make
>
>> OnURLFetchComplete only take a single parameter, the URLFetcher.
>>
> Then, all of
>
>> the other parameters can just be properties of the URLFetcher object.
>>
> I suspect
>
>> this would make things much cleaner.
>>
>
> I made the new version of OnURLFetchComplete() work this way.  Also
> added a function that gives the file system error, if there was one.
>
>
>
>
http://codereview.chromium.org/6969067/diff/1005/chrome/common/net/url_fetche...
> chrome/common/net/url_fetcher.h:226: void
> set_response_destination(ResponseDestinationType response_destination) {
> On 2011/05/16 04:57:04, darin wrote:
>
>> It seems a bit verbose to use an enum here.  Maybe you could just have
>>
> a method
>
>> named SaveResponseToTemporaryFile?
>>
>
> Done.  Made the enum private.
>
>
>  <rant>Man, people have really been abusing this class, dumping all
>>
> sorts of
>
>> methods in here in such a disorganized fashion.  It is hard to tell
>>
> what methods
>
>> should be called before or after Start.  This file needs some major
>>
> TLC.
>
>> Non-inline methods using unix_hacker() naming convention!
>>
> /ugh/</rant>
>
> http://codereview.chromium.org/6969067/
>

[Sam Kerner (Chrome), 2011-05-18 05:09:47]

+Antony

Added a first user of the fetch-to-file behavior: The extensions updater.  Added
Antony to review the updater changes.

[asargent_no_longer_on_chrome, 2011-05-18 19:00:49]

http://codereview.chromium.org/6969067/diff/14001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_updater.cc (left):

http://codereview.chromium.org/6969067/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_updater.cc:890: NotifyIfFinished();
whoa, are you sure this isn't needed anymore?

http://codereview.chromium.org/6969067/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_updater.cc:896: NotifyIfFinished();
same question here

http://codereview.chromium.org/6969067/diff/14001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_updater.cc (right):

http://codereview.chromium.org/6969067/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_updater.cc:750:
OnCRXFileWriteError(current_extension_fetch_.id);
It looks like we've lost the histogramming of file write success/failure from 
ExtensionUpdaterFileHandler::WriteTempFile. Should you add something back here?
Or would it make sense to add an option to URLFetcher to have it do histograms
of such failures (perhaps with a tag the consumer passes in for the histogram
name)? This could be a future enhancement.

http://codereview.chromium.org/6969067/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_updater.cc:753: (response_code == 200 ||
url.SchemeIsFile())) {
Slightly off-topic: I vaguely recall a bug where we didn't handle redirects
properly, and I wonder if this is because this just handles status 200. Do you
know off the top of your head if URLFetcher automatically follows redirects and
only calls your Delegate with the final result of any redirect chain?

http://codereview.chromium.org/6969067/diff/14001/chrome/common/net/url_fetch...
File chrome/common/net/url_fetcher.h (right):

http://codereview.chromium.org/6969067/diff/14001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.h:204: void SaveResponseToTemporaryFile();
Here's a thought for future cleanup along the lines of Darin's concern about it
being confusing what methods must be called before Start. Would it make sense to
move to a pattern where there is a URLFetcherOptions struct that you pass to the
Start method? Then instead of:

my_fetcher_ = new URLFetcher();
my_fetcher_->SaveResponseToTemporaryFile();
my_fetcher_->Start();

it would be:

URLFetcherOptions options;
options.save_response_to_tempfile = true;
my_fetcher_ = new URLFetcher();
my_fetcher_->Start(&options);

http://codereview.chromium.org/6969067/diff/14001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.h:206: // Set the message loop proxy for file
operations. Used to write the
Are there other file operations that this would apply to apart from
SaveResponseToTemporaryFile? If not, why not just have the MessageLoopProxy be a
required parameter to SaveResponseToTemporaryFile()?

[Sam Kerner (Chrome), 2011-05-18 23:45:56]

Comments addressed.  Ready for another look.

http://codereview.chromium.org/6969067/diff/14001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_updater.cc (left):

http://codereview.chromium.org/6969067/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_updater.cc:890: NotifyIfFinished();
On 2011/05/18 19:00:49, Antony Sargent wrote:
> whoa, are you sure this isn't needed anymore?

OnCRXFileWritten() and OnCRXFileWriteError() are now called from
OnURLFetchComplete().  The call to NotifyIfFinished() at the end of 
ExtensionUpdater::OnURLFetchComplete() will be called after they run.  I
initially left them in, and saw test failures because the notification fired
twice.

http://codereview.chromium.org/6969067/diff/14001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_updater.cc (right):

http://codereview.chromium.org/6969067/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_updater.cc:750:
OnCRXFileWriteError(current_extension_fetch_.id);
On 2011/05/18 19:00:49, Antony Sargent wrote:
> It looks like we've lost the histogramming of file write success/failure from 
> ExtensionUpdaterFileHandler::WriteTempFile. Should you add something back
here?
> Or would it make sense to add an option to URLFetcher to have it do histograms
> of such failures (perhaps with a tag the consumer passes in for the histogram
> name)? This could be a future enhancement. 

Added a histogram to this file.  A histogram option is hard because histograms
with non-constant names are not allowed.

http://codereview.chromium.org/6969067/diff/14001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_updater.cc:753: (response_code == 200 ||
url.SchemeIsFile())) {
On 2011/05/18 19:00:49, Antony Sargent wrote:
> Slightly off-topic: I vaguely recall a bug where we didn't handle redirects
> properly, and I wonder if this is because this just handles status 200. Do you
> know off the top of your head if URLFetcher automatically follows redirects
and
> only calls your Delegate with the final result of any redirect chain?

I am not sure about this.  URLRequest::Delegate may implement
OnReceivedRedirect(), but URLFetcher does not implement it.

There are comments in this file about dealing with 5xx responses.  URLFetcher
can do this.  The default number of retries is 0, so we are not doing it today.

http://codereview.chromium.org/6969067/diff/14001/chrome/common/net/url_fetch...
File chrome/common/net/url_fetcher.h (right):

http://codereview.chromium.org/6969067/diff/14001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.h:204: void SaveResponseToTemporaryFile();
On 2011/05/18 19:00:49, Antony Sargent wrote:
> Here's a thought for future cleanup along the lines of Darin's concern about
it
> being confusing what methods must be called before Start. Would it make sense
to
> move to a pattern where there is a URLFetcherOptions struct that you pass to
the
> Start method? Then instead of:
> 
> my_fetcher_ = new URLFetcher();
> my_fetcher_->SaveResponseToTemporaryFile();
> my_fetcher_->Start();
> 
> it would be:
> 
> URLFetcherOptions options;
> options.save_response_to_tempfile = true;
> my_fetcher_ = new URLFetcher();
> my_fetcher_->Start(&options);
> 

A good idea, but more than I can chew in this CL.

http://codereview.chromium.org/6969067/diff/14001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.h:206: // Set the message loop proxy for file
operations. Used to write the
On 2011/05/18 19:00:49, Antony Sargent wrote:
> Are there other file operations that this would apply to apart from
> SaveResponseToTemporaryFile? If not, why not just have the MessageLoopProxy be
a
> required parameter to SaveResponseToTemporaryFile()?

Done.

[asargent_no_longer_on_chrome, 2011-05-19 00:01:11]

Did you forget to upload the new version?

On Wed, May 18, 2011 at 4:45 PM, <skerner@chromium.org> wrote:

> Comments addressed.  Ready for another look.
>
>
>
>
>
http://codereview.chromium.org/6969067/diff/14001/chrome/browser/extensions/e...
> File chrome/browser/extensions/extension_updater.cc (left):
>
>
>
http://codereview.chromium.org/6969067/diff/14001/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_updater.cc:890: NotifyIfFinished();
> On 2011/05/18 19:00:49, Antony Sargent wrote:
>
>> whoa, are you sure this isn't needed anymore?
>>
>
> OnCRXFileWritten() and OnCRXFileWriteError() are now called from
> OnURLFetchComplete().  The call to NotifyIfFinished() at the end of
> ExtensionUpdater::OnURLFetchComplete() will be called after they run.  I
> initially left them in, and saw test failures because the notification
> fired twice.
>
>
>
>
http://codereview.chromium.org/6969067/diff/14001/chrome/browser/extensions/e...
> File chrome/browser/extensions/extension_updater.cc (right):
>
>
>
http://codereview.chromium.org/6969067/diff/14001/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_updater.cc:750:
> OnCRXFileWriteError(current_extension_fetch_.id);
> On 2011/05/18 19:00:49, Antony Sargent wrote:
>
>> It looks like we've lost the histogramming of file write
>>
> success/failure from
>
>> ExtensionUpdaterFileHandler::WriteTempFile. Should you add something
>>
> back here?
>
>> Or would it make sense to add an option to URLFetcher to have it do
>>
> histograms
>
>> of such failures (perhaps with a tag the consumer passes in for the
>>
> histogram
>
>> name)? This could be a future enhancement.
>>
>
> Added a histogram to this file.  A histogram option is hard because
> histograms with non-constant names are not allowed.
>
>
>
>
http://codereview.chromium.org/6969067/diff/14001/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_updater.cc:753: (response_code ==
> 200 || url.SchemeIsFile())) {
> On 2011/05/18 19:00:49, Antony Sargent wrote:
>
>> Slightly off-topic: I vaguely recall a bug where we didn't handle
>>
> redirects
>
>> properly, and I wonder if this is because this just handles status
>>
> 200. Do you
>
>> know off the top of your head if URLFetcher automatically follows
>>
> redirects and
>
>> only calls your Delegate with the final result of any redirect chain?
>>
>
> I am not sure about this.  URLRequest::Delegate may implement
> OnReceivedRedirect(), but URLFetcher does not implement it.
>
> There are comments in this file about dealing with 5xx responses.
> URLFetcher can do this.  The default number of retries is 0, so we are
> not doing it today.
>
>
>
>
http://codereview.chromium.org/6969067/diff/14001/chrome/common/net/url_fetch...
> File chrome/common/net/url_fetcher.h (right):
>
>
>
http://codereview.chromium.org/6969067/diff/14001/chrome/common/net/url_fetch...
> chrome/common/net/url_fetcher.h:204: void SaveResponseToTemporaryFile();
> On 2011/05/18 19:00:49, Antony Sargent wrote:
>
>> Here's a thought for future cleanup along the lines of Darin's concern
>>
> about it
>
>> being confusing what methods must be called before Start. Would it
>>
> make sense to
>
>> move to a pattern where there is a URLFetcherOptions struct that you
>>
> pass to the
>
>> Start method? Then instead of:
>>
>
>  my_fetcher_ = new URLFetcher();
>> my_fetcher_->SaveResponseToTemporaryFile();
>> my_fetcher_->Start();
>>
>
>  it would be:
>>
>
>  URLFetcherOptions options;
>> options.save_response_to_tempfile = true;
>> my_fetcher_ = new URLFetcher();
>> my_fetcher_->Start(&options);
>>
>
>
> A good idea, but more than I can chew in this CL.
>
>
>
>
http://codereview.chromium.org/6969067/diff/14001/chrome/common/net/url_fetch...
> chrome/common/net/url_fetcher.h:206: // Set the message loop proxy for
> file operations. Used to write the
> On 2011/05/18 19:00:49, Antony Sargent wrote:
>
>> Are there other file operations that this would apply to apart from
>> SaveResponseToTemporaryFile? If not, why not just have the
>>
> MessageLoopProxy be a
>
>> required parameter to SaveResponseToTemporaryFile()?
>>
>
> Done.
>
>
> http://codereview.chromium.org/6969067/
>

[skerner, 2011-05-19 00:05:11]

On Wed, May 18, 2011 at 8:00 PM, Antony Sargent <asargent@chromium.org> wrote:
> Did you forget to upload the new version?

Fixed.

> On Wed, May 18, 2011 at 4:45 PM, <skerner@chromium.org> wrote:
>>
>> Comments addressed.  Ready for another look.
>>
>>
>>
>>
http://codereview.chromium.org/6969067/diff/14001/chrome/browser/extensions/e...
>> File chrome/browser/extensions/extension_updater.cc (left):
>>
>>
>>
http://codereview.chromium.org/6969067/diff/14001/chrome/browser/extensions/e...
>> chrome/browser/extensions/extension_updater.cc:890: NotifyIfFinished();
>> On 2011/05/18 19:00:49, Antony Sargent wrote:
>>>
>>> whoa, are you sure this isn't needed anymore?
>>
>> OnCRXFileWritten() and OnCRXFileWriteError() are now called from
>> OnURLFetchComplete().  The call to NotifyIfFinished() at the end of
>> ExtensionUpdater::OnURLFetchComplete() will be called after they run.  I
>> initially left them in, and saw test failures because the notification
>> fired twice.
>>
>>
>>
http://codereview.chromium.org/6969067/diff/14001/chrome/browser/extensions/e...
>> File chrome/browser/extensions/extension_updater.cc (right):
>>
>>
>>
http://codereview.chromium.org/6969067/diff/14001/chrome/browser/extensions/e...
>> chrome/browser/extensions/extension_updater.cc:750:
>> OnCRXFileWriteError(current_extension_fetch_.id);
>> On 2011/05/18 19:00:49, Antony Sargent wrote:
>>>
>>> It looks like we've lost the histogramming of file write
>>
>> success/failure from
>>>
>>> ExtensionUpdaterFileHandler::WriteTempFile. Should you add something
>>
>> back here?
>>>
>>> Or would it make sense to add an option to URLFetcher to have it do
>>
>> histograms
>>>
>>> of such failures (perhaps with a tag the consumer passes in for the
>>
>> histogram
>>>
>>> name)? This could be a future enhancement.
>>
>> Added a histogram to this file.  A histogram option is hard because
>> histograms with non-constant names are not allowed.
>>
>>
>>
http://codereview.chromium.org/6969067/diff/14001/chrome/browser/extensions/e...
>> chrome/browser/extensions/extension_updater.cc:753: (response_code ==
>> 200 || url.SchemeIsFile())) {
>> On 2011/05/18 19:00:49, Antony Sargent wrote:
>>>
>>> Slightly off-topic: I vaguely recall a bug where we didn't handle
>>
>> redirects
>>>
>>> properly, and I wonder if this is because this just handles status
>>
>> 200. Do you
>>>
>>> know off the top of your head if URLFetcher automatically follows
>>
>> redirects and
>>>
>>> only calls your Delegate with the final result of any redirect chain?
>>
>> I am not sure about this.  URLRequest::Delegate may implement
>> OnReceivedRedirect(), but URLFetcher does not implement it.
>>
>> There are comments in this file about dealing with 5xx responses.
>> URLFetcher can do this.  The default number of retries is 0, so we are
>> not doing it today.
>>
>>
>>
http://codereview.chromium.org/6969067/diff/14001/chrome/common/net/url_fetch...
>> File chrome/common/net/url_fetcher.h (right):
>>
>>
>>
http://codereview.chromium.org/6969067/diff/14001/chrome/common/net/url_fetch...
>> chrome/common/net/url_fetcher.h:204: void SaveResponseToTemporaryFile();
>> On 2011/05/18 19:00:49, Antony Sargent wrote:
>>>
>>> Here's a thought for future cleanup along the lines of Darin's concern
>>
>> about it
>>>
>>> being confusing what methods must be called before Start. Would it
>>
>> make sense to
>>>
>>> move to a pattern where there is a URLFetcherOptions struct that you
>>
>> pass to the
>>>
>>> Start method? Then instead of:
>>
>>> my_fetcher_ = new URLFetcher();
>>> my_fetcher_->SaveResponseToTemporaryFile();
>>> my_fetcher_->Start();
>>
>>> it would be:
>>
>>> URLFetcherOptions options;
>>> options.save_response_to_tempfile = true;
>>> my_fetcher_ = new URLFetcher();
>>> my_fetcher_->Start(&options);
>>
>>
>> A good idea, but more than I can chew in this CL.
>>
>>
>>
http://codereview.chromium.org/6969067/diff/14001/chrome/common/net/url_fetch...
>> chrome/common/net/url_fetcher.h:206: // Set the message loop proxy for
>> file operations. Used to write the
>> On 2011/05/18 19:00:49, Antony Sargent wrote:
>>>
>>> Are there other file operations that this would apply to apart from
>>> SaveResponseToTemporaryFile? If not, why not just have the
>>
>> MessageLoopProxy be a
>>>
>>> required parameter to SaveResponseToTemporaryFile()?
>>
>> Done.
>>
>> http://codereview.chromium.org/6969067/
>
>

[asargent_no_longer_on_chrome, 2011-05-19 00:30:00]

LGTM

[willchan no longer on Chromium, 2011-06-21 13:55:15]

In the future, can you add eroman and me to reviews for URLFetcher? Even though
this is not in net/ proper, it's one of the main interfaces to net/ and so we're
trying to keep it sane, even though it's a pretty bad mess.

http://codereview.chromium.org/6969067/diff/20001/chrome/common/net/url_fetch...
File chrome/common/net/url_fetcher.cc (right):

http://codereview.chromium.org/6969067/diff/20001/chrome/common/net/url_fetch...
chrome/common/net/url_fetcher.cc:394: core_->InformDelegateFetchIsComplete();
This code is wrong. The delegate can only be called on the delegate message
loop. URLRequest can only be used on the IO message loop. The delegate message
loop and the IO message loop are not always the same. Therefore, we have a race
condition here.

[skerner, 2011-06-21 16:52:34]

On Tue, Jun 21, 2011 at 9:55 AM,  <willchan@chromium.org> wrote:
> In the future, can you add eroman and me to reviews for URLFetcher? Even
> though
> this is not in net/ proper, it's one of the main interfaces to net/ and so
> we're
> trying to keep it sane, even though it's a pretty bad mess.

Okay, will do.

>
>
>
http://codereview.chromium.org/6969067/diff/20001/chrome/common/net/url_fetch...
> File chrome/common/net/url_fetcher.cc (right):
>
>
http://codereview.chromium.org/6969067/diff/20001/chrome/common/net/url_fetch...
> chrome/common/net/url_fetcher.cc:394:
> core_->InformDelegateFetchIsComplete();
> This code is wrong. The delegate can only be called on the delegate
> message loop. URLRequest can only be used on the IO message loop. The
> delegate message loop and the IO message loop are not always the same.
> Therefore, we have a race condition here.

Good catch.  Will send a CL to fix this shortly.

>
> http://codereview.chromium.org/6969067/
>