Refactor sync passphrase setup flow and fix passphrase tests

chrome/browser/sync/profile_sync_service.cc

 // Copyright (c) 2011 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/profile_sync_service.h"
 
 #include <stddef.h>
 #include <map>
 #include <ostream>
 #include <set>
@@ skipping 51 matching lines @@
 const char* ProfileSyncService::kDevServerUrl =
     "https://clients4.google.com/chrome-sync/dev";
 
 static const int kSyncClearDataTimeoutInSeconds = 60;  // 1 minute.
 
 ProfileSyncService::ProfileSyncService(ProfileSyncFactory* factory,
                                        Profile* profile,
                                        const std::string& cros_user)
     : last_auth_error_(AuthError::None()),
       observed_passphrase_required_(false),
+      observed_passphrase_failed_(false),
       passphrase_required_for_decryption_(false),
       passphrase_migration_in_progress_(false),
       factory_(factory),
       profile_(profile),
       cros_user_(cros_user),
       sync_service_url_(kDevServerUrl),
       backend_initialized_(false),
       is_auth_in_progress_(false),
       wizard_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
       unrecoverable_error_detected_(false),
@@ skipping 545 matching lines @@
   }
 
   NotifyObservers();
 }
 
 void ProfileSyncService::OnPassphraseAccepted() {
   // Make sure the data types that depend on the passphrase are started at
   // this time.
   syncable::ModelTypeSet types;
   GetPreferredDataTypes(&types);
-  // Reset "passphrase_required" flag before configuring the DataTypeManager
-  // since we know we no longer require the passphrase.
+
+  // Reset "passphrase_required" and "passphrase_failed" flags before
+  // configuring the DataTypeManager since we know we no longer require the
+  // passphrase.
   observed_passphrase_required_ = false;
+  observed_passphrase_failed_ = false;
+
   if (data_type_manager_.get())
     data_type_manager_->Configure(types);
 
   NotifyObservers();
 
   wizard_.Step(SyncSetupWizard::DONE);
 }
 
+void ProfileSyncService::OnPassphraseFailed() {
+  observed_passphrase_failed_ = true;
+  OnPassphraseRequired(true);

[tim (not reviewing), 2011/04/28 17:18:00]

This seems like a step in the wrong direction.  We've tried to reduce the amount
of variables like this the PSS keeps around, and now we have synonyms with
uber-subtle differences :(   There has to be another way.

[Raghu Simha, 2011/04/28 17:44:21]

Like I explained in the other comment, this is to differentiate between cases
where forward progress is possible and cases when it's not. If you look at the
header comments in profile_sync_service_observer.h, it says that "Observers
should query the service to determine what happened". As of today, there is
nothing in ProfileSyncService that the harness can query to find out if an
existing (cached) passphrase simply needs to be tried out, or if it has already
been tried and the attempt failed.

I agree this mechanism doesn't scale well. I'd love to hear your ideas for how
to fix this. The thing is that if we treat both cases identically, every test
that sets a passphrase on one client will have to have other clients wait for up
to 45 seconds before it knows that it should try a new passphrase. Keeping count
of the number of times OnStateChange() was called doesn't work because it can be
called for any number of other reasons.

+}
+
 void ProfileSyncService::OnEncryptionComplete(
     const syncable::ModelTypeSet& encrypted_types) {
   if (encrypted_types_ != encrypted_types) {
     encrypted_types_ = encrypted_types;
     NotifyObservers();
   }
 }
 
 void ProfileSyncService::OnMigrationNeededForTypes(
     const syncable::ModelTypeSet& types) {
@@ skipping 621 matching lines @@
   // is initialized, all enabled data types are consistent with one
   // another, and no unrecoverable error has transpired.
   if (unrecoverable_error_detected_)
     return false;
 
   if (!data_type_manager_.get())
     return false;
 
   return data_type_manager_->state() == DataTypeManager::CONFIGURED;
 }