[Sync] Support open tabs experiment enabling before sync setup completion.

[Sync] Support open tabs experiment enabling before sync setup completion.

BUG=99403
TEST=sync_integration_tests, manually ensure on first time sync open tabs is
an option in customize for an account with sync_tabs true.

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

[Nicolas Zea, 2011-10-14 22:18:15]

PTAL

[akalin, 2011-10-14 23:18:48]

http://codereview.chromium.org/8295017/diff/1/chrome/browser/sync/glue/sync_b...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/8295017/diff/1/chrome/browser/sync/glue/sync_b...
chrome/browser/sync/glue/sync_backend_host.cc:823: if
(sync_manager()->ReceivedExperimentalTypes(&to_add)) {
hmm.  Thinking about this, I think it's cleaner to just cache to_add if not
initialized yet and call OnDataTypesChanged after we do initialize (in
HandleInitializationCompleteOnFrontendLoop).  I'm wary of making some functions
callable when uninitialized and some not.  Although it's a pain because it would
add an extra member variable and have two places to do the same thing.

Maybe have a helper fn:

SBH::AddExperimentalTypes() {
  CHECK(initialized());
  ModelTypeSet to_add;
  if (sync_manager()->Received..()) {
    frontend_->OnDataTypesChanged(...);
  }
}

and then here:

if (host->initialized()) {
  host->AddExperimentalTypes();
}

and in HandleInitializationCompleteOnFrontendLoop:

case REFRESHING_ENCRYPTION:
  initialization_state_ = INITIALIZED;
  frontend_->OnBackendInitialized(...);
  AddExperimentalTypes();

[Nicolas Zea, 2011-10-15 00:31:35]

Comment addressed. PTAL

http://codereview.chromium.org/8295017/diff/1/chrome/browser/sync/glue/sync_b...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/8295017/diff/1/chrome/browser/sync/glue/sync_b...
chrome/browser/sync/glue/sync_backend_host.cc:823: if
(sync_manager()->ReceivedExperimentalTypes(&to_add)) {
On 2011/10/14 23:18:48, akalin wrote:
> hmm.  Thinking about this, I think it's cleaner to just cache to_add if not
> initialized yet and call OnDataTypesChanged after we do initialize (in
> HandleInitializationCompleteOnFrontendLoop).  I'm wary of making some
functions
> callable when uninitialized and some not.  Although it's a pain because it
would
> add an extra member variable and have two places to do the same thing.
> 
> Maybe have a helper fn:
> 
> SBH::AddExperimentalTypes() {
>   CHECK(initialized());
>   ModelTypeSet to_add;
>   if (sync_manager()->Received..()) {
>     frontend_->OnDataTypesChanged(...);
>   }
> }
> 
> and then here:
> 
> if (host->initialized()) {
>   host->AddExperimentalTypes();
> }
> 
> and in HandleInitializationCompleteOnFrontendLoop:
> 
> case REFRESHING_ENCRYPTION:
>   initialization_state_ = INITIALIZED;
>   frontend_->OnBackendInitialized(...);
>   AddExperimentalTypes();

Done.

[akalin, 2011-10-15 01:21:55]

LGTM

http://codereview.chromium.org/8295017/diff/1004/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/8295017/diff/1004/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:610: if (migrator_.get() &&
can remove the _.get() check, or convert to CHECK

http://codereview.chromium.org/8295017/diff/1004/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:658: if (!to_register.empty() &&
HasSyncSetupCompleted() && migrator_.get()) {
can remove migrator_.get() check

http://codereview.chromium.org/8295017/diff/1004/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:658: if (!to_register.empty() &&
HasSyncSetupCompleted() && migrator_.get()) {
do we still need the HasSyncSetupCompleted check?

[Nicolas Zea, 2011-10-15 02:06:27]

Committing once trybots go green.

http://codereview.chromium.org/8295017/diff/1004/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/8295017/diff/1004/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:610: if (migrator_.get() &&
On 2011/10/15 01:21:55, akalin wrote:
> can remove the _.get() check, or convert to CHECK
This is getting called before the frontend is notified of the backend
initializing (else the sync setup screen shows before we enable the type), so
these conditions have to stay.

http://codereview.chromium.org/8295017/diff/1004/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:658: if (!to_register.empty() &&
HasSyncSetupCompleted() && migrator_.get()) {
On 2011/10/15 01:21:55, akalin wrote:
> can remove migrator_.get() check
see above.

http://codereview.chromium.org/8295017/diff/1004/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:658: if (!to_register.empty() &&
HasSyncSetupCompleted() && migrator_.get()) {
On 2011/10/15 01:21:55, akalin wrote:
> do we still need the HasSyncSetupCompleted check?

Theoretically the migrator check covers this case, and vice versa, but I'd
rather not make assumptions about both of them being there in case something
changes in the future. The main point here is that we don't want to reconfigure
if the user hasn't finished setting up yet, so HasSyncSetupCompleted is the main
condition.

[akalin, 2011-10-18 19:13:42]

Can you add the comment suggested below in a separate CL?

http://codereview.chromium.org/8295017/diff/1004/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/8295017/diff/1004/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:610: if (migrator_.get() &&
On 2011/10/15 02:06:27, nzea wrote:
> On 2011/10/15 01:21:55, akalin wrote:
> > can remove the _.get() check, or convert to CHECK
> This is getting called before the frontend is notified of the backend
> initializing (else the sync setup screen shows before we enable the type), so
> these conditions have to stay.

Can you add a comment here to that effect?