drive: Move FileSystem::LoadDirectoryIfNeeded to ChangeListLoader

chrome/browser/chromeos/drive/change_list_loader.cc

Index: chrome/browser/chromeos/drive/change_list_loader.cc
diff --git a/chrome/browser/chromeos/drive/change_list_loader.cc b/chrome/browser/chromeos/drive/change_list_loader.cc
index ae18ccd05a73851d32a16c68f72db5cbaa9b649d..40a2a3b3e26700da835635a0228ba5c2112f540a 100644
--- a/chrome/browser/chromeos/drive/change_list_loader.cc
+++ b/chrome/browser/chromeos/drive/change_list_loader.cc
@@ -360,24 +360,33 @@ void ChangeListLoader::CheckForUpdates(const FileOperationCallback& callback) {
   }
 }
 
-void ChangeListLoader::LoadIfNeeded(
-    const DirectoryFetchInfo& directory_fetch_info,
+void ChangeListLoader::LoadDirectoryIfNeeded(
+    const base::FilePath& directory_path,
     const FileOperationCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
 
-  // If the resource metadata has been already loaded, for normal change list
-  // fetch (= empty directory_fetch_info), we have nothing to do. For "fast
-  // fetch", we need to schedule a fetching if a refresh is currently
-  // running, because we don't want to wait a possibly large delta change
-  // list to arrive.
-  if (loaded_ && (directory_fetch_info.empty() || !IsRefreshing())) {
-    base::MessageLoopProxy::current()->PostTask(
-        FROM_HERE,
-        base::Bind(callback, FILE_ERROR_OK));
-    return;
-  }
-  Load(directory_fetch_info, callback);
+  ResourceEntry* entry = new ResourceEntry;
+  base::PostTaskAndReplyWithResult(
+      blocking_task_runner_.get(),
+      FROM_HERE,
+      base::Bind(&ResourceMetadata::GetResourceEntryByPath,
+                 base::Unretained(resource_metadata_),
+                 directory_path,
+                 entry),
+      base::Bind(&ChangeListLoader::LoadDirectoryIfNeededAfterGetEntry,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 directory_path,
+                 callback,
+                 true,  // should_try_loading_parent
+                 base::Owned(entry)));
+}
+
+void ChangeListLoader::LoadForTesting(const FileOperationCallback& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(!callback.is_null());
+
+  Load(DirectoryFetchInfo(), callback);
 }
 
 void ChangeListLoader::GetAboutResource(
@@ -398,6 +407,81 @@ void ChangeListLoader::GetAboutResource(
   }
 }
 
+void ChangeListLoader::LoadDirectoryIfNeededAfterGetEntry(
+    const base::FilePath& directory_path,
+    const FileOperationCallback& callback,
+    bool should_try_loading_parent,
+    const ResourceEntry* entry,
+    FileError error) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(!callback.is_null());
+
+  if (error == FILE_ERROR_NOT_FOUND && should_try_loading_parent) {
+    // This entry may be found after loading the parent.
+    LoadDirectoryIfNeeded(
+        directory_path.DirName(),

[kinaba, 2013/12/20 04:54:14]

This recursive call implicitly assume that it eventually escapes
FILE_ERROR_NOT_FOUND, in other words, the path is always an ancestor of /drive.

I think it always holds so there's no problem, but to be explicit, could you add
a check or a comment on that here?

[hashimoto, 2013/12/20 06:29:29]

Moved a check from FileSystem::GetResourceEntry.

+        base::Bind(&ChangeListLoader::LoadDirectoryIfNeededAfterLoadParent,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   directory_path,
+                   callback));
+    return;
+  }
+  if (error != FILE_ERROR_OK) {
+    callback.Run(error);
+    return;
+  }
+
+  if (!entry->file_info().is_directory()) {
+    callback.Run(FILE_ERROR_NOT_A_DIRECTORY);
+    return;
+  }
+
+  // drive/other does not exist on the server.
+  if (entry->local_id() == util::kDriveOtherDirLocalId) {
+    callback.Run(FILE_ERROR_OK);
+    return;
+  }
+
+  // If the resource metadata has been already loaded, we need to schedule a
+  // fetching if a refresh is currently running, because we don't want to wait a
+  // possibly large delta change list to arrive.
+  if (loaded_ && !IsRefreshing()) {

[kinaba, 2013/12/20 04:54:14]

Can we move this check to the very beginning of LoadDirectoryIfNeeded?
I think you put it here for not to change the existing code, but if after all it
returns without updating the resource metadata, the recursive access to DB
doesn't look quite meaningful to me.

[hashimoto, 2013/12/20 06:29:29]

Sounds good, but this results in LoadDirectoryIfNeeded returns FILE_ERROR_OK
even when the directory does not exist in the local metadata.
Changed user code to ignore result.

+    callback.Run(FILE_ERROR_OK);
+    return;
+  }
+  Load(DirectoryFetchInfo(entry->resource_id(),
+                          entry->directory_specific_info().changestamp()),
+       callback);
+}
+
+void ChangeListLoader::LoadDirectoryIfNeededAfterLoadParent(
+    const base::FilePath& directory_path,
+    const FileOperationCallback& callback,
+    FileError error) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(!callback.is_null());
+
+  if (error != FILE_ERROR_OK) {
+    callback.Run(error);
+    return;
+  }
+
+  ResourceEntry* entry = new ResourceEntry;
+  base::PostTaskAndReplyWithResult(
+      blocking_task_runner_.get(),
+      FROM_HERE,
+      base::Bind(&ResourceMetadata::GetResourceEntryByPath,
+                 base::Unretained(resource_metadata_),
+                 directory_path,
+                 entry),
+      base::Bind(&ChangeListLoader::LoadDirectoryIfNeededAfterGetEntry,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 directory_path,
+                 callback,
+                 false,  // should_try_loading_parent
+                 base::Owned(entry)));
+}
+
 void ChangeListLoader::Load(const DirectoryFetchInfo& directory_fetch_info,
                             const FileOperationCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));