Mac: Autofill should not immediately request access to address book.

components/autofill/core/browser/autofill_external_delegate.cc

 // 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/autofill_external_delegate.h"
 
 #include "base/strings/utf_string_conversions.h"
 #include "components/autofill/core/browser/autocomplete_history_manager.h"
 #include "components/autofill/core/browser/autofill_driver.h"
 #include "components/autofill/core/browser/autofill_manager.h"
 #include "components/autofill/core/browser/popup_item_ids.h"
 #include "grit/component_strings.h"
 #include "ui/base/l10n/l10n_util.h"
 
 namespace autofill {
 
-AutofillExternalDelegate::AutofillExternalDelegate(
-    AutofillManager* manager,
-    AutofillDriver* driver)
+AutofillExternalDelegate::AutofillExternalDelegate(AutofillManager* manager,
+                                                   AutofillDriver* driver)
     : manager_(manager),
       driver_(driver),
       query_id_(0),
       display_warning_if_disabled_(false),
       has_suggestion_(false),
       has_shown_popup_for_current_edit_(false),
+#if defined(OS_MACOSX)
+      redisplay_popop_on_focus_lost_(false),
+#endif
       weak_ptr_factory_(this) {
   DCHECK(manager);
 }
 
 AutofillExternalDelegate::~AutofillExternalDelegate() {}
 
 void AutofillExternalDelegate::OnQuery(int query_id,
                                        const FormData& form,
                                        const FormFieldData& field,
                                        const gfx::RectF& element_bounds,
@@ skipping 48 matching lines @@
     labels.pop_back();
     icons.pop_back();
     ids.pop_back();
   }
 
   // If anything else is added to modify the values after inserting the data
   // list, AutofillPopupControllerImpl::UpdateDataListValues will need to be
   // updated to match.
   InsertDataListValues(&values, &labels, &icons, &ids);
 
+#if defined(OS_MACOSX)
+  if (values.empty() &&
+      manager_->ShouldShowAccessAddressBookSuggestion(query_form_,
+                                                      query_field_)) {
+    values.push_back(
+        l10n_util::GetStringUTF16(IDS_AUTOFILL_ACCESS_MAC_CONTACTS));
+    labels.push_back(base::string16());
+    icons.push_back(base::string16());
+    ids.push_back(POPUP_ITEM_ID_MAC_ACCESS_CONTACTS);
+  }
+#endif
+
   if (values.empty()) {
     // No suggestions, any popup currently showing is obsolete.
     manager_->delegate()->HideAutofillPopup();
     return;
   }
 
   // Send to display.
   if (query_field_.is_focusable) {
     manager_->delegate()->ShowAutofillPopup(
         element_bounds_,
@@ skipping 46 matching lines @@
   } else if (identifier == POPUP_ITEM_ID_CLEAR_FORM) {
     // User selected 'Clear form'.
     driver_->RendererShouldClearFilledForm();
   } else if (identifier == POPUP_ITEM_ID_PASSWORD_ENTRY) {
     NOTREACHED();  // Should be handled elsewhere.
   } else if (identifier == POPUP_ITEM_ID_DATALIST_ENTRY) {
     driver_->RendererShouldAcceptDataListSuggestion(value);
   } else if (identifier == POPUP_ITEM_ID_AUTOCOMPLETE_ENTRY) {
     // User selected an Autocomplete, so we fill directly.
     driver_->RendererShouldFillFieldWithValue(value);
+  } else if (identifier == POPUP_ITEM_ID_MAC_ACCESS_CONTACTS) {
+#if defined(OS_MACOSX)
+    // User wants to give Chrome access to user's address book.
+    bool modal_presented = manager_->AccessAddressBook();
+
+    // If a modal view was not presented, immediately refresh the popup.
+    if (!modal_presented) {
+      manager_->delegate()->HideAutofillPopup();
+      ReissueQuery();
+      return;
+    }
+
+    // A blocking modal was presented. When the user responds, the text field
+    // loses focus. At that point, the user may have granted permission to their
+    // address book. Immediately redisplay the popup, which may contain new
+    // suggestions from the address book.
+    redisplay_popop_on_focus_lost_ = true;

[Ilya Sherman, 2014/05/23 15:47:10]

Hmm, this still seems not really correct.  Presumably focus is lost as soon as
the modal dialog is displayed, right?  In that case, wouldn't we be reissuing
the query too soon?  I think instead it makes sense to just always post a task
to redisplay the popup.  That way, if the modal dialog is blocking the UI
thread, the task will wait.  If it's not, it'll fire immediately.  Does that
sound sensible?

[erikchen, 2014/05/23 17:19:38]

Focus is not lost as soon as the modal dialog is displayed. I've updated the
comment to more accurately describe the behavior of Cocoa. (There's an NSEvent
queued in the NSRunLoop which is going to cause the popup to lose focus. The
current logic redisplays the popup at the earliest possible point)

[Ilya Sherman, 2014/05/28 06:43:27]

Hmm, this still seems like more complexity than we need to handle this case. 
I'm also worried that it might be a little brittle -- what if the Cocoa behavior
changes in a subsequent OS release?

Did you consider my suggestion of posting a task back to the current message
loop?  That seems like an easier way to always do the right thing, assuming that
it works correctly.

+#else
+    NOTREACHED();
+#endif
   } else {
     FillAutofillFormData(identifier, false);
   }
 
   manager_->delegate()->HideAutofillPopup();
 }
 
 void AutofillExternalDelegate::RemoveSuggestion(const base::string16& value,
                                                 int identifier) {
   if (identifier > 0)
     manager_->RemoveAutofillProfileOrCreditCard(identifier);
   else
     manager_->RemoveAutocompleteEntry(query_field_.name, value);
 }
 
 void AutofillExternalDelegate::DidEndTextFieldEditing() {
   manager_->delegate()->HideAutofillPopup();
 
   has_shown_popup_for_current_edit_ = false;
+
+#if defined(OS_MACOSX)
+  // The user was presented a modal dialog, which they had to interact with
+  // before the Chrome process could resume. Their interaction is guaranteed to
+  // cause the text field to lose focus. They were either granting or denying
+  // Chrome access to their address book, so immediately try to redisplay the
+  // autofill popup.
+  if (redisplay_popop_on_focus_lost_) {
+    ReissueQuery();
+    redisplay_popop_on_focus_lost_ = false;
+  }
+#endif
 }
 
 void AutofillExternalDelegate::ClearPreviewedForm() {
   driver_->RendererShouldClearPreviewedForm();
 }
 
 void AutofillExternalDelegate::Reset() {
   manager_->delegate()->HideAutofillPopup();
 }
 
@@ skipping 112 matching lines @@
 
   // Set the values that all datalist elements share.
   icons->insert(icons->begin(),
                 data_list_values_.size(),
                 base::string16());
   unique_ids->insert(unique_ids->begin(),
                      data_list_values_.size(),
                      POPUP_ITEM_ID_DATALIST_ENTRY);
 }
 
+#if defined(OS_MACOSX)
+void AutofillExternalDelegate::ReissueQuery() {
+  manager_->OnQueryFormFieldAutofill(query_id_,
+                                     query_form_,
+                                     query_field_,
+                                     element_bounds_,
+                                     display_warning_if_disabled_);
+}
+#endif
+
 }  // namespace autofill