Fix ChangeListLoader's callback invocation.

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/metrics/histogram.h"
@@ skipping 190 matching lines @@
 
     // No changes detected, tell the client that the loading was successful.
     OnChangeListLoadComplete(callback, DRIVE_FILE_OK);
     return;
   }
 
   int64 start_changestamp = local_changestamp > 0 ? local_changestamp + 1 : 0;
   if (start_changestamp == 0 && !about_resource.get()) {
     // Full update needs AboutResource. If this is a full update, we should just
     // give up.
-    callback.Run(DRIVE_FILE_ERROR_FAILED);
+    OnChangeListLoadComplete(callback, DRIVE_FILE_ERROR_FAILED);

[kinaba, 2013/03/26 01:48:31]

To exit from the feed loading, we always have to flush the pending callback
queue.

[satorux1, 2013/03/26 03:48:59]

Add this as a comment?

[kinaba, 2013/03/26 04:13:22]

Done.

     return;
   }
 
   if (directory_fetch_info.empty()) {
     // If the caller is not interested in a particular directory, just start
     // loading the change list.
     LoadChangeListFromServer(about_resource.Pass(),
                              start_changestamp,
                              callback);
   } else if (directory_fetch_info.changestamp() < remote_changestamp) {
     // If the caller is interested in a particular directory, and the
     // directory changestamp is older than server's, start loading the
     // directory first.
     DVLOG(1) << "Fast-fetching directory: " << directory_fetch_info.ToString()
              << "; remote_changestamp: " << remote_changestamp;
     const DirectoryFetchInfo new_directory_fetch_info(
         directory_fetch_info.resource_id(), remote_changestamp);
     DoLoadDirectoryFromServer(
         new_directory_fetch_info,
         base::Bind(&ChangeListLoader::StartLoadChangeListFromServer,
                    weak_ptr_factory_.GetWeakPtr(),
                    directory_fetch_info,
                    base::Passed(&about_resource),
                    start_changestamp,
                    callback));
   } else {
-    // The directory is up-to-date, hence there is no need to load.
-    OnChangeListLoadComplete(callback, DRIVE_FILE_OK);
+    // The directory is up-to-date. Proceed to change list loading.

[kinaba, 2013/03/26 01:48:31]

The directory is up-to-date, but not the case for other parts.
(because we are in the local_changestamp < remote_changestamp state here).

Note that StartLoadChangeListFromServer runs callback for the directory soon.

[satorux1, 2013/03/26 03:48:59]

add this as a comment?

[kinaba, 2013/03/26 04:13:22]

Done.

+    StartLoadChangeListFromServer(directory_fetch_info,
+                                  about_resource.Pass(),
+                                  start_changestamp,
+                                  callback,
+                                  DRIVE_FILE_OK);
   }
 }
 
 void ChangeListLoader::LoadChangeListFromServer(
     scoped_ptr<google_apis::AboutResource> about_resource,
     int64 start_changestamp,
     const FileOperationCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
   DCHECK(refreshing_);
@@ skipping 13 matching lines @@
     const DirectoryFetchInfo& directory_fetch_info,
     scoped_ptr<google_apis::AboutResource> about_resource,
     int64 start_changestamp,
     const FileOperationCallback& callback,
     DriveFileError error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
   DCHECK(refreshing_);
 
   if (error == DRIVE_FILE_OK) {
-    callback.Run(DRIVE_FILE_OK);
+    OnDirectoryLoadComplete(directory_fetch_info, callback, DRIVE_FILE_OK);

[kinaba, 2013/03/26 01:48:31]

see below.

     DVLOG(1) << "Fast-fetch was successful: " << directory_fetch_info.ToString()
              << "; Start loading the change list";
     // Stop passing |callback| as it's just consumed.
     LoadChangeListFromServer(
         about_resource.Pass(),
         start_changestamp,
         base::Bind(&util::EmptyFileOperationCallback));
   } else {
     // The directory fast-fetch failed, but the change list loading may
     // succeed. Keep passing |callback| so it's run after the change list
     // loading is complete.
     LoadChangeListFromServer(
         about_resource.Pass(), start_changestamp, callback);
   }
 }
 
 void ChangeListLoader::OnGetAppList(google_apis::GDataErrorCode status,
-                                   scoped_ptr<google_apis::AppList> app_list) {
+                                    scoped_ptr<google_apis::AppList> app_list) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DriveFileError error = util::GDataToDriveFileError(status);
   if (error != DRIVE_FILE_OK)
     return;
 
   if (app_list.get()) {
     webapps_registry_->UpdateFromAppList(*app_list);
   }
 }
@@ skipping 105 matching lines @@
 
 void ChangeListLoader::DoLoadDirectoryFromServerAfterRefresh(
     const DirectoryFetchInfo& directory_fetch_info,
     const FileOperationCallback& callback,
     DriveFileError error,
     const base::FilePath& directory_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
 
   DVLOG(1) << "Directory loaded: " << directory_fetch_info.ToString();
-  OnDirectoryLoadComplete(directory_fetch_info, callback, error);
+  callback.Run(error);

[kinaba, 2013/03/26 01:48:31]

Let me set the policy that "DoLoadDirectoryFromServer should always exit
callback.Run(). flushing pending callbacks are the duty of callers of
DoLoadDirectory".

Before this patch, DoLoadDirectory flushes pending tasks in case of success, and
does not flush in failure cases. This is OK (and desired) if
StartLoadChangeListFromServer is always following. But it is not the case (the
callsite in ScheduleRun()) and in failure case pending tasks are not flushed.

[satorux1, 2013/03/26 03:48:59]

Add this as a function comment in .h file?

[kinaba, 2013/03/26 04:13:22]

Done.

   // Also notify the observers.
   if (error == DRIVE_FILE_OK) {
     FOR_EACH_OBSERVER(ChangeListLoaderObserver, observers_,
                       OnDirectoryChanged(directory_path));
   }
 }
 
 void ChangeListLoader::SearchFromServer(
     const std::string& search_query,
     bool shared_with_me,
@@ skipping 305 matching lines @@
 
   // If the directory's changestamp is up-to-date, just schedule to run the
   // callback, as there is no need to fetch the directory.
   if (directory_fetch_info.changestamp() >= last_known_remote_changestamp_) {
     base::MessageLoopProxy::current()->PostTask(
         FROM_HERE,
         base::Bind(callback, DRIVE_FILE_OK));
     return;
   }
 
-  // The directory should be fetched. Add the callback to the pending list. The
-  // callback will be run after the directory is loaded.
-  pending_load_callback_[resource_id].push_back(callback);
-  DoLoadDirectoryFromServer(directory_fetch_info,
-                            base::Bind(&util::EmptyFileOperationCallback));
+  // The directory should be fetched. Add a dummy task to so ScheduleRun()
+  // can check that the directory is being fetched.
+  pending_load_callback_[resource_id].push_back(
+      base::Bind(&util::EmptyFileOperationCallback));
+  DoLoadDirectoryFromServer(
+      directory_fetch_info,
+      base::Bind(&ChangeListLoader::OnDirectoryLoadComplete,

[kinaba, 2013/03/26 01:48:31]

see above. flushing tasks is the duty of callers of DoLoadDirectoryFromServer.

+                 weak_ptr_factory_.GetWeakPtr(),
+                 directory_fetch_info,
+                 callback));
 }
 
 void ChangeListLoader::NotifyDirectoryChangedAfterApplyFeed(
     bool should_notify_changed_directories,
     const base::Closure& update_finished_callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(change_list_processor_.get());
   DCHECK(!update_finished_callback.is_null());
 
   loaded_ = true;
@@ skipping 86 matching lines @@
     for (size_t i = 0; i < callbacks.size(); ++i) {
       base::MessageLoopProxy::current()->PostTask(
           FROM_HERE,
           base::Bind(callbacks[i], error));
     }
     pending_load_callback_.erase(it);
   }
 }
 
 }  // namespace drive