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

chrome/browser/chromeos/file_manager/fileapi_util.cc

Index: chrome/browser/chromeos/file_manager/fileapi_util.cc
diff --git a/chrome/browser/chromeos/file_manager/fileapi_util.cc b/chrome/browser/chromeos/file_manager/fileapi_util.cc
index 4f7860ab6ea65e1cf440d4733f27e34b055ca4e8..dda0b089a1a45073878155dba5cdbd8f1ffc79ed 100644
--- a/chrome/browser/chromeos/file_manager/fileapi_util.cc
+++ b/chrome/browser/chromeos/file_manager/fileapi_util.cc
@@ -71,9 +71,10 @@ class FileDefinitionListConverter {
   // 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.
@@ -132,16 +133,6 @@ void FileDefinitionListConverter::ConvertNextIterator(
     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,
@@ -150,13 +141,8 @@ void FileDefinitionListConverter::ConvertNextIterator(
 
   // 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),
@@ -166,21 +152,32 @@ void FileDefinitionListConverter::ConvertNextIterator(
 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;