Avoid disk accesses on the wrong thread in URLRequestFileJob

net/url_request/url_request_file_job.h

Index: net/url_request/url_request_file_job.h
===================================================================
--- net/url_request/url_request_file_job.h	(revision 151422)
+++ net/url_request/url_request_file_job.h	(working copy)
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/file_path.h"
+#include "base/memory/weak_ptr.h"
 #include "net/base/file_stream.h"
 #include "net/base/net_export.h"
 #include "net/http/http_byte_range.h"
@@ -52,29 +53,55 @@
   FilePath file_path_;
 
  private:
+  // Meta information about the file. It's used as a member in the
+  // URLRequestFileJob and also passed between threads because disk access is
+  // necessary to obtain it.
+  struct FileMetaInfo {
+    FileMetaInfo();
+
+    // Size of the file.
+    int64 file_size;
+    // Mime type associated with the file.
+    std::string mime_type;
+    // Result returned from GetMimeTypeFromFile(), i.e. flag showing whether
+    // obtaining of the mime type was successful.
+    bool mime_type_result;
+    // Flag showing whether the file exists.
+    bool file_exists;

[wtc, 2012/08/14 18:00:40]

Nit: I suggest listing the struct members in this order:
  file_exists
  file_size
  is_directory
  mime_type_result
  mime_type

This order highlights the importance of file_exists and
mime_type_results -- if they are false, the members after
them are irrelevant.

[pivanof, 2012/08/15 06:36:50]

I've placed members like that to avoid unnecessary alignment gaps. Do you think
that's less important than such logical order? If we change the order then
probably comments should be changed to explain the placement logic, right?

[wtc, 2012/08/17 00:24:04]

Ah, I see.  Then please disregard my suggestion.

+    // Flag showing whether the file name actually refers to a directory.
+    bool is_directory;
+  };
+
   // Tests to see if access to |path| is allowed. If g_allow_file_access_ is
   // true, then this will return true. If the NetworkDelegate associated with
   // the |request| says it's OK, then this will also return true.
   static bool IsFileAccessAllowed(const URLRequest& request,
                                   const FilePath& path);
 
+  // Fetches file info on a background thread.
+  static void FetchMetaInfo(const FilePath& file_path,
+                            FileMetaInfo* meta_info);
+
   // Callback after fetching file info on a background thread.
-  void DidResolve(bool exists, const base::PlatformFileInfo& file_info);
+  void DidFetchMetaInfo(const FileMetaInfo* meta_info);

[wtc, 2012/08/14 18:00:40]

Nit: should the input parameter be a const reference instead of
a const pointer?  Does PostTaskAndReply require this to be
a pointer?

[pivanof, 2012/08/15 06:36:50]

IIUC, with const reference PostTaskAndReply will save contents of FileMetaInfo
struct at the time of calling base::Bind(), i.e. even before the call to
PostTaskAndReply(). And so any changes inside FetchMetaInfo() won't come to
DidFetchMetaInfo().

 
+  // Callback after opening file on a background thread.
+  void DidOpen(int result);
+
+  // Callback after seeking to the requested position in the file
+  // on a background thread.

[wtc, 2012/08/17 00:24:04]

Please change this comment to:
  // Callback after seeking to the beginning of |byte_range_| in
  // the file on a background thread

+  void DidSeek(int64 result);
+
   // Callback after data is asynchronously read from the file.
   void DidRead(int result);
 
-  FileStream stream_;
-  bool is_directory_;
+  scoped_ptr<FileStream> stream_;
+  FileMetaInfo meta_info_;
 
   HttpByteRange byte_range_;
   int64 remaining_bytes_;
 
-  // The initial file metadata is fetched on a background thread.
-  // AsyncResolver runs that task.
-  class AsyncResolver;
-  friend class AsyncResolver;
-  scoped_refptr<AsyncResolver> async_resolver_;
+  base::WeakPtrFactory<URLRequestFileJob> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(URLRequestFileJob);
 };