[Passwords] Fix Passwords Reauth in the Password Bubble

Bug: 1049085
Change-Id: I88df16d795642d7e5e789f0b0ce2599d8bec1140
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2041751
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#738999}

chrome/browser/ui/passwords/bubble_controllers/password_bubble_controller_base.cc

@@ -13,6 +13,8 @@ PasswordBubbleControllerBase::PasswordBubbleControllerBase(
     base::WeakPtr<PasswordsModelDelegate> delegate,
     password_manager::metrics_util::UIDisplayDisposition display_disposition)
     : metrics_recorder_(delegate->GetPasswordFormMetricsRecorder()),
+      are_passwords_revealed_when_bubble_is_opened_(
+          delegate->ArePasswordsRevealedWhenBubbleIsOpened()),
       delegate_(std::move(delegate)) {
   if (metrics_recorder_) {
     metrics_recorder_->RecordPasswordBubbleShown(

chrome/browser/ui/passwords/bubble_controllers/password_bubble_controller_base.h

@@ -61,6 +61,9 @@ class PasswordBubbleControllerBase {
   // the bubble is closing.
   bool interaction_reported_ = false;
 
+  // True iff bubble should pop up with revealed password value.
+  const bool are_passwords_revealed_when_bubble_is_opened_;
+
   // A bridge to ManagePasswordsUIController instance.
   base::WeakPtr<PasswordsModelDelegate> delegate_;
 };

chrome/browser/ui/passwords/bubble_controllers/save_update_bubble_controller.cc

@@ -97,7 +97,6 @@ SaveUpdateBubbleController::SaveUpdateBubbleController(
       display_disposition_(
           ComputeDisplayDisposition(display_reason, delegate->GetState())),
       password_revealing_requires_reauth_(false),
-      are_passwords_revealed_when_bubble_is_opened_(false),
       enable_editing_(false),
       dismissal_reason_(metrics_util::NO_DIRECT_INTERACTION),
       clock_(base::DefaultClock::GetInstance()) {
@@ -119,8 +118,7 @@ SaveUpdateBubbleController::SaveUpdateBubbleController(
     }
   }
 
-  if (delegate_->ArePasswordsRevealedWhenBubbleIsOpened()) {
-    are_passwords_revealed_when_bubble_is_opened_ = true;
+  if (are_passwords_revealed_when_bubble_is_opened_) {
     delegate_->OnPasswordsRevealed();
   }
   // The condition for the password reauth:

chrome/browser/ui/passwords/bubble_controllers/save_update_bubble_controller.h

@@ -113,9 +113,6 @@ class SaveUpdateBubbleController : public PasswordBubbleControllerBase {
   // True iff password revealing should require re-auth for privacy reasons.
   bool password_revealing_requires_reauth_;
 
-  // True iff bubble should pop up with revealed password value.
-  bool are_passwords_revealed_when_bubble_is_opened_;
-
   // True iff username/password editing should be enabled.
   bool enable_editing_;
 

chrome/browser/ui/passwords/bubble_controllers/save_update_bubble_controller_unittest.cc

@@ -142,7 +142,6 @@ void SaveUpdateBubbleControllerTest::SetUpWithState(
   GURL origin(kSiteOrigin);
   EXPECT_CALL(*delegate(), GetOrigin()).WillOnce(ReturnRef(origin));
   EXPECT_CALL(*delegate(), GetState()).WillRepeatedly(Return(state));
-  EXPECT_CALL(*delegate(), OnBubbleShown());
   EXPECT_CALL(*delegate(), GetWebContents())
       .WillRepeatedly(Return(test_web_contents_.get()));
   controller_.reset(
@@ -227,6 +226,19 @@ SaveUpdateBubbleControllerTest::GetCurrentForms() const {
   return forms;
 }
 
+// Tests that the controller reads the value of
+// ArePasswordsRevealedWhenBubbleIsOpened() before invoking OnBubbleShown()
+// since the latter resets the value returned by the former. (crbug.com/1049085)
+TEST_F(SaveUpdateBubbleControllerTest,
+       ArePasswordsRevealedWhenBubbleIsOpenedBeforeOnBubbleShown) {
+  {
+    testing::InSequence s;
+    EXPECT_CALL(*delegate(), ArePasswordsRevealedWhenBubbleIsOpened());
+    EXPECT_CALL(*delegate(), OnBubbleShown());
+  }
+  PretendPasswordWaiting();
+}
+
 TEST_F(SaveUpdateBubbleControllerTest, CloseWithoutInteraction) {
   PretendPasswordWaiting();
 

chrome/browser/ui/passwords/bubble_controllers/save_update_with_account_store_bubble_controller.cc

@@ -98,7 +98,6 @@ SaveUpdateWithAccountStoreBubbleController::
       display_disposition_(
           ComputeDisplayDisposition(display_reason, delegate->GetState())),
       password_revealing_requires_reauth_(false),
-      are_passwords_revealed_when_bubble_is_opened_(false),
       enable_editing_(false),
       dismissal_reason_(metrics_util::NO_DIRECT_INTERACTION),
       clock_(base::DefaultClock::GetInstance()) {
@@ -119,8 +118,7 @@ SaveUpdateWithAccountStoreBubbleController::
       interaction_stats_.dismissal_count = stats->dismissal_count;
     }
   }
-  if (delegate_->ArePasswordsRevealedWhenBubbleIsOpened()) {
-    are_passwords_revealed_when_bubble_is_opened_ = true;
+  if (are_passwords_revealed_when_bubble_is_opened_) {
     delegate_->OnPasswordsRevealed();
   }
   // The condition for the password reauth:

chrome/browser/ui/passwords/bubble_controllers/save_update_with_account_store_bubble_controller.h

@@ -110,9 +110,6 @@ class SaveUpdateWithAccountStoreBubbleController
   // True iff password revealing should require re-auth for privacy reasons.
   bool password_revealing_requires_reauth_;
 
-  // True iff bubble should pop up with revealed password value.
-  bool are_passwords_revealed_when_bubble_is_opened_;
-
   // True iff username/password editing should be enabled.
   bool enable_editing_;
 

chrome/browser/ui/passwords/bubble_controllers/save_update_with_account_store_bubble_controller_unittest.cc

@@ -138,7 +138,6 @@ void SaveUpdateWithAccountStoreBubbleControllerTest::SetUpWithState(
   GURL origin(kSiteOrigin);
   EXPECT_CALL(*delegate(), GetOrigin()).WillOnce(ReturnRef(origin));
   EXPECT_CALL(*delegate(), GetState()).WillRepeatedly(Return(state));
-  EXPECT_CALL(*delegate(), OnBubbleShown());
   EXPECT_CALL(*delegate(), GetWebContents())
       .WillRepeatedly(Return(test_web_contents_.get()));
   controller_.reset(new SaveUpdateWithAccountStoreBubbleController(
@@ -224,6 +223,19 @@ SaveUpdateWithAccountStoreBubbleControllerTest::GetCurrentForms() const {
   return forms;
 }
 
+// Tests that the controller reads the value of
+// ArePasswordsRevealedWhenBubbleIsOpened() before invoking OnBubbleShown()
+// since the latter resets the value returned by the former. (crbug.com/1049085)
+TEST_F(SaveUpdateWithAccountStoreBubbleControllerTest,
+       ArePasswordsRevealedWhenBubbleIsOpenedBeforeOnBubbleShown) {
+  {
+    testing::InSequence s;
+    EXPECT_CALL(*delegate(), ArePasswordsRevealedWhenBubbleIsOpened());
+    EXPECT_CALL(*delegate(), OnBubbleShown());
+  }
+  PretendPasswordWaiting();
+}
+
 TEST_F(SaveUpdateWithAccountStoreBubbleControllerTest,
        CloseWithoutInteraction) {
   PretendPasswordWaiting();