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..fffca903d617f4bf2ad7087578225ecf35c6f216 100644
--- a/components/autofill/core/browser/personal_data_manager_mac.mm
+++ b/components/autofill/core/browser/personal_data_manager_mac.mm
@@ -31,6 +31,24 @@ 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;
+}
+
+// Whether Chromium has prompted the user for permission to access the user's
+// address book.
+bool HasPromptedForAccessToAddressBook(PrefService* pref_service) {
+  return pref_service->GetBoolean(prefs::kAutofillAuxiliaryProfilesQueried);
+}
+
 // 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 +99,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 (!HasPromptedForAccessToAddressBook(pref_service))
+    return;
+
+  ABAddressBook* addressBook = GetAddressBook();
 
   ABPerson* me = [addressBook me];
   if (!me)
@@ -281,4 +292,19 @@ void PersonalDataManager::LoadAuxiliaryProfiles() const {
   impl.GetAddressBookMeCard(app_locale_, pref_service_);
 }
 
+bool PersonalDataManager::HasPromptedForAccessToAddressBook() {
+  return ::autofill::HasPromptedForAccessToAddressBook(pref_service_);
+}
+
+void PersonalDataManager::AccessAddressBook() {
+  // Immediately record that Chromium has requested permissions.
+  pref_service_->SetBoolean(prefs::kAutofillAuxiliaryProfilesQueried, true);

[Ilya Sherman, 2014/05/21 11:29:15]

I'd prefer that this remain in the GetAddressBook() method, since that is what
actually prompts for access.  If there were ever another codepath introduced
that could call that method, it would be correct to set the pref to true in that
case.

[erikchen, 2014/05/21 22:00:54]

Good point. I've done as you suggested.

+
+  // Request permissions.
+  ABAddressBook* addressBook = GetAddressBook();
+
+  UMA_HISTOGRAM_BOOLEAN("Autofill.AddressBookAvailableOnFirstAttempt",
+                        addressBook != nil);

[Ilya Sherman, 2014/05/21 11:29:15]

I'd prefer to be extra careful, and only set this when the pref was previously
false.

[erikchen, 2014/05/21 22:00:54]

I've added a check for that pref value, as well as a check for
kAutofillAuxiliaryProfilesEnabled, which was missing.

+}
+
 }  // namespace autofill