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..b70ad8855ac55a89ef9211682d11fb64315ae7bc 100644
--- a/components/autofill/core/browser/personal_data_manager_mac.mm
+++ b/components/autofill/core/browser/personal_data_manager_mac.mm
@@ -31,6 +31,18 @@
 namespace autofill {
 namespace {
 
+// The following two variables are intended to be shared across all instances
+// of the personal_data_manager, and reset when Chrome is first launched. They
+// are only intended to be accessed from the main thread.

[Ilya Sherman, 2014/06/14 01:18:51]

There's generally no need to document that something is not thread safe, unless
readers of the code might have some reason to guess that it is.  Chrome code is
assumed to be non-thread safe by default.

[erikchen, 2014/06/16 20:30:46]

okay. I've removed comments about thread safety.

+//
+// After each fresh launch of Chrome, if the Address Book is accessed for the
+// first time after the binary has been changed, then the Address Book
+// permissions dialog is guaranteed to appear. Chrome should never attempt to
+// automatically access the Address Book on behalf of the user in this case,
+// since it presents a blocking dialog.

[Ilya Sherman, 2014/06/14 01:18:51]

I found it kind of hard to understand what this comment meant.  Could you please
rephrase it to be a little clearer?  It might help to explicitly describe the
sequence of events that leads to a bad state in a more overt way, perhaps as a
numbered list, or a simple ASCII art diagram.

[erikchen, 2014/06/16 20:30:46]

I've completely rewritten the comments.

+static bool kAccessedAddressBook = false;
+static bool kBinaryChanged = false;

[Ilya Sherman, 2014/06/14 01:18:51]

These are not constants, so they should not be named as such.  You can either
name them as g_accessed_address_book, etc.; or, better yet, make them static
private members of the class.

[erikchen, 2014/06/16 20:30:46]

I renamed them as global variables. If you strongly prefer, I can make them
static private members of personal_data_manager, but that adds complexity to the
logic of this class. The meaning and intended use of the variables is tied to
accessing the address book, rather than the personal_data_manager.

+
 const char kAddressBookOrigin[] = "OS X Address Book";
 
 // Whether Chrome has prompted the user for permission to access the user's
@@ -62,6 +74,7 @@ ABAddressBook* GetAddressBook(PrefService* pref_service) {
                           addressBook != nil);
   }
 
+  kAccessedAddressBook = true;
   pref_service->SetBoolean(prefs::kAutofillMacAddressBookQueried, true);
   return addressBook;
 }
@@ -121,6 +134,12 @@ void AuxiliaryProfilesImpl::GetAddressBookMeCard(const std::string& app_locale,
   if (!ShouldUseAddressBook(pref_service))
     return;
 
+  // Do not attempt to access the Address Book on behalf of the user if this is
+  // the first access attempt since Chrome was launched, and the binary has
+  // changed.

[Ilya Sherman, 2014/06/14 01:18:51]

Same comment applies here.  If you find that it's hard to succinctly describe
what this is doing, it might make sense to have one verbose comment, and then
from the other comment just say "See this verbose comment here, it'll explain
everything."

[erikchen, 2014/06/16 20:30:46]

I've used your suggestion and changed the comment to redirect the user to the
previous comment for more explanation.

+  if (kBinaryChanged && !kAccessedAddressBook)
+    return;
+
   ABAddressBook* addressBook = GetAddressBook(pref_service);
 
   ABPerson* me = [addressBook me];
@@ -343,4 +362,8 @@ bool PersonalDataManager::ShouldShowAccessAddressBookSuggestion(
   return false;
 }
 
+void PersonalDataManager::BinaryChanging() {
+  kBinaryChanged = true;
+}
+
 }  // namespace autofill