mac: Prevent Address Book permissions dialog from erroneously appearing.

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 2be22fca949233856cad780c6f9aa4a3b575c3b0..ca77ae6f6deea9709f1165cc9eb64284961ecbf1 100644
--- a/components/autofill/core/browser/personal_data_manager_mac.mm
+++ b/components/autofill/core/browser/personal_data_manager_mac.mm
@@ -31,6 +31,27 @@
 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
@@ -45,6 +66,13 @@ 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);
 
@@ -62,6 +90,7 @@ ABAddressBook* GetAddressBook(PrefService* pref_service) {
                           addressBook != nil);
   }
 
+  g_accessed_address_book = true;
   pref_service->SetBoolean(prefs::kAutofillMacAddressBookQueried, true);
   return addressBook;
 }
@@ -121,6 +150,14 @@ void AuxiliaryProfilesImpl::GetAddressBookMeCard(const std::string& app_locale,
   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];
@@ -343,4 +380,8 @@ bool PersonalDataManager::ShouldShowAccessAddressBookSuggestion(
   return false;
 }
 
+void PersonalDataManager::BinaryChanging() {
+  g_binary_changed = true;
+}
+
 }  // namespace autofill