Added nudge source counters to about:sync.

Added nudge source counters to about:sync.

Added counters for each possible nudge source on the summary tab
of the about:sync page. The counters are stored in SyncStatus and
managed by SyncManagerImpl via allstatus_.

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

[vishwath, 2012-09-24 22:56:45]

The nudge source counters are in SyncStatus, where I thought they fit best. This
is still the first few days of my experience with the code base though, so if
there's a more appropriate place I can move it there.

[rlarocque, 2012-09-24 23:24:37]

Looks good!  I've got a few minor comments.

I'm not sure it makes sense to introduce the SyncManagerImpl::ScheduleNudge*()
methods.  I can see some pros and cons, but overall I'm leaning towards the con
side of the argument.  Thoughts?

https://codereview.chromium.org/10991005/diff/1/chrome/browser/sync/about_syn...
File chrome/browser/sync/about_sync_util.cc (right):

https://codereview.chromium.org/10991005/diff/1/chrome/browser/sync/about_syn...
chrome/browser/sync/about_sync_util.cc:258: IntSyncStat
nudge_source_unknown(section_nudge_info, "Unknown Source");
There should be no nudges with source unknown.  That's just something we added
to the enum to catch errors.

https://codereview.chromium.org/10991005/diff/1/sync/engine/all_status.cc
File sync/engine/all_status.cc (right):

https://codereview.chromium.org/10991005/diff/1/sync/engine/all_status.cc#new...
sync/engine/all_status.cc:192: case NUDGE_SOURCE_UNKNOWN:
This shouldn't happen.  I think you could NOTREACHED() if the source is UNKNOWN
or the default: case is hit.

Better yet, if you don't specify the default case then most compilers will warn
you if you've missed an enum value.  You can omit 'default:', change the
'break's to 'return's, and put a NOTREACHED() after the end of the switch
statement.

https://codereview.chromium.org/10991005/diff/1/sync/internal_api/sync_manage...
File sync/internal_api/sync_manager_impl.cc (right):

https://codereview.chromium.org/10991005/diff/1/sync/internal_api/sync_manage...
sync/internal_api/sync_manager_impl.cc:1353: const base::TimeDelta& delay,
A TimeDelta isn't very big.  It would be cheaper to pass a copy than a const
reference.

https://codereview.chromium.org/10991005/diff/1/sync/internal_api/sync_manage...
sync/internal_api/sync_manager_impl.cc:1358:
scheduler_->ScheduleNudgeAsync(delay, source,
nit: This style of line wrapping for parameters is discouraged.

See http://dev.chromium.org/developers/coding-style#Code_Formatting

[vishwath, 2012-09-25 00:02:23]

I added the counter for NUDGE_SOURCE_UNKNOWN on the off chance that it was
being triggered by weird bugs, but if that's not the case then it doesn't
make sense to have it in there and I've removed it. I fixed all references
to it, and made the code formatting changes (that I really should have made
to begin with) that you pointed out.

I definitely see the cons of adding the ScheduleNudge* methods. I added
them to help ensure a stronger coupling between calls to the SyncScheduler
versions of the functions and incrementing the appropriate nudge counter.
Without them the onus of incrementing the counter then falls on the next
developer who adds additional calls to the original
SyncScheduler::ScheduleNudge*() functions. Of course, one could (correctly)
argue that the presence of the SyncManagerImpl versions of the functions
won't deter that kind of mistake, so I'm open to not having them too. Are
we reasonably sure that additional calls to the nudge scheduling functions
are unlikely in the future?


On Mon, Sep 24, 2012 at 4:24 PM, <rlarocque@chromium.org> wrote:

> Looks good!  I've got a few minor comments.
>
> I'm not sure it makes sense to introduce the SyncManagerImpl::**
> ScheduleNudge*()
> methods.  I can see some pros and cons, but overall I'm leaning towards
> the con
> side of the argument.  Thoughts?
>
>
> https://codereview.chromium.**org/10991005/diff/1/chrome/**
>
browser/sync/about_sync_util.**cc<https://codereview.chromium.org/10991005/diff/1/chrome/browser/sync/about_sync_util.cc>
> File chrome/browser/sync/about_**sync_util.cc (right):
>
> https://codereview.chromium.**org/10991005/diff/1/chrome/**
>
browser/sync/about_sync_util.**cc#newcode258<https://codereview.chromium.org/10991005/diff/1/chrome/browser/sync/about_sync_util.cc#newcode258>
> chrome/browser/sync/about_**sync_util.cc:258: IntSyncStat
> nudge_source_unknown(section_**nudge_info, "Unknown Source");
> There should be no nudges with source unknown.  That's just something we
> added to the enum to catch errors.
>
> https://codereview.chromium.**org/10991005/diff/1/sync/**
>
engine/all_status.cc<https://codereview.chromium.org/10991005/diff/1/sync/engine/all_status.cc>
> File sync/engine/all_status.cc (right):
>
> https://codereview.chromium.**org/10991005/diff/1/sync/**
>
engine/all_status.cc#**newcode192<https://codereview.chromium.org/10991005/diff/1/sync/engine/all_status.cc#newcode192>
> sync/engine/all_status.cc:192: case NUDGE_SOURCE_UNKNOWN:
> This shouldn't happen.  I think you could NOTREACHED() if the source is
> UNKNOWN or the default: case is hit.
>
> Better yet, if you don't specify the default case then most compilers
> will warn you if you've missed an enum value.  You can omit 'default:',
> change the 'break's to 'return's, and put a NOTREACHED() after the end
> of the switch statement.
>
> https://codereview.chromium.**org/10991005/diff/1/sync/**
>
internal_api/sync_manager_**impl.cc<https://codereview.chromium.org/10991005/diff/1/sync/internal_api/sync_manager_impl.cc>
> File sync/internal_api/sync_**manager_impl.cc (right):
>
> https://codereview.chromium.**org/10991005/diff/1/sync/**
>
internal_api/sync_manager_**impl.cc#newcode1353<https://codereview.chromium.org/10991005/diff/1/sync/internal_api/sync_manager_impl.cc#newcode1353>
> sync/internal_api/sync_**manager_impl.cc:1353: const base::TimeDelta&
> delay,
> A TimeDelta isn't very big.  It would be cheaper to pass a copy than a
> const reference.
>
> https://codereview.chromium.**org/10991005/diff/1/sync/**
>
internal_api/sync_manager_**impl.cc#newcode1358<https://codereview.chromium.org/10991005/diff/1/sync/internal_api/sync_manager_impl.cc#newcode1358>
> sync/internal_api/sync_**manager_impl.cc:1358:
> scheduler_->**ScheduleNudgeAsync(delay, source,
> nit: This style of line wrapping for parameters is discouraged.
>
> See
http://dev.chromium.org/**developers/coding-style#Code_**Formatting<http://de...
>
>
https://codereview.chromium.**org/10991005/<https://codereview.chromium.org/1...
>

[rlarocque, 2012-09-25 00:06:12]

On 2012/09/25 00:02:23, vishwath wrote:
> I definitely see the cons of adding the ScheduleNudge* methods. I added
> them to help ensure a stronger coupling between calls to the SyncScheduler
> versions of the functions and incrementing the appropriate nudge counter.
> Without them the onus of incrementing the counter then falls on the next
> developer who adds additional calls to the original
> SyncScheduler::ScheduleNudge*() functions. Of course, one could (correctly)
> argue that the presence of the SyncManagerImpl versions of the functions
> won't deter that kind of mistake, so I'm open to not having them too. Are
> we reasonably sure that additional calls to the nudge scheduling functions
> are unlikely in the future?
> 

Good question.  I'm willing to assume that we won't be touching that code very
often.

[vishwath, 2012-09-25 00:30:08]

Given that additional ScheduleNudge*() calls are unlikely, I've gone ahead and
removed the SyncManagerImpl versions of the functions.

[rlarocque, 2012-09-25 16:49:27]

LGTM with nits.

https://codereview.chromium.org/10991005/diff/4002/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl.cc (right):

https://codereview.chromium.org/10991005/diff/4002/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:984: NUDGE_SOURCE_LOCAL,
nit: fix indentation

[vishwath, 2012-09-25 17:17:22]

Sorry about that, it should be fixed now.

[commit-bot: I haz the power, 2012-09-25 17:30:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vishwath@google.com/10991005/9

[commit-bot: I haz the power, 2012-09-25 19:47:29]

Change committed as 158633