|
|
DescriptionAdd 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… #
Messages
Total messages: 39 (23 generated)
Description was changed from ========== Added GetMinimizeButtonHeight() method, which can be used for bug 635699 to set the profile avatar button to the same height as the minimize button. This separates the GetMinimizeButtonHeight changes from issue 2832823002/ into a new CL and addresses comments by msarda from that CL. BUG=635699 ========== to ========== Added GetMinimizeButtonHeight() method, which can be used for bug 635699 to set the profile avatar button to the same height as the minimize button. This separates the GetMinimizeButtonHeight changes from issue 2832823002/ into a new CL and addresses comments by msarda from that CL. BUG=635699 ==========
The CQ bit was checked by emx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
emx@chromium.org changed reviewers: + msarda@chromium.org
GetMinimizeButtonHeight is now a separate CL and I've added default button heights for Windows 7 and 8, as discussed.
Code LGTM. Please do the following change to the CL description: * first line must match the CL title * references to other codereviews and bugs must be links (use http://crbug.com/<bug_id> for bugs and http://crrev.com/<cl_nid> for code reviews) * write the CL description as a stand-alone CL, that somebody may refer to in the future and understand that it does. A proposal may be: Add GetMinimizeButtonHeight() method. This CL adds the the function GetMinimizeButtonHeight() method that returns the hight of the minimuze button on Windows. This value will be used to set the height of the profile avatar button (see http://crrev.com/2832823002). BUG=635699
Description was changed from ========== Added GetMinimizeButtonHeight() method, which can be used for bug 635699 to set the profile avatar button to the same height as the minimize button. This separates the GetMinimizeButtonHeight changes from issue 2832823002/ into a new CL and addresses comments by msarda from that CL. BUG=635699 ========== to ========== Add GetMinimizeButtonHeight() method. This CL adds the the function GetMinimizeButtonHeight() method that returns the height of the minimize button on Windows. This value will be used to set the height of the profile avatar button (see http://crrev.com/2832823002). BUG=635699 ==========
Description was changed from ========== Add GetMinimizeButtonHeight() method. This CL adds the the function GetMinimizeButtonHeight() method that returns the height of the minimize button on Windows. This value will be used to set the height of the profile avatar button (see http://crrev.com/2832823002). BUG=635699 ========== to ========== Add GetMinimizeButtonHeight() method This CL adds the the function GetMinimizeButtonHeight() method that returns the height of the minimize button on Windows. This value will be used to set the height of the profile avatar button (see http://crrev.com/2832823002). BUG=635699 ==========
emx@chromium.org changed reviewers: + pkasting@chromium.org
Hi Peter, could you please take a look at this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Nit: I'd prefer to call everything here GetCaptionButtonHeight(). https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... 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/fra... chrome/browser/ui/views/frame/browser_desktop_window_tree_host_x11.cc:48: return 0; This looks wrong at a glance. Is this NOTIMPLEMENTED or NOTREACHED? https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_frame_ash.cc (right): https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_frame_ash.cc:159: return 0; Same question https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_frame_mus.cc (right): https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_frame_mus.cc:97: return 0; And again https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:31: const int kInvalidHeight = static_cast<int>(0x80000000); Nit: This is a bizarre choice. Why not just -1? Better yet, why not 0, and just check for that directly below and eliminate this entirely? https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:213: // solution [http://crbug.com/668278], but GetSystemMetrics provides a good Bug 668278 is a tracking bug, it's not about adding a GetSystemMetricsForDpi method, and shouldn't be about that. I don't know what such a method is supposed to do anyway. Explain more, and/or file a specific bug on this. https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:217: // scaling settings other than 100%: In what way are they incorrect? https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:225: if (button_height_from_sysmetrics == 0) { When can this happen? This needs to be explained. I'd like to remove GetDefaultButtonHeight() entirely. https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:226: DLOG(ERROR) << "GetSystemMetrics(SM_CYSIZE) error " << ::GetLastError(); Avoid logging. https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/minimize_button_metrics_win.h (right): https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.h:59: // Static cache of Windows caption button height Nit: Trailing period. https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.h:60: static int button_bounds_height_; Nit: Name |caption_button_height_|?
Description was changed from ========== Add GetMinimizeButtonHeight() method This CL adds the the function GetMinimizeButtonHeight() method that returns the height of the minimize button on Windows. This value will be used to set the height of the profile avatar button (see http://crrev.com/2832823002). BUG=635699 ========== to ========== Add GetCaptionButtonHeight() method This CL adds the the function GetCaptionButtonHeight() method that returns the height of the caption buttons on Windows. This value will be used to set the height of the profile avatar button (see http://crrev.com/2832823002). BUG=635699 ==========
The CQ bit was checked by emx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... 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/fra... 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: > This looks wrong at a glance. Is this NOTIMPLEMENTED or NOTREACHED? Yes, it's now NOTIMPLEMENTED() https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_frame_ash.cc (right): https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_frame_ash.cc:159: return 0; On 2017/04/24 23:00:17, Peter Kasting wrote: > Same question NOTIMPLEMENTED() https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_frame_mus.cc (right): https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_frame_mus.cc:97: return 0; On 2017/04/24 23:00:17, Peter Kasting wrote: > And again NOTIMPLEMENTED() https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:31: const int kInvalidHeight = static_cast<int>(0x80000000); On 2017/04/24 23:00:17, Peter Kasting wrote: > Nit: This is a bizarre choice. Why not just -1? Better yet, why not 0, and > just check for that directly below and eliminate this entirely? I agree that it's a bizarre choice, but I wanted to keep it consistent with kInvalidOffset. (That one probably can't be 0 since 0 is a valid offset.) Should I change both to -1? https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:213: // solution [http://crbug.com/668278], but GetSystemMetrics provides a good On 2017/04/24 23:00:17, Peter Kasting wrote: > Bug 668278 is a tracking bug, it's not about adding a GetSystemMetricsForDpi > method, and shouldn't be about that. > > I don't know what such a method is supposed to do anyway. Explain more, and/or > file a specific bug on this. robliao@ and bsep@ told me that GetSystemMetricsForDpi is likely to be the solution to this. I've revised the comment. I don't think I'm in a position to file another bug on this as I don't know any more about this than what they told me. https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:217: // scaling settings other than 100%: On 2017/04/24 23:00:17, Peter Kasting wrote: > In what way are they incorrect? More accurately, they're not what the caller expects. I don't know the correct terminology for this, but in this case the caller expects the "unscaled" size, i.e. 28 regardless of DPI settings (DIPs?). The first 3 methods return the "scaled" size, e.g. 56 at 200% scaling. The last one returns 28 / primary monitor scaling. I've edited the comment. https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:225: if (button_height_from_sysmetrics == 0) { On 2017/04/24 23:00:17, Peter Kasting wrote: > When can this happen? This needs to be explained. I'd like to remove > GetDefaultButtonHeight() entirely. I don't know when it can happen. MSDN does not explain this, but the function does provide for the possibility of failure [https://msdn.microsoft.com/en-us/library/windows/desktop/ms724385(v=vs.85).aspx], so I don't think it's safe to assume it will never fail. https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:226: DLOG(ERROR) << "GetSystemMetrics(SM_CYSIZE) error " << ::GetLastError(); On 2017/04/24 23:00:17, Peter Kasting wrote: > Avoid logging. Logging removed. https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/minimize_button_metrics_win.h (right): https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.h:59: // Static cache of Windows caption button height On 2017/04/24 23:00:17, Peter Kasting wrote: > Nit: Trailing period. Done. https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.h:60: static int button_bounds_height_; On 2017/04/24 23:00:17, Peter Kasting wrote: > Nit: Name |caption_button_height_|? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... 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 wrote: > On 2017/04/24 23:00:17, Peter Kasting wrote: > > Nit: This is a bizarre choice. Why not just -1? Better yet, why not 0, and > > just check for that directly below and eliminate this entirely? > > I agree that it's a bizarre choice, but I wanted to keep it consistent with > kInvalidOffset. (That one probably can't be 0 since 0 is a valid offset.) It's theoretically valid, but the only real constraint is that the offset not match one of the ones we really use. Honestly, IMO using signal values here is bad anyway, and it'd be better to use signal bools ("value is set") or base::Optional or similar. > Should > I change both to -1? Maybe? Although I wouldn't aim for any consistency, as it doesn't make anything easier to read or understand IMO. https://codereview.chromium.org/2833363002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:225: if (button_height_from_sysmetrics == 0) { On 2017/04/25 11:57:40, emx wrote: > On 2017/04/24 23:00:17, Peter Kasting wrote: > > When can this happen? This needs to be explained. I'd like to remove > > GetDefaultButtonHeight() entirely. > > I don't know when it can happen. MSDN does not explain this, but the function > does provide for the possibility of failure > [https://msdn.microsoft.com/en-us/library/windows/desktop/ms724385(v=vs.85).aspx], > so I don't think it's safe to assume it will never fail. Most other places in code that we call GetSystemMetrics(), we don't check for a zero return. I think it's OK to avoid specially handling failure here. The only way this should fail, I think, is if you try to read a metric you don't have permission to read, which should never happen in Chrome. https://codereview.chromium.org/2833363002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/2833363002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:215: // approximation - it seems to be no more than 1px off in practice. This comment confuses me a bit because it's not clear if what we're saying is simply that views' current DIP-based layout doesn't capture heights well when they aren't integral DIPs, or that this function is supposed to return a px-based value but returns the wrong one, or what. This is complicated by the fact that I think your actual implementation here returns a px-based value (that's what MSDN documents GetSystemMetrics as doing), and I think you need to return a DIP-based value. https://codereview.chromium.org/2833363002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:222: // these all return (height * scale factor) e.g. 56 at 200% This doesn't seem right, reading the code. GetSystemMetricsInDIP actually does what you've documented GetSystemMetricsForHwnd as doing -- returns (height / primary monitor DSF). Presumably GetSystemMetrics returns the metric in pixels based on the primary monitor's DSF to begin with, which makes this result in coherent DIP-based value, rounded to the nearest DIP (which is what you want here, I think). Other frame code uses this function for similar purposes to yours -- see e.g. CustomFrameView::IconSize(). It really seems like the right one to use. I'm wondering if somehow the original test you did assumed it multiplied by the scale factor when it didn't, and tried to correct for that unnecessarily, or something? I don't know what happened, but can you check again here?
I've removed the caching entirely. It's unnecessary, as GetSystemMetrics(SM_CYSIZE) is very fast. So GetCaptionButtonHeight now simply returns GetSystemMetrics(SM_CYSIZE). https://codereview.chromium.org/2833363002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/2833363002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:215: // approximation - it seems to be no more than 1px off in practice. OK, Mihai and I looked into this together and I've updated the comment. GetSystemMetrics(SM_CYSIZE) always returns 28, regardless of the DPI settings of the primary monitor (or any other monitor). I think that means it's "device-independent" pixels, despite what the documentation says. 28 is what we need to later pass to View::SetBounds() to make the avatar button look the same height as the caption buttons. (My understanding is that a View does the scaling internally somewhere.) So basically the height of the caption buttons - in whatever units Views::SetBounds() takes - is unaffected by DPI scaling, which is why the 4 methods that scale it give the wrong result. https://codereview.chromium.org/2833363002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:222: // these all return (height * scale factor) e.g. 56 at 200% On 2017/04/26 01:04:23, Peter Kasting wrote: > This doesn't seem right, reading the code. GetSystemMetricsInDIP actually does > what you've documented GetSystemMetricsForHwnd as doing -- returns (height / > primary monitor DSF). Presumably GetSystemMetrics returns the metric in pixels > based on the primary monitor's DSF to begin with, which makes this result in > coherent DIP-based value, rounded to the nearest DIP (which is what you want > here, I think). Yes, you're right, I got GetSystemMetricsForHwnd and GetSystemMetricsInDIP the wrong way around. GetSystemMetricsForHwnd returns 56 at 200% scaling, like DwmGetWindowAttribute and SendMessage(WM_GETTITLEBARINFOEX). Updated the comment. > Other frame code uses this function for similar purposes to yours -- see e.g. > CustomFrameView::IconSize(). It really seems like the right one to use. I'm > wondering if somehow the original test you did assumed it multiplied by the > scale factor when it didn't, and tried to correct for that unnecessarily, or > something? I don't know what happened, but can you check again here? I've tested it again and 28 is the only value that gives the (approximately) right result at all DPI scaling settings. The above comment is Mihai's and my theory as to why, but even if the explanation is wrong, the test results remain the same.
Since there is no longer any caching and hwnd_ is not used the method can now be static, which greatly simplifies the whole CL.
https://codereview.chromium.org/2833363002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/2833363002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:57: // Caption button height is not affected by DPI settings. robliao says that in his testing, this isn't correct. He gets the same result MSDN suggests: doubled values at double DPI. His suggestion is that if you saw something different, you likely did one of two things: (1) Tested in a sandbox program -- by default Windows assumes programs are not "DPI-aware" and virtualizes the results of GetSystemMetrics(). You have to tell windows you are DPI-aware. Chrome does this. (2) Failed to log out and log in again after changing DPI, in which case GetSystemMetrics() gives you the wrong results. Given this, I think you really do want GetSystemMetricsInDIP(). https://codereview.chromium.org/2833363002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:59: // settings of any monitor (28 on Windows 10). This is what needs to be Rob also notes he gets 22 here, not 28. I would just avoid specifying a particular height. https://codereview.chromium.org/2833363002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:60: // passed to a View, which then takes care of the scaling. Nit: No need for the last sentence here https://codereview.chromium.org/2833363002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:62: // A View drawn to this height may look ~1px off at some DPI settings. I think "~1" is inaccurate here. Assuming the conversion rounds, we could look off by up to (DSF/2) px. So if we ever get up to e.g. DSF=4, we could be off by 2 px. I would just say "TODO(...): Due to conversion inaccuracy, returning the height in DIPs may result in slight height differences with the native buttons. http://crbug.com/..." Then I would file a separate bug on this specific issue and block 668278 on it, and link that above. https://codereview.chromium.org/2833363002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:74: // returns (height / by primary monitor scale factor) e.g. 22 for 125% I would omit this comment entirely, as your first one covers it by saying SM_CYSIZE is not affected by DPI. https://codereview.chromium.org/2833363002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/minimize_button_metrics_win.h (right): https://codereview.chromium.org/2833363002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/minimize_button_metrics_win.h:21: // Get the height of the native caption buttons in DIPs. Nit: Get -> Returns (make comments declarative, not imperative)
On 2017/04/26 17:19:52, Peter Kasting wrote: > (2) Failed to log out and log in again after changing DPI, in which case > GetSystemMetrics() gives you the wrong results. > > Given this, I think you really do want GetSystemMetricsInDIP(). You're right, I had my primary monitor set to 125% scaling and was testing by changing the secondary monitor scaling. With this GetSystemMetrics(SM_CYSIZE) returned 28. With the primary monitor set to 100%, after logging out and in again, it returned 22. Unfortunately this doesn't give me a value that gets the button drawn to the right size - see email. I'll need to speak to someone about what to do here.
On 2017/04/27 15:00:01, emx wrote: > On 2017/04/26 17:19:52, Peter Kasting wrote: > > (2) Failed to log out and log in again after changing DPI, in which case > > GetSystemMetrics() gives you the wrong results. > > > > Given this, I think you really do want GetSystemMetricsInDIP(). > > You're right, I had my primary monitor set to 125% scaling and was testing by > changing the secondary monitor scaling. With this GetSystemMetrics(SM_CYSIZE) > returned 28. With the primary monitor set to 100%, after logging out and in > again, it returned 22. Unfortunately this doesn't give me a value that gets the > button drawn to the right size - see email. I'll need to speak to someone about > what to do here. I suspect the issue is that this is returning the additional height of the caption button below the top border thickness. If you're trying to draw from the top of the window, you'll need to add in that height; see e.g. GlassBrowserFrameView::FrameTopBorderThickness(). Be careful about px -> dip conversion errors compounding in this process. I suspect what you want is to add the px-based CYSIZEFRAME and CYSIZE values together, then divide by the scale factor. See also how the function linked above is careful to floor for correct hittesting purposes. bsep@ is probably the most knowledgeable person about frame layout stuff if you need to follow up further while I'm gone.
Description was changed from ========== Add GetCaptionButtonHeight() method This CL adds the the function GetCaptionButtonHeight() method that returns the height of the caption buttons on Windows. This value will be used to set the height of the profile avatar button (see http://crrev.com/2832823002). BUG=635699 ========== to ========== 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 ==========
OK, I've changed the function to return floor((SM_CYSIZE+SM_CYSIZEFRAME)/primary_device_scale) with comments. I can't use the same explanation for floor() as GlassBrowserFrameView::FrameTopBorderThickness() since there is no click location involved here, but in my testing flooring does give a closer approximation than rounding (see emailed spreadsheet).
The CQ bit was checked by emx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM 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); Nit: Dunno if this would be clearer if these were |caption_height| and |frame_thickness| or something. Also these could optionally be declared const.
The CQ bit was checked by emx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2833363002/#ps100001 (title: "Renamed variables and removed call to GetCaptionButtonHeightInDIPs, which depends on a CL still bei…")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494320234036960, "parent_rev": "b94383086bbb49474afd397fa447a8b940c84684", "commit_rev": "d6dd415275e2ddf1b1e816de4a9f2b23d4236339"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/d6dd415275e2ddf1b1e816de4a9f... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/d6dd415275e2ddf1b1e816de4a9f...
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. |