Media Galleries: On Linux, write data to snapshots in chunks rather than at once.

chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc

Index: chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc
===================================================================
--- chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc	(revision 170030)
+++ chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc	(working copy)
@@ -4,6 +4,10 @@
 
 #include "chrome/browser/media_gallery/mtp_device_delegate_impl_linux.h"
 
+#include <fcntl.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+
 #include "base/bind.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
@@ -353,14 +357,15 @@
   // Constructed on |media_task_runner_| thread.
   ReadFileWorker(const std::string& handle,
                  const std::string& path,
-                 uint32 total_size,
+                 uint32 read_offset,
+                 uint32 bytes_to_read,
                  SequencedTaskRunner* task_runner,
                  WaitableEvent* task_completed_event,
                  WaitableEvent* shutdown_event)
       : device_handle_(handle),
         path_(path),
-        total_bytes_(total_size),
-        error_occurred_(false),
+        read_offset_(read_offset),
+        bytes_to_read_(bytes_to_read),
         media_task_runner_(task_runner),
         on_task_completed_event_(task_completed_event),
         on_shutdown_event_(shutdown_event) {
@@ -377,14 +382,12 @@
       return;
     }
 
-    while (!error_occurred_ && (data_.size() < total_bytes_) &&
-           !cancel_tasks_flag_.IsSet()) {
-      BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                              Bind(&ReadFileWorker::DoWorkOnUIThread, this));
-      on_task_completed_event_->Wait();
-      if (on_shutdown_event_->IsSignaled())

[kmadhusu, 2012/11/29 18:08:17]

Instead of moving this logic from ReadFileWorker => MTPDeviceDelegateImplLinux,
have you considered passing |local_path| to ReadFileWorker and appending the
data contents to the snapshot file? By doing that, CreateSnapshotFile
implementation will be simpler and all the read file logic will be confined to
ReadFileWorker class.

[Lei Zhang, 2012/11/30 01:01:34]

Done.

-        cancel_tasks_flag_.Set();
-    }
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                            Bind(&ReadFileWorker::DoWorkOnUIThread, this));
+    on_task_completed_event_->Wait();
+
+    if (on_shutdown_event_->IsSignaled())
+      cancel_tasks_flag_.Set();
   }
 
   // Returns the media file contents received from mtpd.
@@ -415,7 +418,7 @@
       return;
 
     GetMediaTransferProtocolManager()->ReadFileChunkByPath(
-        device_handle_, path_, data_.size(), BytesToRead(),
+        device_handle_, path_, read_offset_, bytes_to_read_,
         Bind(&ReadFileWorker::OnDidWorkOnUIThread, this));
   }
 
@@ -427,42 +430,25 @@
     if (cancel_tasks_flag_.IsSet())
       return;
 
-    error_occurred_ = error;
     if (!error) {
-      if ((BytesToRead() == data.size())) {
-        // TODO(kmadhusu): Data could be really huge. Consider passing data by
-        // pointer/ref rather than by value here to avoid an extra data copy.
-        data_.append(data);
-      } else {
-        NOTREACHED();
-        error_occurred_ = true;
-      }
+      DCHECK_EQ(bytes_to_read_, data.size());
+      data_ = data;
     }
     on_task_completed_event_->Signal();
   }
 
-  uint32 BytesToRead() const {
-    // Read data in 1 MB chunks.
-    static const uint32 kReadChunkSize = 1024 * 1024;
-    return std::min(kReadChunkSize,
-                    total_bytes_ - static_cast<uint32>(data_.size()));
-  }
-
   // The device unique identifier to query the device.
   const std::string device_handle_;
 
   // The media device file path.
   const std::string path_;
 
+  const uint32 read_offset_;
+  const uint32 bytes_to_read_;
+
   // The data from mtpd.
   std::string data_;
 
-  // Number of bytes to read.
-  const uint32 total_bytes_;
-
-  // Whether an error occurred during file transfer.
-  bool error_occurred_;
-
   // A reference to |media_task_runner_| to destruct this object on the correct
   // thread.
   scoped_refptr<SequencedTaskRunner> media_task_runner_;
@@ -857,25 +843,39 @@
   if (file_info->size <= 0 || file_info->size > kuint32max)
     return base::PLATFORM_FILE_ERROR_FAILED;
 
-  scoped_refptr<ReadFileWorker> worker(new ReadFileWorker(
-      device_handle_,
-      GetDeviceRelativePath(device_path_, device_file_path.value()),
-      file_info->size,
-      media_task_runner_, &on_task_completed_event_, &on_shutdown_event_));
-  worker->Run();
+  // Read data in 1 MB chunks.
+  static const uint32 kReadChunkSize = 1024 * 1024;
+  uint32 bytes_remaining = file_info->size;
+  int fd = open(local_path.value().c_str(), O_WRONLY);
+  if (fd < 0)
+    return base::PLATFORM_FILE_ERROR_FAILED;
+  file_util::ScopedFD fd_scoper(&fd);
 
-  const std::string& file_data = worker->data();
-  int data_size = static_cast<int>(file_data.length());
-  if (file_data.empty() ||
-      file_util::WriteFile(local_path, file_data.c_str(),
-                           data_size) != data_size) {
-    return base::PLATFORM_FILE_ERROR_FAILED;
+  while (bytes_remaining > 0) {
+    uint32 offset = file_info->size - bytes_remaining;
+    uint32 bytes_to_read = std::min(kReadChunkSize, bytes_remaining);
+
+    scoped_refptr<ReadFileWorker> worker(new ReadFileWorker(
+        device_handle_,
+        GetDeviceRelativePath(device_path_, device_file_path.value()),
+        offset, bytes_to_read,
+        media_task_runner_, &on_task_completed_event_, &on_shutdown_event_));
+    worker->Run();
+
+    const std::string& file_data = worker->data();
+    int data_size = static_cast<int>(file_data.length());
+    if (bytes_to_read != file_data.length() ||
+        file_util::WriteFileDescriptor(fd, file_data.c_str(), data_size) < 0) {
+      return base::PLATFORM_FILE_ERROR_FAILED;
+    }
+
+    bytes_remaining -= data_size;
   }
 
   // Modify the last modified time to null. This prevents the time stamp
   // verfication in LocalFileStreamReader.
   file_info->last_modified = base::Time();
-  return error;
+  return base::PLATFORM_FILE_OK;
 }
 
 SequencedTaskRunner* MTPDeviceDelegateImplLinux::GetMediaTaskRunner() {