Fix various issues in RedirectToFileResourceHandler.

content/browser/loader/redirect_to_file_resource_handler.h

 // Copyright (c) 2012 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.
 
 #ifndef CONTENT_BROWSER_LOADER_REDIRECT_TO_FILE_RESOURCE_HANDLER_H_
 #define CONTENT_BROWSER_LOADER_REDIRECT_TO_FILE_RESOURCE_HANDLER_H_
 
+#include "base/basictypes.h"
+#include "base/compiler_specific.h"
 #include "base/files/file_path.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/platform_file.h"
 #include "content/browser/loader/layered_resource_handler.h"
+#include "content/common/content_export.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_status.h"
 
 namespace net {
 class FileStream;
 class GrowableIOBuffer;
 }
 
 namespace webkit_blob {
 class ShareableFileReference;
 }
 
 namespace content {
-class ResourceDispatcherHostImpl;
 
-// Redirects network data to a file.  This is intended to be layered in front
-// of either the AsyncResourceHandler or the SyncResourceHandler.
-class RedirectToFileResourceHandler : public LayeredResourceHandler {
+// Redirects network data to a file.  This is intended to be layered in front of
+// either the AsyncResourceHandler or the SyncResourceHandler.  The downstream
+// resource handler does not see OnWillRead or OnReadCompleted calls. Instead,
+// the ResourceResponse contains the path to a temporary file and
+// OnDataDownloaded is called as the file downloads.
+//
+// The temporary is vended through a delegate interface.
+class CONTENT_EXPORT RedirectToFileResourceHandler
+    : public LayeredResourceHandler {
  public:
+  // A delegate class for vending temporary files.
+  class Delegate {
+   public:
+    virtual ~Delegate() {}
+
+    // Creates a temporary file for a request. DidCreateTemporaryFile is called
+    // on |handler| with a net::FileStream to read from and a
+    // ShareableFileReference. The caller should retain a reference to the
+    // ShareableFileReference until it is done writing to it. When all
+    // references to the ShareableFileReference are released, the temporary is
+    // deleted. The factory itself may retain its own reference to the file.
+    //
+    // If there is an error in creating the temporary, DidCreateTemporaryFile is
+    // called with a base::PlatformFileError.
+    virtual void CreateTemporary(

[darin (slow to review), 2013/12/06 17:56:07]

Perhaps this would be cleaner as a base::Callback specialization? Ditto for
DidCreateTemporaryFile.

At the very least it seems the relationship between TemporaryFileManager and
RedirectToFileResourceHandler is inverted in this CL. TemporaryFileManager is
the utility that RedirectToFileResourceHandler could consume. Other things like
RedirectToFileResourceHandler might like to consume TemporaryFileManager too,
but TemporaryFileManager is only useful to RedirectToFileResourceHandler. As
constructed in this CL, TemporaryFileManager is an implementation detail of
RedirectToFileResourceHandler.

So, a variant on passing base::Callback<...> to RedirectToFileResourceHandler
would be to make RedirectToFileResourceHandler depend directly on
TemporaryFileManager and be passed an instance of it, but I would be fine with
this being abstracted as well.

[davidben, 2013/12/19 22:21:01]

Hrm. TemporaryFileManager's job is pretty unclear, yeah. It exists partially to
remove the handler's dependency on the RDH and its child process logic and
partially just to mock out temporary file and net::FileStream creation. I
originally split it out of RDH just so the CreateTemporary call has something to
bind the callback's lifetime to.

I've reorganized it a bit so that the child process logic is back in RDH and 
AsyncResourceHandler and SyncResourceHandler call RegisterDownloadedTemp file
instead of RedirectToFileResourceHandler. It's a little annoying with requiring
the code in two places, but I think it makes more sense for it to be at the
entry point to the child. RedirectToFileResourceHandler just downloads to a
file, and it's the downstream handler's business what happens to the file.
Granted I'm now just pushing RDH dependency from one handler to another, but
less code shuffle.

TemporaryFileManager is now just a single function and the
RedirectToFileResourceHandler gets it as a callback so the unit tests can still
inject errors and a MockFileStream.

+        int child_id,
+        int request_id,
+        base::WeakPtr<RedirectToFileResourceHandler> handler) = 0;
+  };
+
+  // Create a RedirectToFileResourceHandler for |request| which wraps
+  // |next_handler|. |delegate| must outlive the handler.
   RedirectToFileResourceHandler(
       scoped_ptr<ResourceHandler> next_handler,
       net::URLRequest* request,
-      ResourceDispatcherHostImpl* resource_dispatcher_host);
+      Delegate* delegate);
   virtual ~RedirectToFileResourceHandler();
 
-  // ResourceHandler implementation:
+  // LayeredResourceHandler implementation:
   virtual bool OnResponseStarted(int request_id,
                                  ResourceResponse* response,
                                  bool* defer) OVERRIDE;
   virtual bool OnWillStart(int request_id,
                            const GURL& url,
                            bool* defer) OVERRIDE;
   virtual bool OnWillRead(int request_id,
                           scoped_refptr<net::IOBuffer>* buf,
                           int* buf_size,
                           int min_size) OVERRIDE;
   virtual bool OnReadCompleted(int request_id,
                                int bytes_read,
                                bool* defer) OVERRIDE;
   virtual void OnResponseCompleted(int request_id,
                                    const net::URLRequestStatus& status,
                                    const std::string& security_info,
                                    bool* defer) OVERRIDE;
 
+  // Called by the delegate.
+  void DidCreateTemporaryFile(
+      base::PlatformFileError error_code,
+      scoped_ptr<net::FileStream> file_stream,
+      webkit_blob::ShareableFileReference* deletable_file);
+
  private:
-  void DidCreateTemporaryFile(base::PlatformFileError error_code,
-                              base::PassPlatformFile file_handle,
-                              const base::FilePath& file_path);
+  // Called by RedirectToFileResourceHandler::Writer.
   void DidWriteToFile(int result);
+
   bool WriteMore();
   bool BufIsFull() const;
   void ResumeIfDeferred();
 
-  base::WeakPtrFactory<RedirectToFileResourceHandler> weak_factory_;
-
-  ResourceDispatcherHostImpl* host_;
+  Delegate* delegate_;
 
   // We allocate a single, fixed-size IO buffer (buf_) used to read from the
   // network (buf_write_pending_ is true while the system is copying data into
   // buf_), and then write this buffer out to disk (write_callback_pending_ is
   // true while writing to disk).  Reading from the network is suspended while
   // the buffer is full (BufIsFull returns true).  The write_cursor_ member
   // tracks the offset into buf_ that we are writing to disk.
 
   scoped_refptr<net::GrowableIOBuffer> buf_;
   bool buf_write_pending_;
   int write_cursor_;
 
-  scoped_ptr<net::FileStream> file_stream_;
-  bool write_callback_pending_;
+  // Helper writer object which maintains references to the net::FileStream and
+  // webkit_blob::ShareableFileReference. This is maintained separately so that,
+  // on Windows, the temporary file isn't deleted until after it is closed.
+  class Writer;
+  scoped_ptr<Writer> writer_;
 
   // |next_buffer_size_| is the size of the buffer to be allocated on the next
   // OnWillRead() call.  We exponentially grow the size of the buffer allocated
   // when our owner fills our buffers. On the first OnWillRead() call, we
   // allocate a buffer of 32k and double it in OnReadCompleted() if the buffer
   // was filled, up to a maximum size of 512k.
   int next_buffer_size_;
 
-  // We create a ShareableFileReference that's deletable for the temp
-  // file created as  a result of the download.
-  scoped_refptr<webkit_blob::ShareableFileReference> deletable_file_;
-
-  bool did_defer_ ;
+  bool did_defer_;
 
   bool completed_during_write_;
+  GURL will_start_url_;
   net::URLRequestStatus completed_status_;
   std::string completed_security_info_;
 
+  base::WeakPtrFactory<RedirectToFileResourceHandler> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(RedirectToFileResourceHandler);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_LOADER_REDIRECT_TO_FILE_RESOURCE_HANDLER_H_