Refactor BaseFile methods to return a DownloadInterruptReason instead of a net::Error.

content/browser/download/base_file.h

Index: content/browser/download/base_file.h
diff --git a/content/browser/download/base_file.h b/content/browser/download/base_file.h
index d1daaf192ee0e7dc0d098f12b513d72eb29a0fd2..19fe060e7d0154bf8b6010e00818154ae4c6ab33 100644
--- a/content/browser/download/base_file.h
+++ b/content/browser/download/base_file.h
@@ -13,11 +13,15 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/time.h"
 #include "content/common/content_export.h"
+#include "content/public/browser/download_interrupt_reasons.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/file_stream.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_log.h"
 
+namespace content {
+enum DownloadInterruptReason;
+}
 namespace crypto {
 class SecureHash;
 }
@@ -41,21 +45,23 @@ class CONTENT_EXPORT BaseFile {
            const net::BoundNetLog& bound_net_log);
   virtual ~BaseFile();
 
-  // Returns net::OK on success, or a network error code on failure.
-  // |default_directory| specifies the directory to create the temporary file in
-  // if |full_path()| is empty. If |default_directory| and |full_path()| are
-  // empty, then a temporary file will be created in the default download
-  // location as determined by ContentBrowserClient.
-  net::Error Initialize(const FilePath& default_directory);
+  // Returns DOWNLOAD_INTERRUPT_REASON_NONE on success, or a
+  // DownloadInterruptReason on failure.  |default_directory| specifies the
+  // directory to create the temporary file in if |full_path()| is empty. If
+  // |default_directory| and |full_path()| are empty, then a temporary file will
+  // be created in the default download location as determined by
+  // ContentBrowserClient.
+  content::DownloadInterruptReason Initialize(
+      const FilePath& default_directory);
 
-  // Write a new chunk of data to the file.
-  // Returns net::OK on success (all bytes written to the file),
-  // or a network error code on failure.
-  net::Error AppendDataToFile(const char* data, size_t data_len);
+  // Write a new chunk of data to the file. Returns a DownloadInterruptReason
+  // indicating the result of the operation.
+  content::DownloadInterruptReason AppendDataToFile(const char* data,
+                                                    size_t data_len);
 
-  // Rename the download file.
-  // Returns net::OK on success, or a network error code on failure.
-  virtual net::Error Rename(const FilePath& full_path);
+  // Rename the download file. Returns a DownloadInterruptReason indicating the
+  // result of the operation.
+  virtual content::DownloadInterruptReason Rename(const FilePath& full_path);
 
   // Detach the file so it is not deleted on destruction.
   virtual void Detach();
@@ -66,8 +72,9 @@ class CONTENT_EXPORT BaseFile {
   // Indicate that the download has finished. No new data will be received.
   void Finish();
 
-  // Informs the OS that this file came from the internet.
-  void AnnotateWithSourceInformation();
+  // Informs the OS that this file came from the internet. Returns a
+  // DownloadInterruptReason indicating the result of the operation.
+  content::DownloadInterruptReason AnnotateWithSourceInformation();

[Randy Smith (Not in Mondays), 2012/10/23 20:18:13]

I don't object, but why?  Nothing appears to return an error.  If for a future
CL, I think it's better to have the change to returning an error code in that
CL.

[asanka, 2012/10/23 21:53:34]

Done.

 
   // Calculate and return the current speed in bytes per second.
   int64 CurrentSpeed() const;
@@ -90,30 +97,51 @@ class CONTENT_EXPORT BaseFile {
 
   virtual std::string DebugString() const;
 
- protected:
-  virtual void CreateFileStream();  // For testing.
-  // Returns net::OK on success, or a network error code on failure.
-  net::Error Open();
-  void Close();
-
-  // Full path to the file including the file name.
-  FilePath full_path_;
-
  private:
   friend class BaseFileTest;
   FRIEND_TEST_ALL_PREFIXES(BaseFileTest, IsEmptyHash);
 
+  // Re-initializes file_stream_ with a newly allocated net::FileStream().
+  void CreateFileStream();
+
+  // Creates and opens the file_stream_ if it is NULL.
+  content::DownloadInterruptReason Open();
+
+  // Closes and resets file_stream_.
+  void Close();
+
+  // Resets file_stream_.
+  void ClearStream();
+
+  // Platform specific method that moves a file to a new path and adjusts the
+  // security descriptor / permissions on the file to match the defaults for the
+  // new directory.
+  content::DownloadInterruptReason MoveFileAndAdjustPermissions(
+      const FilePath& new_path);
+
   // Split out from CurrentSpeed to enable testing.
   int64 CurrentSpeedAtTime(base::TimeTicks current_time) const;
 
   // Resets the current state of the hash to the contents of |hash_state_bytes|.
   virtual bool SetHashState(const std::string& hash_state_bytes);
 
-  net::Error ClearStream(net::Error error);
+  // Log a TYPE_DOWNLOAD_FILE_ERROR NetLog event with |error| and passes error
+  // on through, converting to a |content::DownloadInterruptReason|.
+  content::DownloadInterruptReason LogNetError(
+      const char* operation, net::Error error);
+
+  // Log a TYPE_DOWNLOAD_FILE_ERROR NetLog event with |os_error| and |reason|.
+  // Returns |reason|.
+  content::DownloadInterruptReason LogInterruptReason(
+      const char* operation, int os_error,

[Randy Smith (Not in Mondays), 2012/10/23 20:18:13]

Why associate the os_error logging with the DownloadInterruptReason logging and
not net::Error logging?

[asanka, 2012/10/23 21:53:34]

It's based on usage. When we have a net::Error we don't have an os_error.

I added a LogSystemError() method that's used when we only have a system error.

+      content::DownloadInterruptReason reason);
 
   static const size_t kSha256HashLen = 32;
   static const unsigned char kEmptySha256Hash[kSha256HashLen];
 
+  // Full path to the file including the file name.
+  FilePath full_path_;
+
   // Source URL for the file being downloaded.
   GURL source_url_;