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

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

Once the Address Book prompt has been shown on 5 times (repeatedly being shown
on the same form doesn't count), don't show it ever again.

This CL also re-enables the access Address Book prompt, which was disabled due
to bugs in the implementation.

BUG=414359
Committed: https://crrev.com/d90fe6dde92f70c6c133d97ae7e686c186060f1c
Cr-Commit-Position: refs/heads/master@{#296114}

[erikchen, 2014-09-18 18:51:00]

erikchen@chromium.org changed reviewers:
+ estade@chromium.org

[erikchen, 2014-09-18 18:51:01]

estade: Please review.

[Evan Stade, 2014-09-18 21:26:37]

https://codereview.chromium.org/578383002/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/578383002/diff/1/components/autofill/core/bro...
components/autofill/core/browser/autofill_external_delegate.cc:213:
EmitHistogram(SELECTED_ACCESS_ADDRESS_BOOK_ENTRY);
histogram to count how many times the prompt was shown before it was selected?

https://codereview.chromium.org/578383002/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/autofill_external_delegate.h (right):

https://codereview.chromium.org/578383002/diff/1/components/autofill/core/bro...
components/autofill/core/browser/autofill_external_delegate.h:158: bool
has_shown_address_book_prompt;
nit: I would not bother with the ifdef because that adds a bit of cruft. Just
notate that the var is only used on mac.

https://codereview.chromium.org/578383002/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/personal_data_manager_mac.mm (right):

https://codereview.chromium.org/578383002/diff/1/components/autofill/core/bro...
components/autofill/core/browser/personal_data_manager_mac.mm:36: int
kMaxTimesToShowMacAddressBookCount = 5;
nit: "Times" and "Count" seem redundant

https://codereview.chromium.org/578383002/diff/1/components/autofill/core/bro...
components/autofill/core/browser/personal_data_manager_mac.mm:382:
kMaxTimesToShowMacAddressBookCount)
nit: 4 more indent

https://codereview.chromium.org/578383002/diff/1/components/autofill/core/bro...
components/autofill/core/browser/personal_data_manager_mac.mm:383: return false;
nit: curlies

[erikchen, 2014-09-18 21:55:58]

estade: PTAL

https://codereview.chromium.org/578383002/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/578383002/diff/1/components/autofill/core/bro...
components/autofill/core/browser/autofill_external_delegate.cc:213:
EmitHistogram(SELECTED_ACCESS_ADDRESS_BOOK_ENTRY);
On 2014/09/18 21:26:37, Evan Stade wrote:
> histogram to count how many times the prompt was shown before it was selected?

Done.

https://codereview.chromium.org/578383002/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/autofill_external_delegate.h (right):

https://codereview.chromium.org/578383002/diff/1/components/autofill/core/bro...
components/autofill/core/browser/autofill_external_delegate.h:158: bool
has_shown_address_book_prompt;
On 2014/09/18 21:26:37, Evan Stade wrote:
> nit: I would not bother with the ifdef because that adds a bit of cruft. Just
> notate that the var is only used on mac.

Done.

https://codereview.chromium.org/578383002/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/personal_data_manager_mac.mm (right):

https://codereview.chromium.org/578383002/diff/1/components/autofill/core/bro...
components/autofill/core/browser/personal_data_manager_mac.mm:36: int
kMaxTimesToShowMacAddressBookCount = 5;
On 2014/09/18 21:26:37, Evan Stade wrote:
> nit: "Times" and "Count" seem redundant

Changed to kMaxTimesToShowMacAddressBook

https://codereview.chromium.org/578383002/diff/1/components/autofill/core/bro...
components/autofill/core/browser/personal_data_manager_mac.mm:382:
kMaxTimesToShowMacAddressBookCount)
On 2014/09/18 21:26:37, Evan Stade wrote:
> nit: 4 more indent

Doesn't apply anymore, since conditional fits on a single line.

For reference, this indentation is the output of clang-format. It also matches
the indentation of the other conditionals in this file. e.g. lines 321-367 have
multiple examples of this.

https://codereview.chromium.org/578383002/diff/1/components/autofill/core/bro...
components/autofill/core/browser/personal_data_manager_mac.mm:383: return false;
On 2014/09/18 21:26:37, Evan Stade wrote:
> nit: curlies

N/A

https://codereview.chromium.org/578383002/diff/20001/components/autofill/core...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/578383002/diff/20001/components/autofill/core...
components/autofill/core/browser/autofill_external_delegate.cc:211: 30);
This number is intentionally larger than 5, and also intentionally independent.

[Evan Stade, 2014-09-18 22:06:09]

https://codereview.chromium.org/578383002/diff/20001/components/autofill/core...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/578383002/diff/20001/components/autofill/core...
components/autofill/core/browser/autofill_external_delegate.cc:209:
LOCAL_HISTOGRAM_ENUMERATION("Autofill.MacAddressBook.ShowedCount",
instead of ShowedCount, how about NumShowsBeforeSelected?

https://codereview.chromium.org/578383002/diff/20001/components/autofill/core...
File components/autofill/core/browser/personal_data_manager_mac.mm (right):

https://codereview.chromium.org/578383002/diff/20001/components/autofill/core...
components/autofill/core/browser/personal_data_manager_mac.mm:378: if
(HasPromptedForAccessToAddressBook(pref_service_))
this is pretty confusing function naming. HasPromptedForAccessToAddressBook
might be false, even though AccessAddressBookPromptCount is non zero? There must
be some better names to help distinguish between the suggestion item and the osx
dialog box.

[erikchen, 2014-09-18 22:49:50]

estade: PTAL

https://codereview.chromium.org/578383002/diff/20001/components/autofill/core...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/578383002/diff/20001/components/autofill/core...
components/autofill/core/browser/autofill_external_delegate.cc:209:
LOCAL_HISTOGRAM_ENUMERATION("Autofill.MacAddressBook.ShowedCount",
On 2014/09/18 22:06:09, Evan Stade wrote:
> instead of ShowedCount, how about NumShowsBeforeSelected?

Done.

https://codereview.chromium.org/578383002/diff/20001/components/autofill/core...
File components/autofill/core/browser/personal_data_manager_mac.mm (right):

https://codereview.chromium.org/578383002/diff/20001/components/autofill/core...
components/autofill/core/browser/personal_data_manager_mac.mm:378: if
(HasPromptedForAccessToAddressBook(pref_service_))
On 2014/09/18 22:06:09, Evan Stade wrote:
> this is pretty confusing function naming. HasPromptedForAccessToAddressBook
> might be false, even though AccessAddressBookPromptCount is non zero? There
must
> be some better names to help distinguish between the suggestion item and the
osx
> dialog box.

I went with the name of the preference, which uses the key word "query", and
avoids the word "prompt".

[Evan Stade, 2014-09-19 23:17:52]

lgtm

[erikchen, 2014-09-19 23:29:16]

erikchen@chromium.org changed reviewers:
+ isherman@chromium.org

[erikchen, 2014-09-19 23:29:16]

isherman: Looking for OWNER review of tools/metrics/histograms/histograms.xml

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

https://codereview.chromium.org/578383002/diff/40001/components/autofill/core...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/578383002/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_external_delegate.cc:209:
LOCAL_HISTOGRAM_ENUMERATION(
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?

https://codereview.chromium.org/578383002/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_external_delegate.cc:212: 30);
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]

https://codereview.chromium.org/578383002/diff/40001/components/autofill/core...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/578383002/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_external_delegate.cc:209:
LOCAL_HISTOGRAM_ENUMERATION(
On 2014/09/20 00:01:24, Ilya Sherman wrote:
> 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?

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.

https://codereview.chromium.org/578383002/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_external_delegate.cc:212: 30);
On 2014/09/20 00:01:24, Ilya Sherman wrote:
> 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.

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:44]

+cc Alexei for discussion about how to make LOCAL_ vs. UMA_ histograms even
clearer.

https://codereview.chromium.org/578383002/diff/40001/components/autofill/core...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/578383002/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_external_delegate.cc:209:
LOCAL_HISTOGRAM_ENUMERATION(
On 2014/09/20 00:36:05, erikchen wrote:
> On 2014/09/20 00:01:24, Ilya Sherman wrote:
> > 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?
> 
> 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.

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.

https://codereview.chromium.org/578383002/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_external_delegate.cc:212: 30);
On 2014/09/20 00:36:05, erikchen wrote:
> On 2014/09/20 00:01:24, Ilya Sherman wrote:
> > 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.
> 
> 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.

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]

isherman: PTAL

Diff patch set 5 against patch set 3.

https://codereview.chromium.org/578383002/diff/40001/components/autofill/core...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/578383002/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_external_delegate.cc:212: 30);
On 2014/09/20 00:44:43, Ilya Sherman wrote:
> On 2014/09/20 00:36:05, erikchen wrote:
> > On 2014/09/20 00:01:24, Ilya Sherman wrote:
> > > 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.
> > 
> > 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.
> 
> 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.

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.

[Ilya Sherman, 2014-09-20 01:04:19]

LGTM

[erikchen, 2014-09-20 01:04:40]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-09-20 01:06:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/578383002/80001

[commit-bot: I haz the power, 2014-09-20 01:06:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/578383002/80001

[commit-bot: I haz the power, 2014-09-20 03:08:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-20 03:08:04]

Exceeded time limit waiting for builds to trigger.

[Alexei Svitkine (slow), 2014-09-22 15:10:46]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

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

https://codereview.chromium.org/578383002/diff/40001/components/autofill/core...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/578383002/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_external_delegate.cc:209:
LOCAL_HISTOGRAM_ENUMERATION(
On 2014/09/20 00:44:43, Ilya Sherman wrote:
> On 2014/09/20 00:36:05, erikchen wrote:
> > On 2014/09/20 00:01:24, Ilya Sherman wrote:
> > > 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?
> > 
> > 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.
> 
> 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.

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

[erikchen, 2014-09-23 00:51:50]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-09-23 00:52:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/578383002/80001

[commit-bot: I haz the power, 2014-09-23 00:53:59]

Committed patchset #5 (id:80001) as b17df349625c5727fcd050b66d93f0eaa7012367

[commit-bot: I haz the power, 2014-09-23 00:54:38]

Patchset 5 (id:??) landed as
https://crrev.com/d90fe6dde92f70c6c133d97ae7e686c186060f1c
Cr-Commit-Position: refs/heads/master@{#296114}