sync: Add GetAllSyncData to sync/api

sync: Add GetAllSyncData to sync/api

This allows datatype SyncableServices to request a fresh copy of the data
that was previously only passed over in batch form during
MergeDataAndStartSyncing. Since it can be slow, it should be used
sparingly.

Though currently unused, Sessions will soon need this behavior to deal with
cases where (e.g) the local session is deleted by a foreign client, thereby
corrupting TabNodePool tracking and requiring a full re-association.

TBR=zea@chromium.org
^Many function signature touchups under browser/

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

[haitaol1, 2013-08-19 17:11:03]

https://codereview.chromium.org/23129007/diff/3001/sync/api/sync_change_proce...
File sync/api/sync_change_processor.h (right):

https://codereview.chromium.org/23129007/diff/3001/sync/api/sync_change_proce...
sync/api/sync_change_processor.h:48: virtual SyncDataList
GetAllSyncData(ModelType type) const = 0;
Should we care about error?

https://codereview.chromium.org/23129007/diff/3001/sync/api/syncable_service.h
File sync/api/syncable_service.h (left):

https://codereview.chromium.org/23129007/diff/3001/sync/api/syncable_service....
sync/api/syncable_service.h:62: virtual SyncDataList GetAllSyncData(ModelType
type) const = 0;
Should this also be removed from subclasses of SyncableService?

[tim (not reviewing), 2013-08-19 18:03:07]

Thanks for the review.

https://codereview.chromium.org/23129007/diff/3001/sync/api/sync_change_proce...
File sync/api/sync_change_processor.h (right):

https://codereview.chromium.org/23129007/diff/3001/sync/api/sync_change_proce...
sync/api/sync_change_processor.h:48: virtual SyncDataList
GetAllSyncData(ModelType type) const = 0;
On 2013/08/19 17:11:04, haitaol1 wrote:
> Should we care about error?

I'm not sure it's worthwhile? My basic thinking was that I wanted to keep the
API the same in both (sync/native) directions, and we didn't have an error here
before.

https://codereview.chromium.org/23129007/diff/3001/sync/api/syncable_service.h
File sync/api/syncable_service.h (left):

https://codereview.chromium.org/23129007/diff/3001/sync/api/syncable_service....
sync/api/syncable_service.h:62: virtual SyncDataList GetAllSyncData(ModelType
type) const = 0;
On 2013/08/19 17:11:04, haitaol1 wrote:
> Should this also be removed from subclasses of SyncableService?

Note that I've really only moved this method up into the superclass, so
SyncableServices still need to implement.  SyncableService is a
SyncChangeProcessor.

[haitaol1, 2013-08-19 21:20:06]

https://codereview.chromium.org/23129007/diff/3001/chrome/browser/sync/glue/g...
File chrome/browser/sync/glue/generic_change_processor.h (right):

https://codereview.chromium.org/23129007/diff/3001/chrome/browser/sync/glue/g...
chrome/browser/sync/glue/generic_change_processor.h:68: virtual
syncer::SyncDataList GetAllSyncData(syncer::ModelType type)
This seems redundant with GetSyncDataForType().

https://codereview.chromium.org/23129007/diff/3001/sync/api/sync_change_proce...
File sync/api/sync_change_processor.h (right):

https://codereview.chromium.org/23129007/diff/3001/sync/api/sync_change_proce...
sync/api/sync_change_processor.h:48: virtual SyncDataList
GetAllSyncData(ModelType type) const = 0;
ProcessSyncChanges() returns error. SyncableService::GetAllSyncData() doesn't
return error but it seems to be called only in tests. Without knowing error,
will caller destroy local model if seeing empty list?


On 2013/08/19 18:03:08, timsteele wrote:
> On 2013/08/19 17:11:04, haitaol1 wrote:
> > Should we care about error?
> 
> I'm not sure it's worthwhile? My basic thinking was that I wanted to keep the
> API the same in both (sync/native) directions, and we didn't have an error
here
> before.

[tim (not reviewing), 2013-09-04 23:47:56]

> zea as Haitao is out.

I implemented (3) described below, which is what we talked about offline.

https://codereview.chromium.org/23129007/diff/3001/chrome/browser/sync/glue/g...
File chrome/browser/sync/glue/generic_change_processor.h (right):

https://codereview.chromium.org/23129007/diff/3001/chrome/browser/sync/glue/g...
chrome/browser/sync/glue/generic_change_processor.h:68: virtual
syncer::SyncDataList GetAllSyncData(syncer::ModelType type)
On 2013/08/19 21:20:06, haitaol1 wrote:
> This seems redundant with GetSyncDataForType().

I went back and forth between a few options here. The first was adding a
SyncError return to GetAllSyncData for all data type implementations and
removing GetSyncDataForType here.  As I started drudging through the 70 or so
files that needed updating I realized I was doing the same thing over and over
for every SyncableService -- none of them ever needed to return an error.  That
was enough to convince me it was the wrong approach and was adding complexity. 

I then backtracked to having only a single GetAllSyncData method that returned a
list, but then some unit tests proved to me that was insufficient for the
DataTypeController case, or at least overly awkward.  They shouldn't really
continue with MergeDataAndStartSyncing if there's a hard failure, although in
reality it didn't break anything in a serious way.

I then saw three remaining options.  1) add state to the DataTypeController
(which is the DataTypeErrorHandler) to track whether
OnSingleDataTypeUnrecoverableError was called during GetAllSyncData. It was bad
enough to add state to these complicated classes, but it was really awkward too
because GetAllSyncData is const and after the call we'd inspect state to see if
something changed!  2) Add state to the SharedChangeProcessor to track if it hit
an unrecoverable error.  Bad for the same reasons as 1.  And 3) -- have a
special method for direct clients of SCP and GCP that are typically doing "more
intricate stuff" than the SyncableServices, to allow these classes more
visibility into what happened.  This gives us the best of both worlds.
Simplicity in the SyncableService, and just the right level of granularity
without extra state tracking in the glue layer.

[Nicolas Zea, 2013-09-05 00:13:07]

LGTM with nits

https://codereview.chromium.org/23129007/diff/111001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/generic_change_processor.h (right):

https://codereview.chromium.org/23129007/diff/111001/chrome/browser/sync/glue...
chrome/browser/sync/glue/generic_change_processor.h:72: // of
GenericChangeProcessor that may need more visibility.
more visibility -> more error visibility

https://codereview.chromium.org/23129007/diff/111001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/non_ui_data_type_controller.cc (right):

https://codereview.chromium.org/23129007/diff/111001/chrome/browser/sync/glue...
chrome/browser/sync/glue/non_ui_data_type_controller.cc:368: type(),
&initial_sync_data);
nit: fix indent

https://codereview.chromium.org/23129007/diff/111001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/ui_data_type_controller.cc (right):

https://codereview.chromium.org/23129007/diff/111001/chrome/browser/sync/glue...
chrome/browser/sync/glue/ui_data_type_controller.cc:172: type(),
&initial_sync_data);
nit: fix indent

[tim (not reviewing), 2013-09-05 00:22:24]

Thanks for the review.

https://codereview.chromium.org/23129007/diff/111001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/generic_change_processor.h (right):

https://codereview.chromium.org/23129007/diff/111001/chrome/browser/sync/glue...
chrome/browser/sync/glue/generic_change_processor.h:72: // of
GenericChangeProcessor that may need more visibility.
On 2013/09/05 00:13:08, Nicolas Zea wrote:
> more visibility -> more error visibility

Done.

https://codereview.chromium.org/23129007/diff/111001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/non_ui_data_type_controller.cc (right):

https://codereview.chromium.org/23129007/diff/111001/chrome/browser/sync/glue...
chrome/browser/sync/glue/non_ui_data_type_controller.cc:368: type(),
&initial_sync_data);
On 2013/09/05 00:13:08, Nicolas Zea wrote:
> nit: fix indent

Done.

https://codereview.chromium.org/23129007/diff/111001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/ui_data_type_controller.cc (right):

https://codereview.chromium.org/23129007/diff/111001/chrome/browser/sync/glue...
chrome/browser/sync/glue/ui_data_type_controller.cc:172: type(),
&initial_sync_data);
On 2013/09/05 00:13:08, Nicolas Zea wrote:
> nit: fix indent

Done.

[commit-bot: I haz the power, 2013-09-05 00:24:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/23129007/67031

[commit-bot: I haz the power, 2013-09-05 01:41:18]

Retried try job too often on ios_dbg_simulator for step(s) ui_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-09-05 05:19:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/23129007/67031

[commit-bot: I haz the power, 2013-09-05 05:46:19]

Retried try job too often on ios_dbg_simulator for step(s) ui_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[tim (not reviewing), 2013-09-05 23:30:08]

Committed patchset #8 manually as r221540 (presubmit successful).