Only enable password generation if password manager and autofill are both

enabled.

BUG=124112
TEST=Ran unittests


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

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

chrome/browser/autofill/autofill_manager.cc

@@ -34,6 +34,7 @@
 #include "chrome/browser/autofill/phone_number_i18n.h"
 #include "chrome/browser/autofill/select_control_handler.h"
 #include "chrome/browser/infobars/infobar_tab_helper.h"
+#include "chrome/browser/password_manager/password_manager.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
@@ -188,7 +189,7 @@ AutofillManager::AutofillManager(TabContentsWrapper* tab_contents)
       user_did_type_(false),
       user_did_autofill_(false),
       user_did_edit_autofilled_field_(false),
-      password_sync_enabled_(false),
+      password_generation_enabled_(false),
       external_delegate_(NULL) {
   // |personal_data_| is NULL when using test-enabled WebContents.
   personal_data_ = PersonalDataManagerFactory::GetForProfile(
@@ -233,36 +234,43 @@ void AutofillManager::RegisterWithSyncService() {
   }
 }
 
-void AutofillManager::SendPasswordSyncStateToRenderer(
+void AutofillManager::SendPasswordGenerationStateToRenderer(
     content::RenderViewHost* host, bool enabled) {
-  host->Send(new AutofillMsg_PasswordSyncEnabled(host->GetRoutingID(),
-                                                 enabled));
+  host->Send(new AutofillMsg_PasswordGenerationEnabled(host->GetRoutingID(),
+                                                       enabled));
 }
 
-void AutofillManager::UpdatePasswordSyncState(content::RenderViewHost* host,
-                                              bool new_renderer) {
+void AutofillManager::UpdatePasswordGenerationState(
+    content::RenderViewHost* host,
+    bool new_renderer) {
   if (!sync_service_)
     return;
 
+  // Password generation requires sync for passwords and the password manager
+  // to both be enabled.
   syncable::ModelTypeSet sync_set = sync_service_->GetPreferredDataTypes();
-  bool new_password_sync_enabled = (sync_service_->HasSyncSetupCompleted() &&
-                                    sync_set.Has(syncable::PASSWORDS));
-  if (new_password_sync_enabled != password_sync_enabled_ ||
+  bool password_sync_enabled = (sync_service_->HasSyncSetupCompleted() &&
+                                sync_set.Has(syncable::PASSWORDS));
+  bool new_password_generation_enabled =
+      password_sync_enabled &&
+      tab_contents_wrapper_->password_manager()->IsEnabled();
+  if (new_password_generation_enabled != password_generation_enabled_ ||
       new_renderer) {
-    password_sync_enabled_ = new_password_sync_enabled;
-    SendPasswordSyncStateToRenderer(host, password_sync_enabled_);
+    password_generation_enabled_ = new_password_generation_enabled;
+    SendPasswordGenerationStateToRenderer(host, password_generation_enabled_);
   }
 }
 
 void AutofillManager::RenderViewCreated(content::RenderViewHost* host) {
-  UpdatePasswordSyncState(host, true);
+      UpdatePasswordGenerationState(host, true);
 }
 
 void AutofillManager::OnStateChanged() {
   // It is possible for sync state to change during tab contents destruction.
   // In this case, we don't need to update the renderer since it's going away.
   if (web_contents() && web_contents()->GetRenderViewHost()) {
-    UpdatePasswordSyncState(web_contents()->GetRenderViewHost(), false);
+    UpdatePasswordGenerationState(web_contents()->GetRenderViewHost(),
+                                  false);
   }
 }
 
@@ -831,7 +839,7 @@ AutofillManager::AutofillManager(TabContentsWrapper* tab_contents,
       user_did_type_(false),
       user_did_autofill_(false),
       user_did_edit_autofilled_field_(false),
-      password_sync_enabled_(false),
+      password_generation_enabled_(false),
       external_delegate_(NULL) {
   DCHECK(tab_contents);
   RegisterWithSyncService();

chrome/browser/autofill/autofill_manager.h

@@ -115,10 +115,11 @@ class AutofillManager : public content::WebContentsObserver,
   // Reset cache.
   void Reset();
 
-  // Informs the renderers of the current password sync state for use in
-  // password generation. This is a separate function to aid with testing.
-  virtual void SendPasswordSyncStateToRenderer(content::RenderViewHost* host,
-                                               bool enabled);
+  // Informs the renderer of the current password generation state. This is a
+  // separate function to aid with testing.
+  virtual void SendPasswordGenerationStateToRenderer(
+      content::RenderViewHost* host,
+      bool enabled);
 
   // Logs quality metrics for the |submitted_form| and uploads the form data
   // to the crowdsourcing server, if appropriate.
@@ -172,12 +173,12 @@ class AutofillManager : public content::WebContentsObserver,
   // Register as an observer with the sync service.
   void RegisterWithSyncService();
 
-  // Determines what the current state of password sync is, and if it has
-  // changed from password_sync_enabled_. If it has changed or if
+  // Determines what the current state of password generation is, and if it has
+  // changed from |password_generation_enabled_|. If it has changed or if
   // |new_renderer| is true, it notifies the renderer of this change via
-  // SendPasswordSyncStateToRenderer.
-  void UpdatePasswordSyncState(content::RenderViewHost* host,
-                               bool new_renderer);
+  // SendPasswordGenerationStateToRenderer.
+  void UpdatePasswordGenerationState(content::RenderViewHost* host,
+                                     bool new_renderer);
 
   void OnFormsSeen(const std::vector<webkit::forms::FormData>& forms,
                    const base::TimeTicks& timestamp);
@@ -336,10 +337,10 @@ class AutofillManager : public content::WebContentsObserver,
   // When the user first interacted with a potentially fillable form on this
   // page.
   base::TimeTicks initial_interaction_timestamp_;
-  // If password sync is enabled. We cache this value so that we don't
+  // If password generation is enabled. We cache this value so that we don't
   // spam the renderer with messages during startup when the sync state
   // is changing rapidly.
-  bool password_sync_enabled_;
+  bool password_generation_enabled_;
   // The ProfileSyncService associated with this tab. This may be NULL in
   // testing.
   base::WeakPtr<ProfileSyncService> sync_service_;

chrome/browser/autofill/autofill_manager_unittest.cc

@@ -488,7 +488,7 @@ class TestAutofillManager : public AutofillManager {
     submitted_form_signature_ = submitted_form.FormSignature();
   }
 
-  virtual void SendPasswordSyncStateToRenderer(
+  virtual void SendPasswordGenerationStateToRenderer(
       content::RenderViewHost* host, bool enabled) OVERRIDE {
     sent_states_.push_back(enabled);
   }
@@ -583,8 +583,8 @@ class AutofillManagerTest : public TabContentsWrapperTestHarness {
     TabContentsWrapperTestHarness::TearDown();
   }
 
-  void UpdatePasswordSyncState(bool new_renderer) {
-    autofill_manager_->UpdatePasswordSyncState(NULL, new_renderer);
+  void UpdatePasswordGenerationState(bool new_renderer) {
+    autofill_manager_->UpdatePasswordGenerationState(NULL, new_renderer);
   }
 
   void GetAutofillSuggestions(int query_id,
@@ -684,6 +684,10 @@ class AutofillManagerTest : public TabContentsWrapperTestHarness {
   scoped_refptr<TestAutofillManager> autofill_manager_;
   TestPersonalDataManager personal_data_;
 
+  // Used when we want an off the record profile. This will store the original
+  // profile from which the off the record profile is derived.
+  scoped_ptr<Profile> other_browser_context_;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(AutofillManagerTest);
 };
@@ -2887,7 +2891,7 @@ TEST_F(AutofillManagerTest, DeterminePossibleFieldTypesForUpload) {
   FormSubmitted(form);
 }
 
-TEST_F(AutofillManagerTest, UpdatePasswordSyncState) {
+TEST_F(AutofillManagerTest, UpdatePasswordGenerationState) {
   // Allow this test to control what should get synced.
   profile()->GetPrefs()->SetBoolean(prefs::kSyncKeepEverythingSynced, false);
 
@@ -2900,13 +2904,13 @@ TEST_F(AutofillManagerTest, UpdatePasswordSyncState) {
   preferred_set.Put(syncable::PREFERENCES);
   sync_service->ChangePreferredDataTypes(preferred_set);
   syncable::ModelTypeSet new_set = sync_service->GetPreferredDataTypes();
-  UpdatePasswordSyncState(false);
+  UpdatePasswordGenerationState(false);
   EXPECT_EQ(0u, autofill_manager_->GetSentStates().size());
 
   // Now sync passwords.
   preferred_set.Put(syncable::PASSWORDS);
   sync_service->ChangePreferredDataTypes(preferred_set);
-  UpdatePasswordSyncState(false);
+  UpdatePasswordGenerationState(false);
   EXPECT_EQ(1u, autofill_manager_->GetSentStates().size());
   EXPECT_TRUE(autofill_manager_->GetSentStates()[0]);
   autofill_manager_->ClearSentStates();
@@ -2914,19 +2918,26 @@ TEST_F(AutofillManagerTest, UpdatePasswordSyncState) {
   // Add some additional synced state. Nothing should be sent.
   preferred_set.Put(syncable::THEMES);
   sync_service->ChangePreferredDataTypes(preferred_set);
-  UpdatePasswordSyncState(false);
+  UpdatePasswordGenerationState(false);
   EXPECT_EQ(0u, autofill_manager_->GetSentStates().size());
 
-  // Disable syncing. This should be sent.
+  // Disable syncing. This should disable the feature.
   sync_service->DisableForUser();
-  UpdatePasswordSyncState(false);
+  UpdatePasswordGenerationState(false);
   EXPECT_EQ(1u, autofill_manager_->GetSentStates().size());
   EXPECT_FALSE(autofill_manager_->GetSentStates()[0]);
   autofill_manager_->ClearSentStates();
 
+  // Disable password manager by going incognito, and re-enable syncing. The
+  // feature should still be disabled, and nothing will be sent.
+  sync_service->SetSyncSetupCompleted();
+  profile()->set_incognito(true);
+  UpdatePasswordGenerationState(false);
+  EXPECT_EQ(0u, autofill_manager_->GetSentStates().size());
+
   // When a new render_view is created, we send the state even if it's the
   // same.
-  UpdatePasswordSyncState(true);
+  UpdatePasswordGenerationState(true);
   EXPECT_EQ(1u, autofill_manager_->GetSentStates().size());
   EXPECT_FALSE(autofill_manager_->GetSentStates()[0]);
   autofill_manager_->ClearSentStates();

chrome/browser/password_manager/password_manager.cc

@@ -74,6 +74,11 @@ PasswordManager::PasswordManager(WebContents* web_contents,
 PasswordManager::~PasswordManager() {
 }
 
+bool PasswordManager::IsEnabled() const {
+  const Profile* profile = delegate_->GetProfileForPasswordManager();
+  return profile && !profile->IsOffTheRecord() && *password_manager_enabled_;
+}
+
 void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
   if (!IsEnabled())
     return;
@@ -240,8 +245,3 @@ void PasswordManager::Autofill(
       }
   }
 }
-
-bool PasswordManager::IsEnabled() const {
-  const Profile* profile = delegate_->GetProfileForPasswordManager();
-  return profile && !profile->IsOffTheRecord() && *password_manager_enabled_;
-}

chrome/browser/password_manager/password_manager.h

@@ -37,6 +37,9 @@ class PasswordManager : public LoginModel,
                   PasswordManagerDelegate* delegate);
   virtual ~PasswordManager();
 
+  // Is password autofill enabled for the current profile?
+  bool IsEnabled() const;
+
   // Called by a PasswordFormManager when it decides a form can be autofilled
   // on the page.
   void Autofill(const webkit::forms::PasswordForm& form_for_autofill,
@@ -82,9 +85,6 @@ 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.
 
-  // Is password autofill enabled for the current profile?
-  bool IsEnabled() const;
-
   ScopedVector<PasswordFormManager> pending_login_managers_;
 
   // When the user submits a password/credential, this contains the

chrome/common/autofill_messages.h

@@ -118,9 +118,8 @@ IPC_MESSAGE_ROUTED1(AutofillMsg_SetNodeText,
 IPC_MESSAGE_ROUTED1(AutofillMsg_GeneratedPasswordAccepted,
                     string16 /* generated_password */)
 
-// Sends the state of password sync to the renderer. Used to determine if
-// password generation should be enabled.
-IPC_MESSAGE_ROUTED1(AutofillMsg_PasswordSyncEnabled,
+// Tells the renderer if password generation should be enabled.
+IPC_MESSAGE_ROUTED1(AutofillMsg_PasswordGenerationEnabled,
                     bool /* is_enabled */)
 
 // Tells the renderer that the password field has accept the suggestion.

chrome/renderer/autofill/password_generation_manager.cc

@@ -20,12 +20,13 @@ namespace autofill {
 PasswordGenerationManager::PasswordGenerationManager(
     content::RenderView* render_view)
     : content::RenderViewObserver(render_view),
-      sync_enabled_(false) {}
+      enabled_(false) {}
 PasswordGenerationManager::~PasswordGenerationManager() {}
 
 void PasswordGenerationManager::DidFinishDocumentLoad(WebKit::WebFrame* frame) {
-  // We don't want to generate passwords if password sync isn't enabled.
-  if (!sync_enabled_)
+  // We don't want to generate passwords if the browser won't store or sync
+  // them.
+  if (!enabled_)
     return;
 
   if (!ShouldAnalyzeFrame(*frame))
@@ -89,7 +90,7 @@ bool PasswordGenerationManager::OnMessageReceived(const IPC::Message& message) {
   IPC_BEGIN_MESSAGE_MAP(PasswordGenerationManager, message)
     IPC_MESSAGE_HANDLER(AutofillMsg_GeneratedPasswordAccepted,
                         OnPasswordAccepted)
-    IPC_MESSAGE_HANDLER(AutofillMsg_PasswordSyncEnabled,
+    IPC_MESSAGE_HANDLER(AutofillMsg_PasswordGenerationEnabled,
                         OnPasswordSyncEnabled)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
@@ -106,7 +107,7 @@ void PasswordGenerationManager::OnPasswordAccepted(const string16& password) {
 }
 
 void PasswordGenerationManager::OnPasswordSyncEnabled(bool enabled) {
-  sync_enabled_ = enabled;
+  enabled_ = enabled;
 }
 
 }  // namespace autofill

chrome/renderer/autofill/password_generation_manager.h

@@ -42,9 +42,9 @@ class PasswordGenerationManager : public content::RenderViewObserver {
   void OnPasswordAccepted(const string16& password);
   void OnPasswordSyncEnabled(bool enabled);
 
-  // True if password sync is enabled for the profile associated with this
+  // True if the browser believes that generating passwords is okay for this
   // renderer.
-  bool sync_enabled_;
+  bool enabled_;
 
   std::pair<WebKit::WebInputElement,
             std::vector<WebKit::WebInputElement> > account_creation_elements_;

chrome/renderer/autofill/password_generation_manager_browsertest.cc

@@ -113,7 +113,7 @@ TEST_F(PasswordGenerationManagerTest, DetectionTest) {
   EXPECT_EQ(0u, generation_manager_->messages().size());
 
   // Pretend like sync was enabled.
-  AutofillMsg_PasswordSyncEnabled msg(0, true);
+  AutofillMsg_PasswordGenerationEnabled msg(0, true);
   generation_manager_->OnMessageReceived(msg);
 
   // Now we will send a message once the first password feld is focused.
@@ -135,7 +135,7 @@ TEST_F(PasswordGenerationManagerTest, DetectionTest) {
 
 TEST_F(PasswordGenerationManagerTest, FillTest) {
   // Make sure that we are enabled before loading HTML.
-  AutofillMsg_PasswordSyncEnabled enabled_msg(0, true);
+  AutofillMsg_PasswordGenerationEnabled enabled_msg(0, true);
   generation_manager_->OnMessageReceived(enabled_msg);
   LoadHTML(kAccountCreationFormHTML);