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

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

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170414

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

https://codereview.chromium.org/11414221/diff/1/chrome/browser/media_gallery/...
File chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc (left):

https://codereview.chromium.org/11414221/diff/1/chrome/browser/media_gallery/...
chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:385: if
(on_shutdown_event_->IsSignaled())
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]

https://codereview.chromium.org/11414221/diff/1/chrome/browser/media_gallery/...
File chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc (left):

https://codereview.chromium.org/11414221/diff/1/chrome/browser/media_gallery/...
chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:385: if
(on_shutdown_event_->IsSignaled())
On 2012/11/29 18:08:17, kmadhusu wrote:
> 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.

Done.

[kmadhusu, 2012-11-30 01:13:09]

https://codereview.chromium.org/11414221/diff/5001/chrome/browser/media_galle...
File chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc (right):

https://codereview.chromium.org/11414221/diff/5001/chrome/browser/media_galle...
chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:448:
file_util::WriteFileDescriptor(dest_fd_, data.data(), data.size());
You are trying to do a file write operation on a UI thread. You should move this
code to line 397.

[Lei Zhang, 2012-11-30 01:47:28]

See patch set 3.

[kmadhusu, 2012-11-30 02:05:12]

https://codereview.chromium.org/11414221/diff/7001/chrome/browser/media_galle...
File chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc (right):

https://codereview.chromium.org/11414221/diff/7001/chrome/browser/media_galle...
chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:400: }
When the MTR resumes, if |error_occured_| is true, we should break.

https://codereview.chromium.org/11414221/diff/7001/chrome/browser/media_galle...
chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:412: return
!error_occurred_ && bytes_read_ == total_bytes_;
nit: () around binary expressions.

https://codereview.chromium.org/11414221/diff/7001/chrome/browser/media_galle...
chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:453: if
(!error_occurred_)
If |error_occurred_| is true, we should reset |data_|. If not, we will write the
previous value of |data_| in line 401.

https://codereview.chromium.org/11414221/diff/7001/chrome/browser/media_galle...
chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:480:
file_util::ScopedFD dest_fd_scoper_;
Is there any specific reason to have |dest_fd_| and |dest_fd_scoper_| as member
variables instead of local variables in Run()?

https://codereview.chromium.org/11414221/diff/7001/chrome/browser/media_galle...
chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:897:
media_task_runner_, &on_task_completed_event_, &on_shutdown_event_));
nit: some of these params can fit in the previous line.

[Lei Zhang, 2012-11-30 02:30:55]

https://codereview.chromium.org/11414221/diff/7001/chrome/browser/media_galle...
File chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc (right):

https://codereview.chromium.org/11414221/diff/7001/chrome/browser/media_galle...
chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:400: }
On 2012/11/30 02:05:12, kmadhusu wrote:
> When the MTR resumes, if |error_occured_| is true, we should break. 

Done.

https://codereview.chromium.org/11414221/diff/7001/chrome/browser/media_galle...
chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:412: return
!error_occurred_ && bytes_read_ == total_bytes_;
On 2012/11/30 02:05:12, kmadhusu wrote:
> nit: () around binary expressions.

Done.

https://codereview.chromium.org/11414221/diff/7001/chrome/browser/media_galle...
chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:453: if
(!error_occurred_)
On 2012/11/30 02:05:12, kmadhusu wrote:
> If |error_occurred_| is true, we should reset |data_|. If not, we will write
the
> previous value of |data_| in line 401.

I fixed the checks above so that won't happen.

https://codereview.chromium.org/11414221/diff/7001/chrome/browser/media_galle...
chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:480:
file_util::ScopedFD dest_fd_scoper_;
On 2012/11/30 02:05:12, kmadhusu wrote:
> Is there any specific reason to have |dest_fd_| and |dest_fd_scoper_| as
member
> variables instead of local variables in Run()?

Not needed any more. Removed.

https://codereview.chromium.org/11414221/diff/7001/chrome/browser/media_galle...
chrome/browser/media_gallery/mtp_device_delegate_impl_linux.cc:897:
media_task_runner_, &on_task_completed_event_, &on_shutdown_event_));
On 2012/11/30 02:05:12, kmadhusu wrote:
> nit: some of these params can fit in the previous line.

Done.
That could have been said of the previous code too.

[kmadhusu, 2012-11-30 02:39:02]

lgtm

[commit-bot: I haz the power, 2012-11-30 03:59:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/11414221/7002

[commit-bot: I haz the power, 2012-11-30 07:27:38]

Change committed as 170414