Implement snapshotting for non-Drive non-local file systems.

Implement snapshotting for non-Drive non-local file systems.

This is for supporting uploading files from those file systems
(MTP, providedFS) to web pages, by a workaround before the
long term solution resolving the issue crbug/126902 comes.

BUG=383207
R=hashimoto@chromium.org

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

[hashimoto, 2014-06-19 02:26:01]

https://codereview.chromium.org/339323002/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_manager/snapshot_manager.cc (right):

https://codereview.chromium.org/339323002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_manager/snapshot_manager.cc:30: void
DeleteDirectory(const base::FilePath& directory) {
nit: What is the point of having this function instead of directory calling
Bind() with base::DeleteFile?

https://codereview.chromium.org/339323002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_manager/snapshot_manager.cc:118: if (task_runner_
&& !base_path_.empty()) {
nit: In which case task_runner_ becomes NULL?

https://codereview.chromium.org/339323002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_manager/snapshot_manager.cc:121: FROM_HERE,
base::Bind(&DeleteDirectory, base_path_));
I don't remember in which order objects are destructed during a shutdown.
Did you confirm that task_runner_ is still alive when SnapshotManager is
destroyed?
How about having DCHECK(posted)?

https://codereview.chromium.org/339323002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_manager/snapshot_manager.cc:139: if
(base_path_.empty()) {
nit: Why can't this be initialized in the ctor?

https://codereview.chromium.org/339323002/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_manager/snapshot_manager.h (right):

https://codereview.chromium.org/339323002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_manager/snapshot_manager.h:23: // managed by this
manager class.
Why should we create a copy of the snapshot instead of keeping the file ref with
this class?

[kinaba, 2014-06-19 04:41:56]

https://codereview.chromium.org/339323002/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_manager/snapshot_manager.cc (right):

https://codereview.chromium.org/339323002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_manager/snapshot_manager.cc:30: void
DeleteDirectory(const base::FilePath& directory) {
On 2014/06/19 02:26:01, hashimoto wrote:
> nit: What is the point of having this function instead of directory calling
> Bind() with base::DeleteFile?

Good catch. Removed.
(Actually I hadn't been able to figure out base::IgnoreResult is usable in this
case.)

https://codereview.chromium.org/339323002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_manager/snapshot_manager.cc:118: if (task_runner_
&& !base_path_.empty()) {
On 2014/06/19 02:26:01, hashimoto wrote:
> nit: In which case task_runner_ becomes NULL?

Never.

https://codereview.chromium.org/339323002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_manager/snapshot_manager.cc:121: FROM_HERE,
base::Bind(&DeleteDirectory, base_path_));
On 2014/06/19 02:26:01, hashimoto wrote:
> I don't remember in which order objects are destructed during a shutdown.
> Did you confirm that task_runner_ is still alive when SnapshotManager is
> destroyed?
> How about having DCHECK(posted)?

It is still alive at this point (profile destruction). Added DCHECK.

https://codereview.chromium.org/339323002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_manager/snapshot_manager.cc:139: if
(base_path_.empty()) {
On 2014/06/19 02:26:01, hashimoto wrote:
> nit: Why can't this be initialized in the ctor?

StorageParition/FileSystemContext related part is not yet initialized at that
point.

https://codereview.chromium.org/339323002/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_manager/snapshot_manager.h (right):

https://codereview.chromium.org/339323002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_manager/snapshot_manager.h:23: // managed by this
manager class.
On 2014/06/19 02:26:01, hashimoto wrote:
> Why should we create a copy of the snapshot instead of keeping the file ref
with
> this class?

That sounds far more better! Rewrote in the suggested approach and verified it
works.

[hashimoto, 2014-06-19 06:16:52]

lgtm

https://codereview.chromium.org/339323002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/snapshot_manager.cc (right):

https://codereview.chromium.org/339323002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/snapshot_manager.cc:11: #include
"chrome/browser/profiles/profile.h"
nit: No need to include this? Seems this class is just passing a pointer of
Profile without accessing any members.

https://codereview.chromium.org/339323002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/snapshot_manager.cc:22: const
fileapi::FileSystemOperation::SnapshotFileCallback& callback) {
q: Are you going to implement a mechanism to evict file refs when the disk space
is not sufficient?

https://codereview.chromium.org/339323002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/snapshot_manager.h (right):

https://codereview.chromium.org/339323002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/snapshot_manager.h:13: #include
"webkit/common/blob/shareable_file_reference.h"
nit: Can't be replaced with a forward declaration?

[kinaba, 2014-06-19 08:47:59]

Thanks! I'll update and commit the CL tomorrow. (My workstation was broken today
>_< ...)

https://codereview.chromium.org/339323002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/snapshot_manager.cc (right):

https://codereview.chromium.org/339323002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/snapshot_manager.cc:22: const
fileapi::FileSystemOperation::SnapshotFileCallback& callback) {
On 2014/06/19 06:16:51, hashimoto wrote:
> q: Are you going to implement a mechanism to evict file refs when the disk
space
> is not sufficient?

Yes. Filed Bug 386519.

[kinaba, 2014-06-20 01:17:08]

https://codereview.chromium.org/339323002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/snapshot_manager.cc (right):

https://codereview.chromium.org/339323002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/snapshot_manager.cc:11: #include
"chrome/browser/profiles/profile.h"
On 2014/06/19 06:16:51, hashimoto wrote:
> nit: No need to include this? Seems this class is just passing a pointer of
> Profile without accessing any members.

Done.

https://codereview.chromium.org/339323002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_manager/snapshot_manager.h (right):

https://codereview.chromium.org/339323002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_manager/snapshot_manager.h:13: #include
"webkit/common/blob/shareable_file_reference.h"
On 2014/06/19 06:16:52, hashimoto wrote:
> nit: Can't be replaced with a forward declaration?

Done.

[kinaba, 2014-06-20 01:17:44]

The CQ bit was checked by kinaba@chromium.org

[commit-bot: I haz the power, 2014-06-20 01:19:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kinaba@chromium.org/339323002/60001

[kinaba, 2014-06-20 06:52:09]

Committed patchset #4 manually as r278637 (presubmit successful).