Remove thread restriction of fileapi::FileSystemContext::ResolveURL.

chrome/browser/chromeos/file_manager/fileapi_util.cc

 // Copyright 2013 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 "chrome/browser/chromeos/file_manager/fileapi_util.h"
 
 #include "base/files/file.h"
 #include "base/files/file_path.h"
 #include "chrome/browser/chromeos/drive/file_system_util.h"
 #include "chrome/browser/extensions/extension_util.h"
@@ skipping 53 matching lines @@
   // Converts the element under the iterator to an entry. First, converts
   // the virtual path to an URL, and calls OnResolvedURL(). In case of error
   // calls OnIteratorConverted with an error entry definition.
   void ConvertNextIterator(scoped_ptr<FileDefinitionListConverter> self_deleter,
                            FileDefinitionList::const_iterator iterator);
 
   // Creates an entry definition from the URL as well as the file definition.
   // Then, calls OnIteratorConverted with the created entry definition.
   void OnResolvedURL(scoped_ptr<FileDefinitionListConverter> self_deleter,
                      FileDefinitionList::const_iterator iterator,
-                     const GURL& root_url,
-                     const std::string& name,
-                     base::File::Error error);
+                     base::File::Error error,
+                     const fileapi::FileSystemInfo& info,
+                     const base::FilePath& file_path,
+                     fileapi::FileSystemContext::ResolvedEntryType type);
 
   // Called when the iterator is converted. Adds the |entry_definition| to
   // |results_| and calls ConvertNextIterator() for the next element.
   void OnIteratorConverted(scoped_ptr<FileDefinitionListConverter> self_deleter,
                            FileDefinitionList::const_iterator iterator,
                            const EntryDefinition& entry_definition);
 
   scoped_refptr<fileapi::FileSystemContext> file_system_context_;
   const std::string extension_id_;
   const FileDefinitionList file_definition_list_;
@@ skipping 38 matching lines @@
   }
 
   if (!file_system_context_.get()) {
     OnIteratorConverted(self_deleter.Pass(),
                         iterator,
                         CreateEntryDefinitionWithError(
                             base::File::FILE_ERROR_INVALID_OPERATION));
     return;
   }
 
-  fileapi::ExternalFileSystemBackend* backend =
-      file_system_context_->external_backend();
-  if (!backend) {
-    OnIteratorConverted(self_deleter.Pass(),
-                        iterator,
-                        CreateEntryDefinitionWithError(
-                            base::File::FILE_ERROR_INVALID_OPERATION));
-    return;
-  }
-
   fileapi::FileSystemURL url = file_system_context_->CreateCrackedFileSystemURL(
       extensions::Extension::GetBaseURLFromExtensionId(extension_id_),
       fileapi::kFileSystemTypeExternal,
       iterator->virtual_path);
   DCHECK(url.is_valid());
 
   // The converter object will be deleted if the callback is not called because
   // of shutdown during ResolveURL().
-  //
-  // TODO(mtomasz, nhiroki): Call FileSystemContext::ResolveURL() directly,
-  // after removing redundant thread restrictions in there.
-  backend->ResolveURL(
+  file_system_context_->ResolveURL(
       url,
-      // Not sandboxed file systems are not creatable via ResolveURL().
-      fileapi::OPEN_FILE_SYSTEM_FAIL_IF_NONEXISTENT,
       base::Bind(&FileDefinitionListConverter::OnResolvedURL,
                  base::Unretained(this),
                  base::Passed(&self_deleter),
                  iterator));
 }
 
 void FileDefinitionListConverter::OnResolvedURL(
     scoped_ptr<FileDefinitionListConverter> self_deleter,
     FileDefinitionList::const_iterator iterator,
-    const GURL& root_url,
-    const std::string& name,
-    base::File::Error error) {
+    base::File::Error error,
+    const fileapi::FileSystemInfo& info,
+    const base::FilePath& file_path,
+    fileapi::FileSystemContext::ResolvedEntryType type) {

[mtomasz, 2014/04/21 05:03:05]

I'm wondering if introducing a new argument is necessary. Why not just using
base::File::FILE_ERROR_NOT_FOUND instead, and removing the #161 if?

[kinaba, 2014/04/21 05:19:09]

I think "ERROR but output value fields (like |entry_definition|) filled" state
is less common in Chromium and thus confusing. Even worse, FILE_ERROR_NOT_FOUND
may be returned either from FileSystemBackend or GetMetadata, and the latter is
the only case I want to treat as success.

[mtomasz, 2014/04/21 05:48:33]

My concern was that, if we returned an error, then the implementation of
FileSystemContext::ResolveURL() would be much easier. It would just return error
in case of an error. With this patch, it is quite complex. If the file doesn't
exist, then it will return FILE_OK, but with RESOLVED_ENTRY_NOT_FOUND, with is
dangerously similar to FILE_ERROR_NOT_FOUND.

If we just returned File::FILE_ERROR_NOT_FOUND, then we could move the tricky
logic (treating not found as success) to this converter class, and hence keeping
FileSystemContext simple.

But I'm not very familiar with the FileSystemContext class, so if you prefer the
current solution, feel free to go ahead. I don't have hard feelings here.

[kinaba, 2014/04/21 07:59:25]

My concern is that the suggested plan makes the API of FileSystemContext
complex (i.e., when the callback parameters are available or not?
previously they're available iff error code indicates success, but now not.)

[mtomasz, 2014/04/21 08:04:07]

Ah, now I understand your point of view. Yep, I think both solutions are not
perfect. Let see what's @kinuko's opinion on this.

   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  if (error != base::File::FILE_OK) {
+    OnIteratorConverted(self_deleter.Pass(),
+                        iterator,
+                        CreateEntryDefinitionWithError(error));
+    return;
+  }
+
   EntryDefinition entry_definition;
-  entry_definition.file_system_root_url = root_url.spec();
-  entry_definition.file_system_name = name;
+  entry_definition.file_system_root_url = info.root_url.spec();
+  entry_definition.file_system_name = info.name;
+  DCHECK(type == fileapi::FileSystemContext::RESOLVED_ENTRY_NOT_FOUND ||
+         iterator->is_directory ==
+             (type == fileapi::FileSystemContext::RESOLVED_ENTRY_DIRECTORY));
   entry_definition.is_directory = iterator->is_directory;
   entry_definition.error = error;

[mtomasz, 2014/04/21 05:50:02]

nit: This is always FILE_OK here, so how about inlining FILE_OK? 

[kinaba, 2014/04/21 07:59:25]

Done.

 
   // Construct a target Entry.fullPath value from the virtual path and the
   // root URL. Eg. Downloads/A/b.txt -> A/b.txt.
   const base::FilePath root_virtual_path =
-      file_system_context_->CrackURL(root_url).virtual_path();
+      file_system_context_->CrackURL(info.root_url).virtual_path();
   DCHECK(root_virtual_path == iterator->virtual_path ||
          root_virtual_path.IsParent(iterator->virtual_path));
   base::FilePath full_path;
   root_virtual_path.AppendRelativePath(iterator->virtual_path, &full_path);
   entry_definition.full_path = full_path;
 
   OnIteratorConverted(self_deleter.Pass(), iterator, entry_definition);
 }
 
 void FileDefinitionListConverter::OnIteratorConverted(
@@ skipping 125 matching lines @@
   file_definition_list.push_back(file_definition);
   ConvertFileDefinitionListToEntryDefinitionList(
       profile,
       extension_id,
       file_definition_list,
       base::Bind(&OnConvertFileDefinitionDone, callback));
 }
 
 }  // namespace util
 }  // namespace file_manager