Make password generation switched by a preference in chrome settings rather than a command line fla…

chrome/browser/autofill/autofill_manager.cc

Index: chrome/browser/autofill/autofill_manager.cc
diff --git a/chrome/browser/autofill/autofill_manager.cc b/chrome/browser/autofill/autofill_manager.cc
index 1ec53e74eceda2afed3dfe4ac9bb59493d443dd2..b04a34fdc29e173a634628834489cbe69d13df53 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,
+                             false,
+                             PrefService::SYNCABLE_PREF);
 #if defined(OS_MACOSX)
   prefs->RegisterBooleanPref(prefs::kAutofillAuxiliaryProfilesEnabled,
                              true,
@@ -234,26 +239,33 @@ void AutofillManager::RegisterWithSyncService() {
   }
 }
 
-void AutofillManager::SendPasswordGenerationStateToRenderer(
-    content::RenderViewHost* host, bool enabled) {
-  host->Send(new AutofillMsg_PasswordGenerationEnabled(host->GetRoutingID(),
-                                                       enabled));
-}
-
 void AutofillManager::UpdatePasswordGenerationState(
     content::RenderViewHost* host,
     bool new_renderer) {
+  // In order for password generation to be enabled, we need to make sure:
+  // (1) Password sync is enabled, and

[Ilya Sherman, 2012/05/01 22:27:48]

nit: No need for "and" on this line -- just on the penultimate line in the list.

[zysxqn, 2012/05/03 00:57:24]

Done.

+  // (2) Password manager is enabled, and
+  // (3) Password generation preference check box is checked.

[Ilya Sherman, 2012/05/01 22:27:48]

nit: Please move this comment a bit further up still: immediately above the
method definition.

[zysxqn, 2012/05/03 00:57:24]

Done.

   if (!sync_service_)
     return;

[Ilya Sherman, 2012/05/01 22:27:48]

nit: Please add a blank line after this if-stmt.

[zysxqn, 2012/05/03 00:57:24]

Done.

-
-  // 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 new_password_generation_enabled =
-      password_sync_enabled &&
+  bool new_password_sync_enabled =

[Ilya Sherman, 2012/05/01 22:27:48]

nit: There is no "old_password_sync_enabled" to compare to, so please continue
to call this "password_sync_enabled".

[zysxqn, 2012/05/03 00:57:24]

Done.

+      sync_service_->HasSyncSetupCompleted() &&
+      sync_set.Has(syncable::PASSWORDS);
+
+  bool new_password_manager_enabled =
       tab_contents_wrapper_->password_manager()->IsEnabled();
+
+  Profile* profile = Profile::FromBrowserContext(
+      web_contents()->GetBrowserContext());
+  bool new_preference_checked =
+      profile->GetPrefs()->GetBoolean(prefs::kPasswordGenerationEnabled);
+
+  bool new_password_generation_enabled =
+      new_password_sync_enabled &&
+      new_password_manager_enabled &&
+      new_preference_checked;
+
   if (new_password_generation_enabled != password_generation_enabled_ ||
       new_renderer) {
     password_generation_enabled_ = new_password_generation_enabled;
@@ -261,8 +273,31 @@ void AutofillManager::UpdatePasswordGenerationState(
   }
 }
 
+void AutofillManager::SendPasswordGenerationStateToRenderer(
+    content::RenderViewHost* host, bool enabled) {
+  host->Send(new AutofillMsg_PasswordGenerationEnabled(host->GetRoutingID(),
+                                                       enabled));
+}
+
 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) {
+  switch (type) {

[Ilya Sherman, 2012/05/01 22:27:48]

nit: Since this code currently only listens for exactly one type of
notification, please just start with DCHECK_EQ(chrome::NOTIFICATION_PREF_CHANGE,
type), and skip the switch stmt.

[zysxqn, 2012/05/03 00:57:24]

Done.

+    case chrome::NOTIFICATION_PREF_CHANGED: {
+      DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+      std::string* pref = content::Details<std::string>(details).ptr();
+      DCHECK(*pref == prefs::kPasswordGenerationEnabled);

[Ilya Sherman, 2012/05/01 22:27:48]

nit: DCHECK_EQ(prefs::kPasswordGenerationEnabled, *pref)

[zysxqn, 2012/05/03 00:57:24]

Done.

+      UpdatePasswordGenerationState(web_contents()->GetRenderViewHost(), false);

[Ilya Sherman, 2012/05/01 22:27:48]

Why do you always pass |false| for the second parameter?

[zysxqn, 2012/05/03 00:57:24]

In AutofillManager::RenderViewCreated() we call the function with |true| for the
second parameter, to indicate a newly created render view and to update the
password generation state no matter what's the old state.

[Ilya Sherman, 2012/05/04 06:04:50]

Sorry, I misread this as calling SendPasswordGenerationStateToRenderer() rather
than UpdatePasswordGenerationState().  Looks fine now that I'm reading it
correctly :)

+      break;
+    }
+    default:
+      NOTREACHED();
+  }
 }
 
 void AutofillManager::OnStateChanged() {
@@ -843,6 +878,7 @@ AutofillManager::AutofillManager(TabContentsWrapper* tab_contents,
       external_delegate_(NULL) {
   DCHECK(tab_contents);
   RegisterWithSyncService();
+  // Test code doens't need registrar_.

[Ilya Sherman, 2012/05/01 22:27:48]

nit: "doens't" -> "doesn't"

[zysxqn, 2012/05/03 00:57:24]

Done.

 }
 
 void AutofillManager::set_metric_logger(const AutofillMetrics* metric_logger) {