Persist "Save password" infobars across redirects

chrome/browser/password_manager/password_manager_browsertest.cc

Index: chrome/browser/password_manager/password_manager_browsertest.cc
diff --git a/chrome/browser/password_manager/password_manager_browsertest.cc b/chrome/browser/password_manager/password_manager_browsertest.cc
index db3c84ce1e9b286564480ffa913d4c70c411068a..a031dbe76a609bd2a8691b984818a429c597563d 100644
--- a/chrome/browser/password_manager/password_manager_browsertest.cc
+++ b/chrome/browser/password_manager/password_manager_browsertest.cc
@@ -46,10 +46,15 @@ class NavigationObserver : public content::NotificationObserver,
       : content::WebContentsObserver(web_contents),
         message_loop_runner_(new content::MessageLoopRunner),
         infobar_shown_(false),
+        infobar_removed_(false),
+        auto_accept_infobar_(true),
         infobar_service_(InfoBarService::FromWebContents(web_contents)) {
     registrar_.Add(this,
                    chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_ADDED,
                    content::Source<InfoBarService>(infobar_service_));
+    registrar_.Add(this,
+                   chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED,
+                   content::Source<InfoBarService>(infobar_service_));
   }
 
   virtual ~NavigationObserver() {}
@@ -65,9 +70,19 @@ class NavigationObserver : public content::NotificationObserver,
   virtual void Observe(int type,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE {
-    infobar_service_->infobar_at(0)->delegate()->AsConfirmInfoBarDelegate()->
-        Accept();
-    infobar_shown_ = true;
+    switch (type) {
+      case chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_ADDED:
+        if (auto_accept_infobar_)

[Ilya Sherman, 2014/01/08 23:34:53]

nit: This needs curlies.

[vabr (Chromium), 2014/01/09 11:39:38]

Done.

+          infobar_service_->infobar_at(0)
+              ->delegate()
+              ->AsConfirmInfoBarDelegate()
+              ->Accept();
+        infobar_shown_ = true;
+        break;
+      case chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED:
+        infobar_removed_ = true;
+        break;
+    }

[Ilya Sherman, 2014/01/08 23:34:53]

nit: Pretty sure you need to also have a default case in order to appease clang.

[vabr (Chromium), 2014/01/09 11:39:38]

Clang seemed to be fine with this, but I still added default: NOTREACHED for
documentation sake.

Also, I changed breaks to returns, because there is meant to be no common
processing after the switch statement.

   }
 
   // content::WebContentsObserver:
@@ -85,6 +100,9 @@ class NavigationObserver : public content::NotificationObserver,
   }
 
   bool infobar_shown() const { return infobar_shown_; }
+  bool infobar_removed() const { return infobar_removed_; }
+
+  void DisableAutoAcceptingInfobars() { auto_accept_infobar_ = false; }

[Ilya Sherman, 2014/01/08 23:34:53]

nit: I'd name this "disable_foo", where "foo" is the variable name (see also my
comment below).

[vabr (Chromium), 2014/01/09 11:39:38]

Done.

 
   void Wait() {
     message_loop_runner_->Run();
@@ -94,6 +112,8 @@ class NavigationObserver : public content::NotificationObserver,
   std::string wait_for_path_;
   scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
   bool infobar_shown_;
+  bool infobar_removed_;
+  bool auto_accept_infobar_;

[Ilya Sherman, 2014/01/08 23:34:53]

nit: Please document this member var.

[Ilya Sherman, 2014/01/08 23:34:53]

Optional nit: Perhaps a name like "should_automatically_accept_infobar_" would
be a bit clearer?

[vabr (Chromium), 2014/01/09 11:39:38]

Done.

[vabr (Chromium), 2014/01/09 11:39:38]

Done.

   content::NotificationRegistrar registrar_;
   InfoBarService* infobar_service_;
 
@@ -182,6 +202,29 @@ IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest,
   EXPECT_TRUE(observer.infobar_shown());
 }
 
+IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest, Redirects) {
+  NavigateToFile("/password/password_form.html");
+
+  // Fill a form and submit through a <input type="submit"> button. The form
+  // points to a redirection page.
+  NavigationObserver observer(WebContents());
+  std::string fill_and_submit =
+      "document.getElementById('username_redirect').value = 'temp';"
+      "document.getElementById('password_redirect').value = 'random';"
+      "document.getElementById('submit_redirect').click()";
+  ASSERT_TRUE(content::ExecuteScript(RenderViewHost(), fill_and_submit));
+  observer.DisableAutoAcceptingInfobars();
+  observer.Wait();
+  EXPECT_TRUE(observer.infobar_shown());
+
+  // The redirection page now redirects via Javascript. We check that the
+  // infobar stays.
+  ASSERT_TRUE(content::ExecuteScript(RenderViewHost(),
+                                     "window.location.href = 'done.html';"));
+  observer.Wait();
+  EXPECT_FALSE(observer.infobar_removed());
+}
+
 IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest,
                        PromptForSubmitUsingJavaScript) {
   NavigateToFile("/password/password_form.html");