Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2225683005:
[ash-md] Prevents auto-positioning after cancelling overview (Closed)
Description[ash-md] Prevents auto-positioning after cancelling overview
This fixes the bug differently. Rother than disabling the
auto-positioning for specific types of display metrics changes it is
disabled for any display metrics changes but only after Shutdown has
been called. Consider a situation when display metrics change while
overview mode is active - it should respond by auto-positioning windows.
BUG=634489
Committed: https://crrev.com/e1280a8d37f97c49f121a6628a3b7543add3a68c
Cr-Commit-Position: refs/heads/master@{#410648}
Patch Set 1 #
Total comments: 2
Patch Set 2 : [ash-md] Prevents auto-positioning after cancelling overview (simpler) #
Total comments: 2
Messages
Total messages: 16 (7 generated)
varkha@chromium.org changed reviewers: + hariank@google.com, oshima@chromium.org, reveman@chromium.org
Please consider this alternative to https://codereview.chromium.org/2225523002. I think this is a safer approach with fewer potential side effects. Thanks!
https://codereview.chromium.org/2225683005/diff/1/ash/common/wm/overview/wind... File ash/common/wm/overview/window_selector.cc (right): https://codereview.chromium.org/2225683005/diff/1/ash/common/wm/overview/wind... ash/common/wm/overview/window_selector.cc:385: shutting_down_ = true; can we just remove observer?
https://codereview.chromium.org/2225683005/diff/1/ash/common/wm/overview/wind... File ash/common/wm/overview/window_selector.cc (right): https://codereview.chromium.org/2225683005/diff/1/ash/common/wm/overview/wind... ash/common/wm/overview/window_selector.cc:385: shutting_down_ = true; On 2016/08/08 22:47:37, oshima wrote: > can we just remove observer? Yes, I considered this before but thought for some reason that we DCHECK on repeated removal but we don't, at least not implicitly. Done.
The CQ bit was checked by varkha@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.
lgtm with one q. https://codereview.chromium.org/2225683005/diff/20001/ash/common/wm/overview/... File ash/common/wm/overview/window_selector.cc (left): https://codereview.chromium.org/2225683005/diff/20001/ash/common/wm/overview/... ash/common/wm/overview/window_selector.cc:558: if (metrics != DISPLAY_METRIC_WORK_AREA) { Note that we used to ignore work area only metrics change and this was to keep the same behavior. Are you sure it's safer?
https://codereview.chromium.org/2225683005/diff/20001/ash/common/wm/overview/... File ash/common/wm/overview/window_selector.cc (left): https://codereview.chromium.org/2225683005/diff/20001/ash/common/wm/overview/... ash/common/wm/overview/window_selector.cc:558: if (metrics != DISPLAY_METRIC_WORK_AREA) { On 2016/08/09 10:59:56, oshima wrote: > Note that we used to ignore work area only metrics change and this was to keep > the same behavior. Are you sure it's safer? Yes, this callback is here to position windows and UI elements while in overview mode when work area changes. Work area insets such as from the shelf, ChromeVox, etc. should trigger repositioning. While we attempt to block those insets from changing in overview I think it is safer to still respond to those changes while still in overview. So for instance, if there is some shortcut to activate ChromeVox while in overview, the new code will deal with it while the old code may allow UI elements to overlap.
The CQ bit was checked by varkha@chromium.org
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 #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [ash-md] Prevents auto-positioning after cancelling overview This fixes the bug differently. Rother than disabling the auto-positioning for specific types of display metrics changes it is disabled for any display metrics changes but only after Shutdown has been called. Consider a situation when display metrics change while overview mode is active - it should respond by auto-positioning windows. BUG=634489 ========== to ========== [ash-md] Prevents auto-positioning after cancelling overview This fixes the bug differently. Rother than disabling the auto-positioning for specific types of display metrics changes it is disabled for any display metrics changes but only after Shutdown has been called. Consider a situation when display metrics change while overview mode is active - it should respond by auto-positioning windows. BUG=634489 Committed: https://crrev.com/e1280a8d37f97c49f121a6628a3b7543add3a68c Cr-Commit-Position: refs/heads/master@{#410648} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e1280a8d37f97c49f121a6628a3b7543add3a68c Cr-Commit-Position: refs/heads/master@{#410648}
Message was sent while issue was closed.
On 2016/08/09 13:13:39, varkha wrote: > https://codereview.chromium.org/2225683005/diff/20001/ash/common/wm/overview/... > File ash/common/wm/overview/window_selector.cc (left): > > https://codereview.chromium.org/2225683005/diff/20001/ash/common/wm/overview/... > ash/common/wm/overview/window_selector.cc:558: if (metrics != > DISPLAY_METRIC_WORK_AREA) { > On 2016/08/09 10:59:56, oshima wrote: > > Note that we used to ignore work area only metrics change and this was to keep > > the same behavior. Are you sure it's safer? > > Yes, this callback is here to position windows and UI elements while in overview > mode when work area changes. Work area insets such as from the shelf, ChromeVox, > etc. should trigger repositioning. > While we attempt to block those insets from > changing in overview I think it is safer to still respond to those changes while > still in overview. That was my question, especially it *wasn't* doing it. Anyhow we'll see how it goes. > So for instance, if there is some shortcut to activate ChromeVox while in > overview, the new code will deal with it while the old code may allow UI > elements to overlap. |
