Update the number of fetched files more frequently.

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 8a58515fd61042604cdf8369d4999c8195d46556..70f28954b16f29b0beefdff132ebd67822ea2f08 100644
--- a/chrome/browser/chromeos/gdata/gdata_file_system.cc
+++ b/chrome/browser/chromeos/gdata/gdata_file_system.cc
@@ -55,6 +55,8 @@ const int kGDataUpdateCheckIntervalInSec = 5;
 const int kGDataUpdateCheckIntervalInSec = 60;
 #endif
 
+const int kNumUiUpdateStepsPerFetch = 50;

[satorux1, 2012/08/02 08:03:14]

With kMaxDocumentsPerFeed = 500 defined elsewhere, and 50 here, we update the
fetch progress UI per every 10 feeds. If we change kMaxDocumentsPerFeed to 700,
we'll end up counting per every 14 feeds, which would look bad.

That said, Wouldn't it be better to define something like

// Update the fetch progress UI per every 10 feeds, though it's giving illusion
to users. The feeds are actually processed per kMaxDocumentsPerFeed 
const int kFetchUiUpdateStep = 10;

[hidehiko, 2012/08/02 13:47:05]

Done.

+
 // Schedule for dumping root file system proto buffers to disk depending its
 // total protobuffer size in MB.
 typedef struct {
@@ -610,6 +612,54 @@ void RunGetEntryInfoWithFilePathCallback(
 
 }  // namespace
 
+// GDataFileSystem::GetDocumentsUIState struct implementation.

[satorux1, 2012/08/02 08:03:14]

UI -> Ui

[hidehiko, 2012/08/02 13:47:05]

Done.

+// This is a trick to update the number of fetched documents frequently on
+// UI. Due to performance reason, we need to fetch a number of files at
+// a time. However, it'll take long time, and a user has no way to know
+// the current update state. In order to make users confortable,
+// we increment the number of fetched documents with more frequent but smaller
+// steps than actual fetching.
+struct GDataFileSystem::GetDocumentsUIState {
+  GetDocumentsUIState(
+      base::TimeTicks start_time,
+      const base::Callback<void(GetDocumentsUIState*)>& callback)
+      : num_fetched_documents(0),
+        num_showing_documents(0),
+        num_remaining_ui_update(0),
+        start_time(start_time),
+        callback(callback),
+        ui_weak_ptr_factory(this),
+        ui_weak_ptr(ui_weak_ptr_factory.GetWeakPtr()) {
+  }
+
+  // Callback trampoline. By introducing this callback, the main procedure
+  // can cancel the pending events by just deleteing this instance.
+  void Run() {

[satorux1, 2012/08/02 08:03:14]

I'm guessing that this method is unnecessary. see below.

[hidehiko, 2012/08/02 13:47:05]

Done.

+    callback.Run(this);
+  }
+
+  // The number of fetched documents.
+  int num_fetched_documents;
+
+  // The number documents shown on UI.
+  int num_showing_documents;
+
+  // How many times the UI will be updated.
+  int num_remaining_ui_update;
+
+  // When the UI update has started.
+  base::TimeTicks start_time;
+
+  // How long duration the actual fetching takes in total.
+  // Heuristically, we use the duration to decide the UI update step period.
+  base::TimeDelta fetched_duration;
+
+  base::Callback<void(GetDocumentsUIState*)> callback;
+  base::WeakPtrFactory<GetDocumentsUIState> ui_weak_ptr_factory;
+  base::WeakPtr<GetDocumentsUIState> ui_weak_ptr;
+};
+
+
 // GDataFileSystem::GetDocumentsParams struct implementation.
 struct GDataFileSystem::GetDocumentsParams {
   GetDocumentsParams(int start_changestamp,
@@ -619,7 +669,8 @@ struct GDataFileSystem::GetDocumentsParams {
                      const FilePath& search_file_path,
                      const std::string& search_query,
                      const std::string& directory_resource_id,
-                     const FindEntryCallback& callback);
+                     const FindEntryCallback& callback,
+                     GDataFileSystem::GetDocumentsUIState *ui_state);
   ~GetDocumentsParams();
 
   // Changestamps are positive numbers in increasing order. The difference
@@ -636,6 +687,7 @@ struct GDataFileSystem::GetDocumentsParams {
   std::string search_query;
   std::string directory_resource_id;
   FindEntryCallback callback;
+  scoped_ptr<GDataFileSystem::GetDocumentsUIState> ui_state;
 };
 
 GDataFileSystem::GetDocumentsParams::GetDocumentsParams(
@@ -646,7 +698,8 @@ GDataFileSystem::GetDocumentsParams::GetDocumentsParams(
     const FilePath& search_file_path,
     const std::string& search_query,
     const std::string& directory_resource_id,
-    const FindEntryCallback& callback)
+    const FindEntryCallback& callback,
+    GDataFileSystem::GetDocumentsUIState *ui_state)
     : start_changestamp(start_changestamp),
       root_feed_changestamp(root_feed_changestamp),
       feed_list(feed_list),
@@ -654,7 +707,8 @@ GDataFileSystem::GetDocumentsParams::GetDocumentsParams(
       search_file_path(search_file_path),
       search_query(search_query),
       directory_resource_id(directory_resource_id),
-      callback(callback) {
+      callback(callback),
+      ui_state(ui_state) {
 }
 
 GDataFileSystem::GetDocumentsParams::~GetDocumentsParams() {
@@ -1109,7 +1163,8 @@ void GDataFileSystem::LoadFeedFromServer(
                                                     search_file_path,
                                                     search_query,
                                                     directory_resource_id,
-                                                    entry_found_callback)),
+                                                    entry_found_callback,
+                                                    NULL)),
                  start_time));
 }
 
@@ -2849,11 +2904,32 @@ void GDataFileSystem::OnGetDocuments(ContentOrigin initial_origin,
   int num_accumulated_entries = 0;
   for (size_t i = 0; i < params->feed_list->size(); ++i)
     num_accumulated_entries += params->feed_list->at(i)->entries().size();
-  NotifyDocumentFeedFetched(num_accumulated_entries);
 
   // Check if we need to collect more data to complete the directory list.
   if (params->should_fetch_multiple_feeds && has_next_feed_url &&
       !next_feed_url.is_empty()) {
+
+    // Post an UI update event to make the UI smoother.
+    GetDocumentsUIState* ui_state = params->ui_state.get();
+    if (ui_state == NULL) {
+      ui_state = new GetDocumentsUIState(
+          base::TimeTicks::Now(),
+          base::Bind(&GDataFileSystem::OnNotifyDocumentFeedFetched,
+                     ui_weak_ptr_));

[satorux1, 2012/08/02 08:03:14]

I have a feeling that this base::Bind() is unnecessary.

[satorux1, 2012/08/02 08:03:14]

I'm guessing that this callback parameter is unnecessary. see below.

[hidehiko, 2012/08/02 13:47:05]

Done.

[hidehiko, 2012/08/02 13:47:05]

Done.

+      params->ui_state.reset(ui_state);
+    }
+    DCHECK(ui_state != NULL);

[satorux1, 2012/08/02 08:03:14]

remove  != NULL, to be consistent with elsewhere.

[hidehiko, 2012/08/02 13:47:05]

Done.

+
+    if (ui_state->num_remaining_ui_update == 0) {
+      // Currently the UI update is stopped. Start UI periodical callback.
+      MessageLoop::current()->PostTask(
+          FROM_HERE,
+          base::Bind(&GetDocumentsUIState::Run, ui_state->ui_weak_ptr));

[satorux1, 2012/08/02 08:03:14]

Cannot we just do the following here?

 base::Bind(&GDataFileSystem::OnNotifyDocumentFeedFetched, ui_weak_ptr_,
ui_state) 

[hidehiko, 2012/08/02 13:47:05]

Done.

+    }
+    ui_state->num_fetched_documents = num_accumulated_entries;
+    ui_state->num_remaining_ui_update += kNumUiUpdateStepsPerFetch;
+    ui_state->fetched_duration = base::TimeTicks::Now() - start_time;
+
     // Kick of the remaining part of the feeds.
     documents_service_->GetDocuments(
         next_feed_url,
@@ -2873,10 +2949,12 @@ void GDataFileSystem::OnGetDocuments(ContentOrigin initial_origin,
                            params->search_file_path,
                            params->search_query,
                            params->directory_resource_id,
-                           params->callback)),
+                           params->callback,
+                           params->ui_state.release())),
                    start_time));
     return;
   }
+  NotifyDocumentFeedFetched(num_accumulated_entries);
 
   UMA_HISTOGRAM_TIMES("Gdata.EntireFeedLoadTime",
                       base::TimeTicks::Now() - start_time);
@@ -3603,6 +3681,36 @@ void GDataFileSystem::NotifyDocumentFeedFetched(int num_accumulated_entries) {
                     OnDocumentFeedFetched(num_accumulated_entries));
 }
 
+void GDataFileSystem::OnNotifyDocumentFeedFetched(
+    GDataFileSystem::GetDocumentsUIState* ui_state) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  base::TimeDelta consumed_duration =
+      base::TimeTicks::Now() - ui_state->start_time;
+
+  // Split the remaining (i.e. not yet shown on UI) fetched documents evenly
+  // to the remaining updates, cumulate one of them to the showing number of
+  // fetched documents on UI, and notify it to the observers.
+  int update_num_documents =
+      (ui_state->num_fetched_documents - ui_state->num_showing_documents)
+      / ui_state->num_remaining_ui_update;
+  ui_state->num_showing_documents += update_num_documents;
+  NotifyDocumentFeedFetched(ui_state->num_showing_documents);
+
+  --ui_state->num_remaining_ui_update;
+  if (ui_state->num_remaining_ui_update > 0) {
+    // Heuristically, we use fetched time duration to calculate the next
+    // UI update timing.
+    base::TimeDelta remaining_duration =
+        ui_state->fetched_duration - consumed_duration;
+    MessageLoop::current()->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&GetDocumentsUIState::Run, ui_state->ui_weak_ptr),
+        remaining_duration / ui_state->num_remaining_ui_update);
+  }
+}
+
+
 void GDataFileSystem::RunAndNotifyInitialLoadFinished(
     const FindEntryCallback& callback,
     GDataFileError error,