Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2244263002:
Ensure window border icons repaint when DWM is enabled (Closed)
DescriptionEnsure window border icons repaint when DWM is enabled
When the user movers the cursor over the minimize/maximize/close
buttons, DWM paints them in a hover state. If the user subsequently
moves the cursor out of the window, DWM was not repainting the icons
with the unhovered state because Chrome ate the WM_NCMOUSELEAVE
message.This change notifies DWM of the exit.
BUG=637114
TEST=Unmaximize Chrome. Hover Red-X at top right. Fling mouse upward.
R=sky@chromium.org
Committed: https://crrev.com/7af2fc83ecab219d9d2902db766ab0d2ba4dabf1
Cr-Commit-Position: refs/heads/master@{#412541}
Patch Set 1 #Patch Set 2 : Correct parameters when passing WM_NCMouseLeave to DwmDefaultWndProc #
Total comments: 5
Patch Set 3 : Address review feedback #
Total comments: 2
Patch Set 4 : Add two braces to strategic brace reserve #Messages
Total messages: 22 (11 generated)
The CQ bit was checked by elawrence@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...
elawrence@chromium.org changed required reviewers: + sky@chromium.org
Please have a look and let me know if I'm horribly off base :)
sky@chromium.org changed reviewers: + ananta@chromium.org, msw@chromium.org
+ananta and +msw for subtle windows behavior. This seems right to me. Anyone think we shouldn't do this? https://codereview.chromium.org/2244263002/diff/20001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2244263002/diff/20001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2644: if (HasSystemFrame()) { Does the conditional really matter here?
https://codereview.chromium.org/2244263002/diff/20001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2244263002/diff/20001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2644: if (HasSystemFrame()) { On 2016/08/15 16:17:13, sky wrote: > Does the conditional really matter here? I'm afraid I'm ill-qualified to answer that; I matched the guard around the other call to DwmDefWindowProc earlier in this file. I can say that without my change, Windows 7 shows the bug when an Aero theme is selected, regardless of whether transparent glass is on. The bug does not exist in the Windows 7 "Basic" theme. After my patch is applied, the bug is not present when an Aero theme is selected (nor when Basic is chosen).
Nice fix for this longstanding issue! lgtm with a q. https://codereview.chromium.org/2244263002/diff/20001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2244263002/diff/20001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2644: if (HasSystemFrame()) { On 2016/08/15 16:31:14, elawrence wrote: > On 2016/08/15 16:17:13, sky wrote: > > Does the conditional really matter here? > > I'm afraid I'm ill-qualified to answer that; I matched the guard around the > other call to DwmDefWindowProc earlier in this file. > > I can say that without my change, Windows 7 shows the bug when an Aero theme is > selected, regardless of whether transparent glass is on. The bug does not exist > in the Windows 7 "Basic" theme. > > After my patch is applied, the bug is not present when an Aero theme is selected > (nor when Basic is chosen). The issue doesn't seem to repro with non-glass browser frames (ie. theme applied) on Win7 Aero Glass, so it's probably not necessary to call DwmDefWindowProc if HasSystemFrame, but it probably wouldn't hurt either. https://codereview.chromium.org/2244263002/diff/20001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2646: DwmDefWindowProc(hwnd(), WM_NCMOUSELEAVE, 0, 0, &result); nit q: I wonder if we should set handled to true for certain result values? That might prevent some redundant dispatching later, but it's probably not critical. Otherwise, can we pass null for the result if we don't care to check it?
> https://codereview.chromium.org/2244263002/diff/20001/ui/views/win/hwnd_messa... > ui/views/win/hwnd_message_handler.cc:2646: DwmDefWindowProc(hwnd(), > WM_NCMOUSELEAVE, 0, 0, &result); > nit q: I wonder if we should set handled to true for certain result values? That > might prevent some redundant dispatching later, but it's probably not critical. > Otherwise, can we pass null for the result if we don't care to check it? Thanks! Updated to PatchSet #3 to address this feedback.
LGTM https://codereview.chromium.org/2244263002/diff/40001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2244263002/diff/40001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2644: if (HasSystemFrame()) { no {}
https://codereview.chromium.org/2244263002/diff/20001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2244263002/diff/20001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2646: DwmDefWindowProc(hwnd(), WM_NCMOUSELEAVE, 0, 0, &result); On 2016/08/15 18:57:35, msw wrote: > nit q: I wonder if we should set handled to true for certain result values? That > might prevent some redundant dispatching later, but it's probably not critical. > Otherwise, can we pass null for the result if we don't care to check it? Done. https://codereview.chromium.org/2244263002/diff/40001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2244263002/diff/40001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2644: if (HasSystemFrame()) { On 2016/08/15 22:03:50, sky wrote: > no {} Done.
The CQ bit was checked by elawrence@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: This issue passed the CQ dry run.
I'm going to move forward and commit. ananta-- Please holler if you see a problem and want to discuss or revert. Thanks!
The CQ bit was checked by elawrence@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2244263002/#ps60001 (title: "Add two braces to strategic brace reserve")
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.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Ensure window border icons repaint when DWM is enabled When the user movers the cursor over the minimize/maximize/close buttons, DWM paints them in a hover state. If the user subsequently moves the cursor out of the window, DWM was not repainting the icons with the unhovered state because Chrome ate the WM_NCMOUSELEAVE message.This change notifies DWM of the exit. BUG=637114 TEST=Unmaximize Chrome. Hover Red-X at top right. Fling mouse upward. R=sky@chromium.org ========== to ========== Ensure window border icons repaint when DWM is enabled When the user movers the cursor over the minimize/maximize/close buttons, DWM paints them in a hover state. If the user subsequently moves the cursor out of the window, DWM was not repainting the icons with the unhovered state because Chrome ate the WM_NCMOUSELEAVE message.This change notifies DWM of the exit. BUG=637114 TEST=Unmaximize Chrome. Hover Red-X at top right. Fling mouse upward. R=sky@chromium.org Committed: https://crrev.com/7af2fc83ecab219d9d2902db766ab0d2ba4dabf1 Cr-Commit-Position: refs/heads/master@{#412541} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7af2fc83ecab219d9d2902db766ab0d2ba4dabf1 Cr-Commit-Position: refs/heads/master@{#412541} |
