[Password Manager] Remove Profile access Password[Form]Manager

Access now happens through the PasswordManagerDriver or PasswordManagerDelegate.

BUG=334670,335107

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

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

chrome/browser/password_manager/chrome_password_manager_client.cc

@@ -11,6 +11,7 @@
 #include "chrome/browser/password_manager/password_form_manager.h"
 #include "chrome/browser/password_manager/password_manager.h"
 #include "chrome/browser/password_manager/password_manager_util.h"
+#include "chrome/browser/password_manager/password_store_factory.h"
 #include "chrome/browser/password_manager/save_password_infobar_delegate.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/profile_sync_service.h"
@@ -70,7 +71,7 @@ void ChromePasswordManagerClient::PromptUserToSavePassword(
     std::string uma_histogram_suffix(
         password_manager_metrics_util::GroupIdToString(
             password_manager_metrics_util::MonitoredDomainGroupId(
-                form_to_save->realm(), GetProfile()->GetPrefs())));
+                form_to_save->realm(), GetPrefs())));
     SavePasswordInfoBarDelegate::Create(
         web_contents_, form_to_save, uma_histogram_suffix);
   }
@@ -114,6 +115,14 @@ PrefService* ChromePasswordManagerClient::GetPrefs() {
   return GetProfile()->GetPrefs();
 }
 
+PasswordStore* ChromePasswordManagerClient::GetPasswordStore() {
+  // Always use EXPLICIT_ACCESS as the password manager checks IsOffTheRecord
+  // itself when it shouldn't access the PasswordStore.
+  // TODO(gcasto): Is is safe to change this to Profile::IMPLICIT_ACCESS?
+  return PasswordStoreFactory::GetForProfile(GetProfile(),
+                                             Profile::EXPLICIT_ACCESS).get();
+}
+
 PasswordManagerDriver* ChromePasswordManagerClient::GetDriver() {
   return &driver_;
 }

chrome/browser/password_manager/chrome_password_manager_client.h

@@ -13,6 +13,7 @@
 
 class PasswordGenerationManager;
 class PasswordManager;
+class Profile;
 
 namespace content {
 class WebContents;
@@ -32,8 +33,8 @@ class ChromePasswordManagerClient
       const autofill::PasswordFormMap& best_matches) const OVERRIDE;
   virtual void AuthenticateAutofillAndFillForm(
       scoped_ptr<autofill::PasswordFormFillData> fill_data) OVERRIDE;
-  virtual Profile* GetProfile() OVERRIDE;
   virtual PrefService* GetPrefs() OVERRIDE;
+  virtual PasswordStore* GetPasswordStore() OVERRIDE;
   virtual PasswordManagerDriver* GetDriver() OVERRIDE;
   virtual base::FieldTrial::Probability GetProbabilityForExperiment(
       const std::string& experiment_name) OVERRIDE;
@@ -45,8 +46,7 @@ class ChromePasswordManagerClient
       content::WebContents* contents);
 
   // Convenience method to allow //chrome code easy access to a
-  // PasswordGenerationManager
-  // from a WebContents instance.
+  // PasswordGenerationManager from a WebContents instance.
   static PasswordGenerationManager* GetGenerationManagerFromWebContents(
       content::WebContents* contents);
 
@@ -59,6 +59,8 @@ class ChromePasswordManagerClient
   // not supported, this will be triggered directly.
   void CommitFillPasswordForm(autofill::PasswordFormFillData* fill_data);
 
+  Profile* GetProfile();
+
   content::WebContents* web_contents_;
   ContentPasswordManagerDriver driver_;
 

chrome/browser/password_manager/password_form_manager.cc

@@ -10,8 +10,9 @@
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "chrome/browser/password_manager/password_manager.h"
+#include "chrome/browser/password_manager/password_manager_client.h"
+#include "chrome/browser/password_manager/password_manager_driver.h"
 #include "chrome/browser/password_manager/password_store_factory.h"
-#include "chrome/browser/profiles/profile.h"
 #include "components/autofill/core/browser/autofill_manager.h"
 #include "components/autofill/core/browser/form_structure.h"
 #include "components/autofill/core/browser/validation.h"
@@ -23,8 +24,8 @@ using autofill::PasswordForm;
 using autofill::PasswordFormMap;
 using base::Time;
 
-PasswordFormManager::PasswordFormManager(Profile* profile,
-                                         PasswordManager* password_manager,
+PasswordFormManager::PasswordFormManager(PasswordManager* password_manager,
+                                         PasswordManagerClient* client,
                                          PasswordManagerDriver* driver,
                                          const PasswordForm& observed_form,
                                          bool ssl_valid)
@@ -35,12 +36,11 @@ PasswordFormManager::PasswordFormManager(Profile* profile,
       password_manager_(password_manager),
       preferred_match_(NULL),
       state_(PRE_MATCHING_PHASE),
-      profile_(profile),
+      client_(client),
       driver_(driver),
       manager_action_(kManagerActionNone),
       user_action_(kUserActionNone),
       submit_result_(kSubmitResultNotSubmitted) {
-  DCHECK(profile_);
   if (observed_form_.origin.is_valid())
     base::SplitString(observed_form_.origin.path(), '/', &form_path_tokens_);
   observed_form_.ssl_valid = ssl_valid;
@@ -127,8 +127,7 @@ void PasswordFormManager::PermanentlyBlacklist() {
   int num_passwords_deleted = 0;
   if (!best_matches_.empty()) {
     PasswordFormMap::const_iterator iter;
-    PasswordStore* password_store = PasswordStoreFactory::GetForProfile(
-        profile_, Profile::EXPLICIT_ACCESS).get();
+    PasswordStore* password_store = client_->GetPasswordStore();
     if (!password_store) {
       NOTREACHED();
       return;
@@ -249,7 +248,7 @@ void PasswordFormManager::ProvisionallySave(
 
 void PasswordFormManager::Save() {
   DCHECK_EQ(state_, POST_MATCHING_PHASE);
-  DCHECK(!profile_->IsOffTheRecord());
+  DCHECK(!driver_->IsOffTheRecord());
 
   if (IsNewLogin())
     SaveAsNewLogin(true);
@@ -261,8 +260,7 @@ void PasswordFormManager::FetchMatchingLoginsFromPasswordStore(
     PasswordStore::AuthorizationPromptPolicy prompt_policy) {
   DCHECK_EQ(state_, PRE_MATCHING_PHASE);
   state_ = MATCHING_PHASE;
-  PasswordStore* password_store = PasswordStoreFactory::GetForProfile(
-      profile_, Profile::EXPLICIT_ACCESS).get();
+  PasswordStore* password_store = client_->GetPasswordStore();
   if (!password_store) {
     NOTREACHED();
     return;
@@ -379,7 +377,7 @@ void PasswordFormManager::OnRequestDone(
   // (1) we are in Incognito mode, (2) the ACTION paths don't match,
   // or (3) if it matched using public suffix domain matching.
   bool wait_for_username =
-      profile_->IsOffTheRecord() ||
+      driver_->IsOffTheRecord() ||
       observed_form_.action.GetWithEmptyPath() !=
           preferred_match_->action.GetWithEmptyPath() ||
           preferred_match_->IsPublicSuffixMatch();
@@ -428,10 +426,9 @@ void PasswordFormManager::SaveAsNewLogin(bool reset_preferred_login) {
   // new_form contains the same basic data as observed_form_ (because its the
   // same form), but with the newly added credentials.
 
-  DCHECK(!profile_->IsOffTheRecord());
+  DCHECK(!driver_->IsOffTheRecord());
 
-  PasswordStore* password_store = PasswordStoreFactory::GetForProfile(
-      profile_, Profile::IMPLICIT_ACCESS).get();
+  PasswordStore* password_store = client_->GetPasswordStore();
   if (!password_store) {
     NOTREACHED();
     return;
@@ -486,10 +483,9 @@ void PasswordFormManager::UpdateLogin() {
   // username, or the user selected one of the non-preferred matches,
   // thus requiring a swap of preferred bits.
   DCHECK(!IsNewLogin() && pending_credentials_.preferred);
-  DCHECK(!profile_->IsOffTheRecord());
+  DCHECK(!driver_->IsOffTheRecord());
 
-  PasswordStore* password_store = PasswordStoreFactory::GetForProfile(
-      profile_, Profile::IMPLICIT_ACCESS).get();
+  PasswordStore* password_store = client_->GetPasswordStore();
   if (!password_store) {
     NOTREACHED();
     return;

chrome/browser/password_manager/password_form_manager.h

@@ -21,20 +21,18 @@ class WebContents;
 }  // namespace content
 
 class PasswordManager;
-class Profile;
+class PasswordManagerClient;
 
 // Per-password-form-{on-page, dialog} class responsible for interactions
 // between a given form, the per-tab PasswordManager, and the PasswordStore.
 class PasswordFormManager : public PasswordStoreConsumer {
  public:
-  // profile contains the link to the PasswordStore and whether we're off
-  //           the record
-  // password_manager owns this object
-  // form_on_page is the form that may be submitted and could need login data.
-  // ssl_valid represents the security of the page containing observed_form,
+  // |password_manager| owns this object
+  // |form_on_page| is the form that may be submitted and could need login data.
+  // |ssl_valid| represents the security of the page containing observed_form,
   //           used to filter login results from database.
-  PasswordFormManager(Profile* profile,
-                      PasswordManager* password_manager,
+  PasswordFormManager(PasswordManager* password_manager,
+                      PasswordManagerClient* client,
                       PasswordManagerDriver* driver,
                       const autofill::PasswordForm& observed_form,
                       bool ssl_valid);
@@ -299,8 +297,8 @@ class PasswordFormManager : public PasswordStoreConsumer {
   // when we actually haven't.
   PasswordFormManagerState state_;
 
-  // The profile from which we get the PasswordStore.
-  Profile* profile_;
+  // The client which implements embedder-specific PasswordManager operations.
+  PasswordManagerClient* client_;
 
   // The driver which implements platform-specific PasswordManager operations.
   PasswordManagerDriver* driver_;

chrome/browser/password_manager/password_manager.cc

@@ -97,7 +97,7 @@ void PasswordManager::SetFormHasGeneratedPassword(const PasswordForm& form) {
   bool ssl_valid = (form.origin.SchemeIsSecure() &&
                     !driver_->DidLastPageLoadEncounterSSLErrors());
   PasswordFormManager* manager = new PasswordFormManager(
-      client_->GetProfile(), this, driver_, form, ssl_valid);
+      this, client_, driver_, form, ssl_valid);
   pending_login_managers_.push_back(manager);
   manager->SetHasGeneratedPassword();
   // TODO(gcasto): Add UMA stats to track this.
@@ -256,7 +256,7 @@ void PasswordManager::OnPasswordFormsParsed(
 
     bool ssl_valid = iter->origin.SchemeIsSecure() && !had_ssl_error;
     PasswordFormManager* manager = new PasswordFormManager(
-        client_->GetProfile(), this, driver_, *iter, ssl_valid);
+        this, client_, driver_, *iter, ssl_valid);
     pending_login_managers_.push_back(manager);
 
     // Avoid prompting the user for access to a password if they don't have

chrome/browser/password_manager/password_manager_client.h

@@ -11,12 +11,11 @@
 
 class PasswordFormManager;
 class PasswordManagerDriver;
+class PasswordStore;
 class PrefService;
-class Profile;
 
-// An abstraction of operations in the external environment (WebContents)
-// that the PasswordManager depends on.  This allows for more targeted
-// unit testing.
+// An abstraction of operations that depend on the embedders (e.g. Chrome)
+// environment.
 class PasswordManagerClient {
  public:
   PasswordManagerClient() {}
@@ -36,13 +35,12 @@ class PasswordManagerClient {
   virtual void AuthenticateAutofillAndFillForm(
       scoped_ptr<autofill::PasswordFormFillData> fill_data) = 0;
 
-  // Get the profile for which we are managing passwords.
-  // TODO(gcasto): Remove this function. crbug.com/335107.
-  virtual Profile* GetProfile() = 0;
-
   // Gets prefs associated with this embedder.
   virtual PrefService* GetPrefs() = 0;
 
+  // Returns the PasswordStore associated with this instance.
+  virtual PasswordStore* GetPasswordStore() = 0;
+
   // Returns the PasswordManagerDriver instance associated with this instance.
   virtual PasswordManagerDriver* GetDriver() = 0;
 

chrome/browser/password_manager/password_manager_unittest.cc

@@ -42,6 +42,7 @@ class MockPasswordManagerClient : public PasswordManagerClient {
  public:
   MOCK_METHOD1(PromptUserToSavePassword, void(PasswordFormManager*));
   MOCK_METHOD0(GetProfile, Profile*());
+  MOCK_METHOD0(GetPasswordStore, PasswordStore*());
   MOCK_METHOD0(GetPrefs, PrefService*());
   MOCK_METHOD0(GetDriver, PasswordManagerDriver*());
   MOCK_METHOD1(GetProbabilityForExperiment,
@@ -103,7 +104,7 @@ class PasswordManagerTest : public ChromeRenderViewHostTestHarness {
         factory->GetForProfile(profile(), Profile::IMPLICIT_ACCESS));
     store_ = static_cast<MockPasswordStore*>(store_temp.get());
 
-    EXPECT_CALL(client_, GetProfile()).WillRepeatedly(Return(profile()));
+    EXPECT_CALL(client_, GetPasswordStore()).WillRepeatedly(Return(store_));
     EXPECT_CALL(client_, GetPrefs())
         .WillRepeatedly(Return(profile()->GetTestingPrefService()));
     EXPECT_CALL(client_, GetDriver()).WillRepeatedly(Return(&driver_));