Fix various issues in RedirectToFileResourceHandler.

content/browser/loader/redirect_to_file_resource_handler.h

Index: content/browser/loader/redirect_to_file_resource_handler.h
diff --git a/content/browser/loader/redirect_to_file_resource_handler.h b/content/browser/loader/redirect_to_file_resource_handler.h
index 3d62835e8888d734a8e144356177d6a1faa87ea2..eb166efe0806082af4902ef91708036405d418e0 100644
--- a/content/browser/loader/redirect_to_file_resource_handler.h
+++ b/content/browser/loader/redirect_to_file_resource_handler.h
@@ -5,12 +5,17 @@
 #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/callback.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/browser/loader/temporary_file_stream.h"
+#include "content/common/content_export.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_status.h"
 
@@ -24,19 +29,28 @@ 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:
-  RedirectToFileResourceHandler(
-      scoped_ptr<ResourceHandler> next_handler,
-      net::URLRequest* request,
-      ResourceDispatcherHostImpl* resource_dispatcher_host);
+  typedef base::Callback<void(const CreateTemporaryFileStreamCallback&)>
+      CreateTemporaryCB;

[mmenke, 2014/01/16 16:43:26]

Google style guide strongly discourages uncommon abbreviations.  Seems like
"callback" is almost always written out.

Also, I think this name is very confusing.  A
"CreateTemporaryFileStreamCallback" is what's called after the FileStream is
created, but a "CreateTemporaryCB" is a callback that creates a FileStream.

I think naming this just "CreateTemporaryFileStream" would be much clearer.

[mmenke, 2014/01/16 16:43:26]

I also think this should have a comment...  Maybe something like "Creates a file
stream and a ShareableFileReference, and then asynchronously passes them to
CreateTemporaryFileStreamCallback."

[davidben, 2014/01/29 21:41:50]

How's this version?

+
+  // Create a RedirectToFileResourceHandler for |request| which wraps
+  // |next_handler|. |create_temporary_cb| is called to create a temporary file.
+  RedirectToFileResourceHandler(scoped_ptr<ResourceHandler> next_handler,
+                                net::URLRequest* request,
+                                const CreateTemporaryCB& create_temporary_cb);
   virtual ~RedirectToFileResourceHandler();
 
-  // ResourceHandler implementation:
+  // LayeredResourceHandler implementation:
   virtual bool OnResponseStarted(int request_id,
                                  ResourceResponse* response,
                                  bool* defer) OVERRIDE;
@@ -56,17 +70,19 @@ class RedirectToFileResourceHandler : public LayeredResourceHandler {
                                    bool* defer) OVERRIDE;
 
  private:
-  void DidCreateTemporaryFile(base::PlatformFileError error_code,
-                              base::PassPlatformFile file_handle,
-                              const base::FilePath& file_path);
+  void DidCreateTemporaryFile(
+      base::PlatformFileError error_code,
+      scoped_ptr<net::FileStream> file_stream,
+      webkit_blob::ShareableFileReference* deletable_file);
+
+  // Called by RedirectToFileResourceHandler::Writer.
   void DidWriteToFile(int result);
+
   bool WriteMore();
   bool BufIsFull() const;
   void ResumeIfDeferred();
 
-  base::WeakPtrFactory<RedirectToFileResourceHandler> weak_factory_;
-
-  ResourceDispatcherHostImpl* host_;
+  CreateTemporaryCB create_temporary_cb_;

[mmenke, 2014/01/16 16:43:26]

const?

[davidben, 2014/01/29 21:41:50]

Pulled it out into a Set...ForTesting function.

 
   // 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
@@ -79,8 +95,11 @@ class RedirectToFileResourceHandler : public LayeredResourceHandler {
   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;
+  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
@@ -89,16 +108,15 @@ class RedirectToFileResourceHandler : public LayeredResourceHandler {
   // 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);
 };