Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1707233002:
Reduce the fullscreen window height by 1px on activation loss. (Closed)
DescriptionReduce the fullscreen window height by 1px on activation loss and restore it back on activation gain.
This is to workaround a bug/behavior of the Windows taskbar, where in it does not stay as topmost window when a window
on a thread which has a fullscreen window is active. The symptom observed in this case is that the windows on the same thread
as the fullscreen window end up obscuring the taskbar or cause autohide to not work.
The other changes in this patch are as below:-
1. When the bounds of the WinWindow class which serves as
the PlatformWindow for the WindowTreeHost instance
change, we don't activate it if the underlying HWND
is not visible.
2. The WidgetTestInteractive.ViewFocusOnHWNDEnabledChanges
test explicitly shows the WindowTreeHost instance which
ensures that the HWND is visible and activated.
BUG=472139
TEST=Covered by views_unittest Widget.FullscreenBoundsReducedOnActivationLoss
Committed: https://crrev.com/ed6530c9ab6cd4a56ffd70612be526dbc7db1552
Cr-Commit-Position: refs/heads/master@{#377084}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Reduce the size of the fullscreen window by 1px on activation loss #
Total comments: 4
Patch Set 3 : Update comments #
Total comments: 2
Patch Set 4 : Move the fullscreen hacks to HWNDMessageHandler #
Total comments: 5
Patch Set 5 : Reset the background fullscreen state member in HWNDMessageHandler::SetBounds and in WM_WINDOWPOSCH… #
Total comments: 2
Patch Set 6 : Combine ifs #Patch Set 7 : The FullscreenBoundsReducedOnActivationLoss test is now an interactive test as it fails intermittently on the trybots. #Patch Set 8 : Revert changes to widget_unittest.cc #Patch Set 9 : Check in OnWindowPosChanging whether the size is changing before resetting the background_fullscree… #Patch Set 10 : Don't grab activation in the WinWindow class if the window is not visible #Patch Set 11 : Show the WindowTreeHost in the WidgetTestInteractive.ViewFocusOnHWNDEnabledChanges ui test #Patch Set 12 : Reorganize the WidgetTest.FullscreenBoundsReducedOnActivationLoss test to make it less flaky #Patch Set 13 : Make the WidgetNotActivatedOnFakeActivationMessages an interactive ui test #Patch Set 14 : Revert changes to widget_unittest.cc #
Messages
Total messages: 38 (14 generated)
ananta@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/1707233002/diff/1/ui/views/widget/desktop_aur... File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1707233002/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:715: // If the window losing activation is a fullscreen window and it is on the Is there a reason not to do this any time the active window is fullscreen and loses activation? Why only do some of the time? https://codereview.chromium.org/1707233002/diff/1/ui/views/win/hwnd_message_h... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/1707233002/diff/1/ui/views/win/hwnd_message_h... ui/views/win/hwnd_message_handler.cc:931: if (message == WM_ACTIVATE && IsTopLevelWindow(window)) nit: add {} now
Also, do we need to remember we changed fullscreen and switch back on gaining activation?
https://codereview.chromium.org/1707233002/diff/1/ui/views/widget/desktop_aur... File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1707233002/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:715: // If the window losing activation is a fullscreen window and it is on the On 2016/02/18 17:18:47, sky wrote: > Is there a reason not to do this any time the active window is fullscreen and > loses activation? Why only do some of the time? We only want to do this if we are losing activation to a window on the same monitor. Across monitors this works correctly. Is that what you are asking?
On 2016/02/18 20:29:28, ananta wrote: > https://codereview.chromium.org/1707233002/diff/1/ui/views/widget/desktop_aur... > File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): > > https://codereview.chromium.org/1707233002/diff/1/ui/views/widget/desktop_aur... > ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:715: // If the > window losing activation is a fullscreen window and it is on the > On 2016/02/18 17:18:47, sky wrote: > > Is there a reason not to do this any time the active window is fullscreen and > > loses activation? Why only do some of the time? > > We only want to do this if we are losing activation to a window on the same > monitor. Across monitors this works correctly. Is that what you are asking?
https://codereview.chromium.org/1707233002/diff/1/ui/views/widget/desktop_aur... File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1707233002/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:715: // If the window losing activation is a fullscreen window and it is on the On 2016/02/18 20:29:28, ananta wrote: > On 2016/02/18 17:18:47, sky wrote: > > Is there a reason not to do this any time the active window is fullscreen and > > loses activation? Why only do some of the time? > > We only want to do this if we are losing activation to a window on the same > monitor. Across monitors this works correctly. Is that what you are asking? I'm asking why not do this: if (!active && IsFullscreen()) SetFullscreen(false); Even if it works now cross monitors, why not do it always? I'm also asking if we gain focus and were fullscreen should set SetFullscreen(true)?
Description was changed from ========== Switch a Chrome window away from fullscreen mode if another top level window on the same monitor is becoming active. This is to workaround a bug/behavior of the Windows taskbar, where in it does not stay as topmost window when a window on a thread which has a fullscreen window is active. The symptom observed in this case is that the windows on the same thread as the fullscreen window end up obscuring the taskbar or cause autohide to not work. This works correctly when the windows are across monitors. BUG=472139 TEST=Covered by views_unittest Widget.FullscreenStateSwitchedOnActivationLoss ========== to ========== Reduce the fullscreen window height by 1px on activation loss and restore it back on activation gain. This is to workaround a bug/behavior of the Windows taskbar, where in it does not stay as topmost window when a window on a thread which has a fullscreen window is active. The symptom observed in this case is that the windows on the same thread as the fullscreen window end up obscuring the taskbar or cause autohide to not work. BUG=472139 TEST=Covered by views_unittest Widget.FullscreenBoundsReducedOnActivationLoss ==========
The bug you site is all about maximized windows, not fullscreen. Are you sure you are fixing the right bug? https://codereview.chromium.org/1707233002/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1707233002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:715: // If the window losing activation is a fullscreen window and it is on the Update your comment as you no longer look at monitor. https://codereview.chromium.org/1707233002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:722: // autohide to not work correctly, etc. Your comment here is still a bit vague. What we're doing is making it so the taskbar shows up if we lose activation and are fullscreen.
The bug is about maximized windows obscuring the taskbar. If you look at comments in a related bug, folks have mentioned that this typically happens when they have fullcreen windows active. I feel that this patch should address those concerns. https://codereview.chromium.org/1707233002/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1707233002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:715: // If the window losing activation is a fullscreen window and it is on the On 2016/02/19 19:14:49, sky wrote: > Update your comment as you no longer look at monitor. Done. https://codereview.chromium.org/1707233002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:722: // autohide to not work correctly, etc. On 2016/02/19 19:14:48, sky wrote: > Your comment here is still a bit vague. What we're doing is making it so the > taskbar shows up if we lose activation and are fullscreen. Done.
https://codereview.chromium.org/1707233002/diff/40001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1707233002/diff/40001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:712: if (!::IsWindow(window_gaining_or_losing_activation)) Is there a reason not to put all this logic in HWNDMessageHandler? You' making it maintain some state anyway.
https://codereview.chromium.org/1707233002/diff/40001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc (right): https://codereview.chromium.org/1707233002/diff/40001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:712: if (!::IsWindow(window_gaining_or_losing_activation)) On 2016/02/19 21:17:00, sky wrote: > Is there a reason not to put all this logic in HWNDMessageHandler? You' making > it maintain some state anyway. I did not want to add state to HWNDMessageHandler. However the code in WM_WINDOWPOSCHANGING necessiated that change. I thought it is better to have this logic in the delegate.
If you move all the state to DesktopWindowTreeHostWin, then I'm ok with it. But if you need some state in HWNDMessageHandler, then it should take care of it all. DWTHW is told when the bounds change, so you shouldn't need any state in HWNDMessageHandler. On Fri, Feb 19, 2016 at 1:24 PM, <ananta@chromium.org> wrote: > > > https://codereview.chromium.org/1707233002/diff/40001/ui/views/widget/desktop... > File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc > (right): > > https://codereview.chromium.org/1707233002/diff/40001/ui/views/widget/desktop... > ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:712: if > (!::IsWindow(window_gaining_or_losing_activation)) > On 2016/02/19 21:17:00, sky wrote: >> Is there a reason not to put all this logic in HWNDMessageHandler? > You' making >> it maintain some state anyway. > > I did not want to add state to HWNDMessageHandler. However the code in > WM_WINDOWPOSCHANGING necessiated that change. I thought it is better to > have this logic in the delegate. > > https://codereview.chromium.org/1707233002/ -- 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.
On 2016/02/19 21:27:53, sky wrote: > If you move all the state to DesktopWindowTreeHostWin, then I'm ok > with it. But if you need some state in HWNDMessageHandler, then it > should take care of it all. DWTHW is told when the bounds change, so > you shouldn't need any state in HWNDMessageHandler. > > On Fri, Feb 19, 2016 at 1:24 PM, <mailto:ananta@chromium.org> wrote: > > > > > > > https://codereview.chromium.org/1707233002/diff/40001/ui/views/widget/desktop... > > File ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc > > (right): > > > > > https://codereview.chromium.org/1707233002/diff/40001/ui/views/widget/desktop... > > ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc:712: if > > (!::IsWindow(window_gaining_or_losing_activation)) > > On 2016/02/19 21:17:00, sky wrote: > >> Is there a reason not to put all this logic in HWNDMessageHandler? > > You' making > >> it maintain some state anyway. > > > > I did not want to add state to HWNDMessageHandler. However the code in > > WM_WINDOWPOSCHANGING necessiated that change. I thought it is better to > > have this logic in the delegate. > > > > https://codereview.chromium.org/1707233002/ > > -- > 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 mailto:chromium-reviews+unsubscribe@chromium.org. > Moved the code to HWNDMessageHandler. It is better this way as all hacks live in one file.
https://codereview.chromium.org/1707233002/diff/60001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/1707233002/diff/60001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:542: !background_fullscreen_hack()) { Shouldn't SetBounds() reset background_fullscreen_hack_? The SetBounds() called from PostPorcessActivateMessage shouldn't tell the delegate, but the if someone else calls SetBounds (it's public) then I would expect it to reset background_fullscreen_hack_. https://codereview.chromium.org/1707233002/diff/60001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2218: !background_fullscreen_hack()) || Shouldn't this reset background_fullscreen_hack_ if the size is changing in some other way? https://codereview.chromium.org/1707233002/diff/60001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/1707233002/diff/60001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:209: // Setter getter combination for a background fullscreen window, i.e a You no longer need the setter/getter.
https://codereview.chromium.org/1707233002/diff/60001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/1707233002/diff/60001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:542: !background_fullscreen_hack()) { On 2016/02/19 22:30:50, sky wrote: > Shouldn't SetBounds() reset background_fullscreen_hack_? The SetBounds() called > from PostPorcessActivateMessage shouldn't tell the delegate, but the if someone > else calls SetBounds (it's public) then I would expect it to reset > background_fullscreen_hack_. Done. Added a SetBoundsInternal private function which is called from PostProcessActivateMessage and from SetBounds. We reset the flag in the public function. https://codereview.chromium.org/1707233002/diff/60001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2218: !background_fullscreen_hack()) || On 2016/02/19 22:30:50, sky wrote: > Shouldn't this reset background_fullscreen_hack_ if the size is changing in some > other way? Done. We reset the flag if it is true and the bounds don't differ by 1px.
LGTM https://codereview.chromium.org/1707233002/diff/80001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/1707233002/diff/80001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2199: if (monitor_rect.height() - window_pos->cy != 1) nit: combing ifs.
https://codereview.chromium.org/1707233002/diff/80001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/1707233002/diff/80001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2199: if (monitor_rect.height() - window_pos->cy != 1) On 2016/02/19 23:29:24, sky wrote: > nit: combing ifs. Done.
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/1707233002/#ps100001 (title: "Combine ifs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1707233002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707233002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
SLGTM
Description was changed from ========== Reduce the fullscreen window height by 1px on activation loss and restore it back on activation gain. This is to workaround a bug/behavior of the Windows taskbar, where in it does not stay as topmost window when a window on a thread which has a fullscreen window is active. The symptom observed in this case is that the windows on the same thread as the fullscreen window end up obscuring the taskbar or cause autohide to not work. BUG=472139 TEST=Covered by views_unittest Widget.FullscreenBoundsReducedOnActivationLoss ========== to ========== Reduce the fullscreen window height by 1px on activation loss and restore it back on activation gain. This is to workaround a bug/behavior of the Windows taskbar, where in it does not stay as topmost window when a window on a thread which has a fullscreen window is active. The symptom observed in this case is that the windows on the same thread as the fullscreen window end up obscuring the taskbar or cause autohide to not work. The other changes in this patch are as below:- 1. When the bounds of the WinWindow class which serves as the PlatformWindow for the WindowTreeHost instance change, we don't activate it if the underlying HWND is not visible. 2. The WidgetTestInteractive.ViewFocusOnHWNDEnabledChanges test explicitly shows the WindowTreeHost instance which ensures that the HWND is visible and activated. BUG=472139 TEST=Covered by views_unittest Widget.FullscreenBoundsReducedOnActivationLoss ==========
Description was changed from ========== Reduce the fullscreen window height by 1px on activation loss and restore it back on activation gain. This is to workaround a bug/behavior of the Windows taskbar, where in it does not stay as topmost window when a window on a thread which has a fullscreen window is active. The symptom observed in this case is that the windows on the same thread as the fullscreen window end up obscuring the taskbar or cause autohide to not work. The other changes in this patch are as below:- 1. When the bounds of the WinWindow class which serves as the PlatformWindow for the WindowTreeHost instance change, we don't activate it if the underlying HWND is not visible. 2. The WidgetTestInteractive.ViewFocusOnHWNDEnabledChanges test explicitly shows the WindowTreeHost instance which ensures that the HWND is visible and activated. BUG=472139 TEST=Covered by views_unittest Widget.FullscreenBoundsReducedOnActivationLoss ========== to ========== Reduce the fullscreen window height by 1px on activation loss and restore it back on activation gain. This is to workaround a bug/behavior of the Windows taskbar, where in it does not stay as topmost window when a window on a thread which has a fullscreen window is active. The symptom observed in this case is that the windows on the same thread as the fullscreen window end up obscuring the taskbar or cause autohide to not work. The other changes in this patch are as below:- 1. When the bounds of the WinWindow class which serves as the PlatformWindow for the WindowTreeHost instance change, we don't activate it if the underlying HWND is not visible. 2. The WidgetTestInteractive.ViewFocusOnHWNDEnabledChanges test explicitly shows the WindowTreeHost instance which ensures that the HWND is visible and activated. BUG=472139 TEST=Covered by views_unittest Widget.FullscreenBoundsReducedOnActivationLoss ==========
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/1707233002/#ps220001 (title: "Reorganize the WidgetTest.FullscreenBoundsReducedOnActivationLoss test to make it less flaky")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1707233002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707233002/220001
The CQ bit was unchecked by ananta@chromium.org
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/1707233002/#ps260001 (title: "Revert changes to widget_unittest.cc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1707233002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707233002/260001
Message was sent while issue was closed.
Description was changed from ========== Reduce the fullscreen window height by 1px on activation loss and restore it back on activation gain. This is to workaround a bug/behavior of the Windows taskbar, where in it does not stay as topmost window when a window on a thread which has a fullscreen window is active. The symptom observed in this case is that the windows on the same thread as the fullscreen window end up obscuring the taskbar or cause autohide to not work. The other changes in this patch are as below:- 1. When the bounds of the WinWindow class which serves as the PlatformWindow for the WindowTreeHost instance change, we don't activate it if the underlying HWND is not visible. 2. The WidgetTestInteractive.ViewFocusOnHWNDEnabledChanges test explicitly shows the WindowTreeHost instance which ensures that the HWND is visible and activated. BUG=472139 TEST=Covered by views_unittest Widget.FullscreenBoundsReducedOnActivationLoss ========== to ========== Reduce the fullscreen window height by 1px on activation loss and restore it back on activation gain. This is to workaround a bug/behavior of the Windows taskbar, where in it does not stay as topmost window when a window on a thread which has a fullscreen window is active. The symptom observed in this case is that the windows on the same thread as the fullscreen window end up obscuring the taskbar or cause autohide to not work. The other changes in this patch are as below:- 1. When the bounds of the WinWindow class which serves as the PlatformWindow for the WindowTreeHost instance change, we don't activate it if the underlying HWND is not visible. 2. The WidgetTestInteractive.ViewFocusOnHWNDEnabledChanges test explicitly shows the WindowTreeHost instance which ensures that the HWND is visible and activated. BUG=472139 TEST=Covered by views_unittest Widget.FullscreenBoundsReducedOnActivationLoss ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Reduce the fullscreen window height by 1px on activation loss and restore it back on activation gain. This is to workaround a bug/behavior of the Windows taskbar, where in it does not stay as topmost window when a window on a thread which has a fullscreen window is active. The symptom observed in this case is that the windows on the same thread as the fullscreen window end up obscuring the taskbar or cause autohide to not work. The other changes in this patch are as below:- 1. When the bounds of the WinWindow class which serves as the PlatformWindow for the WindowTreeHost instance change, we don't activate it if the underlying HWND is not visible. 2. The WidgetTestInteractive.ViewFocusOnHWNDEnabledChanges test explicitly shows the WindowTreeHost instance which ensures that the HWND is visible and activated. BUG=472139 TEST=Covered by views_unittest Widget.FullscreenBoundsReducedOnActivationLoss ========== to ========== Reduce the fullscreen window height by 1px on activation loss and restore it back on activation gain. This is to workaround a bug/behavior of the Windows taskbar, where in it does not stay as topmost window when a window on a thread which has a fullscreen window is active. The symptom observed in this case is that the windows on the same thread as the fullscreen window end up obscuring the taskbar or cause autohide to not work. The other changes in this patch are as below:- 1. When the bounds of the WinWindow class which serves as the PlatformWindow for the WindowTreeHost instance change, we don't activate it if the underlying HWND is not visible. 2. The WidgetTestInteractive.ViewFocusOnHWNDEnabledChanges test explicitly shows the WindowTreeHost instance which ensures that the HWND is visible and activated. BUG=472139 TEST=Covered by views_unittest Widget.FullscreenBoundsReducedOnActivationLoss Committed: https://crrev.com/ed6530c9ab6cd4a56ffd70612be526dbc7db1552 Cr-Commit-Position: refs/heads/master@{#377084} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/ed6530c9ab6cd4a56ffd70612be526dbc7db1552 Cr-Commit-Position: refs/heads/master@{#377084} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
