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

chrome/common/net/url_fetcher.h

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // This file contains URLFetcher, a wrapper around net::URLRequest that handles
 // low-level details like thread safety, ref counting, and incremental buffer
 // reading.  This is useful for callers who simply want to get the data from a
 // URL and don't care about all the nitty-gritty details.
 //
 // NOTE(willchan): Only one "IO" thread is supported for URLFetcher.  This is a
 // temporary situation.  We will work on allowing support for multiple "io"
 // threads per process.
 
 #ifndef CHROME_COMMON_NET_URL_FETCHER_H_
 #define CHROME_COMMON_NET_URL_FETCHER_H_
 #pragma once
 
 #include <string>
 #include <vector>
 
 #include "base/memory/ref_counted.h"
 #include "base/message_loop.h"
+#include "base/platform_file.h"
 #include "base/time.h"
 
+class FilePath;
 class GURL;
 
+namespace base {
+class MessageLoopProxy;
+}  // namespace base
+
 namespace net {
 class HostPortPair;
 class HttpResponseHeaders;
 class URLRequestContextGetter;
 class URLRequestStatus;
 typedef std::vector<std::string> ResponseCookies;
 }  // namespace net
 
 // To use this class, create an instance with the desired URL and a pointer to
 // the object to be notified when the URL has been loaded:
@@ skipping 24 matching lines @@
 // interception is explicitly enabled in tests.
 
 class URLFetcher {
  public:
   enum RequestType {
     GET,
     POST,
     HEAD,
   };
 
+  // Where should the response be saved? Use set_response_destination()
+  // to choose.
+  enum ResponseDestinationType {
+    STRING,  // Default
+    TEMP_FILE
+  };
+
   class Delegate {
    public:
-    // This will be called when the URL has been fetched, successfully or not.
-    // |response_code| is the HTTP response code (200, 404, etc.) if
-    // applicable.  |url|, |status| and |data| are all valid until the
-    // URLFetcher instance is destroyed.
+    // TODO(skerner): This will be removed in favor of the |data|-free
+    // version below. Leaving this for now to make the initial code review
+    // easy to read.
     virtual void OnURLFetchComplete(const URLFetcher* source,
                                     const GURL& url,
                                     const net::URLRequestStatus& status,
                                     int response_code,
                                     const net::ResponseCookies& cookies,
-                                    const std::string& data) = 0;
+                                    const std::string& data);
+
+    // This will be called when the URL has been fetched, successfully or not.
+    // |response_code| is the HTTP response code (200, 404, etc.) if
+    // applicable.  |url| and |status| are all valid until the URLFetcher
+    // instance is destroyed.

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

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.

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

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.
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?
I made the new version of OnURLFetchComplete() work this way.  Also added a
function that gives the file system error, if there was one.

+    virtual void OnURLFetchComplete(const URLFetcher* source,
+                                    const GURL& url,
+                                    const net::URLRequestStatus& status,
+                                    int response_code,
+                                    const net::ResponseCookies& cookies);
+
+    // Called on a file writing error. Only called when writing the response
+    // to a file.
+    // REVIEWER PLEASE NOTE: This method would be unnecessary if the failure
+    // could be encoded in a net::URLRequestStatus. However, it seems odd
+    // to put a file system error in that object. Is it worth having this
+    // method?
+    virtual void OnFileWriteError(const URLFetcher* source,
+                                  const GURL& url,
+                                  base::PlatformFileError error);
 
    protected:
     virtual ~Delegate() {}
   };
 
   // URLFetcher::Create uses the currently registered Factory to create the
   // URLFetcher. Factory is intended for testing.
   class Factory {
    public:
     virtual URLFetcher* CreateURLFetcher(int id,
@@ skipping 83 matching lines @@
   // when a 5xx response was received.
   base::TimeDelta backoff_delay() const { return backoff_delay_; }
 
   // Sets the back-off delay, allowing to mock 5xx requests in unit-tests.
 #if defined(UNIT_TEST)
   void set_backoff_delay(base::TimeDelta backoff_delay) {
     backoff_delay_ = backoff_delay;
   }
 #endif  // defined(UNIT_TEST)
 
+  ResponseDestinationType response_destination() {
+    return response_destination_;
+  }
+
+  // Where should the response be saved? This should only be called before
+  // calling Start(). If the response should be saved to a file, you need
+  // to call set_file_message_loop_proxy() to give the thread on which file
+  // operations may be done.
+  void set_response_destination(ResponseDestinationType response_destination) {

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

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]

Done.  Made the enum private.

+    response_destination_ = response_destination;
+  }
+
+  // Set the message loop proxy for file operations. Used to write the
+  // response to a file.
+  void set_file_message_loop_proxy(
+      scoped_refptr<base::MessageLoopProxy> file_message_loop_proxy);
+
   // Retrieve the response headers from the request.  Must only be called after
   // the OnURLFetchComplete callback has run.
   virtual net::HttpResponseHeaders* response_headers() const;
 
   // Retrieve the remote socket address from the request.  Must only
   // be called after the OnURLFetchComplete callback has run and if
   // the request has not failed.
   net::HostPortPair socket_address() const;
 
   // Returns true if the request was delivered through a proxy.  Must only
   // be called after the OnURLFetchComplete callback has run and the request
   // has not failed.
   bool was_fetched_via_proxy() const;
 
   // Start the request.  After this is called, you may not change any other
   // settings.
   virtual void Start();
 
   // Return the URL that this fetcher is processing.
   const GURL& url() const;
 
   // Reports that the received content was malformed.
   void ReceivedContentWasMalformed();
 
+  // Get the response as a string. Return false if the fetcher was not
+  // set to store the response as a string.
+  bool GetResponseAsString(std::string* response_string) const;
+
+  // Get the path to the file containing the response body. Returns false
+  // if the response body was not saved to a file.
+  bool GetResponseAsFilePath(FilePath* response_path) const;
+
   // Cancels all existing URLFetchers.  Will notify the URLFetcher::Delegates.
   // Note that any new URLFetchers created while this is running will not be
   // cancelled.  Typically, one would call this in the CleanUp() method of an IO
   // thread, so that no new URLRequests would be able to start on the IO thread
   // anyway.  This doesn't prevent new URLFetchers from trying to post to the IO
   // thread though, even though the task won't ever run.
   static void CancelAll();
 
  protected:
   // Returns the delegate.
@@ skipping 18 matching lines @@
   // If |automatically_retry_on_5xx_| is false, 5xx responses will be
   // propagated to the observer, if it is true URLFetcher will automatically
   // re-execute the request, after the back-off delay has expired.
   // true by default.
   bool automatically_retry_on_5xx_;
   // Back-off time delay. 0 by default.
   base::TimeDelta backoff_delay_;
   // Maximum retries allowed.
   int max_retries_;
 
+  // Where should responses be saved?
+  ResponseDestinationType response_destination_;
+
   static bool g_interception_enabled;
 
   DISALLOW_COPY_AND_ASSIGN(URLFetcher);
 };
 
 #endif  // CHROME_COMMON_NET_URL_FETCHER_H_