sync: use progress markers instead of timestamps during GetUpdates

The sync server supports a new "progress token" scheme that allows data types to be requested with a different timestamp per datatype.  The benefits of this approach are:
 - timestamps become opaque to the client, which makes
 - the client no longer has to rely on timestamps converging for all datatypes.
 - the server will send fewer redundant updates after enabling a new datatype.
 - the flexibility of the opaque field allows the server to implement other optimizations which will increase scalability & performance

BUG=58217
TEST=manual test of sync (old profile+new profile), unit tests, integ tests

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

[Raghu Simha, 2011-01-11 18:53:04]

Just a drive by comment -- no need to wait for an LGTM from me.

http://codereview.chromium.org/6104003/diff/23001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/6104003/diff/23001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:24: static const int
kMinTimestampNeededNone = -1;
I believe you can now get rid of this constant.

[tim (not reviewing), 2011-01-11 19:14:23]

looks nice!

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/all_status.cc (right):

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/all_status.cc:19:
sync_api::SyncManager::Status::Summary summary) {
Did you consider moving this to syncapi too? We should really get rid of
'BuildSyncStatusSummaryText' in profile_sync_service.cc, it does the exact same
thing.

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/all_status.cc:86: // counter preserve its value
across sessions.
Referencing bug 26339 here would be better than nothing.  This'll be handled as
a result of that refactoring.

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/all_status.h (left):

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/all_status.h:82: };
oooooo
aaaaaah

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/download_updates_command.cc (right):

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/download_updates_command.cc:44: // Request updates
for all enabled types.
Is it okay if the routing_info is actually a subset of all enabled types? 
Scenario: "configuration" mode where we only want to download updates for 2 out
of 5 enabled types. I'm working on a patch for that at the moment, and it uses
routing_info to control which types we query.  I know you know this case, but
might want to tweak the comment?

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/download_updates_command.h (right):

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/download_updates_command.h:27: // point to the
server. DownloadUpdatesCommand store the server response
'DownloadUpdatesCommand store_s_ the server response...'

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/store_timestamps_command.cc (right):

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/store_timestamps_command.cc:41: NOTREACHED();  //
Unintelligible server resp2onse.
'response'
Also, since you have the comment, may as well redirect it at the dcheck.

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.cc:2068:
SyncManager::SyncInternal::ComputeAggregatedStatusSummary() {
'Compute' is misleading now, Maybe just GetStatusSummary and GetStatus (below)?

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

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.h:680: // total updates available.  If zero,
nothing left to download.
'Total'

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.h:682: // total updates received by the
syncer since browser start
caps + period

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_proto_util.cc:176:
DCHECK(message.get_updates().from_progress_marker_size() > 0);
nit - DCHECK_GT

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_unittest.cc (right):

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_unittest.cc:3570: const int DEPTH = 400;
would you be so kind as to lower case this?

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

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/session...
chrome/browser/sync/sessions/session_state.cc:245:
PerModelTypeState::PerModelTypeState(bool* dirty_flag) {
Is your reason for leaving this a) you're not sure if it is still needed, b) we
may want to add something here in the future so why remove all the plumbing, or
c) you don't want to make your patch larger?  I'm pretty sure it's not needed
now, and if we don't needed I'd rather see it go, which leaves (c), so I'd
suggest filing a bug and referencing it.  

Or is there a (d) I'm missing?

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/session...
File chrome/browser/sync/sessions/session_state.h (right):

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/session...
chrome/browser/sync/sessions/session_state.h:55: // Download event counters.
It looks like you update the counters at the same time as we update the
'UpdateProgress', which is where the number of verified downloaded updates is
currently tracked (VerifiedUpdatesSize()). It is split up by model safe group, 
so you would need a function in StatusController to iterate and sum (similar to
what ComputeMaxLocalTimestamp did). UpdateProgress doesn't count the tombstones,
but we pass in the update to AddVerifyResult so it'd be trivial to add a
num_tombstone_updates_downloaded there.  And you could revert
verify_updates_command, I think.

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/syncabl...
File chrome/browser/sync/syncable/directory_backing_store.cc (right):

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/syncabl...
chrome/browser/sync/syncable/directory_backing_store.cc:1057: if (type !=
UNSPECIFIED) {
TOP_LEVEL_FOLDER?

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/syncabl...
File chrome/browser/sync/syncable/syncable.h (right):

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/syncabl...
chrome/browser/sync/syncable/syncable.h:662: void
reset_download_progress(ModelType model_type);
comment? Just seems out of place without context...

[ncarter (slow), 2011-01-13 00:06:13]

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/all_status.cc (right):

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/all_status.cc:19:
sync_api::SyncManager::Status::Summary summary) {
On 2011/01/11 19:14:23, timsteele wrote:
> Did you consider moving this to syncapi too? We should really get rid of
> 'BuildSyncStatusSummaryText' in profile_sync_service.cc, it does the exact
same
> thing.

Moved this body to PSS.  This function was never actually called.

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/all_status.cc:86: // counter preserve its value
across sessions.
On 2011/01/11 19:14:23, timsteele wrote:
> Referencing bug 26339 here would be better than nothing.  This'll be handled
as
> a result of that refactoring.

Done.

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/all_status.h (left):

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/all_status.h:82: };
On 2011/01/11 19:14:23, timsteele wrote:
> oooooo
> aaaaaah

Done.

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/download_updates_command.cc (right):

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/download_updates_command.cc:44: // Request updates
for all enabled types.
On 2011/01/11 19:14:23, timsteele wrote:
> Is it okay if the routing_info is actually a subset of all enabled types? 
> Scenario: "configuration" mode where we only want to download updates for 2
out
> of 5 enabled types. I'm working on a patch for that at the moment, and it uses
> routing_info to control which types we query.  I know you know this case, but
> might want to tweak the comment?

Sure; with the new protocol it's completely orthogonal what gets requested,
since the progress markers track their state indepenently.  We no longer have to
worry about timestamps converging.

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/download_updates_command.h (right):

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/download_updates_command.h:27: // point to the
server. DownloadUpdatesCommand store the server response
On 2011/01/11 19:14:23, timsteele wrote:
> 'DownloadUpdatesCommand store_s_ the server response...'

Done.

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/store_timestamps_command.cc (right):

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/store_timestamps_command.cc:41: NOTREACHED();  //
Unintelligible server resp2onse.
On 2011/01/11 19:14:23, timsteele wrote:
> 'response'
> Also, since you have the comment, may as well redirect it at the dcheck.

Done.

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.cc:2068:
SyncManager::SyncInternal::ComputeAggregatedStatusSummary() {
On 2011/01/11 19:14:23, timsteele wrote:
> 'Compute' is misleading now, Maybe just GetStatusSummary and GetStatus
(below)?

Done.

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

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.h:680: // total updates available.  If zero,
nothing left to download.
On 2011/01/11 19:14:23, timsteele wrote:
> 'Total'

Done.

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.h:682: // total updates received by the
syncer since browser start
On 2011/01/11 19:14:23, timsteele wrote:
> caps + period

Done.

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_proto_util.cc:176:
DCHECK(message.get_updates().from_progress_marker_size() > 0);
On 2011/01/11 19:14:23, timsteele wrote:
> nit - DCHECK_GT

Done.

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_unittest.cc (right):

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_unittest.cc:3570: const int DEPTH = 400;
On 2011/01/11 19:14:23, timsteele wrote:
> would you be so kind as to lower case this?

Done.

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

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/session...
chrome/browser/sync/sessions/session_state.cc:245:
PerModelTypeState::PerModelTypeState(bool* dirty_flag) {
On 2011/01/11 19:14:23, timsteele wrote:
> Is your reason for leaving this a) you're not sure if it is still needed, b)
we
> may want to add something here in the future so why remove all the plumbing,
or
> c) you don't want to make your patch larger?  I'm pretty sure it's not needed
> now, and if we don't needed I'd rather see it go, which leaves (c), so I'd
> suggest filing a bug and referencing it.  
> 
> Or is there a (d) I'm missing?

It was (b) ("might need in the future"), and you've convinced me that it can go.
 Done.

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/session...
File chrome/browser/sync/sessions/session_state.h (right):

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/session...
chrome/browser/sync/sessions/session_state.h:55: // Download event counters.
On 2011/01/11 19:14:23, timsteele wrote:
> It looks like you update the counters at the same time as we update the
> 'UpdateProgress', which is where the number of verified downloaded updates is
> currently tracked (VerifiedUpdatesSize()). It is split up by model safe group,

> so you would need a function in StatusController to iterate and sum (similar
to
> what ComputeMaxLocalTimestamp did). UpdateProgress doesn't count the
tombstones,
> but we pass in the update to AddVerifyResult so it'd be trivial to add a
> num_tombstone_updates_downloaded there.  And you could revert
> verify_updates_command, I think.

Moving these to the UpdateProgress would require additional handling in
constructing the SyncSessionSnapshot.  I rather like that there's just one POD
field that gets incremented in one place, and plumbed through.  And, my goal for
these fields is for them to be durable across sessions, in which case the
ephemerality of UpdateProgress would make for an inappropriate place.

They can't go in the Context because the context is const.

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/syncabl...
File chrome/browser/sync/syncable/directory_backing_store.cc (right):

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/syncabl...
chrome/browser/sync/syncable/directory_backing_store.cc:1057: if (type !=
UNSPECIFIED) {
On 2011/01/11 19:14:23, timsteele wrote:
> TOP_LEVEL_FOLDER?

Added, thanks.  The implementatation of the called function makes that value
impossible, but it makes sense to be defensive.

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/syncabl...
File chrome/browser/sync/syncable/syncable.h (right):

http://codereview.chromium.org/6104003/diff/20001/chrome/browser/sync/syncabl...
chrome/browser/sync/syncable/syncable.h:662: void
reset_download_progress(ModelType model_type);
On 2011/01/11 19:14:23, timsteele wrote:
> comment? Just seems out of place without context...

Done.

http://codereview.chromium.org/6104003/diff/23001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/6104003/diff/23001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:24: static const int
kMinTimestampNeededNone = -1;
On 2011/01/11 18:53:04, rsimha wrote:
> I believe you can now get rid of this constant.

Done.

[tim (not reviewing), 2011-01-13 19:22:56]

LGTM