Remove useless blocking handle from sync integ tests.

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

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
 
 #include <cstddef>
 #include <iterator>
 #include <ostream>
 #include <sstream>
@@ skipping 134 matching lines @@
     syncer::ModelTypeSet synced_datatypes) {
   DCHECK(!profile_->IsLegacySupervised())
       << "SetupSync should not be used for legacy supervised users.";
 
   // Initialize the sync client's profile sync service object.
   if (service() == nullptr) {
     LOG(ERROR) << "SetupSync(): service() is null.";
     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_);
     DCHECK(browser);
     if (!login_ui_test_utils::SignInWithUI(browser, username_, password_)) {
       LOG(ERROR) << "Could not sign in to GAIA servers.";
       return false;
     }
   } else if (signin_type_ == SigninType::FAKE_SIGNIN) {
     // Authenticate sync client using GAIA credentials.
@@ skipping 12 matching lines @@
 
   if (!AwaitEngineInitialization()) {
     return false;
   }
 
   // Choose the datatypes to be synced. If all datatypes are to be synced,
   // set sync_everything to true; otherwise, set it to false.
   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(
           chrome::FindBrowserWithProfile(profile_),
           base::TimeDelta::FromSeconds(30))) {
     LOG(ERROR) << "Failed to dismiss sync confirmation dialog.";
     return false;
   }
 
   // Set an implicit passphrase for encryption if an explicit one hasn't already
@@ skipping 92 matching lines @@
 
 std::string ProfileSyncServiceHarness::GenerateFakeOAuth2RefreshTokenString() {
   return base::StringPrintf("oauth2_refresh_token_%d",
                             ++oauth2_refesh_token_number_);
 }
 
 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()) {
     return service()->GetLastCycleSnapshot();
   }
   return SyncCycleSnapshot();
 }
 
 bool ProfileSyncServiceHarness::EnableSyncForDatatype(
     syncer::ModelType datatype) {
@@ skipping 156 matching lines @@
 
 std::string ProfileSyncServiceHarness::GetServiceStatus() {
   std::unique_ptr<base::DictionaryValue> value(
       syncer::sync_ui_util::ConstructAboutInformation(
           service(), service()->signin(), chrome::GetChannel()));
   std::string service_status;
   base::JSONWriter::WriteWithOptions(
       *value, base::JSONWriter::OPTIONS_PRETTY_PRINT, &service_status);
   return service_status;
 }