sync: Introduce classes for per-type counters

sync: Introduce classes for per-type counters

Introduces the classes required to support per-type debug information.
This system will eventually replace much of the StatusController and
ModelNeutralState.

Adds three counter objects.  There is one for counters incremented
during a GetUpdates, one for counters incremented during a commit, and
one for "status" indicators like the total number of sync entities.

Adds the DirectoryTypeDebugInfoEmitter, which will manage these
counters.  There will be one instance of this object per enabled
directory-style sync ModelType.  It owns the counters that will be
updated by the CommitContributors and UpdateHandlers.  It also
collaborates with the ModelTypeRegistry to allow these
CommitContributors and UpdateHandlers to notify observers of updated
counter state.

Adds the TypeDebugInfoObserver, the interface for classes that listen
for counter updates.

At the moment, this code won't actually do much.  The counters are never
incrememnted, no observers register for counter change events, and no
such events are generated.  Those pieces will be added in future
commits.

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

[rlarocque, 2014-04-24 21:40:29]

Please review.

[tim (not reviewing), 2014-04-25 16:23:15]

On 2014/04/24 21:40:29, rlarocque wrote:
> Please review.

All of the StatusController / ModelNeutralState stuff currently resides in
sessions, and from the CL description + cursory glance at files you don't seem
to be changing the fundamental need or purpose of the components. You're just
doing things in a different way with different names.  That's fine, but I don't
see the rationale for totally changing directory structure and relationships
here... (or even file names, although granted 'StatusController' isn't the best
of names).  Why aren't the new files roughly in the same places as the old ones?

[rlarocque, 2014-04-25 17:28:05]

On 2014/04/25 16:23:15, timsteele wrote:
> On 2014/04/24 21:40:29, rlarocque wrote:
> > Please review.
> 
> All of the StatusController / ModelNeutralState stuff currently resides in
> sessions, and from the CL description + cursory glance at files you don't seem
> to be changing the fundamental need or purpose of the components. You're just
> doing things in a different way with different names.  That's fine, but I
don't
> see the rationale for totally changing directory structure and relationships
> here... (or even file names, although granted 'StatusController' isn't the
best
> of names).  Why aren't the new files roughly in the same places as the old
ones?

A few reasons.

For one, the concept of a "session" is making less and less sense.  With
pre-commit GU avoidance and Bismarck, the concept of a session doesn't really
fit anymore.  That's the main reason for this work.

The new counters will have lifetimes that are not limited by a session.  They'll
live as long as the data type is configured.  I figured that disqualified them
from being lumped in to the other sessions objects.

Also, they'll eventually replace a lot of the duplicate code hanging around the
current stats gathering system.  We can use these counters to transfer data
directly, rather than sending it through AllStatus, SyncSessionSnapshot, etc. 
To make that work, though, the classes further up the chain need to have access
to their headers.  We need to be able to include the UpdateCounters from
chrome/browser/sync/profile_sync_service.cc and
chrome/browser/ui/webui/sync_internals_message_handler.cc.

[tim (not reviewing), 2014-04-28 17:18:08]

On 2014/04/25 17:28:05, rlarocque wrote:
> On 2014/04/25 16:23:15, timsteele wrote:
> > On 2014/04/24 21:40:29, rlarocque wrote:
> > > Please review.
> > 
> > All of the StatusController / ModelNeutralState stuff currently resides in
> > sessions, and from the CL description + cursory glance at files you don't
seem
> > to be changing the fundamental need or purpose of the components. You're
just
> > doing things in a different way with different names.  That's fine, but I
> don't
> > see the rationale for totally changing directory structure and relationships
> > here... (or even file names, although granted 'StatusController' isn't the
> best
> > of names).  Why aren't the new files roughly in the same places as the old
> ones?
> 
> A few reasons.
> 
> For one, the concept of a "session" is making less and less sense.  With
> pre-commit GU avoidance and Bismarck, the concept of a session doesn't really
> fit anymore.  That's the main reason for this work.
> 
> The new counters will have lifetimes that are not limited by a session. 
They'll
> live as long as the data type is configured.  I figured that disqualified them
> from being lumped in to the other sessions objects.
> 
> Also, they'll eventually replace a lot of the duplicate code hanging around
the
> current stats gathering system.  We can use these counters to transfer data
> directly, rather than sending it through AllStatus, SyncSessionSnapshot, etc. 
> To make that work, though, the classes further up the chain need to have
access
> to their headers.  We need to be able to include the UpdateCounters from
> chrome/browser/sync/profile_sync_service.cc and
> chrome/browser/ui/webui/sync_internals_message_handler.cc.

Maybe we should chat. On one hand I think it'll always be reasonable to refer
(without loss of generality between GU / Commit) to the time between the
scheduler deciding it's time for a GU or Commit and that operation finishing as
a 'sync' or a 'sync session'.  It's true we may no longer need a bunch of extra
objects to carry state between subsequent such operations because we're only
doing one-at-a-time in the new world. But I don't think the concept of state
that lives between the Directory and Scheduler that is updated during a 'sync
session' is fundamentally going away even if the directory does or is renamed.
Today sessions/ houses it, my thinking is we can add a new home for that now if
you prefer (I think we discussed this in a different CL), or just keep using the
same one and maybe rename it some day. What I want to avoid is scattering
similar-but-slightly-different code around in multiple places as it makes the
interim code much harder to understand.

[rlarocque, 2014-04-28 21:15:59]

Patch updated.  PTAL.

[tim (not reviewing), 2014-04-28 21:26:21]

https://codereview.chromium.org/254473008/diff/50001/sync/engine/DEPS
File sync/engine/DEPS (right):

https://codereview.chromium.org/254473008/diff/50001/sync/engine/DEPS#newcode3
sync/engine/DEPS:3: "+sync/internal_api/public",
Unnecessary?

https://codereview.chromium.org/254473008/diff/50001/sync/internal_api/public...
File sync/internal_api/public/sessions/commit_counters.h (right):

https://codereview.chromium.org/254473008/diff/50001/sync/internal_api/public...
sync/internal_api/public/sessions/commit_counters.h:23: int
num_commits_attempted;
Make this a struct if you want public data members. ToValue and ToString are
commonly found on structs.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Structs_vs._Cl...

https://codereview.chromium.org/254473008/diff/50001/sync/internal_api/public...
File sync/internal_api/public/sessions/status_counters.h (right):

https://codereview.chromium.org/254473008/diff/50001/sync/internal_api/public...
sync/internal_api/public/sessions/status_counters.h:24: };
Make this a struct if you want public data members.

https://codereview.chromium.org/254473008/diff/50001/sync/internal_api/public...
File sync/internal_api/public/sessions/update_counters.h (right):

https://codereview.chromium.org/254473008/diff/50001/sync/internal_api/public...
sync/internal_api/public/sessions/update_counters.h:24: int
num_updates_received;
Make this a struct if you want public data members.

https://codereview.chromium.org/254473008/diff/50001/sync/sessions/directory_...
File sync/sessions/directory_type_debug_info_emitter.h (right):

https://codereview.chromium.org/254473008/diff/50001/sync/sessions/directory_...
sync/sessions/directory_type_debug_info_emitter.h:41:
ObserverList<TypeDebugInfoObserver>* observers);
Comment |observers| parameter, since e.g you can't unregister observers.

[rlarocque, 2014-04-28 21:59:59]

Patch updated.  PTAL.

https://codereview.chromium.org/254473008/diff/50001/sync/engine/DEPS
File sync/engine/DEPS (right):

https://codereview.chromium.org/254473008/diff/50001/sync/engine/DEPS#newcode3
sync/engine/DEPS:3: "+sync/internal_api/public",
On 2014/04/28 21:26:21, timsteele wrote:
> Unnecessary?

Not needed anymore, no.  Removed.

https://codereview.chromium.org/254473008/diff/50001/sync/internal_api/public...
File sync/internal_api/public/sessions/commit_counters.h (right):

https://codereview.chromium.org/254473008/diff/50001/sync/internal_api/public...
sync/internal_api/public/sessions/commit_counters.h:23: int
num_commits_attempted;
On 2014/04/28 21:26:21, timsteele wrote:
> Make this a struct if you want public data members. ToValue and ToString are
> commonly found on structs.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Structs_vs._Cl...

Done.

https://codereview.chromium.org/254473008/diff/50001/sync/internal_api/public...
File sync/internal_api/public/sessions/status_counters.h (right):

https://codereview.chromium.org/254473008/diff/50001/sync/internal_api/public...
sync/internal_api/public/sessions/status_counters.h:24: };
On 2014/04/28 21:26:21, timsteele wrote:
> Make this a struct if you want public data members.

Done.

https://codereview.chromium.org/254473008/diff/50001/sync/internal_api/public...
File sync/internal_api/public/sessions/update_counters.h (right):

https://codereview.chromium.org/254473008/diff/50001/sync/internal_api/public...
sync/internal_api/public/sessions/update_counters.h:24: int
num_updates_received;
On 2014/04/28 21:26:21, timsteele wrote:
> Make this a struct if you want public data members.

Done.

https://codereview.chromium.org/254473008/diff/50001/sync/sessions/directory_...
File sync/sessions/directory_type_debug_info_emitter.h (right):

https://codereview.chromium.org/254473008/diff/50001/sync/sessions/directory_...
sync/sessions/directory_type_debug_info_emitter.h:41:
ObserverList<TypeDebugInfoObserver>* observers);
On 2014/04/28 21:26:21, timsteele wrote:
> Comment |observers| parameter, since e.g you can't unregister observers.

Comment added.

[tim (not reviewing), 2014-04-28 22:03:38]

LGTM

[rlarocque, 2014-04-28 22:05:07]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-04-28 22:06:42]

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

[commit-bot: I haz the power, 2014-04-28 22:09:49]

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

[commit-bot: I haz the power, 2014-04-28 22:09:50]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_gn_rel

[rlarocque, 2014-04-29 01:16:16]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-04-29 01:16:39]

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

[commit-bot: I haz the power, 2014-04-29 02:12:44]

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

[commit-bot: I haz the power, 2014-04-29 02:12:44]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[rlarocque, 2014-04-29 17:18:27]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-04-29 17:19:01]

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

[commit-bot: I haz the power, 2014-04-29 23:16:37]

Change committed as 266991