Fix ChangeListLoader's callback invocation.

chrome/browser/chromeos/drive/change_list_loader.cc

Index: chrome/browser/chromeos/drive/change_list_loader.cc
diff --git a/chrome/browser/chromeos/drive/change_list_loader.cc b/chrome/browser/chromeos/drive/change_list_loader.cc
index 571213726c2da11e2d10c033acc25f65c7eaa28e..c92787f1c85c0429c557c568f597fbc429863d2d 100644
--- a/chrome/browser/chromeos/drive/change_list_loader.cc
+++ b/chrome/browser/chromeos/drive/change_list_loader.cc
@@ -208,7 +208,7 @@ void ChangeListLoader::CompareChangestampsAndLoadIfNeeded(
   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;
   }
 
@@ -235,8 +235,12 @@ void ChangeListLoader::CompareChangestampsAndLoadIfNeeded(
                    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);
   }
 }
 
@@ -270,7 +274,7 @@ void ChangeListLoader::StartLoadChangeListFromServer(
   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.
@@ -288,7 +292,7 @@ void ChangeListLoader::StartLoadChangeListFromServer(
 }
 
 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);
@@ -414,7 +418,7 @@ void ChangeListLoader::DoLoadDirectoryFromServerAfterRefresh(
   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_,
@@ -740,11 +744,16 @@ void ChangeListLoader::ScheduleRun(
     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(