drive: Update directory changestamp after "fast fetch" for the content.

drive: Update directory changestamp after "fast fetch" for the content.

BUG=226011
TEST=The manual steps described in the BUG.

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

[kinaba, 2013-04-03 06:53:10]

ptal.

[satorux1, 2013-04-03 07:14:07]

https://codereview.chromium.org/13449004/diff/2001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/change_list_loader.cc (left):

https://codereview.chromium.org/13449004/diff/2001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/change_list_loader.cc:359: remote_changestamp =
about_resource->largest_change_id();
we should probably update last_known_remote_changestamp_  here too.

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

https://codereview.chromium.org/13449004/diff/2001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/change_list_loader.cc:757:
last_known_remote_changestamp_);
not sure using last_known_remote_changestamp_ is correct.
last_known_remote_changestamp_ could be 0?

Maybe we should call LoadDirectoryFromServer()? Then, the latest remote
changestamp will be retrieved from the server.

[kinaba, 2013-04-03 07:58:25]

https://codereview.chromium.org/13449004/diff/2001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/change_list_loader.cc (left):

https://codereview.chromium.org/13449004/diff/2001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/change_list_loader.cc:359: remote_changestamp =
about_resource->largest_change_id();
On 2013/04/03 07:14:07, satorux1 wrote:
> we should probably update last_known_remote_changestamp_  here too.

Done.

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

https://codereview.chromium.org/13449004/diff/2001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/change_list_loader.cc:757:
last_known_remote_changestamp_);
On 2013/04/03 07:14:07, satorux1 wrote:
> not sure using last_known_remote_changestamp_ is correct.
> last_known_remote_changestamp_ could be 0?

If it is 0 we are exiting the function in the if-statement above
(it could be argued that the if-statement is wrong, though...)

> Maybe we should call LoadDirectoryFromServer()? Then, the latest remote
> changestamp will be retrieved from the server.

Ideally we want the latest value, but I think it is still ok using
|last_known_remote_changestamp_| here.

- Getting changestamp takes hundreds of milliseconds that we want to avoid if
possible.

- This call is reached only when a normal feed fetching operation, which should
update |last_known_remote_changestamp_|, is already in-flight.

- There is a chance that it is reached in the dangerous time window before "the
normal operation" updates changestamp, but
-- it does not happen during initial feed loading (users cannot initiate another
fast-fetch until the update by root directory fetching is finished.)
-- if it is being racy with delta feed fetching, observing an old value has the
same effect as just the directory listing has started slightly earlier than the
delta fetching; so it shouldn't be a big deal.

What do you think?

[kinaba, 2013-04-04 08:14:57]

friendly ping

[satorux1, 2013-04-04 08:30:47]

sorry for the belated response.

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

https://codereview.chromium.org/13449004/diff/2001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/change_list_loader.cc:743: if
(directory_fetch_info.changestamp() >= last_known_remote_changestamp_) {
Seems to me that last_known_remote_changestamp_ is not guaranteed to be non-zero
here. ScheduleRun() is called from LoadIfNeeded(). If LoadIfNeeded() is called
twice, last_known_remote_changestamp_ may be zero. Am I reading the code right?

If so, we should change the conditional to "last_known_remote_changestamp_ > 0
&& ..."

https://codereview.chromium.org/13449004/diff/2001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/change_list_loader.cc:757:
last_known_remote_changestamp_);
On 2013/04/03 07:58:25, kinaba wrote:
> On 2013/04/03 07:14:07, satorux1 wrote:
> > not sure using last_known_remote_changestamp_ is correct.
> > last_known_remote_changestamp_ could be 0?
> 
> If it is 0 we are exiting the function in the if-statement above
> (it could be argued that the if-statement is wrong, though...)

See above.

> 
> > Maybe we should call LoadDirectoryFromServer()? Then, the latest remote
> > changestamp will be retrieved from the server.
> 
> Ideally we want the latest value, but I think it is still ok using
> |last_known_remote_changestamp_| here.
> 
> - Getting changestamp takes hundreds of milliseconds that we want to avoid if
> possible.
> 
> - This call is reached only when a normal feed fetching operation, which
should
> update |last_known_remote_changestamp_|, is already in-flight.
> 
> - There is a chance that it is reached in the dangerous time window before
"the
> normal operation" updates changestamp, but
> -- it does not happen during initial feed loading (users cannot initiate
another
> fast-fetch until the update by root directory fetching is finished.)
> -- if it is being racy with delta feed fetching, observing an old value has
the
> same effect as just the directory listing has started slightly earlier than
the
> delta fetching; so it shouldn't be a big deal.
> 
> What do you think?

Sounds reasonable. Could you add some comment why we are using
last_known_remote_changestamp_ here?

[kinaba, 2013-04-05 07:04:36]

Thank you for the comments. Updated the patch.

> Sounds reasonable. Could you add some comment why we are using
> last_known_remote_changestamp_ here?

Done.

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

https://codereview.chromium.org/13449004/diff/2001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/change_list_loader.cc:743: if
(directory_fetch_info.changestamp() >= last_known_remote_changestamp_) {
On 2013/04/04 08:30:47, satorux1 wrote:
> Seems to me that last_known_remote_changestamp_ is not guaranteed to be
non-zero
> here.

Correct.

> If so, we should change the conditional to "last_known_remote_changestamp_ > 0
> && ..."

Done.

[satorux1, 2013-04-05 23:21:29]

LGTM. sorry for the belated response.

[commit-bot: I haz the power, 2013-04-06 00:09:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinaba@chromium.org/13449004/15001

[commit-bot: I haz the power, 2013-04-06 06:48:49]

Change committed as 192714