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

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/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/autofill/core/browser/autocomplete_history_manager.h"
@@ skipping 31 matching lines @@
 namespace autofill {
 
 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),
-      weak_ptr_factory_(this) {
+      weak_ptr_factory_(this),
+      has_shown_address_book_prompt(false) {
   DCHECK(manager);
 }
 
 AutofillExternalDelegate::~AutofillExternalDelegate() {}
 
 void AutofillExternalDelegate::OnQuery(int query_id,
                                        const FormData& form,
                                        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;
   query_id_ = query_id;
   element_bounds_ = element_bounds;
 }
 
 void AutofillExternalDelegate::OnSuggestionsReturned(
     int query_id,
     const std::vector<base::string16>& suggested_values,
@@ skipping 37 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);
 
-// Temporarily disabled. See http://crbug.com/408695
-#if 0
 #if defined(OS_MACOSX) && !defined(OS_IOS)
   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::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.
     manager_->client()->HideAutofillPopup();
     return;
   }
 
   // Send to display.
   if (query_field_.is_focusable) {
     manager_->client()->ShowAutofillPopup(element_bounds_,
@@ skipping 48 matching lines @@
   } 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) && !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();
 
     // There is no deterministic method for deciding whether a blocking dialog
     // was presented. The following comments and code assume that a blocking
     // dialog was presented, but still behave correctly if no dialog was
     // presented.
 
     // A blocking dialog was presented, and the user has already responded to
@@ skipping 178 matching lines @@
                      POPUP_ITEM_ID_DATALIST_ENTRY);
 }
 
 #if defined(OS_MACOSX) && !defined(OS_IOS)
 void AutofillExternalDelegate::PingRenderer() {
   driver_->PingRenderer();
 }
 #endif  // defined(OS_MACOSX) && !defined(OS_IOS)
 
 }  // namespace autofill