Merge 263610 "drive: Stop reading child entries in ReadDirectory..."

> drive: Stop reading child entries in ReadDirectory() when not needed
> 
> We should avoid calling ResourceMetadata::ReadDirectoryById when possible because it's costly with large directories.
> 
> Change ReadDirectory() to have two callbacks, |entries_callback| and |completion_callback|.
> When |entries_callback| is null, do not call ReadDirectoryById.
> Stop calling ReadDirectoryById in CheckLocalState()
> 
> BUG=362087
> TEST=unit_tests
> 
> Review URL: https://codereview.chromium.org/234793003

TBR=hashimoto@chromium.org

Review URL: https://codereview.chromium.org/270593007

git-svn-id: svn://svn.chromium.org/chrome/branches/1916/src@269954 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 97 insertions, 109 deletions

diff --git a/chrome/browser/chromeos/drive/directory_loader.cc b/chrome/browser/chromeos/drive/directory_loader.cc
index 0b4ab8c..4c4f5e7 100644
--- a/chrome/browser/chromeos/drive/directory_loader.cc
+++ b/chrome/browser/chromeos/drive/directory_loader.cc
@@ -36,7 +36,6 @@ FileError CheckLocalState(ResourceMetadata* resource_metadata,
                           const google_apis::AboutResource& about_resource,
                           const std::string& local_id,
                           ResourceEntry* entry,
-                          ResourceEntryVector* child_entries,
                           int64* local_changestamp) {
   // Fill My Drive resource ID.
   ResourceEntry mydrive;
@@ -57,11 +56,6 @@ FileError CheckLocalState(ResourceMetadata* resource_metadata,
   if (error != FILE_ERROR_OK)
     return error;
 
-  // Get child entries.
-  error = resource_metadata->ReadDirectoryById(local_id, child_entries);
-  if (error != FILE_ERROR_OK)
-    return error;
-
   // Get the local changestamp.
   *local_changestamp = resource_metadata->GetLargestChangestamp();
   return FILE_ERROR_OK;
@@ -95,7 +89,8 @@ FileError UpdateChangestamp(ResourceMetadata* resource_metadata,
 }  // namespace
 
 struct DirectoryLoader::ReadDirectoryCallbackState {
-  ReadDirectoryCallback callback;
+  ReadDirectoryEntriesCallback entries_callback;
+  FileOperationCallback completion_callback;
   std::set<std::string> sent_entry_names;
 };
 
@@ -192,8 +187,7 @@ class DirectoryLoader::FeedFetcher {
       return;
     }
 
-    loader_->SendEntries(directory_fetch_info_.local_id(), *refreshed_entries,
-                         true /*has_more*/);
+    loader_->SendEntries(directory_fetch_info_.local_id(), *refreshed_entries);
 
     if (!next_url.is_empty()) {
       // There is the remaining result so fetch it.
@@ -276,10 +270,12 @@ void DirectoryLoader::RemoveObserver(ChangeListLoaderObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
-void DirectoryLoader::ReadDirectory(const base::FilePath& directory_path,
-                                    const ReadDirectoryCallback& callback) {
+void DirectoryLoader::ReadDirectory(
+    const base::FilePath& directory_path,
+    const ReadDirectoryEntriesCallback& entries_callback,
+    const FileOperationCallback& completion_callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(!callback.is_null());
+  DCHECK(!completion_callback.is_null());
 
   ResourceEntry* entry = new ResourceEntry;
   base::PostTaskAndReplyWithResult(
@@ -292,39 +288,42 @@ void DirectoryLoader::ReadDirectory(const base::FilePath& directory_path,
       base::Bind(&DirectoryLoader::ReadDirectoryAfterGetEntry,
                  weak_ptr_factory_.GetWeakPtr(),
                  directory_path,
-                 callback,
+                 entries_callback,
+                 completion_callback,
                  true,  // should_try_loading_parent
                  base::Owned(entry)));
 }
 
 void DirectoryLoader::ReadDirectoryAfterGetEntry(
     const base::FilePath& directory_path,
-    const ReadDirectoryCallback& callback,
+    const ReadDirectoryEntriesCallback& entries_callback,
+    const FileOperationCallback& completion_callback,
     bool should_try_loading_parent,
     const ResourceEntry* entry,
     FileError error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(!callback.is_null());
+  DCHECK(!completion_callback.is_null());
 
   if (error == FILE_ERROR_NOT_FOUND &&
       should_try_loading_parent &&
       util::GetDriveGrandRootPath().IsParent(directory_path)) {
     // This entry may be found after loading the parent.
     ReadDirectory(directory_path.DirName(),
+                  ReadDirectoryEntriesCallback(),
                   base::Bind(&DirectoryLoader::ReadDirectoryAfterLoadParent,
                              weak_ptr_factory_.GetWeakPtr(),
                              directory_path,
-                             callback));
+                             entries_callback,
+                             completion_callback));
     return;
   }
   if (error != FILE_ERROR_OK) {
-    callback.Run(error, scoped_ptr<ResourceEntryVector>(), false /*has_more*/);
+    completion_callback.Run(error);
     return;
   }
 
   if (!entry->file_info().is_directory()) {
-    callback.Run(FILE_ERROR_NOT_A_DIRECTORY,
-                 scoped_ptr<ResourceEntryVector>(), false /*has_more*/);
+    completion_callback.Run(FILE_ERROR_NOT_A_DIRECTORY);
     return;
   }
 
@@ -336,7 +335,8 @@ void DirectoryLoader::ReadDirectoryAfterGetEntry(
   // Register the callback function to be called when it is loaded.
   const std::string& local_id = directory_fetch_info.local_id();
   ReadDirectoryCallbackState callback_state;
-  callback_state.callback = callback;
+  callback_state.entries_callback = entries_callback;
+  callback_state.completion_callback = completion_callback;
   pending_load_callback_[local_id].push_back(callback_state);
 
   // If loading task for |local_id| is already running, do nothing.
@@ -357,18 +357,14 @@ void DirectoryLoader::ReadDirectoryAfterGetEntry(
 
 void DirectoryLoader::ReadDirectoryAfterLoadParent(
     const base::FilePath& directory_path,
-    const ReadDirectoryCallback& callback,
-    FileError error,
-    scoped_ptr<ResourceEntryVector> entries,
-    bool has_more) {
+    const ReadDirectoryEntriesCallback& entries_callback,
+    const FileOperationCallback& completion_callback,
+    FileError error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(!callback.is_null());
-
-  if (has_more)
-    return;
+  DCHECK(!completion_callback.is_null());
 
   if (error != FILE_ERROR_OK) {
-    callback.Run(error, scoped_ptr<ResourceEntryVector>(), false /*has_more*/);
+    completion_callback.Run(error);
     return;
   }
 
@@ -383,7 +379,8 @@ void DirectoryLoader::ReadDirectoryAfterLoadParent(
       base::Bind(&DirectoryLoader::ReadDirectoryAfterGetEntry,
                  weak_ptr_factory_.GetWeakPtr(),
                  directory_path,
-                 callback,
+                 entries_callback,
+                 completion_callback,
                  false,  // should_try_loading_parent
                  base::Owned(entry)));
 }
@@ -405,7 +402,6 @@ void DirectoryLoader::ReadDirectoryAfterGetAboutResource(
   // Check the current status of local metadata, and start loading if needed.
   google_apis::AboutResource* about_resource_ptr = about_resource.get();
   ResourceEntry* entry = new ResourceEntry;
-  ResourceEntryVector* child_entries = new ResourceEntryVector;
   int64* local_changestamp = new int64;
   base::PostTaskAndReplyWithResult(
       blocking_task_runner_,
@@ -415,14 +411,12 @@ void DirectoryLoader::ReadDirectoryAfterGetAboutResource(
                  *about_resource_ptr,
                  local_id,
                  entry,
-                 child_entries,
                  local_changestamp),
       base::Bind(&DirectoryLoader::ReadDirectoryAfterCheckLocalState,
                  weak_ptr_factory_.GetWeakPtr(),
                  base::Passed(&about_resource),
                  local_id,
                  base::Owned(entry),
-                 base::Owned(child_entries),
                  base::Owned(local_changestamp)));
 }
 
@@ -430,7 +424,6 @@ void DirectoryLoader::ReadDirectoryAfterCheckLocalState(
     scoped_ptr<google_apis::AboutResource> about_resource,
     const std::string& local_id,
     const ResourceEntry* entry,
-    const ResourceEntryVector* child_entries,
     const int64* local_changestamp,
     FileError error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -469,8 +462,6 @@ void DirectoryLoader::ReadDirectoryAfterCheckLocalState(
   if (directory_changestamp + kMinimumChangestampGap > remote_changestamp) {
     OnDirectoryLoadComplete(local_id, FILE_ERROR_OK);
   } else {
-    // Send locally found entries to callbacks.
-    SendEntries(local_id, *child_entries, true /*has_more*/);
     // Start fetching the directory content, and mark it with the changestamp
     // |remote_changestamp|.
     LoadDirectoryFromServer(directory_fetch_info);
@@ -486,6 +477,23 @@ void DirectoryLoader::OnDirectoryLoadComplete(const std::string& local_id,
                local_id.c_str(),
                FileErrorToString(error).c_str());
 
+  LoadCallbackMap::iterator it = pending_load_callback_.find(local_id);
+  if (it == pending_load_callback_.end())
+    return;
+
+  // No need to read metadata when no one needs entries.
+  bool needs_to_send_entries = false;
+  for (size_t i = 0; i < it->second.size(); ++i) {
+    const ReadDirectoryCallbackState& callback_state = it->second[i];
+    if (!callback_state.entries_callback.is_null())
+      needs_to_send_entries = true;
+  }
+
+  if (!needs_to_send_entries) {
+    OnDirectoryLoadCompleteAfterRead(local_id, NULL, FILE_ERROR_OK);
+    return;
+  }
+
   ResourceEntryVector* entries = new ResourceEntryVector;
   base::PostTaskAndReplyWithResult(
       blocking_task_runner_.get(),
@@ -506,28 +514,26 @@ void DirectoryLoader::OnDirectoryLoadCompleteAfterRead(
   if (it != pending_load_callback_.end()) {
     DVLOG(1) << "Running callback for " << local_id;
 
-    const bool kHasMore = false;
-    if (error == FILE_ERROR_OK) {
-      SendEntries(local_id, *entries, kHasMore);
-    } else {
-      for (size_t i = 0; i < it->second.size(); ++i) {
-        const ReadDirectoryCallbackState& callback_state = it->second[i];
-        callback_state.callback.Run(error, scoped_ptr<ResourceEntryVector>(),
-                                    kHasMore);
-      }
+    if (error == FILE_ERROR_OK && entries)
+      SendEntries(local_id, *entries);
+
+    for (size_t i = 0; i < it->second.size(); ++i) {
+      const ReadDirectoryCallbackState& callback_state = it->second[i];
+      callback_state.completion_callback.Run(error);
     }
     pending_load_callback_.erase(it);
   }
 }
 
 void DirectoryLoader::SendEntries(const std::string& local_id,
-                                  const ResourceEntryVector& entries,
-                                  bool has_more) {
+                                  const ResourceEntryVector& entries) {
   LoadCallbackMap::iterator it = pending_load_callback_.find(local_id);
   DCHECK(it != pending_load_callback_.end());
 
   for (size_t i = 0; i < it->second.size(); ++i) {
     ReadDirectoryCallbackState* callback_state = &it->second[i];
+    if (callback_state->entries_callback.is_null())
+      continue;
 
     // Filter out entries which were already sent.
     scoped_ptr<ResourceEntryVector> entries_to_send(new ResourceEntryVector);
@@ -538,8 +544,7 @@ void DirectoryLoader::SendEntries(const std::string& local_id,
         entries_to_send->push_back(entry);
       }
     }
-    callback_state->callback.Run(FILE_ERROR_OK, entries_to_send.Pass(),
-                                 has_more);
+    callback_state->entries_callback.Run(entries_to_send.Pass());
   }
 }
 
diff --git a/chrome/browser/chromeos/drive/directory_loader_unittest.cc b/chrome/browser/chromeos/drive/directory_loader_unittest.cc
index bb93d4b..4d6a07e 100644
--- a/chrome/browser/chromeos/drive/directory_loader_unittest.cc
+++ b/chrome/browser/chromeos/drive/directory_loader_unittest.cc
@@ -57,21 +57,10 @@ class TestDirectoryLoaderObserver : public ChangeListLoaderObserver {
   DISALLOW_COPY_AND_ASSIGN(TestDirectoryLoaderObserver);
 };
 
-void AccumulateReadDirectoryResult(FileError* out_error,
-                                   ResourceEntryVector* out_entries,
-                                   bool* last_has_more,
-                                   FileError error,
-                                   scoped_ptr<ResourceEntryVector> entries,
-                                   bool has_more) {
-  EXPECT_TRUE(*last_has_more);
-  *out_error = error;
-  *last_has_more = has_more;
-  if (error == FILE_ERROR_OK) {
-    ASSERT_TRUE(entries);
-    out_entries->insert(out_entries->end(), entries->begin(), entries->end());
-  } else {
-    EXPECT_FALSE(has_more);
-  }
+void AccumulateReadDirectoryResult(ResourceEntryVector* out_entries,
+                                   scoped_ptr<ResourceEntryVector> entries) {
+  ASSERT_TRUE(entries);
+  out_entries->insert(out_entries->end(), entries->begin(), entries->end());
 }
 
 }  // namespace
@@ -158,14 +147,12 @@ TEST_F(DirectoryLoaderTest, ReadDirectory_GrandRoot) {
   // Load grand root.
   FileError error = FILE_ERROR_FAILED;
   ResourceEntryVector entries;
-  bool last_has_more = true;
   directory_loader_->ReadDirectory(
       util::GetDriveGrandRootPath(),
-      base::Bind(&AccumulateReadDirectoryResult,
-                 &error, &entries, &last_has_more));
+      base::Bind(&AccumulateReadDirectoryResult, &entries),
+      google_apis::test_util::CreateCopyResultCallback(&error));
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(FILE_ERROR_OK, error);
-  EXPECT_FALSE(last_has_more);
   EXPECT_EQ(0U, observer.changed_directories().size());
   observer.clear_changed_directories();
 
@@ -190,14 +177,12 @@ TEST_F(DirectoryLoaderTest, ReadDirectory_MyDrive) {
   // Load My Drive.
   FileError error = FILE_ERROR_FAILED;
   ResourceEntryVector entries;
-  bool last_has_more = true;
   directory_loader_->ReadDirectory(
       util::GetDriveMyDriveRootPath(),
-      base::Bind(&AccumulateReadDirectoryResult,
-                 &error, &entries, &last_has_more));
+      base::Bind(&AccumulateReadDirectoryResult, &entries),
+      google_apis::test_util::CreateCopyResultCallback(&error));
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(FILE_ERROR_OK, error);
-  EXPECT_FALSE(last_has_more);
   EXPECT_EQ(1U, observer.changed_directories().count(
       util::GetDriveMyDriveRootPath()));
 
@@ -222,27 +207,23 @@ TEST_F(DirectoryLoaderTest, ReadDirectory_MultipleCalls) {
   // Load grand root.
   FileError error = FILE_ERROR_FAILED;
   ResourceEntryVector entries;
-  bool last_has_more = true;
   directory_loader_->ReadDirectory(
       util::GetDriveGrandRootPath(),
-      base::Bind(&AccumulateReadDirectoryResult,
-                 &error, &entries, &last_has_more));
+      base::Bind(&AccumulateReadDirectoryResult, &entries),
+      google_apis::test_util::CreateCopyResultCallback(&error));
 
   // Load grand root again without waiting for the result.
   FileError error2 = FILE_ERROR_FAILED;
   ResourceEntryVector entries2;
-  bool last_has_more2 = true;
   directory_loader_->ReadDirectory(
       util::GetDriveGrandRootPath(),
-      base::Bind(&AccumulateReadDirectoryResult,
-                 &error2, &entries2, &last_has_more2));
+      base::Bind(&AccumulateReadDirectoryResult, &entries2),
+      google_apis::test_util::CreateCopyResultCallback(&error2));
   base::RunLoop().RunUntilIdle();
 
   // Callback is called for each method call.
   EXPECT_EQ(FILE_ERROR_OK, error);
-  EXPECT_FALSE(last_has_more);
   EXPECT_EQ(FILE_ERROR_OK, error2);
-  EXPECT_FALSE(last_has_more2);
 }
 
 TEST_F(DirectoryLoaderTest, Lock) {
@@ -253,11 +234,10 @@ TEST_F(DirectoryLoaderTest, Lock) {
   TestDirectoryLoaderObserver observer(directory_loader_.get());
   FileError error = FILE_ERROR_FAILED;
   ResourceEntryVector entries;
-  bool last_has_more = true;
   directory_loader_->ReadDirectory(
       util::GetDriveMyDriveRootPath(),
-      base::Bind(&AccumulateReadDirectoryResult,
-                 &error, &entries, &last_has_more));
+      base::Bind(&AccumulateReadDirectoryResult, &entries),
+      google_apis::test_util::CreateCopyResultCallback(&error));
   base::RunLoop().RunUntilIdle();
 
   // Update is pending due to the lock.
diff --git a/chrome/browser/chromeos/drive/dummy_file_system.h b/chrome/browser/chromeos/drive/dummy_file_system.h
index 1147f8c..2590ce9 100644
--- a/chrome/browser/chromeos/drive/dummy_file_system.h
+++ b/chrome/browser/chromeos/drive/dummy_file_system.h
@@ -66,8 +66,10 @@ class DummyFileSystem : public FileSystemInterface {
   virtual void GetResourceEntry(
       const base::FilePath& file_path,
       const GetResourceEntryCallback& callback) OVERRIDE {}
-  virtual void ReadDirectory(const base::FilePath& file_path,
-                             const ReadDirectoryCallback& callback) OVERRIDE {}
+  virtual void ReadDirectory(
+      const base::FilePath& file_path,
+      const ReadDirectoryEntriesCallback& entries_callback,
+      const FileOperationCallback& completion_callback) OVERRIDE {}
   virtual void Search(const std::string& search_query,
                       const GURL& next_link,
                       const SearchCallback& callback) OVERRIDE {}
diff --git a/chrome/browser/chromeos/drive/fake_file_system.cc b/chrome/browser/chromeos/drive/fake_file_system.cc
index 00555dc..1497ad8 100644
--- a/chrome/browser/chromeos/drive/fake_file_system.cc
+++ b/chrome/browser/chromeos/drive/fake_file_system.cc
@@ -169,7 +169,8 @@ void FakeFileSystem::GetResourceEntry(
 
 void FakeFileSystem::ReadDirectory(
     const base::FilePath& file_path,
-    const ReadDirectoryCallback& callback) {
+    const ReadDirectoryEntriesCallback& entries_callback,
+    const FileOperationCallback& completion_callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
diff --git a/chrome/browser/chromeos/drive/file_system_interface.h b/chrome/browser/chromeos/drive/file_system_interface.h
index d80e292..1ddbc8f 100644
--- a/chrome/browser/chromeos/drive/file_system_interface.h
+++ b/chrome/browser/chromeos/drive/file_system_interface.h
@@ -79,11 +79,8 @@ typedef base::Callback<void(FileError error,
     GetFileContentInitializedCallback;
 
 // Used to get list of entries under a directory.
-// If |error| is not FILE_ERROR_OK, |entries| is null.
-typedef base::Callback<void(FileError error,
-                            scoped_ptr<ResourceEntryVector> entries,
-                            bool has_more)>
-    ReadDirectoryCallback;
+typedef base::Callback<void(scoped_ptr<ResourceEntryVector> entries)>
+    ReadDirectoryEntriesCallback;
 
 // Used to get drive content search results.
 // If |error| is not FILE_ERROR_OK, |result_paths| is empty.
@@ -359,10 +356,13 @@ class FileSystemInterface {
 
   // Finds and reads a directory by |file_path|. This call will also retrieve
   // and refresh file system content from server and disk cache.
+  // |entries_callback| can be a null callback when not interested in entries.
   //
-  // |callback| must not be null.
-  virtual void ReadDirectory(const base::FilePath& file_path,
-                             const ReadDirectoryCallback& callback) = 0;
+  // |completion_callback| must not be null.
+  virtual void ReadDirectory(
+      const base::FilePath& file_path,
+      const ReadDirectoryEntriesCallback& entries_callback,
+      const FileOperationCallback& completion_callback) = 0;
 
   // Does server side content search for |search_query|.
   // If |next_link| is set, this is the search result url that will be
diff --git a/chrome/browser/chromeos/drive/fileapi/fileapi_worker.cc b/chrome/browser/chromeos/drive/fileapi/fileapi_worker.cc
index 2ae6908..f172c4c 100644
--- a/chrome/browser/chromeos/drive/fileapi/fileapi_worker.cc
+++ b/chrome/browser/chromeos/drive/fileapi/fileapi_worker.cc
@@ -66,19 +66,10 @@ void RunGetFileInfoCallback(const GetFileInfoCallback& callback,
   callback.Run(base::File::FILE_OK, file_info);
 }
 
-// Runs |callback| with arguments converted from |error| and |resource_entries|.
-void RunReadDirectoryCallback(
+// Runs |callback| with entries.
+void RunReadDirectoryCallbackWithEntries(
     const ReadDirectoryCallback& callback,
-    FileError error,
-    scoped_ptr<ResourceEntryVector> resource_entries,
-    bool has_more) {
-  if (error != FILE_ERROR_OK) {
-    DCHECK(!has_more);
-    callback.Run(FileErrorToBaseFileError(error),
-                 std::vector<fileapi::DirectoryEntry>(), has_more);
-    return;
-  }
-
+    scoped_ptr<ResourceEntryVector> resource_entries) {
   DCHECK(resource_entries);
 
   std::vector<fileapi::DirectoryEntry> entries;
@@ -97,7 +88,14 @@ void RunReadDirectoryCallback(
     entries.push_back(entry);
   }
 
-  callback.Run(base::File::FILE_OK, entries, has_more);
+  callback.Run(base::File::FILE_OK, entries, true /*has_more*/);
+}
+
+// Runs |callback| with |error|.
+void RunReadDirectoryCallbackOnCompletion(const ReadDirectoryCallback& callback,
+                                          FileError error) {
+  callback.Run(FileErrorToBaseFileError(error),
+               std::vector<fileapi::DirectoryEntry>(), false /*has_more*/);
 }
 
 // Runs |callback| with arguments based on |error|, |local_path| and |entry|.
@@ -265,8 +263,10 @@ void ReadDirectory(const base::FilePath& file_path,
                    const ReadDirectoryCallback& callback,
                    FileSystemInterface* file_system) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  file_system->ReadDirectory(file_path,
-                             base::Bind(&RunReadDirectoryCallback, callback));
+  file_system->ReadDirectory(
+      file_path,
+      base::Bind(&RunReadDirectoryCallbackWithEntries, callback),
+      base::Bind(&RunReadDirectoryCallbackOnCompletion, callback));
 }
 
 void Remove(const base::FilePath& file_path,