Make password generation switched by a preference in chrome settings rather than a command line flag.

BUG=
TEST= AutofillManager_Unittest,  PasswordgenerationManager_Browsertest


Review URL: https://chromiumcodereview.appspot.com/10222017

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

16 files changed, 148 insertions, 20 deletions

diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd
index fccd8bb..8fdd48c 100644
--- a/chrome/app/generated_resources.grd
+++ b/chrome/app/generated_resources.grd
@@ -9210,6 +9210,9 @@ experiment id: "<ph name="EXPERIMENT_ID">$5<ex>ar1</ex></ph>"
       <message name="IDS_OPTIONS_SAFEBROWSING_ENABLEPROTECTION" desc="The label of the 'Enable phishing and malware protection' checkbox">
         Enable phishing and malware protection
       </message>
+      <message name="IDS_OPTIONS_PASSWORD_GENERATION_ENABLED_LABEL" desc="The label of the 'Enable automatic password generation' checkbox">
+        Enable automatic password generation
+      </message>
       <if expr="pp_ifdef('android')">
         <message name="IDS_OPTIONS_SSL_GROUP_DESCRIPTION" desc="Mobile: The description of the 'SSL Connections' group">
           Mobile device-wide SSL settings:
diff --git a/chrome/browser/autofill/autofill_external_delegate_unittest.cc b/chrome/browser/autofill/autofill_external_delegate_unittest.cc
index 7c6131a..ce23a58 100644
--- a/chrome/browser/autofill/autofill_external_delegate_unittest.cc
+++ b/chrome/browser/autofill/autofill_external_delegate_unittest.cc
@@ -57,7 +57,9 @@ class MockAutofillExternalDelegate : public TestAutofillExternalDelegate {
 class MockAutofillManager : public AutofillManager {
  public:
   explicit MockAutofillManager(TabContentsWrapper* tab_contents)
-      : AutofillManager(tab_contents) {}
+      // Force to use the constructor designated for unit test, but we don't
+      // really need personal_data in this test so we pass a NULL pointer.
+      : AutofillManager(tab_contents, NULL) {}
 
   MOCK_METHOD4(OnFillAutofillFormData,
                void(int query_id,
diff --git a/chrome/browser/autofill/autofill_manager.cc b/chrome/browser/autofill/autofill_manager.cc
index 74677e8..146c429 100644
--- a/chrome/browser/autofill/autofill_manager.cc
+++ b/chrome/browser/autofill/autofill_manager.cc
@@ -195,6 +195,8 @@ AutofillManager::AutofillManager(TabContentsWrapper* tab_contents)
   personal_data_ = PersonalDataManagerFactory::GetForProfile(
       tab_contents->profile()->GetOriginalProfile());
   RegisterWithSyncService();
+  registrar_.Init(tab_contents->profile()->GetPrefs());
+  registrar_.Add(prefs::kPasswordGenerationEnabled, this);
 }
 
 AutofillManager::~AutofillManager() {
@@ -207,6 +209,9 @@ void AutofillManager::RegisterUserPrefs(PrefService* prefs) {
   prefs->RegisterBooleanPref(prefs::kAutofillEnabled,
                              true,
                              PrefService::SYNCABLE_PREF);
+  prefs->RegisterBooleanPref(prefs::kPasswordGenerationEnabled,
+                             true,
+                             PrefService::SYNCABLE_PREF);
 #if defined(OS_MACOSX)
   prefs->RegisterBooleanPref(prefs::kAutofillAuxiliaryProfilesEnabled,
                              true,
@@ -240,20 +245,34 @@ void AutofillManager::SendPasswordGenerationStateToRenderer(
                                                        enabled));
 }
 
+// In order for password generation to be enabled, we need to make sure:
+// (1) Password sync is enabled,
+// (2) Password manager is enabled, and
+// (3) Password generation preference check box is checked.
 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 password_sync_enabled = (sync_service_->HasSyncSetupCompleted() &&
-                                sync_set.Has(syncable::PASSWORDS));
+  bool password_sync_enabled =
+      sync_service_->HasSyncSetupCompleted() &&
+      sync_set.Has(syncable::PASSWORDS);
+
+  bool password_manager_enabled =
+      tab_contents_wrapper_->password_manager()->IsSavingEnabled();
+
+  Profile* profile = Profile::FromBrowserContext(
+      web_contents()->GetBrowserContext());
+  bool preference_checked =
+      profile->GetPrefs()->GetBoolean(prefs::kPasswordGenerationEnabled);
+
   bool new_password_generation_enabled =
       password_sync_enabled &&
-      tab_contents_wrapper_->password_manager()->IsSavingEnabled();
+      password_manager_enabled &&
+      preference_checked;
+
   if (new_password_generation_enabled != password_generation_enabled_ ||
       new_renderer) {
     password_generation_enabled_ = new_password_generation_enabled;
@@ -262,7 +281,18 @@ void AutofillManager::UpdatePasswordGenerationState(
 }
 
 void AutofillManager::RenderViewCreated(content::RenderViewHost* host) {
-      UpdatePasswordGenerationState(host, true);
+  UpdatePasswordGenerationState(host, true);
+}
+
+void AutofillManager::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  DCHECK_EQ(chrome::NOTIFICATION_PREF_CHANGED, type);
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  std::string* pref = content::Details<std::string>(details).ptr();
+  DCHECK(*pref == prefs::kPasswordGenerationEnabled);
+  UpdatePasswordGenerationState(web_contents()->GetRenderViewHost(), false);
 }
 
 void AutofillManager::OnStateChanged() {
@@ -867,6 +897,7 @@ AutofillManager::AutofillManager(TabContentsWrapper* tab_contents,
       external_delegate_(NULL) {
   DCHECK(tab_contents);
   RegisterWithSyncService();
+  // Test code doesn't need registrar_.
 }
 
 void AutofillManager::set_metric_logger(const AutofillMetrics* metric_logger) {
diff --git a/chrome/browser/autofill/autofill_manager.h b/chrome/browser/autofill/autofill_manager.h
index 9566216..b483097 100644
--- a/chrome/browser/autofill/autofill_manager.h
+++ b/chrome/browser/autofill/autofill_manager.h
@@ -23,7 +23,9 @@
 #include "chrome/browser/autofill/autofill_download.h"
 #include "chrome/browser/autofill/field_types.h"
 #include "chrome/browser/autofill/form_structure.h"
+#include "chrome/browser/prefs/pref_change_registrar.h"
 #include "chrome/browser/sync/profile_sync_service_observer.h"
+#include "content/public/browser/notification_observer.h"
 #include "content/public/browser/web_contents_observer.h"
 
 class AutofillExternalDelegate;
@@ -60,7 +62,8 @@ struct PasswordFormFillData;
 
 // Manages saving and restoring the user's personal information entered into web
 // forms.
-class AutofillManager : public content::WebContentsObserver,
+class AutofillManager : public content::NotificationObserver,
+                        public content::WebContentsObserver,
                         public AutofillDownloadManager::Observer,
                         public ProfileSyncServiceObserver,
                         public base::RefCounted<AutofillManager> {
@@ -177,6 +180,11 @@ class AutofillManager : public content::WebContentsObserver,
   // Register as an observer with the sync service.
   void RegisterWithSyncService();
 
+  // content::NotificationObserver override
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
+
   // 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
@@ -348,6 +356,8 @@ class AutofillManager : public content::WebContentsObserver,
   // The ProfileSyncService associated with this tab. This may be NULL in
   // testing.
   base::WeakPtr<ProfileSyncService> sync_service_;
+  // Listens for changes to the 'enabled' state for password generation.
+  PrefChangeRegistrar registrar_;
 
   // Our copy of the form data.
   ScopedVector<FormStructure> form_structures_;
diff --git a/chrome/browser/autofill/autofill_manager_unittest.cc b/chrome/browser/autofill/autofill_manager_unittest.cc
index 2f3ae81..2ec5081 100644
--- a/chrome/browser/autofill/autofill_manager_unittest.cc
+++ b/chrome/browser/autofill/autofill_manager_unittest.cc
@@ -2921,9 +2921,12 @@ TEST_F(AutofillManagerTest, DeterminePossibleFieldTypesForUpload) {
   FormSubmitted(form);
 }
 
-TEST_F(AutofillManagerTest, UpdatePasswordGenerationState) {
+TEST_F(AutofillManagerTest, UpdatePasswordSyncState) {
   // Allow this test to control what should get synced.
   profile()->GetPrefs()->SetBoolean(prefs::kSyncKeepEverythingSynced, false);
+  // Always set password generation enabled check box so we can test the
+  // behavior of password sync.
+  profile()->GetPrefs()->SetBoolean(prefs::kPasswordGenerationEnabled, true);
 
   // Sync some things, but not passwords. Shouldn't send anything since
   // password generation is disabled by default.
@@ -2973,6 +2976,48 @@ TEST_F(AutofillManagerTest, UpdatePasswordGenerationState) {
   autofill_manager_->ClearSentStates();
 }
 
+TEST_F(AutofillManagerTest, UpdatePasswordGenerationState) {
+  // Always set password sync enabled so we can test the behavior of password
+  // generation.
+  profile()->GetPrefs()->SetBoolean(prefs::kSyncKeepEverythingSynced, false);
+  ProfileSyncService* sync_service = GetProfileSyncService();
+  sync_service->SetSyncSetupCompleted();
+  syncable::ModelTypeSet preferred_set;
+  preferred_set.Put(syncable::PASSWORDS);
+  sync_service->ChangePreferredDataTypes(preferred_set);
+
+  // Enabled state remains false, should not sent.
+  profile()->GetPrefs()->SetBoolean(prefs::kPasswordGenerationEnabled, false);
+  UpdatePasswordGenerationState(false);
+  EXPECT_EQ(0u, autofill_manager_->GetSentStates().size());
+
+  // Enabled state from false to true, should sent true.
+  profile()->GetPrefs()->SetBoolean(prefs::kPasswordGenerationEnabled, true);
+  UpdatePasswordGenerationState(false);
+  EXPECT_EQ(1u, autofill_manager_->GetSentStates().size());
+  EXPECT_TRUE(autofill_manager_->GetSentStates()[0]);
+  autofill_manager_->ClearSentStates();
+
+  // Enabled states remains true, should not sent.
+  profile()->GetPrefs()->SetBoolean(prefs::kPasswordGenerationEnabled, true);
+  UpdatePasswordGenerationState(false);
+  EXPECT_EQ(0u, autofill_manager_->GetSentStates().size());
+
+  // Enabled states from true to false, should sent false.
+  profile()->GetPrefs()->SetBoolean(prefs::kPasswordGenerationEnabled, false);
+  UpdatePasswordGenerationState(false);
+  EXPECT_EQ(1u, autofill_manager_->GetSentStates().size());
+  EXPECT_FALSE(autofill_manager_->GetSentStates()[0]);
+  autofill_manager_->ClearSentStates();
+
+  // When a new render_view is created, we send the state even if it's the
+  // same.
+  UpdatePasswordGenerationState(true);
+  EXPECT_EQ(1u, autofill_manager_->GetSentStates().size());
+  EXPECT_FALSE(autofill_manager_->GetSentStates()[0]);
+  autofill_manager_->ClearSentStates();
+}
+
 TEST_F(AutofillManagerTest, RemoveProfile) {
   // Add and remove an Autofill profile.
   AutofillProfile* profile = new AutofillProfile;
diff --git a/chrome/browser/resources/options2/browser_options.html b/chrome/browser/resources/options2/browser_options.html
index e800e47..a5d86b4 100644
--- a/chrome/browser/resources/options2/browser_options.html
+++ b/chrome/browser/resources/options2/browser_options.html
@@ -310,6 +310,13 @@
           i18n-content="managePasswords"
           pref="profile.password_manager_enabled"></button>
     </div>
+    <div class="checkbox" id="password-generation-checkbox">
+      <label>
+        <input id="password-generation-enabled" pref="password_generation.enabled"
+            metric="Options_PasswordGenerationCheckbox" type="checkbox">
+        <span i18n-content="passwordGenerationEnabledDescription"></span>
+      </label>
+    </div>
 <if expr="is_macosx">
     <div id="mac-passwords-warning" i18n-content="macPasswordsWarning" hidden>
     </div>
diff --git a/chrome/browser/resources/options2/browser_options.js b/chrome/browser/resources/options2/browser_options.js
index 35f7510..247ee65 100644
--- a/chrome/browser/resources/options2/browser_options.js
+++ b/chrome/browser/resources/options2/browser_options.js
@@ -1050,6 +1050,17 @@ cr.define('options', function() {
     },
 
     /**
+     * Set the visibility of the password generation checkbox.
+     * @private
+     */
+    setPasswordGenerationSettingVisibility_: function(visible) {
+      if (visible)
+        $('password-generation-checkbox').style.display = 'block';
+      else
+        $('password-generation-checkbox').style.display = 'none';
+    },
+
+    /**
      * Set the font size selected item.
      * @private
      */
@@ -1349,6 +1360,7 @@ cr.define('options', function() {
     'setInstantFieldTrialStatus',
     'setMetricsReportingCheckboxState',
     'setMetricsReportingSettingVisibility',
+    'setPasswordGenerationSettingVisibility',
     'setProfilesInfo',
     'setScreenMagnifierCheckboxState',
     'setSpokenFeedbackCheckboxState',
diff --git a/chrome/browser/ui/webui/options2/browser_options_handler2.cc b/chrome/browser/ui/webui/options2/browser_options_handler2.cc
index df7886f..6f645ec 100644
--- a/chrome/browser/ui/webui/options2/browser_options_handler2.cc
+++ b/chrome/browser/ui/webui/options2/browser_options_handler2.cc
@@ -214,6 +214,8 @@ void BrowserOptionsHandler::GetLocalizedValues(DictionaryValue* values) {
     { "passwordsAndAutofillGroupName",
       IDS_OPTIONS_PASSWORDS_AND_FORMS_GROUP_NAME },
     { "passwordManagerEnabled", IDS_OPTIONS_PASSWORD_MANAGER_ENABLE },
+    { "passwordGenerationEnabledDescription",
+      IDS_OPTIONS_PASSWORD_GENERATION_ENABLED_LABEL },
     { "privacyClearDataButton", IDS_OPTIONS_PRIVACY_CLEAR_DATA_BUTTON },
     { "privacyContentSettingsButton",
       IDS_OPTIONS_PRIVACY_CONTENT_SETTINGS_BUTTON },
@@ -612,6 +614,7 @@ void BrowserOptionsHandler::InitializePage() {
 
   SetupMetricsReportingCheckbox();
   SetupMetricsReportingSettingVisibility();
+  SetupPasswordGenerationSettingVisibility();
   SetupFontSizeSelector();
   SetupPageZoomSelector();
   SetupAutoOpenFileTypes();
@@ -1328,6 +1331,14 @@ void BrowserOptionsHandler::SetupMetricsReportingSettingVisibility() {
 #endif
 }
 
+void BrowserOptionsHandler::SetupPasswordGenerationSettingVisibility() {
+  base::FundamentalValue visible(
+      CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnablePasswordGeneration));
+  web_ui()->CallJavascriptFunction(
+      "BrowserOptions.setPasswordGenerationSettingVisibility", visible);
+}
+
 void BrowserOptionsHandler::SetupFontSizeSelector() {
   // We're only interested in integer values, so convert to int.
   base::FundamentalValue font_size(default_font_size_.GetValue());
diff --git a/chrome/browser/ui/webui/options2/browser_options_handler2.h b/chrome/browser/ui/webui/options2/browser_options_handler2.h
index fe5510c..2315fea 100644
--- a/chrome/browser/ui/webui/options2/browser_options_handler2.h
+++ b/chrome/browser/ui/webui/options2/browser_options_handler2.h
@@ -251,6 +251,9 @@ class BrowserOptionsHandler
   // Setup the visibility for the metrics reporting setting.
   void SetupMetricsReportingSettingVisibility();
 
+  // Setup the visibility for the password generation setting.
+  void SetupPasswordGenerationSettingVisibility();
+
   // Setup the font size selector control.
   void SetupFontSizeSelector();
 
diff --git a/chrome/common/autofill_messages.h b/chrome/common/autofill_messages.h
index 8c008d0..fa17176 100644
--- a/chrome/common/autofill_messages.h
+++ b/chrome/common/autofill_messages.h
@@ -118,7 +118,7 @@ IPC_MESSAGE_ROUTED1(AutofillMsg_SetNodeText,
 IPC_MESSAGE_ROUTED1(AutofillMsg_GeneratedPasswordAccepted,
                     string16 /* generated_password */)
 
-// Tells the renderer if password generation should be enabled.
+// Tells the renderer whether password generation is enabled.
 IPC_MESSAGE_ROUTED1(AutofillMsg_PasswordGenerationEnabled,
                     bool /* is_enabled */)
 
diff --git a/chrome/common/pref_names.cc b/chrome/common/pref_names.cc
index 100edf8..f564a87 100644
--- a/chrome/common/pref_names.cc
+++ b/chrome/common/pref_names.cc
@@ -267,6 +267,9 @@ const char kPasswordManagerEnabled[] = "profile.password_manager_enabled";
 const char kPasswordManagerAllowShowPasswords[] =
     "profile.password_manager_allow_show_passwords";
 
+// Boolean that is true when password generation is enabled.
+const char kPasswordGenerationEnabled[] = "password_generation.enabled";
+
 // Booleans identifying whether normal and reverse auto-logins are enabled.
 const char kAutologinEnabled[] = "autologin.enabled";
 const char kReverseAutologinEnabled[] = "reverse_autologin.enabled";
diff --git a/chrome/common/pref_names.h b/chrome/common/pref_names.h
index 84dd564..10ab3f7 100644
--- a/chrome/common/pref_names.h
+++ b/chrome/common/pref_names.h
@@ -119,6 +119,7 @@ extern const char kWebKitAllowDisplayingInsecureContent[];
 extern const char kWebKitAllowRunningInsecureContent[];
 extern const char kPasswordManagerEnabled[];
 extern const char kPasswordManagerAllowShowPasswords[];
+extern const char kPasswordGenerationEnabled[];
 extern const char kAutologinEnabled[];
 extern const char kReverseAutologinEnabled[];
 extern const char kSafeBrowsingEnabled[];
diff --git a/chrome/renderer/autofill/password_generation_manager.cc b/chrome/renderer/autofill/password_generation_manager.cc
index 687c035..65d9603 100644
--- a/chrome/renderer/autofill/password_generation_manager.cc
+++ b/chrome/renderer/autofill/password_generation_manager.cc
@@ -91,7 +91,7 @@ bool PasswordGenerationManager::OnMessageReceived(const IPC::Message& message) {
     IPC_MESSAGE_HANDLER(AutofillMsg_GeneratedPasswordAccepted,
                         OnPasswordAccepted)
     IPC_MESSAGE_HANDLER(AutofillMsg_PasswordGenerationEnabled,
-                        OnPasswordSyncEnabled)
+                        OnPasswordGenerationEnabled)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
@@ -106,7 +106,7 @@ void PasswordGenerationManager::OnPasswordAccepted(const string16& password) {
   }
 }
 
-void PasswordGenerationManager::OnPasswordSyncEnabled(bool enabled) {
+void PasswordGenerationManager::OnPasswordGenerationEnabled(bool enabled) {
   enabled_ = enabled;
 }
 
diff --git a/chrome/renderer/autofill/password_generation_manager.h b/chrome/renderer/autofill/password_generation_manager.h
index 85a9bb5..8aa656f 100644
--- a/chrome/renderer/autofill/password_generation_manager.h
+++ b/chrome/renderer/autofill/password_generation_manager.h
@@ -40,10 +40,10 @@ class PasswordGenerationManager : public content::RenderViewObserver {
 
   // Message handlers.
   void OnPasswordAccepted(const string16& password);
-  void OnPasswordSyncEnabled(bool enabled);
+  void OnPasswordGenerationEnabled(bool enabled);
 
-  // True if the browser believes that generating passwords is okay for this
-  // renderer.
+  // True if password generation is enabled for the profile associated
+  // with this renderer.
   bool enabled_;
 
   std::pair<WebKit::WebInputElement,
diff --git a/chrome/renderer/autofill/password_generation_manager_browsertest.cc b/chrome/renderer/autofill/password_generation_manager_browsertest.cc
index 69074b0..12a5158 100644
--- a/chrome/renderer/autofill/password_generation_manager_browsertest.cc
+++ b/chrome/renderer/autofill/password_generation_manager_browsertest.cc
@@ -112,7 +112,7 @@ TEST_F(PasswordGenerationManagerTest, DetectionTest) {
   SetFocused(first_password_element.to<WebNode>());
   EXPECT_EQ(0u, generation_manager_->messages().size());
 
-  // Pretend like sync was enabled.
+  // Pretend like password generation was enabled.
   AutofillMsg_PasswordGenerationEnabled msg(0, true);
   generation_manager_->OnMessageReceived(msg);
 
diff --git a/chrome/renderer/chrome_content_renderer_client.cc b/chrome/renderer/chrome_content_renderer_client.cc
index e380f31..4013b92f 100644
--- a/chrome/renderer/chrome_content_renderer_client.cc
+++ b/chrome/renderer/chrome_content_renderer_client.cc
@@ -255,6 +255,10 @@ void ChromeContentRendererClient::RenderViewCreated(
       new PasswordAutofillManager(render_view);
   AutofillAgent* autofill_agent = new AutofillAgent(render_view,
                                                     password_autofill_manager);
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnablePasswordGeneration)) {
+    new PasswordGenerationManager(render_view);
+  }
   PageClickTracker* page_click_tracker = new PageClickTracker(render_view);
   // Note that the order of insertion of the listeners is important.
   // The password_autocomplete_manager takes the first shot at processing the
@@ -272,10 +276,6 @@ void ChromeContentRendererClient::RenderViewCreated(
           switches::kDomAutomationController)) {
     new AutomationRendererHelper(render_view);
   }
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnablePasswordGeneration)) {
-    new PasswordGenerationManager(render_view);
-  }
 }
 
 void ChromeContentRendererClient::SetNumberOfViews(int number_of_views) {