Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2190593002:
[Win] CustomFrameView::IconSize() should return the value in dip unit (Closed)
|
Created:
4 years, 4 months ago by Sungmann Cho Modified:
4 years, 4 months ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Win] CustomFrameView::IconSize() should return the value in dip unit
CustomFrameView::IconSize() returns the size of the window icon. On Windows,
the value is obtained from GetSystemMetrics(SM_CYSMICON) call, but the problem
is that the value returned is in pixel, not in dip. This leads to a wrong sized
window icon if an user is using a higher scale ratio. This CL replaces
GetSystemMetrics() with display::win::GetSystemMetricsInDIP() to fix this.
BUG=631488
TEST=See the bug page for the reproduce steps
Committed: https://crrev.com/bfff171967e262866054da6ce1e07961fde797e8
Cr-Commit-Position: refs/heads/master@{#408290}
Patch Set 1 #
Total comments: 1
Messages
Total messages: 19 (6 generated)
Description was changed from ========== [Win] CustomFrameView::IconSize() should return the value in dip unit CustomFrameView::IconSize() returns the size of the window icon. On Windows, the value is obtained from GetSystemMetrics(SM_CYSMICON) call, but the problem is that the value returned is in pixel, not in dip. This leads to a wrong sized window icon if an user is using a higher scale ratio. This CL replaces GetSystemMetrics() with display::win::GetSystemMetricsInDIP() to fix this. BUG=631488 TEST=See the bug page for the reproduce steps ========== to ========== [Win] CustomFrameView::IconSize() should return the value in dip unit CustomFrameView::IconSize() returns the size of the window icon. On Windows, the value is obtained from GetSystemMetrics(SM_CYSMICON) call, but the problem is that the value returned is in pixel, not in dip. This leads to a wrong sized window icon if an user is using a higher scale ratio. This CL replaces GetSystemMetrics() with display::win::GetSystemMetricsInDIP() to fix this. BUG=631488 TEST=See the bug page for the reproduce steps ==========
sungmann.cho@navercorp.com changed reviewers: + sky@chromium.org
Please take a look. Thanks!
sky@chromium.org changed reviewers: + robliao@chromium.org
I tend to think we should get rid of the functions that don't take an HWND. Rob, WDYT? https://codereview.chromium.org/2190593002/diff/1/ui/views/window/custom_fram... File ui/views/window/custom_frame_view.cc (right): https://codereview.chromium.org/2190593002/diff/1/ui/views/window/custom_fram... ui/views/window/custom_frame_view.cc:311: return display::win::GetSystemMetricsInDIP(SM_CYSMICON); The conversion may vary between HWNDs. You should be using the conversion routines in ScreenWin that take an HWND.
On 2016/07/27 15:30:15, sky wrote: > I tend to think we should get rid of the functions that don't take an HWND. Rob, > WDYT? > > https://codereview.chromium.org/2190593002/diff/1/ui/views/window/custom_fram... > File ui/views/window/custom_frame_view.cc (right): > > https://codereview.chromium.org/2190593002/diff/1/ui/views/window/custom_fram... > ui/views/window/custom_frame_view.cc:311: return > display::win::GetSystemMetricsInDIP(SM_CYSMICON); > The conversion may vary between HWNDs. You should be using the conversion > routines in ScreenWin that take an HWND. Agreed. I've got a TODO on my end to remove most of dpi.h as much as possible. This should indeed be using display::win::ScreenWin::GetSystemMetricsForHwnd.
Rob, WDYT of adding a comment to said functions that they are deprecated? That way hopefully folks don't add new usage like this. On Wed, Jul 27, 2016 at 10:03 AM, <robliao@chromium.org> wrote: > On 2016/07/27 15:30:15, sky wrote: >> I tend to think we should get rid of the functions that don't take an >> HWND. > Rob, >> WDYT? >> >> > https://codereview.chromium.org/2190593002/diff/1/ui/views/window/custom_fram... >> File ui/views/window/custom_frame_view.cc (right): >> >> > https://codereview.chromium.org/2190593002/diff/1/ui/views/window/custom_fram... >> ui/views/window/custom_frame_view.cc:311: return >> display::win::GetSystemMetricsInDIP(SM_CYSMICON); >> The conversion may vary between HWNDs. You should be using the conversion >> routines in ScreenWin that take an HWND. > > Agreed. I've got a TODO on my end to remove most of dpi.h as much as > possible. > This should indeed be using > display::win::ScreenWin::GetSystemMetricsForHwnd. > > https://codereview.chromium.org/2190593002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Works for me. I'll send a CL out once the sheriff stuff quiets down. On Wed, Jul 27, 2016 at 11:18 AM, Scott Violet <sky@chromium.org> wrote: > Rob, WDYT of adding a comment to said functions that they are > deprecated? That way hopefully folks don't add new usage like this. > > On Wed, Jul 27, 2016 at 10:03 AM, <robliao@chromium.org> wrote: > > On 2016/07/27 15:30:15, sky wrote: > >> I tend to think we should get rid of the functions that don't take an > >> HWND. > > Rob, > >> WDYT? > >> > >> > > > https://codereview.chromium.org/2190593002/diff/1/ui/views/window/custom_fram... > >> File ui/views/window/custom_frame_view.cc (right): > >> > >> > > > https://codereview.chromium.org/2190593002/diff/1/ui/views/window/custom_fram... > >> ui/views/window/custom_frame_view.cc:311: return > >> display::win::GetSystemMetricsInDIP(SM_CYSMICON); > >> The conversion may vary between HWNDs. You should be using the > conversion > >> routines in ScreenWin that take an HWND. > > > > Agreed. I've got a TODO on my end to remove most of dpi.h as much as > > possible. > > This should indeed be using > > display::win::ScreenWin::GetSystemMetricsForHwnd. > > > > https://codereview.chromium.org/2190593002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I'm trying to use display::win::ScreenWin::GetSystemMetricsForHwnd(), but this method seems to return a metric in pixel, not in dpi. Would we need to have something like GetSystemMetricsInDIPForHwnd()?
On 2016/07/27 20:14:32, Sungmann Cho wrote: > I'm trying to use display::win::ScreenWin::GetSystemMetricsForHwnd(), but this > method seems to return a metric in pixel, not in dpi. Would we need to have > something like GetSystemMetricsInDIPForHwnd()? I guess this goes to show don't sheriff and make design decisions on the fly. I misread your code review and thought you needed the pixel equivalent for the icon (I recently a change that did this for some frame sizing). If you're using this specifically for the DIP value, this indeed should work and I'll fix the comment in dpi.h to account for this.
lgtm
LGTM
On 2016/07/27 20:59:24, robliao wrote: > On 2016/07/27 20:14:32, Sungmann Cho wrote: > > I'm trying to use display::win::ScreenWin::GetSystemMetricsForHwnd(), but this > > method seems to return a metric in pixel, not in dpi. Would we need to have > > something like GetSystemMetricsInDIPForHwnd()? > > I guess this goes to show don't sheriff and make design decisions on the fly. > I misread your code review and thought you needed the pixel equivalent for the > icon (I recently a change that did this for some frame sizing). > If you're using this specifically for the DIP value, this indeed should work and > I'll fix the comment in dpi.h to account for this. Got it. I'll submit a cleanup patch after landing of your CL (https://codereview.chromium.org/2186163002) Thanks for the review!
The CQ bit was checked by sungmann.cho@navercorp.com
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 ========== [Win] CustomFrameView::IconSize() should return the value in dip unit CustomFrameView::IconSize() returns the size of the window icon. On Windows, the value is obtained from GetSystemMetrics(SM_CYSMICON) call, but the problem is that the value returned is in pixel, not in dip. This leads to a wrong sized window icon if an user is using a higher scale ratio. This CL replaces GetSystemMetrics() with display::win::GetSystemMetricsInDIP() to fix this. BUG=631488 TEST=See the bug page for the reproduce steps ========== to ========== [Win] CustomFrameView::IconSize() should return the value in dip unit CustomFrameView::IconSize() returns the size of the window icon. On Windows, the value is obtained from GetSystemMetrics(SM_CYSMICON) call, but the problem is that the value returned is in pixel, not in dip. This leads to a wrong sized window icon if an user is using a higher scale ratio. This CL replaces GetSystemMetrics() with display::win::GetSystemMetricsInDIP() to fix this. BUG=631488 TEST=See the bug page for the reproduce steps ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [Win] CustomFrameView::IconSize() should return the value in dip unit CustomFrameView::IconSize() returns the size of the window icon. On Windows, the value is obtained from GetSystemMetrics(SM_CYSMICON) call, but the problem is that the value returned is in pixel, not in dip. This leads to a wrong sized window icon if an user is using a higher scale ratio. This CL replaces GetSystemMetrics() with display::win::GetSystemMetricsInDIP() to fix this. BUG=631488 TEST=See the bug page for the reproduce steps ========== to ========== [Win] CustomFrameView::IconSize() should return the value in dip unit CustomFrameView::IconSize() returns the size of the window icon. On Windows, the value is obtained from GetSystemMetrics(SM_CYSMICON) call, but the problem is that the value returned is in pixel, not in dip. This leads to a wrong sized window icon if an user is using a higher scale ratio. This CL replaces GetSystemMetrics() with display::win::GetSystemMetricsInDIP() to fix this. BUG=631488 TEST=See the bug page for the reproduce steps Committed: https://crrev.com/bfff171967e262866054da6ce1e07961fde797e8 Cr-Commit-Position: refs/heads/master@{#408290} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/bfff171967e262866054da6ce1e07961fde797e8 Cr-Commit-Position: refs/heads/master@{#408290} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
