Refactor sync passphrase setup flow and fix passphrase tests

chrome/browser/sync/profile_sync_service.cc

Index: chrome/browser/sync/profile_sync_service.cc
diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc
index d6347d85b9ffdac614afb43f20dda792dd20f129..6fb561c9b8b0dcbb94657a3767f85d3f92828800 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -69,6 +69,7 @@ ProfileSyncService::ProfileSyncService(ProfileSyncFactory* factory,
                                        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),
@@ -634,9 +635,13 @@ void ProfileSyncService::OnPassphraseAccepted() {
   // 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);
 
@@ -645,6 +650,11 @@ void ProfileSyncService::OnPassphraseAccepted() {
   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) {