[sync] Notify OnPassphraseAccepted() last upon SetEncryptionPassphrase()

OnPassphraseAccepted() indicates the completion of the asynchronous
operation initiated by SetEncryptionPassphrase(), so it's cleanest to
notify it last.

This is expected to introduce no noticeable behavioral difference but
simplifies writing tests.

Change-Id: I261c19bcc1b1386234ca878233f34954046297f7
Bug: None
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2328966
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#793149}

components/sync/driver/sync_service_crypto_unittest.cc

@@ -26,6 +26,7 @@ namespace {
 
 using testing::_;
 using testing::Eq;
+using testing::Ne;
 
 sync_pb::EncryptedData MakeEncryptedData(
     const std::string& passphrase,
@@ -282,6 +283,46 @@ class SyncServiceCryptoTest : public testing::Test {
   SyncServiceCrypto crypto_;
 };
 
+// Happy case where no user action is required upon startup.
+TEST_F(SyncServiceCryptoTest, ShouldRequireNoUserAction) {
+  crypto_.SetSyncEngine(CoreAccountInfo(), &engine_);
+  EXPECT_FALSE(crypto_.IsPassphraseRequired());
+  EXPECT_FALSE(crypto_.IsTrustedVaultKeyRequired());
+  EXPECT_TRUE(crypto_.IsTrustedVaultKeyRequiredStateKnown());
+}
+
+TEST_F(SyncServiceCryptoTest, ShouldSetUpNewCustomPassphrase) {
+  const std::string kTestPassphrase = "somepassphrase";
+
+  crypto_.SetSyncEngine(CoreAccountInfo(), &engine_);
+  ASSERT_FALSE(crypto_.IsPassphraseRequired());
+  ASSERT_FALSE(crypto_.IsUsingSecondaryPassphrase());
+  ASSERT_FALSE(crypto_.IsEncryptEverythingEnabled());
+  ASSERT_THAT(crypto_.GetPassphraseType(),
+              Ne(PassphraseType::kCustomPassphrase));
+
+  EXPECT_CALL(engine_, SetEncryptionPassphrase(kTestPassphrase));
+  crypto_.SetEncryptionPassphrase(kTestPassphrase);
+
+  // Mimic completion of the procedure in the sync engine.
+  EXPECT_CALL(notify_observers_cb_, Run());
+  crypto_.OnPassphraseTypeChanged(PassphraseType::kCustomPassphrase,
+                                  base::Time::Now());
+  // The current implementation notifies observers again upon
+  // crypto_.OnEncryptedTypesChanged(). This may change in the future.
+  EXPECT_CALL(notify_observers_cb_, Run());
+  crypto_.OnEncryptedTypesChanged(syncer::EncryptableUserTypes(),
+                                  /*encrypt_everything=*/true);
+  EXPECT_CALL(reconfigure_cb_, Run(CONFIGURE_REASON_CRYPTO));
+  crypto_.OnPassphraseAccepted();
+
+  EXPECT_FALSE(crypto_.IsPassphraseRequired());
+  EXPECT_TRUE(crypto_.IsEncryptEverythingEnabled());
+  ASSERT_THAT(crypto_.GetPassphraseType(),
+              Eq(PassphraseType::kCustomPassphrase));
+  EXPECT_TRUE(crypto_.IsUsingSecondaryPassphrase());
+}
+
 TEST_F(SyncServiceCryptoTest, ShouldExposePassphraseRequired) {
   const std::string kTestPassphrase = "somepassphrase";
 
@@ -308,7 +349,7 @@ TEST_F(SyncServiceCryptoTest, ShouldExposePassphraseRequired) {
   EXPECT_CALL(engine_, SetDecryptionPassphrase(kTestPassphrase))
       .WillOnce([&](const std::string&) { crypto_.OnPassphraseAccepted(); });
   // The current implementation issues two reconfigurations: one immediately
-  // after checking the passphase in the UI thread and a second time later when
+  // after checking the passphrase in the UI thread and a second time later when
   // the engine confirms with OnPassphraseAccepted().
   EXPECT_CALL(reconfigure_cb_, Run(CONFIGURE_REASON_CRYPTO)).Times(2);
   EXPECT_TRUE(crypto_.SetDecryptionPassphrase(kTestPassphrase));

components/sync/nigori/nigori_sync_bridge_impl_unittest.cc

@@ -1010,15 +1010,16 @@ TEST_F(NigoriSyncBridgeImplTest,
   EXPECT_THAT(bridge()->GetData(), HasCustomPassphraseNigori());
 
   // Mimic commit completion.
-  EXPECT_CALL(*observer(), OnPassphraseAccepted());
-  EXPECT_CALL(*observer(), OnEncryptedTypesChanged(
-                               /*encrypted_types=*/EncryptableUserTypes(),
-                               /*encrypt_everything=*/true));
-  EXPECT_CALL(*observer(), OnCryptographerStateChanged(
-                               NotNull(), /*has_pending_keys=*/false));
+  testing::InSequence seq;
   EXPECT_CALL(*observer(),
               OnPassphraseTypeChanged(PassphraseType::kCustomPassphrase,
                                       /*passphrase_time=*/Not(NullTime())));
+  EXPECT_CALL(*observer(), OnCryptographerStateChanged(
+                               NotNull(), /*has_pending_keys=*/false));
+  EXPECT_CALL(*observer(), OnEncryptedTypesChanged(
+                               /*encrypted_types=*/EncryptableUserTypes(),
+                               /*encrypt_everything=*/true));
+  EXPECT_CALL(*observer(), OnPassphraseAccepted());
   EXPECT_CALL(*observer(), OnBootstrapTokenUpdated(Ne(std::string()),
                                                    PASSPHRASE_BOOTSTRAP_TOKEN));
   EXPECT_THAT(bridge()->ApplySyncChanges(base::nullopt), Eq(base::nullopt));

components/sync/nigori/pending_local_nigori_commit.cc

@@ -87,13 +87,13 @@ class CustomPassphraseSetter : public PendingLocalNigoriCommit {
                  SyncEncryptionHandler::Observer* observer) override {
     DCHECK(!state.pending_keys.has_value());
 
-    observer->OnPassphraseAccepted();
     observer->OnPassphraseTypeChanged(PassphraseType::kCustomPassphrase,
                                       state.custom_passphrase_time);
     observer->OnCryptographerStateChanged(state.cryptographer.get(),
                                           /*has_pending_keys=*/false);
     observer->OnEncryptedTypesChanged(EncryptableUserTypes(),
                                       /*encrypt_everything=*/true);
+    observer->OnPassphraseAccepted();
 
     UMA_HISTOGRAM_BOOLEAN("Sync.CustomEncryption", true);
   }