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

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

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