[Sync] Add support for getRootNode() and getNodeById() JS messages

Added rudimentary node browser on chrome://sync-internals.

Some cleanup for syncapi tests.

BUG=69500
TEST=

Review URL: http://codereview.chromium.org/6250157

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@73923 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 440 insertions, 19 deletions

diff --git a/chrome/browser/resources/sync_internals/sync_index.html b/chrome/browser/resources/sync_internals/sync_index.html
index 51e397d..2eb37515 100644
--- a/chrome/browser/resources/sync_internals/sync_index.html
+++ b/chrome/browser/resources/sync_internals/sync_index.html
@@ -8,6 +8,7 @@ chrome/test/functional/special_tabs.py. -->
 function onLoad() {
   chrome.send('getAboutInfo');
   chrome.send('getNotificationState');
+  chrome.send('getRootNode');
 }
 
 function onGetAboutInfoFinished(aboutInfo) {
@@ -19,6 +20,41 @@ function onGetNotificationStateFinished(notificationsEnabled) {
   onSyncNotificationStateChange(notificationsEnabled);
 }
 
+function onGetRootNodeFinished(rootNodeInfo) {
+  onGetNodeByIdFinished(rootNodeInfo);
+}
+
+// TODO(akalin): Use an existing JSON-to-string library.
+function jsonToString(json) {
+  var str = '{ ';
+  for (var i in json) {
+    str += i + ': ';
+    var v = json[i];
+    var t = typeof v;
+    if (t === 'object') {
+      str += jsonToString(v);
+    } else if (t === 'string') {
+      str += '"' + v + '"';
+    } else {
+      str += json[i];
+    }
+    str += ', ';
+  }
+  str += ' }';
+  return str;
+}
+
+function onGetNodeByIdFinished(nodeInfo) {
+  var nodeBrowser = document.getElementById('nodeBrowser');
+  nodeInfo.specifics = jsonToString(nodeInfo.specifics);
+  jstProcess(new JsEvalContext(nodeInfo), nodeBrowser);
+}
+
+function processNodeLink(link) {
+  var id = link.text;
+  chrome.send('getNodeById', [id]);
+}
+
 function onSyncServiceStateChanged() {
   chrome.send('getAboutInfo');
 }
@@ -274,5 +310,20 @@ table.list#details .name {
 </table>
 </td>
 
+<div class="desc"><h2> Node Browser </h2></div>
+<ul id='nodeBrowser'>
+<li>ID: <span jscontent='id'></span></li>
+<li>Modification Time: <span jscontent='modificationTime'></span></li>
+<li>Parent: <a jscontent='parentId' href="#" onclick="processNodeLink(this); return false"></a></li>
+<li>Is Folder: <span jscontent='isFolder'></span></li>
+<li>Title: <span jscontent='title'></span></li>
+<li>Type: <span jscontent='type'></span></li>
+<li>Specifics: <span jscontent='specifics'></span></li>
+<li>External ID: <span jscontent='externalId'></span></li>
+<li>Predecessor: <a jscontent='predecessorId' href="#" onclick="processNodeLink(this); return false"></a></li>
+<li>Successor: <a jscontent='successorId' href="#" onclick="processNodeLink(this); return false"></a></li>
+<li>First Child: <a jscontent='firstChildId' href="#" onclick="processNodeLink(this); return false"></a></li>
+</ul>
+
 </body>
 </html>
diff --git a/chrome/browser/sync/engine/syncapi.cc b/chrome/browser/sync/engine/syncapi.cc
index f989da1..22c47df 100644
--- a/chrome/browser/sync/engine/syncapi.cc
+++ b/chrome/browser/sync/engine/syncapi.cc
@@ -15,6 +15,7 @@
 #include "base/message_loop.h"
 #include "base/scoped_ptr.h"
 #include "base/sha1.h"
+#include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #include "base/synchronization/lock.h"
 #include "base/task.h"
@@ -40,8 +41,9 @@
 #include "chrome/browser/sync/protocol/extension_specifics.pb.h"
 #include "chrome/browser/sync/protocol/nigori_specifics.pb.h"
 #include "chrome/browser/sync/protocol/preference_specifics.pb.h"
-#include "chrome/browser/sync/protocol/session_specifics.pb.h"
+#include "chrome/browser/sync/protocol/proto_value_conversions.h"
 #include "chrome/browser/sync/protocol/service_constants.h"
+#include "chrome/browser/sync/protocol/session_specifics.pb.h"
 #include "chrome/browser/sync/protocol/sync.pb.h"
 #include "chrome/browser/sync/protocol/theme_specifics.pb.h"
 #include "chrome/browser/sync/protocol/typed_url_specifics.pb.h"
@@ -263,6 +265,42 @@ int64 BaseNode::GetFirstChildId() const {
   return IdToMetahandle(GetTransaction()->GetWrappedTrans(), id_string);
 }
 
+DictionaryValue* BaseNode::ToValue() const {
+  DictionaryValue* node_info = new DictionaryValue();
+  node_info->SetString("id", base::Int64ToString(GetId()));
+  // TODO(akalin): Return time in a better format.
+  node_info->SetString("modificationTime",
+                       base::Int64ToString(GetModificationTime()));
+  node_info->SetString("parentId", base::Int64ToString(GetParentId()));
+  node_info->SetBoolean("isFolder", GetIsFolder());
+  // TODO(akalin): Add a std::string accessor for the title.
+  node_info->SetString("title", WideToUTF8(GetTitle()));
+  {
+    syncable::ModelType model_type = GetModelType();
+    if (model_type >= syncable::FIRST_REAL_MODEL_TYPE) {
+      node_info->SetString("type", ModelTypeToString(model_type));
+    } else if (model_type == syncable::TOP_LEVEL_FOLDER) {
+      node_info->SetString("type", "Top-level folder");
+    } else if (model_type == syncable::UNSPECIFIED) {
+      node_info->SetString("type", "Unspecified");
+    } else {
+      node_info->SetString("type", base::IntToString(model_type));
+    }
+  }
+  node_info->Set(
+      "specifics",
+      browser_sync::EntitySpecificsToValue(GetEntry()->Get(SPECIFICS)));
+  node_info->SetString("externalId",
+                       base::Int64ToString(GetExternalId()));
+  node_info->SetString("predecessorId",
+                       base::Int64ToString(GetPredecessorId()));
+  node_info->SetString("successorId",
+                       base::Int64ToString(GetSuccessorId()));
+  node_info->SetString("firstChildId",
+                       base::Int64ToString(GetFirstChildId()));
+  return node_info;
+}
+
 void BaseNode::GetFaviconBytes(std::vector<unsigned char>* output) const {
   if (!output)
     return;
@@ -1263,6 +1301,10 @@ class SyncManager::SyncInternal
   // Checks for server reachabilty and requests a nudge.
   void OnIPAddressChangedImpl();
 
+  // Functions called by ProcessMessage().
+  browser_sync::JsArgList ProcessGetNodeByIdMessage(
+      const browser_sync::JsArgList& args);
+
   // We couple the DirectoryManager and username together in a UserShare member
   // so we can return a handle to share_ to clients of the API for use when
   // constructing any transaction type.
@@ -1480,7 +1522,7 @@ bool SyncManager::SyncInternal::Init(
       method_factory_.NewRunnableMethod(&SyncInternal::CheckServerReachable));
 
   // Test mode does not use a syncer context or syncer thread.
-  if (!setup_for_test_mode) {
+  if (!setup_for_test_mode_) {
     // Build a SyncSessionContext and store the worker in it.
     VLOG(1) << "Sync is bringing up SyncSessionContext.";
     std::vector<SyncEngineEventListener*> listeners;
@@ -1634,7 +1676,9 @@ bool SyncManager::SyncInternal::SignIn(const SyncCredentials& credentials) {
   if (!OpenDirectory())
     return false;
 
-  UpdateCredentials(credentials);
+  if (!setup_for_test_mode_) {
+    UpdateCredentials(credentials);
+  }
   return true;
 }
 
@@ -2190,27 +2234,83 @@ const browser_sync::JsEventRouter*
   return parent_router_;
 }
 
+namespace {
+
+void LogNoRouter(const std::string& name,
+                 const browser_sync::JsArgList& args) {
+  VLOG(1) << "No parent router; not replying to message " << name
+          << " with args " << args.ToString();
+}
+
+}  // namespace
+
 void SyncManager::SyncInternal::ProcessMessage(
     const std::string& name, const browser_sync::JsArgList& args,
     const browser_sync::JsEventHandler* sender) {
+  DCHECK(initialized_);
   if (name == "getNotificationState") {
-    if (parent_router_) {
-      bool notifications_enabled = allstatus_.status().notifications_enabled;
-      ListValue return_args;
-      return_args.Append(Value::CreateBooleanValue(notifications_enabled));
-      parent_router_->RouteJsEvent(
-          "onGetNotificationStateFinished",
-          browser_sync::JsArgList(return_args), sender);
-    } else {
-      VLOG(1) << "No parent router; not replying to message " << name
-              << " with args " << args.ToString();
+    if (!parent_router_) {
+      LogNoRouter(name, args);
+      return;
+    }
+    bool notifications_enabled = allstatus_.status().notifications_enabled;
+    ListValue return_args;
+    return_args.Append(Value::CreateBooleanValue(notifications_enabled));
+    parent_router_->RouteJsEvent(
+        "onGetNotificationStateFinished",
+        browser_sync::JsArgList(return_args), sender);
+  } else if (name == "getRootNode") {
+    if (!parent_router_) {
+      LogNoRouter(name, args);
+      return;
     }
+    ReadTransaction trans(GetUserShare());
+    ReadNode root(&trans);
+    root.InitByRootLookup();
+    ListValue return_args;
+    return_args.Append(root.ToValue());
+    parent_router_->RouteJsEvent(
+        "onGetRootNodeFinished",
+        browser_sync::JsArgList(return_args), sender);
+  } else if (name == "getNodeById") {
+    if (!parent_router_) {
+      LogNoRouter(name, args);
+      return;
+    }
+    parent_router_->RouteJsEvent(
+        "onGetNodeByIdFinished", ProcessGetNodeByIdMessage(args), sender);
   } else {
     VLOG(1) << "Dropping unknown message " << name
               << " with args " << args.ToString();
   }
 }
 
+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);
+  std::string id_str;
+  if (!args.Get().GetString(0, &id_str)) {
+    return null_return_args;
+  }
+  int64 id;
+  if (!base::StringToInt64(id_str, &id)) {
+    return null_return_args;
+  }
+  if (id == kInvalidId) {
+    return null_return_args;
+  }
+  ReadTransaction trans(GetUserShare());
+  ReadNode node(&trans);
+  if (!node.InitByIdLookup(id)) {
+    return null_return_args;
+  }
+  ListValue return_args;
+  return_args.Append(node.ToValue());
+  return browser_sync::JsArgList(return_args);
+}
+
 void SyncManager::SyncInternal::OnNotificationStateChange(
     bool notifications_enabled) {
   VLOG(1) << "P2P: Notifications enabled = "
diff --git a/chrome/browser/sync/engine/syncapi.h b/chrome/browser/sync/engine/syncapi.h
index e215bda..2737b6c 100644
--- a/chrome/browser/sync/engine/syncapi.h
+++ b/chrome/browser/sync/engine/syncapi.h
@@ -53,6 +53,7 @@
 #include "chrome/common/net/gaia/google_service_auth_error.h"
 #include "googleurl/src/gurl.h"
 
+class DictionaryValue;
 class FilePath;
 
 namespace browser_sync {
@@ -251,6 +252,10 @@ class BaseNode {
   virtual const syncable::Entry* GetEntry() const = 0;
   virtual const BaseTransaction* GetTransaction() const = 0;
 
+  // Dumps all node info into a DictionaryValue and returns it.
+  // Transfers ownership of the DictionaryValue to the caller.
+  DictionaryValue* ToValue() const;
+
  protected:
   BaseNode();
   virtual ~BaseNode();
@@ -915,10 +920,22 @@ class SyncManager {
   //
   // getNotificationState():
   //   If there is a parent router, sends the
-  //   onGetNotificationStateFinished(boolean notifications_enabled)
+  //   onGetNotificationStateFinished(boolean notificationsEnabled)
   //   event to |sender| via the parent router with 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.
+  //
+  // 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).
+  //
   // 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 3856ef8..f4a05a5 100644
--- a/chrome/browser/sync/engine/syncapi_unittest.cc
+++ b/chrome/browser/sync/engine/syncapi_unittest.cc
@@ -6,9 +6,12 @@
 // functionality is provided by the Syncable layer, which has its own
 // unit tests. We'll test SyncApi specific things in this harness.
 
+#include "base/basictypes.h"
 #include "base/message_loop.h"
 #include "base/scoped_ptr.h"
 #include "base/scoped_temp_dir.h"
+#include "base/string_number_conversions.h"
+#include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/browser_thread.h"
 #include "chrome/browser/sync/engine/syncapi.h"
@@ -18,9 +21,11 @@
 #include "chrome/browser/sync/js_event_router.h"
 #include "chrome/browser/sync/js_test_util.h"
 #include "chrome/browser/sync/protocol/password_specifics.pb.h"
+#include "chrome/browser/sync/protocol/proto_value_conversions.h"
 #include "chrome/browser/sync/syncable/directory_manager.h"
 #include "chrome/browser/sync/syncable/syncable.h"
 #include "chrome/test/sync/engine/test_directory_setter_upper.h"
+#include "jingle/notifier/base/notifier_options.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -30,6 +35,7 @@ using browser_sync::JsArgList;
 using browser_sync::MockJsEventHandler;
 using browser_sync::MockJsEventRouter;
 using testing::_;
+using testing::SaveArg;
 using testing::StrictMock;
 
 namespace sync_api {
@@ -280,15 +286,111 @@ TEST_F(SyncApiTest, WriteAndReadPassword) {
 
 namespace {
 
+int64 GetInt64Value(const DictionaryValue& value, const std::string& key) {
+  std::string str;
+  EXPECT_TRUE(value.GetString(key, &str));
+  int64 val = 0;
+  EXPECT_TRUE(base::StringToInt64(str, &val));
+  return val;
+}
+
+void CheckNodeValue(const BaseNode& node, const DictionaryValue& value) {
+  EXPECT_EQ(node.GetId(), GetInt64Value(value, "id"));
+  EXPECT_EQ(node.GetModificationTime(),
+            GetInt64Value(value, "modificationTime"));
+  EXPECT_EQ(node.GetParentId(), GetInt64Value(value, "parentId"));
+  {
+    bool is_folder = false;
+    EXPECT_TRUE(value.GetBoolean("isFolder", &is_folder));
+    EXPECT_EQ(node.GetIsFolder(), is_folder);
+  }
+  {
+    std::string title;
+    EXPECT_TRUE(value.GetString("title", &title));
+    EXPECT_EQ(node.GetTitle(), UTF8ToWide(title));
+  }
+  {
+    syncable::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) {
+      syncable::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();
+    }
+  }
+  {
+    scoped_ptr<DictionaryValue> expected_specifics(
+        browser_sync::EntitySpecificsToValue(
+            node.GetEntry()->Get(syncable::SPECIFICS)));
+    Value* specifics = NULL;
+    EXPECT_TRUE(value.Get("specifics", &specifics));
+    EXPECT_TRUE(Value::Equals(specifics, expected_specifics.get()));
+  }
+  EXPECT_EQ(node.GetExternalId(), GetInt64Value(value, "externalId"));
+  EXPECT_EQ(node.GetPredecessorId(), GetInt64Value(value, "predecessorId"));
+  EXPECT_EQ(node.GetSuccessorId(), GetInt64Value(value, "successorId"));
+  EXPECT_EQ(node.GetFirstChildId(), GetInt64Value(value, "firstChildId"));
+  EXPECT_EQ(11u, value.size());
+}
+
+}  // namespace
+
+TEST_F(SyncApiTest, BaseNodeToValue) {
+  ReadTransaction trans(&share_);
+  ReadNode node(&trans);
+  node.InitByRootLookup();
+  scoped_ptr<DictionaryValue> value(node.ToValue());
+  if (value.get()) {
+    CheckNodeValue(node, *value);
+  } else {
+    ADD_FAILURE();
+  }
+}
+
+namespace {
+
+class TestHttpPostProviderFactory : public HttpPostProviderFactory {
+ public:
+  virtual ~TestHttpPostProviderFactory() {}
+  virtual HttpPostProviderInterface* Create() {
+    NOTREACHED();
+    return NULL;
+  }
+  virtual void Destroy(HttpPostProviderInterface* http) {
+    NOTREACHED();
+  }
+};
+
 class SyncManagerTest : public testing::Test {
  protected:
   SyncManagerTest() : ui_thread_(BrowserThread::UI, &ui_loop_) {}
 
+  void SetUp() {
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    sync_manager_.Init(temp_dir_.path(), "bogus", 0, false,
+                       new TestHttpPostProviderFactory(), NULL, "bogus",
+                       SyncCredentials(), notifier::NotifierOptions(),
+                       "", true /* setup_for_test_mode */);
+  }
+
+  void TearDown() {
+    sync_manager_.Shutdown();
+  }
+
  private:
   // Needed by |ui_thread_|.
   MessageLoopForUI ui_loop_;
   // Needed by |sync_manager_|.
   BrowserThread ui_thread_;
+  // Needed by |sync_manager_|.
+  ScopedTempDir temp_dir_;
 
  protected:
   SyncManager sync_manager_;
@@ -327,8 +429,7 @@ TEST_F(SyncManagerTest, ProcessMessage) {
 
     EXPECT_CALL(event_router,
                 RouteJsEvent("onGetNotificationStateFinished",
-                             HasArgsAsList(false_args),
-                             &event_handler)).Times(1);
+                             HasArgsAsList(false_args), &event_handler));
 
     js_backend->SetParentJsEventRouter(&event_router);
 
@@ -354,6 +455,155 @@ TEST_F(SyncManagerTest, ProcessMessage) {
   }
 }
 
+TEST_F(SyncManagerTest, ProcessMessageGetRootNode) {
+  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,
+              RouteJsEvent("onGetRootNodeFinished", _, &event_handler)).
+      WillOnce(SaveArg<1>(&return_args));
+
+  js_backend->SetParentJsEventRouter(&event_router);
+
+  // Should trigger the reply.
+  js_backend->ProcessMessage("getRootNode", 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);
+  } else {
+    ADD_FAILURE();
+  }
+
+  js_backend->RemoveParentJsEventRouter();
+}
+
+void CheckGetNodeByIdReturnArgs(const SyncManager& sync_manager,
+                                const JsArgList& return_args,
+                                int64 id) {
+  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.InitByIdLookup(id);
+    CheckNodeValue(node, *node_info);
+  } else {
+    ADD_FAILURE();
+  }
+}
+
+TEST_F(SyncManagerTest, ProcessMessageGetNodeById) {
+  int64 child_id = kInvalidId;
+  {
+    WriteTransaction trans(sync_manager_.GetUserShare());
+
+    ReadNode root_node(&trans);
+    root_node.InitByRootLookup();
+
+    WriteNode node(&trans);
+    EXPECT_TRUE(node.InitUniqueByCreation(syncable::BOOKMARKS,
+                                          root_node, "testtag"));
+    node.SetIsFolder(false);
+    child_id = node.GetId();
+  }
+
+  browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend();
+
+  StrictMock<MockJsEventHandler> event_handler;
+  StrictMock<MockJsEventRouter> event_router;
+
+  JsArgList return_args;
+
+  EXPECT_CALL(event_router,
+              RouteJsEvent("onGetNodeByIdFinished", _, &event_handler))
+      .Times(2).WillRepeatedly(SaveArg<1>(&return_args));
+
+  js_backend->SetParentJsEventRouter(&event_router);
+
+  // Should trigger the reply.
+  {
+    ListValue args;
+    args.Append(Value::CreateStringValue("1"));
+    js_backend->ProcessMessage("getNodeById", JsArgList(args), &event_handler);
+  }
+
+  CheckGetNodeByIdReturnArgs(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);
+  }
+
+  CheckGetNodeByIdReturnArgs(sync_manager_, return_args, child_id);
+
+  js_backend->RemoveParentJsEventRouter();
+}
+
+TEST_F(SyncManagerTest, ProcessMessageGetNodeByIdFailure) {
+  browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend();
+
+  StrictMock<MockJsEventHandler> event_handler;
+  StrictMock<MockJsEventRouter> event_router;
+
+  ListValue null_args;
+  null_args.Append(Value::CreateNullValue());
+
+  EXPECT_CALL(event_router,
+              RouteJsEvent("onGetNodeByIdFinished",
+                           HasArgsAsList(null_args), &event_handler))
+      .Times(5);
+
+  js_backend->SetParentJsEventRouter(&event_router);
+
+  {
+    ListValue args;
+    js_backend->ProcessMessage("getNodeById", JsArgList(args), &event_handler);
+  }
+
+  {
+    ListValue args;
+    args.Append(Value::CreateStringValue(""));
+    js_backend->ProcessMessage("getNodeById", JsArgList(args), &event_handler);
+  }
+
+  {
+    ListValue args;
+    args.Append(Value::CreateStringValue("nonsense"));
+    js_backend->ProcessMessage("getNodeById", JsArgList(args), &event_handler);
+  }
+
+  {
+    ListValue args;
+    args.Append(Value::CreateStringValue("nonsense"));
+    js_backend->ProcessMessage("getNodeById", JsArgList(args), &event_handler);
+  }
+
+  {
+    ListValue args;
+    args.Append(Value::CreateStringValue("0"));
+    js_backend->ProcessMessage("getNodeById", JsArgList(args), &event_handler);
+  }
+
+  // TODO(akalin): Figure out how to test InitByIdLookup() failure.
+
+  js_backend->RemoveParentJsEventRouter();
+}
+
 TEST_F(SyncManagerTest, OnNotificationStateChange) {
   StrictMock<MockJsEventRouter> event_router;
 
@@ -364,10 +614,10 @@ TEST_F(SyncManagerTest, OnNotificationStateChange) {
 
   EXPECT_CALL(event_router,
               RouteJsEvent("onSyncNotificationStateChange",
-                           HasArgsAsList(true_args), NULL)).Times(1);
+                           HasArgsAsList(true_args), NULL));
   EXPECT_CALL(event_router,
               RouteJsEvent("onSyncNotificationStateChange",
-                           HasArgsAsList(false_args), NULL)).Times(1);
+                           HasArgsAsList(false_args), NULL));
 
   browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend();
 
@@ -410,7 +660,7 @@ TEST_F(SyncManagerTest, OnIncomingNotification) {
 
   EXPECT_CALL(event_router,
               RouteJsEvent("onSyncIncomingNotification",
-                           HasArgsAsList(expected_args), NULL)).Times(1);
+                           HasArgsAsList(expected_args), NULL));
 
   browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend();
 
diff --git a/chrome/browser/sync/protocol/proto_value_conversions.h b/chrome/browser/sync/protocol/proto_value_conversions.h
index b28fd12..c6e4b1d 100644
--- a/chrome/browser/sync/protocol/proto_value_conversions.h
+++ b/chrome/browser/sync/protocol/proto_value_conversions.h
@@ -42,6 +42,9 @@ class TypedUrlSpecifics;
 
 namespace browser_sync {
 
+// Ownership of all returned DictionaryValues are transferred to the
+// caller.
+
 // TODO(akalin): Perhaps extend this to decrypt?
 DictionaryValue* EncryptedDataToValue(
     const sync_pb::EncryptedData& encrypted_data);