|
|
DescriptionAllow Alt-Tab to move the focus to docked windows.
BUG=343237
TEST=WindowSelectorTest.BasicWithDocked, plus manual
Committed: https://crrev.com/bc374788beea179e18ecdbb190f2099b7494668d
Cr-Commit-Position: refs/heads/master@{#324155}
Patch Set 1 #
Total comments: 13
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Messages
Total messages: 22 (7 generated)
oshima@chromium.org changed reviewers: + flackr@chromium.org
https://codereview.chromium.org/1059903002/diff/1/ash/accelerators/accelerato... File ash/accelerators/accelerator_controller_unittest.cc (left): https://codereview.chromium.org/1059903002/diff/1/ash/accelerators/accelerato... ash/accelerators/accelerator_controller_unittest.cc:658: docked_container->AddChild(window.get()); This was a bug. This doesn't do what it's intended.
https://codereview.chromium.org/1059903002/diff/1/ash/switchable_windows.cc File ash/switchable_windows.cc (right): https://codereview.chromium.org/1059903002/diff/1/ash/switchable_windows.cc#n... ash/switchable_windows.cc:16: kShellWindowId_DockedContainer, Lacking MRU information, I believe we arrange window selection order by the order here. I'd expect docked windows to come before attached panels. https://codereview.chromium.org/1059903002/diff/1/ash/wm/overview/window_sele... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/1059903002/diff/1/ash/wm/overview/window_sele... ash/wm/overview/window_selector.cc:580: !IsSelectable(*iter)) { It's confusing that IsSelectable is not a subset of the windows in the mru window list (i.e. mru window list checks ash::wm::CanActivateWindow). If it was, we could remove HideAndTrackNonOverviewWindows as we would skip every remaining window with this IsSelectable test. It's probably too much for this CL, but we should check if HideAndTrackNonOverviewWindows is necessary anymore (http://crbug.com/295862) since with transient windows included in selection (http://crbug.com/304289) I think it may no longer be needed. https://codereview.chromium.org/1059903002/diff/1/ash/wm/overview/window_sele... File ash/wm/overview/window_selector_controller.cc (right): https://codereview.chromium.org/1059903002/diff/1/ash/wm/overview/window_sele... ash/wm/overview/window_selector_controller.cc:52: for (aura::Window::Windows::iterator iter = windows.begin(); Use std::remove_if for linear runtime?
https://codereview.chromium.org/1059903002/diff/1/ash/switchable_windows.cc File ash/switchable_windows.cc (right): https://codereview.chromium.org/1059903002/diff/1/ash/switchable_windows.cc#n... ash/switchable_windows.cc:16: kShellWindowId_DockedContainer, On 2015/04/07 15:08:38, flackr wrote: > Lacking MRU information, I believe we arrange window selection order by the > order here. I'd expect docked windows to come before attached panels. Thanks, done. https://codereview.chromium.org/1059903002/diff/1/ash/wm/overview/window_sele... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/1059903002/diff/1/ash/wm/overview/window_sele... ash/wm/overview/window_selector.cc:580: !IsSelectable(*iter)) { On 2015/04/07 15:08:39, flackr wrote: > It's confusing that IsSelectable is not a subset of the windows in the mru > window list (i.e. mru window list checks ash::wm::CanActivateWindow). If it was, > we could remove HideAndTrackNonOverviewWindows as we would skip every remaining > window with this IsSelectable test. > > It's probably too much for this CL, but we should check if > HideAndTrackNonOverviewWindows is necessary anymore (http://crbug.com/295862) > since with transient windows included in selection (http://crbug.com/304289) I > think it may no longer be needed. My understanding is that WindowSelector accepts window list in Init, which implies that it's independent of MRU list, at least conceptually (otherwise, Init should be able to just use MRU window list inside). If we can remove this, that'd be better. let me look into it. https://codereview.chromium.org/1059903002/diff/1/ash/wm/overview/window_sele... File ash/wm/overview/window_selector_controller.cc (right): https://codereview.chromium.org/1059903002/diff/1/ash/wm/overview/window_sele... ash/wm/overview/window_selector_controller.cc:52: for (aura::Window::Windows::iterator iter = windows.begin(); On 2015/04/07 15:08:39, flackr wrote: > Use std::remove_if for linear runtime? This is linear too. To use remove_if, I need to change IsSelectable to IsNotSelectable, or define functor. Please let me know if you prefer remove_if, and I'll update the code.
https://codereview.chromium.org/1059903002/diff/1/ash/wm/overview/window_sele... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/1059903002/diff/1/ash/wm/overview/window_sele... ash/wm/overview/window_selector.cc:580: !IsSelectable(*iter)) { On 2015/04/07 18:00:32, oshima wrote: > On 2015/04/07 15:08:39, flackr wrote: > > It's confusing that IsSelectable is not a subset of the windows in the mru > > window list (i.e. mru window list checks ash::wm::CanActivateWindow). If it > was, > > we could remove HideAndTrackNonOverviewWindows as we would skip every > remaining > > window with this IsSelectable test. > > > > It's probably too much for this CL, but we should check if > > HideAndTrackNonOverviewWindows is necessary anymore (http://crbug.com/295862) > > since with transient windows included in selection (http://crbug.com/304289) I > > think it may no longer be needed. > > My understanding is that WindowSelector accepts window list in Init, which > implies that it's independent of MRU list, at least conceptually (otherwise, > Init should be able to just use MRU window list inside). This is historical from when we engaged overview automatically after a short pause while alt tabbing. It is only ever initialized with the mru window list: https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/overview/wi... > > If we can remove this, that'd be better. let me look into it. That'd be great. https://codereview.chromium.org/1059903002/diff/1/ash/wm/overview/window_sele... File ash/wm/overview/window_selector_controller.cc (right): https://codereview.chromium.org/1059903002/diff/1/ash/wm/overview/window_sele... ash/wm/overview/window_selector_controller.cc:52: for (aura::Window::Windows::iterator iter = windows.begin(); On 2015/04/07 18:00:32, oshima wrote: > On 2015/04/07 15:08:39, flackr wrote: > > Use std::remove_if for linear runtime? > > This is linear too. To use remove_if, I need to change IsSelectable to > IsNotSelectable, or define functor. Please let me know if you prefer remove_if, > and I'll update the code. aura::Window::Windows looks to be a vector making each erase(iter) operation linear time so for lots of docked windows this would become quadratic wouldn't it?
https://codereview.chromium.org/1059903002/diff/1/ash/wm/overview/window_sele... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/1059903002/diff/1/ash/wm/overview/window_sele... ash/wm/overview/window_selector.cc:580: !IsSelectable(*iter)) { On 2015/04/07 18:00:32, oshima wrote: > On 2015/04/07 15:08:39, flackr wrote: > > It's confusing that IsSelectable is not a subset of the windows in the mru > > window list (i.e. mru window list checks ash::wm::CanActivateWindow). If it > was, > > we could remove HideAndTrackNonOverviewWindows as we would skip every > remaining > > window with this IsSelectable test. > > > > It's probably too much for this CL, but we should check if > > HideAndTrackNonOverviewWindows is necessary anymore (http://crbug.com/295862) > > since with transient windows included in selection (http://crbug.com/304289) I > > think it may no longer be needed. > > My understanding is that WindowSelector accepts window list in Init, which > implies that it's independent of MRU list, at least conceptually (otherwise, > Init should be able to just use MRU window list inside). > > If we can remove this, that'd be better. let me look into it. WindowSelectorTest.NonActivatableWindowsHiddens fails without this code. I assume we can remove this because there is no other non activatable windows to hide?
https://codereview.chromium.org/1059903002/diff/1/ash/wm/overview/window_sele... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/1059903002/diff/1/ash/wm/overview/window_sele... ash/wm/overview/window_selector.cc:580: !IsSelectable(*iter)) { On 2015/04/07 18:20:47, oshima wrote: > On 2015/04/07 18:00:32, oshima wrote: > > On 2015/04/07 15:08:39, flackr wrote: > > > It's confusing that IsSelectable is not a subset of the windows in the mru > > > window list (i.e. mru window list checks ash::wm::CanActivateWindow). If it > > was, > > > we could remove HideAndTrackNonOverviewWindows as we would skip every > > remaining > > > window with this IsSelectable test. > > > > > > It's probably too much for this CL, but we should check if > > > HideAndTrackNonOverviewWindows is necessary anymore > (http://crbug.com/295862) > > > since with transient windows included in selection (http://crbug.com/304289) > I > > > think it may no longer be needed. > > > > My understanding is that WindowSelector accepts window list in Init, which > > implies that it's independent of MRU list, at least conceptually (otherwise, > > Init should be able to just use MRU window list inside). > > > > If we can remove this, that'd be better. let me look into it. > > WindowSelectorTest.NonActivatableWindowsHiddens fails without this code. I > assume we can remove this because there is no other non activatable windows to > hide? I believe so. I can't think of any non activatable normal windows which are not transient parents or children of an activatable window.
Patchset #2 (id:20001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1059903002/diff/1/ash/wm/overview/window_sele... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/1059903002/diff/1/ash/wm/overview/window_sele... ash/wm/overview/window_selector.cc:580: !IsSelectable(*iter)) { On 2015/04/07 18:26:22, flackr wrote: > On 2015/04/07 18:20:47, oshima wrote: > > On 2015/04/07 18:00:32, oshima wrote: > > > On 2015/04/07 15:08:39, flackr wrote: > > > > It's confusing that IsSelectable is not a subset of the windows in the mru > > > > window list (i.e. mru window list checks ash::wm::CanActivateWindow). If > it > > > was, > > > > we could remove HideAndTrackNonOverviewWindows as we would skip every > > > remaining > > > > window with this IsSelectable test. > > > > > > > > It's probably too much for this CL, but we should check if > > > > HideAndTrackNonOverviewWindows is necessary anymore > > (http://crbug.com/295862) > > > > since with transient windows included in selection > (http://crbug.com/304289) > > I > > > > think it may no longer be needed. > > > > > > My understanding is that WindowSelector accepts window list in Init, which > > > implies that it's independent of MRU list, at least conceptually (otherwise, > > > Init should be able to just use MRU window list inside). > > > > > > If we can remove this, that'd be better. let me look into it. > > > > WindowSelectorTest.NonActivatableWindowsHiddens fails without this code. I > > assume we can remove this because there is no other non activatable windows to > > hide? > > I believe so. I can't think of any non activatable normal windows which are not > transient parents or children of an activatable window. Done. https://codereview.chromium.org/1059903002/diff/1/ash/wm/overview/window_sele... File ash/wm/overview/window_selector_controller.cc (right): https://codereview.chromium.org/1059903002/diff/1/ash/wm/overview/window_sele... ash/wm/overview/window_selector_controller.cc:52: for (aura::Window::Windows::iterator iter = windows.begin(); On 2015/04/07 18:10:33, flackr wrote: > On 2015/04/07 18:00:32, oshima wrote: > > On 2015/04/07 15:08:39, flackr wrote: > > > Use std::remove_if for linear runtime? > > > > This is linear too. To use remove_if, I need to change IsSelectable to > > IsNotSelectable, or define functor. Please let me know if you prefer > remove_if, > > and I'll update the code. > > aura::Window::Windows looks to be a vector making each erase(iter) operation > linear time so for lots of docked windows this would become quadratic wouldn't > it? You're absolutely right, sorry. Updated.
LGTM, thanks for cleaning up the window hiding stuff.
varkha@chromium.org changed reviewers: + varkha@chromium.org
https://codereview.chromium.org/1059903002/diff/80001/ash/wm/overview/window_... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/1059903002/diff/80001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:223: state->GetStateType() == wm::WINDOW_STATE_TYPE_DOCKED_MINIMIZED) nit: Multi-line condition needs { https://codereview.chromium.org/1059903002/diff/80001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:224: return false; Just a question: Are you considering to make docked windows selectable from overview mode in any way?
https://codereview.chromium.org/1059903002/diff/80001/ash/wm/overview/window_... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/1059903002/diff/80001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:224: return false; On 2015/04/07 21:45:45, varkha wrote: > Just a question: Are you considering to make docked windows selectable from > overview mode in any way? Yes, we should be - http://crbug.com/351329. It will likely require more testing and tweaking to make sure nothing breaks and that it looks reasonable.
https://codereview.chromium.org/1059903002/diff/80001/ash/wm/overview/window_... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/1059903002/diff/80001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:223: state->GetStateType() == wm::WINDOW_STATE_TYPE_DOCKED_MINIMIZED) On 2015/04/07 21:45:45, varkha wrote: > nit: Multi-line condition needs { Done.
lgtm
The CQ bit was checked by oshima@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org Link to the patchset: https://codereview.chromium.org/1059903002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1059903002/100001
Message was sent while issue was closed.
Committed patchset #3 (id:100001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/bc374788beea179e18ecdbb190f2099b7494668d Cr-Commit-Position: refs/heads/master@{#324155} |