|
|
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. |
DescriptionAdd 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. #
Messages
Total messages: 23 (10 generated)
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
mvanouwerkerk@chromium.org changed reviewers: + isherman@chromium.org, peter@chromium.org
Peter, could you take a look please from a feature perspective? Ilya, could you take a look please as histogram owner?
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
Metrics LGTM % nit: https://codereview.chromium.org/1715763002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1715763002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:31323: + Notification. Does this count only successful loads, or all loads? Please clarify in the description text.
notifications lgtm % one measure https://codereview.chromium.org/1715763002/diff/1/content/child/notifications... File content/child/notifications/notification_image_loader.cc (right): https://codereview.chromium.org/1715763002/diff/1/content/child/notifications... content/child/notifications/notification_image_loader.cc:103: 10000000 /* ~10mb */, 50); Perhaps this would be better named "FileSize" to distinguish from "ImageSize"? We have 50 buckets from 0 to 10000000 bytes, which means that buckets increment in steps of 200KB. It seems to me that most images, especially icons, will be stuck in the first few buckets? I think a more reasonable range would be from 0 to 1000000 bytes (1MB), which happens to be provided by the UMA_HISTOGRAM_COUNTS macro. This gives us 20KB increments. (If the overflow bucket is large the next steps will most likely be in the realms of dev advocacy :-).)
https://codereview.chromium.org/1715763002/diff/1/content/child/notifications... File content/child/notifications/notification_image_loader.cc (right): https://codereview.chromium.org/1715763002/diff/1/content/child/notifications... content/child/notifications/notification_image_loader.cc:103: 10000000 /* ~10mb */, 50); On 2016/02/19 17:13:44, Peter Beverloo wrote: > Perhaps this would be better named "FileSize" to distinguish from "ImageSize"? > > We have 50 buckets from 0 to 10000000 bytes, which means that buckets increment > in steps of 200KB. It seems to me that most images, especially icons, will be > stuck in the first few buckets? > > I think a more reasonable range would be from 0 to 1000000 bytes (1MB), which > happens to be provided by the UMA_HISTOGRAM_COUNTS macro. This gives us 20KB > increments. > > (If the overflow bucket is large the next steps will most likely be in the > realms of dev advocacy :-).) Note that histogram buckets are exponentially sized, rather than linearly sized. So, you should still have relatively fine resolution at the low end, even with a 10MB upper bound. +1 to the "FileSize" suggestion.
On 2016/02/19 17:13:44, Peter Beverloo wrote: > notifications lgtm % one measure > > https://codereview.chromium.org/1715763002/diff/1/content/child/notifications... > File content/child/notifications/notification_image_loader.cc (right): > > https://codereview.chromium.org/1715763002/diff/1/content/child/notifications... > content/child/notifications/notification_image_loader.cc:103: 10000000 /* ~10mb > */, 50); > Perhaps this would be better named "FileSize" to distinguish from "ImageSize"? > > We have 50 buckets from 0 to 10000000 bytes, which means that buckets increment > in steps of 200KB. It seems to me that most images, especially icons, will be > stuck in the first few buckets? > > I think a more reasonable range would be from 0 to 1000000 bytes (1MB), which > happens to be provided by the UMA_HISTOGRAM_COUNTS macro. This gives us 20KB > increments. > > (If the overflow bucket is large the next steps will most likely be in the > realms of dev advocacy :-).) Actually, the scaling mechanism for bucket sizes may take care of this. https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo... I don't know. Ilya, would 0-10MB work fine if we're expecting most values to be low?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add histograms for notification icons. * Notifications.Icon.LoadFailTime * Notifications.Icon.LoadFinishTime * Notifications.Icon.ScaleDownTime * Notifications.Icon.Size BUG=586082,579137 ========== to ========== Add histograms for notification icons. * Notifications.Icon.FileSize * Notifications.Icon.LoadFailTime * Notifications.Icon.LoadFinishTime * Notifications.Icon.ScaleDownTime BUG=586082,579137 ==========
Thanks for the reviews https://codereview.chromium.org/1715763002/diff/1/content/child/notifications... File content/child/notifications/notification_image_loader.cc (right): https://codereview.chromium.org/1715763002/diff/1/content/child/notifications... content/child/notifications/notification_image_loader.cc:103: 10000000 /* ~10mb */, 50); On 2016/02/19 17:13:44, Peter Beverloo wrote: > Perhaps this would be better named "FileSize" to distinguish from "ImageSize"? > > We have 50 buckets from 0 to 10000000 bytes, which means that buckets increment > in steps of 200KB. It seems to me that most images, especially icons, will be > stuck in the first few buckets? > > I think a more reasonable range would be from 0 to 1000000 bytes (1MB), which > happens to be provided by the UMA_HISTOGRAM_COUNTS macro. This gives us 20KB > increments. > > (If the overflow bucket is large the next steps will most likely be in the > realms of dev advocacy :-).) Renamed to "FileSize". https://codereview.chromium.org/1715763002/diff/1/content/child/notifications... content/child/notifications/notification_image_loader.cc:103: 10000000 /* ~10mb */, 50); On 2016/02/19 17:16:58, Ilya Sherman wrote: > On 2016/02/19 17:13:44, Peter Beverloo wrote: > > Perhaps this would be better named "FileSize" to distinguish from "ImageSize"? > > > > We have 50 buckets from 0 to 10000000 bytes, which means that buckets > increment > > in steps of 200KB. It seems to me that most images, especially icons, will be > > stuck in the first few buckets? > > > > I think a more reasonable range would be from 0 to 1000000 bytes (1MB), which > > happens to be provided by the UMA_HISTOGRAM_COUNTS macro. This gives us 20KB > > increments. > > > > (If the overflow bucket is large the next steps will most likely be in the > > realms of dev advocacy :-).) > > Note that histogram buckets are exponentially sized, rather than linearly sized. > So, you should still have relatively fine resolution at the low end, even with > a 10MB upper bound. > > +1 to the "FileSize" suggestion. Done. https://codereview.chromium.org/1715763002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1715763002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:31323: + Notification. On 2016/02/19 17:09:43, Ilya Sherman wrote: > Does this count only successful loads, or all loads? Please clarify in the > description text. This one is for the successful ones. Done.
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mvanouwerkerk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1715763002/#ps40001 (title: "Rebase.")
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
Message was sent while issue was closed.
Description was changed from ========== Add histograms for notification icons. * Notifications.Icon.FileSize * Notifications.Icon.LoadFailTime * Notifications.Icon.LoadFinishTime * Notifications.Icon.ScaleDownTime BUG=586082,579137 ========== to ========== Add histograms for notification icons. * Notifications.Icon.FileSize * Notifications.Icon.LoadFailTime * Notifications.Icon.LoadFinishTime * Notifications.Icon.ScaleDownTime BUG=586082,579137 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add histograms for notification icons. * Notifications.Icon.FileSize * Notifications.Icon.LoadFailTime * Notifications.Icon.LoadFinishTime * Notifications.Icon.ScaleDownTime BUG=586082,579137 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/dc883ea2c71a5d8886091f4e773d29c537546f88 Cr-Commit-Position: refs/heads/master@{#377271} |