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..b22ec8fe44bd3e6670df9502cbfa39c3e289bb8a 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 kFetchUiUpdateStep = 10;

[satorux1, 2012/08/02 16:31:03]

Please put some comment like

// Update the fetch progress UI per every this number of feeds. 

[hidehiko, 2012/08/03 09:50:18]

Done.

+
 // Schedule for dumping root file system proto buffers to disk depending its
 // total protobuffer size in MB.
 typedef struct {
@@ -610,6 +612,40 @@ void RunGetEntryInfoWithFilePathCallback(
 
 }  // namespace
 
+// GDataFileSystem::GetDocumentsUiState struct implementation.
+// 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 {
+  explicit GetDocumentsUiState(base::TimeTicks start_time)
+      : num_fetched_documents(0),
+        num_showing_documents(0),
+        start_time(start_time),
+        ui_weak_ptr_factory(this),
+        ui_weak_ptr(ui_weak_ptr_factory.GetWeakPtr()) {
+  }
+
+  // The number of fetched documents.
+  int num_fetched_documents;
+
+  // The number documents shown on UI.
+  int num_showing_documents;
+
+  // When the UI update has started.
+  base::TimeTicks start_time;
+
+  // How long duration the actual fetching takes in total.

[satorux1, 2012/08/02 16:31:03]

What about:

// Time elapsed since the the feed fetching was started.

[hidehiko, 2012/08/03 09:50:18]

Done.

+  // Heuristically, we use the duration to decide the UI update step period.
+  base::TimeDelta fetched_duration;

[satorux1, 2012/08/02 16:31:03]

maybe: feed_fetching_elapsed_time ?

[hidehiko, 2012/08/03 09:50:18]

Done.

+
+  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 +655,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 +673,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 +684,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 +693,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 +1149,8 @@ void GDataFileSystem::LoadFeedFromServer(
                                                     search_file_path,
                                                     search_query,
                                                     directory_resource_id,
-                                                    entry_found_callback)),
+                                                    entry_found_callback,
+                                                    NULL)),
                  start_time));
 }
 
@@ -2849,11 +2890,30 @@ 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());
+      params->ui_state.reset(ui_state);
+    }
+    DCHECK(ui_state);
+
+    if ((ui_state->num_fetched_documents - ui_state->num_showing_documents)
+        < kFetchUiUpdateStep) {
+      // Currently the UI update is stopped. Start UI periodical callback.

[satorux1, 2012/08/02 16:31:03]

periodic

[hidehiko, 2012/08/03 09:50:18]

Done.

+      MessageLoop::current()->PostTask(
+          FROM_HERE,
+          base::Bind(&GDataFileSystem::OnNotifyDocumentFeedFetched,
+                     ui_weak_ptr_, ui_state->ui_weak_ptr));
+    }
+    ui_state->num_fetched_documents = num_accumulated_entries;
+    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 +2933,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 +3665,41 @@ void GDataFileSystem::NotifyDocumentFeedFetched(int num_accumulated_entries) {
                     OnDocumentFeedFetched(num_accumulated_entries));
 }
 
+void GDataFileSystem::OnNotifyDocumentFeedFetched(
+    base::WeakPtr<GDataFileSystem::GetDocumentsUiState> ui_state) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  if (!ui_state) {
+    // The ui state instance is already released, which means the fetching
+    // is done and we don't need to update any more.
+    return;

[satorux1, 2012/08/02 16:31:03]

I think managing lifetime of ui_state using weak ptr is tricky. What about:

1) Make ui_state a member of GDataWapiLoader (sorry I submitted a large
refactoring, and this part is now in a separate class)
2) Create ui_state_ in OnGetDocuments.
3) Delete it in OnGetDocuments, once all feeds are processed.

[hidehiko, 2012/08/02 19:06:33]

It looks slightly danger to me, because;

When OnGetDocuments with all feed fetch done is called, a pending task to call
OnNotifyDocumentFeedFetched may be in the message queue.
In that case, once the ui_state_ would be deleted. However, it is possible to
re-run another data fetching before pending OnNotifyDocumentFeedFetched is run
(I guess it should be quite rare in the real world, though), then ui_state_ will
be re-created and another OnNotifyDocumentFeedFetched task would be enqueued.
We here would need to ignore first OnNofityDocumentFeedFetched invocation
somehow, and to process the second invocation normally (otherwise the callback
sequence would get buggy state). If we make ui_state_ the member of
GDataWapiLoader, it'd be very hard to distinguish the first and second
invocation.

I put GetDocumentUiState into GetDocumentsParams because GetDocumentsParams
holds a data for one whole fetching process and I thought the UI state is also a
part of it.

What do you think?

[satorux1, 2012/08/02 19:37:16]

Are you are right, can we delete it in OnNotifyDocumentFeedFetched when the last
feed is processed?

WeakPtr is tricky so I'd like to minimize use of it.

[hidehiko, 2012/08/02 19:59:06]

No, unfortunately, because it's potential memory leaking.
When GDataWapiLoader is deleted, there may be a pending task to invoke
OnNotifyDocumentFeedFetched. In that case, OnNotifyDocumentFeedFetched wouldn't
be actually invoked, and then delete operation wouldn't be executed.

If we keep ui_state as a member of GDataWapiLoader, there is another (but
probably rare case) issue that; in the scenario in my previous reply, it is
impossible to store the secondary created ui-state instance into ui_state_
field, because there remains the older ui_state instance.

What we actually need here is cancelling the pending OnNotifyDocumentFeedFetched
conceptually, and WeakPtr seems the commonly used implementation of the (pseudo)
cancelling? (Though, I agree WeakPtr looks tricky in general...)

[satorux1, 2012/08/02 21:14:48]

Ah I see. it's complicated. :)

I thought about it again, but could not come up with a good idea. Let's go with
the weak ptr approach for now.

[hidehiko, 2012/08/03 09:50:18]

Done.

+  }
+
+  base::TimeDelta consumed_duration =

[satorux1, 2012/08/02 16:31:03]

maybe: elapsed_time

[hidehiko, 2012/08/03 09:50:18]

Done.

+      base::TimeTicks::Now() - ui_state->start_time;
+
+  if (ui_state->num_showing_documents + kFetchUiUpdateStep <=
+      ui_state->num_fetched_documents) {
+    ui_state->num_showing_documents += kFetchUiUpdateStep;
+    NotifyDocumentFeedFetched(ui_state->num_showing_documents);
+
+    int num_remaining_ui_update =

[satorux1, 2012/08/02 16:31:03]

num_remaining_ui_updates

[hidehiko, 2012/08/03 09:50:18]

Done.

+        (ui_state->num_fetched_documents - ui_state->num_showing_documents)
+        / kFetchUiUpdateStep;
+    if (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(&GDataFileSystem::OnNotifyDocumentFeedFetched,
+                     ui_weak_ptr_, ui_state->ui_weak_ptr),
+          remaining_duration / num_remaining_ui_update);
+    }
+  }
+}
+
 void GDataFileSystem::RunAndNotifyInitialLoadFinished(
     const FindEntryCallback& callback,
     GDataFileError error,