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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months ago by mastiz
Modified:
3 months, 4 weeks 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
4 months 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 ...
4 months 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 ...
4 months 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
4 months 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 ...
4 months 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 ...
4 months 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: > > > ...
4 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 ...
3 months, 4 weeks 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 months, 4 weeks 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 months, 4 weeks 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 months, 4 weeks 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 months, 4 weeks ago (2017-04-26 08:17:29 UTC) #19
Marc Treib
Drive-by owners LGTM. Thanks for fixing this!
3 months, 4 weeks 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 months, 4 weeks ago (2017-04-26 10:26:29 UTC) #23
commit-bot: I haz the power
3 months, 4 weeks 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 b40b6558b