Fix the no password save issue for ajax login

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
new file mode 100644
index 0000000000000000000000000000000000000000..192f7e52447bbdf27f9a6165b8aac754fa849bf2
--- /dev/null
+++ b/chrome/browser/password_manager/password_manager_browsertest.cc
@@ -0,0 +1,144 @@
+// Copyright (c) 2013 The Chromium Authors. All rights reserved.

[Ilya Sherman, 2013/03/22 03:26:48]

ultra nit: Omit the "(c)"

[Garrett Casto, 2013/03/22 21:05:05]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <string>
+
+#include "chrome/browser/api/infobars/confirm_infobar_delegate.h"
+#include "chrome/browser/api/infobars/infobar_service.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/chrome_notification_types.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/render_view_host.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/test/browser_test_utils.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "ui/base/keycodes/keyboard_codes.h"
+
+class NavigationObserver : public content::NotificationObserver,
+                           public content::WebContentsObserver {
+ public:
+  explicit NavigationObserver(Browser* browser)

[Ilya Sherman, 2013/03/22 03:26:48]

nit: Can you pass in just the WebContents, rather than the full Browser?

[Garrett Casto, 2013/03/22 21:05:05]

Done.

+      : content::WebContentsObserver(
+            browser->tab_strip_model()->GetActiveWebContents()),
+        has_run_message_loop_(false),
+        done_loading_(false),
+        info_bar_shown_(false),
+        browser_(browser),
+        infobar_service_(NULL) {
+    registrar_.Add(this, chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_ADDED,
+                   content::NotificationService::AllSources());

[Ilya Sherman, 2013/03/22 03:26:48]

nit: Can you listen to notifications from a specific source?  Better yet, can
you avoid listening for notifications at all, i.e. hook into this event in a
more direct way?

[Garrett Casto, 2013/03/22 21:05:05]

Changed to use a specific source. I don't think that I can hook this in a more
direct way at the moment. I could add a hook into the PasswordManager where the
password is being saved, but I'm not sure if it's worth it.

+  }
+
+  virtual ~NavigationObserver() {
+    if (!infobar_service_)
+      return;
+
+    InfoBarDelegate* infobar = NULL;
+    if (infobar_service_->GetInfoBarCount() > 0 &&
+        (infobar = infobar_service_->GetInfoBarDelegateAt(0))) {
+      infobar_service_->RemoveInfoBar(infobar);

[Ilya Sherman, 2013/03/22 03:26:48]

Hmm, why do you need to do this manually?  Doesn't calling Accept() on the
infobar cause it to be closed?

[Garrett Casto, 2013/03/22 21:05:05]

I actually just pulled this from autofill_browsertest, but it does look like it
isn't necessary. Removed.

+    }
+  }
+
+  void Wait() {
+    if (!done_loading_) {
+      has_run_message_loop_ = true;
+      content::RunMessageLoop();
+    }
+  }
+
+  bool InfoBarWasShown() {
+    return info_bar_shown_;
+  }
+
+  // content::NotificationObserver:
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE {
+    // Accept in the infobar.
+    infobar_service_ = InfoBarService::FromWebContents(
+        browser_->tab_strip_model()->GetActiveWebContents());
+    InfoBarDelegate* infobar = infobar_service_->GetInfoBarDelegateAt(0);
+
+    ConfirmInfoBarDelegate* confirm_infobar =
+        infobar->AsConfirmInfoBarDelegate();
+    confirm_infobar->Accept();
+    info_bar_shown_ = true;
+  }
+
+  // content::WebContentsObserver
+  virtual void DidFinishLoad(int64 frame_id,
+                             const GURL& validated_url,
+                             bool is_main_frame,
+                             content::RenderViewHost* render_view_host) {
+    done_loading_ = true;
+    if (has_run_message_loop_) {
+      MessageLoopForUI::current()->Quit();
+      has_run_message_loop_ = false;
+    }
+  }
+
+ private:
+  bool has_run_message_loop_;

[Ilya Sherman, 2013/03/22 03:26:48]

Use content::MessageLoopRunner instead.

[Garrett Casto, 2013/03/22 21:05:05]

Done.

+  bool done_loading_;
+  bool info_bar_shown_;
+  content::NotificationRegistrar registrar_;
+  Browser* browser_;
+  InfoBarService* infobar_service_;
+};

[Ilya Sherman, 2013/03/22 03:26:48]

nit: Tuck this into an anonymous namespace.

[Garrett Casto, 2013/03/22 21:05:05]

Done.

+
+class PasswordManagerBrowserTest : public InProcessBrowserTest {
+ protected:
+  content::RenderViewHost* render_view_host() {

[Ilya Sherman, 2013/03/22 03:26:48]

nit: This should use MixedCaps rather than hacker_case.

[Garrett Casto, 2013/03/22 21:05:05]

Done.

+    return browser()->tab_strip_model()->GetActiveWebContents()->
+        GetRenderViewHost();
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest, PromptForXHRSubmit) {

[Ilya Sherman, 2013/03/22 03:26:48]

As long as you're adding test coverage (thanks!), could you add a test for the
regular (non-XHR) case as well?

[Garrett Casto, 2013/03/22 21:05:05]

There is actually a full pyauto test suite for the password manager that I
didn't know about until recently. I was going to file a bug to migrate it to
BrowserTest. That would include testing for the normal case. 

[Ilya Sherman, 2013/03/23 01:10:58]

Ok, fair enough :)

+  ASSERT_TRUE(test_server()->Start());
+
+  GURL url = test_server()->GetURL("files/password/password_xhr_submit.html");
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  // Verify that we show the save password prompt if a form returns false
+  // in it's onsubmit handler but instead logs in/navigates via XHR.

[Ilya Sherman, 2013/03/22 03:26:48]

nit: "it's" -> "its"

[Garrett Casto, 2013/03/22 21:05:05]

Done.

+  // Note that calling 'submit()' on a form with javascript doesn't call
+  // the onsubmit handler, so we click the submit button instead.
+  NavigationObserver observer(browser());
+  std::string fill_and_submit =
+      "document.getElementById('username_field').value = 'temp';"
+      "document.getElementById('password_field').value = 'random';"
+      "document.getElementById('submit_button').click()";
+  ASSERT_TRUE(content::ExecuteScript(render_view_host(), fill_and_submit));

[Ilya Sherman, 2013/03/22 03:26:48]

Can you have the loaded page send a result when it's done loading (using
something like ExecuteScriptAndExtractBool), and use a
WindowedNotificationObserver that listens for that instead?  That seems like it
would be less code, and possibly less error-prone as well.

[Garrett Casto, 2013/03/22 21:05:05]

I'm not sure what exactly you have in mind here. I can imagine setting some
variable during onload of the next page, and then extracting it with
ExecuteScriptAndExtractBool, but this extraction would have to be a loop because
it may run before the page is finished loading, right? That seems like a step in
the wrong direction. Or were you thinking of some other way of doing this?

[Ilya Sherman, 2013/03/23 01:10:58]

Hmm, I kind of ignored the fact that the JavaScript context probably doesn't
last across page loads.  So, never mind about this comment :)

+  observer.Wait();
+  ASSERT_TRUE(observer.InfoBarWasShown());

[Ilya Sherman, 2013/03/22 03:26:48]

nit: I think this can be EXPECT_TRUE.

[Garrett Casto, 2013/03/22 21:05:05]

Done.

+}
+
+IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest, NoPromptForOtherXHR) {
+  ASSERT_TRUE(test_server()->Start());
+
+  GURL url = test_server()->GetURL("files/password/password_xhr_submit.html");
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  // Verify that if random XHR navigation occurs, we don't try and save the
+  // password.
+  //
+  // We may want to change this functionality in the future to account for
+  // cases where the element that users click on isn't a submit button.
+  NavigationObserver observer(browser());
+  std::string fill_and_navigate =
+      "document.getElementById('username_field').value = 'temp';"
+      "document.getElementById('password_field').value = 'random';"
+      "send_xhr()";
+  ASSERT_TRUE(content::ExecuteScript(render_view_host(), fill_and_navigate));
+  observer.Wait();
+  ASSERT_FALSE(observer.InfoBarWasShown());

[Ilya Sherman, 2013/03/22 03:26:48]

nit: I think this can be EXPECT_FALSE.

[Garrett Casto, 2013/03/22 21:05:05]

Done.

+}