drive: Enable recursive fast-fetch.

- LoadDirectoryIfNeeded calls GetResourceEntryByPath for directory metadata.
- GetResourceEntryByPath calls LoadDirectoryIfNeeded for its parent if needed.

The cycle enables to fetch deep entries at any time without waiting for
the full feed nor user to dig down the hierarchy from the root.

Along the way, I removed the handling of  ReadDirectoryByPath("drive/other")
that waited the full feed to arrive. By removing it, all FileSystem
operations are now clearly ensured not blocked by full feed fetching.
Listing up "drive/other" is meaningless; Files.app never does that.

BUG=285614

Review URL: https://chromiumcodereview.appspot.com/23441049

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@221712 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chromeos/drive/change_list_loader_unittest.cc

@@ -69,35 +69,6 @@ class TestChangeListLoaderObserver : public ChangeListLoaderObserver {
   DISALLOW_COPY_AND_ASSIGN(TestChangeListLoaderObserver);
 };
 
-class TestDriveService : public FakeDriveService {
- public:
-  TestDriveService() : never_return_all_resource_list_(false),
-                       blocked_call_count_(0) {}
-
-  void set_never_return_all_resource_list(bool value) {
-    never_return_all_resource_list_ = value;
-  }
-
-  int blocked_call_count() const { return blocked_call_count_; }
-
-  // FakeDriveService override.
-  virtual google_apis::CancelCallback GetAllResourceList(
-      const google_apis::GetResourceListCallback& callback) OVERRIDE {
-    if (never_return_all_resource_list_) {
-      ++blocked_call_count_;
-      return google_apis::CancelCallback();
-    }
-    return FakeDriveService::GetAllResourceList(callback);
-  }
-
- private:
-  // GetAllResourceList never returns result when this is set to true.
-  // Used to emulate the real server's slowness.
-  bool never_return_all_resource_list_;
-
-  int blocked_call_count_;  // Number of blocked method calls.
-};
-
 class ChangeListLoaderTest : public testing::Test {
  protected:
   virtual void SetUp() OVERRIDE {
@@ -105,7 +76,7 @@ class ChangeListLoaderTest : public testing::Test {
     pref_service_.reset(new TestingPrefServiceSimple);
     test_util::RegisterDrivePrefs(pref_service_->registry());
 
-    drive_service_.reset(new TestDriveService);
+    drive_service_.reset(new FakeDriveService);
     ASSERT_TRUE(drive_service_->LoadResourceListForWapi(
         "gdata/root_feed.json"));
     ASSERT_TRUE(drive_service_->LoadAccountMetadataForWapi(
@@ -154,7 +125,7 @@ class ChangeListLoaderTest : public testing::Test {
   content::TestBrowserThreadBundle thread_bundle_;
   base::ScopedTempDir temp_dir_;
   scoped_ptr<TestingPrefServiceSimple> pref_service_;
-  scoped_ptr<TestDriveService> drive_service_;
+  scoped_ptr<FakeDriveService> drive_service_;
   scoped_ptr<JobScheduler> scheduler_;
   scoped_ptr<ResourceMetadataStorage,
              test_util::DestroyHelperForTests> metadata_storage_;
@@ -276,7 +247,7 @@ TEST_F(ChangeListLoaderTest, LoadIfNeeded_MyDrive) {
   observer.clear_changed_directories();
 
   // GetAllResourceList() was called.
-  EXPECT_EQ(1, drive_service_->blocked_call_count());
+  EXPECT_EQ(1, drive_service_->blocked_resource_list_load_count());
 
   // My Drive is present in the local metadata, but its child is not.
   ResourceEntry entry;

chrome/browser/chromeos/drive/file_system.cc

@@ -57,8 +57,7 @@ FileError GetLocallyStoredResourceEntry(
     const base::FilePath& file_path,
     ResourceEntry* entry) {
   std::string local_id;
-  FileError error =
-      resource_metadata->GetIdByPath(file_path, &local_id);
+  FileError error = resource_metadata->GetIdByPath(file_path, &local_id);
   if (error != FILE_ERROR_OK)
     return error;
 
@@ -647,19 +646,26 @@ void FileSystem::GetResourceEntryByPathAfterGetEntry(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
 
-  if (error == FILE_ERROR_OK) {
-    callback.Run(error, entry.Pass());
-    return;
+  if (error == FILE_ERROR_NOT_FOUND) {
+    // If the information about the path is not in the local ResourceMetadata,
+    // try fetching information of the directory and retry.
+    //
+    // Note: this forms mutual recursion between GetResourceEntryByPath and
+    // LoadDirectoryIfNeeded, because directory loading requires the existence
+    // of directory entry itself. The recursion terminates because we always go
+    // up the hierarchy by .DirName() bounded under the Drive root path.
+    if (util::GetDriveGrandRootPath().IsParent(file_path)) {
+      LoadDirectoryIfNeeded(
+          file_path.DirName(),
+          base::Bind(&FileSystem::GetResourceEntryByPathAfterLoad,
+                     weak_ptr_factory_.GetWeakPtr(),
+                     file_path,
+                     callback));
+      return;
+    }
   }
 
-  // If the information about the path is not in the local ResourceMetadata,
-  // try fetching information of the directory and retry.
-  LoadDirectoryIfNeeded(
-      file_path.DirName(),
-      base::Bind(&FileSystem::GetResourceEntryByPathAfterLoad,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 file_path,
-                 callback));
+  callback.Run(error, entry.Pass());
 }
 
 void FileSystem::GetResourceEntryByPathAfterLoad(
@@ -708,9 +714,7 @@ void FileSystem::LoadDirectoryIfNeeded(const base::FilePath& directory_path,
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
 
-  // ResourceMetadata may know about the entry even if the resource
-  // metadata is not yet fully loaded.
-  resource_metadata_->GetResourceEntryByPathOnUIThread(
+  GetResourceEntryByPath(
       directory_path,
       base::Bind(&FileSystem::LoadDirectoryIfNeededAfterGetEntry,
                  weak_ptr_factory_.GetWeakPtr(),
@@ -726,12 +730,8 @@ void FileSystem::LoadDirectoryIfNeededAfterGetEntry(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
 
-  if (error != FILE_ERROR_OK ||
-      entry->resource_id() == util::kDriveOtherDirSpecialResourceId) {
-    // If we don't know about the directory, or it is the "drive/other"
-    // directory that has to gather all orphan entries, start loading full
-    // resource list.
-    change_list_loader_->LoadIfNeeded(internal::DirectoryFetchInfo(), callback);
+  if (error != FILE_ERROR_OK) {
+    callback.Run(error);
     return;
   }
 

chrome/browser/chromeos/drive/file_system_unittest.cc

@@ -364,13 +364,23 @@ TEST_F(FileSystemTest, GetMyDriveRoot) {
 }
 
 TEST_F(FileSystemTest, GetExistingFile) {
-  const base::FilePath kFilePath(FILE_PATH_LITERAL("drive/root/File 1.txt"));
+  // Simulate the situation that full feed fetching takes very long time,
+  // to test the recursive "fast fetch" feature is properly working.
+  fake_drive_service_->set_never_return_all_resource_list(true);
+
+  const base::FilePath kFilePath(
+      FILE_PATH_LITERAL("drive/root/Directory 1/SubDirectory File 1.txt"));
   scoped_ptr<ResourceEntry> entry = GetResourceEntryByPathSync(kFilePath);
   ASSERT_TRUE(entry);
-  EXPECT_EQ("file:2_file_resource_id", entry->resource_id());
+  EXPECT_EQ("file:subdirectory_file_1_id", entry->resource_id());
 
-  EXPECT_EQ(1, fake_drive_service_->about_resource_load_count());
-  EXPECT_EQ(1, fake_drive_service_->resource_list_load_count());
+  // One server changestamp check (about_resource), three directory load for
+  // "drive", "drive/root", and "drive/root/Directory 1", and one background
+  // full resource list loading. Note that the directory load for "drive" is
+  // special and resorts to about_resource.
+  EXPECT_EQ(2, fake_drive_service_->about_resource_load_count());
+  EXPECT_EQ(2, fake_drive_service_->directory_load_count());
+  EXPECT_EQ(1, fake_drive_service_->blocked_resource_list_load_count());
 }
 
 TEST_F(FileSystemTest, GetExistingDocument) {
@@ -409,6 +419,9 @@ TEST_F(FileSystemTest, GetInSubSubdir) {
 }
 
 TEST_F(FileSystemTest, GetOrphanFile) {
+  ASSERT_TRUE(LoadFullResourceList());
+
+  // Entry without parents are placed under "drive/other".
   const base::FilePath kFilePath(
       FILE_PATH_LITERAL("drive/other/Orphan File 1.txt"));
   scoped_ptr<ResourceEntry> entry = GetResourceEntryByPathSync(kFilePath);

chrome/browser/drive/fake_drive_service.cc

@@ -176,7 +176,9 @@ FakeDriveService::FakeDriveService()
       directory_load_count_(0),
       about_resource_load_count_(0),
       app_list_load_count_(0),
-      offline_(false) {
+      blocked_resource_list_load_count_(0),
+      offline_(false),
+      never_return_all_resource_list_(false) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
@@ -328,6 +330,11 @@ CancelCallback FakeDriveService::GetAllResourceList(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
 
+  if (never_return_all_resource_list_) {
+    ++blocked_resource_list_load_count_;
+    return CancelCallback();
+  }
+
   GetResourceListInternal(0,  // start changestamp
                           std::string(),  // empty search query
                           std::string(),  // no directory resource id,

chrome/browser/drive/fake_drive_service.h

@@ -40,6 +40,12 @@ class FakeDriveService : public DriveServiceInterface {
   // when offline. By default the offline state is false.
   void set_offline(bool offline) { offline_ = offline; }
 
+  // GetAllResourceList never returns result when this is set to true.
+  // Used to emulate the real server's slowness.
+  void set_never_return_all_resource_list(bool value) {
+    never_return_all_resource_list_ = value;
+  }
+
   // Changes the default max results returned from GetResourceList().
   // By default, it's set to 0, which is unlimited.
   void set_default_max_results(int default_max_results) {
@@ -81,6 +87,12 @@ class FakeDriveService : public DriveServiceInterface {
   // GetAppList().
   int app_list_load_count() const { return app_list_load_count_; }
 
+  // Returns the number of times GetAllResourceList are blocked due to
+  // set_never_return_all_resource_list().
+  int blocked_resource_list_load_count() const {
+    return blocked_resource_list_load_count_;
+  }
+
   // Returns the file path whose request is cancelled just before this method
   // invocation.
   const base::FilePath& last_cancelled_file() const {
@@ -300,7 +312,9 @@ class FakeDriveService : public DriveServiceInterface {
   int directory_load_count_;
   int about_resource_load_count_;
   int app_list_load_count_;
+  int blocked_resource_list_load_count_;
   bool offline_;
+  bool never_return_all_resource_list_;
   base::FilePath last_cancelled_file_;
   GURL share_url_base_;