Omnibox - Clipboard Provider - Record Number of Times Suggestion Offered

Omnibox - Clipboard Provider - Record Number of Times Suggestion Offered

BUG=718187
Review-Url: https://codereview.chromium.org/2904293002
Cr-Commit-Position: refs/heads/master@{#476453}
Committed: https://chromium.googlesource.com/chromium/src/+/ea5016a9460ba717ca21893de068f4d2a3f43746

[Mark P, 2017-05-30 23:31:13]

mpearson@chromium.org changed reviewers:
+ isherman@chromium.org, jif@chromium.org

[Mark P, 2017-05-30 23:31:14]

Hey folks,

Please take a look a when you get a chance:
- jif@ code, and histograms.xml file to whatever extent you want
- isherman@ histograms.xml, and the code to whatever extent you want

thanks,
mark

[jif, 2017-05-31 09:27:48]

https://codereview.chromium.org/2904293002/diff/60001/components/omnibox/brow...
File components/omnibox/browser/clipboard_url_provider.cc (right):

https://codereview.chromium.org/2904293002/diff/60001/components/omnibox/brow...
components/omnibox/browser/clipboard_url_provider.cc:27:
history_url_provider_(history_url_provider) {
add:
current_url_suggested_times_(0)

https://codereview.chromium.org/2904293002/diff/60001/components/omnibox/brow...
components/omnibox/browser/clipboard_url_provider.cc:100: void
ClipboardURLProvider::AddProviderInfo(ProvidersInfo* provider_info) const {
Can we pass |provider_info| by reference? This would make it clear that
|provider_info| can't be null.
This can of course be done in a separate CL.

[Ilya Sherman, 2017-05-31 23:40:23]

Metrics LGTM % nits:

https://codereview.chromium.org/2904293002/diff/60001/components/omnibox/brow...
File components/omnibox/browser/clipboard_url_provider.cc (right):

https://codereview.chromium.org/2904293002/diff/60001/components/omnibox/brow...
components/omnibox/browser/clipboard_url_provider.cc:80:
current_url_suggested_times_, 20);
nit: Is this better than UMA_HISTOGRAM_SPARSE_SLOWLY with explicit capping of
the recorded value?

https://codereview.chromium.org/2904293002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2904293002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:46190: +    the third time it is shown,
we'll emit to a three to this histogram, and
nit: s/to a three/a three

[Mark P, 2017-05-31 23:41:52]

https://codereview.chromium.org/2904293002/diff/60001/components/omnibox/brow...
File components/omnibox/browser/clipboard_url_provider.cc (right):

https://codereview.chromium.org/2904293002/diff/60001/components/omnibox/brow...
components/omnibox/browser/clipboard_url_provider.cc:27:
history_url_provider_(history_url_provider) {
On 2017/05/31 09:27:47, jif wrote:
> add:
> current_url_suggested_times_(0)

Good catch.  I'd thought it should be initialized automatically. Apparently I'm
wrong.
https://stackoverflow.com/a/2218275

https://codereview.chromium.org/2904293002/diff/60001/components/omnibox/brow...
components/omnibox/browser/clipboard_url_provider.cc:100: void
ClipboardURLProvider::AddProviderInfo(ProvidersInfo* provider_info) const {
On 2017/05/31 09:27:47, jif wrote:
> Can we pass |provider_info| by reference? This would make it clear that
> |provider_info| can't be null.
> This can of course be done in a separate CL.

This is an inherited function from the base class.  It would be a pain to chance
the interface and everywhere that uses it for relatively little gain.

So, I'll punt this to a separate changelist, then probably postpone the
changelist indefinitely. /honesty  :-)

[Mark P, 2017-06-01 04:16:33]

Hey folks, please take another look if you want.

thanks,
mark

https://codereview.chromium.org/2904293002/diff/60001/components/omnibox/brow...
File components/omnibox/browser/clipboard_url_provider.cc (right):

https://codereview.chromium.org/2904293002/diff/60001/components/omnibox/brow...
components/omnibox/browser/clipboard_url_provider.cc:80:
current_url_suggested_times_, 20);
On 2017/05/31 23:40:23, Ilya Sherman wrote:
> nit: Is this better than UMA_HISTOGRAM_SPARSE_SLOWLY with explicit capping of
> the recorded value?

No, not really.  Changed.

Note that I had to use a static_cast (ugh) because I couldn't figure out a
size_t constant that works on all platforms.  (E.g., 20ul works on linux but not
android; 20u works on android but not linux.)

https://codereview.chromium.org/2904293002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2904293002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:46190: +    the third time it is shown,
we'll emit to a three to this histogram, and
On 2017/05/31 23:40:23, Ilya Sherman wrote:
> nit: s/to a three/a three

Done.

[jif, 2017-06-01 06:51:13]

lgtm

[Mark P, 2017-06-01 20:38:07]

The CQ bit was checked by mpearson@chromium.org

[Mark P, 2017-06-01 20:38:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2904293002/#ps100001
(title: "isherman's comments")

[commit-bot: I haz the power, 2017-06-01 20:38:37]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 22:20:35]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496349487531270,
"parent_rev": "4ca61512813b424e238dd5dc546d3f4b92991023", "commit_rev":
"ea5016a9460ba717ca21893de068f4d2a3f43746"}

[commit-bot: I haz the power, 2017-06-01 22:20:49]

Description was changed from

==========
Omnibox - Clipboard Provider - Record Number of Times Suggestion Offered

BUG=718187
==========

to

==========
Omnibox - Clipboard Provider - Record Number of Times Suggestion Offered

BUG=718187

Review-Url: https://codereview.chromium.org/2904293002
Cr-Commit-Position: refs/heads/master@{#476453}
Committed:
https://chromium.googlesource.com/chromium/src/+/ea5016a9460ba717ca21893de068...
==========

[commit-bot: I haz the power, 2017-06-01 22:20:51]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/ea5016a9460ba717ca21893de068...