diff options
-rw-r--r-- | chrome/browser/resources/sync_internals/chrome_sync.js | 3 | ||||
-rw-r--r-- | chrome/browser/resources/sync_internals/sync_node_browser.js | 41 | ||||
-rw-r--r-- | chrome/browser/sync/engine/syncapi.cc | 89 | ||||
-rw-r--r-- | chrome/browser/sync/engine/syncapi.h | 28 | ||||
-rw-r--r-- | chrome/browser/sync/engine/syncapi_unittest.cc | 177 |
5 files changed, 263 insertions, 75 deletions
diff --git a/chrome/browser/resources/sync_internals/chrome_sync.js b/chrome/browser/resources/sync_internals/chrome_sync.js index de1e292..e0eb4cf 100644 --- a/chrome/browser/resources/sync_internals/chrome_sync.js +++ b/chrome/browser/resources/sync_internals/chrome_sync.js @@ -140,7 +140,8 @@ var syncFunctions = [ // Node lookup functions. 'getRootNode', - 'getNodeById', + 'getNodesById', + 'getChildNodeIds', 'findNodesContainingString' ]; diff --git a/chrome/browser/resources/sync_internals/sync_node_browser.js b/chrome/browser/resources/sync_internals/sync_node_browser.js index 4939337..3ce4469 100644 --- a/chrome/browser/resources/sync_internals/sync_node_browser.js +++ b/chrome/browser/resources/sync_internals/sync_node_browser.js @@ -18,6 +18,23 @@ cr.define('chrome.sync', function() { // hide all these details. /** + * Returns an object which measures elapsed time. + */ + var makeTimer = function() { + var start = new Date(); + + return { + /** + * @return {number} The number of seconds since the timer was + * created. + */ + get elapsedSeconds() { + return ((new Date()).getTime() - start.getTime()) / 1000.0; + } + }; + }; + + /** * Gets all children of the given node and passes it to the given * callback. * @param {Object} nodeInfo The info for the node whose children we @@ -26,16 +43,19 @@ cr.define('chrome.sync', function() { * children. */ function getSyncNodeChildren(nodeInfo, callback) { - var children = []; - function processChildInfo(childNodeInfo) { - if (!childNodeInfo) { + var timer = makeTimer(); + chrome.sync.getChildNodeIds(nodeInfo.id, function(childNodeIds) { + console.debug('getChildNodeIds took ' + + timer.elapsedSeconds + 's to retrieve ' + + childNodeIds.length + ' ids'); + timer = makeTimer(); + chrome.sync.getNodesById(childNodeIds, function(children) { + console.debug('getNodesById took ' + + timer.elapsedSeconds + 's to retrieve ' + + children.length + ' nodes'); callback(children); - return; - } - children.push(childNodeInfo); - chrome.sync.getNodeById(childNodeInfo.successorId, processChildInfo); - }; - chrome.sync.getNodeById(nodeInfo.firstChildId, processChildInfo); + }); + }); } /** @@ -59,10 +79,13 @@ cr.define('chrome.sync', function() { treeItem.addEventListener('expand', function(event) { if (!treeItem.triggeredLoad_) { getSyncNodeChildren(nodeInfo, function(children) { + var timer = makeTimer(); for (var i = 0; i < children.length; ++i) { var childTreeItem = makeNodeTreeItem(children[i]); treeItem.add(childTreeItem); } + console.debug('adding ' + children.length + ' children took ' + + timer.elapsedSeconds + 's'); }); treeItem.triggeredLoad_ = true; } diff --git a/chrome/browser/sync/engine/syncapi.cc b/chrome/browser/sync/engine/syncapi.cc index 8c0eec1..ca11c34 100644 --- a/chrome/browser/sync/engine/syncapi.cc +++ b/chrome/browser/sync/engine/syncapi.cc @@ -1548,7 +1548,10 @@ class SyncManager::SyncInternal void OnIPAddressChangedImpl(); // Functions called by ProcessMessage(). - browser_sync::JsArgList ProcessGetNodeByIdMessage( + browser_sync::JsArgList ProcessGetNodesByIdMessage( + const browser_sync::JsArgList& args); + + browser_sync::JsArgList ProcessGetChildNodeIdsMessage( const browser_sync::JsArgList& args); browser_sync::JsArgList ProcessFindNodesContainingString( @@ -2719,13 +2722,20 @@ void SyncManager::SyncInternal::ProcessMessage( return_args.Append(root.ToValue()); parent_router_->RouteJsMessageReply( name, browser_sync::JsArgList(&return_args), sender); - } else if (name == "getNodeById") { + } else if (name == "getNodesById") { + if (!parent_router_) { + LogNoRouter(name, args); + return; + } + parent_router_->RouteJsMessageReply( + name, ProcessGetNodesByIdMessage(args), sender); + } else if (name == "getChildNodeIds") { if (!parent_router_) { LogNoRouter(name, args); return; } parent_router_->RouteJsMessageReply( - name, ProcessGetNodeByIdMessage(args), sender); + name, ProcessGetChildNodeIdsMessage(args), sender); } else if (name == "findNodesContainingString") { if (!parent_router_) { LogNoRouter(name, args); @@ -2758,29 +2768,70 @@ void SyncManager::SyncInternal::RouteJsMessageReply( parent_router_->RouteJsMessageReply(name, args, target); } -browser_sync::JsArgList SyncManager::SyncInternal::ProcessGetNodeByIdMessage( - const browser_sync::JsArgList& args) { - ListValue null_return_args_list; - null_return_args_list.Append(Value::CreateNullValue()); - browser_sync::JsArgList null_return_args(&null_return_args_list); +namespace { + +bool GetId(const ListValue& ids, int i, int64* id) { std::string id_str; - if (!args.Get().GetString(0, &id_str)) { - return null_return_args; + if (!ids.GetString(i, &id_str)) { + return false; } - int64 id; - if (!base::StringToInt64(id_str, &id)) { - return null_return_args; + if (!base::StringToInt64(id_str, id)) { + return false; } - if (id == kInvalidId) { - return null_return_args; + if (*id == kInvalidId) { + return false; } + return true; +} + +} // namespace + +browser_sync::JsArgList SyncManager::SyncInternal::ProcessGetNodesByIdMessage( + const browser_sync::JsArgList& args) { + ListValue return_args; + ListValue* nodes = new ListValue(); + return_args.Append(nodes); + ListValue* id_list = NULL; ReadTransaction trans(GetUserShare()); - ReadNode node(&trans); - if (!node.InitByIdLookup(id)) { - return null_return_args; + if (args.Get().GetList(0, &id_list)) { + for (size_t i = 0; i < id_list->GetSize(); ++i) { + int64 id = kInvalidId; + if (!GetId(*id_list, i, &id)) { + continue; + } + ReadNode node(&trans); + if (!node.InitByIdLookup(id)) { + continue; + } + nodes->Append(node.ToValue()); + } } + return browser_sync::JsArgList(&return_args); +} + +browser_sync::JsArgList SyncManager::SyncInternal:: + ProcessGetChildNodeIdsMessage( + const browser_sync::JsArgList& args) { ListValue return_args; - return_args.Append(node.ToValue()); + ListValue* child_ids = new ListValue(); + return_args.Append(child_ids); + int64 id = kInvalidId; + if (GetId(args.Get(), 0, &id)) { + ReadTransaction trans(GetUserShare()); + ReadNode node(&trans); + if (node.InitByIdLookup(id)) { + int64 child_id = node.GetFirstChildId(); + while (child_id != kInvalidId) { + ReadNode child_node(&trans); + if (!child_node.InitByIdLookup(child_id)) { + break; + } + child_ids->Append(Value::CreateStringValue( + base::Int64ToString(child_id))); + child_id = child_node.GetSuccessorId(); + } + } + } return browser_sync::JsArgList(&return_args); } diff --git a/chrome/browser/sync/engine/syncapi.h b/chrome/browser/sync/engine/syncapi.h index f51c574..c454c40 100644 --- a/chrome/browser/sync/engine/syncapi.h +++ b/chrome/browser/sync/engine/syncapi.h @@ -979,22 +979,24 @@ class SyncManager { // The following messages are processed by the returned backend: // // getNotificationState(): - // If there is a parent router, sends the - // onGetNotificationStateFinished(boolean notificationsEnabled) - // event to |sender| via the parent router with whether or not - // notifications are enabled. + // callback(boolean notificationsEnabled): + // notificationsEnabled: whether or not notifications are + // enabled. // // getRootNode(): - // If there is a parent router, sends the - // onGetRootNodeFinished(dictionary nodeInfo) event to |sender| - // via the parent router with information on the root node. + // callback(dictionary nodeInfo): + // nodeInfo: Information on the root node. // - // getNodeById(string id): - // If there is a parent router, sends the - // onGetNodeByIdFinished(dictionary nodeInfo) event to |sender| - // via the parent router with information on the node with the - // given id (metahandle), if the id is valid and a node with that - // id exists. Otherwise, calls onGetNodeByIdFinished(null). + // getNodesById(array idList): + // idList: A list of IDs as strings. + // callback(array nodeList): + // nodeList: Information on each node for each valid id in + // idList. Not guaranteed to be in any order. + // + // getChildNodeIds(string id): + // id: The id of the node for which to return the child node ids. + // callback(array idList): + // idList: The child node IDs of the node with the given id. // // All other messages are dropped. browser_sync::JsBackend* GetJsBackend(); diff --git a/chrome/browser/sync/engine/syncapi_unittest.cc b/chrome/browser/sync/engine/syncapi_unittest.cc index ba18eb6..229947b 100644 --- a/chrome/browser/sync/engine/syncapi_unittest.cc +++ b/chrome/browser/sync/engine/syncapi_unittest.cc @@ -871,23 +871,24 @@ TEST_F(SyncManagerTest, ProcessMessageGetRootNode) { js_backend->RemoveParentJsEventRouter(); } -void CheckGetNodeByIdReturnArgs(const SyncManager& sync_manager, - const JsArgList& return_args, - int64 id) { +void CheckGetNodesByIdReturnArgs(const SyncManager& sync_manager, + const JsArgList& return_args, + int64 id) { 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(return_args.Get().GetDictionary(0, &node_info)); - if (node_info) { - ReadTransaction trans(sync_manager.GetUserShare()); - ReadNode node(&trans); - node.InitByIdLookup(id); - CheckNodeValue(node, *node_info); - } else { - ADD_FAILURE(); - } + 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); } -TEST_F(SyncManagerTest, ProcessMessageGetNodeById) { +TEST_F(SyncManagerTest, ProcessMessageGetNodesById) { int64 child_id = MakeNode(sync_manager_.GetUserShare(), syncable::BOOKMARKS, "testtag"); @@ -899,7 +900,7 @@ TEST_F(SyncManagerTest, ProcessMessageGetNodeById) { JsArgList return_args; EXPECT_CALL(event_router, - RouteJsMessageReply("getNodeById", _, &event_handler)) + RouteJsMessageReply("getNodesById", _, &event_handler)) .Times(2).WillRepeatedly(SaveArg<1>(&return_args)); js_backend->SetParentJsEventRouter(&event_router); @@ -907,70 +908,180 @@ TEST_F(SyncManagerTest, ProcessMessageGetNodeById) { // Should trigger the reply. { ListValue args; - args.Append(Value::CreateStringValue("1")); - js_backend->ProcessMessage("getNodeById", JsArgList(&args), &event_handler); + ListValue* ids = new ListValue(); + args.Append(ids); + ids->Append(Value::CreateStringValue("1")); + js_backend->ProcessMessage("getNodesById", + JsArgList(&args), &event_handler); } - CheckGetNodeByIdReturnArgs(sync_manager_, return_args, 1); + CheckGetNodesByIdReturnArgs(sync_manager_, return_args, 1); // Should trigger another reply. { ListValue args; - args.Append(Value::CreateStringValue(base::Int64ToString(child_id))); - js_backend->ProcessMessage("getNodeById", JsArgList(&args), &event_handler); + ListValue* ids = new ListValue(); + args.Append(ids); + ids->Append(Value::CreateStringValue(base::Int64ToString(child_id))); + js_backend->ProcessMessage("getNodesById", + JsArgList(&args), &event_handler); } - CheckGetNodeByIdReturnArgs(sync_manager_, return_args, child_id); + CheckGetNodesByIdReturnArgs(sync_manager_, return_args, child_id); js_backend->RemoveParentJsEventRouter(); } -TEST_F(SyncManagerTest, ProcessMessageGetNodeByIdFailure) { +TEST_F(SyncManagerTest, ProcessMessageGetNodesByIdFailure) { browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend(); StrictMock<MockJsEventHandler> event_handler; StrictMock<MockJsEventRouter> event_router; - ListValue null_args; - null_args.Append(Value::CreateNullValue()); + ListValue empty_list_args; + empty_list_args.Append(new ListValue()); EXPECT_CALL(event_router, - RouteJsMessageReply("getNodeById", HasArgsAsList(null_args), + RouteJsMessageReply("getNodesById", + HasArgsAsList(empty_list_args), &event_handler)) - .Times(5); + .Times(6); js_backend->SetParentJsEventRouter(&event_router); { ListValue args; - js_backend->ProcessMessage("getNodeById", JsArgList(&args), &event_handler); + js_backend->ProcessMessage("getNodesById", + JsArgList(&args), &event_handler); } { ListValue args; - args.Append(Value::CreateStringValue("")); - js_backend->ProcessMessage("getNodeById", JsArgList(&args), &event_handler); + args.Append(new ListValue()); + js_backend->ProcessMessage("getNodesById", + JsArgList(&args), &event_handler); } { ListValue args; - args.Append(Value::CreateStringValue("nonsense")); - js_backend->ProcessMessage("getNodeById", JsArgList(&args), &event_handler); + 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) { + 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", + HasArgsAsList(empty_list_args), + &event_handler)) + .Times(5); + + js_backend->SetParentJsEventRouter(&event_router); + + { + ListValue args; + js_backend->ProcessMessage("getChildNodeIds", + JsArgList(&args), &event_handler); + } + + { + ListValue args; + args.Append(Value::CreateStringValue("")); + js_backend->ProcessMessage("getChildNodeIds", + JsArgList(&args), &event_handler); } { ListValue args; args.Append(Value::CreateStringValue("nonsense")); - js_backend->ProcessMessage("getNodeById", JsArgList(&args), &event_handler); + js_backend->ProcessMessage("getChildNodeIds", + JsArgList(&args), &event_handler); } { ListValue args; args.Append(Value::CreateStringValue("0")); - js_backend->ProcessMessage("getNodeById", JsArgList(&args), &event_handler); + js_backend->ProcessMessage("getChildNodeIds", + JsArgList(&args), &event_handler); } - // TODO(akalin): Figure out how to test InitByIdLookup() failure. + { + ListValue args; + args.Append(Value::CreateStringValue("9999")); + js_backend->ProcessMessage("getChildNodeIds", + JsArgList(&args), &event_handler); + } js_backend->RemoveParentJsEventRouter(); } |