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

Issue 2833363002: Add GetCaptionButtonHeightInDIPs() method (Closed)

Created:
3 years, 8 months ago by emx
Modified:
3 years, 7 months ago
Reviewers:
Peter Kasting, msarda
CC:
chromium-reviews, tfarina, mac-reviews_chromium.org, robliao
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add GetCaptionButtonHeightInDIPs() method This CL adds the the function GetCaptionButtonHeightInDIPs() method that returns the height of the caption buttons on Windows in DIPs and uses it for the Windows 10 MD avatar button (see http://crrev.com/2851543002). BUG=635699 Review-Url: https://codereview.chromium.org/2833363002 Cr-Commit-Position: refs/heads/master@{#470271} Committed: https://chromium.googlesource.com/chromium/src/+/d6dd415275e2ddf1b1e816de4a9f2b23d4236339

Patch Set 1 #

Total comments: 22

Patch Set 2 : Addressed review comments #

Total comments: 4

Patch Set 3 : Removed caching of GetCaptionButtonHeight() and updated comment #

Patch Set 4 : Made MinimizeButtonMetrics::GetCaptionButtonHeight() static and reverted the rest of the changes #

Total comments: 6

Patch Set 5 : GetCaptionButtonHeightInDIPs now returns (SM_CYSIZE+SM_CYSIZEFRAME)/primary_device_scale #

Total comments: 2

Patch Set 6 : Renamed variables and removed call to GetCaptionButtonHeightInDIPs, which depends on a CL still bei… #

Unified diffs Side-by-side diffs Delta from patch set Stats (+30 lines, -3 lines) Patch
M chrome/browser/ui/views/frame/minimize_button_metrics_win.h View 1 2 3 4 2 chunks +4 lines, -1 line 0 comments Download
M chrome/browser/ui/views/frame/minimize_button_metrics_win.cc View 1 2 3 4 5 3 chunks +26 lines, -2 lines 0 comments Download

Messages

Total messages: 39 (23 generated)
emx
GetMinimizeButtonHeight is now a separate CL and I've added default button heights for Windows 7 ...
3 years, 8 months ago (2017-04-24 09:32:58 UTC) #5
msarda
Code LGTM. Please do the following change to the CL description: * first line must ...
3 years, 8 months ago (2017-04-24 10:50:35 UTC) #6
emx
Hi Peter, could you please take a look at this?
3 years, 8 months ago (2017-04-24 11:26:12 UTC) #10
Peter Kasting
Nit: I'd prefer to call everything here GetCaptionButtonHeight(). https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/frame/browser_desktop_window_tree_host_x11.cc File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/frame/browser_desktop_window_tree_host_x11.cc#newcode48 chrome/browser/ui/views/frame/browser_desktop_window_tree_host_x11.cc:48: return ...
3 years, 8 months ago (2017-04-24 23:00:17 UTC) #13
emx
https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/frame/browser_desktop_window_tree_host_x11.cc File chrome/browser/ui/views/frame/browser_desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/frame/browser_desktop_window_tree_host_x11.cc#newcode48 chrome/browser/ui/views/frame/browser_desktop_window_tree_host_x11.cc:48: return 0; On 2017/04/24 23:00:17, Peter Kasting wrote: > ...
3 years, 8 months ago (2017-04-25 11:57:40 UTC) #17
Peter Kasting
https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/frame/minimize_button_metrics_win.cc File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/frame/minimize_button_metrics_win.cc#newcode31 chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:31: const int kInvalidHeight = static_cast<int>(0x80000000); On 2017/04/25 11:57:40, emx ...
3 years, 8 months ago (2017-04-26 01:04:23 UTC) #20
emx
I've removed the caching entirely. It's unnecessary, as GetSystemMetrics(SM_CYSIZE) is very fast. So GetCaptionButtonHeight now ...
3 years, 8 months ago (2017-04-26 14:19:26 UTC) #21
emx
Since there is no longer any caching and hwnd_ is not used the method can ...
3 years, 8 months ago (2017-04-26 14:49:53 UTC) #22
Peter Kasting
https://codereview.chromium.org/2833363002/diff/60001/chrome/browser/ui/views/frame/minimize_button_metrics_win.cc File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/2833363002/diff/60001/chrome/browser/ui/views/frame/minimize_button_metrics_win.cc#newcode57 chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:57: // Caption button height is not affected by DPI ...
3 years, 8 months ago (2017-04-26 17:19:52 UTC) #23
emx
On 2017/04/26 17:19:52, Peter Kasting wrote: > (2) Failed to log out and log in ...
3 years, 7 months ago (2017-04-27 15:00:01 UTC) #24
Peter Kasting
On 2017/04/27 15:00:01, emx wrote: > On 2017/04/26 17:19:52, Peter Kasting wrote: > > (2) ...
3 years, 7 months ago (2017-04-27 20:44:25 UTC) #25
emx
OK, I've changed the function to return floor((SM_CYSIZE+SM_CYSIZEFRAME)/primary_device_scale) with comments. I can't use the same ...
3 years, 7 months ago (2017-04-28 13:53:19 UTC) #27
Peter Kasting
LGTM https://codereview.chromium.org/2833363002/diff/80001/chrome/browser/ui/views/frame/minimize_button_metrics_win.cc File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/2833363002/diff/80001/chrome/browser/ui/views/frame/minimize_button_metrics_win.cc#newcode64 chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:64: int cysizeframe = GetSystemMetrics(SM_CYSIZEFRAME); Nit: Dunno if this ...
3 years, 7 months ago (2017-05-05 22:41:02 UTC) #32
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/2833363002/100001
3 years, 7 months ago (2017-05-09 08:57:45 UTC) #35
commit-bot: I haz the power
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/d6dd415275e2ddf1b1e816de4a9f2b23d4236339
3 years, 7 months ago (2017-05-09 09:34:43 UTC) #38
emx
3 years, 7 months ago (2017-05-09 09:37:58 UTC) #39
Message was sent while issue was closed.
Done, thanks!

https://codereview.chromium.org/2833363002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right):

https://codereview.chromium.org/2833363002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:64: int cysizeframe
= GetSystemMetrics(SM_CYSIZEFRAME);
On 2017/05/05 22:41:01, Peter Kasting wrote:
> Nit: Dunno if this would be clearer if these were |caption_height| and
> |frame_thickness| or something.  Also these could optionally be declared
const.

Done.

Powered by Google App Engine
This is Rietveld 408576698