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

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 338 matching lines @@
       const std::string& directory_resource_id,
       const FileOperationCallback& callback,
       google_apis::GDataErrorCode status,
       scoped_ptr<google_apis::AboutResource> about_resource) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
 
   int64 remote_changestamp = 0;
   if (util::GDataToDriveFileError(status) == DRIVE_FILE_OK) {
     DCHECK(about_resource);
     remote_changestamp = about_resource->largest_change_id();

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

we should probably update last_known_remote_changestamp_  here too.

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

Done.

   }
 
   const DirectoryFetchInfo directory_fetch_info(directory_resource_id,
                                                 remote_changestamp);
   DoLoadDirectoryFromServer(directory_fetch_info, callback);
 }
 
 void ChangeListLoader::DoLoadDirectoryFromServer(
     const DirectoryFetchInfo& directory_fetch_info,
     const FileOperationCallback& callback) {
@@ skipping 363 matching lines @@
   // If the directory of interest is already scheduled to be fetched, add the
   // callback to the pending list and return.
   LoadCallbackMap::iterator it = pending_load_callback_.find(resource_id);
   if (it != pending_load_callback_.end()) {
     it->second.push_back(callback);
     return;
   }
 
   // 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_) {

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

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

[kinaba, 2013/04/05 07:04:37]

Correct.
Done.

     base::MessageLoopProxy::current()->PostTask(
         FROM_HERE,
         base::Bind(callback, DRIVE_FILE_OK));
     return;
   }
 
   // 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));
+
+  DirectoryFetchInfo new_directory_fetch_info(
+      directory_fetch_info.resource_id(),
+      last_known_remote_changestamp_);

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

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]

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

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

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

   DoLoadDirectoryFromServer(
-      directory_fetch_info,
+      new_directory_fetch_info,
       base::Bind(&ChangeListLoader::OnDirectoryLoadComplete,
                  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));
@@ skipping 90 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