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

chrome/common/net/url_fetcher.h

Index: chrome/common/net/url_fetcher.h
diff --git a/chrome/common/net/url_fetcher.h b/chrome/common/net/url_fetcher.h
index f4fcf77ee5696ae4ae6c5ea7b06038cf2368d557..5ff7fa7e937d671924d8fb3d2d8691247dc905d7 100644
--- a/chrome/common/net/url_fetcher.h
+++ b/chrome/common/net/url_fetcher.h
@@ -20,10 +20,16 @@
 
 #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;
@@ -68,18 +74,44 @@ class URLFetcher {
     HEAD,
   };
 
+  // Where should the response be saved? Use set_response_destination()
+  // to choose.
+  enum ResponseDestinationType {
+    STRING,  // Default
+    TEMP_FILE
+  };
+
   class Delegate {
    public:
+    // 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);
+
     // 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.
+    // 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,
-                                    const std::string& data) = 0;
+                                    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() {}
@@ -183,6 +215,23 @@ class URLFetcher {
   }
 #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;
@@ -207,6 +256,14 @@ class URLFetcher {
   // 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
@@ -245,6 +302,9 @@ class URLFetcher {
   // Maximum retries allowed.
   int max_retries_;
 
+  // Where should responses be saved?
+  ResponseDestinationType response_destination_;
+
   static bool g_interception_enabled;
 
   DISALLOW_COPY_AND_ASSIGN(URLFetcher);