Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1103)

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

Created:
1 year, 1 month ago by mastiz
Modified:
1 year, 1 month 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+6 lines, -2 lines) Patch
M chrome/browser/resources/local_ntp/most_visited_single.js View 1 chunk +2 lines, -2 lines 1 comment Download
M tools/metrics/histograms/histograms.xml View 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 26 (11 generated)
mastiz
1 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month 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 ...
1 year, 1 month 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: > > > ...
1 year, 1 month 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 ...
1 year, 1 month 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 ...
1 year, 1 month 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 ...
1 year, 1 month 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
1 year, 1 month 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)
1 year, 1 month ago (2017-04-26 08:17:29 UTC) #19
Marc Treib
Drive-by owners LGTM. Thanks for fixing this!
1 year, 1 month 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
1 year, 1 month ago (2017-04-26 10:26:29 UTC) #23
commit-bot: I haz the power
1 year, 1 month 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...

Powered by Google App Engine
This is Rietveld 408576698