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/callback_forward.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 "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

[mmenke, 2013/12/05 21:34:03]

Should include content_export.h

[davidben, 2013/12/05 22:43:20]

Done.

+    : public LayeredResourceHandler {
  public:
+  typedef base::Callback<

[mmenke, 2013/12/05 21:34:03]

Should include callback.h or callback_forward.h.

[davidben, 2013/12/05 22:43:20]

It's already there, but...

+    void(base::PlatformFileError,
+         scoped_ptr<net::FileStream>,
+         scoped_refptr<webkit_blob::ShareableFileReference>)>
+  CreateTemporaryCallback;

[mmenke, 2013/12/05 21:34:03]

I'm unable to find anything that's quite this ugly to model this off of, but I
suggest:

typedef base::Callback<
    void(base::PlatformFileError,
         scoped_ptr<net::FileStream>,
         scoped_refptr<webkit_blob::ShareableFileReference>)>
    CreateTemporaryCallback;

As being slightly easier to read.

[davidben, 2013/12/05 22:43:20]

Actually deleting it altogether. It's gone now and I forgot to remove the
typedef.

+
+

[mmenke, 2013/12/05 21:34:03]

nit:  Remove extra blank line.

[davidben, 2013/12/05 22:43:20]

Done.

+  // 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(
+        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 RedirectToFileResourceHandler::Writer.
+  void DidWriteToFile(int result);

[mmenke, 2013/12/05 21:34:03]

This can be private, since inner classes are friends of their parent classes
(But not the other way around).

[davidben, 2013/12/05 22:43:20]

Done. Huh, I didn't actually know that. Handy.

+
+  // 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);
-  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);

[mmenke, 2013/12/05 21:34:03]

While you're fixing this file, should add includes for compiler_specific.h and
basictypes.h

[davidben, 2013/12/05 22:43:20]

Done. I kinda wonder if we should just have a single header included everywhere
which pulls in compiler_specific.h and basictypes.h. They seem pretty common.

[mmenke, 2013/12/06 16:15:39]

Indeed.  Even just having one include the other would make life simpler.

 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_LOADER_REDIRECT_TO_FILE_RESOURCE_HANDLER_H_