[Sync] Add support for enabling session sync remotely.

[Sync] Add support for enabling session sync remotely.

A "sync_tabs" field has been added to the nigori node. When this field is set,
and if the user has elected to keep everything synced, we automatically enable
session sync for them. This is done by way of the migrator and does not require
a restart of sync or user intervention.

BUG=none
TEST=sync_integration_tests --gtest_filter="*SetSyncTabs*"


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

[Nicolas Zea, 2011-08-19 01:30:24]

Please take look!

Tim, in particular I'm slightly concerned about the case where a legitimate
migration happens while we're enabling tab sync. Does the "migration done"
response only occur once? Or will it keep happening until the datatype is
restarted?

[Nicolas Zea, 2011-08-19 16:59:21]

Just realized theres two cases I didn't cover: Restarting after sessions being
enabled should show still have sessions enabled automatically (probably need a
preference for this). Also, once it's been enabled, the user should be able to
disable if they choose (this might already be covered, but I need to test).

So please hold off on the review.

[Nicolas Zea, 2011-08-19 22:09:18]

New, fresher, improved! Please take a look. Drew, I've confirmed that the promo
appears, link works, and disappears after you acknowledge (it also doesn't
require a restart).

Tim as primary reviewer, Drew as FYI and in case anything goes against clank
expectations.

[Nicolas Zea, 2011-08-19 22:11:40]

Also adding Yaron as FYI. The proto changes will need to be duplicated on the
real server, but this can be tested using the python testserver. When you
navigate to http://127.0.0.1:<server_port>/chromiumsync/synctabs it flips the
field for you.

[tim (not reviewing), 2011-08-22 15:42:08]

MigrationDone will keep being returned if a getupdates or commit is issued for
the type in question.  Like I mentioned before, I'm concerned about the overlap
case too. One thing we could do is drop the enable-tab-sync code path if the
migrator is in a non-IDLE state. We can always abort sync-tabs because afaict
we'll keep trying later.  Can we have some similar check to abort if we get the
mig-done code during enabling?

http://codereview.chromium.org/7669073/diff/4002/chrome/browser/sync/glue/syn...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/7669073/diff/4002/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/sync_backend_host.cc:881: if
(registered_types.count(syncable::SESSIONS) == 0) {
Something about this is circuitous.  First, 'Get'ing things from the
SyncFrontend observer is strange.  Especially since you add a pointer to the
factory here but still need to have a Register callback. Seems like
toe-stepping...  I also think we should avoid having so much sessions-specific
(or any type specific) code here.  All of this if (ShouldEnableSessions) logic
doesn't belong in SyncBackendHost.  Can we send a notification out instead?

http://codereview.chromium.org/7669073/diff/4002/chrome/browser/sync/internal...
File chrome/browser/sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/7669073/diff/4002/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/sync_manager.cc:1979: return false;
maybe a VLOG(SyncManager::ShouldEnableSessions can't find Nigori node.)?

http://codereview.chromium.org/7669073/diff/4002/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service_harness.h (right):

http://codereview.chromium.org/7669073/diff/4002/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service_harness.h:164: bool
IsTypeSynced(syncable::ModelType type);
Lets not introduce new terminology for an already crowded space :)  Registered /
Preferred should work, right?

[Andrew T Wilson (Slow), 2011-08-22 20:30:32]

LGTM - I'm going to defer to Tim about the changes to the frontend/backend,
although I did review them and I didn't have any objections.

http://codereview.chromium.org/7669073/diff/4002/chrome/browser/sync/glue/dat...
File chrome/browser/sync/glue/data_type_manager_impl.h (right):

http://codereview.chromium.org/7669073/diff/4002/chrome/browser/sync/glue/dat...
chrome/browser/sync/glue/data_type_manager_impl.h:27:
DataTypeController::TypeMap* controllers);
Can we make |controllers| const?

http://codereview.chromium.org/7669073/diff/4002/chrome/browser/sync/glue/dat...
chrome/browser/sync/glue/data_type_manager_impl.h:79:
DataTypeController::TypeMap* controllers_;
Should this be a const TypeMap*, and can we document who owns this? Not entirely
certain why we changed this from a reference to a ptr, other than the fact that
it's kind of weird to keep a reference to something owned by a separate object.

[Nicolas Zea, 2011-08-22 22:50:15]

Please take another look.

http://codereview.chromium.org/7669073/diff/4002/chrome/browser/sync/glue/dat...
File chrome/browser/sync/glue/data_type_manager_impl.h (right):

http://codereview.chromium.org/7669073/diff/4002/chrome/browser/sync/glue/dat...
chrome/browser/sync/glue/data_type_manager_impl.h:27:
DataTypeController::TypeMap* controllers);
On 2011/08/22 20:30:33, Andrew T Wilson wrote:
> Can we make |controllers| const?

Done.

http://codereview.chromium.org/7669073/diff/4002/chrome/browser/sync/glue/dat...
chrome/browser/sync/glue/data_type_manager_impl.h:79:
DataTypeController::TypeMap* controllers_;
It was actually a copy, not a reference before. Made it const though.

On 2011/08/22 20:30:33, Andrew T Wilson wrote:
> Should this be a const TypeMap*, and can we document who owns this? Not
entirely
> certain why we changed this from a reference to a ptr, other than the fact
that
> it's kind of weird to keep a reference to something owned by a separate
object.

http://codereview.chromium.org/7669073/diff/4002/chrome/browser/sync/glue/syn...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/7669073/diff/4002/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/sync_backend_host.cc:881: if
(registered_types.count(syncable::SESSIONS) == 0) {
On 2011/08/22 15:42:08, timsteele wrote:
> Something about this is circuitous.  First, 'Get'ing things from the
> SyncFrontend observer is strange.  Especially since you add a pointer to the
> factory here but still need to have a Register callback. Seems like
> toe-stepping...  I also think we should avoid having so much sessions-specific
> (or any type specific) code here.  All of this if (ShouldEnableSessions) logic
> doesn't belong in SyncBackendHost.  Can we send a notification out instead?

Done.

http://codereview.chromium.org/7669073/diff/4002/chrome/browser/sync/internal...
File chrome/browser/sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/7669073/diff/4002/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/sync_manager.cc:1979: return false;
On 2011/08/22 15:42:08, timsteele wrote:
> maybe a VLOG(SyncManager::ShouldEnableSessions can't find Nigori node.)?

Done.

http://codereview.chromium.org/7669073/diff/4002/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service_harness.h (right):

http://codereview.chromium.org/7669073/diff/4002/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service_harness.h:164: bool
IsTypeSynced(syncable::ModelType type);
On 2011/08/22 15:42:08, timsteele wrote:
> Lets not introduce new terminology for an already crowded space :)  Registered
/
> Preferred should work, right?

Done.

[tim (not reviewing), 2011-08-23 13:58:52]

http://codereview.chromium.org/7669073/diff/11002/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/7669073/diff/11002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.h:104: virtual void
OnDataTypesChanged(const syncable::ModelTypeSet& to_add) = 0;
Add a descriptive comment.

http://codereview.chromium.org/7669073/diff/11002/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/7669073/diff/11002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:560: return "sync-typed-urls";
How come typed urls is here but the proto only has tabs?

http://codereview.chromium.org/7669073/diff/11002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:575: profile_,
nit - indent

http://codereview.chromium.org/7669073/diff/11002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:669: if (migrator_->state() !=
browser_sync::BackendMigrator::IDLE) {
Did you give any thought to keeping a second instance of BackendMigrator around
for on-the-fly type changes?

[Nicolas Zea, 2011-08-23 16:48:25]

Comments addressed.

http://codereview.chromium.org/7669073/diff/11002/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/7669073/diff/11002/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.h:104: virtual void
OnDataTypesChanged(const syncable::ModelTypeSet& to_add) = 0;
On 2011/08/23 13:58:52, timsteele wrote:
> Add a descriptive comment.

Done.

http://codereview.chromium.org/7669073/diff/11002/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/7669073/diff/11002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:560: return "sync-typed-urls";
On 2011/08/23 13:58:52, timsteele wrote:
> How come typed urls is here but the proto only has tabs?

Because it's an experimental type with a flag, but we don't yet have plans to
add support in the nigori node yet. I figured I'd go ahead and add support for
those experimental types we have here and make this general purpose in case in
the future we want to expand.

http://codereview.chromium.org/7669073/diff/11002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:575: profile_,
On 2011/08/23 13:58:52, timsteele wrote:
> nit - indent

Done.

http://codereview.chromium.org/7669073/diff/11002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:669: if (migrator_->state() !=
browser_sync::BackendMigrator::IDLE) {
I'm didn't really think the extra complexity there was necessary, given we can
just delay until the migrator becomes idle, and that Fred's new patch always
preempts current migrations.

On 2011/08/23 13:58:52, timsteele wrote:
> Did you give any thought to keeping a second instance of BackendMigrator
around
> for on-the-fly type changes?

[tim (not reviewing), 2011-08-23 18:31:53]

LGTM

[commit-bot: I haz the power, 2011-08-23 18:34:21]

Presubmit check for 7669073-14001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
net/tools/testserver/testserver.py,net/tools/testserver/chromiumsync.py

Presubmit checks took 5.0s to calculate.

[Nicolas Zea, 2011-08-23 18:38:02]

+Nick for testserver.py and chromiumsync.py ownership

[ncarter (slow), 2011-08-25 00:05:09]

LGTM with style fixes.

http://codereview.chromium.org/7669073/diff/14001/net/tools/testserver/chromi...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/7669073/diff/14001/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:867: """ Set the 'sync_tabs' field to this
account's nigori node.
No space after """

http://codereview.chromium.org/7669073/diff/14001/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:883: class TestServer(object):
Each top-level definition should be preceded by 2 blank lines; please restore.

http://codereview.chromium.org/7669073/diff/14001/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:959: """ Set the 'sync_tab' field of the
nigori node for this account. """
No space after opening """ and no space between . and closing """

http://codereview.chromium.org/7669073/diff/14001/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:960: self.account.TriggerSyncTabs();
SEMICOLON?!?!

[commit-bot: I haz the power, 2011-08-25 22:21:07]

Change committed as 98328

[Nicolas Zea, 2011-08-25 23:36:23]

http://codereview.chromium.org/7669073/diff/14001/net/tools/testserver/chromi...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/7669073/diff/14001/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:867: """ Set the 'sync_tabs' field to this
account's nigori node.
On 2011/08/25 00:05:09, ncarter wrote:
> No space after """

Done.

http://codereview.chromium.org/7669073/diff/14001/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:883: class TestServer(object):
On 2011/08/25 00:05:09, ncarter wrote:
> Each top-level definition should be preceded by 2 blank lines; please restore.

Done.

http://codereview.chromium.org/7669073/diff/14001/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:959: """ Set the 'sync_tab' field of the
nigori node for this account. """
On 2011/08/25 00:05:09, ncarter wrote:
> No space after opening """ and no space between . and closing """

Done.

http://codereview.chromium.org/7669073/diff/14001/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:960: self.account.TriggerSyncTabs();
On 2011/08/25 00:05:09, ncarter wrote:
> SEMICOLON?!?!

Done.