Fix ChangeListLoader's callback invocation.

Fix ChangeListLoader's callback invocation.

In some corner cases, it might have not invoked pending callback tasks,
or it invoked them too early before all feeds are ready.

BUG=223517
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190554

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

ptal. see my comments for the rationale for each change.

https://codereview.chromium.org/13079006/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/change_list_loader.cc (right):

https://codereview.chromium.org/13079006/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.cc:211:
OnChangeListLoadComplete(callback, DRIVE_FILE_ERROR_FAILED);
To exit from the feed loading, we always have to flush the pending callback
queue.

https://codereview.chromium.org/13079006/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.cc:238: // The directory is
up-to-date. Proceed to change list loading.
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.

https://codereview.chromium.org/13079006/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.cc:277:
OnDirectoryLoadComplete(directory_fetch_info, callback, DRIVE_FILE_OK);
see below.

https://codereview.chromium.org/13079006/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.cc:421: callback.Run(error);
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.

https://codereview.chromium.org/13079006/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.cc:753:
base::Bind(&ChangeListLoader::OnDirectoryLoadComplete,
see above. flushing tasks is the duty of callers of DoLoadDirectoryFromServer.

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

LGTM. Thank you for fixing the corner cases.

https://codereview.chromium.org/13079006/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/change_list_loader.cc (right):

https://codereview.chromium.org/13079006/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.cc:211:
OnChangeListLoadComplete(callback, DRIVE_FILE_ERROR_FAILED);
On 2013/03/26 01:48:31, kinaba wrote:
> To exit from the feed loading, we always have to flush the pending callback
> queue.

Add this as a comment?

https://codereview.chromium.org/13079006/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.cc:238: // The directory is
up-to-date. Proceed to change list loading.
On 2013/03/26 01:48:31, kinaba wrote:
> 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.

add this as a comment?

https://codereview.chromium.org/13079006/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.cc:421: callback.Run(error);
On 2013/03/26 01:48:31, kinaba wrote:
> Let me set the policy that "DoLoadDirectoryFromServer should always exit
> callback.Run(). flushing pending callbacks are the duty of callers of
> DoLoadDirectory".

Add this as a function comment in .h file?

> 
> 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.

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

https://codereview.chromium.org/13079006/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/change_list_loader.cc (right):

https://codereview.chromium.org/13079006/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.cc:211:
OnChangeListLoadComplete(callback, DRIVE_FILE_ERROR_FAILED);
On 2013/03/26 03:48:59, satorux1 wrote:
> On 2013/03/26 01:48:31, kinaba wrote:
> > To exit from the feed loading, we always have to flush the pending callback
> > queue.
> 
> Add this as a comment?

Done.

https://codereview.chromium.org/13079006/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.cc:238: // The directory is
up-to-date. Proceed to change list loading.
On 2013/03/26 03:48:59, satorux1 wrote:
> On 2013/03/26 01:48:31, kinaba wrote:
> > 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.
> 
> add this as a comment?

Done.

https://codereview.chromium.org/13079006/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/change_list_loader.cc:421: callback.Run(error);
On 2013/03/26 03:48:59, satorux1 wrote:
> On 2013/03/26 01:48:31, kinaba wrote:
> > Let me set the policy that "DoLoadDirectoryFromServer should always exit
> > callback.Run(). flushing pending callbacks are the duty of callers of
> > DoLoadDirectory".
> 
> Add this as a function comment in .h file?
> 
> > 
> > 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.
> 

Done.

[kinaba, 2013-03-26 04:21:36]

Committed patchset #2 manually as r190554 (presubmit successful).