Update the number of fetched files more frequently.

chrome/browser/chromeos/gdata/gdata_file_system.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/gdata/gdata_file_system.h"
 
 #include <set>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 37 matching lines @@
 
 const char kEmptyFilePath[] = "/dev/null";
 
 // GData update check interval (in seconds).
 #ifndef NDEBUG
 const int kGDataUpdateCheckIntervalInSec = 5;
 #else
 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 {
   double size;
   int timeout;
 } SerializationTimetable;
 
 SerializationTimetable kSerializeTimetable[] = {
 #ifndef NDEBUG
     {0.5, 0},    // Less than 0.5MB, dump immediately.
@@ skipping 535 matching lines @@
     const GetEntryInfoWithFilePathCallback& callback,
     const FilePath& path,
     GDataFileError error,
     scoped_ptr<GDataEntryProto> entry_proto) {
   if (!callback.is_null())
     callback.Run(error, path, entry_proto.Pass());
 }
 
 }  // 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,
                      int root_feed_changestamp,
                      std::vector<DocumentFeed*>* feed_list,
                      bool should_fetch_multiple_feeds,
                      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
   // between two changestamps is proportional equal to number of items in
   // delta feed between them - bigger the difference, more likely bigger
   // number of items in delta feeds.
   int start_changestamp;
   int root_feed_changestamp;
   scoped_ptr<std::vector<DocumentFeed*> > feed_list;
   // Should we stop after getting first feed chunk, even if there is more
   // data.
   bool should_fetch_multiple_feeds;
   FilePath search_file_path;
   std::string search_query;
   std::string directory_resource_id;
   FindEntryCallback callback;
+  scoped_ptr<GDataFileSystem::GetDocumentsUiState> ui_state;
 };
 
 GDataFileSystem::GetDocumentsParams::GetDocumentsParams(
     int start_changestamp,
     int root_feed_changestamp,
     std::vector<DocumentFeed*>* feed_list,
     bool should_fetch_multiple_feeds,
     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),
       should_fetch_multiple_feeds(should_fetch_multiple_feeds),
       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() {
   STLDeleteElements(feed_list.get());
 }
 
 // GDataFileSystem::CreateDirectoryParams struct implementation.
 struct GDataFileSystem::CreateDirectoryParams {
   CreateDirectoryParams(const FilePath& created_directory_path,
                         const FilePath& target_directory_path,
@@ skipping 434 matching lines @@
                  ui_weak_ptr_,
                  initial_origin,
                  feed_load_callback,
                  base::Owned(new GetDocumentsParams(start_changestamp,
                                                     root_feed_changestamp,
                                                     feed_list.release(),
                                                     should_fetch_multiple_feeds,
                                                     search_file_path,
                                                     search_query,
                                                     directory_resource_id,
-                                                    entry_found_callback)),
+                                                    entry_found_callback,
+                                                    NULL)),
                  start_time));
 }
 
 void GDataFileSystem::OnFeedFromServerLoaded(GetDocumentsParams* params,
                                              GDataFileError error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (error != GDATA_FILE_OK) {
     if (!params->callback.is_null())
       params->callback.Run(error, NULL);
@@ skipping 1719 matching lines @@
                  base::Passed(&data)));
 #endif
 
   // Add the current feed to the list of collected feeds for this directory.
   params->feed_list->push_back(current_feed.release());
 
   // Compute and notify the number of entries fetched so far.
   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,
         params->start_changestamp,
         params->search_query,
         params->directory_resource_id,
         base::Bind(&GDataFileSystem::OnGetDocuments,
                    ui_weak_ptr_,
                    initial_origin,
                    callback,
                    base::Owned(
                        new GetDocumentsParams(
                            params->start_changestamp,
                            params->root_feed_changestamp,
                            params->feed_list.release(),
                            params->should_fetch_multiple_feeds,
                            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);
 
   if (!callback.is_null())
     callback.Run(params, error);
 }
 
 void GDataFileSystem::LoadRootFeedFromCache(
     bool should_load_from_server,
@@ skipping 706 matching lines @@
 
 void GDataFileSystem::NotifyDocumentFeedFetched(int num_accumulated_entries) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DVLOG(1) << "Document feed fetched: " << num_accumulated_entries;
   // Notify the observers that a document feed is fetched.
   FOR_EACH_OBSERVER(Observer, observers_,
                     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,
     GDataEntry* entry) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DVLOG(1) << "Initial load finished";
   if (!callback.is_null())
     callback.Run(error, entry);
 
@@ skipping 651 matching lines @@
   }
 
   PlatformFileInfoProto entry_file_info;
   GDataEntry::ConvertPlatformFileInfoToProto(*file_info, &entry_file_info);
   *entry_proto->mutable_file_info() = entry_file_info;
   if (!callback.is_null())
     callback.Run(GDATA_FILE_OK, entry_proto.Pass());
 }
 
 }  // namespace gdata