Hide the password bubble when the underlying ManagePasswordsUIController changes its state.

BUG=468474
TBR=vabr@chromium.org,groby@chromium.org

Review URL: https://codereview.chromium.org/1028243004

Cr-Commit-Position: refs/heads/master@{#323882}
(cherry picked from commit 4a7c97ce6afccbc4b45997dad2a1b1c5d65c4c7f)

Review URL: https://codereview.chromium.org/1069323002

Cr-Commit-Position: refs/branch-heads/2357@{#15}
Cr-Branched-From: 59d4494849b405682265ed5d3f5164573b9a939b-refs/heads/master@{#323860}

11 files changed, 60 insertions, 12 deletions

diff --git a/chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration.h b/chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration.h
index fb23053..e43b6df 100644
--- a/chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration.h
+++ b/chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration.h
@@ -23,6 +23,7 @@ class ManagePasswordsIconCocoa : public ManagePasswordsIcon {
   ManagePasswordsIconCocoa(ManagePasswordsDecoration* decoration);
   virtual ~ManagePasswordsIconCocoa();
   void UpdateVisibleUI() override;
+  void OnChangingState() override;
 
   int icon_id() { return icon_id_; }
   int tooltip_text_id() { return tooltip_text_id_; }
@@ -48,6 +49,9 @@ class ManagePasswordsDecoration : public ImageDecoration {
   // Updates the decoration according to icon state changes.
   void UpdateVisibleUI();
 
+  // Closes the bubble if it's currently displayed.
+  void HideBubble();
+
   // Accessor for the platform-independent interface.
   ManagePasswordsIconCocoa* icon() { return icon_.get(); }
 
diff --git a/chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration.mm b/chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration.mm
index edab9a0..802f4ed 100644
--- a/chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration.mm
+++ b/chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration.mm
@@ -25,6 +25,10 @@ void ManagePasswordsIconCocoa::UpdateVisibleUI() {
   decoration_->UpdateVisibleUI();
 }
 
+void ManagePasswordsIconCocoa::OnChangingState() {
+  decoration_->HideBubble();
+}
+
 // ManagePasswordsDecoration
 
 ManagePasswordsDecoration::ManagePasswordsDecoration(
@@ -75,8 +79,6 @@ void ManagePasswordsDecoration::UpdateUIState() {
   if (icon_->state() == password_manager::ui::INACTIVE_STATE) {
     SetVisible(false);
     SetImage(nil);
-    if (ManagePasswordsBubbleCocoa::instance())
-      ManagePasswordsBubbleCocoa::instance()->Close();
     return;
   }
   SetVisible(true);
@@ -87,3 +89,8 @@ void ManagePasswordsDecoration::UpdateVisibleUI() {
   UpdateUIState();
   OnChange();
 }
+
+void ManagePasswordsDecoration::HideBubble() {
+  if (icon()->active() && ManagePasswordsBubbleCocoa::instance())
+    ManagePasswordsBubbleCocoa::instance()->Close();
+}
diff --git a/chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_cocoa_unittest.mm b/chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_cocoa_unittest.mm
index bd14004..4c7d1e4 100644
--- a/chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_cocoa_unittest.mm
+++ b/chrome/browser/ui/cocoa/passwords/manage_passwords_bubble_cocoa_unittest.mm
@@ -9,6 +9,7 @@
 #include "base/compiler_specific.h"
 #include "base/mac/foundation_util.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_command_controller.h"
 #include "chrome/browser/ui/browser_window.h"
 #import "chrome/browser/ui/cocoa/browser_window_controller.h"
 #include "chrome/browser/ui/cocoa/cocoa_profile_test.h"
@@ -77,6 +78,11 @@ class ManagePasswordsBubbleCocoaTest : public CocoaProfileTest {
     return bubble ? [bubble->controller_ window] : nil;
   }
 
+  ManagePasswordsIconCocoa* icon() {
+    return static_cast<ManagePasswordsIconCocoa*>(
+      ManagePasswordsBubbleCocoa::instance()->icon_);
+  }
+
  private:
   scoped_refptr<content::SiteInstance> siteInstance_;
   content::WebContents* test_web_contents_;  // weak
@@ -124,3 +130,14 @@ TEST_F(ManagePasswordsBubbleCocoaTest, ShowBubbleOnInactiveTabShouldDoNothing) {
   ShowBubble();
   EXPECT_FALSE(ManagePasswordsBubbleCocoa::instance());
 }
+
+TEST_F(ManagePasswordsBubbleCocoaTest, HideBubbleOnChangedState) {
+  ShowBubble();
+  EXPECT_TRUE(ManagePasswordsBubbleCocoa::instance());
+  EXPECT_TRUE([bubbleWindow() isVisible]);
+  EXPECT_TRUE(icon()->active());
+
+  icon()->OnChangingState();
+  EXPECT_FALSE(ManagePasswordsBubbleCocoa::instance());
+  EXPECT_FALSE([bubbleWindow() isVisible]);
+}
diff --git a/chrome/browser/ui/passwords/manage_passwords_icon.cc b/chrome/browser/ui/passwords/manage_passwords_icon.cc
index cbe2855..6ebdda8 100644
--- a/chrome/browser/ui/passwords/manage_passwords_icon.cc
+++ b/chrome/browser/ui/passwords/manage_passwords_icon.cc
@@ -28,6 +28,7 @@ void ManagePasswordsIcon::SetActive(bool active) {
 void ManagePasswordsIcon::SetState(password_manager::ui::State state) {
   if (state_ == state)
     return;
+  OnChangingState();
   state_ = state;
   UpdateIDs();
   UpdateVisibleUI();
diff --git a/chrome/browser/ui/passwords/manage_passwords_icon.h b/chrome/browser/ui/passwords/manage_passwords_icon.h
index 1653740..d4760d4 100644
--- a/chrome/browser/ui/passwords/manage_passwords_icon.h
+++ b/chrome/browser/ui/passwords/manage_passwords_icon.h
@@ -28,10 +28,13 @@ class ManagePasswordsIcon {
   ManagePasswordsIcon();
   ~ManagePasswordsIcon();
 
-  // Called from SetState() iff the icon's state has changed in order to do
-  // whatever platform-specific UI work is necessary given the new state.
+  // Called from SetState() and SetActive() in order to do whatever
+  // platform-specific UI work is necessary.
   virtual void UpdateVisibleUI() = 0;
 
+  // Called from SetState() iff the icon's state has changed.
+  virtual void OnChangingState() = 0;
+
  private:
   // Updates the resource IDs in response to state changes.
   void UpdateIDs();
diff --git a/chrome/browser/ui/passwords/manage_passwords_icon_mock.cc b/chrome/browser/ui/passwords/manage_passwords_icon_mock.cc
index b6150db..1b96561 100644
--- a/chrome/browser/ui/passwords/manage_passwords_icon_mock.cc
+++ b/chrome/browser/ui/passwords/manage_passwords_icon_mock.cc
@@ -12,3 +12,6 @@ ManagePasswordsIconMock::~ManagePasswordsIconMock() {
 
 void ManagePasswordsIconMock::UpdateVisibleUI() {
 }
+
+void ManagePasswordsIconMock::OnChangingState() {
+}
diff --git a/chrome/browser/ui/passwords/manage_passwords_icon_mock.h b/chrome/browser/ui/passwords/manage_passwords_icon_mock.h
index 947dabb..3801a45 100644
--- a/chrome/browser/ui/passwords/manage_passwords_icon_mock.h
+++ b/chrome/browser/ui/passwords/manage_passwords_icon_mock.h
@@ -16,6 +16,7 @@ class ManagePasswordsIconMock : public ManagePasswordsIcon {
  protected:
   // ManagePasswordsIcon:
   void UpdateVisibleUI() override;
+  void OnChangingState() override;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ManagePasswordsIconMock);
diff --git a/chrome/browser/ui/passwords/manage_passwords_ui_controller.cc b/chrome/browser/ui/passwords/manage_passwords_ui_controller.cc
index 5fcb462..5ef980b 100644
--- a/chrome/browser/ui/passwords/manage_passwords_ui_controller.cc
+++ b/chrome/browser/ui/passwords/manage_passwords_ui_controller.cc
@@ -218,11 +218,8 @@ void ManagePasswordsUIController::ChooseCredential(
 void ManagePasswordsUIController::SavePasswordInternal() {
   password_manager::PasswordFormManager* form_manager =
       passwords_data_.form_manager();
-  // TODO(vasilii): it's not OK to call SavePassword() when |form_manager| is 0.
-  // If this is a cause of http://crbug.com/468474 then we should hide the
-  // bubble when ManagePasswordsUIController changes its internal state.
-  if (form_manager)
-    form_manager->Save();
+  DCHECK(form_manager);
+  form_manager->Save();
 }
 
 void ManagePasswordsUIController::NeverSavePassword() {
diff --git a/chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc b/chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc
index 049b679..654b1d56 100644
--- a/chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc
+++ b/chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc
@@ -193,11 +193,12 @@ IN_PROC_BROWSER_TEST_F(ManagePasswordsBubbleViewTest,
   SetupAutomaticPassword();
   EXPECT_TRUE(ManagePasswordsBubbleView::IsShowing());
   ManagePasswordsBubbleView::CloseBubble();
+  content::RunAllPendingInMessageLoop();
   // Re-opening should count as manual.
   ExecuteManagePasswordsCommand();
   EXPECT_TRUE(ManagePasswordsBubbleView::IsShowing());
 
- scoped_ptr<base::HistogramSamples> samples(
+  scoped_ptr<base::HistogramSamples> samples(
       GetSamples(kDisplayDispositionMetric));
   EXPECT_EQ(
       1,
@@ -244,6 +245,15 @@ IN_PROC_BROWSER_TEST_F(ManagePasswordsBubbleViewTest, CloseOnKey) {
   EXPECT_FALSE(ManagePasswordsBubbleView::IsShowing());
 }
 
+IN_PROC_BROWSER_TEST_F(ManagePasswordsBubbleViewTest, CloseOnChangedState) {
+  SetupPendingPassword();
+  EXPECT_TRUE(ManagePasswordsBubbleView::IsShowing());
+  // User navigated very fast and landed on another page with an autofilled
+  // password. The save password bubble should disappear.
+  SetupManagingPasswords();
+  EXPECT_FALSE(ManagePasswordsBubbleView::IsShowing());
+}
+
 IN_PROC_BROWSER_TEST_F(ManagePasswordsBubbleViewTest, TwoTabsWithBubble) {
   // Set up the first tab with the bubble.
   SetupPendingPassword();
diff --git a/chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc b/chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc
index 2294575..a0a54b1 100644
--- a/chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc
+++ b/chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc
@@ -23,8 +23,6 @@ ManagePasswordsIconView::~ManagePasswordsIconView() {}
 void ManagePasswordsIconView::UpdateVisibleUI() {
   if (state() == password_manager::ui::INACTIVE_STATE) {
     SetVisible(false);
-    if (ManagePasswordsBubbleView::IsShowing())
-      ManagePasswordsBubbleView::CloseBubble();
     return;
   }
 
@@ -38,6 +36,12 @@ void ManagePasswordsIconView::UpdateVisibleUI() {
   parent()->Layout();
 }
 
+void ManagePasswordsIconView::OnChangingState() {
+  // If there is an opened bubble for the current icon it should go away.
+  if (active())
+    ManagePasswordsBubbleView::CloseBubble();
+}
+
 bool ManagePasswordsIconView::IsBubbleShowing() const {
   // If the bubble is being destroyed, it's considered showing though it may be
   // already invisible currently.
diff --git a/chrome/browser/ui/views/passwords/manage_passwords_icon_view.h b/chrome/browser/ui/views/passwords/manage_passwords_icon_view.h
index 7004a84..2d0c3d0 100644
--- a/chrome/browser/ui/views/passwords/manage_passwords_icon_view.h
+++ b/chrome/browser/ui/views/passwords/manage_passwords_icon_view.h
@@ -38,6 +38,7 @@ class ManagePasswordsIconView : public ManagePasswordsIcon,
  protected:
   // ManagePasswordsIcon:
   void UpdateVisibleUI() override;
+  void OnChangingState() override;
 
  private: