drive: Move FileSystem::LoadDirectoryIfNeeded to ChangeListLoader

chrome/browser/chromeos/drive/change_list_loader.cc

 // Copyright (c) 2012 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/drive/change_list_loader.h"
 
 #include <set>
 
 #include "base/callback.h"
 #include "base/callback_helpers.h"
@@ skipping 342 matching lines @@
 
   if (loaded_) {
     // We only start to check for updates iff the load is done.
     // I.e., we ignore checking updates if not loaded to avoid starting the
     // load without user's explicit interaction (such as opening Drive).
     util::Log(logging::LOG_INFO, "Checking for updates");
     Load(DirectoryFetchInfo(), 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(
     const google_apis::AboutResourceCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
 
   if (cached_about_resource_) {
     base::MessageLoopProxy::current()->PostTask(
         FROM_HERE,
         base::Bind(
             callback,
             google_apis::HTTP_NO_CONTENT,
             base::Passed(scoped_ptr<google_apis::AboutResource>(
                 new google_apis::AboutResource(*cached_about_resource_)))));
   } else {
     UpdateAboutResource(callback);
   }
 }
 
+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));
   DCHECK(!callback.is_null());
 
   // Check if this is the first time this ChangeListLoader do loading.
   // Note: IsRefreshing() depends on pending_load_callback_ so check in advance.
   const bool is_initial_load = (!loaded_ && !IsRefreshing());
 
   // Register the callback function to be called when it is loaded.
@@ skipping 425 matching lines @@
 
     cached_about_resource_.reset(
         new google_apis::AboutResource(*about_resource));
   }
 
   callback.Run(status, about_resource.Pass());
 }
 
 }  // namespace internal
 }  // namespace drive