Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2972963004:
Another attempt at fixing the fullscreen issues with Chrome. (Closed)
DescriptionAnother attempt at fixing the fullscreen issues with Chrome.
The ITaskbarList2 shell interface provides a way to mark a window as fullscreen. As per
msdn https://msdn.microsoft.com/en-us/library/bb774640(v=VS.85).aspx the MarkFullscreenWindow()
method on this interface when called for a fullscreen window ensures that the shell moves the taskbar
to the bottom of the ZOrder when the window is active. When the window is not fullscreen, it fallsback
to its heuristics which include looking at the styles of the window, whether it is maximized etc, which
sadly means that a non fullscreen window could be treated as a fullscreen window.
In any case this should hopefully help us make incremental progress on this issue which has
been affecting users for a long time.
BUG=604359
Review-Url: https://codereview.chromium.org/2972963004
Cr-Commit-Position: refs/heads/master@{#484753}
Committed: https://chromium.googlesource.com/chromium/src/+/9c35b80be7b82051abec46fabdcdef6be4b214a2
Patch Set 1 #Patch Set 2 : Rephrase comment #
Total comments: 14
Patch Set 3 : Address review comments #
Total comments: 4
Patch Set 4 : Address review comments #
Messages
Total messages: 27 (19 generated)
ananta@chromium.org changed reviewers: + sky@chromium.org
The CQ bit was checked by ananta@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.
Description was changed from ========== Another attempt at fixing the fullscreen issues with Chrome. The ITaskbarList2 shell interface provides a way to mark a window as fullscreen. As per msdn https://msdn.microsoft.com/en-us/library/bb774640(v=VS.85).aspx the MarkFullscreenWindow() method on this interface when called for a fullscreen window ensures that the shell moves the taskbar to the bottom of the ZOrder when the window is active. When the window is not fullscreen, it fallsback to its heuristics which include looking at the styles of the window, whether it is maximized etc, which sadly means that a non fullscreen window could be treated as a fullscreen window. In any case this should hopefully help us make incremental progress on this issue which has been affecting users for a long time. BUG=604359 ========== to ========== Another attempt at fixing the fullscreen issues with Chrome. The ITaskbarList2 shell interface provides a way to mark a window as fullscreen. As per msdn https://msdn.microsoft.com/en-us/library/bb774640(v=VS.85).aspx the MarkFullscreenWindow() method on this interface when called for a fullscreen window ensures that the shell moves the taskbar to the bottom of the ZOrder when the window is active. When the window is not fullscreen, it fallsback to its heuristics which include looking at the styles of the window, whether it is maximized etc, which sadly means that a non fullscreen window could be treated as a fullscreen window. In any case this should hopefully help us make incremental progress on this issue which has been affecting users for a long time. BUG=604359 ==========
ananta@chromium.org changed reviewers: + msw@chromium.org
+msw
lgtm with minor nits and some questions. It seems like this can't hurt. https://codereview.chromium.org/2972963004/diff/20001/ui/views/win/fullscreen... File ui/views/win/fullscreen_handler.cc (right): https://codereview.chromium.org/2972963004/diff/20001/ui/views/win/fullscreen... ui/views/win/fullscreen_handler.cc:29: if (!task_bar_list_) { It's a little odd to init the value here and use it unchecked in the impl function, but that isn't called anywhere else, so it's probably fine. https://codereview.chromium.org/2972963004/diff/20001/ui/views/win/fullscreen... ui/views/win/fullscreen_handler.cc:81: SWP_NOZORDER | SWP_NOACTIVATE | SWP_FRAMECHANGED); q: Maybe these args let windows enter fullscreen when they are *already z-ordered below the taskbar*, and by explicitly not changing the activation or z-order, we leave the fullscreen window behind the taskbar? I'm not sure why we don't activate or re-order the window when it enters fullscreen. Perhaps we should consider adjusting/removing SWP_NOZORDER ("Retains the current Z order (ignores the hWndInsertAfter parameter).") and SWP_NOACTIVATE ("Does not activate the window. If this flag is not set, the window is activated and moved to the top of either the topmost or non-topmost group (depending on the setting of the hWndInsertAfter parameter)."), maybe in a separate CL? https://codereview.chromium.org/2972963004/diff/20001/ui/views/win/fullscreen... ui/views/win/fullscreen_handler.cc:94: } nit: add a blank line after this https://codereview.chromium.org/2972963004/diff/20001/ui/views/win/fullscreen... ui/views/win/fullscreen_handler.cc:95: // As per msdn marking the window as fullscreen should ensure that the nit: capitalize 'MSDN' https://codereview.chromium.org/2972963004/diff/20001/ui/views/win/fullscreen... ui/views/win/fullscreen_handler.cc:96: // taskbar is moved to the bottom of the Z order when the fullscreen window nit: hyphenate 'Z-order' https://codereview.chromium.org/2972963004/diff/20001/ui/views/win/fullscreen... ui/views/win/fullscreen_handler.cc:97: // is activated. If the window is not fullscreen the Shell fallsback to nits: comma after 'fullscreen,' and split words 'falls back' https://codereview.chromium.org/2972963004/diff/20001/ui/views/win/fullscreen... ui/views/win/fullscreen_handler.cc:100: task_bar_list_->MarkFullscreenWindow(hwnd_, !!fullscreen); As this only moves the taskbar to the bottom of the Z-order *when the window is active/activated*, perhaps this won't help inactive fullscreen windows (but let users fix the defect by just clicking the window?). This is another reason to consider adjusting the SetWindowPos args when entering fullscreen. I'm mostly just thinking aloud.
https://codereview.chromium.org/2972963004/diff/20001/ui/views/win/fullscreen... File ui/views/win/fullscreen_handler.cc (right): https://codereview.chromium.org/2972963004/diff/20001/ui/views/win/fullscreen... ui/views/win/fullscreen_handler.cc:29: if (!task_bar_list_) { On 2017/07/06 18:32:55, msw wrote: > It's a little odd to init the value here and use it unchecked in the impl > function, but that isn't called anywhere else, so it's probably fine. Thanks. I moved this block to the SetFullscreenImpl() function. https://codereview.chromium.org/2972963004/diff/20001/ui/views/win/fullscreen... ui/views/win/fullscreen_handler.cc:81: SWP_NOZORDER | SWP_NOACTIVATE | SWP_FRAMECHANGED); On 2017/07/06 18:32:55, msw wrote: > q: Maybe these args let windows enter fullscreen when they are *already > z-ordered below the taskbar*, and by explicitly not changing the activation or > z-order, we leave the fullscreen window behind the taskbar? I'm not sure why we > don't activate or re-order the window when it enters fullscreen. Perhaps we > should consider adjusting/removing SWP_NOZORDER ("Retains the current Z order > (ignores the hWndInsertAfter parameter).") and SWP_NOACTIVATE ("Does not > activate the window. If this flag is not set, the window is activated and moved > to the top of either the topmost or non-topmost group (depending on the setting > of the hWndInsertAfter parameter)."), maybe in a separate CL? The window should be active when we enter this function in most cases. However if for some reason we lost activation, due to another window on the thread becoming active, etc we don't want to activate the window again here. https://codereview.chromium.org/2972963004/diff/20001/ui/views/win/fullscreen... ui/views/win/fullscreen_handler.cc:94: } On 2017/07/06 18:32:55, msw wrote: > nit: add a blank line after this Done. https://codereview.chromium.org/2972963004/diff/20001/ui/views/win/fullscreen... ui/views/win/fullscreen_handler.cc:95: // As per msdn marking the window as fullscreen should ensure that the On 2017/07/06 18:32:55, msw wrote: > nit: capitalize 'MSDN' Done. https://codereview.chromium.org/2972963004/diff/20001/ui/views/win/fullscreen... ui/views/win/fullscreen_handler.cc:96: // taskbar is moved to the bottom of the Z order when the fullscreen window On 2017/07/06 18:32:55, msw wrote: > nit: hyphenate 'Z-order' Done. https://codereview.chromium.org/2972963004/diff/20001/ui/views/win/fullscreen... ui/views/win/fullscreen_handler.cc:97: // is activated. If the window is not fullscreen the Shell fallsback to On 2017/07/06 18:32:55, msw wrote: > nits: comma after 'fullscreen,' and split words 'falls back' Done. https://codereview.chromium.org/2972963004/diff/20001/ui/views/win/fullscreen... ui/views/win/fullscreen_handler.cc:100: task_bar_list_->MarkFullscreenWindow(hwnd_, !!fullscreen); On 2017/07/06 18:32:55, msw wrote: > As this only moves the taskbar to the bottom of the Z-order *when the window is > active/activated*, perhaps this won't help inactive fullscreen windows (but let > users fix the defect by just clicking the window?). This is another reason to > consider adjusting the SetWindowPos args when entering fullscreen. I'm mostly > just thinking aloud. I think the reason we use SWP_NOACTIVATE is to avoid activating the window again in case it lost activation before it became fullscreen due to the user clicking elsewhere. However marking the window fullscreen here would ensure that if the user switches to the fullscreen window the taskbar would go to the bottom of the Z-order
The CQ bit was checked by ananta@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...
Still lgtm with nits. Hopefully this helps, at least on click to activate. https://codereview.chromium.org/2972963004/diff/40001/ui/views/win/fullscreen... File ui/views/win/fullscreen_handler.cc (right): https://codereview.chromium.org/2972963004/diff/40001/ui/views/win/fullscreen... ui/views/win/fullscreen_handler.cc:42: if (!task_bar_list_) { optional nit: move this down to just above the task_bar_list_ usage. https://codereview.chromium.org/2972963004/diff/40001/ui/views/win/fullscreen... ui/views/win/fullscreen_handler.cc:97: // taskbar is moved to the bottom of the Z- order when the fullscreen window nit: no space after hyphen ('Z-order')
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2972963004/diff/40001/ui/views/win/fullscreen... File ui/views/win/fullscreen_handler.cc (right): https://codereview.chromium.org/2972963004/diff/40001/ui/views/win/fullscreen... ui/views/win/fullscreen_handler.cc:42: if (!task_bar_list_) { On 2017/07/06 19:33:52, msw wrote: > optional nit: move this down to just above the task_bar_list_ usage. Done. https://codereview.chromium.org/2972963004/diff/40001/ui/views/win/fullscreen... ui/views/win/fullscreen_handler.cc:97: // taskbar is moved to the bottom of the Z- order when the fullscreen window On 2017/07/06 19:33:52, msw wrote: > nit: no space after hyphen ('Z-order') Thanks done.
The CQ bit was checked by ananta@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.
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2972963004/#ps60001 (title: "Address review comments")
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": 60001, "attempt_start_ts": 1499377846103510,
"parent_rev": "b10ef0b9cfc8c2de5c363275a6af8068e58df525", "commit_rev":
"9c35b80be7b82051abec46fabdcdef6be4b214a2"}
Message was sent while issue was closed.
Description was changed from ========== Another attempt at fixing the fullscreen issues with Chrome. The ITaskbarList2 shell interface provides a way to mark a window as fullscreen. As per msdn https://msdn.microsoft.com/en-us/library/bb774640(v=VS.85).aspx the MarkFullscreenWindow() method on this interface when called for a fullscreen window ensures that the shell moves the taskbar to the bottom of the ZOrder when the window is active. When the window is not fullscreen, it fallsback to its heuristics which include looking at the styles of the window, whether it is maximized etc, which sadly means that a non fullscreen window could be treated as a fullscreen window. In any case this should hopefully help us make incremental progress on this issue which has been affecting users for a long time. BUG=604359 ========== to ========== Another attempt at fixing the fullscreen issues with Chrome. The ITaskbarList2 shell interface provides a way to mark a window as fullscreen. As per msdn https://msdn.microsoft.com/en-us/library/bb774640(v=VS.85).aspx the MarkFullscreenWindow() method on this interface when called for a fullscreen window ensures that the shell moves the taskbar to the bottom of the ZOrder when the window is active. When the window is not fullscreen, it fallsback to its heuristics which include looking at the styles of the window, whether it is maximized etc, which sadly means that a non fullscreen window could be treated as a fullscreen window. In any case this should hopefully help us make incremental progress on this issue which has been affecting users for a long time. BUG=604359 Review-Url: https://codereview.chromium.org/2972963004 Cr-Commit-Position: refs/heads/master@{#484753} Committed: https://chromium.googlesource.com/chromium/src/+/9c35b80be7b82051abec46fabdcd... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/9c35b80be7b82051abec46fabdcd... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
