mac: Clean up autofill integration with Address Book.

components/autofill/core/browser/autofill_manager.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/autofill/core/browser/autofill_manager.h"
 
 #include <stddef.h>
 
 #include <limits>
 #include <map>
@@ skipping 196 matching lines @@
       true,
       user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
 #else  // defined(OS_MACOSX) || defined(OS_ANDROID)
   registry->RegisterBooleanPref(
       prefs::kAutofillAuxiliaryProfilesEnabled,
       false,
       user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
 #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)
   registry->RegisterDoublePref(
       prefs::kAutofillPositiveUploadRate,
       kAutofillPositiveUploadRateDefaultValue,
       user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
   registry->RegisterDoublePref(
       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
   // work if the delegate has a pointer to the AutofillManager, but
   // future directions may not need such a pointer.
   external_delegate_ = delegate;
   autocomplete_history_manager_->SetExternalDelegate(delegate);
 }
 
 void AutofillManager::ShowAutofillSettings() {
   manager_delegate_->ShowAutofillSettings();
@@ skipping 919 matching lines @@
     return false;
 
   // Disregard forms that we wouldn't ever autofill in the first place.
   if (!form.ShouldBeParsed(true))
     return false;
 
   return true;
 }
 
 }  // namespace autofill