[Sync] Simplify sync startup behavior.

[Sync] Simplify sync startup behavior.

Now IsFirstSetupComplete, IsSetupInProgress, and AUTO_START do not
directly factor into the sync startup logic. Instead, AUTO_START simply
sets IsFirstSetupComplete to true on signin, and sync datatypes can only
be configured if IsFirstSetupComplete && !IsSetupInProgress (aka the new
CanConfigureDataTypes helper function).

This change is motivated by wanting to have better understanding and
control over when and how sync starts up in order to fix the attached
bug.

BUG=462796
Committed: https://crrev.com/76b6100f27b728e8235746b14e4a8c784eae4549
Cr-Commit-Position: refs/heads/master@{#380685}

[maxbogue, 2016-02-25 18:59:13]

maxbogue@chromium.org changed reviewers:
+ zea@chromium.org

[maxbogue, 2016-02-25 18:59:14]

Nicolas, PTAL. I'm still trying to figure out whether the bot failures are
actually caused by my patch somehow, but I think it's ready for a review.

[Nicolas Zea, 2016-02-29 21:21:56]

Do you expect this change to modify any behavior, or should it be entirely
refactoring? Would be good to include that in the commit comment.

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:941:
SetFirstSetupComplete();
Could we just have IsFirstSetupComplete encapsulate this logic? I.e.
IsSetupComplete inspects the start_behavior?

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:1235: #if
!defined(OS_CHROMEOS) && !defined(OS_ANDROID)
I'd like to move away from ifdefs as much as possible, especially controlling
logic like this.

Could we make this dependent on start_behavior_ instead?

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service.h (right):

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.h:157: //   For privacy
reasons, it is usually desirable to avoid syncing any data
Maybe update this comment to mention IsSetupComplete?

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.h:783: bool
IsSetupComplete() const;
I wonder if there's a better way of distinguishing this from
IsFirstSetupComplete. I think they're too similar in name right now.

Effectively this is controlling whether we can kick off configuration right.
Maybe CanStartConfigure?

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service_startup_unittest.cc
(right):

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service_startup_unittest.cc:433:
SetUpSyncBackendHost();
Why is this needed now?

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service_unittest.cc (right):

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service_unittest.cc:487:
EXPECT_FALSE(service()->IsSyncActive());
Verify IsSyncRequested?

https://codereview.chromium.org/1575153004/diff/140001/components/sync_driver...
File components/sync_driver/startup_controller.cc (right):

https://codereview.chromium.org/1575153004/diff/140001/components/sync_driver...
components/sync_driver/startup_controller.cc:151: if (first_start_ &&
!received_start_request_ && !setup_in_progress_) {
This doesn't seem right. Previously if this was CrOS/Android, and it's the first
startup we would start immediately. On the other hand, if it's a restart, we
would defer. The first_start condition here doesn't seem to make sense.

It would  also be good to explain in the comment whether all of the items stated
need to be true.

[maxbogue, 2016-03-09 02:00:59]

PTAL!

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:941:
SetFirstSetupComplete();
On 2016/02/29 21:21:55, Nicolas Zea wrote:
> Could we just have IsFirstSetupComplete encapsulate this logic? I.e.
> IsSetupComplete inspects the start_behavior?

I think that's a bad idea because it's possible to inspect the value directly
through SyncPrefs and we don't ever want the two to be returning different
things.

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:1235: #if
!defined(OS_CHROMEOS) && !defined(OS_ANDROID)
On 2016/02/29 21:21:55, Nicolas Zea wrote:
> I'd like to move away from ifdefs as much as possible, especially controlling
> logic like this.
> 
> Could we make this dependent on start_behavior_ instead?

I changed it this way because this behavior doesn't seem actually related to
auto start. As in, if we decided to turn off auto start on Android (for
example), we'd still not want to sign the user out on Android, right?

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service.h (right):

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.h:157: //   For privacy
reasons, it is usually desirable to avoid syncing any data
On 2016/02/29 21:21:55, Nicolas Zea wrote:
> Maybe update this comment to mention IsSetupComplete?

Done, PTAL.

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.h:783: bool
IsSetupComplete() const;
On 2016/02/29 21:21:55, Nicolas Zea wrote:
> I wonder if there's a better way of distinguishing this from
> IsFirstSetupComplete. I think they're too similar in name right now.
> 
> Effectively this is controlling whether we can kick off configuration right.
> Maybe CanStartConfigure? 

Changed to CanConfigureDatatypes; that sound good?

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service_unittest.cc (right):

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service_unittest.cc:487:
EXPECT_FALSE(service()->IsSyncActive());
On 2016/02/29 21:21:55, Nicolas Zea wrote:
> Verify IsSyncRequested?

Done.

https://codereview.chromium.org/1575153004/diff/140001/components/sync_driver...
File components/sync_driver/startup_controller.cc (right):

https://codereview.chromium.org/1575153004/diff/140001/components/sync_driver...
components/sync_driver/startup_controller.cc:151: if (first_start_ &&
!received_start_request_ && !setup_in_progress_) {
On 2016/02/29 21:21:55, Nicolas Zea wrote:
> This doesn't seem right. Previously if this was CrOS/Android, and it's the
first
> startup we would start immediately. On the other hand, if it's a restart, we
> would defer. The first_start condition here doesn't seem to make sense.

I've clarified the comment; first_start means the first start in this browser
lifetime, not the first start of sync since sync was enabled. Now that you've
mentioned that, isn't this fixing a bug? Don't we want sync startup to still
defer on ChromeOS and Android so it doesn't slow down the browser startup?

> It would  also be good to explain in the comment whether all of the items
stated
> need to be true.

Gah I think my ", and" clauses got eaten by a vim buffer. Fixed.

[Nicolas Zea, 2016-03-09 19:49:45]

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:941:
SetFirstSetupComplete();
On 2016/03/09 02:00:59, maxbogue wrote:
> On 2016/02/29 21:21:55, Nicolas Zea wrote:
> > Could we just have IsFirstSetupComplete encapsulate this logic? I.e.
> > IsSetupComplete inspects the start_behavior?
> 
> I think that's a bad idea because it's possible to inspect the value directly
> through SyncPrefs and we don't ever want the two to be returning different
> things.

Acknowledged.

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:1235: #if
!defined(OS_CHROMEOS) && !defined(OS_ANDROID)
On 2016/03/09 02:00:59, maxbogue wrote:
> On 2016/02/29 21:21:55, Nicolas Zea wrote:
> > I'd like to move away from ifdefs as much as possible, especially
controlling
> > logic like this.
> > 
> > Could we make this dependent on start_behavior_ instead?
> 
> I changed it this way because this behavior doesn't seem actually related to
> auto start. As in, if we decided to turn off auto start on Android (for
> example), we'd still not want to sign the user out on Android, right?
> 

Hmm, interesting. I guess I could see that. Maybe just update the comment to
mention that dekstop and IOS Chrome get signed out (since IOS also is affected
by this)? I started a separate thread about this detail though, as the behavior
doesn't really seem well defined.

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service_startup_unittest.cc
(right):

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service_startup_unittest.cc:433:
SetUpSyncBackendHost();
On 2016/02/29 21:21:55, Nicolas Zea wrote:
> Why is this needed now?

Ping?

https://codereview.chromium.org/1575153004/diff/140001/components/sync_driver...
File components/sync_driver/startup_controller.cc (right):

https://codereview.chromium.org/1575153004/diff/140001/components/sync_driver...
components/sync_driver/startup_controller.cc:151: if (first_start_ &&
!received_start_request_ && !setup_in_progress_) {
On 2016/03/09 02:00:59, maxbogue wrote:
> On 2016/02/29 21:21:55, Nicolas Zea wrote:
> > This doesn't seem right. Previously if this was CrOS/Android, and it's the
> first
> > startup we would start immediately. On the other hand, if it's a restart, we
> > would defer. The first_start condition here doesn't seem to make sense.
> 
> I've clarified the comment; first_start means the first start in this browser
> lifetime, not the first start of sync since sync was enabled. Now that you've
> mentioned that, isn't this fixing a bug? Don't we want sync startup to still
> defer on ChromeOS and Android so it doesn't slow down the browser startup?
> 
> > It would  also be good to explain in the comment whether all of the items
> stated
> > need to be true.
> 
> Gah I think my ", and" clauses got eaten by a vim buffer. Fixed.

Sync should always start up immediately the first time it gets set up. It's only
on restarts that we want to defer. 

Now that I look more closely, this first_start_ boolean was actually introduced
for backup/rollback, and doesn't relate to first time sync vs restart. I think
we can probably get rid of it altogether.

Seems like the condition here should be:
if (setup_in_progress || received_start_request || !FirstSetupComplete)
.. immediate
else
.. deferred

That said, I don't think that catches the first time startup case on
Android/ChromeOS. In those platforms, maybe you need to manually trigger
received_start_request in the same place you set FirstSetupComplete?

[maxbogue, 2016-03-10 18:48:15]

PTAL! I'll be keeping an eye on the bots because the StartupController change
may have broken some tests.

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:1235: #if
!defined(OS_CHROMEOS) && !defined(OS_ANDROID)
On 2016/03/09 19:49:45, Nicolas Zea wrote:
> On 2016/03/09 02:00:59, maxbogue wrote:
> > On 2016/02/29 21:21:55, Nicolas Zea wrote:
> > > I'd like to move away from ifdefs as much as possible, especially
> controlling
> > > logic like this.
> > > 
> > > Could we make this dependent on start_behavior_ instead?
> > 
> > I changed it this way because this behavior doesn't seem actually related to
> > auto start. As in, if we decided to turn off auto start on Android (for
> > example), we'd still not want to sign the user out on Android, right?
> > 
> 
> Hmm, interesting. I guess I could see that. Maybe just update the comment to
> mention that dekstop and IOS Chrome get signed out (since IOS also is affected
> by this)? I started a separate thread about this detail though, as the
behavior
> doesn't really seem well defined.

Done.

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service_startup_unittest.cc
(right):

https://codereview.chromium.org/1575153004/diff/140001/components/browser_syn...
components/browser_sync/browser/profile_sync_service_startup_unittest.cc:433:
SetUpSyncBackendHost();
On 2016/03/09 19:49:45, Nicolas Zea wrote:
> On 2016/02/29 21:21:55, Nicolas Zea wrote:
> > Why is this needed now?
> 
> Ping?

Whoops, I forgot to respond in the previous update but I was still working on
figuring out exactly why. I have now changed this test more extensively, which
should make it clearer. PTAL!

https://codereview.chromium.org/1575153004/diff/140001/components/sync_driver...
File components/sync_driver/startup_controller.cc (right):

https://codereview.chromium.org/1575153004/diff/140001/components/sync_driver...
components/sync_driver/startup_controller.cc:151: if (first_start_ &&
!received_start_request_ && !setup_in_progress_) {
On 2016/03/09 19:49:45, Nicolas Zea wrote:
> On 2016/03/09 02:00:59, maxbogue wrote:
> > On 2016/02/29 21:21:55, Nicolas Zea wrote:
> > > This doesn't seem right. Previously if this was CrOS/Android, and it's the
> > first
> > > startup we would start immediately. On the other hand, if it's a restart,
we
> > > would defer. The first_start condition here doesn't seem to make sense.
> > 
> > I've clarified the comment; first_start means the first start in this
browser
> > lifetime, not the first start of sync since sync was enabled. Now that
you've
> > mentioned that, isn't this fixing a bug? Don't we want sync startup to still
> > defer on ChromeOS and Android so it doesn't slow down the browser startup?
> > 
> > > It would  also be good to explain in the comment whether all of the items
> > stated
> > > need to be true.
> > 
> > Gah I think my ", and" clauses got eaten by a vim buffer. Fixed.
> 
> Sync should always start up immediately the first time it gets set up. It's
only
> on restarts that we want to defer. 
> 
> Now that I look more closely, this first_start_ boolean was actually
introduced
> for backup/rollback, and doesn't relate to first time sync vs restart. I think
> we can probably get rid of it altogether.
> 
> Seems like the condition here should be:
> if (setup_in_progress || received_start_request || !FirstSetupComplete)
> .. immediate
> else
> .. deferred
> 
> That said, I don't think that catches the first time startup case on
> Android/ChromeOS. In those platforms, maybe you need to manually trigger
> received_start_request in the same place you set FirstSetupComplete?

I was wondering if first_start_ was actually needed any more. Glad to see it's
not. I've made the change you suggested and updated the comment. I believe it
will still work on ChromeOS and Android because auto start doesn't flip the
FirstSetupComplete flag until after the backend has started up anyways.

[Nicolas Zea, 2016-03-10 21:21:39]

https://codereview.chromium.org/1575153004/diff/180001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service.cc (left):

https://codereview.chromium.org/1575153004/diff/180001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:948:
DCHECK(IsFirstSetupInProgress());
Should we keep this DCHECK?

https://codereview.chromium.org/1575153004/diff/180001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/1575153004/diff/180001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:818: if
(IsBackendInitialized()) {
Should this check CanConfigureDataTypes? If so, perhaps IsBackendInitialized
should be a condition for CanConfigureDataTypes?

https://codereview.chromium.org/1575153004/diff/180001/components/sync_driver...
File components/sync_driver/startup_controller_unittest.cc (right):

https://codereview.chromium.org/1575153004/diff/180001/components/sync_driver...
components/sync_driver/startup_controller_unittest.cc:215:
TEST_F(StartupControllerTest, NoDeferralWithSetupInProgress) {
nit: would be good to keep the comments that give more detail about the
rationale behind each test

https://codereview.chromium.org/1575153004/diff/180001/components/sync_driver...
components/sync_driver/startup_controller_unittest.cc:223:
TEST_F(StartupControllerTest, NoDeferralOnRestart) {
nit: Shouldn't the name of the test be the opposite?

https://codereview.chromium.org/1575153004/diff/180001/components/sync_driver...
components/sync_driver/startup_controller_unittest.cc:236: ExpectStarted();
I think this should be deferred. I'm pretty sure the test was previously
checking for the reset during the backup/normal transition. We should always
defer on restart. Is this test passing?

[maxbogue, 2016-03-11 00:35:34]

https://codereview.chromium.org/1575153004/diff/180001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service.cc (left):

https://codereview.chromium.org/1575153004/diff/180001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:948:
DCHECK(IsFirstSetupInProgress());
On 2016/03/10 21:21:39, Nicolas Zea wrote:
> Should we keep this DCHECK?

No. IsFirstSetupInProgress is not the inverse of CanConfigureDatatypes. This is
actually the core logic change of this CL: don't configure if
!FirstSetupComplete || SetupInProgress (note ||, not &&). Previously those
conditions were tangled up with whether or not to start at all.

https://codereview.chromium.org/1575153004/diff/180001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/1575153004/diff/180001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:818: if
(IsBackendInitialized()) {
On 2016/03/10 21:21:39, Nicolas Zea wrote:
> Should this check CanConfigureDataTypes? If so, perhaps IsBackendInitialized
> should be a condition for CanConfigureDataTypes?

Doesn't really matter. CanConfigureDatatypes (just noticed the t is supposed to
be uppercase; fixing that) is checked inside ConfigureDataTypes which is checked
inside ReconfigureDataTypes. I'd like to clean up those two functions in a
followup patch, actually. I think having ConfigureDataTypes and
TryConfigureDataTypes might make more sense? TBD.

https://codereview.chromium.org/1575153004/diff/180001/components/sync_driver...
File components/sync_driver/startup_controller_unittest.cc (right):

https://codereview.chromium.org/1575153004/diff/180001/components/sync_driver...
components/sync_driver/startup_controller_unittest.cc:215:
TEST_F(StartupControllerTest, NoDeferralWithSetupInProgress) {
On 2016/03/10 21:21:39, Nicolas Zea wrote:
> nit: would be good to keep the comments that give more detail about the
> rationale behind each test

Done.

https://codereview.chromium.org/1575153004/diff/180001/components/sync_driver...
components/sync_driver/startup_controller_unittest.cc:223:
TEST_F(StartupControllerTest, NoDeferralOnRestart) {
On 2016/03/10 21:21:39, Nicolas Zea wrote:
> nit: Shouldn't the name of the test be the opposite?

Done.

https://codereview.chromium.org/1575153004/diff/180001/components/sync_driver...
components/sync_driver/startup_controller_unittest.cc:236: ExpectStarted();
On 2016/03/10 21:21:39, Nicolas Zea wrote:
> I think this should be deferred. I'm pretty sure the test was previously
> checking for the reset during the backup/normal transition. We should always
> defer on restart. Is this test passing?

It's not passing in this patch, no. I was letting the bots find all the broken
tests for me because I thought more than just StartupControllerTests would
break, but I guess not.

It WAS passing before the patch, which I believe was due to first_start? That
would explain why I was so confused about the intended behavior in this case.
PTAL, should be fixed now.

[maxbogue, 2016-03-11 00:36:23]

Description was changed from

==========
[Sync] Simplify sync startup behavior.

Now IsFirstSetupComplete, IsSetupInProgress, and AUTO_START do not
directly factor into the sync startup logic. Instead, AUTO_START simply
sets IsFirstSetupComplete to true on signin, and sync datatypes can only
be configured if IsFirstSetupComplete && !IsSetupInProgress (aka the new
IsSetupComplete helper function).

This change is motivated by wanting to have better understanding and
control over when and how sync starts up in order to fix the attached
bug.

BUG=462796
==========

to

==========
[Sync] Simplify sync startup behavior.

Now IsFirstSetupComplete, IsSetupInProgress, and AUTO_START do not
directly factor into the sync startup logic. Instead, AUTO_START simply
sets IsFirstSetupComplete to true on signin, and sync datatypes can only
be configured if IsFirstSetupComplete && !IsSetupInProgress (aka the new
CanConfigureDataTypes helper function).

This change is motivated by wanting to have better understanding and
control over when and how sync starts up in order to fix the attached
bug.

BUG=462796
==========

[Nicolas Zea, 2016-03-11 00:47:53]

LGTM!

https://codereview.chromium.org/1575153004/diff/180001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service.cc (left):

https://codereview.chromium.org/1575153004/diff/180001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:948:
DCHECK(IsFirstSetupInProgress());
On 2016/03/11 00:35:34, maxbogue wrote:
> On 2016/03/10 21:21:39, Nicolas Zea wrote:
> > Should we keep this DCHECK?
> 
> No. IsFirstSetupInProgress is not the inverse of CanConfigureDatatypes. This
is
> actually the core logic change of this CL: don't configure if
> !FirstSetupComplete || SetupInProgress (note ||, not &&). Previously those
> conditions were tangled up with whether or not to start at all.

Acknowledged.

https://codereview.chromium.org/1575153004/diff/180001/components/browser_syn...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/1575153004/diff/180001/components/browser_syn...
components/browser_sync/browser/profile_sync_service.cc:818: if
(IsBackendInitialized()) {
On 2016/03/11 00:35:34, maxbogue wrote:
> On 2016/03/10 21:21:39, Nicolas Zea wrote:
> > Should this check CanConfigureDataTypes? If so, perhaps IsBackendInitialized
> > should be a condition for CanConfigureDataTypes?
> 
> Doesn't really matter. CanConfigureDatatypes (just noticed the t is supposed
to
> be uppercase; fixing that) is checked inside ConfigureDataTypes which is
checked
> inside ReconfigureDataTypes. I'd like to clean up those two functions in a
> followup patch, actually. I think having ConfigureDataTypes and
> TryConfigureDataTypes might make more sense? TBD.

Acknowledged.

[maxbogue, 2016-03-11 01:19:17]

The CQ bit was checked by maxbogue@chromium.org

[commit-bot: I haz the power, 2016-03-11 01:20:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1575153004/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1575153004/200001

[commit-bot: I haz the power, 2016-03-11 02:02:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 02:02:15]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[maxbogue, 2016-03-11 17:59:27]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2016-03-11 17:59:27]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1575153004/#ps220001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-03-11 18:00:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1575153004/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1575153004/220001

[commit-bot: I haz the power, 2016-03-11 18:47:20]

Description was changed from

==========
[Sync] Simplify sync startup behavior.

Now IsFirstSetupComplete, IsSetupInProgress, and AUTO_START do not
directly factor into the sync startup logic. Instead, AUTO_START simply
sets IsFirstSetupComplete to true on signin, and sync datatypes can only
be configured if IsFirstSetupComplete && !IsSetupInProgress (aka the new
CanConfigureDataTypes helper function).

This change is motivated by wanting to have better understanding and
control over when and how sync starts up in order to fix the attached
bug.

BUG=462796
==========

to

==========
[Sync] Simplify sync startup behavior.

Now IsFirstSetupComplete, IsSetupInProgress, and AUTO_START do not
directly factor into the sync startup logic. Instead, AUTO_START simply
sets IsFirstSetupComplete to true on signin, and sync datatypes can only
be configured if IsFirstSetupComplete && !IsSetupInProgress (aka the new
CanConfigureDataTypes helper function).

This change is motivated by wanting to have better understanding and
control over when and how sync starts up in order to fix the attached
bug.

BUG=462796
==========

[commit-bot: I haz the power, 2016-03-11 18:47:21]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-03-11 18:48:34]

Description was changed from

==========
[Sync] Simplify sync startup behavior.

Now IsFirstSetupComplete, IsSetupInProgress, and AUTO_START do not
directly factor into the sync startup logic. Instead, AUTO_START simply
sets IsFirstSetupComplete to true on signin, and sync datatypes can only
be configured if IsFirstSetupComplete && !IsSetupInProgress (aka the new
CanConfigureDataTypes helper function).

This change is motivated by wanting to have better understanding and
control over when and how sync starts up in order to fix the attached
bug.

BUG=462796
==========

to

==========
[Sync] Simplify sync startup behavior.

Now IsFirstSetupComplete, IsSetupInProgress, and AUTO_START do not
directly factor into the sync startup logic. Instead, AUTO_START simply
sets IsFirstSetupComplete to true on signin, and sync datatypes can only
be configured if IsFirstSetupComplete && !IsSetupInProgress (aka the new
CanConfigureDataTypes helper function).

This change is motivated by wanting to have better understanding and
control over when and how sync starts up in order to fix the attached
bug.

BUG=462796

Committed: https://crrev.com/76b6100f27b728e8235746b14e4a8c784eae4549
Cr-Commit-Position: refs/heads/master@{#380685}
==========

[commit-bot: I haz the power, 2016-03-11 18:48:35]

Patchset 12 (id:??) landed as
https://crrev.com/76b6100f27b728e8235746b14e4a8c784eae4549
Cr-Commit-Position: refs/heads/master@{#380685}