Add full text regex searching to chrome://sync

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=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132259

[rlarocque, 2012-03-27 01:43:41]

This change is half-baked, but I wanted to get your initial impressions before
you go on vacation.

It includes the JS include fixes from http://codereview.chromium.org/9864019/.

This is a significant change to the implementation of about:sync search.  When a
search is requested, it will return *all* nodes to the JavaScript layer.  The
JavaScript layer will then serialize these nodes and match them against the
specified regex.  

Some discussion of why I chose to implement it this way and the trade-offs
involved can be seen in comment #5 of issue 104574.

[rlarocque, 2012-03-27 01:49:04]

Note some limitations of the current patch.

http://codereview.chromium.org/9836100/diff/1/chrome/browser/resources/sync_i...
File chrome/browser/resources/sync_internals/sync_search.js (right):

http://codereview.chromium.org/9836100/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/sync_search.js:24: return
JSON.stringify(elem).match(regex);
I believe JSON.stringify returns a "compact" version of the node.  This affects
how users should be writing queries in non-obvious ways (ie. how much whitespace
to expect).  

It's easy enough to work around this issue by specifying lots of wildcards in
the regex, but it's still likely to confuse people.

http://codereview.chromium.org/9836100/diff/1/chrome/browser/sync/internal_ap...
File chrome/browser/sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/9836100/diff/1/chrome/browser/sync/internal_ap...
chrome/browser/sync/internal_api/sync_manager.cc:2251:
result->Append((*it)->ToValue());
This returns the JSON-ified node to the JavaScript layer.  It's a bit confusing
because the right-hand pane of the search UI will display the
GetNodeDetailsById() version of the node.  From a user's point of view, it's not
obvious which attributes are part of the search.

[tim (not reviewing), 2012-03-27 17:36:48]

On 2012/03/27 01:49:04, rlarocque wrote:
> Note some limitations of the current patch.
> 
>
http://codereview.chromium.org/9836100/diff/1/chrome/browser/resources/sync_i...
> File chrome/browser/resources/sync_internals/sync_search.js (right):
> 
>
http://codereview.chromium.org/9836100/diff/1/chrome/browser/resources/sync_i...
> chrome/browser/resources/sync_internals/sync_search.js:24: return
> JSON.stringify(elem).match(regex);
> I believe JSON.stringify returns a "compact" version of the node.  This
affects
> how users should be writing queries in non-obvious ways (ie. how much
whitespace
> to expect).  
> 
> It's easy enough to work around this issue by specifying lots of wildcards in
> the regex, but it's still likely to confuse people.
> 
>
http://codereview.chromium.org/9836100/diff/1/chrome/browser/sync/internal_ap...
> File chrome/browser/sync/internal_api/sync_manager.cc (right):
> 
>
http://codereview.chromium.org/9836100/diff/1/chrome/browser/sync/internal_ap...
> chrome/browser/sync/internal_api/sync_manager.cc:2251:
> result->Append((*it)->ToValue());
> This returns the JSON-ified node to the JavaScript layer.  It's a bit
confusing
> because the right-hand pane of the search UI will display the
> GetNodeDetailsById() version of the node.  From a user's point of view, it's
not
> obvious which attributes are part of the search.

As we discussed in standup I think the delay is a fine given the huge short term
win.  If it winds up being uber slow / unusable (I doubt it.. but) we could
always present the entries in the unsynced + unapplied indices in their own pane
of about sync, or something, since most of the time I bet we'll be querying for
those fields.

[akalin, 2012-03-29 18:41:42]

Initial high-level comments

http://codereview.chromium.org/9836100/diff/1/chrome/browser/resources/sync_i...
File chrome/browser/resources/sync_internals/sync_index.html (right):

http://codereview.chromium.org/9836100/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/sync_index.html:15: <script
src="chrome://resources/js/event_tracker.js"></script>
this is part of your other patch, right?

http://codereview.chromium.org/9836100/diff/1/chrome/browser/resources/sync_i...
File chrome/browser/resources/sync_internals/sync_search.js (right):

http://codereview.chromium.org/9836100/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/sync_search.js:24: return
JSON.stringify(elem).match(regex);
On 2012/03/27 01:49:05, rlarocque wrote:
> I believe JSON.stringify returns a "compact" version of the node.  This
affects
> how users should be writing queries in non-obvious ways (ie. how much
whitespace
> to expect).  
> 
> It's easy enough to work around this issue by specifying lots of wildcards in
> the regex, but it's still likely to confuse people.

Does searching the raw json provide us any benefit?  What about writing a
Javascript function that recurses through the hash/array structure and tests the
regexp with the keys, string values, int values, etc.?  That would avoid the
confusion.

http://codereview.chromium.org/9836100/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/sync_search.js:62: return
this.NON_UNIQUE_NAME;
any reason you changed this?  is the NON_UNIQUE_NAME more readable?

http://codereview.chromium.org/9836100/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/sync_search.js:79:
chrome.sync.getNodeDetailsById([selected.META_HANDLE],
hunh.  is id == META_HANDLE?  any reason you changed this?

http://codereview.chromium.org/9836100/diff/1/chrome/browser/sync/internal_ap...
File chrome/browser/sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/9836100/diff/1/chrome/browser/sync/internal_ap...
chrome/browser/sync/internal_api/sync_manager.cc:168: "getAllNodes",
i think adding a getAllNodes message won't work, because the returned string
will be huge and will be converted multiple times (utf8 -> utf16, etc.).  You
may run out of memory, too.

If you're going to grab all nodes and search them, you can do so by doing
basically a BFS starting with the root node and using getChildNodeIds.  Doing so
will probably be slower, but will take less memory, and may be done
interactively. (Not sure if the current javascript code will already give you
that for free.)

I suggest writing a getAllNodeDetails helper function in javascript which takes
a callback and does the BFS.  Then you can simply call it with a function that
does the querying.

http://codereview.chromium.org/9836100/diff/1/sync/syncable/syncable.cc
File sync/syncable/syncable.cc (right):

http://codereview.chromium.org/9836100/diff/1/sync/syncable/syncable.cc#newco...
sync/syncable/syncable.cc:22: #include "base/json/json_writer.h"
is this needed?

[rlarocque, 2012-03-29 20:51:32]

Thanks for the input.

I'll try to clean this up a bit and fix some of the rough edges.

http://codereview.chromium.org/9836100/diff/1/chrome/browser/resources/sync_i...
File chrome/browser/resources/sync_internals/sync_index.html (right):

http://codereview.chromium.org/9836100/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/sync_index.html:15: <script
src="chrome://resources/js/event_tracker.js"></script>
On 2012/03/29 18:41:43, akalin wrote:
> this is part of your other patch, right?

Yes.

http://codereview.chromium.org/9836100/diff/1/chrome/browser/resources/sync_i...
File chrome/browser/resources/sync_internals/sync_search.js (right):

http://codereview.chromium.org/9836100/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/sync_search.js:24: return
JSON.stringify(elem).match(regex);
On 2012/03/29 18:41:43, akalin wrote:
> On 2012/03/27 01:49:05, rlarocque wrote:
> > I believe JSON.stringify returns a "compact" version of the node.  This
> affects
> > how users should be writing queries in non-obvious ways (ie. how much
> whitespace
> > to expect).  
> > 
> > It's easy enough to work around this issue by specifying lots of wildcards
in
> > the regex, but it's still likely to confuse people.
> 
> Does searching the raw json provide us any benefit?  What about writing a
> Javascript function that recurses through the hash/array structure and tests
the
> regexp with the keys, string values, int values, etc.?  That would avoid the
> confusion.

Searching the serialized version makes it easy to implement some very powerful
hacks.  For example:

"ID":\s*"sZ\S*,.*"IS_DIR":\s*true,.*"IS_DEL":\s*true,.*"IS_UNSYNCED":\s*true

That example is not tested and probably not 100% correct, but that's the general
idea.  In theory, it should search for Zipit items (the "ID" starts with "sZ")
which are unsynced, locally deleted, and directories.

Full text search gives us the ability to search for any combination of
attributes with very little effort (to implement, that is).  

Less hack-ish solutions are certainly possible, but they would likely require us
to define our own query language and write functions to parse and implement
those queries.  This language would have its own learning curve, probably almost
as bad as the one for these strange regexes.  To me, that doesn't seem worth it,
but that may be because I haven't put much thought into what such a query
language might look like.  Suggestions welcome.  

If nothing else, using regexes for now gets us a useful starting point that we
can use to gather information about what a proper solution should look like.

http://codereview.chromium.org/9836100/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/sync_search.js:62: return
this.NON_UNIQUE_NAME;
On 2012/03/29 18:41:43, akalin wrote:
> any reason you changed this?  is the NON_UNIQUE_NAME more readable?

Less readable, actually.  It's a long story.  

The old search function used getNodeSummariesById() to fetch nodes.  This
returned an array of sync_api::BaseNode::GetNodeSummaryAsValue() results.  The
GetNodeSummaryAsValue() function returns as dictionary with four entries, one of
which is the title, which is derrived from sync_api::BaseNode::GetTitle().

AFAIK, it's impossible to get a list of nodes from the sync_api layer.  To get a
full list, you need to get it from the syncable layer.  At that layer, you can't
get the "summary" of a node.  Instead you can convert the entire node into a
DictionaryValue.  This does not include anything equivalent to the "title" found
in the summary.

I changed this value because the "title" field is not among the attributes
returned by the getAllNodes() function.  I'm not sure yet how or if I intend to
fix this.

http://codereview.chromium.org/9836100/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/sync_search.js:79:
chrome.sync.getNodeDetailsById([selected.META_HANDLE],
On 2012/03/29 18:41:43, akalin wrote:
> hunh.  is id == META_HANDLE?  any reason you changed this?

Same as above, but id == META_HANDLE (as opposed to the approximate equality of
NON_UNIQUE_NAME and title).

http://codereview.chromium.org/9836100/diff/1/chrome/browser/sync/internal_ap...
File chrome/browser/sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/9836100/diff/1/chrome/browser/sync/internal_ap...
chrome/browser/sync/internal_api/sync_manager.cc:168: "getAllNodes",
On 2012/03/29 18:41:43, akalin wrote:
> i think adding a getAllNodes message won't work, because the returned string
> will be huge and will be converted multiple times (utf8 -> utf16, etc.).  You
> may run out of memory, too.
> 
> If you're going to grab all nodes and search them, you can do so by doing
> basically a BFS starting with the root node and using getChildNodeIds.  Doing
so
> will probably be slower, but will take less memory, and may be done
> interactively. (Not sure if the current javascript code will already give you
> that for free.)
> 
> I suggest writing a getAllNodeDetails helper function in javascript which
takes
> a callback and does the BFS.  Then you can simply call it with a function that
> does the querying.

I understand your concern.  I didn't expect this to work either.  

My test case was a database of over 5000 nodes.  The JSON-serialized result of
GetAllNodes() is 6 million chars long.  

I used the developer tools to take a snapshot of the heap while the search was
in progress.  The heap never exceeded 30MB.  For comparison, a freshly loaded
GMail page is 20MB.  This is my first attempt at using the JavaScript profiler,
so I may be misinterpreting the numbers, but it seems that we can afford to load
all nodes into memory.  

Interactivity/latency is a bit of an issue; see crbug.com/104574 for details. 
For a developers-only interface, though, I think this is acceptable.

http://codereview.chromium.org/9836100/diff/1/sync/syncable/syncable.cc
File sync/syncable/syncable.cc (right):

http://codereview.chromium.org/9836100/diff/1/sync/syncable/syncable.cc#newco...
sync/syncable/syncable.cc:22: #include "base/json/json_writer.h"
On 2012/03/29 18:41:43, akalin wrote:
> is this needed?

No.  Good catch.

[rlarocque, 2012-03-30 21:52:30]

This revision fixes a few things.  

TL;DR: Following this change, what you see is what you searched.  This should
make searching much more intuitive.

I've added error handling.  It's possible to specify an invalid regex.  If that
happens, we will highlight the search box in red and the status text will
describe the error.  

The left-side pane now displays text that is identical to the serialization
being searched.  This information is a subset of what was previously available,
but I don't think any of the additional attributes will be missed.  If you want
to know what this looks like, open up the node browser and highlight a node. 
The old version would have contained all the information you see currently; the
new version will show only the information under the "entry" attribute.  

This change also makes it so we don't need to fetch any additional information
when a node is highlighted.  We no longer need to make a request to C++ code to
gather information for the right side display; we can just fetch it from the
information cached during the search.  This information is stale, but I think
that users would expect stale information in this context.

Fred: Are you willing to review?  If not, I can find someone else to take over.

http://codereview.chromium.org/9836100/diff/1/chrome/browser/resources/sync_i...
File chrome/browser/resources/sync_internals/sync_search.js (right):

http://codereview.chromium.org/9836100/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/sync_search.js:62: return
this.NON_UNIQUE_NAME;
On 2012/03/29 20:51:32, rlarocque wrote:
> On 2012/03/29 18:41:43, akalin wrote:
> > any reason you changed this?  is the NON_UNIQUE_NAME more readable?
> 
> Less readable, actually.  It's a long story.  
> 
> The old search function used getNodeSummariesById() to fetch nodes.  This
> returned an array of sync_api::BaseNode::GetNodeSummaryAsValue() results.  The
> GetNodeSummaryAsValue() function returns as dictionary with four entries, one
of
> which is the title, which is derrived from sync_api::BaseNode::GetTitle().
> 
> AFAIK, it's impossible to get a list of nodes from the sync_api layer.  To get
a
> full list, you need to get it from the syncable layer.  At that layer, you
can't
> get the "summary" of a node.  Instead you can convert the entire node into a
> DictionaryValue.  This does not include anything equivalent to the "title"
found
> in the summary.
> 
> I changed this value because the "title" field is not among the attributes
> returned by the getAllNodes() function.  I'm not sure yet how or if I intend
to
> fix this.

I talked to Nicolas about this.  It turns out that all encrypted nodes have a
non-informative NON_UNIQUE_NAME.  Bookmarks are special in that they store their
title in their specifics, so we can grab a title even from an encrypted bookmark
node.  That's the one case where GetTitle() returns more useful information than
NON_UNIQUE_NAME.

So we're not losing too much by using NON_UNIQUE_NAME instead of the "title"
field generated by GetTitle().  This is a regression I'm willing to tolerate. 
We can revisit this decision later if we decide it's a problem.

[akalin, 2012-04-03 00:36:07]

took a quick look, this LGTM.  may want to get a once-over from someone else for
Javascript stuff.

http://codereview.chromium.org/9836100/diff/9001/chrome/browser/resources/syn...
File chrome/browser/resources/sync_internals/sync_search.js (right):

http://codereview.chromium.org/9836100/diff/9001/chrome/browser/resources/syn...
chrome/browser/resources/sync_internals/sync_search.js:27:
callback(allNodes.filter(function (elem) {
remove space after 'function'

http://codereview.chromium.org/9836100/diff/9001/chrome/browser/resources/syn...
chrome/browser/resources/sync_internals/sync_search.js:28: return
JSON.stringify(elem, null, 2).search(regex) != -1;
use regex.test(str) instead?

http://codereview.chromium.org/9836100/diff/9001/chrome/browser/resources/syn...
chrome/browser/resources/sync_internals/sync_search.js:32: // Sometimes the
provided regex is invalid.
update comment to indicate that this can also catch other errors

http://codereview.chromium.org/9836100/diff/9001/sync/syncable/syncable.cc
File sync/syncable/syncable.cc (right):

http://codereview.chromium.org/9836100/diff/9001/sync/syncable/syncable.cc#ne...
sync/syncable/syncable.cc:22: #include "base/json/json_writer.h"
didn't you mean to remove this?

[rlarocque, 2012-04-03 19:01:41]

I wasn't expecting a full review yet, but thanks!  I've addressed your comments
and added a unit test.  I will delay committing until you approve of the new
code.  

+Drew for JavaScript review.

http://codereview.chromium.org/9836100/diff/9001/chrome/browser/resources/syn...
File chrome/browser/resources/sync_internals/sync_search.js (right):

http://codereview.chromium.org/9836100/diff/9001/chrome/browser/resources/syn...
chrome/browser/resources/sync_internals/sync_search.js:27:
callback(allNodes.filter(function (elem) {
On 2012/04/03 00:36:08, akalin wrote:
> remove space after 'function'

Done.

http://codereview.chromium.org/9836100/diff/9001/chrome/browser/resources/syn...
chrome/browser/resources/sync_internals/sync_search.js:28: return
JSON.stringify(elem, null, 2).search(regex) != -1;
On 2012/04/03 00:36:08, akalin wrote:
> use regex.test(str) instead?

Done.

http://codereview.chromium.org/9836100/diff/9001/chrome/browser/resources/syn...
chrome/browser/resources/sync_internals/sync_search.js:32: // Sometimes the
provided regex is invalid.
On 2012/04/03 00:36:08, akalin wrote:
> update comment to indicate that this can also catch other errors

Done.

http://codereview.chromium.org/9836100/diff/9001/sync/syncable/syncable.cc
File sync/syncable/syncable.cc (right):

http://codereview.chromium.org/9836100/diff/9001/sync/syncable/syncable.cc#ne...
sync/syncable/syncable.cc:22: #include "base/json/json_writer.h"
On 2012/04/03 00:36:08, akalin wrote:
> didn't you mean to remove this?

Yes.  Done.

[Andrew T Wilson (Slow), 2012-04-06 20:30:57]

http://codereview.chromium.org/9836100/diff/11005/chrome/browser/resources/sy...
File chrome/browser/resources/sync_internals/sync_search.css (right):

http://codereview.chromium.org/9836100/diff/11005/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/sync_search.css:10: background-color:
#FAA;
Apparently css style guide says we should use rgb(R,G,B) rather than #RGB.

http://codereview.chromium.org/9836100/diff/11005/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/sync_search.css:14: color: #333;
Amusingly, this one is OK because you can use #RGB (where R=G=B) for shorthand
for greyscale.

http://codereview.chromium.org/9836100/diff/11005/chrome/browser/resources/sy...
File chrome/browser/resources/sync_internals/sync_search.js (right):

http://codereview.chromium.org/9836100/diff/11005/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/sync_search.js:14: * @param
{Array.<NodeSummary>}
Put the word "callback" on this line since it fits now. Also, is |callback| an
actual Array of NodeSummary objects? Because it looks like |callback| is
actually a Function.

http://codereview.chromium.org/9836100/diff/11005/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/sync_search.js:28: return
regex.test(JSON.stringify(elem, null, 2));
One quick question - is this fast enough to not trigger the "script on this page
is slow" dialog even for large data sets?

http://codereview.chromium.org/9836100/diff/11005/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/sync_search.js:71: 'Found ' +
nodes.length + ' nodes in '
nit: move + from next line to be on this line.

http://codereview.chromium.org/9836100/diff/11005/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/sync_search.js:79: }.bind(nodes[i]);
Is this bind actually necessary (is the function invoked as a function, like
this:

var a = nodes[i].toString;
a();

or like this:
nodes[i].toString();

Because if it's invoked as an object invocation, then |this| will be properly
set without bind().

[rlarocque, 2012-04-07 00:58:04]

Thanks for the review.

http://codereview.chromium.org/9836100/diff/11005/chrome/browser/resources/sy...
File chrome/browser/resources/sync_internals/sync_search.css (right):

http://codereview.chromium.org/9836100/diff/11005/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/sync_search.css:10: background-color:
#FAA;
On 2012/04/06 20:30:57, Andrew T Wilson wrote:
> Apparently css style guide says we should use rgb(R,G,B) rather than #RGB.

Done.

http://codereview.chromium.org/9836100/diff/11005/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/sync_search.css:14: color: #333;
On 2012/04/06 20:30:57, Andrew T Wilson wrote:
> Amusingly, this one is OK because you can use #RGB (where R=G=B) for shorthand
> for greyscale.

I'll change it anyway for consistency.

http://codereview.chromium.org/9836100/diff/11005/chrome/browser/resources/sy...
File chrome/browser/resources/sync_internals/sync_search.js (right):

http://codereview.chromium.org/9836100/diff/11005/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/sync_search.js:14: * @param
{Array.<NodeSummary>}
On 2012/04/06 20:30:57, Andrew T Wilson wrote:
> Put the word "callback" on this line since it fits now. Also, is |callback| an
> actual Array of NodeSummary objects? Because it looks like |callback| is
> actually a Function.

You're right.  It wasn't always that way, but the comment got stale after a
series of attempts to refactor this function.  Will fix.

http://codereview.chromium.org/9836100/diff/11005/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/sync_search.js:28: return
regex.test(JSON.stringify(elem, null, 2));
On 2012/04/06 20:30:57, Andrew T Wilson wrote:
> One quick question - is this fast enough to not trigger the "script on this
page
> is slow" dialog even for large data sets?

It depends how you define large.  I think my test, 5000 entries, is reasonably
large.  I'm aware that there are worse data sets out there, though.

I haven't ever seen that popup while testing in either debug or release builds. 
The slowest response I've seen was over 10 seconds on a debug build, but I think
most of that time was spent waiting for the callback and therefore didn't count
towards the timeout.

http://codereview.chromium.org/9836100/diff/11005/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/sync_search.js:71: 'Found ' +
nodes.length + ' nodes in '
On 2012/04/06 20:30:57, Andrew T Wilson wrote:
> nit: move + from next line to be on this line.

Done.

http://codereview.chromium.org/9836100/diff/11005/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/sync_search.js:79: }.bind(nodes[i]);
On 2012/04/06 20:30:57, Andrew T Wilson wrote:
> Is this bind actually necessary (is the function invoked as a function, like
> this:
> 
> var a = nodes[i].toString;
> a();
> 
> or like this:
> nodes[i].toString();
> 
> Because if it's invoked as an object invocation, then |this| will be properly
> set without bind().

Done.

I tried a more clever solution, but it failed.  It looks like omitting the bind
works just fine.

[rlarocque, 2012-04-09 21:21:25]

Whoops, forgot to upload the changes last Friday.  They're uploaded now.  

Tim: It looks like Fred is no longer reachable.  Could you look over the test I
added since his LGTM?

[tim (not reviewing), 2012-04-12 01:22:08]

A couple nits, then test LGTM

http://codereview.chromium.org/9836100/diff/19001/chrome/browser/sync/interna...
File chrome/browser/sync/internal_api/syncapi_unittest.cc (right):

http://codereview.chromium.org/9836100/diff/19001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/syncapi_unittest.cc:1234:
ASSERT_EQ(return_args.Get().GetSize(), static_cast<size_t>(1));
Here and below -

Prefer 1U to static_cast<size_t>(1).
Also, format is ASSERT_EQ(expected, actual), so I think you want to swap these.

[rlarocque, 2012-04-12 02:27:19]

Thanks.  Unit tests updated.

http://codereview.chromium.org/9836100/diff/19001/chrome/browser/sync/interna...
File chrome/browser/sync/internal_api/syncapi_unittest.cc (right):

http://codereview.chromium.org/9836100/diff/19001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/syncapi_unittest.cc:1234:
ASSERT_EQ(return_args.Get().GetSize(), static_cast<size_t>(1));
On 2012/04/12 01:22:08, timsteele wrote:
> Here and below -
> 
> Prefer 1U to static_cast<size_t>(1).
> Also, format is ASSERT_EQ(expected, actual), so I think you want to swap
these. 
> 

Done.

[Andrew T Wilson (Slow), 2012-04-13 00:37:16]

LGTM

[commit-bot: I haz the power, 2012-04-13 01:04:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/9836100/29001

[commit-bot: I haz the power, 2012-04-13 01:04:08]

Presubmit check for 9836100-29001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  chrome/browser/sync/internal_api/sync_manager.cc:1176
    open_result = directory()->OpenInMemoryForTest( \
  sync/syncable/syncable.cc:2142
    EntryKernel* entry = GetPossibleLastChildForTest(lock, parent_id);

If this change requires manual test instructions to QA team, add
TEST=[instructions].

See the JavaScript style guide at
http://www.chromium.org/developers/web-development-style-guide#TOC-JavaScript
and if you have any feedback about the JavaScript PRESUBMIT check, contact
tbreisacher@chromium.org

** Presubmit Warnings **
Found JavaScript style violations in
chrome/browser/resources/sync_internals/chrome_sync.js:
  line 8: E0220: No docs found for member 'chrome.sync'
chrome.sync = chrome.sync || {};
^^^^^^^^^^^
  line 9: E0001: Extra space after "function"
(function () {
         ^

Presubmit checks took 1.6s to calculate.

[rlarocque, 2012-04-13 17:29:57]

It turns out those warnings are not informative only.  I was hoping to ignore
them since I barely touched chrome_sync.js, and I have no idea how to document
the chrome.sync member.

I'll fix the warnings and try again.

On 2012/04/13 01:04:08, I haz the power (commit-bot) wrote:
> Presubmit check for 9836100-29001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit Messages **
> You might be calling functions intended only for testing from
> production code.  It is OK to ignore this warning if you know what
> you are doing, as the heuristics used to detect the situation are
> not perfect.  The commit queue will not block on this warning.
> Email mailto:joi@chromium.org if you have questions.
>   chrome/browser/sync/internal_api/sync_manager.cc:1176
>     open_result = directory()->OpenInMemoryForTest( \
>   sync/syncable/syncable.cc:2142
>     EntryKernel* entry = GetPossibleLastChildForTest(lock, parent_id);
> 
> If this change requires manual test instructions to QA team, add
> TEST=[instructions].
> 
> See the JavaScript style guide at
> http://www.chromium.org/developers/web-development-style-guide#TOC-JavaScript
> and if you have any feedback about the JavaScript PRESUBMIT check, contact
> mailto:tbreisacher@chromium.org
> 
> ** Presubmit Warnings **
> Found JavaScript style violations in
> chrome/browser/resources/sync_internals/chrome_sync.js:
>   line 8: E0220: No docs found for member 'chrome.sync'
> chrome.sync = chrome.sync || {};
> ^^^^^^^^^^^
>   line 9: E0001: Extra space after "function"
> (function () {
>          ^
> 
> Presubmit checks took 1.6s to calculate.

[commit-bot: I haz the power, 2012-04-13 18:20:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/9836100/46001

[commit-bot: I haz the power, 2012-04-13 20:41:13]

Change committed as 132259