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..c28afc42e6f67d9771ffb55f1dfa11d3afb1c7a6 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -69,7 +69,7 @@ ProfileSyncService::ProfileSyncService(ProfileSyncFactory* factory,
                                        const std::string& cros_user)
     : last_auth_error_(AuthError::None()),
       observed_passphrase_required_(false),
-      passphrase_required_for_decryption_(false),
+      passphrase_required_reason_(sync_api::REASON_PASSPHRASE_NOT_REQUIRED),
       passphrase_migration_in_progress_(false),
       factory_(factory),
       profile_(profile),
@@ -592,7 +592,8 @@ void ProfileSyncService::OnClearServerDataSucceeded() {
   }
 }
 
-void ProfileSyncService::OnPassphraseRequired(bool for_decryption) {
+void ProfileSyncService::OnPassphraseRequired(
+    sync_api::PassphraseRequiredReason reason) {
   DCHECK(backend_.get());
   DCHECK(backend_->IsNigoriEnabled());
 
@@ -603,7 +604,12 @@ void ProfileSyncService::OnPassphraseRequired(bool for_decryption) {
     return;
   }
   observed_passphrase_required_ = true;

[tim (not reviewing), 2011/04/29 23:02:13]

can we eliminate this and just check passphrase_reason_ != NOT_REQUIRED ?

We can leave the observed_passphrase_required() method and just change it to
mean this under the covers.

[Raghu Simha, 2011/04/30 00:43:06]

Done.

-  passphrase_required_for_decryption_ = for_decryption;
+
+  // Set passphrase_required_reason_ based on whether the passphrase is required
+  // for encryption or for decryption with a cached passphrase. If decryption
+  // failed and a new passphrase is required, the flag will already be set to
+  // REASON_SET_PASSPHRASE_FAILED.

[Nicolas Zea, 2011/04/29 22:59:30]

That's not true right? If the passphrase changes and we can't decrypt, syncapi
will trigger OnPassphraseRequired(reason..decryption)

[Raghu Simha, 2011/04/30 00:43:06]

The comment is outdated, misleading and probably unnecessary. Removed it, since
right now, all we're doing is simply storing the value of |reason|, whatever it
might be.

+  passphrase_required_reason_ = reason;
 
   if (!cached_passphrase_.value.empty()) {
     SetPassphrase(cached_passphrase_.value,
@@ -613,16 +619,19 @@ void ProfileSyncService::OnPassphraseRequired(bool for_decryption) {
     return;
   }
 
-  // We will skip the passphrase prompt and suppress the warning
-  // if the passphrase is needed for decryption but the user is
-  // not syncing an encrypted data type on this machine.
-  // Otherwise we prompt.
-  if (!IsEncryptedDatatypeEnabled() && for_decryption) {
+  // We will skip the passphrase prompt and suppress the warning if the
+  // passphrase is needed for decryption but the user is not syncing an
+  // encrypted data type on this machine. Otherwise we prompt.
+  if (!IsEncryptedDatatypeEnabled() &&
+      (reason == sync_api::REASON_DECRYPTION ||
+       reason == sync_api::REASON_SET_PASSPHRASE_FAILED)) {
     OnPassphraseAccepted();
     return;
   }
 
-  if (WizardIsVisible() && for_decryption) {
+  if (WizardIsVisible() &&
+      (reason == sync_api::REASON_DECRYPTION ||
+       reason == sync_api::REASON_SET_PASSPHRASE_FAILED)) {
     wizard_.Step(SyncSetupWizard::ENTER_PASSPHRASE);
   }
 
@@ -634,9 +643,12 @@ 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 flags before configuring the DataTypeManager since we know
+  // we no longer require the passphrase.
   observed_passphrase_required_ = false;
+  passphrase_required_reason_ = sync_api::REASON_PASSPHRASE_NOT_REQUIRED;
+
   if (data_type_manager_.get())
     data_type_manager_->Configure(types);
 
@@ -981,12 +993,14 @@ void ProfileSyncService::ConfigureDataTypeManager() {
   encrypted_types_.clear();
   if (types.count(syncable::PASSWORDS) > 0)
     encrypted_types_.insert(syncable::PASSWORDS);
-  if (observed_passphrase_required_ && passphrase_required_for_decryption_) {
+  if (observed_passphrase_required_ &&
+      (passphrase_required_reason_ == sync_api::REASON_DECRYPTION ||
+       passphrase_required_reason_ == sync_api::REASON_SET_PASSPHRASE_FAILED)) {
     if (IsEncryptedDatatypeEnabled()) {
       // We need a passphrase still. Prompt the user for a passphrase, and
       // DataTypeManager::Configure() will get called once the passphrase is
       // accepted.
-      OnPassphraseRequired(true);
+      OnPassphraseRequired(passphrase_required_reason_);
       return;
     } else {
       // We've been informed that a passphrase is required for decryption, but
@@ -1155,7 +1169,9 @@ void ProfileSyncService::Observe(NotificationType type,
       // We should never get in a state where we have no encrypted datatypes
       // enabled, and yet we still think we require a passphrase.
       DCHECK(!(observed_passphrase_required_ &&
-               passphrase_required_for_decryption_ &&
+               (passphrase_required_reason_ == sync_api::REASON_DECRYPTION ||
+                passphrase_required_reason_ ==
+                    sync_api::REASON_SET_PASSPHRASE_FAILED) &&
                !IsEncryptedDatatypeEnabled()));
 
       // TODO(sync): Less wizard, more toast.
@@ -1202,7 +1218,9 @@ void ProfileSyncService::Observe(NotificationType type,
       // If this signin was to initiate a passphrase migration (on the
       // first computer, thus not for decryption), continue the migration.
       if (passphrase_migration_in_progress_ &&
-          !passphrase_required_for_decryption_) {
+          passphrase_required_reason_ != sync_api::REASON_DECRYPTION &&
+          passphrase_required_reason_ !=
+              sync_api::REASON_SET_PASSPHRASE_FAILED) {
         wizard_.Step(SyncSetupWizard::PASSPHRASE_MIGRATION);
         passphrase_migration_in_progress_ = false;
       }