Mac: Autofill should not immediately request access to address book.

components/autofill/core/browser/personal_data_manager_mac.mm

Index: components/autofill/core/browser/personal_data_manager_mac.mm
diff --git a/components/autofill/core/browser/personal_data_manager_mac.mm b/components/autofill/core/browser/personal_data_manager_mac.mm
index 745a60e6b99ce914b4adb2717f9138f7d183f683..7538484c3492a9324699261776a5e6cd4136af42 100644
--- a/components/autofill/core/browser/personal_data_manager_mac.mm
+++ b/components/autofill/core/browser/personal_data_manager_mac.mm
@@ -31,6 +31,18 @@ namespace {
 
 const char kAddressBookOrigin[] = "OS X Address Book";
 
+ABAddressBook* GetAddressBook() {
+  // +[ABAddressBook sharedAddressBook] throws an exception internally in
+  // circumstances that aren't clear. The exceptions are only observed in crash
+  // reports, so it is unknown whether they would be caught by AppKit and nil
+  // returned, or if they would take down the app. In either case, avoid
+  // crashing. http://crbug.com/129022
+  ABAddressBook* addressBook = base::mac::RunBlockIgnoringExceptions(
+      ^{ return [ABAddressBook sharedAddressBook]; });
+  UMA_HISTOGRAM_BOOLEAN("Autofill.AddressBookAvailable", addressBook != nil);
+  return addressBook;
+}
+
 // This implementation makes use of the Address Book API.  Profiles are
 // generated that correspond to addresses in the "me" card that reside in the
 // user's Address Book.  The caller passes a vector of profiles into the
@@ -81,20 +93,13 @@ void AuxiliaryProfilesImpl::GetAddressBookMeCard(const std::string& app_locale,
                                                  PrefService* pref_service) {
   profiles_.clear();
 
-  // +[ABAddressBook sharedAddressBook] throws an exception internally in
-  // circumstances that aren't clear. The exceptions are only observed in crash
-  // reports, so it is unknown whether they would be caught by AppKit and nil
-  // returned, or if they would take down the app. In either case, avoid
-  // crashing. http://crbug.com/129022
-  ABAddressBook* addressBook = base::mac::RunBlockIgnoringExceptions(^{
-      return [ABAddressBook sharedAddressBook];
-  });
-  UMA_HISTOGRAM_BOOLEAN("Autofill.AddressBookAvailable", addressBook != nil);
-  if (!pref_service->GetBoolean(prefs::kAutofillAuxiliaryProfilesQueried)) {
-    pref_service->SetBoolean(prefs::kAutofillAuxiliaryProfilesQueried, true);
-    UMA_HISTOGRAM_BOOLEAN("Autofill.AddressBookAvailableOnFirstAttempt",
-                          addressBook != nil);
-  }
+  // Chrome has not yet requested address book permissions. Attempting to do so
+  // presents a blocking modal dialog, which is undesirable. Instead, just show
+  // no results.
+  if (!pref_service->GetBoolean(prefs::kAutofillAuxiliaryProfilesQueried))
+    return;

[Ilya Sherman, 2014/05/16 22:54:03]

nit: Please call into HasAccessMacContacts() here rather than duplicating the
code.

[erikchen, 2014/05/19 20:58:32]

I've created an anonymous-namespace scoped function to remove the logic
duplication.

+
+  ABAddressBook* addressBook = GetAddressBook();
 
   ABPerson* me = [addressBook me];
   if (!me)
@@ -281,4 +286,21 @@ void PersonalDataManager::LoadAuxiliaryProfiles() const {
   impl.GetAddressBookMeCard(app_locale_, pref_service_);
 }
 
+bool PersonalDataManager::HasAccessMacContacts() {
+  return pref_service_->GetBoolean(prefs::kAutofillAuxiliaryProfilesQueried);
+}
+
+void PersonalDataManager::AccessMacContacts() {
+  // Immediately record that Chromium has requested permissions.
+  pref_service_->SetBoolean(prefs::kAutofillAuxiliaryProfilesQueried, true);
+
+  // Request permissions.
+  ABAddressBook* addressBook = GetAddressBook();
+
+  if (!pref_service_->GetBoolean(prefs::kAutofillAuxiliaryProfilesQueried)) {

[Ilya Sherman, 2014/05/16 22:54:03]

This will always be false now, right?

[erikchen, 2014/05/19 20:58:32]

Right you are. I've removed the conditional.

+    UMA_HISTOGRAM_BOOLEAN("Autofill.AddressBookAvailableOnFirstAttempt",
+                          addressBook != nil);
+  }
+}
+
 }  // namespace autofill