Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(820)

Issue 1715763002: Add histograms for notification icons. (Closed)

Created:
4 years, 10 months ago by Michael van Ouwerkerk
Modified:
4 years, 10 months ago
CC:
chromium-reviews, darin-cc_chromium.org, mlamouri+watch-notifications_chromium.org, jam, asvitkine+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@NotificationIconScaling
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add histograms for notification icons. * Notifications.Icon.FileSize * Notifications.Icon.LoadFailTime * Notifications.Icon.LoadFinishTime * Notifications.Icon.ScaleDownTime BUG=586082, 579137 Committed: https://crrev.com/dc883ea2c71a5d8886091f4e773d29c537546f88 Cr-Commit-Position: refs/heads/master@{#377271}

Patch Set 1 #

Total comments: 6

Patch Set 2 : Address review feedback. #

Patch Set 3 : Rebase. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+47 lines, -0 lines) Patch
M content/child/notifications/notification_image_loader.h View 2 chunks +3 lines, -0 lines 0 comments Download
M content/child/notifications/notification_image_loader.cc View 1 5 chunks +13 lines, -0 lines 0 comments Download
M content/child/notifications/pending_notification.cc View 2 chunks +2 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 1 chunk +29 lines, -0 lines 0 comments Download

Messages

Total messages: 23 (10 generated)
Michael van Ouwerkerk
Peter, could you take a look please from a feature perspective? Ilya, could you take ...
4 years, 10 months ago (2016-02-19 16:53:56 UTC) #3
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1715763002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1715763002/1
4 years, 10 months ago (2016-02-19 16:58:46 UTC) #4
Ilya Sherman
Metrics LGTM % nit: https://codereview.chromium.org/1715763002/diff/1/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1715763002/diff/1/tools/metrics/histograms/histograms.xml#newcode31323 tools/metrics/histograms/histograms.xml:31323: + Notification. Does this count ...
4 years, 10 months ago (2016-02-19 17:09:43 UTC) #5
Peter Beverloo
notifications lgtm % one measure https://codereview.chromium.org/1715763002/diff/1/content/child/notifications/notification_image_loader.cc File content/child/notifications/notification_image_loader.cc (right): https://codereview.chromium.org/1715763002/diff/1/content/child/notifications/notification_image_loader.cc#newcode103 content/child/notifications/notification_image_loader.cc:103: 10000000 /* ~10mb */, ...
4 years, 10 months ago (2016-02-19 17:13:44 UTC) #6
Ilya Sherman
https://codereview.chromium.org/1715763002/diff/1/content/child/notifications/notification_image_loader.cc File content/child/notifications/notification_image_loader.cc (right): https://codereview.chromium.org/1715763002/diff/1/content/child/notifications/notification_image_loader.cc#newcode103 content/child/notifications/notification_image_loader.cc:103: 10000000 /* ~10mb */, 50); On 2016/02/19 17:13:44, Peter ...
4 years, 10 months ago (2016-02-19 17:16:59 UTC) #7
Peter Beverloo
On 2016/02/19 17:13:44, Peter Beverloo wrote: > notifications lgtm % one measure > > https://codereview.chromium.org/1715763002/diff/1/content/child/notifications/notification_image_loader.cc ...
4 years, 10 months ago (2016-02-19 17:17:30 UTC) #8
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 10 months ago (2016-02-19 18:17:12 UTC) #10
Michael van Ouwerkerk
Thanks for the reviews https://codereview.chromium.org/1715763002/diff/1/content/child/notifications/notification_image_loader.cc File content/child/notifications/notification_image_loader.cc (right): https://codereview.chromium.org/1715763002/diff/1/content/child/notifications/notification_image_loader.cc#newcode103 content/child/notifications/notification_image_loader.cc:103: 10000000 /* ~10mb */, 50); ...
4 years, 10 months ago (2016-02-22 17:24:32 UTC) #12
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1715763002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1715763002/20001
4 years, 10 months ago (2016-02-22 17:26:22 UTC) #14
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 10 months ago (2016-02-22 18:38:32 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1715763002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1715763002/40001
4 years, 10 months ago (2016-02-24 10:43:31 UTC) #19
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 10 months ago (2016-02-24 12:13:10 UTC) #21
commit-bot: I haz the power
4 years, 10 months ago (2016-02-24 12:14:18 UTC) #23
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/dc883ea2c71a5d8886091f4e773d29c537546f88
Cr-Commit-Position: refs/heads/master@{#377271}

Powered by Google App Engine
This is Rietveld 408576698