Change AboutResourceLoader::GetAboutResource to wait for inflight update task.

This is a preparation for Bug 329732. The actual fix for the bug
will come in the next separate patch.

The current changes forces GetAboutResource to wait for the result of
running UpdateAboutResource task if any.

BUG=329732

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

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

chrome/browser/chromeos/drive/change_list_loader.cc

@@ -209,6 +209,7 @@ void LoaderController::Unlock() {
 
 AboutResourceLoader::AboutResourceLoader(JobScheduler* scheduler)
     : scheduler_(scheduler),
+      current_update_task_id_(-1),
       weak_ptr_factory_(this) {
 }
 
@@ -219,6 +220,12 @@ void AboutResourceLoader::GetAboutResource(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
 
+  // If the latest UpdateAboutResource task is still running. Wait for it,
+  if (pending_callbacks_.count(current_update_task_id_)) {
+    pending_callbacks_[current_update_task_id_].push_back(callback);
+    return;
+  }
+
   if (cached_about_resource_) {
     base::MessageLoopProxy::current()->PostTask(
         FROM_HERE,
@@ -237,34 +244,47 @@ void AboutResourceLoader::UpdateAboutResource(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
 
+  ++current_update_task_id_;
+  pending_callbacks_[current_update_task_id_].push_back(callback);
+
   scheduler_->GetAboutResource(
       base::Bind(&AboutResourceLoader::UpdateAboutResourceAfterGetAbout,
                  weak_ptr_factory_.GetWeakPtr(),
-                 callback));
+                 current_update_task_id_));
 }
 
 void AboutResourceLoader::UpdateAboutResourceAfterGetAbout(
-    const google_apis::AboutResourceCallback& callback,
+    int task_id,
     google_apis::GDataErrorCode status,
     scoped_ptr<google_apis::AboutResource> about_resource) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(!callback.is_null());
   FileError error = GDataToFileError(status);
 
-  if (error == FILE_ERROR_OK) {
-    if (cached_about_resource_ &&
-        cached_about_resource_->largest_change_id() >
-        about_resource->largest_change_id()) {
-      LOG(WARNING) << "Local cached about resource is fresher than server, "
-                   << "local = " << cached_about_resource_->largest_change_id()
-                   << ", server = " << about_resource->largest_change_id();
-    }
+  const std::vector<google_apis::AboutResourceCallback> callbacks =
+      pending_callbacks_[task_id];
+  pending_callbacks_.erase(task_id);
 
-    cached_about_resource_.reset(
-        new google_apis::AboutResource(*about_resource));
+  if (error != FILE_ERROR_OK) {
+    for (size_t i = 0; i < callbacks.size(); ++i)
+      callbacks[i].Run(status, scoped_ptr<google_apis::AboutResource>());
+    return;
   }
 
-  callback.Run(status, about_resource.Pass());
+  // Updates the cache when the resource is successfully obtained.
+  if (cached_about_resource_ &&
+      cached_about_resource_->largest_change_id() >
+      about_resource->largest_change_id()) {
+    LOG(WARNING) << "Local cached about resource is fresher than server, "
+                 << "local = " << cached_about_resource_->largest_change_id()
+                 << ", server = " << about_resource->largest_change_id();
+  }
+  cached_about_resource_.reset(new google_apis::AboutResource(*about_resource));
+
+  for (size_t i = 0; i < callbacks.size(); ++i) {
+    callbacks[i].Run(
+        status,
+        make_scoped_ptr(new google_apis::AboutResource(*about_resource)));
+  }
 }
 
 ChangeListLoader::ChangeListLoader(

chrome/browser/chromeos/drive/change_list_loader.h

@@ -85,37 +85,46 @@ class AboutResourceLoader {
     return cached_about_resource_.get();
   }
 
-  // Gets the about resource from the cache or the server. If the cache is
-  // availlavle, just runs |callback| with the cached about resource. If not,
-  // calls |UpdateAboutResource| passing |callback|.
+  // Gets the 'latest' about resource and asynchronously runs |callback|. I.e.,
+  // 1) If the last call to UpdateAboutResource call is in-flight, wait for it.
+  // 2) Otherwise, if the resource is cached, just returns the cached value.
+  // 3) If neither of the above hold, queries the API server by calling
+  //   |UpdateAboutResource|.
   void GetAboutResource(const google_apis::AboutResourceCallback& callback);
 
   // Gets the about resource from the server, and caches it if successful. This
   // function calls JobScheduler::GetAboutResource internally. The cache will be
   // used in |GetAboutResource|.
-  void UpdateAboutResource(
-      const google_apis::AboutResourceCallback& callback);
+  void UpdateAboutResource(const google_apis::AboutResourceCallback& callback);
 
  private:
   // Part of UpdateAboutResource().
   // This function should be called when the latest about resource is being
-  // fetched from the server. The retrieved about resoure is cloned, and one is
-  // cached and the other is passed to |callback|.
+  // fetched from the server. The retrieved about resource is cloned, and one is
+  // cached and the other is passed to callbacks associated with |task_id|.
   void UpdateAboutResourceAfterGetAbout(
-      const google_apis::AboutResourceCallback& callback,
+      int task_id,
       google_apis::GDataErrorCode status,
       scoped_ptr<google_apis::AboutResource> about_resource);
 
   JobScheduler* scheduler_;
   scoped_ptr<google_apis::AboutResource> cached_about_resource_;
+
+  // Identifier to denote the latest UpdateAboutResource call.
+  int current_update_task_id_;
+  // Mapping from each UpdateAboutResource task ID to the corresponding
+  // callbacks. Note that there will be multiple callbacks for a single task
+  // when GetAboutResource is called before the task completes.
+  std::map<int, std::vector<google_apis::AboutResourceCallback> >
+      pending_callbacks_;
+
   base::WeakPtrFactory<AboutResourceLoader> weak_ptr_factory_;
   DISALLOW_COPY_AND_ASSIGN(AboutResourceLoader);
 };
 
 // ChangeListLoader is used to load the change list, the full resource list,
-// and directory contents, from WAPI (codename for Documents List API)
-// or Google Drive API.  The class also updates the resource metadata with
-// the change list loaded from the server.
+// and directory contents, from Google Drive API.  The class also updates the
+// resource metadata with the change list loaded from the server.
 //
 // Note that the difference between "resource list" and "change list" is
 // subtle hence the two words are often used interchangeably. To be precise,

chrome/browser/chromeos/drive/change_list_loader_unittest.cc

@@ -143,6 +143,70 @@ class ChangeListLoaderTest : public testing::Test {
   scoped_ptr<ChangeListLoader> change_list_loader_;
 };
 
+TEST_F(ChangeListLoaderTest, AboutResourceLoader) {
+  google_apis::GDataErrorCode error[6] = {};
+  scoped_ptr<google_apis::AboutResource> about[6];
+
+  // No resource is cached at the beginning.
+  ASSERT_FALSE(about_resource_loader_->cached_about_resource());
+
+  // Since no resource is cached, this "Get" should trigger the update.
+  about_resource_loader_->GetAboutResource(
+      google_apis::test_util::CreateCopyResultCallback(error + 0, about + 0));
+  // Since there is one in-flight update, the next "Get" just wait for it.
+  about_resource_loader_->GetAboutResource(
+      google_apis::test_util::CreateCopyResultCallback(error + 1, about + 1));
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(google_apis::HTTP_SUCCESS, error[0]);
+  EXPECT_EQ(google_apis::HTTP_SUCCESS, error[1]);
+  const int64 first_changestamp = about[0]->largest_change_id();
+  EXPECT_EQ(first_changestamp, about[1]->largest_change_id());
+  ASSERT_TRUE(about_resource_loader_->cached_about_resource());
+  EXPECT_EQ(
+      first_changestamp,
+      about_resource_loader_->cached_about_resource()->largest_change_id());
+
+  // Increment changestamp by 1.
+  AddNewFile("temp");
+  // Explicitly calling UpdateAboutResource will start another API call.
+  about_resource_loader_->UpdateAboutResource(
+      google_apis::test_util::CreateCopyResultCallback(error + 2, about + 2));
+  // It again waits for the in-flight UpdateAboutResoure call, even though this
+  // time there is a cached result.
+  about_resource_loader_->GetAboutResource(
+      google_apis::test_util::CreateCopyResultCallback(error + 3, about + 3));
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(google_apis::HTTP_SUCCESS, error[2]);
+  EXPECT_EQ(google_apis::HTTP_SUCCESS, error[3]);
+  EXPECT_EQ(first_changestamp + 1, about[2]->largest_change_id());
+  EXPECT_EQ(first_changestamp + 1, about[3]->largest_change_id());
+  EXPECT_EQ(
+      first_changestamp + 1,
+      about_resource_loader_->cached_about_resource()->largest_change_id());
+
+  // Increment changestamp by 1.
+  AddNewFile("temp2");
+  // Now no UpdateAboutResource task is running. Returns the cached result.
+  about_resource_loader_->GetAboutResource(
+      google_apis::test_util::CreateCopyResultCallback(error + 4, about + 4));
+  // Explicitly calling UpdateAboutResource will start another API call.
+  about_resource_loader_->UpdateAboutResource(
+      google_apis::test_util::CreateCopyResultCallback(error + 5, about + 5));
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(google_apis::HTTP_NO_CONTENT, error[4]);
+  EXPECT_EQ(google_apis::HTTP_SUCCESS, error[5]);
+  EXPECT_EQ(first_changestamp + 1, about[4]->largest_change_id());
+  EXPECT_EQ(first_changestamp + 2, about[5]->largest_change_id());
+  EXPECT_EQ(
+      first_changestamp + 2,
+      about_resource_loader_->cached_about_resource()->largest_change_id());
+
+  EXPECT_EQ(3, drive_service_->about_resource_load_count());
+}
+
 TEST_F(ChangeListLoaderTest, Load) {
   EXPECT_FALSE(change_list_loader_->IsRefreshing());
 

chrome/browser/chromeos/drive/directory_loader.cc

@@ -298,13 +298,6 @@ void DirectoryLoader::ReadDirectoryAfterGetEntry(
   if (pending_load_callback_[local_id].size() > 1)
     return;
 
-  // Note: To be precise, we need to call UpdateAboutResource() here. However,
-  // - It is costly to do GetAboutResource HTTP request every time.
-  // - The chance using an old value is small; it only happens when
-  //   ReadDirectory is called during one GetAboutResource roundtrip time
-  //   of a change list fetching.
-  // - Even if the value is old, it just marks the directory as older. It may
-  //   trigger one future unnecessary re-fetch, but it'll never lose data.
   about_resource_loader_->GetAboutResource(
       base::Bind(&DirectoryLoader::ReadDirectoryAfterGetAboutResource,
                  weak_ptr_factory_.GetWeakPtr(), local_id));