Desktop NTP metrics: Use ntp_tiles::metrics:: functions

Desktop NTP metrics: Use ntp_tiles::metrics:: functions

BUG=514752
Committed: https://crrev.com/6e3618a9b13520950396f9f223abf56c3e9cb5d8
Cr-Commit-Position: refs/heads/master@{#427337}

[Marc Treib, 2016-10-24 16:03:31]

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

[Marc Treib, 2016-10-24 16:03:31]

And another one!

[sfiera, 2016-10-25 12:12:11]

https://codereview.chromium.org/2429283003/diff/60001/components/ntp_tiles/me...
File components/ntp_tiles/metrics.h (right):

https://codereview.chromium.org/2429283003/diff/60001/components/ntp_tiles/me...
components/ntp_tiles/metrics.h:51: MostVisitedTileType tile_type);
Rather than splitting out a separate function for Desktop, wouldn't it make more
sense to add ICON_THUMBNAIL to the tile type enum and use that?

https://codereview.chromium.org/2429283003/diff/60001/components/ntp_tiles/mo...
File components/ntp_tiles/most_visited_sites.cc (right):

https://codereview.chromium.org/2429283003/diff/60001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.cc:402: int num_tiles =
static_cast<int>(current_tiles_.size());
Why not "size_t num_tiles", "size_t i"?

[Marc Treib, 2016-10-25 12:23:39]

https://codereview.chromium.org/2429283003/diff/60001/components/ntp_tiles/me...
File components/ntp_tiles/metrics.h (right):

https://codereview.chromium.org/2429283003/diff/60001/components/ntp_tiles/me...
components/ntp_tiles/metrics.h:51: MostVisitedTileType tile_type);
On 2016/10/25 12:12:11, sfiera wrote:
> Rather than splitting out a separate function for Desktop, wouldn't it make
more
> sense to add ICON_THUMBNAIL to the tile type enum and use that?

Hm. We could add it to the enum, but actually recording it doesn't really make
sense (it'll always be THUMBNAIL on desktop, and never on mobile). And then
RecordImpressionTileTypes would take one input that we'd never expect to
occur... hm. Might still be the better tradeoff? WDYT?

https://codereview.chromium.org/2429283003/diff/60001/components/ntp_tiles/mo...
File components/ntp_tiles/most_visited_sites.cc (right):

https://codereview.chromium.org/2429283003/diff/60001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.cc:402: int num_tiles =
static_cast<int>(current_tiles_.size());
On 2016/10/25 12:12:11, sfiera wrote:
> Why not "size_t num_tiles", "size_t i"?

Then I'll have to cast below, since RecordTileImpression wants an int for the
index (as it should IMO - we don't generally use size_t or unsigned types in
general).

[sfiera, 2016-10-25 12:42:45]

LGTM

https://codereview.chromium.org/2429283003/diff/60001/components/ntp_tiles/me...
File components/ntp_tiles/metrics.h (right):

https://codereview.chromium.org/2429283003/diff/60001/components/ntp_tiles/me...
components/ntp_tiles/metrics.h:51: MostVisitedTileType tile_type);
On 2016/10/25 12:23:39, Marc Treib wrote:
> On 2016/10/25 12:12:11, sfiera wrote:
> > Rather than splitting out a separate function for Desktop, wouldn't it make
> more
> > sense to add ICON_THUMBNAIL to the tile type enum and use that?
> 
> Hm. We could add it to the enum, but actually recording it doesn't really make
> sense (it'll always be THUMBNAIL on desktop, and never on mobile). And then
> RecordImpressionTileTypes would take one input that we'd never expect to
> occur... hm. Might still be the better tradeoff? WDYT?

I think I'd go for it. Personally, I think touch icons tend to be better than
thumbnails anyway and it might be worth experimenting with them on Desktop some
time in the future.

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

PTAL again!

https://codereview.chromium.org/2429283003/diff/60001/components/ntp_tiles/me...
File components/ntp_tiles/metrics.h (right):

https://codereview.chromium.org/2429283003/diff/60001/components/ntp_tiles/me...
components/ntp_tiles/metrics.h:51: MostVisitedTileType tile_type);
On 2016/10/25 12:42:45, sfiera wrote:
> On 2016/10/25 12:23:39, Marc Treib wrote:
> > On 2016/10/25 12:12:11, sfiera wrote:
> > > Rather than splitting out a separate function for Desktop, wouldn't it
make
> > more
> > > sense to add ICON_THUMBNAIL to the tile type enum and use that?
> > 
> > Hm. We could add it to the enum, but actually recording it doesn't really
make
> > sense (it'll always be THUMBNAIL on desktop, and never on mobile). And then
> > RecordImpressionTileTypes would take one input that we'd never expect to
> > occur... hm. Might still be the better tradeoff? WDYT?
> 
> I think I'd go for it. Personally, I think touch icons tend to be better than
> thumbnails anyway and it might be worth experimenting with them on Desktop
some
> time in the future.

Fun fact: A desktop icon NTP is actually mostly implemented - I can tell you
about it if you're interested :)

Anyway, I don't buy the "we might want this at some unspecified point in the
future" argument. If so, it's just a small CL away. Then again, I do like having
just one function for recording a click. So, done.

(In a later CL, I'll similarly merge RecordPageImpression and
RecordImpressionTileTypes.)

[Marc Treib, 2016-10-25 13:27:07]

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

[commit-bot: I haz the power, 2016-10-25 13:27:33]

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-25 13:38:32]

Still LGTM

[Marc Treib, 2016-10-25 13:50:55]

The CQ bit was unchecked by treib@chromium.org

[Marc Treib, 2016-10-25 13:50:55]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2016-10-25 13:51:04]

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-25 14:07:55]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-25 14:09:58]

Description was changed from

==========
Desktop NTP metrics: Use ntp_tiles::metrics:: functions

BUG=514752
==========

to

==========
Desktop NTP metrics: Use ntp_tiles::metrics:: functions

BUG=514752

Committed: https://crrev.com/6e3618a9b13520950396f9f223abf56c3e9cb5d8
Cr-Commit-Position: refs/heads/master@{#427337}
==========

[commit-bot: I haz the power, 2016-10-25 14:09:59]

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