Chromium Code Reviews
Help | Chromium Project | Sign in
(28)

Issue 2832173002: NTP: Fix TileType metrics on desktop (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 1 day ago by mastiz
Modified:
3 days, 12 hours ago
CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, asvitkine+watch_chromium.org, donnd+watch_chromium.org, jfweitz+watch_chromium.org, David Black, arv+watch_chromium.org, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

NTP: Fix TileType metrics on desktop Follow-up to https://codereview.chromium.org/2796643002 where the metrics were added but an inconsistency was introduced between the C++ enum and the Javascript counterpart. This caused the wrong buckets being counted for NewTabPage.TileType (ThumbnailLocal instead Thumbnail; ThumbnailServer instead of ThumbnailFailed). The affected buckets were already deprecated (DeprecatedThumbnailLocal and DeprecatedThumbnailServer) and hence, luckily, there are no conflated buckets. Besides, histograms.xml was missing the new suffixes for NewTabPageIconTypes, used to provide desktop-specific breakdowns for NewTabPage.MostVisited and NewTabPage.SuggestionsImpression. BUG=714095, 703165 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2832173002 Cr-Commit-Position: refs/heads/master@{#467287} Committed: https://chromium.googlesource.com/chromium/src/+/79eaaf7eefb10a3006da5d1a56007e8c6f72ffe1

Patch Set 1 #

Total comments: 1

Messages

Total messages: 26 (11 generated)
mastiz
1 week, 1 day ago (2017-04-21 10:52:38 UTC) #3
sfiera
LGTM Seems our tests weren't good enough to catch this. Have you… * …verified the ...
1 week, 1 day ago (2017-04-21 11:06:30 UTC) #4
mastiz
On 2017/04/21 11:06:30, sfiera wrote: > LGTM > > Seems our tests weren't good enough ...
1 week, 1 day ago (2017-04-21 11:46:57 UTC) #6
mastiz
jeremycho@chromium.org: Please review changes in most_visited_single.js isherman@chromium.org: Please review changes in histograms.xml
1 week, 1 day ago (2017-04-21 11:56:39 UTC) #8
Ilya Sherman
https://codereview.chromium.org/2832173002/diff/1/chrome/browser/resources/local_ntp/most_visited_single.js File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/2832173002/diff/1/chrome/browser/resources/local_ntp/most_visited_single.js#newcode55 chrome/browser/resources/local_ntp/most_visited_single.js:55: THUMBNAIL_FAILED: 8, I'm confused about why there is a ...
1 week, 1 day ago (2017-04-21 19:58:38 UTC) #9
mastiz
On 2017/04/21 19:58:38, Ilya Sherman wrote: > https://codereview.chromium.org/2832173002/diff/1/chrome/browser/resources/local_ntp/most_visited_single.js > File chrome/browser/resources/local_ntp/most_visited_single.js (right): > > https://codereview.chromium.org/2832173002/diff/1/chrome/browser/resources/local_ntp/most_visited_single.js#newcode55 ...
5 days, 15 hours ago (2017-04-24 07:50:53 UTC) #10
Ilya Sherman
On 2017/04/24 07:50:53, mastiz wrote: > On 2017/04/21 19:58:38, Ilya Sherman wrote: > > > ...
5 days, 3 hours ago (2017-04-24 19:31:50 UTC) #11
mastiz
On 2017/04/24 19:31:50, Ilya Sherman wrote: > On 2017/04/24 07:50:53, mastiz wrote: > > On ...
4 days, 13 hours ago (2017-04-25 09:55:16 UTC) #12
Ilya Sherman
I'm also not coming up with a great way to share an enum between JS ...
3 days, 23 hours ago (2017-04-25 23:49:13 UTC) #13
mastiz
On 2017/04/25 23:49:13, Ilya Sherman wrote: > I'm also not coming up with a great ...
3 days, 15 hours ago (2017-04-26 08:10:15 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2832173002/1
3 days, 15 hours ago (2017-04-26 08:10:35 UTC) #17
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/420669)
3 days, 15 hours ago (2017-04-26 08:17:29 UTC) #19
Marc Treib
Drive-by owners LGTM. Thanks for fixing this!
3 days, 13 hours ago (2017-04-26 09:29:07 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2832173002/1
3 days, 12 hours ago (2017-04-26 10:26:29 UTC) #23
commit-bot: I haz the power
3 days, 12 hours ago (2017-04-26 10:30:40 UTC) #26
Message was sent while issue was closed.
Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/79eaaf7eefb10a3006da5d1a5600...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46