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 149338)
+++ 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 {
+    // 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;
+    // Flag showing whether the file name is actually points to a directory.

[wtc, 2012/08/09 16:05:43]

Nit: remove "is", or change "points" to "pointing".

Since "points" is usually associated with links (such as
symbolic links), I think it's better to simply say
"whether the file is actually a directory" or
"whether the file name actually refers to a directory".

+    bool is_directory;
+
+    FileMetaInfo();

[wtc, 2012/08/09 16:05:43]

Declare the constructor before the data members.  See the
Style Guide:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

+  };
+
   // 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);
 
+  // This function fetches file info on a background thread.

[wtc, 2012/08/09 16:05:43]

Nit: here I did mean just "Fetches".  In our comments before
function declarations, we often omit "This function" for
brevity.  See line 75 for an example.  The Style Guide also
shows example of this comment style:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Comments

+  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);
 
+  // Callback after opening file on a background thread.
+  void DidOpen(int result);
+
+  // Callback after seeking to the necessary position in the file

[wtc, 2012/08/09 16:05:43]

Nit: the use of "necessary" sounds a little strange.  If you
don't like "specified" as I suggested, how about "requested"?

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

paivanof wrote:
I see.  We can also say something like
"seeking to the beginning of byte_range_".

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