Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
280423008:
Merge WindowOverview into WindowSelector (Closed)
|
Created:
6 years, 7 months ago by Nina Modified:
6 years, 7 months ago CC:
chromium-reviews, tdanderson+overview_chromium.org, kalyank, sadrul, ben+ash_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionMerge WindowOverview into WindowSelector
This change reduces the amount of code its complexity, and is the natural follow up from having alt tab separated into a different class.
Please note that no functionality has been changed, and that the change is pretty much only moving code around.
BUG=371884
TEST=WindowSelectorTest.*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271142
Patch Set 1 #
Total comments: 6
Patch Set 2 : Restored comment #Patch Set 3 : Reworded comment #
Total comments: 13
Patch Set 4 : Fixed nits, comments, namespace #
Total comments: 2
Patch Set 5 : Fixed nits #Patch Set 6 : Removed unused constants #Patch Set 7 : Fixed bug that made internal window deletions trigger windowselectoritem cleanups #
Total comments: 3
Patch Set 8 : Fixed nits #
Messages
Total messages: 30 (0 generated)
Terry, can you give this patch a first look? Thanks!
A couple of nits and one question below, but otherwise LGTM! https://codereview.chromium.org/280423008/diff/1/ash/wm/overview/window_selec... File ash/wm/overview/window_selector.cc (left): https://codereview.chromium.org/280423008/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector.cc:262: // Remove focus from active window before entering overview. Keep this comment in. https://codereview.chromium.org/280423008/diff/1/ash/wm/overview/window_selec... File ash/wm/overview/window_selector.h (right): https://codereview.chromium.org/280423008/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector.h:45: // (entering Overview Mode). Update this class-level documentation. https://codereview.chromium.org/280423008/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector.h:46: class ASH_EXPORT WindowSelector Just to make sure I didn't miss anything: you're not adding any new methods to this class, right? Just moving stuff over from WindowOverview?
Also, please provide some more details in the issue description.
Terry, please look at my responses. If we're fine, I'll add Rob as reviewer. https://codereview.chromium.org/280423008/diff/1/ash/wm/overview/window_selec... File ash/wm/overview/window_selector.cc (left): https://codereview.chromium.org/280423008/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector.cc:262: // Remove focus from active window before entering overview. On 2014/05/13 23:25:32, tdanderson wrote: > Keep this comment in. Done. https://codereview.chromium.org/280423008/diff/1/ash/wm/overview/window_selec... File ash/wm/overview/window_selector.h (right): https://codereview.chromium.org/280423008/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector.h:45: // (entering Overview Mode). On 2014/05/13 23:25:32, tdanderson wrote: > Update this class-level documentation. I'm not sure what you mean… what should I update? AFAIK we'll still call it "Overview Mode". https://codereview.chromium.org/280423008/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector.h:46: class ASH_EXPORT WindowSelector On 2014/05/13 23:25:32, tdanderson wrote: > Just to make sure I didn't miss anything: you're not adding any new methods to > this class, right? Just moving stuff over from WindowOverview? Exactly
Rob, could you please give this patch a look? Thanks!
Awesome! It's great to see these classes get merged. https://codereview.chromium.org/280423008/diff/30001/ash/wm/overview/window_s... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/280423008/diff/30001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:132: } // namespace Why were all of the following things moved out of the anonymous namespace? https://codereview.chromium.org/280423008/diff/30001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:244: for (aura::WindowTracker::Windows::const_iterator iter = Unindent https://codereview.chromium.org/280423008/diff/30001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:258: ash::Shell* shell = ash::Shell::GetInstance(); Move this up and use for call to Shell::GetInstance() on line 240 as well. https://codereview.chromium.org/280423008/diff/30001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:264: shell->OnOverviewModeEnding(); Can you add a TODO to change this to OnOverviewModeEnded and move it to when everything is done? Allowing other classes to take action before we exit overview increases the risk of strange bugs and I don't think MaximizeModeWindowManager needs to know before we've fully "exited" overview. https://codereview.chromium.org/280423008/diff/30001/ash/wm/overview/window_s... File ash/wm/overview/window_selector.h (right): https://codereview.chromium.org/280423008/diff/30001/ash/wm/overview/window_s... ash/wm/overview/window_selector.h:29: } nit: For nested namespaces it might be easier to read if you comment which one is being closed. https://codereview.chromium.org/280423008/diff/30001/ash/wm/overview/window_s... ash/wm/overview/window_selector.h:74: virtual void OnDisplayRemoved(const gfx::Display& display) OVERRIDE; nit: blank line before // aura::WindowObserver:
Got everything covered. Could you give in another look? https://codereview.chromium.org/280423008/diff/30001/ash/wm/overview/window_s... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/280423008/diff/30001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:132: } // namespace On 2014/05/14 20:59:32, flackr wrote: > Why were all of the following things moved out of the anonymous namespace? Sloppy coding. Corrected. https://codereview.chromium.org/280423008/diff/30001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:244: for (aura::WindowTracker::Windows::const_iterator iter = On 2014/05/14 20:59:32, flackr wrote: > Unindent Done. https://codereview.chromium.org/280423008/diff/30001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:258: ash::Shell* shell = ash::Shell::GetInstance(); On 2014/05/14 20:59:32, flackr wrote: > Move this up and use for call to Shell::GetInstance() on line 240 as well. Done. https://codereview.chromium.org/280423008/diff/30001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:264: shell->OnOverviewModeEnding(); On 2014/05/14 20:59:32, flackr wrote: > Can you add a TODO to change this to OnOverviewModeEnded and move it to when > everything is done? Allowing other classes to take action before we exit > overview increases the risk of strange bugs and I don't think > MaximizeModeWindowManager needs to know before we've fully "exited" overview. Done. For the record, you mean like after the windows have returned to their original locations? https://codereview.chromium.org/280423008/diff/30001/ash/wm/overview/window_s... File ash/wm/overview/window_selector.h (right): https://codereview.chromium.org/280423008/diff/30001/ash/wm/overview/window_s... ash/wm/overview/window_selector.h:29: } On 2014/05/14 20:59:32, flackr wrote: > nit: For nested namespaces it might be easier to read if you comment which one > is being closed. Done. https://codereview.chromium.org/280423008/diff/30001/ash/wm/overview/window_s... ash/wm/overview/window_selector.h:74: virtual void OnDisplayRemoved(const gfx::Display& display) OVERRIDE; On 2014/05/14 20:59:32, flackr wrote: > nit: blank line before // aura::WindowObserver: Done.
LGTM with nits. You can probably say TEST=WindowSelectorTest.* unit tests still pass https://codereview.chromium.org/280423008/diff/30001/ash/wm/overview/window_s... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/280423008/diff/30001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:264: shell->OnOverviewModeEnding(); On 2014/05/15 12:32:04, Nicolás wrote: > On 2014/05/14 20:59:32, flackr wrote: > > Can you add a TODO to change this to OnOverviewModeEnded and move it to when > > everything is done? Allowing other classes to take action before we exit > > overview increases the risk of strange bugs and I don't think > > MaximizeModeWindowManager needs to know before we've fully "exited" overview. > > Done. For the record, you mean like after the windows have returned to their > original locations? Yes. https://codereview.chromium.org/280423008/diff/50001/ash/wm/overview/window_s... File ash/wm/overview/window_selector.h (right): https://codereview.chromium.org/280423008/diff/50001/ash/wm/overview/window_s... ash/wm/overview/window_selector.h:20: #include "ui/gfx/rect.h" This doesn't seem to be used directly. https://codereview.chromium.org/280423008/diff/50001/ash/wm/overview/window_s... ash/wm/overview/window_selector.h:83: // Overridden from aura::client::ActivationChangeObserver: nit: Can you change this to match the override comments above?
The CQ bit was checked by tdanderson@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nsatragno@chromium.org/280423008/70001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was checked by tdanderson@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nsatragno@chromium.org/280423008/90001
The CQ bit was unchecked by tdanderson@chromium.org
SLGTM with a few extra nits i just noticed, speaking of not used stuff. https://codereview.chromium.org/280423008/diff/110001/ash/wm/overview/window_... File ash/wm/overview/window_selector.h (right): https://codereview.chromium.org/280423008/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.h:39: class Widget; This will be unnecessary when selection_widget is gone. https://codereview.chromium.org/280423008/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.h:143: scoped_ptr<views::Widget> selection_widget_; Just noticed this is completely unused. https://codereview.chromium.org/280423008/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.h:147: size_t selection_index_; This too.
The CQ bit was checked by tdanderson@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nsatragno@chromium.org/280423008/130001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by tdanderson@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nsatragno@chromium.org/280423008/130001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by tdanderson@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nsatragno@chromium.org/280423008/130001
Message was sent while issue was closed.
Change committed as 271142 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
