sync: Add UI code for per type debug counters

sync: Add UI code for per type debug counters

Adds a new tab to the about:sync page to display the latest per-type
counters.  These counters will eventually replace the StatusControllers
that contain much of the same information.

This includes some changes to the C++ SyncInternalsMessageHandler to
register to observe these counters as well as some new WebUI to display
them.

There is a known bug in this code.  The StausCounter update logic has
not been implemented yet, so the item counts will always be zero.  This
will be fixed in a future CL.

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

[rlarocque, 2014-05-15 20:21:42]

The quest to remove StatusController and the SyncSessionSnapshot continues.

Please review.

jhawkins is an OWNER for these files, so it's his review I'll be waiting for.

Tim is welcome to comment, too.  He's been reviewing the sync parts of this
change, but I don't think his approval is necessary for this particular CL.

[rlarocque, 2014-05-16 22:33:20]

Since this hasn't been reviewed yet, I'm going to take this opportunity to
update the CL with some naming changes.  This squashes in some changes that were
further down the patch stack.  It should save us the trouble of reviewing some
trivial webUI changes later on.

That should be the only significant change in the latest upload.  It's still
ready for review.

[James Hawkins, 2014-05-19 02:55:32]

I'm not an owner of sync_internals.

[rlarocque, 2014-05-19 17:15:41]

On 2014/05/19 02:55:32, James Hawkins wrote:
> I'm not an owner of sync_internals.

Good point.

I guess this falls to Tim, then.  He's already on the review list.

[rlarocque, 2014-05-22 20:24:08]

Tim is out this week, and no one on the sync team knows JavaScript well enough
to volunteer for this.

I'm going to add Nick as a reviewer.  Nicolas is welcome to take a second look,
too.

[maniscalco, 2014-05-23 00:54:59]

Here's my feedback.  Of course, I don't really know javascript so don't count on
me to catch bugs in the js code.

https://codereview.chromium.org/290023003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/sync_internals/types.html (right):

https://codereview.chromium.org/290023003/diff/40001/chrome/browser/resources...
chrome/browser/resources/sync_internals/types.html:18: <th>Server Overwrite
Conflict</th>
Is this a count? should it read Conflicts" instead of "Conflict"?  Same goes for
one line down.

https://codereview.chromium.org/290023003/diff/40001/chrome/browser/resources...
chrome/browser/resources/sync_internals/types.html:30: <td
jscontent="counters.numEntries || 0">0</td>
What does the || 0 do?

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

https://codereview.chromium.org/290023003/diff/40001/chrome/browser/resources...
chrome/browser/resources/sync_internals/types.js:73: 'onCountersUpdated',
Does it matter that this says onCountersUpdated instead of
onReceivedCountersUpdate ?

https://codereview.chromium.org/290023003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/sync_internals_resources.grd (right):

https://codereview.chromium.org/290023003/diff/40001/chrome/browser/resources...
chrome/browser/resources/sync_internals_resources.grd:17: <include
name="IDR_SYNC_INTERNALS_TYPES_JS"  file="sync_internals/types.js"
type="BINDATA" />
How about this whitelist, do we need to update it as well?

https://code.google.com/p/chromium/codesearch#chromium/src/build/ios/grit_whi...

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

https://codereview.chromium.org/290023003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_message_handler.cc:57:
"registerForPerTypeCounters",
I wonder if we could tie into the existing "registerForEvents"
(HandleRegisterForEvents) callback instead of creating a separate one for per
type counters.  If so, we could drop the new flag.  Does anything prohibit us
from having just one register callback?

https://codereview.chromium.org/290023003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_message_handler.cc:172:
scoped_ptr<base::DictionaryValue> details(new base::DictionaryValue());
Too bad syncer::FooCounters don't have a common base class.  WDYT about using a
function template to reduce duplication?

template <typename T>
scoped_ptr<base::DictionaryValue> CreateDictionaryWithCounters(
    syncer::ModelType type,
    const std::string& description,
    const T& counters) {
  details->SetString("modelType", ModelTypeToString(type));
  details->SetString("counterType", description);
  details->Set("counters", counters.ToValue().release());
}

[rlarocque, 2014-05-27 20:13:25]

Patch updated.

https://codereview.chromium.org/290023003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/sync_internals/types.html (right):

https://codereview.chromium.org/290023003/diff/40001/chrome/browser/resources...
chrome/browser/resources/sync_internals/types.html:18: <th>Server Overwrite
Conflict</th>
On 2014/05/23 00:55:00, maniscalco wrote:
> Is this a count? should it read Conflicts" instead of "Conflict"?  Same goes
for
> one line down.

Right, it should be plural.

These are meant to indicate "There was a conflict, and it was resolved in favor
of the {server,client}".  Let me know if you can think of a better way to name
it.

https://codereview.chromium.org/290023003/diff/40001/chrome/browser/resources...
chrome/browser/resources/sync_internals/types.html:30: <td
jscontent="counters.numEntries || 0">0</td>
On 2014/05/23 00:55:00, maniscalco wrote:
> What does the || 0 do?

It's a bit of a hack to initialize the table to show all zeroes if the
information we're looking for hasn't been received yet.

If counters.numEntries is "falsy", the || expression will return the second
argument (0).  Things like false, null, undefined, and NaN are considered
"falsy".  The 0 value is also falsy, but that's not a big issue since the
short-circuit will end up giving us back a zero value anyway.

The alternatives would be to leave the <td> entries blank if no data has been
received yet, or to initialize the map on the JavaScript side to all zeroes. 
The latter sounds like a good idea, until you realize it means that these names
need to be specified (and kept up to date) in yet another file.

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

https://codereview.chromium.org/290023003/diff/40001/chrome/browser/resources...
chrome/browser/resources/sync_internals/types.js:73: 'onCountersUpdated',
On 2014/05/23 00:55:00, maniscalco wrote:
> Does it matter that this says onCountersUpdated instead of
> onReceivedCountersUpdate ?

Not technically, no.  One specifies the type of event and the other specifies
the function to call when it occurs.

There's also no harm in making them the same.  I guess I'll just do that.

https://codereview.chromium.org/290023003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/sync_internals_resources.grd (right):

https://codereview.chromium.org/290023003/diff/40001/chrome/browser/resources...
chrome/browser/resources/sync_internals_resources.grd:17: <include
name="IDR_SYNC_INTERNALS_TYPES_JS"  file="sync_internals/types.js"
type="BINDATA" />
On 2014/05/23 00:55:00, maniscalco wrote:
> How about this whitelist, do we need to update it as well?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/ios/grit_whi...

I think so.  Done.

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

https://codereview.chromium.org/290023003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_message_handler.cc:57:
"registerForPerTypeCounters",
On 2014/05/23 00:55:00, maniscalco wrote:
> I wonder if we could tie into the existing "registerForEvents"
> (HandleRegisterForEvents) callback instead of creating a separate one for per
> type counters.  If so, we could drop the new flag.  Does anything prohibit us
> from having just one register callback?

It gets complicated.

The problem is that this registration also implicitly sends a request to
immediately issue a series of events that should initialize the newly-registered
listener's state.

If the registration happens as part of the registerForEvents() call in about.js,
we'd have no guarantee that the listeners in types.js will have been setup and
listening by that time.  I think that may be a risk even if the initialization
request and registration were separate, since (I suppose) types.js could request
initialization before registerForEvents() is called, and I'm not sure what the
semantics of that would be.

===

We could probably refactor around this.  I think the eventual "right" way to fix
this would be to implement an abstraction layer between the chrome.send() calls
and responses and the page display logic.  All data received from Chrome would
be stored in a cache.  Listeners could query it explicitly or request
notifications of updates.  sync_log.js follows this pattern.

On the other hand, this is supposed to be a simple, developer-facing UI for
displaying data exported from sync.  It would be nice if we could keep this page
as dumb as possible.  I'm not sure if implementing the technically "right"
solution would be a good idea in this context.

https://codereview.chromium.org/290023003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_message_handler.cc:172:
scoped_ptr<base::DictionaryValue> details(new base::DictionaryValue());
On 2014/05/23 00:55:00, maniscalco wrote:
> Too bad syncer::FooCounters don't have a common base class.  WDYT about using
a
> function template to reduce duplication?
> 
> template <typename T>
> scoped_ptr<base::DictionaryValue> CreateDictionaryWithCounters(
>     syncer::ModelType type,
>     const std::string& description,
>     const T& counters) {
>   details->SetString("modelType", ModelTypeToString(type));
>   details->SetString("counterType", description);
>   details->Set("counters", counters.ToValue().release());
> }

I'll admit that seems like a valid use of templates, but I think we have a
pretty strong bias against templates in our code.

I think your proposal still requires three separate listener functions, right? 
What if we pulled the ToValue() call out of proposed template function and into
the type-specific wrapper?  Then CreateDictionaryWithCounters could take a
DictionaryValue instead of a T.

[maniscalco, 2014-05-27 20:31:07]

https://codereview.chromium.org/290023003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/sync_internals/types.html (right):

https://codereview.chromium.org/290023003/diff/40001/chrome/browser/resources...
chrome/browser/resources/sync_internals/types.html:30: <td
jscontent="counters.numEntries || 0">0</td>
I see.  Thanks for the explanation.  Using "|| 0" here makes sense to me.

On 2014/05/27 20:13:26, rlarocque wrote:
> On 2014/05/23 00:55:00, maniscalco wrote:
> > What does the || 0 do?
> 
> It's a bit of a hack to initialize the table to show all zeroes if the
> information we're looking for hasn't been received yet.
> 
> If counters.numEntries is "falsy", the || expression will return the second
> argument (0).  Things like false, null, undefined, and NaN are considered
> "falsy".  The 0 value is also falsy, but that's not a big issue since the
> short-circuit will end up giving us back a zero value anyway.
> 
> The alternatives would be to leave the <td> entries blank if no data has been
> received yet, or to initialize the map on the JavaScript side to all zeroes. 
> The latter sounds like a good idea, until you realize it means that these
names
> need to be specified (and kept up to date) in yet another file.

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

https://codereview.chromium.org/290023003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_internals_message_handler.cc:172:
scoped_ptr<base::DictionaryValue> details(new base::DictionaryValue());
On 2014/05/27 20:13:26, rlarocque wrote:
> On 2014/05/23 00:55:00, maniscalco wrote:
> > Too bad syncer::FooCounters don't have a common base class.  WDYT about
using
> a
> > function template to reduce duplication?
> > 
> > template <typename T>
> > scoped_ptr<base::DictionaryValue> CreateDictionaryWithCounters(
> >     syncer::ModelType type,
> >     const std::string& description,
> >     const T& counters) {
> >   details->SetString("modelType", ModelTypeToString(type));
> >   details->SetString("counterType", description);
> >   details->Set("counters", counters.ToValue().release());
> > }
> 
> I'll admit that seems like a valid use of templates, but I think we have a
> pretty strong bias against templates in our code.
> 
> I think your proposal still requires three separate listener functions, right?

> What if we pulled the ToValue() call out of proposed template function and
into
> the type-specific wrapper?  Then CreateDictionaryWithCounters could take a
> DictionaryValue instead of a T.

Yep, you'd still have three listener functions, each with a call to
CreateDictionaryWithCounters.

Pulling the ToValue call up into the listener and passing a DictionaryValue to
the common helper function SGTM.

[rlarocque, 2014-05-28 00:01:53]

Implemented the helper function.

[maniscalco, 2014-05-28 00:05:33]

lgtm with one nit

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

https://codereview.chromium.org/290023003/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.h:78: const std::string
counter_type,
nit: looks like you forgot the & after std::string.

[rlarocque, 2014-05-28 00:07:57]

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

https://codereview.chromium.org/290023003/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.h:78: const std::string
counter_type,
On 2014/05/28 00:05:34, maniscalco wrote:
> nit: looks like you forgot the & after std::string.

Good point.  Fixed.

[rlarocque, 2014-05-28 20:45:06]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-05-28 20:46:12]

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

[commit-bot: I haz the power, 2014-05-28 21:43:14]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-05-28 21:50:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-28 21:50:22]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[rlarocque, 2014-05-30 18:48:45]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-05-30 22:32:47]

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

[commit-bot: I haz the power, 2014-05-31 01:35:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-05-31 01:38:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-31 01:38:58]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[rlarocque, 2014-06-02 17:07:29]

On 2014/05/31 01:38:58, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

Oh, right.  jhawkins may not be an owner of sync_internals, but we do need his
LGTM for c/b/ui/webui and c/b/resources.

jhawkins: Could you take another look?

[James Hawkins, 2014-06-02 18:22:22]

webui LGTM

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

https://codereview.chromium.org/290023003/diff/130001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.h:76: // Helper to emit
counter updates.
Optional, but encouraged: The method documentation is superfluous as-is.  I
suggest phrasing it in such a way that someone not familiar with the code can
understand what it does and why.

[rlarocque, 2014-06-02 18:36:27]

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

https://codereview.chromium.org/290023003/diff/130001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.h:76: // Helper to emit
counter updates.
On 2014/06/02 18:22:22, James Hawkins wrote:
> Optional, but encouraged: The method documentation is superfluous as-is.  I
> suggest phrasing it in such a way that someone not familiar with the code can
> understand what it does and why.

Done.

[rlarocque, 2014-06-02 18:36:38]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-06-02 18:37:56]

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

[commit-bot: I haz the power, 2014-06-02 22:16:10]

Change committed as 274348