mac: Prevent Address Book permissions dialog from erroneously appearing.

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/form_structure.h"
 #include "components/autofill/core/browser/phone_number.h"
 #include "components/autofill/core/common/autofill_pref_names.h"
 #include "components/autofill/core/common/form_data.h"
 #include "grit/components_strings.h"
 #include "ui/base/l10n/l10n_util_mac.h"
 
 namespace autofill {
 namespace {
 
+// There is an uncommon sequence of events that causes the Address Book
+// permissions dialog to appear more than once for a given install of Chrome.
+//  1. Chrome has previously presented the Address Book permissions dialog.
+//  2. Chrome is launched.
+//  3. Chrome performs an auto-update, and changes its binary.
+//  4. Chrome attempts to access the Address Book for the first time since (2).
+// This sequence of events is rare because Chrome attempts to acess the Address
+// Book when the user focuses most form fields, so (4) generally occurs before
+// (3). For more details, see http://crbug.com/381763.
+//
+// When this sequence of events does occur, Chrome should not attempt to access
+// the Address Book unless the user explicitly asks Chrome to do so. The
+// jarring nature of the permissions dialog is worse than the potential benefit
+// of pulling information from the Address Book.
+//
+// Set to true after the Address Book is accessed for the first time.
+static bool g_accessed_address_book = false;
+
+// Set to true after the Chrome binary has been changed.
+static bool g_binary_changed = false;
+
 const char kAddressBookOrigin[] = "OS X Address Book";
 
 // Whether Chrome has prompted the user for permission to access the user's
 // address book.
 bool HasPromptedForAccessToAddressBook(PrefService* pref_service) {
   return pref_service->GetBoolean(prefs::kAutofillMacAddressBookQueried);
 }
 
 // Whether the user wants Chrome to use the AddressBook to populate Autofill
 // entries.
 bool ShouldUseAddressBook(PrefService* pref_service) {
   return pref_service->GetBoolean(prefs::kAutofillUseMacAddressBook);
 }
 
+// Records a UMA metric indicating whether an attempt to access the Address
+// Book was skipped because doing so would cause the Address Book permissions
+// prompt to incorrectly appear.
+void RecordSkipReprompt(bool skip) {
+  UMA_HISTOGRAM_BOOLEAN("Autofill.AddressBookReprompt", skip);
+}
+
 ABAddressBook* GetAddressBook(PrefService* pref_service) {
   bool first_access = !HasPromptedForAccessToAddressBook(pref_service);
 
   // +[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 (first_access) {
     UMA_HISTOGRAM_BOOLEAN("Autofill.AddressBookAvailableOnFirstAttempt",
                           addressBook != nil);
   }
 
+  g_accessed_address_book = true;
   pref_service->SetBoolean(prefs::kAutofillMacAddressBookQueried, true);
   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
 // 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
@@ skipping 39 matching lines @@
 // structures.
 void AuxiliaryProfilesImpl::GetAddressBookMeCard(const std::string& app_locale,
                                                  PrefService* pref_service) {
   profiles_.clear();
 
   // The user does not want Chrome to use the AddressBook to populate Autofill
   // entries.
   if (!ShouldUseAddressBook(pref_service))
     return;
 
+  // See the comment at the definition of g_accessed_address_book for an
+  // explanation of this logic.
+  if (g_binary_changed && !g_accessed_address_book) {
+    RecordSkipReprompt(true);
+    return;
+  }
+  RecordSkipReprompt(false);

[Ilya Sherman, 2014/06/17 19:55:01]

This isn't actually guaranteed to be called only when a popup is shown, right? 
I wonder whether we should jump through some extra hoops to get the metric to be
recorded only when it's really relevant.

[erikchen, 2014/06/17 22:36:06]

I think that the question you are trying to answer with metrics is:

"Should we have Address Book integration at all?"

The relevant metrics are:
1a. How often (or for how many people) does Address Book provide useful autofill
results?
1b. How often is one of those results actually used?
2. Of instances where Chrome would provide results (1a) or useful results (1b),
how often does the logic in this CL prevent those useful results from appearing?
The answer lies in the ratio of (2) and (1a), and the ratio of (1a) to (1b).

Neither (1a) nor (1b) is currently measured. I think that the metric that you
are asking for in your comment is (2). The metric I am recording does not tell
you (2), but it provides sufficient information to answer the question. Let A =
# of instances that line 156 is called. Let B = # of instances that line 159 is
called. Then A / B approximates the ratio of (2) : (1a).

[Ilya Sherman, 2014/06/17 23:50:34]

The trouble with this approximation is that we currently request AddressBook
data before the popup is shown.  So, hits on line 159 would proxy for surfacing
useful AddressBook data, even for users who have no AddressBook data stored. 
Moreover, we query for AddressBook data even on forms that aren't autofillable
(right?).  Therefore, much (most?) of the data would come from forms that aren't
relevant to Autofill.

It sounds like you're asserting that these caveats don't matter.  I'm not sure
-- it's entirely possible that users who frequently use AddressBook integration
also fill out autofillable forms more frequently, whereas other users don't...
but still encounter other forms frequently.

If you think that measuring what we actually show in the popup vs. what we would
show is too hard, then we can proceed with this current metric.  But if we do
so, I think we should look carefully at the absolute numbers, as well as the
ratio.  If line 156 is hit by a large number of users in a day, then I'd be
worried about that result, even if line 159 is hit by a much larger number of
users.

+
   ABAddressBook* addressBook = GetAddressBook(pref_service);
 
   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
@@ skipping 202 matching lines @@
     case NO_GROUP:
     case COMPANY:
     case CREDIT_CARD:
     case PASSWORD_FIELD:
       return false;
   }
 
   return false;
 }
 
+void PersonalDataManager::BinaryChanging() {
+  g_binary_changed = true;
+}
+
 }  // namespace autofill