mac: Only show the access Address Book prompt a fixed number of times.

components/autofill/core/browser/autofill_external_delegate.cc

Index: components/autofill/core/browser/autofill_external_delegate.cc
diff --git a/components/autofill/core/browser/autofill_external_delegate.cc b/components/autofill/core/browser/autofill_external_delegate.cc
index 80ad335a44a0e4c50f92eb406bdb0d6884ee5045..95c25c0ff0cff30b142a29f32817fd1cced20262 100644
--- a/components/autofill/core/browser/autofill_external_delegate.cc
+++ b/components/autofill/core/browser/autofill_external_delegate.cc
@@ -49,7 +49,8 @@ AutofillExternalDelegate::AutofillExternalDelegate(AutofillManager* manager,
       display_warning_if_disabled_(false),
       has_suggestion_(false),
       has_shown_popup_for_current_edit_(false),
-      weak_ptr_factory_(this) {
+      weak_ptr_factory_(this),
+      has_shown_address_book_prompt(false) {
   DCHECK(manager);
 }
 
@@ -60,6 +61,9 @@ void AutofillExternalDelegate::OnQuery(int query_id,
                                        const FormFieldData& field,
                                        const gfx::RectF& element_bounds,
                                        bool display_warning_if_disabled) {
+  if (query_form_ != form)
+    has_shown_address_book_prompt = false;
+
   query_form_ = form;
   query_field_ = field;
   display_warning_if_disabled_ = display_warning_if_disabled;
@@ -117,8 +121,6 @@ void AutofillExternalDelegate::OnSuggestionsReturned(
   // updated to match.
   InsertDataListValues(&values, &labels, &icons, &ids);
 
-// Temporarily disabled. See http://crbug.com/408695
-#if 0
 #if defined(OS_MACOSX) && !defined(OS_IOS)
   if (values.empty() &&
       manager_->ShouldShowAccessAddressBookSuggestion(query_form_,
@@ -129,10 +131,13 @@ void AutofillExternalDelegate::OnSuggestionsReturned(
     icons.push_back(base::ASCIIToUTF16("macContactsIcon"));
     ids.push_back(POPUP_ITEM_ID_MAC_ACCESS_CONTACTS);
 
-    EmitHistogram(SHOWED_ACCESS_ADDRESS_BOOK_ENTRY);
+    if (!has_shown_address_book_prompt) {
+      has_shown_address_book_prompt = true;
+      EmitHistogram(SHOWED_ACCESS_ADDRESS_BOOK_ENTRY);
+      manager_->ShowedAccessAddressBookPrompt();
+    }
   }
 #endif  // defined(OS_MACOSX) && !defined(OS_IOS)
-#endif
 
   if (values.empty()) {
     // No suggestions, any popup currently showing is obsolete.
@@ -201,6 +206,10 @@ void AutofillExternalDelegate::DidAcceptSuggestion(const base::string16& value,
   } else if (identifier == POPUP_ITEM_ID_MAC_ACCESS_CONTACTS) {
 #if defined(OS_MACOSX) && !defined(OS_IOS)
     EmitHistogram(SELECTED_ACCESS_ADDRESS_BOOK_ENTRY);
+    LOCAL_HISTOGRAM_ENUMERATION(

[Ilya Sherman, 2014/09/20 00:01:24]

This should be an UMA_HISTOGRAM_ENUMERATION, since you want the data to be
uploaded to UMA.

Out of curiousity, could you please let me know how you landed on the LOCAL_
variant instead?  We recently renamed these macros so that it would be less
likely that someone would accidentally choose the wrong variant.  Any
suggestions on how to make this even clearer?

[erikchen, 2014/09/20 00:36:05]

I was reading the header file, and at the very top, it mentions 3 types of
histograms: Histogram, LinearHistogram, CustomHistogram. It was clear that I
wanted LinearHistogram, so I searched for that, which showed 2 macros that used
it: HISTOGRAM_ENUMERATION_WITH_FLAG and LOCAL_HISTOGRAM_ENUMERATION. I failed to
notice "local" as an important keyword, and I said to myself: "I don't see a
reason why I would need additional flags".

Suggestions: 
1. It would be helpful if the top of the header file indicated that there exists
a distinction between UMA vs local histograms. The first reference to UMA
happens ~line 230, at which point my brain is already dead from reading all of
the implementation details of histograms.
2. It would be helpful if implementation details could be moved into a separate
file, or if the macros intended for public consumption are moved into their own
file.

[Ilya Sherman, 2014/09/20 00:44:43]

Thanks, that's very helpful feedback.  I think that you're right that splitting
the rarely-used macros off into a separate file would be an easy way to improve
clarity in this file.

[Alexei Svitkine (slow), 2014/09/22 15:10:47]

Thanks for the discussion, it makes sense to me too. I filed crbug.com/416479 to
track it.

+        "Autofill.MacAddressBook.NumShowsBeforeSelected",
+        manager_->AccessAddressBookPromptCount(),
+        30);

[Ilya Sherman, 2014/09/20 00:01:24]

Why 30?  That's just allocating space that will never be used.  Please use the
same constant as the current upper-bound.  It's safe to enlarge enumerated
histograms if that ever becomes useful.

[erikchen, 2014/09/20 00:36:05]

I assumed that enlarging and then reducing this value would not be safe. e.g. I
decide to show 10 times, and then I decide to show 3 times. Will the UMA graphs
correctly show the data across all time, or will there be weird truncations? I
assume that you are going to respond with "UMA will do the right thing", so I
changed the value to use the same constant.

[Ilya Sherman, 2014/09/20 00:44:43]

Enlarging is safe, shrinking is not.  If you want something that's guaranteed to
be safe regardless of how you change the constant, you might want to use
UMA_HISTOGRAM_SPARSE_SLOWLY.

[erikchen, 2014/09/20 01:03:21]

I see. Using the same constant has the problem that it implicitly breaks UMA
metrics if someone comes along and reduces the constant, which is unexpected.
I've gone with the sparse histogram.

 
     // User wants to give Chrome access to user's address book.
     manager_->AccessAddressBook();