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

components/autofill/core/browser/personal_data_manager_mac.mm

 // 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/personal_data_manager.h"
 
 #include <math.h>
 
 #import <AddressBook/AddressBook.h>
 
@@ skipping 13 matching lines @@
 #include "components/autofill/core/browser/phone_number.h"
 #include "components/autofill/core/common/autofill_pref_names.h"
 #include "grit/component_strings.h"
 #include "ui/base/l10n/l10n_util_mac.h"
 
 namespace autofill {
 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
 // the constructer and then initiate the fetch from the Mac Address Book "me"
 // card using the main |GetAddressBookMeCard()| method.  This clears any
 // existing addresses and populates new addresses derived from the data found
 // in the "me" card.
 class AuxiliaryProfilesImpl {
  public:
   // Constructor takes a reference to the |profiles| that will be filled in
@@ skipping 30 matching lines @@
 };
 
 // This method uses the |ABAddressBook| system service to fetch the "me" card
 // from the active user's address book.  It looks for the user address
 // information and translates it to the internal list of |AutofillProfile| data
 // structures.
 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)
     return;
 
   ABMultiValue* addresses = [me valueForProperty:kABAddressProperty];
 
   // The number of characters at the end of the GUID to reserve for
   // distinguishing addresses within the "me" card.  Cap the number of addresses
   // we will fetch to the number that can be distinguished by this fragment of
@@ skipping 166 matching lines @@
 }
 
 }  // namespace
 
 // Populate |auxiliary_profiles_| with the Address Book data.
 void PersonalDataManager::LoadAuxiliaryProfiles() const {
   AuxiliaryProfilesImpl impl(&auxiliary_profiles_);
   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