Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2151413002:
Add a UMA histogram for a user's device scale at startup. (Closed)
DescriptionAdd a UMA histogram for a user's device scale at startup.
BUG=628038
Committed: https://crrev.com/6b4e59fe3f30bd3d6e035b552facd81a607b6d80
Cr-Commit-Position: refs/heads/master@{#406694}
Patch Set 1 #
Total comments: 4
Patch Set 2 : move metric to dpi.cc #
Total comments: 4
Patch Set 3 : minor edits #Patch Set 4 : use screen_win instead #Patch Set 5 : revert dpi.cc #
Total comments: 2
Patch Set 6 : use vector #
Total comments: 2
Patch Set 7 : edit comment #
Messages
Total messages: 27 (7 generated)
Description was changed from ========== Add a UMA histogram for a user's device scale at startup. BUG=628038 ========== to ========== Add a UMA histogram for a user's device scale at startup. BUG=628038 ==========
bsep@chromium.org changed reviewers: + isherman@chromium.org, oshima@chromium.org
https://codereview.chromium.org/2151413002/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2151413002/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:282: 1000.f); nit: I think you want 0 and 1000 instead of 0.f and 1000.f, since the final result is an int. https://codereview.chromium.org/2151413002/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:285: #endif Could you please find a different location for recording this metric? chrome_browser_main_extra_parts_metrics isn't intended for reporting all kinds of unrelated metrics; otherwise, it'd end up an overwhelmingly large, hairy file.
https://codereview.chromium.org/2151413002/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2151413002/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:282: 1000.f); On 2016/07/16 00:43:10, Ilya Sherman wrote: > nit: I think you want 0 and 1000 instead of 0.f and 1000.f, since the final > result is an int. std::floor returns a float and std::min/max require their arguments to be the same type. Doing this seemed slightly less silly than doing static_cast<int>(std::floor(etc)) because the value is still unclamped. https://codereview.chromium.org/2151413002/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:285: #endif On 2016/07/16 00:43:10, Ilya Sherman wrote: > Could you please find a different location for recording this metric? > chrome_browser_main_extra_parts_metrics isn't intended for reporting all kinds > of unrelated metrics; otherwise, it'd end up an overwhelmingly large, hairy > file. Done. Moved into dpi.cc initialization.
LGTM, thanks. https://codereview.chromium.org/2151413002/diff/20001/ui/display/win/dpi.cc File ui/display/win/dpi.cc (right): https://codereview.chromium.org/2151413002/diff/20001/ui/display/win/dpi.cc#n... ui/display/win/dpi.cc:11: #include "base/metrics/histogram.h" nit: You shouldn't need this include -- just the sparse_histogram include. https://codereview.chromium.org/2151413002/diff/20001/ui/display/win/dpi.cc#n... ui/display/win/dpi.cc:54: std::max(std::floor(GetUnforcedDeviceScaleFactor() * 100), 0.f), 1000.f); nit: Somewhere, please add some explicit code to convert from float to int, probably using base::checked_cast<>.
https://codereview.chromium.org/2151413002/diff/20001/ui/display/win/dpi.cc File ui/display/win/dpi.cc (right): https://codereview.chromium.org/2151413002/diff/20001/ui/display/win/dpi.cc#n... ui/display/win/dpi.cc:11: #include "base/metrics/histogram.h" On 2016/07/16 01:31:24, Ilya Sherman wrote: > nit: You shouldn't need this include -- just the sparse_histogram include. Done. https://codereview.chromium.org/2151413002/diff/20001/ui/display/win/dpi.cc#n... ui/display/win/dpi.cc:54: std::max(std::floor(GetUnforcedDeviceScaleFactor() * 100), 0.f), 1000.f); On 2016/07/16 01:31:24, Ilya Sherman wrote: > nit: Somewhere, please add some explicit code to convert from float to int, > probably using base::checked_cast<>. Done.
oshima@chromium.org changed reviewers: + robliao@chromium.org
I think you also need to do this for primary in GetMonitorScaleFactor (in screen_win.cc). +robliao@ who's driving this on windows.
On 2016/07/18 17:53:41, oshima wrote: > I think you also need to do this for primary in GetMonitorScaleFactor (in > screen_win.cc). +robliao@ who's driving this on windows. I THINK this is already fetching for the primary display, since it's passing NULL for the hwnd, which is what we want. But I'm not sure.
On 2016/07/18 17:58:21, Bret Sepulveda wrote: > On 2016/07/18 17:53:41, oshima wrote: > > I think you also need to do this for primary in GetMonitorScaleFactor (in > > screen_win.cc). +robliao@ who's driving this on windows. > > I THINK this is already fetching for the primary display, since it's passing > NULL for the hwnd, which is what we want. But I'm not sure. What is this histogram intended to measure? If the user has multiple devices with differing device scales, then we have multiple answers to consider here. GetDPIScale will not be the source of truth in environments that support per-monitor DPI.
On 2016/07/18 18:30:21, robliao wrote: > On 2016/07/18 17:58:21, Bret Sepulveda wrote: > > On 2016/07/18 17:53:41, oshima wrote: > > > I think you also need to do this for primary in GetMonitorScaleFactor (in > > > screen_win.cc). +robliao@ who's driving this on windows. > > > > I THINK this is already fetching for the primary display, since it's passing > > NULL for the hwnd, which is what we want. But I'm not sure. > > What is this histogram intended to measure? > > If the user has multiple devices with differing device scales, then we have > multiple answers to consider here. > GetDPIScale will not be the source of truth in environments that support > per-monitor DPI. Hmm ok after thinking about it we probably want to log once for each display instead, because only logging the primary monitor's DPI will ultimately end up being misleading about the population. So I'll move the metric to somewhere in ScreenWin that's appropriate, maybe Initialize()?
On 2016/07/18 20:19:40, Bret Sepulveda wrote: > On 2016/07/18 18:30:21, robliao wrote: > > On 2016/07/18 17:58:21, Bret Sepulveda wrote: > > > On 2016/07/18 17:53:41, oshima wrote: > > > > I think you also need to do this for primary in GetMonitorScaleFactor (in > > > > screen_win.cc). +robliao@ who's driving this on windows. > > > > > > I THINK this is already fetching for the primary display, since it's passing > > > NULL for the hwnd, which is what we want. But I'm not sure. > > > > What is this histogram intended to measure? > > > > If the user has multiple devices with differing device scales, then we have > > multiple answers to consider here. > > GetDPIScale will not be the source of truth in environments that support > > per-monitor DPI. > > Hmm ok after thinking about it we probably want to log once for each display > instead, because only logging the primary monitor's DPI will ultimately end up > being misleading about the population. So I'll move the metric to somewhere in > ScreenWin that's appropriate, maybe Initialize()? If you only want startup, that's the place to be. If you also want to capture display changes, ScreenWin::UpdateFromDisplayInfos might be a better choice.
On 2016/07/18 20:19:40, Bret Sepulveda wrote: > On 2016/07/18 18:30:21, robliao wrote: > > On 2016/07/18 17:58:21, Bret Sepulveda wrote: > > > On 2016/07/18 17:53:41, oshima wrote: > > > > I think you also need to do this for primary in GetMonitorScaleFactor (in > > > > screen_win.cc). +robliao@ who's driving this on windows. > > > > > > I THINK this is already fetching for the primary display, since it's passing > > > NULL for the hwnd, which is what we want. But I'm not sure. > > > > What is this histogram intended to measure? > > > > If the user has multiple devices with differing device scales, then we have > > multiple answers to consider here. > > GetDPIScale will not be the source of truth in environments that support > > per-monitor DPI. > > Hmm ok after thinking about it we probably want to log once for each display > instead, because only logging the primary monitor's DPI will ultimately end up > being misleading about the population. So I'll move the metric to somewhere in > ScreenWin that's appropriate, maybe Initialize()? FWIW, if you make this change, please also update the metric's description in histograms.xml, accordingly.
On 2016/07/18 23:01:19, Ilya Sherman wrote: > On 2016/07/18 20:19:40, Bret Sepulveda wrote: > > On 2016/07/18 18:30:21, robliao wrote: > > > On 2016/07/18 17:58:21, Bret Sepulveda wrote: > > > > On 2016/07/18 17:53:41, oshima wrote: > > > > > I think you also need to do this for primary in GetMonitorScaleFactor > (in > > > > > screen_win.cc). +robliao@ who's driving this on windows. > > > > > > > > I THINK this is already fetching for the primary display, since it's > passing > > > > NULL for the hwnd, which is what we want. But I'm not sure. > > > > > > What is this histogram intended to measure? > > > > > > If the user has multiple devices with differing device scales, then we have > > > multiple answers to consider here. > > > GetDPIScale will not be the source of truth in environments that support > > > per-monitor DPI. > > > > Hmm ok after thinking about it we probably want to log once for each display > > instead, because only logging the primary monitor's DPI will ultimately end up > > being misleading about the population. So I'll move the metric to somewhere in > > ScreenWin that's appropriate, maybe Initialize()? > > FWIW, if you make this change, please also update the metric's description in > histograms.xml, accordingly. Multiple monitors now accounted for and the histogram description is updated to reflect that.
metrics lgtm
https://codereview.chromium.org/2151413002/diff/80001/ui/display/win/screen_w... File ui/display/win/screen_win.cc (right): https://codereview.chromium.org/2151413002/diff/80001/ui/display/win/screen_w... ui/display/win/screen_win.cc:555: std::set<int> already_reported_scale_factors; Go with an unordered_set or a vector (n is small enough so that vector will outperform most hash based implementations on time and space). n is also small enough where I would be okay with a vector of two passes: once to gather the unique scale factors and the next to report them.
https://codereview.chromium.org/2151413002/diff/80001/ui/display/win/screen_w... File ui/display/win/screen_win.cc (right): https://codereview.chromium.org/2151413002/diff/80001/ui/display/win/screen_w... ui/display/win/screen_win.cc:555: std::set<int> already_reported_scale_factors; On 2016/07/20 00:58:10, robliao wrote: > Go with an unordered_set or a vector (n is small enough so that vector will > outperform most hash based implementations on time and space). > > n is also small enough where I would be okay with a vector of two passes: once > to gather the unique scale factors and the next to report them. Done.
lgtm + comment https://codereview.chromium.org/2151413002/diff/100001/ui/display/win/screen_... File ui/display/win/screen_win.cc (right): https://codereview.chromium.org/2151413002/diff/100001/ui/display/win/screen_... ui/display/win/screen_win.cc:559: // it to the backend. Comment that we multiply by 100 due to the percentage units used by the histogram. Optional: Alternatively you could report dpi and avoid the multiplication game. GetDPIFromScalingFactor should do the trick.
https://codereview.chromium.org/2151413002/diff/100001/ui/display/win/screen_... File ui/display/win/screen_win.cc (right): https://codereview.chromium.org/2151413002/diff/100001/ui/display/win/screen_... ui/display/win/screen_win.cc:559: // it to the backend. On 2016/07/20 19:56:59, robliao wrote: > Comment that we multiply by 100 due to the percentage units used by the > histogram. > > Optional: Alternatively you could report dpi and avoid the multiplication game. > GetDPIFromScalingFactor should do the trick. Added comment. Raw DPI is a lot less intuitive I think.
The CQ bit was checked by bsep@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, robliao@chromium.org Link to the patchset: https://codereview.chromium.org/2151413002/#ps120001 (title: "edit comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add a UMA histogram for a user's device scale at startup. BUG=628038 ========== to ========== Add a UMA histogram for a user's device scale at startup. BUG=628038 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add a UMA histogram for a user's device scale at startup. BUG=628038 ========== to ========== Add a UMA histogram for a user's device scale at startup. BUG=628038 Committed: https://crrev.com/6b4e59fe3f30bd3d6e035b552facd81a607b6d80 Cr-Commit-Position: refs/heads/master@{#406694} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/6b4e59fe3f30bd3d6e035b552facd81a607b6d80 Cr-Commit-Position: refs/heads/master@{#406694} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
