Add full text regex searching to chrome://sync

The search tab of chrome://sync would previously perform a string search
of a select set of fields.  This change modifies the search behaviour to
instead perform a regex match against a serialized version of the node.

Part of this change is to move the searching function out of C++ and
into JavaScript.  When a search is performed, all nodes are loaded from
the database, marshalled, and sent over the fence to the JavaScript side
of things.  This comes with a significant performance cost, but it's not
much worse than a search matching all nodes would have been under the
old system.

While there was no such thing as an invalid search string under the
old system, it is possible to enter an invalid regex.  This change adds
some logic to alert the user if their search query is not a valid regex
(ie. if it ends with a backslash).

In order to make it easier to formulate queries, the way we display
results has been changed.  The right pane will now display the
serialization of the raw node (which exactly reflects the text that was
searched) rather than the "details" used in the old system.  This new
format is a subset of the old, and corresponds to the dictionary value
found under "entry" in the old display.

Finally, this change removes support for some JavaScript calls that are
no longer used.

This change fixes JavaScript style-checker issues in the files that it
touches.

BUG=104574, 122021
TEST=

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

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

chrome/browser/resources/sync_internals/chrome_sync.js

@@ -3,10 +3,17 @@
 // found in the LICENSE file.
 
 var chrome = chrome || {};
+
 // TODO(akalin): Add mocking code for e.g. chrome.send() so that we
 // can test this without rebuilding chrome.
+
+/**
+ * Organize sync event listeners and asynchronous requests.
+ * This object is one of a kind; its constructor is not public.
+ * @type {Object}
+ */
 chrome.sync = chrome.sync || {};
-(function () {
+(function() {
 
 // This Event class is a simplified version of the one from
 // event_bindings.js.
@@ -145,7 +152,7 @@ var syncFunctions = [
   'getNodeSummariesById',
   'getNodeDetailsById',
   'getChildNodeIds',
-  'findNodesContainingString'
+  'getAllNodes',
 ];
 
 for (var i = 0; i < syncFunctions.length; ++i) {

chrome/browser/resources/sync_internals/sync_search.css

@@ -6,8 +6,12 @@
   width: 20em;
 }
 
+#sync-search-query[error] {
+  background-color: rgb(255,170,170);
+}
+
 #sync-search-status {
-  color: #333;
+  color: rgb(51,51,51);
   font-style: italic;
 }
 

chrome/browser/resources/sync_internals/sync_search.js

-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -10,25 +10,27 @@ cr.define('chrome.sync', function() {
   /**
    * Runs a search with the given query.
    *
-   * @param {string} query The query to do the search with.
-   * @param {Array.<{id: string, title: string, isFolder: boolean}>}
-   *     callback The callback called with the search results; not
-   *     called if doSearch() is called again while the search is
-   *     running.
+   * @param {string} query The regex to do the search with.
+   * @param {function} callback The callback called with the search results;
+   *     not called if doSearch() is called again while the search is running.
    */
   var doSearch = function(query, callback) {
     var searchId = ++currSearchId;
-    chrome.sync.findNodesContainingString(query, function(ids) {
-      if (currSearchId != searchId) {
-        return;
-      }
-      chrome.sync.getNodeSummariesById(ids, function(nodeSummaries) {
+    try {
+      var regex = new RegExp(query);
+      chrome.sync.getAllNodes(query, function(allNodes) {
         if (currSearchId != searchId) {
           return;
         }
-        callback(nodeSummaries);
+        callback(allNodes.filter(function(elem) {
+          return regex.test(JSON.stringify(elem, null, 2));
+        }), null);
       });
-    });
+    } catch (err) {
+      // Sometimes the provided regex is invalid.  This and other errors will
+      // be caught and handled here.
+      callback([], err);
+    }
   };
 
   /**
@@ -56,20 +58,28 @@ cr.define('chrome.sync', function() {
         return;
       }
       statusControl.textContent = 'Searching for ' + query + '...';
+      queryControl.removeAttribute('error');
       var timer = chrome.sync.makeTimer();
-      doSearch(query, function(nodeSummaries) {
-        statusControl.textContent =
-          'Found ' + nodeSummaries.length + ' nodes in '
-          + timer.elapsedSeconds + 's';
-        // TODO(akalin): Write a nicer list display.
-        for (var i = 0; i < nodeSummaries.length; ++i) {
-          nodeSummaries[i].toString = function() {
-            return this.title;
-          }.bind(nodeSummaries[i]);
+      doSearch(query, function(nodes, error) {
+        if (error) {
+          statusControl.textContent = 'Error: ' + error;
+          queryControl.setAttribute('error');
+        } else {
+          statusControl.textContent =
+            'Found ' + nodes.length + ' nodes in ' +
+            timer.elapsedSeconds + 's';
+          queryControl.removeAttribute('error');
+
+          // TODO(akalin): Write a nicer list display.
+          for (var i = 0; i < nodes.length; ++i) {
+            nodes[i].toString = function() {
+              return this.NON_UNIQUE_NAME;
+            };
+          }
+          resultsDataModel.push.apply(resultsDataModel, nodes);
+          // Workaround for http://crbug.com/83452 .
+          resultsControl.redraw();
         }
-        resultsDataModel.push.apply(resultsDataModel, nodeSummaries);
-        // Workaround for http://crbug.com/83452 .
-        resultsControl.redraw();
       });
     };
     queryControl.value = '';
@@ -81,13 +91,7 @@ cr.define('chrome.sync', function() {
       detailsControl.textContent = '';
       var selected = resultsControl.selectedItem;
       if (selected) {
-        chrome.sync.getNodeDetailsById([selected.id], function(nodeDetails) {
-          var selectedNodeDetails = nodeDetails[0] || null;
-          if (selectedNodeDetails) {
-            detailsControl.textContent =
-              JSON.stringify(selectedNodeDetails, null, 2);
-          }
-        });
+        detailsControl.textContent = JSON.stringify(selected, null, 2);
       }
     });
   }

chrome/browser/sync/internal_api/sync_manager.cc

@@ -175,12 +175,12 @@ class SyncManager::SyncInternal
     BindJsMessageHandler(
         "getNodeDetailsById",
         &SyncManager::SyncInternal::GetNodeDetailsById);
+    BindJsMessageHandler(
+        "getAllNodes",
+        &SyncManager::SyncInternal::GetAllNodes);
     BindJsMessageHandler(
         "getChildNodeIds",
         &SyncManager::SyncInternal::GetChildNodeIds);
-    BindJsMessageHandler(
-        "findNodesContainingString",
-        &SyncManager::SyncInternal::FindNodesContainingString);
     BindJsMessageHandler(
         "getClientServerTraffic",
         &SyncManager::SyncInternal::GetClientServerTraffic);
@@ -526,10 +526,10 @@ class SyncManager::SyncInternal
   JsArgList GetNotificationState(const JsArgList& args);
   JsArgList GetNotificationInfo(const JsArgList& args);
   JsArgList GetRootNodeDetails(const JsArgList& args);
+  JsArgList GetAllNodes(const JsArgList& args);
   JsArgList GetNodeSummariesById(const JsArgList& args);
   JsArgList GetNodeDetailsById(const JsArgList& args);
   JsArgList GetChildNodeIds(const JsArgList& args);
-  JsArgList FindNodesContainingString(const JsArgList& args);
   JsArgList GetClientServerTraffic(const JsArgList& args);
 
   FilePath database_path_;
@@ -2291,6 +2291,25 @@ JsArgList SyncManager::SyncInternal::GetNodeDetailsById(
   return GetNodeInfoById(args, GetUserShare(), &BaseNode::GetDetailsAsValue);
 }
 
+JsArgList SyncManager::SyncInternal::GetAllNodes(
+    const JsArgList& args) {
+  ListValue return_args;
+  ListValue* result = new ListValue();
+  return_args.Append(result);
+
+  ReadTransaction trans(FROM_HERE, GetUserShare());
+  std::vector<const syncable::EntryKernel*> entry_kernels;
+  trans.GetDirectory()->GetAllEntryKernels(trans.GetWrappedTrans(),
+                                           &entry_kernels);
+
+  for (std::vector<const syncable::EntryKernel*>::const_iterator it =
+           entry_kernels.begin(); it != entry_kernels.end(); ++it) {
+    result->Append((*it)->ToValue());
+  }
+
+  return JsArgList(&return_args);
+}
+
 JsArgList SyncManager::SyncInternal::GetChildNodeIds(
     const JsArgList& args) {
   ListValue return_args;
@@ -2311,39 +2330,6 @@ JsArgList SyncManager::SyncInternal::GetChildNodeIds(
   return JsArgList(&return_args);
 }
 
-JsArgList SyncManager::SyncInternal::FindNodesContainingString(
-    const JsArgList& args) {
-  std::string query;
-  ListValue return_args;
-  if (!args.Get().GetString(0, &query)) {
-    return_args.Append(new ListValue());
-    return JsArgList(&return_args);
-  }
-
-  // Convert the query string to lower case to perform case insensitive
-  // searches.
-  std::string lowercase_query = query;
-  StringToLowerASCII(&lowercase_query);
-
-  ListValue* result = new ListValue();
-  return_args.Append(result);
-
-  ReadTransaction trans(FROM_HERE, GetUserShare());
-  std::vector<const syncable::EntryKernel*> entry_kernels;
-  trans.GetDirectory()->GetAllEntryKernels(trans.GetWrappedTrans(),
-                                           &entry_kernels);
-
-  for (std::vector<const syncable::EntryKernel*>::const_iterator it =
-           entry_kernels.begin(); it != entry_kernels.end(); ++it) {
-    if ((*it)->ContainsString(lowercase_query)) {
-      result->Append(new StringValue(base::Int64ToString(
-          (*it)->ref(syncable::META_HANDLE))));
-    }
-  }
-
-  return JsArgList(&return_args);
-}
-
 void SyncManager::SyncInternal::OnEncryptedTypesChanged(
     syncable::ModelTypeSet encrypted_types,
     bool encrypt_everything) {

chrome/browser/sync/internal_api/syncapi_unittest.cc

@@ -1205,7 +1205,46 @@ TEST_F(SyncManagerTest, GetChildNodeIdsFailure) {
   }
 }
 
-// TODO(akalin): Add unit tests for findNodesContainingString message.
+TEST_F(SyncManagerTest, GetAllNodesTest) {
+  StrictMock<MockJsReplyHandler> reply_handler;
+  JsArgList return_args;
+
+  EXPECT_CALL(reply_handler,
+              HandleJsReply("getAllNodes", _))
+      .Times(1).WillRepeatedly(SaveArg<1>(&return_args));
+
+  {
+    ListValue args;
+    SendJsMessage("getAllNodes",
+                  JsArgList(&args), reply_handler.AsWeakHandle());
+  }
+
+  // There's not much value in verifying every attribute on every node here.
+  // Most of the value of this test has already been achieved: we've verified we
+  // can call the above function without crashing or leaking memory.
+  //
+  // Let's just check the list size and a few of its elements.  Anything more
+  // would make this test brittle without greatly increasing our chances of
+  // catching real bugs.
+
+  ListValue* node_list;
+  DictionaryValue* first_result;
+
+  // The resulting argument list should have one argument, a list of nodes.
+  ASSERT_EQ(1U, return_args.Get().GetSize());
+  ASSERT_TRUE(return_args.Get().GetList(0, &node_list));
+
+  // The database creation logic depends on the routing info.
+  // Refer to setup methods for more information.
+  ModelSafeRoutingInfo routes;
+  GetModelSafeRoutingInfo(&routes);
+  size_t directory_size = routes.size() + 1;
+
+  ASSERT_EQ(directory_size, node_list->GetSize());
+  ASSERT_TRUE(node_list->GetDictionary(0, &first_result));
+  EXPECT_TRUE(first_result->HasKey("ID"));
+  EXPECT_TRUE(first_result->HasKey("NON_UNIQUE_NAME"));
+}
 
 TEST_F(SyncManagerTest, OnNotificationStateChange) {
   InSequence dummy;

sync/syncable/syncable.cc

@@ -302,36 +302,6 @@ syncable::ModelType EntryKernel::GetServerModelType() const {
   return UNSPECIFIED;
 }
 
-bool EntryKernel::ContainsString(const std::string& lowercase_query) const {
-  // TODO(lipalani) - figure out what to do if the node is encrypted.
-  const sync_pb::EntitySpecifics& specifics = ref(SPECIFICS);
-  std::string temp;
-  // The protobuf serialized string contains the original strings. So
-  // we will just serialize it and search it.
-  specifics.SerializeToString(&temp);
-
-  // Now convert to lower case.
-  StringToLowerASCII(&temp);
-
-  if (temp.find(lowercase_query) != std::string::npos)
-    return true;
-
-  // Now go through all the string fields to see if the value is there.
-  for (int i = STRING_FIELDS_BEGIN; i < STRING_FIELDS_END; ++i) {
-    if (StringToLowerASCII(ref(static_cast<StringField>(i))).find(
-            lowercase_query) != std::string::npos)
-      return true;
-  }
-
-  for (int i = ID_FIELDS_BEGIN; i < ID_FIELDS_END; ++i) {
-    const Id& id = ref(static_cast<IdField>(i));
-    if (id.ContainsStringCaseInsensitive(lowercase_query)) {
-      return true;
-    }
-  }
-  return false;
-}
-
 namespace {
 
 // Utility function to loop through a set of enum values and add the

sync/syncable/syncable.h

@@ -379,10 +379,6 @@ struct EntryKernel {
 
   syncable::ModelType GetServerModelType() const;
 
-  // Does a case in-sensitive search for a given string, which must be
-  // lower case.
-  bool ContainsString(const std::string& lowercase_query) const;
-
   // Dumps all kernel info into a DictionaryValue and returns it.
   // Transfers ownership of the DictionaryValue to the caller.
   base::DictionaryValue* ToValue() const;

sync/syncable/syncable_id.cc

@@ -6,7 +6,6 @@
 
 #include <iosfwd>
 
-#include "base/string_util.h"
 #include "base/values.h"
 
 using std::ostream;
@@ -57,12 +56,6 @@ Id Id::GetLexicographicSuccessor() const {
   return id;
 }
 
-bool Id::ContainsStringCaseInsensitive(
-    const std::string& lowercase_query) const {
-  DCHECK_EQ(StringToLowerASCII(lowercase_query), lowercase_query);
-  return StringToLowerASCII(s_).find(lowercase_query) != std::string::npos;
-}
-
 // static
 Id Id::GetLeastIdForLexicographicComparison() {
   Id id;

sync/syncable/syncable_id.h

@@ -97,9 +97,6 @@ class Id {
   // by operator<.
   Id GetLexicographicSuccessor() const;
 
-  // Note: |lowercase_query| should be passed in as lower case.
-  bool ContainsStringCaseInsensitive(const std::string& lowercase_query) const;
-
   // Dumps the ID as a value and returns it.  Transfers ownership of
   // the StringValue to the caller.
   base::StringValue* ToValue() const;