sync: Remove some WebUI debug functions

sync: Remove some WebUI debug functions

This CL removes a number of functions from the sync debugging framework
in order to prepare for some upcoming changes to sync debugging (due to
328606).

This CL removes the following functions:
- GetRootNodeDetails
- GetNodeSummaries
- GetNodeDetails
- GetChildNodeIds

It also adds the function 'getListOfKnownTypes' to help replace some old
functionality required by the 'data' tab and to fix issue 329013.
Rather than having that tab fetch a list of children of the root node,
it now asks for a list of all known data types.  This is a better
solution, since it does not require sync to be fully initialized in
order to properly populate the set of checkboxes.

The most significant user of the removed functionality is the node
browser.  Its javascript code has been modified in order to transition
it to relying on the getAllNodes function.  Rather than fetching node
data on demand by a series of asynchronous callbacks, the tab now
fetches a list of all known sync nodes at once.  This has the advantage
of providing a more consistent (though more stale) snapshot.  This CL
adds a refresh button to the node browser tab in order to give users
some control over its staleness.

This change had some minor side-effects.  The node browser now relies on
items' string-based IDs rather than their metahandles when determining
the nodes' hierarchy.  We've also had to add a 'positionIndex' field to
the output of getAllNodes to make it easier for the node browser to sort
its entries.  In part to make it easier to fetch this field, most of the
code associated with getAllNodes has been moved into the
syncable::Directory.

In addition to all these changes, this CL adds some webui tests to help
prevent regressions in about:sync functionality.  For now, they only
cover the node browser.  More tests will be added in future CLs.

BUG=110517, 329013, 328606
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245313

[rlarocque, 2014-01-10 21:46:34]

I'm still iterating against the trybots, but most of this patch is now stable. 
Please review.

In case you're wondering, my current plan is to land this patch, then work
towards transforming 'getAllNodes' into a 'getAllNodesForType', at least at as
far as the interface to Chrome is concerned.  We can emulate the old behavior
with the new API, so most of the JS code won't need to change right away.

I also have secondary goals around improving test coverage and eliminating much
of the current JsController infrastructure.  

The JsController seems to have been built to make it easier to abstract away
large parts of the declaration of JS callbacks, and to allow calls to be
deferred until the sync backend is initialized.  I don't think either feature is
likely to be valuable as we move towards a 'distributed' sync engine.  If you
see that I've failed to take advantage of the JsController infrastructure in
some cases (eg. when defining mock functions in tests), this is probably the
reason why.

[Nicolas Zea, 2014-01-13 20:10:14]

https://codereview.chromium.org/134443004/diff/80001/chrome/browser/resources...
File chrome/browser/resources/sync_internals/chrome_sync.js (right):

https://codereview.chromium.org/134443004/diff/80001/chrome/browser/resources...
chrome/browser/resources/sync_internals/chrome_sync.js:150:
'getListOfKnownTypes',
Why not just name this getListOfTypes? "known" is kind of redundant I think

https://codereview.chromium.org/134443004/diff/80001/chrome/browser/resources...
File chrome/browser/resources/sync_internals/node_browser.html (right):

https://codereview.chromium.org/134443004/diff/80001/chrome/browser/resources...
chrome/browser/resources/sync_internals/node_browser.html:16: <div
id="node-browser-container">
nit: call this sync-node-browser-container to be consistent?

https://codereview.chromium.org/134443004/diff/80001/chrome/browser/resources...
chrome/browser/resources/sync_internals/node_browser.html:20: <div
id="node-details" hidden>
move hidden attribute to CSS file?

https://codereview.chromium.org/134443004/diff/80001/chrome/browser/resources...
File chrome/browser/resources/sync_internals/node_browser.js (left):

https://codereview.chromium.org/134443004/diff/80001/chrome/browser/resources...
chrome/browser/resources/sync_internals/node_browser.js:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
did you delete this file? For some reason the CL shows this as modified, not
deleted (maybe just a rietveld thing tho).

https://codereview.chromium.org/134443004/diff/80001/chrome/browser/resources...
File chrome/browser/resources/sync_internals/sync_node_browser.js (right):

https://codereview.chromium.org/134443004/diff/80001/chrome/browser/resources...
chrome/browser/resources/sync_internals/sync_node_browser.js:33: * finally
sorting by METAHANDLE
Can the sorting not be done in the C++ code by having the nodes themselves be
part of an array, rather than a dictionary? This might make it so you don't have
to have a separate positionIndex

[rlarocque, 2014-01-13 21:56:17]

Some comments addressed.  PTAL.

https://codereview.chromium.org/134443004/diff/80001/chrome/browser/resources...
File chrome/browser/resources/sync_internals/chrome_sync.js (right):

https://codereview.chromium.org/134443004/diff/80001/chrome/browser/resources...
chrome/browser/resources/sync_internals/chrome_sync.js:150:
'getListOfKnownTypes',
On 2014/01/13 20:10:14, Nicolas Zea wrote:
> Why not just name this getListOfTypes? "known" is kind of redundant I think

Good point.

I think I was planning to hook it up to the set of registered types at some
point, but that's probably not worth the effort for this use case.

I'll rename it.

https://codereview.chromium.org/134443004/diff/80001/chrome/browser/resources...
File chrome/browser/resources/sync_internals/node_browser.html (right):

https://codereview.chromium.org/134443004/diff/80001/chrome/browser/resources...
chrome/browser/resources/sync_internals/node_browser.html:16: <div
id="node-browser-container">
On 2014/01/13 20:10:14, Nicolas Zea wrote:
> nit: call this sync-node-browser-container to be consistent?

Done.

https://codereview.chromium.org/134443004/diff/80001/chrome/browser/resources...
chrome/browser/resources/sync_internals/node_browser.html:20: <div
id="node-details" hidden>
On 2014/01/13 20:10:14, Nicolas Zea wrote:
> move hidden attribute to CSS file?

Done.

https://codereview.chromium.org/134443004/diff/80001/chrome/browser/resources...
File chrome/browser/resources/sync_internals/node_browser.js (left):

https://codereview.chromium.org/134443004/diff/80001/chrome/browser/resources...
chrome/browser/resources/sync_internals/node_browser.js:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
On 2014/01/13 20:10:14, Nicolas Zea wrote:
> did you delete this file? For some reason the CL shows this as modified, not
> deleted (maybe just a rietveld thing tho).

I deleted it.

https://codereview.chromium.org/134443004/diff/80001/chrome/browser/resources...
File chrome/browser/resources/sync_internals/sync_node_browser.js (right):

https://codereview.chromium.org/134443004/diff/80001/chrome/browser/resources...
chrome/browser/resources/sync_internals/sync_node_browser.js:33: * finally
sorting by METAHANDLE
On 2014/01/13 20:10:14, Nicolas Zea wrote:
> Can the sorting not be done in the C++ code by having the nodes themselves be
> part of an array, rather than a dictionary? This might make it so you don't
have
> to have a separate positionIndex

That could work.  I can see two reasons in favor of the current approach,
though:

- Laziness.  With the current system, we don't need to sort the items until
they're actually expanded.  I don't know how expensive it would be to always
sort them on the C++ side, but it may be significant.

- Hierarchy.  Currently, hierarchy is managed by some cleverness in
handleExpand_.  If we were to sort in C++, we might need to take hierarchy into
account there.  Alternatively, we could sort all nodes as if they were siblings
in C++ then be careful to not reorder items when we go looking for child items.

I have a slight preference for the current system, but I'd be willing to try the
alternatives if you feel strongly about it.

[rlarocque, 2014-01-14 01:06:45]

+sky for OWNERS stamp and more reviewing.

sky: Please review changes in 
- chrome/browser/resources/sync_internals_resources.grd
- chrome/browser/ui/webui/*

[sky, 2014-01-14 01:42:35]

estade is a better reviewer for webui changes, so:
+estade
-sky

[Evan Stade, 2014-01-14 17:32:56]

lgtm modulo nits

https://codereview.chromium.org/134443004/diff/190001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/sync_internals_message_handler.cc (right):

https://codereview.chromium.org/134443004/diff/190001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:50:
base::Bind(&SyncInternalsMessageHandler::OnGetListOfKnownTypes,
nit: please make message string and function name match (standardize
"KnownTypes" vs "Types")

https://codereview.chromium.org/134443004/diff/190001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:70: //
TODO(rlarocque): We should DCHECK(!args) here.
so why don't you?

[rlarocque, 2014-01-14 20:59:38]

Patch updated.

Nicolas: PTAL.

https://codereview.chromium.org/134443004/diff/190001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/sync_internals_message_handler.cc (right):

https://codereview.chromium.org/134443004/diff/190001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:50:
base::Bind(&SyncInternalsMessageHandler::OnGetListOfKnownTypes,
On 2014/01/14 17:32:57, Evan Stade wrote:
> nit: please make message string and function name match (standardize
> "KnownTypes" vs "Types")

Oops.  My search and replace missed instances with a capital 'G'.

Fixed.

https://codereview.chromium.org/134443004/diff/190001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:70: //
TODO(rlarocque): We should DCHECK(!args) here.
On 2014/01/14 17:32:57, Evan Stade wrote:
> so why don't you?

See chrome_sync.js.  It has a generic function definition infrastructure.  It
slices and dices the arguments array and seems to always pass at least one
argument through the chrome.send() call, regardless of how the function it
defines is invoked.

I'd like to fix this eventually, either by improving or removing that
infrastructure.  I'm currently leaning towards the former, since this is
probably an easy bug to fix.  I want to save that for a separate patch, though,
since it might break things in subtle ways and I'd like to be able to revert it
independently if something goes wrong.

[Evan Stade, 2014-01-14 22:07:52]

https://codereview.chromium.org/134443004/diff/190001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/sync_internals_message_handler.cc (right):

https://codereview.chromium.org/134443004/diff/190001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:70: //
TODO(rlarocque): We should DCHECK(!args) here.
On 2014/01/14 20:59:39, rlarocque wrote:
> On 2014/01/14 17:32:57, Evan Stade wrote:
> > so why don't you?
> 
> See chrome_sync.js.  It has a generic function definition infrastructure.  It
> slices and dices the arguments array and seems to always pass at least one
> argument through the chrome.send() call, regardless of how the function it
> defines is invoked.
> 
> I'd like to fix this eventually, either by improving or removing that
> infrastructure.  I'm currently leaning towards the former, since this is
> probably an easy bug to fix.  I want to save that for a separate patch,
though,
> since it might break things in subtle ways and I'd like to be able to revert
it
> independently if something goes wrong.

all this info would be good to have in a crbug which you link to in this TODO

[Nicolas Zea, 2014-01-14 22:14:32]

lgtm

https://codereview.chromium.org/134443004/diff/80001/chrome/browser/resources...
File chrome/browser/resources/sync_internals/sync_node_browser.js (right):

https://codereview.chromium.org/134443004/diff/80001/chrome/browser/resources...
chrome/browser/resources/sync_internals/sync_node_browser.js:33: * finally
sorting by METAHANDLE
On 2014/01/13 21:56:17, rlarocque wrote:
> On 2014/01/13 20:10:14, Nicolas Zea wrote:
> > Can the sorting not be done in the C++ code by having the nodes themselves
be
> > part of an array, rather than a dictionary? This might make it so you don't
> have
> > to have a separate positionIndex
> 
> That could work.  I can see two reasons in favor of the current approach,
> though:
> 
> - Laziness.  With the current system, we don't need to sort the items until
> they're actually expanded.  I don't know how expensive it would be to always
> sort them on the C++ side, but it may be significant.
> 
> - Hierarchy.  Currently, hierarchy is managed by some cleverness in
> handleExpand_.  If we were to sort in C++, we might need to take hierarchy
into
> account there.  Alternatively, we could sort all nodes as if they were
siblings
> in C++ then be careful to not reorder items when we go looking for child
items.
> 
> I have a slight preference for the current system, but I'd be willing to try
the
> alternatives if you feel strongly about it.

Maybe just add a note that we can consider sorting in C++ if this becomes
expensive for large accounts.

[rlarocque, 2014-01-14 22:20:43]

Thanks for the review.

I won't try to land this right away.  It's possible that some of this will
conflict with Manuel's changes, so I'll wait for those to land before rebasing
then sending this to the CQ.

https://codereview.chromium.org/134443004/diff/80001/chrome/browser/resources...
File chrome/browser/resources/sync_internals/sync_node_browser.js (right):

https://codereview.chromium.org/134443004/diff/80001/chrome/browser/resources...
chrome/browser/resources/sync_internals/sync_node_browser.js:33: * finally
sorting by METAHANDLE
On 2014/01/14 22:14:33, Nicolas Zea wrote:
> On 2014/01/13 21:56:17, rlarocque wrote:
> > On 2014/01/13 20:10:14, Nicolas Zea wrote:
> > > Can the sorting not be done in the C++ code by having the nodes themselves
> be
> > > part of an array, rather than a dictionary? This might make it so you
don't
> > have
> > > to have a separate positionIndex
> > 
> > That could work.  I can see two reasons in favor of the current approach,
> > though:
> > 
> > - Laziness.  With the current system, we don't need to sort the items until
> > they're actually expanded.  I don't know how expensive it would be to always
> > sort them on the C++ side, but it may be significant.
> > 
> > - Hierarchy.  Currently, hierarchy is managed by some cleverness in
> > handleExpand_.  If we were to sort in C++, we might need to take hierarchy
> into
> > account there.  Alternatively, we could sort all nodes as if they were
> siblings
> > in C++ then be careful to not reorder items when we go looking for child
> items.
> > 
> > I have a slight preference for the current system, but I'd be willing to try
> the
> > alternatives if you feel strongly about it.
> 
> Maybe just add a note that we can consider sorting in C++ if this becomes
> expensive for large accounts.

Done.

https://codereview.chromium.org/134443004/diff/190001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/sync_internals_message_handler.cc (right):

https://codereview.chromium.org/134443004/diff/190001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:70: //
TODO(rlarocque): We should DCHECK(!args) here.
On 2014/01/14 22:07:52, Evan Stade wrote:
> On 2014/01/14 20:59:39, rlarocque wrote:
> > On 2014/01/14 17:32:57, Evan Stade wrote:
> > > so why don't you?
> > 
> > See chrome_sync.js.  It has a generic function definition infrastructure. 
It
> > slices and dices the arguments array and seems to always pass at least one
> > argument through the chrome.send() call, regardless of how the function it
> > defines is invoked.
> > 
> > I'd like to fix this eventually, either by improving or removing that
> > infrastructure.  I'm currently leaning towards the former, since this is
> > probably an easy bug to fix.  I want to save that for a separate patch,
> though,
> > since it might break things in subtle ways and I'd like to be able to revert
> it
> > independently if something goes wrong.
> 
> all this info would be good to have in a crbug which you link to in this TODO

Done.

[commit-bot: I haz the power, 2014-01-16 18:38:38]

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

[commit-bot: I haz the power, 2014-01-16 20:59:05]

Change committed as 245313