Stop returning the true root path of each filesystem from openFileSystem....

webkit/fileapi/local_file_system_file_util.cc

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "webkit/fileapi/local_file_system_file_util.h"
+
+#include "base/file_util_proxy.h"
+#include "googleurl/src/gurl.h"
+#include "webkit/fileapi/file_system_context.h"
+#include "webkit/fileapi/file_system_operation_context.h"
+#include "webkit/fileapi/file_system_path_manager.h"
+#include "webkit/fileapi/file_system_types.h"
+#include "webkit/fileapi/file_system_util.h"
+
+namespace {
+
+FilePath GetRootPath(
+    fileapi::FileSystemOperationContext* context,
+    const GURL& origin_url,
+    fileapi::FileSystemType type) {
+  return context->file_system_context()->path_manager()->
+      GetFileSystemRootPathOnFileThread(origin_url, type, false);
+}
+
+}  // namespace
+
+namespace fileapi {
+
+LocalFileSystemFileUtil* LocalFileSystemFileUtil::GetInstance() {
+  return Singleton<LocalFileSystemFileUtil>::get();
+}
+
+PlatformFileError LocalFileSystemFileUtil::CreateOrOpen(
+    FileSystemOperationContext* context,
+    const FilePath& file_path, int file_flags,
+    PlatformFile* file_handle, bool* created) {
+  FilePath local_path =
+      GetLocalPath(context, context->src_origin_url(), context->src_type(),
+          file_path);
+  if (local_path.empty())
+    return base::PLATFORM_FILE_ERROR_INVALID_OPERATION;
+  return FileSystemFileUtil::GetInstance()->CreateOrOpen(
+      context, local_path, file_flags, file_handle, created);
+}
+
+PlatformFileError LocalFileSystemFileUtil::EnsureFileExists(
+    FileSystemOperationContext* context,
+    const FilePath& file_path,
+    bool* created) {
+  FilePath local_path =
+      GetLocalPath(context, context->src_origin_url(), context->src_type(),
+          file_path);
+  if (local_path.empty())
+    return base::PLATFORM_FILE_ERROR_INVALID_OPERATION;
+  return FileSystemFileUtil::GetInstance()->EnsureFileExists(
+      context, local_path, created);
+}
+
+PlatformFileError LocalFileSystemFileUtil::GetFileInfo(
+    FileSystemOperationContext* context,
+    const FilePath& file_path,
+    base::PlatformFileInfo* file_info) {
+  FilePath local_path =
+      GetLocalPath(context, context->src_origin_url(), context->src_type(),
+          file_path);
+  if (local_path.empty())
+    return base::PLATFORM_FILE_ERROR_INVALID_OPERATION;
+  return FileSystemFileUtil::GetInstance()->GetFileInfo(
+      context, local_path, file_info);
+}
+
+PlatformFileError LocalFileSystemFileUtil::ReadDirectory(
+    FileSystemOperationContext* context,
+    const FilePath& file_path,
+    std::vector<base::FileUtilProxy::Entry>* entries) {
+  // TODO(kkanetkar): Implement directory read in multiple chunks.
+  FilePath local_path =
+      GetLocalPath(context, context->src_origin_url(), context->src_type(),
+          file_path);
+  if (local_path.empty())
+    return base::PLATFORM_FILE_ERROR_INVALID_OPERATION;
+  return FileSystemFileUtil::GetInstance()->ReadDirectory(
+      context, local_path, entries);
+}
+
+PlatformFileError LocalFileSystemFileUtil::CreateDirectory(
+    FileSystemOperationContext* context,
+    const FilePath& file_path,
+    bool exclusive,
+    bool recursive) {
+  FilePath local_path =
+      GetLocalPath(context, context->src_origin_url(), context->src_type(),
+          file_path);
+  if (local_path.empty())
+    return base::PLATFORM_FILE_ERROR_INVALID_OPERATION;
+  return FileSystemFileUtil::GetInstance()->CreateDirectory(
+      context, local_path, exclusive, recursive);
+}
+
+PlatformFileError LocalFileSystemFileUtil::Copy(
+    FileSystemOperationContext* context,
+    const FilePath& src_file_path,
+    const FilePath& dest_file_path) {
+  // TODO(ericu): If they share a root URL, this could be optimized.
+  FilePath local_src_path =
+      GetLocalPath(context, context->src_origin_url(), context->src_type(),
+          src_file_path);
+  if (local_src_path.empty())
+    return base::PLATFORM_FILE_ERROR_INVALID_OPERATION;
+  FilePath local_dest_path =
+      GetLocalPath(context, context->dest_root_url(), context->dest_type(),

[kinuko, 2011/03/14 11:03:57]

Sometimes we call xxx_origin_url() but also call xxx_root_url() for other times.
From a quick glance I think we need to call xxx_origin_url() all the time don't
we?

[ericu, 2011/03/15 02:43:11]

You're right--this should be *_origin_url not *_root_url.  This version produces
the right outfit, but wastes cycles.

+          dest_file_path);
+  if (local_dest_path.empty())
+    return base::PLATFORM_FILE_ERROR_INVALID_OPERATION;
+  return FileSystemFileUtil::GetInstance()->Copy(
+      context, local_src_path, local_dest_path);
+}
+
+PlatformFileError LocalFileSystemFileUtil::Move(
+    FileSystemOperationContext* context,
+    const FilePath& src_file_path,
+    const FilePath& dest_file_path) {
+  // TODO(ericu): If they share a root URL, this could be optimized.
+  FilePath local_src_path =
+      GetLocalPath(context, context->src_root_url(), context->src_type(),

[kinuko, 2011/03/14 11:03:57]

src_origin_url()?

[ericu, 2011/03/15 02:43:11]

Done.

+          src_file_path);
+  if (local_src_path.empty())
+    return base::PLATFORM_FILE_ERROR_INVALID_OPERATION;
+  FilePath local_dest_path =
+      GetLocalPath(context, context->dest_root_url(), context->dest_type(),

[kinuko, 2011/03/14 11:03:57]

dest_origin_url()?

[ericu, 2011/03/15 02:43:11]

Done.

+          dest_file_path);
+  if (local_dest_path.empty())
+    return base::PLATFORM_FILE_ERROR_INVALID_OPERATION;
+  return FileSystemFileUtil::GetInstance()->Move(
+      context, local_src_path, local_dest_path);
+}
+
+PlatformFileError LocalFileSystemFileUtil::Delete(
+    FileSystemOperationContext* context,
+    const FilePath& file_path,
+    bool recursive) {
+  FilePath local_path =
+      GetLocalPath(context, context->src_origin_url(), context->src_type(),
+          file_path);
+  if (local_path.empty())
+    return base::PLATFORM_FILE_ERROR_INVALID_OPERATION;
+  return FileSystemFileUtil::GetInstance()->Delete(
+      context, local_path, recursive);
+}
+
+PlatformFileError LocalFileSystemFileUtil::Touch(
+    FileSystemOperationContext* context,
+    const FilePath& file_path,
+    const base::Time& last_access_time,
+    const base::Time& last_modified_time) {
+  FilePath local_path =
+      GetLocalPath(context, context->src_origin_url(), context->src_type(),
+          file_path);
+  if (local_path.empty())
+    return base::PLATFORM_FILE_ERROR_INVALID_OPERATION;
+  return FileSystemFileUtil::GetInstance()->Touch(
+      context, local_path, last_access_time, last_modified_time);
+}
+
+PlatformFileError LocalFileSystemFileUtil::Truncate(
+    FileSystemOperationContext* context,
+    const FilePath& file_path,
+    int64 length) {
+  FilePath local_path =
+      GetLocalPath(context, context->src_origin_url(), context->src_type(),
+          file_path);
+  if (local_path.empty())
+    return base::PLATFORM_FILE_ERROR_INVALID_OPERATION;
+  return FileSystemFileUtil::GetInstance()->Truncate(
+      context, local_path, length);
+}
+
+FilePath LocalFileSystemFileUtil::GetLocalPath(
+    FileSystemOperationContext* context,
+    const GURL& origin_url,
+    FileSystemType type,
+    const FilePath& virtual_path) {
+  // TODO(ericu): This is the way we allow the tests to use real paths.  Is it
+  // a security problem to do this?
+  if (!context->file_system_context()) {
+    return virtual_path;
+  }

[kinuko, 2011/03/14 11:03:57]

I think we want to minimize the code that performs this context check
(!context->file_system_context()) since it is clearly an exceptional case.
Can we DCHECK(context->file_system_context()) here and directly instantiate
FileSystemFileUtil in FileSystemOperation's constructor if raw filesystem access
is required (i.e. no context is given)?

(Also I think we'll want another TODO to have a  SimpleFileSystemContext and
SimpleMountPointProvider for SimpleFileSystem to let each of its operation go
through LocalFSFU)

[ericu, 2011/03/15 02:43:11]

I like that idea, but I've altered it slightly; those that call the
FileSystemOperation constructor can now force it to accept the
FileSystemFileUtil of their choice.  That way I can get rid of all this
dependence on context here.

However, we still need something in FileSystemOperation.cc inside
VerifyFileSystemPathForRead/Write.  How about I derive a new subclass of
FileSystemOperation, just for use in the tests, that overrides those two
methods?  That way all the test-only code is over in the tests.  If you like
that idea, I'd like to do it in a subsequent CL, to keep this from getting any
bigger.
Good point.  Let me run the layout tests and see if I broke anything there.
Ah, looks like I broke most of them.  I'll get on that, but am going to send
this revision for review in the mean time.

[kinuko, 2011/03/16 19:33:38]

sgtm.

+  FilePath root = GetRootPath(context, origin_url, type);
+  if (root.empty())
+    return root;

[kinuko, 2011/03/14 11:03:57]

If an intention is returning an empty path, return FilePath() might be easier to
follow.

[ericu, 2011/03/15 02:43:11]

Done.

+  return root.Append(virtual_path);
+}
+
+}  // namespace fileapi