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..17943d9ba53e395918269a26488cfeecaec25b8d 100644
--- a/components/autofill/core/browser/personal_data_manager_mac.mm
+++ b/components/autofill/core/browser/personal_data_manager_mac.mm
@@ -31,6 +31,29 @@
 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 crbug.com/381763.

[Ilya Sherman, 2014/06/17 03:29:17]

nit: Please prepend "http://" to the URL, so that it's more likely to be
automagically linkified.

[erikchen, 2014/06/17 18:14:27]

Done.

+//
+// 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.
+//
+// This variable is set to true after the Address Book is accessed for the

[Ilya Sherman, 2014/06/17 03:29:17]

nit: "This variable is set" -> "Set"

[erikchen, 2014/06/17 18:14:28]

Done.

+// first time.
+static bool g_accessed_address_book = false;

[Ilya Sherman, 2014/06/17 03:29:17]

nit: Please leave a blank line between consecutive commented variables, unless
the comment above the first applies to the rest as well.

[erikchen, 2014/06/17 18:14:28]

Done.

+// This variable is set to true after the Chrome binary has been changed.
+static bool g_binary_changed = false;
+// Metrics for skipping the prompt are only recorded once per launch of Chrome.
+static bool g_recorded_skip_metrics = false;

[Ilya Sherman, 2014/06/17 03:29:17]

Hmm, only recording the metrics once per launch seems like it would introduce
considerable bias, since the users who are potentially affected by this issue
are the very same users who don't relaunch Chrome as frequently.

What we really want to measure is: Of the times that we would have shown Address
Book suggestions in the Autofill popup UI, how many times are we prevented from
doing so by this logic?  Of course, we can't directly measure that, because we
don't know whether an Address Book that we can't access actually has useful data
in it or not.  But I think we can do better than measuring just once per launch.
 Perhaps we should measure once per time the popup is shown?

[erikchen, 2014/06/17 18:14:27]

I've removed the logic to record metrics once per launch - the new logic records
metrics once per time the popup is shown. 

+
 const char kAddressBookOrigin[] = "OS X Address Book";
 
 // Whether Chrome has prompted the user for permission to access the user's
@@ -45,6 +68,16 @@ 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 beccause doing so would cause the Address Book permissions

[Ilya Sherman, 2014/06/17 03:29:17]

nit: "beccause" -> "because"

[erikchen, 2014/06/17 18:14:28]

Done.

+// prompt to incorrectly appear.
+void RecordSkipReprompt(bool skip) {
+  if (g_recorded_skip_metrics)
+    return;
+  g_recorded_skip_metrics = true;
+  UMA_HISTOGRAM_BOOLEAN("Autofill.AddressBookReprompt", skip);
+}
+
 ABAddressBook* GetAddressBook(PrefService* pref_service) {
   bool first_access = !HasPromptedForAccessToAddressBook(pref_service);
 
@@ -62,6 +95,7 @@ ABAddressBook* GetAddressBook(PrefService* pref_service) {
                           addressBook != nil);
   }
 
+  g_accessed_address_book = true;
   pref_service->SetBoolean(prefs::kAutofillMacAddressBookQueried, true);
   return addressBook;
 }
@@ -121,6 +155,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);
+
   ABAddressBook* addressBook = GetAddressBook(pref_service);
 
   ABPerson* me = [addressBook me];
@@ -343,4 +385,8 @@ bool PersonalDataManager::ShouldShowAccessAddressBookSuggestion(
   return false;
 }
 
+void PersonalDataManager::BinaryChanging() {
+  g_binary_changed = true;
+}
+
 }  // namespace autofill