Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22715005:
Use overview mode for alt-tab cycling. (Closed)
|
Created:
7 years, 4 months ago by flackr Modified:
7 years, 4 months ago CC:
chromium-reviews, sadrul, ben+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUse overview mode for alt-tab cycling.
BUG=263481, 264289
TEST=WindowSelectorTest.BasicCycle
TEST=Enable --ash-enable-overview-mode and use alt-tab to cycle through MRU windows. Cycling is done in overview mode on active display.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216874
Patch Set 1 #
Total comments: 25
Patch Set 2 : Address comments. #
Messages
Total messages: 12 (0 generated)
Hi James, Would you mind reviewing this patch to enable alt-tab cycling in overview mode (when the flag is enabled)? Sorry for the size, but it required significant refactoring of the existing overview code to support moving windows to another display. Thanks!
On 2013/08/09 17:57:27, flackr wrote: > Hi James, > > Would you mind reviewing this patch to enable alt-tab cycling in overview mode > (when the flag is enabled)? Sorry for the size, but it required significant > refactoring of the existing overview code to support moving windows to another > display. > > Thanks! I'll look, but I probably won't get to it today.
Load balancing code reviews -> to Dan
LGTM with nits https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.cc File ash/wm/window_selector.cc (right): https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.cc#new... ash/wm/window_selector.cc:56: widget->GetNativeWindow()->SetName("OverviewWindowCopy"); nit: include |src_window|'s id and/or name here? https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.cc#new... ash/wm/window_selector.cc:214: WindowSelectorWindow::WindowSelectorWindow(aura::Window* window) doesn't need to happen here, but it'd probably be good for this to be moved into a subdirectory under wm/ and split up into separate files for each of these classes. https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.cc#new... ash/wm/window_selector.cc:292: layer_ = views::corewm::RecreateWindowLayers(window_, true); nit: can you DCHECK(!layer_) and DCHECK(!window_copy_) here? https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.cc#new... ash/wm/window_selector.cc:310: bool operator() (const WindowSelectorWindow* window) const { nit: remove space after operator() https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.cc#new... ash/wm/window_selector.cc:434: if (selection_root_) { // mode == CYCLE nit: is it worthwhile to add DCHECK_EQ(mode_, CYCLE) here and a similar OVERVIEW DCHECK below? https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.cc#new... ash/wm/window_selector.cc:436: for (size_t i = 0; i < windows_.size(); ++i) { nit: don't need curly braces here https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.cc#new... ash/wm/window_selector.cc:442: for (size_t i = 0; i < root_window_list.size(); ++i) { nit: or here https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.h File ash/wm/window_selector.h (right): https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.h#newc... ash/wm/window_selector.h:88: void InitializeSelectionWidget(); nit: add a blank line after this line https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.h#newc... ash/wm/window_selector.h:110: // If not NULL, this is the root window selection is taking place in used nit: i found this a bit hard to parse. how about the following? // In CYCLE mode, the root window in which selection is taking place. // NULL otherwise. https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector_contro... File ash/wm/window_selector_controller.cc (right): https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector_contro... ash/wm/window_selector_controller.cc:27: virtual void OnKeyEvent(ui::KeyEvent* event) OVERRIDE; nit: add a blank line after this one https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector_unitte... File ash/wm/window_selector_unittest.cc (right): https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector_unitte... ash/wm/window_selector_unittest.cc:115: for (size_t i = 0; i < windows.size(); ++i) { nit: don't need curly brackets here https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector_unitte... ash/wm/window_selector_unittest.cc:120: for (size_t i = 0; i < animations.size(); ++i) { nit: or here
https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.cc File ash/wm/window_selector.cc (right): https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.cc#new... ash/wm/window_selector.cc:56: widget->GetNativeWindow()->SetName("OverviewWindowCopy"); On 2013/08/09 20:54:35, Daniel Erat wrote: > nit: include |src_window|'s id and/or name here? Done. https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.cc#new... ash/wm/window_selector.cc:214: WindowSelectorWindow::WindowSelectorWindow(aura::Window* window) On 2013/08/09 20:54:35, Daniel Erat wrote: > doesn't need to happen here, but it'd probably be good for this to be moved into > a subdirectory under wm/ and split up into separate files for each of these > classes. Added TODO. https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.cc#new... ash/wm/window_selector.cc:292: layer_ = views::corewm::RecreateWindowLayers(window_, true); On 2013/08/09 20:54:35, Daniel Erat wrote: > nit: can you DCHECK(!layer_) and DCHECK(!window_copy_) here? Added DCHECK(!layer_), I'm not sure I follow the DCHECK(!window_copy_), it may already exist (and is checked for) if the layout is updating in response to a deleted window. https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.cc#new... ash/wm/window_selector.cc:310: bool operator() (const WindowSelectorWindow* window) const { On 2013/08/09 20:54:35, Daniel Erat wrote: > nit: remove space after operator() Done. https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.cc#new... ash/wm/window_selector.cc:434: if (selection_root_) { // mode == CYCLE On 2013/08/09 20:54:35, Daniel Erat wrote: > nit: is it worthwhile to add DCHECK_EQ(mode_, CYCLE) here and a similar OVERVIEW > DCHECK below? Done. https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.cc#new... ash/wm/window_selector.cc:436: for (size_t i = 0; i < windows_.size(); ++i) { On 2013/08/09 20:54:35, Daniel Erat wrote: > nit: don't need curly braces here Done. https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.cc#new... ash/wm/window_selector.cc:442: for (size_t i = 0; i < root_window_list.size(); ++i) { On 2013/08/09 20:54:35, Daniel Erat wrote: > nit: or here Done. https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.h File ash/wm/window_selector.h (right): https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.h#newc... ash/wm/window_selector.h:88: void InitializeSelectionWidget(); On 2013/08/09 20:54:35, Daniel Erat wrote: > nit: add a blank line after this line Done. https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.h#newc... ash/wm/window_selector.h:110: // If not NULL, this is the root window selection is taking place in used On 2013/08/09 20:54:35, Daniel Erat wrote: > nit: i found this a bit hard to parse. how about the following? > > // In CYCLE mode, the root window in which selection is taking place. > // NULL otherwise. Done. https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector_contro... File ash/wm/window_selector_controller.cc (right): https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector_contro... ash/wm/window_selector_controller.cc:27: virtual void OnKeyEvent(ui::KeyEvent* event) OVERRIDE; On 2013/08/09 20:54:35, Daniel Erat wrote: > nit: add a blank line after this one Done. https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector_unitte... File ash/wm/window_selector_unittest.cc (right): https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector_unitte... ash/wm/window_selector_unittest.cc:115: for (size_t i = 0; i < windows.size(); ++i) { On 2013/08/09 20:54:35, Daniel Erat wrote: > nit: don't need curly brackets here Done. https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector_unitte... ash/wm/window_selector_unittest.cc:120: for (size_t i = 0; i < animations.size(); ++i) { On 2013/08/09 20:54:35, Daniel Erat wrote: > nit: or here Done.
lgtm https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.cc File ash/wm/window_selector.cc (right): https://codereview.chromium.org/22715005/diff/1/ash/wm/window_selector.cc#new... ash/wm/window_selector.cc:292: layer_ = views::corewm::RecreateWindowLayers(window_, true); On 2013/08/09 22:15:36, flackr wrote: > On 2013/08/09 20:54:35, Daniel Erat wrote: > > nit: can you DCHECK(!layer_) and DCHECK(!window_copy_) here? > > Added DCHECK(!layer_), I'm not sure I follow the DCHECK(!window_copy_), it may > already exist (and is checked for) if the layout is updating in response to a > deleted window. whoops, sorry -- forget that part
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/flackr@chromium.org/22715005/11001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/flackr@chromium.org/22715005/11001
Message was sent while issue was closed.
Change committed as 216874
Message was sent while issue was closed.
ASAN bots are complaining
[ RUN ] WindowSelectorTest.Basic
Xlib: extension "RANDR" missing on display ":9".
=================================================================
==7171==ERROR: AddressSanitizer: heap-use-after-free on address 0x60e0002743c8
at pc 0x11a3f46 bp 0x7fff03177950 sp 0x7fff03177948
READ of size 8 at 0x60e0002743c8 thread T0
#0 0x11a3f45 in size
/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_vector.h:571:0
#1 0x11a3f45 in size
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../base/memory/scoped_vector.h:48:0
#2 0x11a3f45 in ash::WindowSelector::OnEvent(ui::Event*)
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ash/wm/window_selector.cc:364:0
#3 0x1371837 in DispatchEvent
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/base/events/event_dispatcher.cc:145:0
#4 0x1371837 in
ui::EventDispatcher::DispatchEventToEventHandlers(std::vector<ui::EventHandler*,
std::allocator<ui::EventHandler*> >&, ui::Event*)
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/base/events/event_dispatcher.cc:124:0
#5 0x1370b62 in ui::EventDispatcher::ProcessEvent(ui::EventTarget*,
ui::Event*)
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/base/events/event_dispatcher.cc:82:0
#6 0x13708a9 in ui::EventDispatcherDelegate::DispatchEvent(ui::EventTarget*,
ui::Event*)
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/base/events/event_dispatcher.cc:48:0
#7 0x12eb007 in ProcessEvent
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/aura/root_window.cc:759:0
#8 0x12eb007 in
aura::RootWindow::DispatchMouseEventToTarget(ui::MouseEvent*, aura::Window*)
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/aura/root_window.cc:1079:0
#9 0x12e82c5 in aura::RootWindow::DispatchMouseEventImpl(ui::MouseEvent*)
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/aura/root_window.cc:1018:0
#10 0x12e7fea in aura::RootWindow::OnHostMouseEvent(ui::MouseEvent*)
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/aura/root_window.cc:912:0
#11 0x12e839f in non-virtual thunk to
aura::RootWindow::OnHostMouseEvent(ui::MouseEvent*)
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/aura/root_window.cc:913:0
#12 0xca2c4b in aura::test::EventGenerator::DoDispatchEvent(ui::Event*,
bool)
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/aura/test/event_generator.cc:540:0
#13 0xc9e8a7 in Dispatch
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/aura/test/event_generator.cc:420:0
#14 0xc9e8a7 in aura::test::EventGenerator::ReleaseButton(int)
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/aura/test/event_generator.cc:473:0
#15 0xb184f5 in ClickWindow
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ash/wm/window_selector_unittest.cc:146:0
#16 0xb184f5 in ash::internal::WindowSelectorTest_Basic_Test::TestBody()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ash/wm/window_selector_unittest.cc:175:0
#17 0x1436df7 in testing::Test::Run()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:2067:0
#18 0x1438bfc in testing::TestInfo::Run()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:2244:0
#19 0x1439a42 in testing::TestCase::Run()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:2351:0
#20 0x1446d25 in testing::internal::UnitTestImpl::RunAllTests()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:4177:0
#21 0x144622c in impl
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:2051:0
#22 0x144622c in testing::UnitTest::Run()
/mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:3810:0
Message was sent while issue was closed.
On 2013/08/11 03:51:20, Dan Beam wrote: > ASAN bots are complaining > > [ RUN ] WindowSelectorTest.Basic > Xlib: extension "RANDR" missing on display ":9". > ================================================================= > ==7171==ERROR: AddressSanitizer: heap-use-after-free on address 0x60e0002743c8 > at pc 0x11a3f46 bp 0x7fff03177950 sp 0x7fff03177948 > READ of size 8 at 0x60e0002743c8 thread T0 > #0 0x11a3f45 in size > /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_vector.h:571:0 > #1 0x11a3f45 in size > /mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../base/memory/scoped_vector.h:48:0 > #2 0x11a3f45 in ash::WindowSelector::OnEvent(ui::Event*) > /mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ash/wm/window_selector.cc:364:0 > #3 0x1371837 in DispatchEvent > /mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/base/events/event_dispatcher.cc:145:0 > #4 0x1371837 in > ui::EventDispatcher::DispatchEventToEventHandlers(std::vector<ui::EventHandler*, > std::allocator<ui::EventHandler*> >&, ui::Event*) > /mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/base/events/event_dispatcher.cc:124:0 > #5 0x1370b62 in ui::EventDispatcher::ProcessEvent(ui::EventTarget*, > ui::Event*) > /mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/base/events/event_dispatcher.cc:82:0 > #6 0x13708a9 in ui::EventDispatcherDelegate::DispatchEvent(ui::EventTarget*, > ui::Event*) > /mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/base/events/event_dispatcher.cc:48:0 > #7 0x12eb007 in ProcessEvent > /mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/aura/root_window.cc:759:0 > #8 0x12eb007 in > aura::RootWindow::DispatchMouseEventToTarget(ui::MouseEvent*, aura::Window*) > /mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/aura/root_window.cc:1079:0 > #9 0x12e82c5 in aura::RootWindow::DispatchMouseEventImpl(ui::MouseEvent*) > /mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/aura/root_window.cc:1018:0 > #10 0x12e7fea in aura::RootWindow::OnHostMouseEvent(ui::MouseEvent*) > /mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/aura/root_window.cc:912:0 > #11 0x12e839f in non-virtual thunk to > aura::RootWindow::OnHostMouseEvent(ui::MouseEvent*) > /mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/aura/root_window.cc:913:0 > #12 0xca2c4b in aura::test::EventGenerator::DoDispatchEvent(ui::Event*, > bool) > /mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/aura/test/event_generator.cc:540:0 > #13 0xc9e8a7 in Dispatch > /mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/aura/test/event_generator.cc:420:0 > #14 0xc9e8a7 in aura::test::EventGenerator::ReleaseButton(int) > /mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ui/aura/test/event_generator.cc:473:0 > #15 0xb184f5 in ClickWindow > /mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ash/wm/window_selector_unittest.cc:146:0 > #16 0xb184f5 in ash::internal::WindowSelectorTest_Basic_Test::TestBody() > /mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../ash/wm/window_selector_unittest.cc:175:0 > #17 0x1436df7 in testing::Test::Run() > /mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:2067:0 > #18 0x1438bfc in testing::TestInfo::Run() > /mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:2244:0 > #19 0x1439a42 in testing::TestCase::Run() > /mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:2351:0 > #20 0x1446d25 in testing::internal::UnitTestImpl::RunAllTests() > /mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:4177:0 > #21 0x144622c in impl > /mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:2051:0 > #22 0x144622c in testing::UnitTest::Run() > /mnt/data/b/build/slave/Linux_Chromium_OS_ASAN_Builder/build/src/out/Release/../../testing/gtest/src/gtest.cc:3810:0 Just put up the fix here: https://codereview.chromium.org/22775003/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
