sync: SyncableService support for starting sync

sync: SyncableService support for starting sync.

Still has no actual effect as operational bits are behind the --sync-enable-deferred-startup flag.  If you pass that flag, autofill will now be permitted to kick off sync init (in fact, it will be the only thing that can kick off sync init, so you probably don't want to pass the flag yet!).

Next I'll add a fallback timer and plumb the flare to more datatypes; non Sync API types will have to call ProfileSyncService directly.

TBR=dhollowa@chromium.org
^ For web_data_service_factory as changes are just basic consequence of the parameter change to AutocompleteSyncableService already LGTM'd.

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

[tim (not reviewing), 2013-04-18 17:14:35]

Took a stab at allowing data type activity to kick off sync initialization,
please take a look.

[tim (not reviewing), 2013-04-19 18:22:45]

Actually, there are a few extra edge cases I should deal with here. Stand by...

[tim (not reviewing), 2013-04-24 17:31:08]

+Haitao for review

Please take a look. Thanks.

[tim (not reviewing), 2013-04-24 17:47:44]

(hadn't uploaded proper patchset, please make sure you see latest).

[haitaol1, 2013-04-24 20:38:27]

On 2013/04/24 17:47:44, timsteele wrote:
> (hadn't uploaded proper patchset, please make sure you see latest).

Is latest patch uploaded?

[tim (not reviewing), 2013-04-25 17:39:50]

On 2013/04/24 20:38:27, haitaol1 wrote:
> On 2013/04/24 17:47:44, timsteele wrote:
> > (hadn't uploaded proper patchset, please make sure you see latest).
> 
> Is latest patch uploaded?

Yes

[haitaol1, 2013-04-25 19:18:18]

https://codereview.chromium.org/14018026/diff/33001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_start_util.h (right):

https://codereview.chromium.org/14018026/diff/33001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_start_util.h:16: // ProfileSyncService it needs
sync to start ASAP.  Typically this would be
dup "would be"

https://codereview.chromium.org/14018026/diff/33001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/14018026/diff/33001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:541:
DCHECK(syncer::UserTypes().Has(type));
Should you check whether sync for this type is enabled?

[tim (not reviewing), 2013-05-01 21:19:47]

https://codereview.chromium.org/14018026/diff/33001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_start_util.h (right):

https://codereview.chromium.org/14018026/diff/33001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_start_util.h:16: // ProfileSyncService it needs
sync to start ASAP.  Typically this would be
On 2013/04/25 19:18:18, haitaol1 wrote:
> dup "would be"

Done.

https://codereview.chromium.org/14018026/diff/33001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/14018026/diff/33001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:541:
DCHECK(syncer::UserTypes().Has(type));
On 2013/04/25 19:18:18, haitaol1 wrote:
> Should you check whether sync for this type is enabled?

Good catch. A disabled type shouldn't trigger sync to start (it wouldn't cause
sync for *that* type to start, but seems wrong nonetheless).

[Nicolas Zea, 2013-05-01 22:37:04]

https://codereview.chromium.org/14018026/diff/40001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_start_util.cc (right):

https://codereview.chromium.org/14018026/diff/40001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_start_util.cc:16: void StartSyncOnUIThread(const
base::FilePath& profile,
have these two methods be within an anon namespace?
 
also nit: newline between this and namespace opening

https://codereview.chromium.org/14018026/diff/40001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/14018026/diff/40001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:548: const syncer::ModelTypeSet
preferred_types = GetPreferredDataTypes();
no need for const.

https://codereview.chromium.org/14018026/diff/40001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:553: << type << "not enabled.";
ModelTypeToString(type)

https://codereview.chromium.org/14018026/diff/40001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:572: type,
Use ModelTypeToHistogramInt

https://codereview.chromium.org/14018026/diff/40001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:575:
data_type_requested_sync_startup_ = true;
why not just make this a parameter to TryStart?

https://codereview.chromium.org/14018026/diff/40001/sync/api/syncable_service.h
File sync/api/syncable_service.h (right):

https://codereview.chromium.org/14018026/diff/40001/sync/api/syncable_service...
sync/api/syncable_service.h:44: virtual void InjectStartSyncFlare(const
StartSyncFlare& flare);
Is this really necessary? Why can't a datatype just include start_sync_until,
build a flare, and call it directly?

[tim (not reviewing), 2013-05-01 23:20:44]

https://codereview.chromium.org/14018026/diff/40001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/14018026/diff/40001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:575:
data_type_requested_sync_startup_ = true;
On 2013/05/01 22:37:04, Nicolas Zea wrote:
> why not just make this a parameter to TryStart?

Because it needs to be remembered?  e.g it's possible in theory this happens
before tokens are available, but in that case when tokens do become available,
we probably want to start syncing.

That said I'd like to move all this state and logic out to a separate thing
focused solely on initializing sync, it's awfully cluttered in this class :(

https://codereview.chromium.org/14018026/diff/40001/sync/api/syncable_service.h
File sync/api/syncable_service.h (right):

https://codereview.chromium.org/14018026/diff/40001/sync/api/syncable_service...
sync/api/syncable_service.h:44: virtual void InjectStartSyncFlare(const
StartSyncFlare& flare);
On 2013/05/01 22:37:04, Nicolas Zea wrote:
> Is this really necessary? Why can't a datatype just include start_sync_until,
> build a flare, and call it directly?

Is there an elegant way to get a hold of a profile or profile path from backend
SyncableServices or would they all wind up having to have that passed in as a
parameter anyway, in which case I was thinking I'd rather pass the more specific
thing it actually needs.

[Nicolas Zea, 2013-05-02 18:01:07]

https://codereview.chromium.org/14018026/diff/40001/sync/api/syncable_service.h
File sync/api/syncable_service.h (right):

https://codereview.chromium.org/14018026/diff/40001/sync/api/syncable_service...
sync/api/syncable_service.h:44: virtual void InjectStartSyncFlare(const
StartSyncFlare& flare);
On 2013/05/01 23:20:44, timsteele wrote:
> On 2013/05/01 22:37:04, Nicolas Zea wrote:
> > Is this really necessary? Why can't a datatype just include
start_sync_until,
> > build a flare, and call it directly?
> 
> Is there an elegant way to get a hold of a profile or profile path from
backend
> SyncableServices or would they all wind up having to have that passed in as a
> parameter anyway, in which case I was thinking I'd rather pass the more
specific
> thing it actually needs.

I'm fine with passing the more specific thing, i.e. the callback, but it just
doesn't seem like InjectStartSyncFlare needs to be part of the SyncableService
api.

The WebDataFactory already knows about the AutocompleteSyncableService, and I'd
imagine any other datatype specific factories/objects would be able to pass the
callback through without needing to use the SyncableService interface.

[tim (not reviewing), 2013-05-02 19:53:29]

Moved Inject out of the sync API interface.  PTAL!

[Nicolas Zea, 2013-05-02 20:18:12]

Btw, I think you didn't address the other comments I had?

https://codereview.chromium.org/14018026/diff/48001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_start_util.h (right):

https://codereview.chromium.org/14018026/diff/48001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_start_util.h:21: // so that non-frontend types
don't have to deal with posting tasks.
nit: non-frontend -> non-ui?

https://codereview.chromium.org/14018026/diff/48001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_start_util.h:26: const base::FilePath&
profile_path);
forward declare?

https://codereview.chromium.org/14018026/diff/48001/chrome/browser/webdata/au...
File chrome/browser/webdata/autocomplete_syncable_service.cc (right):

https://codereview.chromium.org/14018026/diff/48001/chrome/browser/webdata/au...
chrome/browser/webdata/autocomplete_syncable_service.cc:130: 
remove newline

https://codereview.chromium.org/14018026/diff/48001/chrome/browser/webdata/au...
File chrome/browser/webdata/autocomplete_syncable_service.h (right):

https://codereview.chromium.org/14018026/diff/48001/chrome/browser/webdata/au...
chrome/browser/webdata/autocomplete_syncable_service.h:82: void
InjectStartSyncFlare(const syncer::StartSyncFlare& flare);
Comment about what this does?

https://codereview.chromium.org/14018026/diff/48001/sync/api/syncable_service.h
File sync/api/syncable_service.h (right):

https://codereview.chromium.org/14018026/diff/48001/sync/api/syncable_service...
sync/api/syncable_service.h:31: // statistics, make optimizations or tradeoffs
by type, etc.
So...you're right, I realize now that this is weird to have in this file, when
it's really something that's specific to the ProfileSyncService right, and there
isn't really a description of how to build it.

I'm fine with it for now, but maybe add a TODO to move the definition?

[tim (not reviewing), 2013-05-03 00:20:51]

I addressed your comments. I don't know if you'll be a huge fan of the
syncable_service.h change in this patchset, but here's my rationale:  I moved
the declaration *deeper* into SyncableService, because a) it makes less sense
floating around out of context in its own file given that it is only intended to
be used by SyncableServices, b) it needs to be in sync/ for the cleanest
expression of the things a SS expects from the embedder (and avoiding awkward
DEPS violations), and c) it makes it explicitly a flare for use by
SyncableServices.  I anticipate other types of these down the road (for example,
from InvalidationsService or something, which won't want to carry a ModelType
and may have their own parameters to add).

PTAL

https://codereview.chromium.org/14018026/diff/40001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_start_util.cc (right):

https://codereview.chromium.org/14018026/diff/40001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_start_util.cc:16: void StartSyncOnUIThread(const
base::FilePath& profile,
On 2013/05/01 22:37:04, Nicolas Zea wrote:
> have these two methods be within an anon namespace?
>  
> also nit: newline between this and namespace opening

Done.

https://codereview.chromium.org/14018026/diff/40001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/14018026/diff/40001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:548: const syncer::ModelTypeSet
preferred_types = GetPreferredDataTypes();
On 2013/05/01 22:37:04, Nicolas Zea wrote:
> no need for const.

Changed this to do GetPreferredDataTypes.Has(type).

https://codereview.chromium.org/14018026/diff/40001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:553: << type << "not enabled.";
On 2013/05/01 22:37:04, Nicolas Zea wrote:
> ModelTypeToString(type)

Done.

https://codereview.chromium.org/14018026/diff/40001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service.cc:572: type,
On 2013/05/01 22:37:04, Nicolas Zea wrote:
> Use ModelTypeToHistogramInt

Done.

https://codereview.chromium.org/14018026/diff/48001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_start_util.h (right):

https://codereview.chromium.org/14018026/diff/48001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_start_util.h:21: // so that non-frontend types
don't have to deal with posting tasks.
On 2013/05/02 20:18:13, Nicolas Zea wrote:
> nit: non-frontend -> non-ui?

Done.

https://codereview.chromium.org/14018026/diff/48001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_start_util.h:26: const base::FilePath&
profile_path);
On 2013/05/02 20:18:13, Nicolas Zea wrote:
> forward declare?

Done.

https://codereview.chromium.org/14018026/diff/48001/chrome/browser/webdata/au...
File chrome/browser/webdata/autocomplete_syncable_service.cc (right):

https://codereview.chromium.org/14018026/diff/48001/chrome/browser/webdata/au...
chrome/browser/webdata/autocomplete_syncable_service.cc:130: 
On 2013/05/02 20:18:13, Nicolas Zea wrote:
> remove newline

Done.

https://codereview.chromium.org/14018026/diff/48001/chrome/browser/webdata/au...
File chrome/browser/webdata/autocomplete_syncable_service.h (right):

https://codereview.chromium.org/14018026/diff/48001/chrome/browser/webdata/au...
chrome/browser/webdata/autocomplete_syncable_service.h:82: void
InjectStartSyncFlare(const syncer::StartSyncFlare& flare);
On 2013/05/02 20:18:13, Nicolas Zea wrote:
> Comment about what this does?

Done.

[Nicolas Zea, 2013-05-03 00:27:05]

LGTM

[tim (not reviewing), 2013-05-03 00:50:47]

+isherman for webdata/autocomplete*

[Ilya Sherman, 2013-05-03 00:59:58]

LGTM, thanks.

https://codereview.chromium.org/14018026/diff/55001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_start_util.h (right):

https://codereview.chromium.org/14018026/diff/55001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_start_util.h:30: };
nit: Please leave a blank line above this one, omit the semicolon, and add "  //
namespace sync_start_util".  Right now it really looks on first glance like the
namespace isn't closed correctly.

https://codereview.chromium.org/14018026/diff/55001/chrome/browser/webdata/au...
File chrome/browser/webdata/autocomplete_syncable_service.cc (right):

https://codereview.chromium.org/14018026/diff/55001/chrome/browser/webdata/au...
chrome/browser/webdata/autocomplete_syncable_service.cc:315: // starts.
Optional nit: As long as you're here, would you mind updating this comment?

[tim (not reviewing), 2013-05-03 01:14:34]

Thanks, addressed both comments.  Also realized I need an OWNER for
web_data_service_factory, so +dhollowa.

https://codereview.chromium.org/14018026/diff/55001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_start_util.h (right):

https://codereview.chromium.org/14018026/diff/55001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_start_util.h:30: };
On 2013/05/03 00:59:58, Ilya Sherman wrote:
> nit: Please leave a blank line above this one, omit the semicolon, and add " 
//
> namespace sync_start_util".  Right now it really looks on first glance like
the
> namespace isn't closed correctly.

Done.

[commit-bot: I haz the power, 2013-05-03 20:08:34]

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

[commit-bot: I haz the power, 2013-05-03 20:16:51]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-05-03 20:28:18]

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

[commit-bot: I haz the power, 2013-05-03 21:15:57]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-05-03 21:46:29]

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

[commit-bot: I haz the power, 2013-05-04 13:56:43]

Change committed as 198309