Remove useless blocking handle from sync integ tests.

chrome/browser/sync/test/integration/profile_sync_service_harness.cc

Index: chrome/browser/sync/test/integration/profile_sync_service_harness.cc
diff --git a/chrome/browser/sync/test/integration/profile_sync_service_harness.cc b/chrome/browser/sync/test/integration/profile_sync_service_harness.cc
index 97f45645a54a9bc05db265383cc7f7fa1849b5c1..d833b37966bc5db73941ea476e570e1325af2d56 100644
--- a/chrome/browser/sync/test/integration/profile_sync_service_harness.cc
+++ b/chrome/browser/sync/test/integration/profile_sync_service_harness.cc
@@ -152,10 +152,6 @@ bool ProfileSyncServiceHarness::SetupSync(
     return false;
   }
 
-  // Tell the sync service that setup is in progress so we don't start syncing
-  // until we've finished configuration.
-  sync_blocker_ = service()->GetSetupInProgressHandle();

[pavely, 2017/03/07 22:57:53]

Sync blocker guarantees that configuration will not start until after the call
to OnUserChoseDatatypes() while SetFirstSetupComplete() ensures that if
configuration didn't start before, it will at least start now. Configuration
could start earlier if in the future we change code to emulate clicking on sync
confirmation dialog.

I believe your change is correct (you tested that configuration only starts
after SetFirstSyncComplete, right?), but keeping sync_blocker_ is safer.

[skym, 2017/03/07 23:31:10]

My understanding is that Sync will not start until SetFirstSetupComplete() has
been called at least once. The potential threat is that someone else may call it
before you, which is why you'd use the blocker.

You're right, it makes us more fragile to sign in methods, but I'm not convinced
that's a bad thing. If the goal here is "end to end", it makes sense to avoid
cheating as much as possible, right? I think removing the blocker is only good.
Maybe we should even add a check that verifies SetFirstSetupComplete() hasn't
been called yet, before we call it ourselves.

Digging into what SigninType actually does, it looks like UI_SIGNIN is actually
what we use for E2E tests,
https://cs.chromium.org/chromium/src/chrome/browser/sync/test/integration/syn...
. While FAKE_SIGNIN is used for the "normal" integ test runs.

It seems to be like it'd be ideal if on FAKE_SIGNIN we only called
SetFirstSyncComplete() and on UI_SIGNIN we did nothing. Instead we'd let the
dismissal of the dialog call SetFirstSyncComplete() for us, right?

Now, I cannot test this, since E2E tests don't work right now. But I think in
the future we could wrap this as such: 

// Comments.
if (signin_type == SigninType::FAKE_SIGNIN) {
  service()->SetFirstSetupComplete();
}

WDYT?

[pavely, 2017/03/08 01:13:52]

Actually login_ui_test_utils::SignInWithUI only signs user in, it doesn't
dismiss sync confirmation. Instead, call to
login_ui_test_utils::DismissSyncConfirmationDialog does that.

I think your change is safe.

I agree about calling SetFirstSetupComplete() only for FAKE_SIGNIN.

-
   DCHECK(!username_.empty());
   if (signin_type_ == SigninType::UI_SIGNIN) {
     Browser* browser = chrome::FindBrowserWithProfile(profile_);
@@ -188,8 +184,9 @@ bool ProfileSyncServiceHarness::SetupSync(
   bool sync_everything = (synced_datatypes == syncer::UserSelectableTypes());
   service()->OnUserChoseDatatypes(sync_everything, synced_datatypes);
 
-  // Notify ProfileSyncService that we are done with configuration.
-  FinishSyncSetup();
+  // Notify ProfileSyncService that it should begin configuration. This
+  // corresponds to the user confirming it's okay to start Sync.
+  service()->SetFirstSetupComplete();
 
   if ((signin_type_ == SigninType::UI_SIGNIN) &&
       !login_ui_test_utils::DismissSyncConfirmationDialog(
@@ -302,11 +299,6 @@ bool ProfileSyncServiceHarness::IsSyncDisabled() const {
   return !service()->IsSetupInProgress() && !service()->IsFirstSetupComplete();
 }
 
-void ProfileSyncServiceHarness::FinishSyncSetup() {
-  sync_blocker_.reset();
-  service()->SetFirstSetupComplete();
-}
-
 SyncCycleSnapshot ProfileSyncServiceHarness::GetLastCycleSnapshot() const {
   DCHECK(service() != nullptr) << "Sync service has not yet been set up.";
   if (service()->IsSyncActive()) {