Refactor BaseFile class to support sparse files

content/browser/download/base_file.cc

Index: content/browser/download/base_file.cc
diff --git a/content/browser/download/base_file.cc b/content/browser/download/base_file.cc
index 59dfa839cd51dca2373f2b6204d5a64c418fb41c..f4afd4addf655384f9248623b85d91344f65a00c 100644
--- a/content/browser/download/base_file.cc
+++ b/content/browser/download/base_file.cc
@@ -44,7 +44,8 @@ DownloadInterruptReason BaseFile::Initialize(
     base::File file,
     int64_t bytes_so_far,
     const std::string& hash_so_far,
-    std::unique_ptr<crypto::SecureHash> hash_state) {
+    std::unique_ptr<crypto::SecureHash> hash_state,
+    AccessMode access_mode) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
   DCHECK(!detached_);
 
@@ -70,6 +71,8 @@ DownloadInterruptReason BaseFile::Initialize(
 
   bytes_so_far_ = bytes_so_far;
   secure_hash_ = std::move(hash_state);
+  access_mode_ = access_mode;
+  DCHECK(access_mode_ == EXCLUSIVE || !secure_hash_);
   file_ = std::move(file);
 
   return Open(hash_so_far);
@@ -77,9 +80,24 @@ DownloadInterruptReason BaseFile::Initialize(
 
 DownloadInterruptReason BaseFile::AppendDataToFile(const char* data,
                                                    size_t data_len) {
-  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
-  DCHECK(!detached_);
+  DCHECK_EQ(access_mode_, EXCLUSIVE);
+  return WriteAtCurrentPos(data, data_len);
+}
+
+DownloadInterruptReason BaseFile::WriteDataToFile(int64_t offset,
+                                                  const char* data,
+                                                  size_t data_len) {
+  DCHECK_EQ(access_mode_, SHARED);
+
+  if (file_.Seek(base::File::FROM_BEGIN, offset) < 0) {

[asanka, 2017/02/16 18:11:24]

base::File::Write() can be used to write to a specific offset without an
additional Seek()

[asanka, 2017/02/16 18:46:25]

Disregard.

[qinmin, 2017/02/16 21:35:50]

I used this seek() to reuse some of code from AppendDataToFile(). For example,
if I use Write(offset, data, len), when will have to check the return value, and
do a for loop to write the remainder data, and update the bytes_so_far_. By
doing a seek, i can reuse the while loop in AppendDataToFile(). 

It is also very strange that we actually have this while loop in our code.
base::File::Write() already does a while loop for us. If it writes fewer bytes
than asked, this means an error. So I don't understand why this comment is here
"// The Write call below is not guaranteed to write all the data.". Maybe this
is for some systems that have a flawed base::File::Write() implementation?  I
can fix this in a separate CL if this comment is incorrect.

[asanka, 2017/02/16 21:58:02]

Probably because this code predates the logic in base::File ?  We should remove
the loop around the File::Write() calls. Then afterwards we can get rid of the
additional Seek(). I'm okay with doing so in a separate CL.

[qinmin, 2017/02/16 22:20:29]

Done. Removed the while loop around the Write() call and removed the seek()
call.

+    logging::SystemErrorCode error = logging::GetLastSystemErrorCode();
+    return LogSystemError("Unable to seek", error);
+  }
+  return WriteAtCurrentPos(data, data_len);
+}
 
+DownloadInterruptReason BaseFile::WriteAtCurrentPos(const char* data,
+                                                    size_t data_len) {
   // NOTE(benwells): The above DCHECK won't be present in release builds,
   // so we log any occurences to see how common this error is in the wild.
   if (detached_)
@@ -288,6 +306,16 @@ DownloadInterruptReason BaseFile::Open(const std::string& hash_so_far) {
       net::NetLogEventType::DOWNLOAD_FILE_OPENED,
       base::Bind(&FileOpenedNetLogCallback, &full_path_, bytes_so_far_));
 
+  // For SHARED file, skip hash validation.
+  if (access_mode_ == SHARED) {
+    if (file_.GetLength() < bytes_so_far_) {
+      ClearFile();
+      return LogInterruptReason("File has fewer written bytes than expected", 0,
+                                DOWNLOAD_INTERRUPT_REASON_FILE_TOO_SHORT);
+    }
+    return DOWNLOAD_INTERRUPT_REASON_NONE;
+  }
+
   if (!secure_hash_) {
     DownloadInterruptReason reason = CalculatePartialHash(hash_so_far);
     if (reason != DOWNLOAD_INTERRUPT_REASON_NONE) {