[Sync] Speed up sync node browser/search in about:sync

chrome/browser/sync/engine/syncapi_unittest.cc

 // Copyright (c) 2011 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.
 
 // Unit tests for the SyncApi. Note that a lot of the underlying
 // functionality is provided by the Syncable layer, which has its own
 // unit tests. We'll test SyncApi specific things in this harness.
 
 #include <map>
 
@@ skipping 370 matching lines @@
     EXPECT_TRUE(password_node.InitByClientTagLookup(syncable::PASSWORDS,
                                                     "foo"));
     const sync_pb::PasswordSpecificsData& data =
         password_node.GetPasswordSpecifics();
     EXPECT_EQ("secret", data.password_value());
   }
 }
 
 namespace {
 
-void CheckNodeValue(const BaseNode& node, const DictionaryValue& value) {
+void CheckNodeValue(const BaseNode& node, const DictionaryValue& value,
+                    bool is_detailed) {
   ExpectInt64Value(node.GetId(), value, "id");
-  ExpectInt64Value(node.GetModificationTime(), value, "modificationTime");
-  ExpectInt64Value(node.GetParentId(), value, "parentId");
   {
     bool is_folder = false;
     EXPECT_TRUE(value.GetBoolean("isFolder", &is_folder));
     EXPECT_EQ(node.GetIsFolder(), is_folder);
   }
   ExpectDictStringValue(WideToUTF8(node.GetTitle()), value, "title");
   {
     ModelType expected_model_type = node.GetModelType();
     std::string type_str;
     EXPECT_TRUE(value.GetString("type", &type_str));
     if (expected_model_type >= syncable::FIRST_REAL_MODEL_TYPE) {
       ModelType model_type =
           syncable::ModelTypeFromString(type_str);
       EXPECT_EQ(expected_model_type, model_type);
     } else if (expected_model_type == syncable::TOP_LEVEL_FOLDER) {
       EXPECT_EQ("Top-level folder", type_str);
     } else if (expected_model_type == syncable::UNSPECIFIED) {
       EXPECT_EQ("Unspecified", type_str);
     } else {
       ADD_FAILURE();
     }
   }
-  ExpectInt64Value(node.GetExternalId(), value, "externalId");
-  ExpectInt64Value(node.GetPredecessorId(), value, "predecessorId");
-  ExpectInt64Value(node.GetSuccessorId(), value, "successorId");
-  ExpectInt64Value(node.GetFirstChildId(), value, "firstChildId");
-  {
-    scoped_ptr<DictionaryValue> expected_entry(node.GetEntry()->ToValue());
-    Value* entry = NULL;
-    EXPECT_TRUE(value.Get("entry", &entry));
-    EXPECT_TRUE(Value::Equals(entry, expected_entry.get()));
+  if (is_detailed) {
+    ExpectInt64Value(node.GetParentId(), value, "parentId");
+    ExpectInt64Value(node.GetModificationTime(), value, "modificationTime");
+    ExpectInt64Value(node.GetExternalId(), value, "externalId");
+    ExpectInt64Value(node.GetPredecessorId(), value, "predecessorId");
+    ExpectInt64Value(node.GetSuccessorId(), value, "successorId");
+    ExpectInt64Value(node.GetFirstChildId(), value, "firstChildId");
+    {
+      scoped_ptr<DictionaryValue> expected_entry(node.GetEntry()->ToValue());
+      Value* entry = NULL;
+      EXPECT_TRUE(value.Get("entry", &entry));
+      EXPECT_TRUE(Value::Equals(entry, expected_entry.get()));
+    }
+    EXPECT_EQ(11u, value.size());
+  } else {
+    EXPECT_EQ(4u, value.size());
   }
-  EXPECT_EQ(11u, value.size());
 }
 
 }  // namespace
 
-TEST_F(SyncApiTest, BaseNodeToValue) {
+TEST_F(SyncApiTest, BaseNodeGetSummaryAsValue) {
   ReadTransaction trans(test_user_share_.user_share());
   ReadNode node(&trans);
   node.InitByRootLookup();
-  scoped_ptr<DictionaryValue> value(node.ToValue());
-  if (value.get()) {
-    CheckNodeValue(node, *value);
+  scoped_ptr<DictionaryValue> details(node.GetSummaryAsValue());
+  if (details.get()) {
+    CheckNodeValue(node, *details, false);
   } else {
     ADD_FAILURE();
   }
+}
+
+TEST_F(SyncApiTest, BaseNodeGetDetailsAsValue) {
+  ReadTransaction trans(test_user_share_.user_share());
+  ReadNode node(&trans);
+  node.InitByRootLookup();
+  scoped_ptr<DictionaryValue> details(node.GetDetailsAsValue());
+  if (details.get()) {
+    CheckNodeValue(node, *details, true);
+  } else {
+    ADD_FAILURE();
+  }
 }
 
 namespace {
 
 void ExpectChangeRecordActionValue(SyncManager::ChangeRecord::Action
                                        expected_value,
                                    const DictionaryValue& value,
                                    const std::string& key) {
   std::string str_value;
   EXPECT_TRUE(value.GetString(key, &str_value));
@@ skipping 14 matching lines @@
 }
 
 void CheckNonDeleteChangeRecordValue(const SyncManager::ChangeRecord& record,
                                      const DictionaryValue& value,
                                      BaseTransaction* trans) {
   EXPECT_NE(SyncManager::ChangeRecord::ACTION_DELETE, record.action);
   ExpectChangeRecordActionValue(record.action, value, "action");
   {
     ReadNode node(trans);
     EXPECT_TRUE(node.InitByIdLookup(record.id));
-    scoped_ptr<DictionaryValue> expected_node_value(node.ToValue());
-    ExpectDictDictionaryValue(*expected_node_value, value, "node");
+    scoped_ptr<DictionaryValue> expected_details(node.GetDetailsAsValue());
+    ExpectDictDictionaryValue(*expected_details, value, "node");
   }
 }
 
 void CheckDeleteChangeRecordValue(const SyncManager::ChangeRecord& record,
                                   const DictionaryValue& value) {
   EXPECT_EQ(SyncManager::ChangeRecord::ACTION_DELETE, record.action);
   ExpectChangeRecordActionValue(record.action, value, "action");
   DictionaryValue* node_value = NULL;
   EXPECT_TRUE(value.GetDictionary("node", &node_value));
   if (node_value) {
@@ skipping 342 matching lines @@
   // dropped.
   {
     StrictMock<MockJsEventHandler> event_handler;
     js_backend->ProcessMessage("unknownMessage",
                                kNoArgs, &event_handler);
     js_backend->ProcessMessage("getNotificationState",
                                kNoArgs, &event_handler);
   }
 }
 
-TEST_F(SyncManagerTest, ProcessMessageGetRootNode) {
+TEST_F(SyncManagerTest, ProcessMessageGetRootNodeDetails) {
   const JsArgList kNoArgs;
 
   browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend();
 
   StrictMock<MockJsEventHandler> event_handler;
   StrictMock<MockJsEventRouter> event_router;
 
   JsArgList return_args;
 
   EXPECT_CALL(event_router,
-              RouteJsMessageReply("getRootNode", _, &event_handler))
+              RouteJsMessageReply("getRootNodeDetails", _, &event_handler))
       .WillOnce(SaveArg<1>(&return_args));
 
   js_backend->SetParentJsEventRouter(&event_router);
 
   // Should trigger the reply.
-  js_backend->ProcessMessage("getRootNode", kNoArgs, &event_handler);
+  js_backend->ProcessMessage("getRootNodeDetails", kNoArgs, &event_handler);
 
   EXPECT_EQ(1u, return_args.Get().GetSize());
   DictionaryValue* node_info = NULL;
   EXPECT_TRUE(return_args.Get().GetDictionary(0, &node_info));
   if (node_info) {
     ReadTransaction trans(sync_manager_.GetUserShare());
     ReadNode node(&trans);
     node.InitByRootLookup();
-    CheckNodeValue(node, *node_info);
+    CheckNodeValue(node, *node_info, true);
   } else {
     ADD_FAILURE();
   }
 
   js_backend->RemoveParentJsEventRouter();
 }
 
 void CheckGetNodesByIdReturnArgs(const SyncManager& sync_manager,
                                  const JsArgList& return_args,
-                                 int64 id) {
+                                 int64 id,
+                                 bool is_detailed) {
   EXPECT_EQ(1u, return_args.Get().GetSize());
   ListValue* nodes = NULL;
   ASSERT_TRUE(return_args.Get().GetList(0, &nodes));
   ASSERT_TRUE(nodes);
   EXPECT_EQ(1u, nodes->GetSize());
   DictionaryValue* node_info = NULL;
   EXPECT_TRUE(nodes->GetDictionary(0, &node_info));
   ASSERT_TRUE(node_info);
   ReadTransaction trans(sync_manager.GetUserShare());
   ReadNode node(&trans);
   node.InitByIdLookup(id);
-  CheckNodeValue(node, *node_info);
+  CheckNodeValue(node, *node_info, is_detailed);
 }
 
-TEST_F(SyncManagerTest, ProcessMessageGetNodesById) {
-  int64 child_id =
-      MakeNode(sync_manager_.GetUserShare(), syncable::BOOKMARKS, "testtag");
+class SyncManagerGetNodesByIdTest : public SyncManagerTest {
+ protected:
+  virtual ~SyncManagerGetNodesByIdTest() {}
 
+  void RunGetNodesByIdTest(const char* message_name, bool is_detailed) {
+    int64 child_id =
+        MakeNode(sync_manager_.GetUserShare(),
+                 syncable::BOOKMARKS, "testtag");
+
+    browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend();
+
+    StrictMock<MockJsEventHandler> event_handler;
+    StrictMock<MockJsEventRouter> event_router;
+
+    JsArgList return_args;
+
+    EXPECT_CALL(event_router,
+                RouteJsMessageReply(message_name, _, &event_handler))
+        .Times(2).WillRepeatedly(SaveArg<1>(&return_args));
+
+    js_backend->SetParentJsEventRouter(&event_router);
+
+    // Should trigger the reply.
+    {
+      ListValue args;
+      ListValue* ids = new ListValue();
+      args.Append(ids);
+      ids->Append(Value::CreateStringValue("1"));

[lipalani1, 2011/06/02 18:51:52]

shouldnt this be child_id?

[akalin, 2011/06/02 22:13:33]

It should be the id of the root, actually.  Refactored test to make it more
obvious.

+      js_backend->ProcessMessage(message_name,
+                                 JsArgList(&args), &event_handler);
+    }
+
+    CheckGetNodesByIdReturnArgs(sync_manager_, return_args, 1, is_detailed);
+
+    // Should trigger another reply.
+    {
+      ListValue args;
+      ListValue* ids = new ListValue();
+      args.Append(ids);
+      ids->Append(Value::CreateStringValue(base::Int64ToString(child_id)));
+      js_backend->ProcessMessage(message_name,
+                                 JsArgList(&args), &event_handler);
+    }
+
+    CheckGetNodesByIdReturnArgs(sync_manager_, return_args, child_id,
+                                is_detailed);
+
+    js_backend->RemoveParentJsEventRouter();
+  }
+
+  void RunGetNodesByIdFailureTest(const char* message_name) {
+    browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend();
+
+    StrictMock<MockJsEventHandler> event_handler;
+    StrictMock<MockJsEventRouter> event_router;
+
+    ListValue empty_list_args;
+    empty_list_args.Append(new ListValue());
+
+    EXPECT_CALL(event_router,
+                RouteJsMessageReply(message_name,
+                                    HasArgsAsList(empty_list_args),
+                                    &event_handler))
+        .Times(6);
+
+    js_backend->SetParentJsEventRouter(&event_router);
+
+    {
+      ListValue args;
+      js_backend->ProcessMessage(message_name,
+                                 JsArgList(&args), &event_handler);
+    }
+
+    {
+      ListValue args;
+      args.Append(new ListValue());
+      js_backend->ProcessMessage(message_name,
+                                 JsArgList(&args), &event_handler);
+    }
+
+    {
+      ListValue args;
+      ListValue* ids = new ListValue();
+      args.Append(ids);
+      ids->Append(Value::CreateStringValue(""));
+      js_backend->ProcessMessage(message_name,
+                                 JsArgList(&args), &event_handler);
+    }
+
+    {
+      ListValue args;
+      ListValue* ids = new ListValue();
+      args.Append(ids);
+      ids->Append(Value::CreateStringValue("nonsense"));
+      js_backend->ProcessMessage(message_name,
+                                 JsArgList(&args), &event_handler);
+    }
+
+    {
+      ListValue args;
+      ListValue* ids = new ListValue();
+      args.Append(ids);
+      ids->Append(Value::CreateStringValue("0"));
+      js_backend->ProcessMessage(message_name,
+                                 JsArgList(&args), &event_handler);
+    }
+
+    {
+      ListValue args;
+      ListValue* ids = new ListValue();
+      args.Append(ids);
+      ids->Append(Value::CreateStringValue("9999"));
+      js_backend->ProcessMessage(message_name,
+                                 JsArgList(&args), &event_handler);
+    }
+
+    js_backend->RemoveParentJsEventRouter();
+  }
+};
+
+TEST_F(SyncManagerGetNodesByIdTest, GetNodeSummariesById) {
+  RunGetNodesByIdTest("getNodeSummariesById", false);
+}
+
+TEST_F(SyncManagerGetNodesByIdTest, GetNodeDetailsById) {
+  RunGetNodesByIdTest("getNodeDetailsById", true);
+}
+
+TEST_F(SyncManagerGetNodesByIdTest, GetNodeSummariesByIdFailure) {
+  RunGetNodesByIdFailureTest("getNodeSummariesById");
+}
+
+TEST_F(SyncManagerGetNodesByIdTest, GetNodeDetailsByIdFailure) {
+  RunGetNodesByIdFailureTest("getNodeDetailsById");
+}
+
+TEST_F(SyncManagerTest, GetChildNodeIds) {
   browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend();
 
   StrictMock<MockJsEventHandler> event_handler;
-  StrictMock<MockJsEventRouter> event_router;
-
-  JsArgList return_args;
-
-  EXPECT_CALL(event_router,
-              RouteJsMessageReply("getNodesById", _, &event_handler))
-      .Times(2).WillRepeatedly(SaveArg<1>(&return_args));
-
-  js_backend->SetParentJsEventRouter(&event_router);
-
-  // Should trigger the reply.
-  {
-    ListValue args;
-    ListValue* ids = new ListValue();
-    args.Append(ids);
-    ids->Append(Value::CreateStringValue("1"));
-    js_backend->ProcessMessage("getNodesById",
-                               JsArgList(&args), &event_handler);
-  }
-
-  CheckGetNodesByIdReturnArgs(sync_manager_, return_args, 1);
-
-  // Should trigger another reply.
-  {
-    ListValue args;
-    ListValue* ids = new ListValue();
-    args.Append(ids);
-    ids->Append(Value::CreateStringValue(base::Int64ToString(child_id)));
-    js_backend->ProcessMessage("getNodesById",
-                               JsArgList(&args), &event_handler);
-  }
-
-  CheckGetNodesByIdReturnArgs(sync_manager_, return_args, child_id);
-
-  js_backend->RemoveParentJsEventRouter();
-}
-
-TEST_F(SyncManagerTest, ProcessMessageGetNodesByIdFailure) {
-  browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend();
-
-  StrictMock<MockJsEventHandler> event_handler;
-  StrictMock<MockJsEventRouter> event_router;
-
-  ListValue empty_list_args;
-  empty_list_args.Append(new ListValue());
-
-  EXPECT_CALL(event_router,
-              RouteJsMessageReply("getNodesById",
-                                  HasArgsAsList(empty_list_args),
-                                  &event_handler))
-      .Times(6);
-
-  js_backend->SetParentJsEventRouter(&event_router);
-
-  {
-    ListValue args;
-    js_backend->ProcessMessage("getNodesById",
-                               JsArgList(&args), &event_handler);
-  }
-
-  {
-    ListValue args;
-    args.Append(new ListValue());
-    js_backend->ProcessMessage("getNodesById",
-                               JsArgList(&args), &event_handler);
-  }
-
-  {
-    ListValue args;
-    ListValue* ids = new ListValue();
-    args.Append(ids);
-    ids->Append(Value::CreateStringValue(""));
-    js_backend->ProcessMessage("getNodesById",
-                               JsArgList(&args), &event_handler);
-  }
-
-  {
-    ListValue args;
-    ListValue* ids = new ListValue();
-    args.Append(ids);
-    ids->Append(Value::CreateStringValue("nonsense"));
-    js_backend->ProcessMessage("getNodesById",
-                               JsArgList(&args), &event_handler);
-  }
-
-  {
-    ListValue args;
-    ListValue* ids = new ListValue();
-    args.Append(ids);
-    ids->Append(Value::CreateStringValue("0"));
-    js_backend->ProcessMessage("getNodesById",
-                               JsArgList(&args), &event_handler);
-  }
-
-  {
-    ListValue args;
-    ListValue* ids = new ListValue();
-    args.Append(ids);
-    ids->Append(Value::CreateStringValue("9999"));
-    js_backend->ProcessMessage("getNodesById",
-                               JsArgList(&args), &event_handler);
-  }
-
-  js_backend->RemoveParentJsEventRouter();
-}
-
-TEST_F(SyncManagerTest, ProcessMessageGetChildNodeIds) {
-  browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend();
-
-  StrictMock<MockJsEventHandler> event_handler;
   StrictMock<MockJsEventRouter> event_router;
 
   JsArgList return_args;
 
   EXPECT_CALL(event_router,
               RouteJsMessageReply("getChildNodeIds", _, &event_handler))
       .Times(1).WillRepeatedly(SaveArg<1>(&return_args));
 
   js_backend->SetParentJsEventRouter(&event_router);
 
   // Should trigger the reply.
   {
     ListValue args;
     args.Append(Value::CreateStringValue("1"));
     js_backend->ProcessMessage("getChildNodeIds",
                                JsArgList(&args), &event_handler);
   }
 
   EXPECT_EQ(1u, return_args.Get().GetSize());
   ListValue* nodes = NULL;
   ASSERT_TRUE(return_args.Get().GetList(0, &nodes));
   ASSERT_TRUE(nodes);
   EXPECT_EQ(5u, nodes->GetSize());
 
   js_backend->RemoveParentJsEventRouter();
 }
 
-TEST_F(SyncManagerTest, ProcessMessageGetChildNodeIdsFailure) {
+TEST_F(SyncManagerTest, GetChildNodeIdsFailure) {
   browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend();
 
   StrictMock<MockJsEventHandler> event_handler;
   StrictMock<MockJsEventRouter> event_router;
 
   ListValue empty_list_args;
   empty_list_args.Append(new ListValue());
 
   EXPECT_CALL(event_router,
               RouteJsMessageReply("getChildNodeIds",
@@ skipping 33 matching lines @@
   {
     ListValue args;
     args.Append(Value::CreateStringValue("9999"));
     js_backend->ProcessMessage("getChildNodeIds",
                                JsArgList(&args), &event_handler);
   }
 
   js_backend->RemoveParentJsEventRouter();
 }
 
+// TODO(akalin): Add unit tests for findNodesContainingString message.
+
 TEST_F(SyncManagerTest, OnNotificationStateChange) {
   InSequence dummy;
   StrictMock<MockJsEventRouter> event_router;
 
   DictionaryValue true_details;
   true_details.SetBoolean("enabled", true);
   DictionaryValue false_details;
   false_details.SetBoolean("enabled", false);
 
   EXPECT_CALL(event_router,
@@ skipping 136 matching lines @@
                                                    true /* is encrypted */));
     EXPECT_TRUE(syncable::VerifyDataTypeEncryption(trans.GetWrappedTrans(),
                                                    syncable::THEMES,
                                                    false /* not encrypted */));
   }
 }
 
 }  // namespace
 
 }  // namespace browser_sync