Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2244173006:
Reset the focus after windows are restored, and grids are shutdown. (Closed)
DescriptionResetFocusRestoreWindow may change activation, which can resize the window, which then results in reposition. This prevents it by updating the focus after grids are shutdown.
BUG=634572
Committed: https://crrev.com/e26c7e4886eec4d2c58a5810b6440f7faa4293ea
Cr-Commit-Position: refs/heads/master@{#412384}
Patch Set 1 #
Total comments: 4
Patch Set 2 : removed obsolete comment #Patch Set 3 : . #
Total comments: 4
Patch Set 4 : fixd typo #Messages
Total messages: 24 (15 generated)
Description was changed from ========== Restore overview windows after possible resizing happened BUG= ========== to ========== Restore overview windows after possible resizing happened BUG=634572 ==========
The CQ bit was checked by oshima@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...
oshima@chromium.org changed reviewers: + varkha@chromium.org
Description was changed from ========== Restore overview windows after possible resizing happened BUG=634572 ========== to ========== Restore overview windows after possible resizing happened Resize can happen during activation/deactivation, which results in reposition. This prevents it by restoring after restoring focus. BUG=634572 ==========
Description was changed from ========== Restore overview windows after possible resizing happened Resize can happen during activation/deactivation, which results in reposition. This prevents it by restoring after restoring focus. BUG=634572 ========== to ========== Restore overview windows after possible resizing happened ResetFocusRestoreWindow may change activation, which can resize the window, which then results in reposition. This prevents it by restoring after restoring focus. BUG=634572 ==========
Thanks for looking into this. Have a question. https://codereview.chromium.org/2244173006/diff/1/ash/common/wm/overview/wind... File ash/common/wm/overview/window_selector.cc (right): https://codereview.chromium.org/2244173006/diff/1/ash/common/wm/overview/wind... ash/common/wm/overview/window_selector.cc:387: // Setting focus after restoring windows' state avoids unnecessary animations. This comment that I have added in https://codereview.chromium.org/2133333002/diff/60001/ash/common/wm/overview/... no longer makes sense. https://codereview.chromium.org/2244173006/diff/1/ash/common/wm/overview/wind... ash/common/wm/overview/window_selector.cc:402: remaining_items += window_grid->size(); Hmm. This seems right, I probably did the wrong thing in https://codereview.chromium.org/2133333002/ when I have moved the loop restoring windows above the set focus. I have done it for a reason though - there was some badness that I need to check when restoring and updating the layout of the dock upon exiting overview with the newly activated window which could be displacing other items in the dock. Would be good to make sure we are not regressing on something plainly visible with the dock. Maybe try docking 3-4 windows, minimizing one of them and then using overview to restore it thus displacing some other docked window and causing that window to get minimized. See if this scenario still works as expected.
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 oshima@chromium.org to run a CQ dry run
https://codereview.chromium.org/2244173006/diff/1/ash/common/wm/overview/wind... File ash/common/wm/overview/window_selector.cc (right): https://codereview.chromium.org/2244173006/diff/1/ash/common/wm/overview/wind... ash/common/wm/overview/window_selector.cc:387: // Setting focus after restoring windows' state avoids unnecessary animations. On 2016/08/15 23:09:25, varkha wrote: > This comment that I have added in > https://codereview.chromium.org/2133333002/diff/60001/ash/common/wm/overview/... > no longer makes sense. Done. https://codereview.chromium.org/2244173006/diff/1/ash/common/wm/overview/wind... ash/common/wm/overview/window_selector.cc:402: remaining_items += window_grid->size(); On 2016/08/15 23:09:25, varkha wrote: > Hmm. This seems right, I probably did the wrong thing in > https://codereview.chromium.org/2133333002/ when I have moved the loop restoring > windows above the set focus. I have done it for a reason though - there was some > badness that I need to check when restoring and updating the layout of the dock > upon exiting overview with the newly activated window which could be displacing > other items in the dock. > Would be good to make sure we are not regressing on something plainly visible > with the dock. Maybe try docking 3-4 windows, minimizing one of them and then > using overview to restore it thus displacing some other docked window and > causing that window to get minimized. See if this scenario still works as > expected. Thanks for the info. Looks like this has to be after restoring window because DockedWindowLayoutManager can use wrong window state to re-layuout docked windows otherwise. Looks like we can simply move ResetFocusRestoreWindow(true) to the last, and it looks safe. I also tested various scenario as well.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Restore overview windows after possible resizing happened ResetFocusRestoreWindow may change activation, which can resize the window, which then results in reposition. This prevents it by restoring after restoring focus. BUG=634572 ========== to ========== ResetFocusRestoreWindow may change activation, which can resize the window, which then results in reposition. This prevents it by updating the focus after grids are shutdown. BUG=634572 ==========
This seems like a right thing to do here. LGTM. https://codereview.chromium.org/2244173006/diff/40001/ash/common/wm/overview/... File ash/common/wm/overview/window_selector.cc (right): https://codereview.chromium.org/2244173006/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector.cc:408: // 1) DockeWindowLayoutManager uses the the restored state to re-layout upon nit: the the https://codereview.chromium.org/2244173006/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector.cc:410: // 2) Any bounds change due to activation should not result in repositioin. nit: typo - repositioin.
https://codereview.chromium.org/2244173006/diff/40001/ash/common/wm/overview/... File ash/common/wm/overview/window_selector.cc (right): https://codereview.chromium.org/2244173006/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector.cc:408: // 1) DockeWindowLayoutManager uses the the restored state to re-layout upon On 2016/08/16 22:38:35, varkha wrote: > nit: the the Done. https://codereview.chromium.org/2244173006/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector.cc:410: // 2) Any bounds change due to activation should not result in repositioin. On 2016/08/16 22:38:35, varkha wrote: > nit: typo - repositioin. Done.
The CQ bit was checked by oshima@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from varkha@chromium.org Link to the patchset: https://codereview.chromium.org/2244173006/#ps60001 (title: "fixd typo")
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.
Description was changed from ========== ResetFocusRestoreWindow may change activation, which can resize the window, which then results in reposition. This prevents it by updating the focus after grids are shutdown. BUG=634572 ========== to ========== ResetFocusRestoreWindow may change activation, which can resize the window, which then results in reposition. This prevents it by updating the focus after grids are shutdown. BUG=634572 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== ResetFocusRestoreWindow may change activation, which can resize the window, which then results in reposition. This prevents it by updating the focus after grids are shutdown. BUG=634572 ========== to ========== ResetFocusRestoreWindow may change activation, which can resize the window, which then results in reposition. This prevents it by updating the focus after grids are shutdown. BUG=634572 Committed: https://crrev.com/e26c7e4886eec4d2c58a5810b6440f7faa4293ea Cr-Commit-Position: refs/heads/master@{#412384} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e26c7e4886eec4d2c58a5810b6440f7faa4293ea Cr-Commit-Position: refs/heads/master@{#412384}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2251053003/ by oshima@chromium.org. The reason for reverting is: Reverting because this can cause crash with arc++ windows.. |
