mac: Clean up autofill integration with Address Book.

components/autofill/core/browser/autofill_manager.cc

Index: components/autofill/core/browser/autofill_manager.cc
diff --git a/components/autofill/core/browser/autofill_manager.cc b/components/autofill/core/browser/autofill_manager.cc
index 8bceedd686b0bc8d9a61a4f6bee5dcdac12f4ca8..60783ebbd005180e90e77113d9d6ad688a05a204 100644
--- a/components/autofill/core/browser/autofill_manager.cc
+++ b/components/autofill/core/browser/autofill_manager.cc
@@ -214,7 +214,7 @@ void AutofillManager::RegisterProfilePrefs(
 #endif  // defined(OS_MACOSX) || defined(OS_ANDROID)
 #if defined(OS_MACOSX)
   registry->RegisterBooleanPref(
-      prefs::kAutofillAuxiliaryProfilesQueried,
+      prefs::kAutofillMacAddressBookQueried,
       false,
       user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
 #endif  // defined(OS_MACOSX)
@@ -226,7 +226,49 @@ void AutofillManager::RegisterProfilePrefs(
       prefs::kAutofillNegativeUploadRate,
       kAutofillNegativeUploadRateDefaultValue,
       user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
+
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+  registry->RegisterBooleanPref(
+      prefs::kAutofillUseMacAddressBook,
+      false,
+      user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
+#endif  // defined(OS_MACOSX) && !defined(OS_IOS)
+}
+
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+void AutofillManager::MigrateUserPrefs(PrefService* prefs) {
+  const PrefService::Preference* pref =
+      prefs->FindPreference(prefs::kAutofillUseMacAddressBook);
+  DCHECK(pref);

[Ilya Sherman, 2014/06/04 22:46:41]

nit: Since you use this variable immediately below, there's no need for this
DCHECK.  We generally only have DCHECKs for NULL pointers when the code that
uses them is temporally separated from the DCHECK, and hence the stack trace
from the NULL pointer crash would be insufficiently informative.

[erikchen, 2014/06/04 22:50:05]

Good to know, thanks! Done.

+
+  // If the pref is not its default value, then the migration has already been
+  // performed.
+  if (!pref->IsDefaultValue())
+    return;
+
+  // Whether Chrome has already tried to access the user's Address Book.
+  const PrefService::Preference* pref_accessed =
+      prefs->FindPreference(prefs::kAutofillMacAddressBookQueried);
+  // Whether the user wants to use the Address Book to populate Autofill.
+  const PrefService::Preference* pref_enabled =
+      prefs->FindPreference(prefs::kAutofillAuxiliaryProfilesEnabled);
+  DCHECK(pref_accessed);
+  DCHECK(pref_enabled);
+
+  // This is likely a new user. Reset the default value to prevent the migration
+  // from happening again.

[Ilya Sherman, 2014/06/04 22:46:41]

nit: Please move this comment into the if-stmt.  Generally comments outside of
if-stmts are of the form "Check for X", and within if-stmts are of the form "X
is the case, therefore Y".

[erikchen, 2014/06/04 22:50:05]

Okay!

+  if (pref_accessed->IsDefaultValue() && pref_enabled->IsDefaultValue()) {
+    prefs->SetBoolean(prefs::kAutofillUseMacAddressBook,
+                      prefs->GetBoolean(prefs::kAutofillUseMacAddressBook));
+    return;
+  }

[Ilya Sherman, 2014/06/04 20:46:09]

Hmm, is this extra work necessary?  The default value of the accessed pref is
false, and the default value of the UseMacAddressBook pref is also false, so it
seems like the result is the same either way.  Are you worried about the
defaults changing and thereby breaking the new user case if you omit this
explicit code?

[erikchen, 2014/06/04 21:13:03]

I want to make sure that the migration code is called exactly once. If I don't
manually write to the pref, then the migration code is going to be called on
every launch (which requires 3 pref checks), whereas this one time write ensures
that for all future launches, there is only 1 pref checked before the migration
is skipped.

[Ilya Sherman, 2014/06/04 21:21:03]

I'm not following what you mean -- line 269 would always write to the pref,
right?

[Ilya Sherman, 2014/06/04 22:21:45]

^^^

[erikchen, 2014/06/04 22:27:08]

Ack, I misread your comment. Yes, I would like this code to not depend on the
default value of the accessed pref.

[Ilya Sherman, 2014/06/04 22:46:41]

Hrm, alright.

+
+  bool accessed = pref_accessed->GetValue();
+  bool enabled = pref_enabled->GetValue();
+
+  prefs->SetBoolean(prefs::kAutofillUseMacAddressBook, accessed && enabled);
 }
+#endif  // defined(OS_MACOSX) && !defined(OS_IOS)
 
 void AutofillManager::SetExternalDelegate(AutofillExternalDelegate* delegate) {
   // TODO(jrg): consider passing delegate into the ctor.  That won't