Clean up password manager code.

BUG=none
TEST=password saving should continue to work


Review URL: http://codereview.chromium.org/9564001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@124735 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/extensions/extension_menu_manager_unittest.cc

@@ -370,7 +370,7 @@ class MockTestingProfile : public TestingProfile {
  public:
   MockTestingProfile() {}
   MOCK_METHOD0(GetExtensionEventRouter, ExtensionEventRouter*());
-  MOCK_METHOD0(IsOffTheRecord, bool());
+  MOCK_CONST_METHOD0(IsOffTheRecord, bool());
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockTestingProfile);

chrome/browser/password_manager/password_form_manager.cc

-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -305,7 +305,7 @@ void PasswordFormManager::OnRequestDone(int handle,
   else
     manager_action_ = kManagerActionAutofilled;
   password_manager_->Autofill(observed_form_, best_matches_,
-                              preferred_match_, wait_for_username);
+                              *preferred_match_, wait_for_username);
 }
 
 void PasswordFormManager::OnPasswordStoreRequestDone(

chrome/browser/password_manager/password_manager.cc

-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/password_manager/password_manager.h"
 
-#include <vector>
-
-#include "base/stl_util.h"
 #include "base/threading/platform_thread.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/password_manager/password_form_manager.h"
@@ -24,15 +21,7 @@ using content::WebContents;
 using webkit::forms::PasswordForm;
 using webkit::forms::PasswordFormMap;
 
-// static
-void PasswordManager::RegisterUserPrefs(PrefService* prefs) {
-  prefs->RegisterBooleanPref(prefs::kPasswordManagerEnabled,
-                             true,
-                             PrefService::SYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kPasswordManagerAllowShowPasswords,
-                             true,
-                             PrefService::UNSYNCABLE_PREF);
-}
+namespace {
 
 // This routine is called when PasswordManagers are constructed.
 //
@@ -40,7 +29,7 @@ void PasswordManager::RegisterUserPrefs(PrefService* prefs) {
 // that this is only ever called from a single thread in order to
 // avoid needing to lock (a static boolean flag is then sufficient to
 // guarantee running only once).
-static void ReportMetrics(bool password_manager_enabled) {
+void ReportMetrics(bool password_manager_enabled) {
   static base::PlatformThreadId initial_thread_id =
       base::PlatformThread::CurrentId();
   DCHECK(initial_thread_id == base::PlatformThread::CurrentId());
@@ -50,16 +39,29 @@ static void ReportMetrics(bool password_manager_enabled) {
     return;
   ran_once = true;
 
+  // TODO(isherman): This does not actually measure a user action.  It should be
+  // a boolean histogram.
   if (password_manager_enabled)
     content::RecordAction(UserMetricsAction("PasswordManager_Enabled"));
   else
     content::RecordAction(UserMetricsAction("PasswordManager_Disabled"));
 }
 
+}  // anonymous namespace
+
+// static
+void PasswordManager::RegisterUserPrefs(PrefService* prefs) {
+  prefs->RegisterBooleanPref(prefs::kPasswordManagerEnabled,
+                             true,
+                             PrefService::SYNCABLE_PREF);
+  prefs->RegisterBooleanPref(prefs::kPasswordManagerAllowShowPasswords,
+                             true,
+                             PrefService::UNSYNCABLE_PREF);
+}
+
 PasswordManager::PasswordManager(WebContents* web_contents,
                                  PasswordManagerDelegate* delegate)
     : content::WebContentsObserver(web_contents),
-      login_managers_deleter_(&pending_login_managers_),
       delegate_(delegate),
       observer_(NULL) {
   DCHECK(delegate_);
@@ -72,22 +74,21 @@ PasswordManager::PasswordManager(WebContents* web_contents,
 PasswordManager::~PasswordManager() {
 }
 
-void PasswordManager::ProvisionallySavePassword(PasswordForm form) {
-  if (!delegate_->GetProfileForPasswordManager() ||
-      delegate_->GetProfileForPasswordManager()->IsOffTheRecord() ||
-      !*password_manager_enabled_)
+void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
+  if (!IsEnabled())
     return;
 
   // No password to save? Then don't.
   if (form.password_value.empty())
     return;
 
-  LoginManagers::iterator iter;
   PasswordFormManager* manager = NULL;
-  for (iter = pending_login_managers_.begin();
-       iter != pending_login_managers_.end(); iter++) {
+  for (ScopedVector<PasswordFormManager>::iterator iter =
+           pending_login_managers_.begin();
+       iter != pending_login_managers_.end(); ++iter) {
     if ((*iter)->DoesManage(form)) {
       manager = *iter;
+      pending_login_managers_.weak_erase(iter);
       break;
     }
   }
@@ -109,26 +110,12 @@ void PasswordManager::ProvisionallySavePassword(PasswordForm form) {
   if (manager->IsBlacklisted())
     return;
 
-  form.ssl_valid = form.origin.SchemeIsSecure() &&
+  PasswordForm provisionally_saved_form(form);
+  provisionally_saved_form.ssl_valid = form.origin.SchemeIsSecure() &&
       !delegate_->DidLastPageLoadEncounterSSLErrors();
-  form.preferred = true;
-  manager->ProvisionallySave(form);
+  provisionally_saved_form.preferred = true;
+  manager->ProvisionallySave(provisionally_saved_form);
   provisional_save_manager_.reset(manager);
-  pending_login_managers_.erase(iter);
-  // We don't care about the rest of the forms on the page now that one
-  // was selected.
-  STLDeleteElements(&pending_login_managers_);
-}
-
-void PasswordManager::DidNavigate() {
-  // As long as this navigation isn't due to a currently pending
-  // password form submit, we're ready to reset and move on.
-  if (!provisional_save_manager_.get() && !pending_login_managers_.empty())
-    STLDeleteElements(&pending_login_managers_);
-}
-
-void PasswordManager::ClearProvisionalSave() {
-  provisional_save_manager_.reset();
 }
 
 void PasswordManager::SetObserver(LoginModelObserver* observer) {
@@ -139,11 +126,8 @@ void PasswordManager::DidStopLoading() {
   if (!provisional_save_manager_.get())
     return;
 
-  DCHECK(!delegate_->GetProfileForPasswordManager()->IsOffTheRecord());
-  DCHECK(!provisional_save_manager_->IsBlacklisted());
+  DCHECK(IsEnabled());
 
-  if (!delegate_->GetProfileForPasswordManager())
-    return;
   // Form is not completely valid - we do not support it.
   if (!provisional_save_manager_->HasValidPasswordForm())
     return;
@@ -162,8 +146,17 @@ void PasswordManager::DidStopLoading() {
 void PasswordManager::DidNavigateAnyFrame(
       const content::LoadCommittedDetails& details,
       const content::FrameNavigateParams& params) {
-  if (params.password_form.origin.is_valid())
+  if (!params.password_form.origin.is_valid()) {
+    // This codepath essentially means that a frame not containing a password
+    // form was navigated.  For example, this might be a subframe of the main
+    // page, navigated either automatically or in response to a user action.
+    return;
+  }
+
+  // There might be password data to provisionally save. Other than that, we're
+  // ready to reset and move on.
   ProvisionallySavePassword(params.password_form);
+  pending_login_managers_.reset();
 }
 
 bool PasswordManager::OnMessageReceived(const IPC::Message& message) {
@@ -180,16 +173,14 @@ bool PasswordManager::OnMessageReceived(const IPC::Message& message) {
 
 void PasswordManager::OnPasswordFormsFound(
     const std::vector<PasswordForm>& forms) {
-  if (!delegate_->GetProfileForPasswordManager())
-    return;
-  if (!*password_manager_enabled_)
+  if (!IsEnabled())
     return;
 
   // Ask the SSLManager for current security.
   bool had_ssl_error = delegate_->DidLastPageLoadEncounterSSLErrors();
 
-  std::vector<PasswordForm>::const_iterator iter;
-  for (iter = forms.begin(); iter != forms.end(); iter++) {
+  for (std::vector<PasswordForm>::const_iterator iter = forms.begin();
+       iter != forms.end(); ++iter) {
     bool ssl_valid = iter->origin.SchemeIsSecure() && !had_ssl_error;
     PasswordFormManager* manager =
         new PasswordFormManager(delegate_->GetProfileForPasswordManager(),
@@ -203,15 +194,16 @@ void PasswordManager::OnPasswordFormsVisible(
     const std::vector<PasswordForm>& visible_forms) {
   if (!provisional_save_manager_.get())
     return;
-  std::vector<PasswordForm>::const_iterator iter;
-  for (iter = visible_forms.begin(); iter != visible_forms.end(); iter++) {
+
+  for (std::vector<PasswordForm>::const_iterator iter = visible_forms.begin();
+       iter != visible_forms.end(); ++iter) {
     if (provisional_save_manager_->DoesManage(*iter)) {
       // The form trying to be saved has immediately re-appeared. Assume login
       // failure and abort this save, by clearing provisional_save_manager_.
       // Don't delete the login managers since the user may try again
       // and we want to be able to save in that case.
       provisional_save_manager_->SubmitFailed();
-      ClearProvisionalSave();
+      provisional_save_manager_.reset();
       break;
     }
   }
@@ -220,9 +212,8 @@ void PasswordManager::OnPasswordFormsVisible(
 void PasswordManager::Autofill(
     const PasswordForm& form_for_autofill,
     const PasswordFormMap& best_matches,
-    const PasswordForm* const preferred_match,
+    const PasswordForm& preferred_match,
     bool wait_for_username) const {
-  DCHECK(preferred_match);
   switch (form_for_autofill.scheme) {
     case PasswordForm::SCHEME_HTML: {
       // Note the check above is required because the observer_ for a non-HTML
@@ -230,7 +221,7 @@ void PasswordManager::Autofill(
       webkit::forms::PasswordFormFillData fill_data;
       webkit::forms::PasswordFormDomManager::InitFillData(form_for_autofill,
                                                           best_matches,
-                                                        preferred_match,
+                                                          &preferred_match,
                                                           wait_for_username,
                                                           &fill_data);
       delegate_->FillPasswordForm(fill_data);
@@ -238,8 +229,13 @@ void PasswordManager::Autofill(
     }
     default:
       if (observer_) {
-        observer_->OnAutofillDataAvailable(preferred_match->username_value,
-                                           preferred_match->password_value);
+        observer_->OnAutofillDataAvailable(preferred_match.username_value,
+                                           preferred_match.password_value);
       }
   }
 }
+
+bool PasswordManager::IsEnabled() const {
+  const Profile* profile = delegate_->GetProfileForPasswordManager();
+  return profile && !profile->IsOffTheRecord() && *password_manager_enabled_;
+}

chrome/browser/password_manager/password_manager.h

-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -6,7 +6,10 @@
 #define CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_MANAGER_H_
 #pragma once
 
+#include <vector>
+
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/scoped_vector.h"
 #include "base/stl_util.h"
 #include "chrome/browser/password_manager/password_form_manager.h"
 #include "chrome/browser/prefs/pref_member.h"
@@ -38,16 +41,18 @@ class PasswordManager : public LoginModel,
   // on the page.
   void Autofill(const webkit::forms::PasswordForm& form_for_autofill,
                 const webkit::forms::PasswordFormMap& best_matches,
-                const webkit::forms::PasswordForm* const preferred_match,
+                const webkit::forms::PasswordForm& preferred_match,
                 bool wait_for_username) const;
 
   // LoginModel implementation.
   virtual void SetObserver(LoginModelObserver* observer) OVERRIDE;
 
+  // TODO(isherman): This should not be public, but is currently being used by
+  // the LoginPrompt code.
   // When a form is submitted, we prepare to save the password but wait
   // until we decide the user has successfully logged in. This is step 1
   // of 2 (see SavePassword).
-  void ProvisionallySavePassword(webkit::forms::PasswordForm form);
+  void ProvisionallySavePassword(const webkit::forms::PasswordForm& form);
 
   // content::WebContentsObserver overrides.
   virtual void DidStopLoading() OVERRIDE;
@@ -56,14 +61,14 @@ class PasswordManager : public LoginModel,
       const content::FrameNavigateParams& params) OVERRIDE;
   virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
 
+  // TODO(isherman): This should not be public, but is currently being used by
+  // the LoginPrompt code.
   void OnPasswordFormsFound(
       const std::vector<webkit::forms::PasswordForm>& forms);
   void OnPasswordFormsVisible(
       const std::vector<webkit::forms::PasswordForm>& visible_forms);
 
  private:
-  FRIEND_TEST_ALL_PREFIXES(PasswordManagerTest, FormSeenThenLeftPage);
-
   // Note about how a PasswordFormManager can transition from
   // pending_login_managers_ to provisional_save_manager_ and the infobar.
   //
@@ -78,20 +83,10 @@ class PasswordManager : public LoginModel,
   // When a form is "seen" on a page, a PasswordFormManager is created
   // and stored in this collection until user navigates away from page.
 
-  // Clear any pending saves
-  void ClearProvisionalSave();
-
-  // Notification that the user navigated away from the current page.
-  // Unless this is a password form submission, for our purposes this
-  // means we're done with the current page, so we can clean-up.
-  void DidNavigate();
-
-  typedef std::vector<PasswordFormManager*> LoginManagers;
-  LoginManagers pending_login_managers_;
+  // Is password autofill enabled for the current profile?
+  bool IsEnabled() const;
 
-  // Deleter for pending_login_managers_ when PasswordManager is deleted (e.g
-  // tab closes) on a page with a password form, thus containing login managers.
-  STLElementDeleter<LoginManagers> login_managers_deleter_;
+  ScopedVector<PasswordFormManager> pending_login_managers_;
 
   // When the user submits a password/credential, this contains the
   // PasswordFormManager for the form in question until we deem the login
@@ -103,7 +98,7 @@ class PasswordManager : public LoginModel,
 
   // Our delegate for carrying out external operations.  This is typically the
   // containing TabContents.
-  PasswordManagerDelegate* delegate_;
+  PasswordManagerDelegate* const delegate_;
 
   // The LoginModelObserver (i.e LoginView) requiring autofill.
   LoginModelObserver* observer_;

chrome/browser/password_manager/password_manager_unittest.cc

-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -14,6 +14,8 @@
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
 #include "content/browser/tab_contents/test_tab_contents.h"
+#include "content/public/browser/navigation_details.h"
+#include "content/public/common/frame_navigate_params.h"
 #include "content/test/test_browser_thread.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -206,12 +208,51 @@ TEST_F(PasswordManagerTest, FormSeenThenLeftPage) {
   manager()->OnPasswordFormsFound(observed);  // The initial load.
   manager()->OnPasswordFormsVisible(observed);  // The initial layout.
 
-  manager()->DidNavigate();
+  content::LoadCommittedDetails details;
+  content::FrameNavigateParams params;
+  params.password_form = form;
+  manager()->DidNavigateAnyFrame(details, params);
 
   // No expected calls.
   manager()->DidStopLoading();
 }
 
+TEST_F(PasswordManagerTest, FormSubmitAfterNavigateSubframe) {
+  // Test that navigating a subframe does not prevent us from showing the save
+  // password infobar.
+  std::vector<PasswordForm*> result;  // Empty password store.
+  EXPECT_CALL(delegate_, FillPasswordForm(_)).Times(Exactly(0));
+  EXPECT_CALL(*store_, GetLogins(_,_))
+      .WillOnce(DoAll(WithArg<1>(InvokeConsumer(0, result)), Return(0)));
+  std::vector<PasswordForm> observed;
+  PasswordForm form(MakeSimpleForm());
+  observed.push_back(form);
+  manager()->OnPasswordFormsFound(observed);  // The initial load.
+  manager()->OnPasswordFormsVisible(observed);  // The initial layout.
+
+  scoped_ptr<PasswordFormManager> form_to_save;
+  EXPECT_CALL(delegate_, AddSavePasswordInfoBar(_))
+      .WillOnce(WithArg<0>(SaveToScopedPtr(&form_to_save)));
+
+  // Simulate navigating a sub-frame.
+  content::LoadCommittedDetails details;
+  content::FrameNavigateParams params;
+  manager()->DidNavigateAnyFrame(details, params);
+
+  // Simulate navigating the real page.
+  params.password_form = form;
+  manager()->DidNavigateAnyFrame(details, params);
+
+  // Now the password manager waits for the navigation to complete.
+  manager()->DidStopLoading();
+
+  ASSERT_FALSE(NULL == form_to_save.get());
+  EXPECT_CALL(*store_, AddLogin(FormMatches(form)));
+
+  // Simulate saving the form, as if the info bar was accepted.
+  form_to_save->Save();
+}
+
 TEST_F(PasswordManagerTest, FormSubmitFailedLogin) {
   std::vector<PasswordForm*> result;  // Empty password store.
   EXPECT_CALL(delegate_, FillPasswordForm(_)).Times(Exactly(0));

chrome/browser/profiles/off_the_record_profile_impl.cc

@@ -169,7 +169,7 @@ FilePath OffTheRecordProfileImpl::GetPath() {
   return profile_->GetPath();
 }
 
-bool OffTheRecordProfileImpl::IsOffTheRecord() {
+bool OffTheRecordProfileImpl::IsOffTheRecord() const {
   return true;
 }
 

chrome/browser/profiles/off_the_record_profile_impl.h

@@ -107,7 +107,7 @@ class OffTheRecordProfileImpl : public Profile,
 
   // content::BrowserContext implementation:
   virtual FilePath GetPath() OVERRIDE;
-  virtual bool IsOffTheRecord() OVERRIDE;
+  virtual bool IsOffTheRecord() const OVERRIDE;
   virtual content::DownloadManager* GetDownloadManager() OVERRIDE;
   virtual net::URLRequestContextGetter* GetRequestContext() OVERRIDE;
   virtual net::URLRequestContextGetter* GetRequestContextForRenderProcess(

chrome/browser/profiles/profile.h

@@ -188,9 +188,6 @@ class Profile : public content::BrowserContext {
   // the browser frame.
   virtual std::string GetProfileName() = 0;
 
-  // Return whether this profile is incognito. Default is false.
-  virtual bool IsOffTheRecord() = 0;
-
   // Return the incognito version of this profile. The returned pointer
   // is owned by the receiving profile. If the receiving profile is off the
   // record, the same profile is returned.

chrome/browser/profiles/profile_impl.cc

@@ -651,7 +651,7 @@ FilePath ProfileImpl::GetPath() {
   return path_;
 }
 
-bool ProfileImpl::IsOffTheRecord() {
+bool ProfileImpl::IsOffTheRecord() const {
   return false;
 }
 

chrome/browser/profiles/profile_impl.h

@@ -66,7 +66,7 @@ class ProfileImpl : public Profile,
 
   // Profile implementation:
   virtual std::string GetProfileName() OVERRIDE;
-  virtual bool IsOffTheRecord() OVERRIDE;
+  virtual bool IsOffTheRecord() const OVERRIDE;
   virtual Profile* GetOffTheRecordProfile() OVERRIDE;
   virtual void DestroyOffTheRecordProfile() OVERRIDE;
   virtual bool HasOffTheRecordProfile() OVERRIDE;

chrome/browser/safe_browsing/client_side_detection_host_unittest.cc

@@ -123,7 +123,7 @@ class MockTestingProfile : public TestingProfile {
   MockTestingProfile() {}
   virtual ~MockTestingProfile() {}
 
-  MOCK_METHOD0(IsOffTheRecord, bool());
+  MOCK_CONST_METHOD0(IsOffTheRecord, bool());
 };
 
 class MockBrowserFeatureExtractor : public BrowserFeatureExtractor {

chrome/test/base/testing_profile.cc

@@ -439,7 +439,7 @@ std::string TestingProfile::GetProfileName() {
   return std::string("testing_profile");
 }
 
-bool TestingProfile::IsOffTheRecord() {
+bool TestingProfile::IsOffTheRecord() const {
   return incognito_;
 }
 

chrome/test/base/testing_profile.h

@@ -149,7 +149,7 @@ class TestingProfile : public Profile {
 
   // content::BrowserContext
   virtual FilePath GetPath() OVERRIDE;
-  virtual bool IsOffTheRecord() OVERRIDE;
+  virtual bool IsOffTheRecord() const OVERRIDE;
   virtual content::DownloadManager* GetDownloadManager() OVERRIDE;
   // Returns a testing ContextGetter (if one has been created via
   // CreateRequestContext) or NULL. This is not done on-demand for two reasons:

content/public/browser/browser_context.h

@@ -81,7 +81,7 @@ class CONTENT_EXPORT BrowserContext : public base::SupportsUserData {
 
   // Return whether this context is incognito. Default is false.
   // This doesn't belong here; http://crbug.com/89628
-  virtual bool IsOffTheRecord() = 0;
+  virtual bool IsOffTheRecord() const = 0;
 
   // Returns the DownloadManager associated with this context.
   virtual content::DownloadManager* GetDownloadManager() = 0;

content/shell/shell_browser_context.cc

@@ -117,7 +117,7 @@ FilePath ShellBrowserContext::GetPath() {
   return path_;
 }
 
-bool ShellBrowserContext::IsOffTheRecord()  {
+bool ShellBrowserContext::IsOffTheRecord() const {
   return false;
 }
 

content/shell/shell_browser_context.h

@@ -28,7 +28,7 @@ class ShellBrowserContext : public BrowserContext {
 
   // BrowserContext implementation.
   virtual FilePath GetPath() OVERRIDE;
-  virtual bool IsOffTheRecord() OVERRIDE;
+  virtual bool IsOffTheRecord() const OVERRIDE;
   virtual DownloadManager* GetDownloadManager() OVERRIDE;
   virtual net::URLRequestContextGetter* GetRequestContext() OVERRIDE;
   virtual net::URLRequestContextGetter* GetRequestContextForRenderProcess(

content/test/test_browser_context.cc

@@ -31,7 +31,7 @@ FilePath TestBrowserContext::GetPath() {
   return browser_context_dir_.path();
 }
 
-bool TestBrowserContext::IsOffTheRecord() {
+bool TestBrowserContext::IsOffTheRecord() const {
   return false;
 }
 

content/test/test_browser_context.h

@@ -30,7 +30,7 @@ class TestBrowserContext : public content::BrowserContext {
   void SetSpecialStoragePolicy(quota::SpecialStoragePolicy* policy);
 
   virtual FilePath GetPath() OVERRIDE;
-  virtual bool IsOffTheRecord() OVERRIDE;
+  virtual bool IsOffTheRecord() const OVERRIDE;
   virtual content::DownloadManager* GetDownloadManager() OVERRIDE;
   virtual net::URLRequestContextGetter* GetRequestContext() OVERRIDE;
   virtual net::URLRequestContextGetter* GetRequestContextForRenderProcess(