gdata: Remove FindEntryByPathAndRunSync() from GDataWapiFeedLoader.

chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc

Index: chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc
diff --git a/chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc b/chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc
index 569c081de340a15781def5bd99eddd7ec5b971fb..b185a93cb07405c31e42c5f7577f3608e6dd87ee 100644
--- a/chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc
+++ b/chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc
@@ -141,7 +141,7 @@ bool UseLevelDB() {
 LoadRootFeedParams::LoadRootFeedParams(
     FilePath search_file_path,
     bool should_load_from_server,
-    const FindEntryCallback& callback)
+    const FileOperationCallback& callback)
     : search_file_path(search_file_path),
       should_load_from_server(should_load_from_server),
       load_error(GDATA_FILE_OK),
@@ -160,7 +160,7 @@ GetDocumentsParams::GetDocumentsParams(
     const FilePath& search_file_path,
     const std::string& search_query,
     const std::string& directory_resource_id,
-    const FindEntryCallback& callback,
+    const FileOperationCallback& callback,
     GetDocumentsUiState* ui_state)
     : start_changestamp(start_changestamp),
       root_feed_changestamp(root_feed_changestamp),
@@ -238,7 +238,7 @@ void GDataWapiFeedLoader::ReloadFromServerIfNeeded(
     ContentOrigin initial_origin,
     int64 local_changestamp,
     const FilePath& search_file_path,
-    const FindEntryCallback& callback) {
+    const FileOperationCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DVLOG(1) << "ReloadFeedFromServerIfNeeded local_changestamp="
@@ -270,7 +270,7 @@ void GDataWapiFeedLoader::OnGetAccountMetadata(
     ContentOrigin initial_origin,
     int64 local_changestamp,
     const FilePath& search_file_path,
-    const FindEntryCallback& callback,
+    const FileOperationCallback& callback,
     GDataErrorCode status,
     scoped_ptr<base::Value> feed_data) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -338,12 +338,10 @@ void GDataWapiFeedLoader::OnGetAccountMetadata(
     changes_detected = false;
   }
 
-  // No changes detected, continue with search as planned.
+  // No changes detected, tell the client that the loading was successful.
   if (!changes_detected) {
-    if (!callback.is_null()) {
-      directory_service_->FindEntryByPathAndRunSync(search_file_path,
-                                                    callback);
-    }
+    if (!callback.is_null())
+      callback.Run(GDATA_FILE_OK);
     return;
   }
 
@@ -365,7 +363,7 @@ void GDataWapiFeedLoader::OnGetAboutResource(
     ContentOrigin initial_origin,
     int64 local_changestamp,
     const FilePath& search_file_path,
-    const FindEntryCallback& callback,
+    const FileOperationCallback& callback,
     GDataErrorCode status,
     scoped_ptr<base::Value> feed_data) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -422,12 +420,10 @@ void GDataWapiFeedLoader::OnGetAboutResource(
     changes_detected = false;
   }
 
-  // No changes detected, continue with search as planned.
+  // No changes detected, tell the client that the loading was successful.
   if (!changes_detected) {
-    if (!callback.is_null()) {
-      directory_service_->FindEntryByPathAndRunSync(search_file_path,
-                                                    callback);
-    }
+    if (!callback.is_null())
+      callback.Run(GDATA_FILE_OK);
     return;
   }
 
@@ -455,9 +451,10 @@ void GDataWapiFeedLoader::LoadFromServer(
     const std::string& search_query,
     const GURL& feed_to_load,
     const std::string& directory_resource_id,
-    const FindEntryCallback& entry_found_callback,
+    const FileOperationCallback& load_finished_callback,
     const LoadDocumentFeedCallback& feed_load_callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(!feed_load_callback.is_null());
 
   // |feed_list| will contain the list of all collected feed updates that
   // we will receive through calls of DocumentsService::GetDocuments().
@@ -481,7 +478,7 @@ void GDataWapiFeedLoader::LoadFromServer(
                        search_file_path,
                        search_query,
                        directory_resource_id,
-                       entry_found_callback,
+                       load_finished_callback,
                        NULL)),
                    start_time));
     return;
@@ -503,7 +500,7 @@ void GDataWapiFeedLoader::LoadFromServer(
                                                     search_file_path,
                                                     search_query,
                                                     directory_resource_id,
-                                                    entry_found_callback,
+                                                    load_finished_callback,
                                                     NULL)),
                  start_time));
 }
@@ -514,7 +511,7 @@ void GDataWapiFeedLoader::OnFeedFromServerLoaded(GetDocumentsParams* params,
 
   if (error != GDATA_FILE_OK) {
     if (!params->callback.is_null())
-      params->callback.Run(error, NULL);
+      params->callback.Run(error);
     return;
   }
 
@@ -524,7 +521,7 @@ void GDataWapiFeedLoader::OnFeedFromServerLoaded(GetDocumentsParams* params,
 
   if (error != GDATA_FILE_OK) {
     if (!params->callback.is_null())
-      params->callback.Run(error, NULL);
+      params->callback.Run(error);
 
     return;
   }
@@ -532,11 +529,9 @@ void GDataWapiFeedLoader::OnFeedFromServerLoaded(GetDocumentsParams* params,
   // Save file system metadata to disk.
   SaveFileSystem();
 
-  // If we had someone to report this too, then this retrieval was done in a
-  // context of search... so continue search.
+  // Tell the client that the loading was successful.
   if (!params->callback.is_null()) {
-    directory_service_->FindEntryByPathAndRunSync(params->search_file_path,
-                                                  params->callback);
+    params->callback.Run(GDATA_FILE_OK);
   }
 
   FOR_EACH_OBSERVER(Observer, observers_, OnFeedFromServerLoaded());
@@ -550,6 +545,7 @@ void GDataWapiFeedLoader::OnGetDocuments(
     GDataErrorCode status,
     scoped_ptr<base::Value> data) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(!callback.is_null());
 
   if (params->feed_list->empty()) {
     UMA_HISTOGRAM_TIMES("Gdata.InitialFeedLoadTime",
@@ -564,19 +560,14 @@ void GDataWapiFeedLoader::OnGetDocuments(
 
   if (error != GDATA_FILE_OK) {
     directory_service_->set_origin(initial_origin);
-
-    if (!callback.is_null())
-      callback.Run(params, error);
-
+    callback.Run(params, error);
     return;
   }
 
   GURL next_feed_url;
   scoped_ptr<DocumentFeed> current_feed(DocumentFeed::ExtractAndParse(*data));
   if (!current_feed.get()) {
-    if (!callback.is_null()) {
-      callback.Run(params, GDATA_FILE_ERROR_FAILED);
-    }
+    callback.Run(params, GDATA_FILE_ERROR_FAILED);
     return;
   }
   const bool has_next_feed_url = current_feed->GetNextFeedURL(&next_feed_url);
@@ -651,15 +642,15 @@ void GDataWapiFeedLoader::OnGetDocuments(
     return;
   }
 
-  // Notify the observers that a document feed is fetched.
+  // Notify the observers that all document feeds are fetched.
   FOR_EACH_OBSERVER(Observer, observers_,
                     OnDocumentFeedFetched(num_accumulated_entries));
 
   UMA_HISTOGRAM_TIMES("Gdata.EntireFeedLoadTime",
                       base::TimeTicks::Now() - start_time);
 
-  if (!callback.is_null())
-    callback.Run(params, error);
+  // Run the callback so the client can process the retrieved feeds.
+  callback.Run(params, error);
 }
 
 void GDataWapiFeedLoader::OnGetChangelist(
@@ -670,6 +661,7 @@ void GDataWapiFeedLoader::OnGetChangelist(
     GDataErrorCode status,
     scoped_ptr<base::Value> data) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(!callback.is_null());
 
   if (params->feed_list->empty()) {
     UMA_HISTOGRAM_TIMES("Drive.InitialFeedLoadTime",
@@ -684,19 +676,14 @@ void GDataWapiFeedLoader::OnGetChangelist(
 
   if (error != GDATA_FILE_OK) {
     directory_service_->set_origin(initial_origin);
-
-    if (!callback.is_null())
-      callback.Run(params, error);
-
+    callback.Run(params, error);
     return;
   }
 
   GURL next_feed_url;
   scoped_ptr<ChangeList> current_feed(ChangeList::CreateFrom(*data));
   if (!current_feed.get()) {
-    if (!callback.is_null()) {
-      callback.Run(params, GDATA_FILE_ERROR_FAILED);
-    }
+    callback.Run(params, GDATA_FILE_ERROR_FAILED);
     return;
   }
   const bool has_next_feed = !current_feed->next_page_token().empty();
@@ -771,15 +758,15 @@ void GDataWapiFeedLoader::OnGetChangelist(
     return;
   }
 
-  // Notify the observers that a document feed is fetched.
+  // Notify the observers that all document feeds are fetched.
   FOR_EACH_OBSERVER(Observer, observers_,
                     OnDocumentFeedFetched(num_accumulated_entries));
 
   UMA_HISTOGRAM_TIMES("Drive.EntireFeedLoadTime",
                       base::TimeTicks::Now() - start_time);
 
-  if (!callback.is_null())
-    callback.Run(params, error);
+  // Run the callback so the client can process the retrieved feeds.
+  callback.Run(params, error);
 }
 
 void GDataWapiFeedLoader::OnNotifyDocumentFeedFetched(
@@ -822,7 +809,7 @@ void GDataWapiFeedLoader::OnNotifyDocumentFeedFetched(
 void GDataWapiFeedLoader::LoadFromCache(
     bool should_load_from_server,
     const FilePath& search_file_path,
-    const FindEntryCallback& callback) {
+    const FileOperationCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   LoadRootFeedParams* params = new LoadRootFeedParams(search_file_path,
@@ -878,13 +865,13 @@ void GDataWapiFeedLoader::ContinueWithInitializedDirectoryService(
            << (base::Time::Now() - params->load_start_time).InMilliseconds()
            << " milliseconds";
 
-  FindEntryCallback callback = params->callback;
-  // If we got feed content from cache, try search over it.
+  FileOperationCallback callback = params->callback;

[achuithb, 2012/08/14 23:57:36]

This callback code is a bit difficult to follow.

We make a copy of params->callback, then we may Run + Reset it, and then we pass
along the copy (not the original params->callback) to ReloadFromServerIfNeeded.
It would be nice if we didn't have callback and params->callback be different.

WDYT? Don't have to fix this here if you don't want to.

[achuithb, 2012/08/15 00:13:15]

ping

[satorux1, 2012/08/15 00:26:29]

Oops, missed this. Fully agree it's hard to follow. Let me clean this up in a
separate patch

[achuithb, 2012/08/15 00:42:33]

Could you please add a TODO in this CL?

[satorux1, 2012/08/15 01:26:23]

Done.

+  // If we got feed content from cache, tell the client that the loading was
+  // successful.
   if (error == GDATA_FILE_OK && !callback.is_null()) {
-    // Continue file content search operation if the delegate hasn't terminated
-    // this search branch already.
-    directory_service_->FindEntryByPathAndRunSync(params->search_file_path,
-                                                  callback);
+    callback.Run(GDATA_FILE_OK);
+    // Reset the callback so we don't run the same callback once
+    // ReloadFeedFromServerIfNeeded() is complete.
     callback.Reset();
   }