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 {
 
+// 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
 // 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.
@@ skipping 11 matching lines @@
   // 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);
   }
 
+  kAccessedAddressBook = 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;
 
+  // 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];
   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() {
+  kBinaryChanged = true;
+}
+
 }  // namespace autofill