[USS] Move GetAllNodes from backend to controller

components/browser_sync/browser/profile_sync_service.cc

Index: components/browser_sync/browser/profile_sync_service.cc
diff --git a/components/browser_sync/browser/profile_sync_service.cc b/components/browser_sync/browser/profile_sync_service.cc
index a23b37b70282cccb5645fae0277583df6e174878..7862009699043bb79d95cb4f8f02398d49bda3f9 100644
--- a/components/browser_sync/browser/profile_sync_service.cc
+++ b/components/browser_sync/browser/profile_sync_service.cc
@@ -20,6 +20,7 @@
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/metrics/histogram.h"
 #include "base/profiler/scoped_tracker.h"
@@ -61,6 +62,7 @@
 #include "components/sync/driver/backend_migrator.h"
 #include "components/sync/driver/change_processor.h"
 #include "components/sync/driver/data_type_controller.h"
+#include "components/sync/driver/directory_data_type_controller.h"
 #include "components/sync/driver/glue/chrome_report_unrecoverable_error.h"
 #include "components/sync/driver/glue/sync_backend_host.h"
 #include "components/sync/driver/glue/sync_backend_host_impl.h"
@@ -81,6 +83,7 @@
 #include "components/sync/js/js_event_details.h"
 #include "components/sync/protocol/sync.pb.h"
 #include "components/sync/syncable/directory.h"
+#include "components/sync/syncable/syncable_read_transaction.h"
 #include "components/sync_sessions/favicon_cache.h"
 #include "components/sync_sessions/session_data_type_controller.h"
 #include "components/sync_sessions/sessions_sync_manager.h"
@@ -2199,9 +2202,8 @@ class GetAllNodesRequestHelper
       syncer::ModelTypeSet requested_types,
       const base::Callback<void(std::unique_ptr<base::ListValue>)>& callback);
 
-  void OnReceivedNodesForTypes(
-      const std::vector<syncer::ModelType>& types,
-      ScopedVector<base::ListValue> scoped_node_lists);
+  void OnReceivedNodesForType(const syncer::ModelType type,
+                              std::unique_ptr<base::ListValue> node_list);
 
  private:
   friend class base::RefCountedThreadSafe<GetAllNodesRequestHelper>;
@@ -2228,34 +2230,19 @@ GetAllNodesRequestHelper::~GetAllNodesRequestHelper() {
   }
 }
 
-// Called when the set of nodes for a type or set of types has been returned.
-//
-// The nodes for several types can be returned at the same time by specifying
-// their types in the |types| array, and putting their results at the
-// correspnding indices in the |scoped_node_lists|.
-void GetAllNodesRequestHelper::OnReceivedNodesForTypes(
-    const std::vector<syncer::ModelType>& types,
-    ScopedVector<base::ListValue> scoped_node_lists) {
-  DCHECK_EQ(types.size(), scoped_node_lists.size());
+// Called when the set of nodes for a type has been returned.
+// Only return one type of nodes each time.
+void GetAllNodesRequestHelper::OnReceivedNodesForType(
+    const syncer::ModelType type,
+    std::unique_ptr<base::ListValue> node_list) {
+  // Add these results to our list.
+  std::unique_ptr<base::DictionaryValue> type_dict(new base::DictionaryValue());
+  type_dict->SetString("type", ModelTypeToString(type));
+  type_dict->Set("nodes", std::move(node_list));
+  result_accumulator_->Append(std::move(type_dict));
 
-  // Take unsafe ownership of the node list.
-  std::vector<base::ListValue*> node_lists;
-  scoped_node_lists.release(&node_lists);
-
-  for (size_t i = 0; i < node_lists.size() && i < types.size(); ++i) {
-    const ModelType type = types[i];
-    base::ListValue* node_list = node_lists[i];
-
-    // Add these results to our list.
-    std::unique_ptr<base::DictionaryValue> type_dict(
-        new base::DictionaryValue());
-    type_dict->SetString("type", ModelTypeToString(type));
-    type_dict->Set("nodes", node_list);
-    result_accumulator_->Append(std::move(type_dict));
-
-    // Remember that this part of the request is satisfied.
-    awaiting_types_.Remove(type);
-  }
+  // Remember that this part of the request is satisfied.
+  awaiting_types_.Remove(type);
 
   if (awaiting_types_.Empty()) {
     callback_.Run(std::move(result_accumulator_));
@@ -2275,17 +2262,14 @@ void ProfileSyncService::GetAllNodes(
 
   if (!backend_initialized_) {
     // If there's no backend available to fulfill the request, handle it here.
-    ScopedVector<base::ListValue> empty_results;
-    std::vector<ModelType> type_vector;
     for (ModelTypeSet::Iterator it = all_types.First(); it.Good(); it.Inc()) {
-      type_vector.push_back(it.Get());
-      empty_results.push_back(new base::ListValue());
+      helper->OnReceivedNodesForType(it.Get(),
+                                     base::WrapUnique(new base::ListValue()));

[maxbogue, 2016/08/29 21:21:16]

nit: in this case you could just use base::MakeUnique<base::ListValue>().

[Gang Wu, 2016/08/30 20:56:01]

Done.

     }
-    helper->OnReceivedNodesForTypes(type_vector, std::move(empty_results));
   } else {
-    backend_->GetAllNodesForTypes(
+    GetAllNodesForTypes(

[maxbogue, 2016/08/29 21:21:16]

I'm all for modularity, but GetAllNodesForTypes doesn't have any use outside
this function. I'd prefer you just make the !backend_initialized_ case return at
the end, and then insert the GetAllNodesForTypes code here.

[Gang Wu, 2016/08/30 20:56:01]

Done.

         all_types,
-        base::Bind(&GetAllNodesRequestHelper::OnReceivedNodesForTypes, helper));
+        base::Bind(&GetAllNodesRequestHelper::OnReceivedNodesForType, helper));
   }
 }
 
@@ -2540,3 +2524,22 @@ void ProfileSyncService::OnSetupInProgressHandleDestroyed() {
     ReconfigureDatatypeManager();
   NotifyObservers();
 }
+
+void ProfileSyncService::GetAllNodesForTypes(
+    syncer::ModelTypeSet types,
+    base::Callback<void(const syncer::ModelType,
+                        std::unique_ptr<base::ListValue>)> callback) {
+  for (syncer::ModelTypeSet::Iterator it = types.First(); it.Good(); it.Inc()) {
+    DataTypeController::TypeMap::const_iterator iter =

[maxbogue, 2016/08/29 21:21:16]

This is really the sort of place it's great to use auto. Or const auto or
whichever works here. Also I'd call it "dtc_iter" to distinguish from "it".

[Gang Wu, 2016/08/30 20:56:01]

Done.

+        data_type_controllers_.find(it.Get());
+    if (iter != data_type_controllers_.end()) {
+      iter->second->GetAllNodes(callback);
+    } else {
+      // Control Types
+      callback.Run(it.Get(),
+                   sync_driver::DirectoryDataTypeController::
+                       GetAllNodesForTypeFromDirectory(
+                           it.Get(), GetUserShare()->directory.get()));
+    }
+  }
+}