Refactor drive::ChangeListLoader.

Refactor drive::ChangeListLoader.

- Manage all callbacks passed to LoadIfNeeded in the pending queue, not in method argument.
This eliminates the need of |refreshing_| variable, and simplify the logic
when to wait and when to start loading.

- Unify the place where loaded_ is set and OnInitialFeedLoaded() is called.

- Unify directory changestamp checking logic into one place.
Previously we had it twice.

- Get local_changestamp exactly once in every path.
Previously we had it twice in some cases, and zero in
some case (see the BUG).

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

[kinaba, 2013-04-17 05:33:53]

Could you take a look?

I gave up fixing the BUG=231220 incrementally and decided to refactor first...

[satorux1, 2013-04-17 07:50:15]

Looks much simpler! LGTM with nits.

https://codereview.chromium.org/13933016/diff/4001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/change_list_loader.h (right):

https://codereview.chromium.org/13933016/diff/4001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/change_list_loader.h:107: bool refreshing() const;
IsRefreshing()? This is now a non inline function.

https://codereview.chromium.org/13933016/diff/4001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/change_list_loader.h:137: // Check the directory's
changestamp and |last_known_remote_changestamp_|, and
Checks

[hashimoto, 2013-04-17 08:15:36]

lgtm

https://codereview.chromium.org/13933016/diff/4001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/change_list_loader.cc (right):

https://codereview.chromium.org/13933016/diff/4001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/change_list_loader.cc:43: return iter !=
pending_load_callback_.end() && !iter->second.empty();
nit: Is there any chance for iter->second to return empty?

https://codereview.chromium.org/13933016/diff/4001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/change_list_loader.cc:724: DVLOG(1) << "Fast-fetch
was successful: "
nit: How about using DVLOG_IF?

[kinaba, 2013-04-17 08:46:23]

Thanks for the comments.

I think we can add more simplification to the class.
I'll think about it later separately.

https://codereview.chromium.org/13933016/diff/4001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/change_list_loader.cc (right):

https://codereview.chromium.org/13933016/diff/4001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/change_list_loader.cc:43: return iter !=
pending_load_callback_.end() && !iter->second.empty();
On 2013/04/17 08:15:37, hashimoto wrote:
> nit: Is there any chance for iter->second to return empty?

no. done.

https://codereview.chromium.org/13933016/diff/4001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/change_list_loader.cc:724: DVLOG(1) << "Fast-fetch
was successful: "
On 2013/04/17 08:15:37, hashimoto wrote:
> nit: How about using DVLOG_IF?

Done.

https://codereview.chromium.org/13933016/diff/4001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/change_list_loader.h (right):

https://codereview.chromium.org/13933016/diff/4001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/change_list_loader.h:107: bool refreshing() const;
On 2013/04/17 07:50:15, satorux1 wrote:
> IsRefreshing()? This is now a non inline function.

Done.

https://codereview.chromium.org/13933016/diff/4001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/change_list_loader.h:137: // Check the directory's
changestamp and |last_known_remote_changestamp_|, and
On 2013/04/17 07:50:15, satorux1 wrote:
> Checks

Done.

[hashimoto, 2013-04-17 08:49:53]

lgtm

[kinaba, 2013-04-18 01:46:17]

Committed patchset #3 manually as r194746 (presubmit successful).