Popular sites on the NTP: Extend UMA histograms that track clicks and impressions

Popular sites on the NTP: Extend UMA histograms that track clicks and impressions

BUG=516618
Committed: https://crrev.com/783a337e036f3ea631819527370ef32fa9ab406c
Cr-Commit-Position: refs/heads/master@{#345346}

[Marc Treib, 2015-08-21 13:08:50]

treib@chromium.org changed reviewers:
+ bauerb@chromium.org

[Marc Treib, 2015-08-21 13:08:51]

bauerb: PTAL!
fserb/knn: FYI

[Marc Treib, 2015-08-21 13:12:25]

https://codereview.chromium.org/1302363002/diff/1/chrome/browser/android/most...
File chrome/browser/android/most_visited_sites.cc (left):

https://codereview.chromium.org/1302363002/diff/1/chrome/browser/android/most...
chrome/browser/android/most_visited_sites.cc:297:
UMA_HISTOGRAM_SPARSE_SLOWLY(kOpenedItemClientHistogramName, index);
Some of the histograms used to be implemented as sparse histograms, and are now
regular linear histograms. I don't know why they were sparse in the first place,
seems pointless for a histogram with ~8 entries...

[Bernhard Bauer, 2015-08-25 13:55:10]

LGTM w/ nits:

https://codereview.chromium.org/1302363002/diff/20001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/1302363002/diff/20001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:89: void LogHistogramEvent(const
std::string& histogram, int position,
|position| should go on the next line, BTW (for method signatures the rules are
a bit more strict than for call sites: either all parameters fit on a single
line, or one parameter per line).

https://codereview.chromium.org/1302363002/diff/20001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.h (right):

https://codereview.chromium.org/1302363002/diff/20001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.h:138: bool
received_most_visited_sites_;
Can you add an empty line before each comment?

https://codereview.chromium.org/1302363002/diff/20001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites_unittest.cc (right):

https://codereview.chromium.org/1302363002/diff/20001/chrome/browser/android/...
chrome/browser/android/most_visited_sites_unittest.cc:154: // The personal sites
should replace the popular ones, but theorder of the
Nit: "the order"

[Marc Treib, 2015-08-25 14:13:23]

https://codereview.chromium.org/1302363002/diff/20001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/1302363002/diff/20001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:89: void LogHistogramEvent(const
std::string& histogram, int position,
On 2015/08/25 13:55:10, Bernhard Bauer wrote:
> |position| should go on the next line, BTW (for method signatures the rules
are
> a bit more strict than for call sites: either all parameters fit on a single
> line, or one parameter per line).

Done.

https://codereview.chromium.org/1302363002/diff/20001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.h (right):

https://codereview.chromium.org/1302363002/diff/20001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.h:138: bool
received_most_visited_sites_;
On 2015/08/25 13:55:10, Bernhard Bauer wrote:
> Can you add an empty line before each comment?

Done.

https://codereview.chromium.org/1302363002/diff/20001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites_unittest.cc (right):

https://codereview.chromium.org/1302363002/diff/20001/chrome/browser/android/...
chrome/browser/android/most_visited_sites_unittest.cc:154: // The personal sites
should replace the popular ones, but theorder of the
On 2015/08/25 13:55:10, Bernhard Bauer wrote:
> Nit: "the order"

Done.

[Marc Treib, 2015-08-25 14:13:47]

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

[Marc Treib, 2015-08-25 14:13:48]

+asvitkine for histograms.xml. PTAL!

[Alexei Svitkine (slow), 2015-08-25 15:01:19]

lgtm % nits

https://codereview.chromium.org/1302363002/diff/40001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/1302363002/diff/40001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:294: tile_sources_[index].c_str());
Nit: Add a DCHECK() that index is within valid range.

https://codereview.chromium.org/1302363002/diff/40001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:377: if
(suggestion.providers_size()) {
Nit: != 0

https://codereview.chromium.org/1302363002/diff/40001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.h (right):

https://codereview.chromium.org/1302363002/diff/40001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.h:92: std::vector<std::string>*
tile_sources,
Nit: Output parameters should be last.

[Marc Treib, 2015-08-25 15:19:33]

https://codereview.chromium.org/1302363002/diff/40001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/1302363002/diff/40001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:294: tile_sources_[index].c_str());
On 2015/08/25 15:01:19, Alexei Svitkine (OOO Aug15-24) wrote:
> Nit: Add a DCHECK() that index is within valid range.

Done.

https://codereview.chromium.org/1302363002/diff/40001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:377: if
(suggestion.providers_size()) {
On 2015/08/25 15:01:19, Alexei Svitkine (OOO Aug15-24) wrote:
> Nit: != 0

Done.

https://codereview.chromium.org/1302363002/diff/40001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.h (right):

https://codereview.chromium.org/1302363002/diff/40001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.h:92: std::vector<std::string>*
tile_sources,
On 2015/08/25 15:01:19, Alexei Svitkine (OOO Aug15-24) wrote:
> Nit: Output parameters should be last.

Done.

[Marc Treib, 2015-08-25 15:19:49]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-08-25 15:19:49]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1302363002/#ps60001
(title: "asvitkine review")

[commit-bot: I haz the power, 2015-08-25 15:20:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1302363002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1302363002/60001

[commit-bot: I haz the power, 2015-08-25 16:24:46]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-25 16:25:49]

Patchset 4 (id:??) landed as
https://crrev.com/783a337e036f3ea631819527370ef32fa9ab406c
Cr-Commit-Position: refs/heads/master@{#345346}