sync: Re-implement getAllNodes WebUI function

sync: Re-implement getAllNodes WebUI function

Removes the existing implementation of getAllNodes.  This was the last
function based on the old "makeSyncFunction" and "JsMessageHandler"
interface, so its removal leaves behind quite a bit of dead code.  This
CL removes some of it.  The rest will be removed in a future CL.

Replaces it with some infrastructure intended to be more compatible with
the future of sync, where each type is more independent.  Requests to
getAllNodes are routed through a DirectoryTypeDebugInfoEmitter in the
ModelTypeRegistry.  A NonBlockingTypeDebugInfoEmitter will be
implemented in a future CL.

The new system also intended to support "streaming" results.  Rather
than waiting for all types to return their nodes, the system could be
modified to allow some types to get back to the JavaScript layer sooner
than others so it can display results earlier.  However, we do not
currently take advantage of this functionality.

The return type of getAllNodes is now an array of per-type objects,
rather than one big list of nodes.  This, too, should help us implement
streaming support in the future.  For now, most of the JavaScript
callbacks will convert the data back to the old format before operating
on it.

BUG=328606
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262193

[rlarocque, 2014-04-04 20:50:31]

dbeam: Please review the files in chrome/browser/resources and chrome/browser/ui

zea: Please review the rest.

[Dan Beam, 2014-04-04 22:36:30]

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

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/chrome_sync.js:71: 
/** Doc comment of what these do and how they act. */

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/chrome_sync.js:77: * Will eventually
invoke the given callback with the results.
@param {function(!Object)} callback A description of |callback| goes here.

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/chrome_sync.js:92: delete
requestCallbacks[id];
i think technically we don't like delete in webui code, maybe

  requestCallbacks[id] = undefined;

instead?

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/chrome_sync.js:99: 
nit: remove blank lines

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/sync_internals/data.js (right):

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/data.js:61: * @param {Object} node A
JavaScript represenation of a sync entity.
nit: !Object

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/data.js:129: * @param {!Array.<{type:
string, nodes: Array<Object>}>} nodesMap
technically this should probably be:

  nodes: !Array.<!Object>

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

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/sync_node_browser.js:173: * Fetch the
latest set of nodes and refresh the UI.
@param {...} nodeMap

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/sync_node_browser.js:181:
chrome.sync.getAllNodes(function(node_map) {
varsLikeThis (e.g. nodeMap)

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/sync_node_browser.js:181:
chrome.sync.getAllNodes(function(node_map) {
what is nodemap?  an array?

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/sync_internals_browsertest.js (right):

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_browsertest.js:67:
"ATTACHMENT_METADATA":"",
nit: please put a \s after each key, e.g. "BASE_SERVER_SPECIFICS": {},

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_browsertest.js:67:
"ATTACHMENT_METADATA":"",
protip: http://jsbeautifier.org/

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_browsertest.js:125: "SERVER_SPECIFICS": {
"autofill": {
nit:

"SERVER_SPECIFICS": {
  "autofill": {
    ...
  },
}

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_browsertest.js:286:
this.mockHandler.expects(once())
nit: . at end (it's an operator)

  this.mockHandler.expects(once()).
      getAllNodes(...

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

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_message_handler.cc:101: const
base::Value* request_id;
can you use more specific type here?  is this a string or a number, for
instance?  also, coverity will while unless you initialize this to NULL, if I
had to guess.

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_message_handler.cc:102: args->Get(0,
&request_id);
bool success = args->Get(0, &request_id);
DCHECK(success);

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_message_handler.cc:122: response_args);
nit:

web_ui()->CallJavascriptFunction("chrome.sync.getAllNodesCallback",
                                 response_args);

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/sync_internals_message_handler.h (right):

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_message_handler.h:44: // Asynchronous
request for a list of all sync nodes.
nit: // Handler for the "getAllNodes" message. Needs a |request_id| argument.

[Nicolas Zea, 2014-04-04 23:01:27]

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:2088:
base::Callback<void(scoped_ptr<base::ListValue>)> callback);
pass callbacks by const ref, here and elsewhere

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:2090: void OnReceivedNodesForTypes(
Comment for method. In particular, what is |types|.

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:2091: std::vector<syncer::ModelType>
types,
pass types by const ref (and maybe scoped_node_lists too?)

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:2096: ~GetAllNodesRequestHelper();
virtual

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:2107: : result_accumulator_(new
base::ListValue()),
indent by two more spaces

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:2144: callback_ =
base::Callback<void(scoped_ptr<base::ListValue>)>();
callback_.Reset()?

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:2154:
scoped_refptr<GetAllNodesRequestHelper> helper =
Do we want to support multiple oustanding requests at once? Perhaps have the PSS
own a helper_ object, so it can track whether an outstanding request is being
fulfilled or not?

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:280: // returns to this the specified
callback on this thread.
"and returns to this the specified" -> "and returns them to the specified"

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:283: // For safety, the callback
should contain some sort of WeakPtr<> or
"should contain" -> "should be bound to"

https://codereview.chromium.org/224563004/diff/20001/sync/engine/directory_ty...
File sync/engine/directory_type_debug_info_emitter.h (right):

https://codereview.chromium.org/224563004/diff/20001/sync/engine/directory_ty...
sync/engine/directory_type_debug_info_emitter.h:15: // Suports debugging
requests for a certain directory type.
Why only for one type? It seems more efficient to have it be capable of handler
a full ModelTypeSet?

https://codereview.chromium.org/224563004/diff/20001/sync/syncable/directory.h
File sync/syncable/directory.h (right):

https://codereview.chromium.org/224563004/diff/20001/sync/syncable/directory....
sync/syncable/directory.h:331: scoped_ptr<base::ListValue>
GetNodeDetailsForType(
Coupled with the comment about having the DirectoryDebugInfoEmitter handle
multiple types, I think I'd like to see this also handle a ModelTypeSet, to
minimize the number of times we need to invoke this.

[rlarocque, 2014-04-05 00:07:27]

Patch updated.  PTAL.

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

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/chrome_sync.js:71: 
On 2014/04/04 22:36:31, Dan Beam wrote:
> /** Doc comment of what these do and how they act. */

Done.

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/chrome_sync.js:77: * Will eventually
invoke the given callback with the results.
On 2014/04/04 22:36:31, Dan Beam wrote:
> @param {function(!Object)} callback A description of |callback| goes here.

Done.

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/chrome_sync.js:92: delete
requestCallbacks[id];
On 2014/04/04 22:36:31, Dan Beam wrote:
> i think technically we don't like delete in webui code, maybe
> 
>   requestCallbacks[id] = undefined;
> 
> instead?

Works for me.  I'm just trying to avoid gratuitous memory leaks, and that should
do the job.

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/chrome_sync.js:99: 
On 2014/04/04 22:36:31, Dan Beam wrote:
> nit: remove blank lines

Done.

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/sync_internals/data.js (right):

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/data.js:61: * @param {Object} node A
JavaScript represenation of a sync entity.
On 2014/04/04 22:36:31, Dan Beam wrote:
> nit: !Object

Done.

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/data.js:129: * @param {!Array.<{type:
string, nodes: Array<Object>}>} nodesMap
On 2014/04/04 22:36:31, Dan Beam wrote:
> technically this should probably be:
> 
>   nodes: !Array.<!Object>

Done.

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

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/sync_node_browser.js:173: * Fetch the
latest set of nodes and refresh the UI.
On 2014/04/04 22:36:31, Dan Beam wrote:
> @param {...} nodeMap

This function doesn't take any arguments.

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/sync_node_browser.js:181:
chrome.sync.getAllNodes(function(node_map) {
On 2014/04/04 22:36:31, Dan Beam wrote:
> what is nodemap?  an array?

An array of objects.  The objects are of the form

{
  type: "Bookmarks",
  nodes: [ ... object representations of all bookmark nodes ... ],
}

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/sync_node_browser.js:181:
chrome.sync.getAllNodes(function(node_map) {
On 2014/04/04 22:36:31, Dan Beam wrote:
> varsLikeThis (e.g. nodeMap)

Done.

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:2088:
base::Callback<void(scoped_ptr<base::ListValue>)> callback);
On 2014/04/04 23:01:27, Nicolas Zea wrote:
> pass callbacks by const ref, here and elsewhere

Done.

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:2090: void OnReceivedNodesForTypes(
On 2014/04/04 23:01:27, Nicolas Zea wrote:
> Comment for method. In particular, what is |types|.

Done.

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:2091: std::vector<syncer::ModelType>
types,
On 2014/04/04 23:01:27, Nicolas Zea wrote:
> pass types by const ref (and maybe scoped_node_lists too?)

The scoped_node_list is not const for a reason.  This function call transfers
ownership of the ScopedVector.

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:2096: ~GetAllNodesRequestHelper();
On 2014/04/04 23:01:27, Nicolas Zea wrote:
> virtual

Done.

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:2107: : result_accumulator_(new
base::ListValue()),
On 2014/04/04 23:01:27, Nicolas Zea wrote:
> indent by two more spaces

Done.

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:2144: callback_ =
base::Callback<void(scoped_ptr<base::ListValue>)>();
On 2014/04/04 23:01:27, Nicolas Zea wrote:
> callback_.Reset()?

Thanks.  I was looking for that method, but I couldn't remember its name.

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:2154:
scoped_refptr<GetAllNodesRequestHelper> helper =
On 2014/04/04 23:01:27, Nicolas Zea wrote:
> Do we want to support multiple oustanding requests at once? Perhaps have the
PSS
> own a helper_ object, so it can track whether an outstanding request is being
> fulfilled or not?

It does support multiple outstanding requests.  The GetAllNodesRequestHelper is
that helper object.

The GetAllNodesRequestHelper is RefCountedThreadSafe.  The idea is that we can
split up the node fetching into several different callbacks, and each of them
will increment the helper object's refcout as long as it exists.  Once those
requests are issued, the ProfileSyncService lets go of it.  It will continue to
exist until all the callbacks that have bound to it return.  When that happens,
it will invoke its callback then get deleted as its last callback goes out of
scope.

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:280: // returns to this the specified
callback on this thread.
On 2014/04/04 23:01:27, Nicolas Zea wrote:
> "and returns to this the specified" -> "and returns them to the specified"

Done.

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:283: // For safety, the callback
should contain some sort of WeakPtr<> or
On 2014/04/04 23:01:27, Nicolas Zea wrote:
> "should contain" -> "should be bound to"

Done.

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/sync_internals_browsertest.js (right):

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_browsertest.js:67:
"ATTACHMENT_METADATA":"",
On 2014/04/04 22:36:31, Dan Beam wrote:
> protip: http://jsbeautifier.org/

That's much nicer.  Thanks!

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_browsertest.js:125: "SERVER_SPECIFICS": {
"autofill": {
On 2014/04/04 22:36:31, Dan Beam wrote:
> nit:
> 
> "SERVER_SPECIFICS": {
>   "autofill": {
>     ...
>   },
> }

Done.

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_browsertest.js:286:
this.mockHandler.expects(once())
On 2014/04/04 22:36:31, Dan Beam wrote:
> nit: . at end (it's an operator)
> 
>   this.mockHandler.expects(once()).
>       getAllNodes(...

Done.

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

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_message_handler.cc:101: const
base::Value* request_id;
On 2014/04/04 22:36:31, Dan Beam wrote:
> can you use more specific type here?  is this a string or a number, for
> instance?  also, coverity will while unless you initialize this to NULL, if I
> had to guess.

For a while I toyed with the idea of allowing any JSON value to be used as an
identifier.  Since I ended up hiding the whole concept behind an API in
chrome_sync.js, there's no longer much point in being that flexible.  I'll
assert that it's a integer.

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_message_handler.cc:102: args->Get(0,
&request_id);
On 2014/04/04 22:36:31, Dan Beam wrote:
> bool success = args->Get(0, &request_id);
> DCHECK(success);

Done.

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/sync_internals_message_handler.h (right):

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_message_handler.h:44: // Asynchronous
request for a list of all sync nodes.
On 2014/04/04 22:36:31, Dan Beam wrote:
> nit: // Handler for the "getAllNodes" message. Needs a |request_id| argument.

Done.

https://codereview.chromium.org/224563004/diff/20001/sync/engine/directory_ty...
File sync/engine/directory_type_debug_info_emitter.h (right):

https://codereview.chromium.org/224563004/diff/20001/sync/engine/directory_ty...
sync/engine/directory_type_debug_info_emitter.h:15: // Suports debugging
requests for a certain directory type.
On 2014/04/04 23:01:27, Nicolas Zea wrote:
> Why only for one type? It seems more efficient to have it be capable of
handler
> a full ModelTypeSet?

I'm hoping this pattern of one object per type will turn out to be useful in the
future.  This class will be reused to implement per-type application counts,
commit counts, etc.

I suppose we could make a special case for GetAllNodes(), but the efficiency
difference isn't all that great.  The really expensive part of this operation is
serialization.  Iterating over the directory 20+ times is a drop in the bucket
compared to that cost.  Besides, we only need to pay this cost when we're
debugging, so it's not something I'm inclined to optimize very much.

[Dan Beam, 2014-04-05 00:19:24]

lgtm

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

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/sync_node_browser.js:173: * Fetch the
latest set of nodes and refresh the UI.
On 2014/04/05 00:07:27, rlarocque wrote:
> On 2014/04/04 22:36:31, Dan Beam wrote:
> > @param {...} nodeMap
> 
> This function doesn't take any arguments.

oh whoops, yeah, sorry

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

https://codereview.chromium.org/224563004/diff/40001/chrome/browser/resources...
chrome/browser/resources/sync_internals/chrome_sync.js:80: * Used in the
implementation of GetAllNodes.
nit: @type {{number: !Function}} (though technically all object keys are strings
in JS)

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

https://codereview.chromium.org/224563004/diff/40001/chrome/browser/resources...
chrome/browser/resources/sync_internals/sync_node_browser.js:185:
.reduce(function(a, b) { return a.concat(b); });
. at end

https://codereview.chromium.org/224563004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/sync_internals/sync_search.js (right):

https://codereview.chromium.org/224563004/diff/40001/chrome/browser/resources...
chrome/browser/resources/sync_internals/sync_search.js:53: .map(function(x) {
return x.nodes; })
. at end

https://codereview.chromium.org/224563004/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/sync_internals_browsertest.js (right):

https://codereview.chromium.org/224563004/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_browsertest.js:154: }, ],
nit: }], to be consistent with below

https://codereview.chromium.org/224563004/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/sync_internals_message_handler.h (right):

https://codereview.chromium.org/224563004/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_message_handler.h:55:
scoped_ptr<base::ListValue> nodes);
nit:

void OnReceivedAllNodes(int request_id,
                        scoped_ptr<base::ListValue> nodes);

[rlarocque, 2014-04-05 00:27:29]

Thanks for the review.

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

https://codereview.chromium.org/224563004/diff/40001/chrome/browser/resources...
chrome/browser/resources/sync_internals/chrome_sync.js:80: * Used in the
implementation of GetAllNodes.
On 2014/04/05 00:19:25, Dan Beam wrote:
> nit: @type {{number: !Function}} (though technically all object keys are
strings
> in JS)

Done.

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

https://codereview.chromium.org/224563004/diff/40001/chrome/browser/resources...
chrome/browser/resources/sync_internals/sync_node_browser.js:185:
.reduce(function(a, b) { return a.concat(b); });
On 2014/04/05 00:19:25, Dan Beam wrote:
> . at end

Done.

https://codereview.chromium.org/224563004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/sync_internals/sync_search.js (right):

https://codereview.chromium.org/224563004/diff/40001/chrome/browser/resources...
chrome/browser/resources/sync_internals/sync_search.js:53: .map(function(x) {
return x.nodes; })
On 2014/04/05 00:19:25, Dan Beam wrote:
> . at end

Done.

https://codereview.chromium.org/224563004/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/sync_internals_browsertest.js (right):

https://codereview.chromium.org/224563004/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_browsertest.js:154: }, ],
On 2014/04/05 00:19:25, Dan Beam wrote:
> nit: }], to be consistent with below

Done.

https://codereview.chromium.org/224563004/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/sync_internals_message_handler.h (right):

https://codereview.chromium.org/224563004/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_message_handler.h:55:
scoped_ptr<base::ListValue> nodes);
On 2014/04/05 00:19:25, Dan Beam wrote:
> nit:
> 
> void OnReceivedAllNodes(int request_id,
>                         scoped_ptr<base::ListValue> nodes);

Done.  (It all fits on one line.)

[Nicolas Zea, 2014-04-05 00:30:29]

LGTM

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:2154:
scoped_refptr<GetAllNodesRequestHelper> helper =
On 2014/04/05 00:07:27, rlarocque wrote:
> On 2014/04/04 23:01:27, Nicolas Zea wrote:
> > Do we want to support multiple oustanding requests at once? Perhaps have the
> PSS
> > own a helper_ object, so it can track whether an outstanding request is
being
> > fulfilled or not?
> 
> It does support multiple outstanding requests.  The GetAllNodesRequestHelper
is
> that helper object.
> 
> The GetAllNodesRequestHelper is RefCountedThreadSafe.  The idea is that we can
> split up the node fetching into several different callbacks, and each of them
> will increment the helper object's refcout as long as it exists.  Once those
> requests are issued, the ProfileSyncService lets go of it.  It will continue
to
> exist until all the callbacks that have bound to it return.  When that
happens,
> it will invoke its callback then get deleted as its last callback goes out of
> scope.  

I realize it wasn't clear, but I was actually arguing against allowing multiple
outstanding requests. But I guess the biggest risk would be the user refreshing
a bunch of times and therefore triggering a ton of work, which isn't a big issue
(and I suspect involves refreshing the page in addition to hitting the refresh
button).

https://codereview.chromium.org/224563004/diff/40001/sync/engine/directory_ty...
File sync/engine/directory_type_debug_info_emitter.h (right):

https://codereview.chromium.org/224563004/diff/40001/sync/engine/directory_ty...
sync/engine/directory_type_debug_info_emitter.h:15: // Suports debugging
requests for a certain directory type.
Suports -> Supports

[rlarocque, 2014-04-05 00:42:05]

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/224563004/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:2154:
scoped_refptr<GetAllNodesRequestHelper> helper =
On 2014/04/05 00:30:29, Nicolas Zea wrote:
> On 2014/04/05 00:07:27, rlarocque wrote:
> > On 2014/04/04 23:01:27, Nicolas Zea wrote:
> > > Do we want to support multiple oustanding requests at once? Perhaps have
the
> > PSS
> > > own a helper_ object, so it can track whether an outstanding request is
> being
> > > fulfilled or not?
> > 
> > It does support multiple outstanding requests.  The GetAllNodesRequestHelper
> is
> > that helper object.
> > 
> > The GetAllNodesRequestHelper is RefCountedThreadSafe.  The idea is that we
can
> > split up the node fetching into several different callbacks, and each of
them
> > will increment the helper object's refcout as long as it exists.  Once those
> > requests are issued, the ProfileSyncService lets go of it.  It will continue
> to
> > exist until all the callbacks that have bound to it return.  When that
> happens,
> > it will invoke its callback then get deleted as its last callback goes out
of
> > scope.  
> 
> I realize it wasn't clear, but I was actually arguing against allowing
multiple
> outstanding requests. But I guess the biggest risk would be the user
refreshing
> a bunch of times and therefore triggering a ton of work, which isn't a big
issue
> (and I suspect involves refreshing the page in addition to hitting the refresh
> button).

Fortunately, the UI greys out the refresh button while a request is in progress.

https://codereview.chromium.org/224563004/diff/40001/sync/engine/directory_ty...
File sync/engine/directory_type_debug_info_emitter.h (right):

https://codereview.chromium.org/224563004/diff/40001/sync/engine/directory_ty...
sync/engine/directory_type_debug_info_emitter.h:15: // Suports debugging
requests for a certain directory type.
On 2014/04/05 00:30:29, Nicolas Zea wrote:
> Suports -> Supports

Done.

[rlarocque, 2014-04-07 17:21:07]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-04-07 17:21:25]

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

[commit-bot: I haz the power, 2014-04-07 20:30:42]

Change committed as 262193