Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2133333002:
[ash-md] Allows docked windows to be selected in overview mode (Closed)
Description[ash-md] Allows docked windows to be selected in overview mode
BUG=351329
Committed: https://crrev.com/0eeecd3b1104e9038377103085198c9edd9eb6f1
Cr-Commit-Position: refs/heads/master@{#405834}
Patch Set 1 #Patch Set 2 : [ash-md] Allows docked windows to be selected in overview mode (updated test) #Patch Set 3 : [ash-md] Allows docked windows to be selected in overview mode (rebased) #
Total comments: 12
Patch Set 4 : [ash-md] Allows docked windows to be selected in overview mode (comments + tests) #
Messages
Total messages: 35 (27 generated)
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...
varkha@chromium.org changed reviewers: + bruthig@chromium.org
bruthig@, can you please take a look? This makes docked and docked-minimized windows appear in overview. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
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: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Added some higher level thoughts. https://codereview.chromium.org/2133333002/diff/40001/ash/common/wm/dock/dock... File ash/common/wm/dock/docked_window_layout_manager.cc (right): https://codereview.chromium.org/2133333002/diff/40001/ash/common/wm/dock/dock... ash/common/wm/dock/docked_window_layout_manager.cc:12: #include "ash/common/wm/overview/window_selector_controller.h" Instead of making the DockedWindowLayoutManager dependent on the WindowSelector and reaching out to the WindowSelector for state, would it be possible/feasible/better to add the necessary configurations on the DockedWindowLayoutManager and allow some other mediator to configure those as necessary. See the Mediator design pattern: https://sourcemaking.com/design_patterns/mediator https://codereview.chromium.org/2133333002/diff/40001/ash/common/wm/dock/dock... ash/common/wm/dock/docked_window_layout_manager.cc:918: void DockedWindowLayoutManager::OnShelfAlignmentChanged(WmWindow* root_window) { Do you need to override this? https://codereview.chromium.org/2133333002/diff/40001/ash/common/wm/dock/dock... ash/common/wm/dock/docked_window_layout_manager.cc:923: WmWindow* root_window) {} Do you need to override this? https://codereview.chromium.org/2133333002/diff/40001/ash/common/wm/dock/dock... ash/common/wm/dock/docked_window_layout_manager.cc:928: in_overview_ = true; I think |in_overview_| should either always be set, before the short circuit return above, or should be renamed so that it's obvious it will only ever be true in MD. e.g. |in_md_overview_|. https://codereview.chromium.org/2133333002/diff/40001/ash/common/wm/overview/... File ash/common/wm/overview/scoped_transform_overview_window.cc (right): https://codereview.chromium.org/2133333002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/scoped_transform_overview_window.cc:250: docked_minimized_(window->GetWindowState()->GetStateType() == Would it make more sense to define an enum type instead of two bools where 1 state, i.e. docked_minimized_=True && minimized_=True, is invalid? It might make the if conditionals below easier to understand too. https://codereview.chromium.org/2133333002/diff/40001/ash/wm/overview/window_... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/2133333002/diff/40001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:108: class WindowSelectorTest Are there any useful tests that can be added now that you can close docked windows in overview mode?
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 to run a CQ dry run
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2133333002/diff/40001/ash/common/wm/dock/dock... File ash/common/wm/dock/docked_window_layout_manager.cc (right): https://codereview.chromium.org/2133333002/diff/40001/ash/common/wm/dock/dock... ash/common/wm/dock/docked_window_layout_manager.cc:12: #include "ash/common/wm/overview/window_selector_controller.h" On 2016/07/12 21:58:52, bruthig wrote: > Instead of making the DockedWindowLayoutManager dependent on the WindowSelector > and reaching out to the WindowSelector for state, would it be > possible/feasible/better to add the necessary configurations on the > DockedWindowLayoutManager and allow some other mediator to configure those as > necessary. See the Mediator design pattern: > https://sourcemaking.com/design_patterns/mediator Talked about this offline. Shell is one such mediator currently and is used for "big" state updates. In this case the tweaks are small and making them propagate via a mediator will I think complicate the class structure and make it very hard to reason about cyclical updates. https://codereview.chromium.org/2133333002/diff/40001/ash/common/wm/dock/dock... ash/common/wm/dock/docked_window_layout_manager.cc:918: void DockedWindowLayoutManager::OnShelfAlignmentChanged(WmWindow* root_window) { On 2016/07/12 21:58:52, bruthig wrote: > Do you need to override this? Yes, because there is a method with the same name in another interface that this class implements. https://codereview.chromium.org/2133333002/diff/40001/ash/common/wm/dock/dock... ash/common/wm/dock/docked_window_layout_manager.cc:923: WmWindow* root_window) {} On 2016/07/12 21:58:52, bruthig wrote: > Do you need to override this? Yes, same reason. Compiler complains if I don't. https://codereview.chromium.org/2133333002/diff/40001/ash/common/wm/dock/dock... ash/common/wm/dock/docked_window_layout_manager.cc:928: in_overview_ = true; On 2016/07/12 21:58:52, bruthig wrote: > I think |in_overview_| should either always be set, before the short circuit > return above, or should be renamed so that it's obvious it will only ever be > true in MD. e.g. |in_md_overview_|. Done. https://codereview.chromium.org/2133333002/diff/40001/ash/common/wm/overview/... File ash/common/wm/overview/scoped_transform_overview_window.cc (right): https://codereview.chromium.org/2133333002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/scoped_transform_overview_window.cc:250: docked_minimized_(window->GetWindowState()->GetStateType() == On 2016/07/12 21:58:52, bruthig wrote: > Would it make more sense to define an enum type instead of two bools where 1 > state, i.e. docked_minimized_=True && minimized_=True, is invalid? > > It might make the if conditionals below easier to understand too. Done. Definitely better. https://codereview.chromium.org/2133333002/diff/40001/ash/wm/overview/window_... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/2133333002/diff/40001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:108: class WindowSelectorTest On 2016/07/12 21:58:52, bruthig wrote: > Are there any useful tests that can be added now that you can close docked > windows in overview mode? Done, added some. See if those help.
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
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 #4 (id:60001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [ash-md] Allows docked windows to be selected in overview mode BUG=351329 ========== to ========== [ash-md] Allows docked windows to be selected in overview mode BUG=351329 Committed: https://crrev.com/0eeecd3b1104e9038377103085198c9edd9eb6f1 Cr-Commit-Position: refs/heads/master@{#405834} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0eeecd3b1104e9038377103085198c9edd9eb6f1 Cr-Commit-Position: refs/heads/master@{#405834} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
