gdata: Implement periodic file system update checks.

chrome/browser/chromeos/gdata/gdata_file_system.cc

Index: chrome/browser/chromeos/gdata/gdata_file_system.cc
diff --git a/chrome/browser/chromeos/gdata/gdata_file_system.cc b/chrome/browser/chromeos/gdata/gdata_file_system.cc
index bd79a7fada83c8cb2e48f257cbe1f9bfb8cc4cde..0585925cdd5c4b2c00054bb0fe8d18eb765237d7 100644
--- a/chrome/browser/chromeos/gdata/gdata_file_system.cc
+++ b/chrome/browser/chromeos/gdata/gdata_file_system.cc
@@ -77,6 +77,13 @@ const FilePath::CharType kFilesystemProtoFile[] =
     FILE_PATH_LITERAL("file_system.pb");
 const FilePath::CharType kSymLinkToDevNull[] = FILE_PATH_LITERAL("/dev/null");
 
+// GData update check interval (in seconds).
+#ifndef NDEBUG
+const int kGDataUpdateCheckIntervalInSec = 5;
+#else
+const int kGDataUpdateCheckIntervalInSec = 60;
+#endif
+
 // Schedule for dumping root file system proto buffers to disk depending its
 // total protobuffer size in MB.
 typedef struct {
@@ -928,6 +935,7 @@ GDataFileSystem::GDataFileSystem(Profile* profile,
           true /* manual reset */, true /* initially signaled */)),
       cache_initialization_started_(false),
       num_pending_tasks_(0),
+      num_update_requests_(0),
       hide_hosted_docs_(false),
       ui_weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(
           new base::WeakPtrFactory<GDataFileSystem>(this))),
@@ -961,6 +969,18 @@ void GDataFileSystem::Initialize() {
   InitializePreferenceObserver();
 }
 
+void GDataFileSystem::CheckForUpdates() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  ContentOrigin initial_origin = root_->origin();
+  if (initial_origin == FROM_SERVER) {
+    root_->set_origin(REFRESHING);
+    ReloadFeedFromServerIfNeeded(initial_origin,
+                                 root_->largest_changestamp(),
+                                 root_->GetFilePath(),
+                                 gdata::FindEntryCallback());
+  }
+}
+
 bool GDataFileSystem::SetCacheRootPathForTesting(const FilePath& root_path) {
   if (cache_initialization_started_)
     return false;
@@ -1016,6 +1036,31 @@ void GDataFileSystem::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
+void GDataFileSystem::StartUpdates() {
+  // Perform an immediate check for updates.
+  CheckForUpdates();
+  ++num_update_requests_;
+  if (num_update_requests_ == 1) {
+    // If this is the first StartUpdate request, start timer to periodically
+    // check for updates at the specified intervals.
+    update_timer_.Timer::Start(FROM_HERE,
+                               base::TimeDelta::FromSeconds(
+                                   kGDataUpdateCheckIntervalInSec),
+                               base::Bind(&GDataFileSystem::CheckForUpdates,
+                                          ui_weak_ptr_));

[hshi, 2012/05/03 18:26:37]

Unfortunately I have to explicitly call base class (Timer::Start) here. Some
compiler problem with C++ templates and inheritance prevents me from directly
calling the Start method with the callback as the 3rd argument.

[satorux1, 2012/05/03 18:46:20]

I just found this comment in timer.h:

// OneShotTimer and RepeatingTimer both cancel the timer when they go out of    
// scope, which makes it easy to ensure that you do not get called when your    
// object has gone out of scope.

Passing |this| is guaranteed to be safe. So I think your original code is
correct. The API looks rather scary, but should be safe.

[hshi, 2012/05/03 18:55:54]

How about I just use the base::Timer class as Zel suggested? I find that it is
actually cleaner. Passing in |this| is scary and confusing.

On 2012/05/03 18:46:20, satorux1 wrote:

[satorux1, 2012/05/03 18:59:54]

Sounds good.

[hshi, 2012/05/03 19:09:19]

Done.

+  }
+}
+
+void GDataFileSystem::StopUpdates() {
+  if (num_update_requests_ <= 0) {
+    NOTREACHED();
+    return;
+  }

[satorux1, 2012/05/03 18:46:20]

you can replace the four lines with:

DCHECK_GE(0, num_update_requests);

[hshi, 2012/05/03 18:55:54]

Did you mean DCHECK_GE(num_update_requests, 0)?

On 2012/05/03 18:46:20, satorux1 wrote:

[satorux1, 2012/05/03 18:59:54]

oh I meant DCHECK_LE(). not a big deal at all, but seems to me that a constant
(in this case 0) is often the first parameter in DCHECK_XX().

[hshi, 2012/05/03 19:09:19]

Done.

+  --num_update_requests_;
+  if (num_update_requests_ == 0)
+    update_timer_.Stop();
+}
+
 void GDataFileSystem::Authenticate(const AuthStatusCallback& callback) {
   // TokenFetcher, used in DocumentsService, must be run on UI thread.
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -1066,16 +1111,6 @@ void GDataFileSystem::FindEntryByPathAsyncOnUIThread(
                           search_file_path,
                           callback);
     return;
-  } else if (root_->NeedsRefresh()) {
-    // If content is stale or from disk from cache, fetch content from
-    // the server.
-    ContentOrigin initial_origin = root_->origin();
-    root_->set_origin(REFRESHING);
-    ReloadFeedFromServerIfNeeded(initial_origin,
-                                 root_->largest_changestamp(),
-                                 search_file_path,
-                                 callback);
-    return;
   }
 
   // Post a task to the same thread, rather than calling it here, as