Cleanup desktop NTP metrics recording, part 2

Cleanup desktop NTP metrics recording, part 2
Followup to https://codereview.chromium.org/2429523002

- Remove NTP_*_SIDE_SUGGESTION (replaced by LogMostVisitedImpression calls)
- Cleanup tests

BUG=656646
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/76aad9a0600b6ac6357ef6d75b9b88d1e8daf8aa
Cr-Commit-Position: refs/heads/master@{#427086}

[Marc Treib, 2016-10-19 13:25:26]

Description was changed from

==========
Cleanup desktop NTP metrics recording, part 2

- Remove NTP_*_SIDE_SUGGESTION (replaced by LogMostVisitedImpression calls)
- Cleanup tests

BUG=656646
==========

to

==========
Cleanup desktop NTP metrics recording, part 2

- Remove NTP_*_SIDE_SUGGESTION (replaced by LogMostVisitedImpression calls)
- Cleanup tests

BUG=656646
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Marc Treib, 2016-10-19 13:26:49]

Description was changed from

==========
Cleanup desktop NTP metrics recording, part 2

- Remove NTP_*_SIDE_SUGGESTION (replaced by LogMostVisitedImpression calls)
- Cleanup tests

BUG=656646
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Cleanup desktop NTP metrics recording, part 2
Followup to https://codereview.chromium.org/2429523002

- Remove NTP_*_SIDE_SUGGESTION (replaced by LogMostVisitedImpression calls)
- Cleanup tests

BUG=656646
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Marc Treib, 2016-10-24 11:51:43]

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

[Marc Treib, 2016-10-24 11:51:44]

And here's part 2. PTAL!

https://codereview.chromium.org/2435683003/diff/1/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/2435683003/diff/1/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:139: number_of_tiles_++;
This preserves the previous behavior: set has_client/server and increment
number_of_tiles *before* checking if we already had an impression at this index.
Should we change that? Or, can you see any reason why we shouldn't?

https://codereview.chromium.org/2435683003/diff/1/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc (right):

https://codereview.chromium.org/2435683003/diff/1/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc:85: int expect_bin8
= GetBinCount("NewTabPage.NumberOfTiles", 8);
This is a bit ugly.
Histograms are set up in a way that they survive between unit tests, and
apparently there's no way to reset them. So I'm querying the count at the start
of the test, and then essentially EXPECT the diff to that.
The alternative to this would be to use the HistogramTester class, which does
essentially the same thing. Arguably that's less ugly, but the EXPECTs become a
bit weird (it comes with its own ExpectBinCount etc methods). WDYT?

[sfiera, 2016-10-24 13:01:16]

https://codereview.chromium.org/2435683003/diff/1/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/2435683003/diff/1/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:139: number_of_tiles_++;
On 2016/10/24 11:51:43, Marc Treib wrote:
> This preserves the previous behavior: set has_client/server and increment
> number_of_tiles *before* checking if we already had an impression at this
index.
> Should we change that? Or, can you see any reason why we shouldn't?

Does it matter? number_of_tiles_ is only relevant to EmitNtpStatistics(), which
runs only once for any particular NTP. We shouldn't see multiple impressions for
any particular index until we've already emitted the number_of_tiles_ stat.
(impression metrics are logged separately from EmitNtpStatistics(), which is why
there's separate behavior to avoid resending them)

Given that it shouldn't matter, I'd be inclined to move number_of_tiles++ below
the conditional, just because it feels better there. Could go either way about
has_*_suggestions_.

https://codereview.chromium.org/2435683003/diff/1/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc (right):

https://codereview.chromium.org/2435683003/diff/1/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc:85: int expect_bin8
= GetBinCount("NewTabPage.NumberOfTiles", 8);
On 2016/10/24 11:51:44, Marc Treib wrote:
> This is a bit ugly.
> Histograms are set up in a way that they survive between unit tests, and
> apparently there's no way to reset them. So I'm querying the count at the
start
> of the test, and then essentially EXPECT the diff to that.
> The alternative to this would be to use the HistogramTester class, which does
> essentially the same thing. Arguably that's less ugly, but the EXPECTs become
a
> bit weird (it comes with its own ExpectBinCount etc methods). WDYT?

You don't have to use its Expect*() methods, do you? My reading of the class
header is that you can do something like:

  EXPECT_THAT(
      histogram_tester.GetAllSamples("NewTabPage.NumberOfTiles"),
      ElementsAre(Bucket(8, 1)));

[Marc Treib, 2016-10-24 14:29:11]

https://codereview.chromium.org/2435683003/diff/1/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/2435683003/diff/1/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:139: number_of_tiles_++;
On 2016/10/24 13:01:16, sfiera wrote:
> On 2016/10/24 11:51:43, Marc Treib wrote:
> > This preserves the previous behavior: set has_client/server and increment
> > number_of_tiles *before* checking if we already had an impression at this
> index.
> > Should we change that? Or, can you see any reason why we shouldn't?
> 
> Does it matter? number_of_tiles_ is only relevant to EmitNtpStatistics(),
which
> runs only once for any particular NTP. We shouldn't see multiple impressions
for
> any particular index until we've already emitted the number_of_tiles_ stat.
> (impression metrics are logged separately from EmitNtpStatistics(), which is
why
> there's separate behavior to avoid resending them)

Exactly, it *shouldn't* matter. If for whatever reason, the JS was sending
multiple impressions for the same index, then the old code would count a larger
number of tiles.
Then again, we don't see evidence of that in UMA, so I suppose it's fine to
change. In fact, that lets us get rid of number_of_tiles_ completely, we can
just count the set bits in impression_was_logged_ :)

> Given that it shouldn't matter, I'd be inclined to move number_of_tiles++
below
> the conditional, just because it feels better there. Could go either way about
> has_*_suggestions_.

I've moved has_*_suggestions_ too, for consistency - it also shouldn't matter,
and no reason to keep this one thing separate.

https://codereview.chromium.org/2435683003/diff/1/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc (right):

https://codereview.chromium.org/2435683003/diff/1/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc:85: int expect_bin8
= GetBinCount("NewTabPage.NumberOfTiles", 8);
On 2016/10/24 13:01:16, sfiera wrote:
> On 2016/10/24 11:51:44, Marc Treib wrote:
> > This is a bit ugly.
> > Histograms are set up in a way that they survive between unit tests, and
> > apparently there's no way to reset them. So I'm querying the count at the
> start
> > of the test, and then essentially EXPECT the diff to that.
> > The alternative to this would be to use the HistogramTester class, which
does
> > essentially the same thing. Arguably that's less ugly, but the EXPECTs
become
> a
> > bit weird (it comes with its own ExpectBinCount etc methods). WDYT?
> 
> You don't have to use its Expect*() methods, do you? My reading of the class
> header is that you can do something like:
> 
>   EXPECT_THAT(
>       histogram_tester.GetAllSamples("NewTabPage.NumberOfTiles"),
>       ElementsAre(Bucket(8, 1)));

Right. Done.

[Marc Treib, 2016-10-24 14:29:21]

The CQ bit was checked by treib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 14:29:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sfiera, 2016-10-24 14:41:11]

LGTM

https://codereview.chromium.org/2435683003/diff/1/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/2435683003/diff/1/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:139: number_of_tiles_++;
On 2016/10/24 14:29:11, Marc Treib wrote:
> On 2016/10/24 13:01:16, sfiera wrote:
> > On 2016/10/24 11:51:43, Marc Treib wrote:
> > > This preserves the previous behavior: set has_client/server and increment
> > > number_of_tiles *before* checking if we already had an impression at this
> > index.
> > > Should we change that? Or, can you see any reason why we shouldn't?
> > 
> > Does it matter? number_of_tiles_ is only relevant to EmitNtpStatistics(),
> which
> > runs only once for any particular NTP. We shouldn't see multiple impressions
> for
> > any particular index until we've already emitted the number_of_tiles_ stat.
> > (impression metrics are logged separately from EmitNtpStatistics(), which is
> why
> > there's separate behavior to avoid resending them)
> 
> Exactly, it *shouldn't* matter. If for whatever reason, the JS was sending
> multiple impressions for the same index, then the old code would count a
larger
> number of tiles.
> Then again, we don't see evidence of that in UMA, so I suppose it's fine to
> change. In fact, that lets us get rid of number_of_tiles_ completely, we can
> just count the set bits in impression_was_logged_ :)

Even better!

[commit-bot: I haz the power, 2016-10-24 16:14:09]

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

[commit-bot: I haz the power, 2016-10-24 16:14:10]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2016-10-24 16:19:12]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2016-10-24 16:19:41]

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, 2016-10-24 16:36:10]

Description was changed from

==========
Cleanup desktop NTP metrics recording, part 2
Followup to https://codereview.chromium.org/2429523002

- Remove NTP_*_SIDE_SUGGESTION (replaced by LogMostVisitedImpression calls)
- Cleanup tests

BUG=656646
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Cleanup desktop NTP metrics recording, part 2
Followup to https://codereview.chromium.org/2429523002

- Remove NTP_*_SIDE_SUGGESTION (replaced by LogMostVisitedImpression calls)
- Cleanup tests

BUG=656646
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-10-24 16:36:11]

Committed patchset #3 (id:30001)

[commit-bot: I haz the power, 2016-10-24 16:50:04]

Description was changed from

==========
Cleanup desktop NTP metrics recording, part 2
Followup to https://codereview.chromium.org/2429523002

- Remove NTP_*_SIDE_SUGGESTION (replaced by LogMostVisitedImpression calls)
- Cleanup tests

BUG=656646
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Cleanup desktop NTP metrics recording, part 2
Followup to https://codereview.chromium.org/2429523002

- Remove NTP_*_SIDE_SUGGESTION (replaced by LogMostVisitedImpression calls)
- Cleanup tests

BUG=656646
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/76aad9a0600b6ac6357ef6d75b9b88d1e8daf8aa
Cr-Commit-Position: refs/heads/master@{#427086}
==========

[commit-bot: I haz the power, 2016-10-24 16:50:05]

Patchset 3 (id:??) landed as
https://crrev.com/76aad9a0600b6ac6357ef6d75b9b88d1e8daf8aa
Cr-Commit-Position: refs/heads/master@{#427086}