Add information about sync session and other useful data to about:sync.

Adds the following information to about sync page:
Count of empty and non empty getupdates.
Download count and commit count.
Last sync source and if sync was throttled.
Number of entries in sync db.

BUG=55806
TEST=


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

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

[lipalani1, 2011-05-13 00:19:33]

Please review. Couple of things:
1. I am not sure what is meant by the following line in the bug:
"Rename "Updates Received" to "Max Version", or just deprecate "max version",
which may not make sense after upcoming protocol changes."

2. Please ensure my logic for calculating the empty and non empty getupdates is
not missing any edge cases.

[lipalani1, 2011-05-13 00:43:54]

I have corrected a mistake and left a comment there. The rest of the CR is
valid. Expect a CR with the correction in next 20 mts.

http://codereview.chromium.org/7013040/diff/3003/chrome/browser/sync/sync_ui_...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/7013040/diff/3003/chrome/browser/sync/sync_ui_...
chrome/browser/sync/sync_ui_util.cc:435: // when syncing.
ACtually holding a transaction is not necessary. May be the right thing to do is
protect the code to get the count with a lock. But I still prefer this method
otherwise we will have to plumb a method from PSS all the directory. However I
will add a lock to the count method and send another cr.

[lipalani1, 2011-05-13 01:06:43]

Fixing the bug I mentioned earlier by adding a lock.

http://codereview.chromium.org/7013040/diff/3003/chrome/browser/sync/sync_ui_...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/7013040/diff/3003/chrome/browser/sync/sync_ui_...
chrome/browser/sync/sync_ui_util.cc:435: // when syncing.
On 2011/05/13 00:43:54, lipalani1 wrote:
> ACtually holding a transaction is not necessary. May be the right thing to do
is
> protect the code to get the count with a lock. But I still prefer this method
> otherwise we will have to plumb a method from PSS all the directory. However I
> will add a lock to the count method and send another cr.

Done.

[akalin, 2011-05-13 22:01:42]

Some comments

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/engine/all...
File chrome/browser/sync/engine/all_status.cc (right):

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/engine/all...
chrome/browser/sync/engine/all_status.cc:84: ++status.empty_getupdates;
getupdates -> get_updates

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/engine/all...
chrome/browser/sync/engine/all_status.cc:86: ++status.nonempty_getupdates;
can't num_successful_commits be non-zero, i.e. can't this be a commit?

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/engine/syn...
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/engine/syn...
chrome/browser/sync/engine/syncapi.h:741: int empty_getupdates;
shouldn't you be initialize these somewhere?

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/sessions/s...
File chrome/browser/sync/sessions/session_state.h (right):

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/sessions/s...
chrome/browser/sync/sessions/session_state.h:89: bool throttled;
we already store this state (it's called is_silenced in SyncSessionSnapshot). 
Maybe rename that one?

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/sync_ui_ut...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/sync_ui_ut...
chrome/browser/sync/sync_ui_util.cc:412: "Empty Getupdates",
Getupdates -> GetUpdates

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/sync_ui_ut...
chrome/browser/sync/sync_ui_util.cc:423: sync_ui_util::AddIntSyncDetail(details,
"Download Count(This Session)",
space before (

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/sync_ui_ut...
chrome/browser/sync/sync_ui_util.cc:425: sync_ui_util::AddIntSyncDetail(details,
"Commit Count(This Session)",
space before (

[lipalani1, 2011-05-13 22:35:39]

Please review.

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/engine/all...
File chrome/browser/sync/engine/all_status.cc (right):

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/engine/all...
chrome/browser/sync/engine/all_status.cc:84: ++status.empty_getupdates;
On 2011/05/13 22:01:42, akalin wrote:
> getupdates -> get_updates

Done.

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/engine/all...
chrome/browser/sync/engine/all_status.cc:86: ++status.nonempty_getupdates;
Not sure i understand. if the commits is zero and download is zero then we count
this as a non emtpy get update.
On 2011/05/13 22:01:42, akalin wrote:
> can't num_successful_commits be non-zero, i.e. can't this be a commit?

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/engine/syn...
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/engine/syn...
chrome/browser/sync/engine/syncapi.h:741: int empty_getupdates;
This structure does not have a constructor. Instead it is initialized by value
by filling the summary alone. Rest are autoinitialized to zero.
On 2011/05/13 22:01:42, akalin wrote:
> shouldn't you be initialize these somewhere?

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/sessions/s...
File chrome/browser/sync/sessions/session_state.h (right):

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/sessions/s...
chrome/browser/sync/sessions/session_state.h:89: bool throttled;
On 2011/05/13 22:01:42, akalin wrote:
> we already store this state (it's called is_silenced in SyncSessionSnapshot). 
> Maybe rename that one?

Done.

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/sync_ui_ut...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/sync_ui_ut...
chrome/browser/sync/sync_ui_util.cc:412: "Empty Getupdates",
On 2011/05/13 22:01:42, akalin wrote:
> Getupdates -> GetUpdates

Done.

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/sync_ui_ut...
chrome/browser/sync/sync_ui_util.cc:423: sync_ui_util::AddIntSyncDetail(details,
"Download Count(This Session)",
On 2011/05/13 22:01:42, akalin wrote:
> space before (

Done.

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/sync_ui_ut...
chrome/browser/sync/sync_ui_util.cc:425: sync_ui_util::AddIntSyncDetail(details,
"Commit Count(This Session)",
On 2011/05/13 22:01:42, akalin wrote:
> space before (

Done.

[akalin, 2011-05-16 22:11:23]

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/engine/all...
File chrome/browser/sync/engine/all_status.cc (right):

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/engine/all...
chrome/browser/sync/engine/all_status.cc:86: ++status.nonempty_getupdates;
I mean if the download count is 0 and the commit count is >0, isn't that a
commit, and not a getupdates?

On 2011/05/13 22:35:39, lipalani1 wrote:
> Not sure i understand. if the commits is zero and download is zero then we
count
> this as a non emtpy get update.
> On 2011/05/13 22:01:42, akalin wrote:
> > can't num_successful_commits be non-zero, i.e. can't this be a commit?
>

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/engine/syn...
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/engine/syn...
chrome/browser/sync/engine/syncapi.h:741: int empty_getupdates;
On 2011/05/13 22:35:39, lipalani1 wrote:
> This structure does not have a constructor. Instead it is initialized by value
> by filling the summary alone. Rest are autoinitialized to zero.

Hmm primitive types aren't zero-initialized in C++.  It may be a bug that these
aren't being initialized somewhere.  Can you track down when this struct is
created/loaded and make sure that it's initialized?

[akalin, 2011-05-16 22:34:58]

http://codereview.chromium.org/7013040/diff/3004/chrome/browser/sync/engine/a...
File chrome/browser/sync/engine/all_status.cc (right):

http://codereview.chromium.org/7013040/diff/3004/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/all_status.cc:84: ++status.empty_get_updates;
rename to "useless_sync_cycles"

http://codereview.chromium.org/7013040/diff/3004/chrome/browser/sync/engine/a...
chrome/browser/sync/engine/all_status.cc:86: ++status.nonempty_get_updates;
rename to "useful_sync_cycles"

http://codereview.chromium.org/7013040/diff/3004/chrome/browser/sync/sync_ui_...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/7013040/diff/3004/chrome/browser/sync/sync_ui_...
chrome/browser/sync/sync_ui_util.cc:412: "Empty GetUpdates",
rename to "Useless Sync Cycles"

http://codereview.chromium.org/7013040/diff/3004/chrome/browser/sync/sync_ui_...
chrome/browser/sync/sync_ui_util.cc:415: "Nonempty GetUpdates",
"Useful Sync Cycles"

[lipalani1, 2011-05-16 22:51:34]

Fixed all the feedback.

[lipalani1, 2011-05-16 22:52:16]

hang on there is a spelling mistake that I missed the in my CR pass. Let me fix
it.

[ncarter (slow), 2011-05-16 22:57:50]

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/engine/all...
File chrome/browser/sync/engine/all_status.cc (right):

http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/engine/all...
chrome/browser/sync/engine/all_status.cc:86: ++status.nonempty_getupdates;
On 2011/05/16 22:11:23, akalin wrote:
> I mean if the download count is 0 and the commit count is >0, isn't that a
> commit, and not a getupdates?
> 
> On 2011/05/13 22:35:39, lipalani1 wrote:
> > Not sure i understand. if the commits is zero and download is zero then we
> count
> > this as a non emtpy get update.
> > On 2011/05/13 22:01:42, akalin wrote:
> > > can't num_successful_commits be non-zero, i.e. can't this be a commit?
> > 
> 

I also don't understand why the commit counter factors into the determination of
the empty_getupdates counter.  Moreover, I would want for these counters to
count the number of GetUpdates requests, not the number of sync cycles.  A sync
cycle can do arbitrarily many GetUpdates requests.

[lipalani, 2011-05-16 22:59:31]

we just renamed it to useless_sync_cycles and useful_sync_cycles. I am
assuming thats what we wanted to count.

Thanks!
regards,
Lingesh

On Mon, May 16, 2011 at 3:57 PM, <nick@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/engine/all...
>
> File chrome/browser/sync/engine/all_status.cc (right):
>
>
>
http://codereview.chromium.org/7013040/diff/27/chrome/browser/sync/engine/all...
>
> chrome/browser/sync/engine/all_status.cc:86:
> ++status.nonempty_getupdates;
>
> On 2011/05/16 22:11:23, akalin wrote:
>
>> I mean if the download count is 0 and the commit count is >0, isn't
>>
> that a
>
>> commit, and not a getupdates?
>>
>
>  On 2011/05/13 22:35:39, lipalani1 wrote:
>> > Not sure i understand. if the commits is zero and download is zero
>>
> then we
>
>> count
>> > this as a non emtpy get update.
>> > On 2011/05/13 22:01:42, akalin wrote:
>> > > can't num_successful_commits be non-zero, i.e. can't this be a
>>
> commit?
>
>> >
>>
>
>
> I also don't understand why the commit counter factors into the
> determination of the empty_getupdates counter.  Moreover, I would want
> for these counters to count the number of GetUpdates requests, not the
> number of sync cycles.  A sync cycle can do arbitrarily many GetUpdates
> requests.
>
>
> http://codereview.chromium.org/7013040/
>

[lipalani1, 2011-05-16 23:17:37]

Fixing cr feedback.

[akalin, 2011-05-17 02:03:00]

I think Nick is also confused for the same reason as I was.  He knows more about
how the commit counts / get update counts interact, so if he's confused, then
maybe there's still something wrong.

Should we be only looking at get updates counts and not commit counts?

http://codereview.chromium.org/7013040/diff/15001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/7013040/diff/15001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.h:742: int useful_syn_cycles;
syn -> sync

http://codereview.chromium.org/7013040/diff/15001/chrome/browser/sync/session...
File chrome/browser/sync/sessions/session_state.cc (right):

http://codereview.chromium.org/7013040/diff/15001/chrome/browser/sync/session...
chrome/browser/sync/sessions/session_state.cc:170:
value->SetInteger("num_entries", num_entries);
num_entries -> numEntries

http://codereview.chromium.org/7013040/diff/15001/chrome/browser/sync/sync_ui...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/7013040/diff/15001/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.cc:412: "Empty Syncs",
Empty -> useless

http://codereview.chromium.org/7013040/diff/15001/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.cc:415: "Nonempty Syncs",
Nonempty -> useful

[lipalani1, 2011-05-18 00:28:07]

Fixed CR feedback. After talking to Nick added a nonempty_getupdates and
empty_getupdates member. It looks like both the useful/useless and
empty/nonempty metrics are interesting.

http://codereview.chromium.org/7013040/diff/15001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/7013040/diff/15001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.h:742: int useful_syn_cycles;
oh boy. I did a compile before sending out.The only reason I was able to think
is I might have pressed a delete key or undo button before committing to git...
On 2011/05/17 02:03:01, akalin wrote:
> syn -> sync

Done.

http://codereview.chromium.org/7013040/diff/15001/chrome/browser/sync/session...
File chrome/browser/sync/sessions/session_state.cc (right):

http://codereview.chromium.org/7013040/diff/15001/chrome/browser/sync/session...
chrome/browser/sync/sessions/session_state.cc:170:
value->SetInteger("num_entries", num_entries);
On 2011/05/17 02:03:01, akalin wrote:
> num_entries -> numEntries

Done.

http://codereview.chromium.org/7013040/diff/15001/chrome/browser/sync/sync_ui...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/7013040/diff/15001/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.cc:412: "Empty Syncs",
On 2011/05/17 02:03:01, akalin wrote:
> Empty -> useless

Done.

http://codereview.chromium.org/7013040/diff/15001/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.cc:415: "Nonempty Syncs",
On 2011/05/17 02:03:01, akalin wrote:
> Nonempty -> useful

Done.

[akalin, 2011-05-18 01:06:34]

LGTM

http://codereview.chromium.org/7013040/diff/16002/chrome/browser/sync/sync_ui...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/7013040/diff/16002/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.cc:412: "Empty Get Updates",
Get Updates -> GetUpdates

http://codereview.chromium.org/7013040/diff/16002/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.cc:415: "Nonempty Get Updates",
Get Updates -> GetUpdates

http://codereview.chromium.org/7013040/diff/16002/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.cc:418: "Useless Syncs",
Syncs -> Sync Cycles

http://codereview.chromium.org/7013040/diff/16002/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.cc:421: "Useful Syncs",
Syncs -> Sync Cycles

http://codereview.chromium.org/7013040/diff/16002/chrome/browser/sync/sync_ui...
chrome/browser/sync/sync_ui_util.cc:429: sync_ui_util::AddIntSyncDetail(details,
"Download Count (This Session)",
Proper indentation is:

sync_ui_util::AddIntSyncDetail(
    details, "Download Count (This Session)",
    snapshot->syncer_status.num_updates_downloaded_total);
sync_ui_util::AddIntSyncDetail(
    details, "Commit Count (This Session)",
    snapshot->syncer_status.num_successful_commits);
sync_ui_util::AddStringSyncDetails(
    details, "Last Sync Source",
    browser_sync::GetUpdatesSourceString(snapshot->source.updates_source));

[commit-bot: I haz the power, 2011-05-18 05:10:21]

Try job failure for 7013040-20010 on linux:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-05-18 19:47:37]

Try job failure for 7013040-22001 on linux:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-05-19 23:34:34]

Try job failure for 7013040-26001 on mac:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2011-05-20 01:30:48]

Change committed as 86032