Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
251103005:
Added arrow key navigation to Overview Mode (Closed)
|
Created:
6 years, 7 months ago by Nina Modified:
6 years, 6 months ago CC:
chromium-reviews, tdanderson+overview_chromium.org, kalyank, sadrul, ben+ash_chromium.org, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdded arrow key navigation to Overview Mode
This change makes possible to use the arrow keys to move in Overview Mode across windows. It also raises an a11y event to notify a disabled user of the selection on the window. However, since currently there's no support for that, we fake the labels to appear as windows for the a11y framework.
Multi monitor is supported. More tests to follow in a future patch.
TEST=WindowSelectorTest.BasicArrowKeyNavigation
BUG=291151
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275489
Patch Set 1 #Patch Set 2 : Rebase with latest changes, test stub (don't look into that yet!) #
Total comments: 34
Patch Set 3 : Fixed issues #Patch Set 4 : Starting again from design doc #
Total comments: 30
Patch Set 5 : Added reposition animation, shortened movement time, fixed what Terry commented. #
Total comments: 54
Patch Set 6 : Addressed Rob's comments #Patch Set 7 : Simplified RemoveWindow() #
Total comments: 46
Patch Set 8 : Checked Rob's comments #Patch Set 9 : Fixed a bug with panels and changing windows bounds #
Total comments: 41
Patch Set 10 : Fixed Rob's comments (except the test). #Patch Set 11 : Replaced unittest by a better one, fixed lints, minor tweaks. #
Total comments: 18
Patch Set 12 : Added tween to the selection widget animation #
Total comments: 23
Patch Set 13 : Fixed rob's comments, clang warnings. #
Total comments: 7
Patch Set 14 : Fixed last nits. #
Messages
Total messages: 21 (0 generated)
Hi Terry, could you please look at this CL? I'm working on the tests BTW.
On 2014/04/28 14:30:32, nsatragno wrote: > Hi Terry, could you please look at this CL? I'm working on the tests BTW. Sorry, ran out of time today, will take a first look at this tomorrow.
Nico, please see my first round of comments below. In the next review I'll take a closer look at the architecture here. I suspect that we'll want to better encapsulate the information about the grid / selection widget so that the WindowOverview class doesn't become too bloated. After that, let's also decide the best way to split this into smaller patches. https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... File ash/wm/overview/window_overview.cc (right): https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.cc:131: main_grid_.root_window = secondary_grid_.root_window = NULL; Initialize on two lines instead of one. https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.cc:202: if (fade_out_direction) { Is this check needed? Under which circumstances would |fade_out_direction| be NULL? https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.cc:406: secondary_grid_.windows = main_grid_.windows = 0; Two lines. https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.cc:427: for (size_t i = 0; i < windows_->size() && selected_window_removed; ++i) { No {} needed since the body of the for loop is a single line. https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.cc:460: WindowGrid grid; Declare this right before initialization (line 479). Also include a comment to explain what you're doing (making a new grid which will be replacing either main_grid_ or secondary_grid_). Perhaps also call it something more descriptive than just |grid|. https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.cc:578: if (arrow_cursor_.x >= current_grid->columns || Don't align your boolean operators in this way. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Boolea... https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.cc:669: if (arrow_cursor_.primary_display) Use {} here since the body of if/else exceeds one line. https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... File ash/wm/overview/window_overview.h (right): https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.h:40: // Represents a grid of windows in OM. Useful for arrow key navigation. "OM" -> "Overview Mode" https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.h:101: }; space https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.h:138: // Returns the bounds for the selection widget for the specified |window| . at end https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.h:155: // Pointer to the currently selected window. This is used for the arrow key Indicate in the comment that this is NULL if no window is currently selected. https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.h:157: ash::WindowSelectorItem* selected_window_; ash:: not needed https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.h:181: ArrowCursor arrow_cursor_; When I hear "arrow cursor" I think of an arrow-shaped mouse cursor, so for that reason I don't like this name. I'd prefer if we used the term "selection widget" consistently. https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... File ash/wm/overview/window_selector_item.cc (left): https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_selector_item.cc:51: params.visible_on_all_workspaces = true; Why are you removing this? https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... File ash/wm/overview/window_selector_item.cc (right): https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_selector_item.cc:53: views::WindowLabel* label = new views::WindowLabel; I think that introducing a new class is overkill here. WindowLabel only has one method, and its implementation is almost identical to the one in the base class. How about this instead: * Add a member to Label of type AXRole and initialize it to AX_ROLE_STATIC_TEXT in the Label constructor. * Then at line 53 here, create a Label as before but then say label->SetAXRole(AX_ROLE_BUTTON) along with the TODO for Dominic. What do you think? https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_selector_item.cc:160: return window_label_.get(); Since this is a simple accessor, you can move the implementation into the .h https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... File ash/wm/overview/window_selector_item.h (right): https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_selector_item.h:75: // Returns the label widget . at end of comment https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_selector_item.h:76: views::Widget* get_window_label(); Call this just window_label() instead of get_window_label() https://chromiumcodereview.appspot.com/251103005/diff/10001/ui/views/controls... File ui/views/controls/window_label.cc (right): https://chromiumcodereview.appspot.com/251103005/diff/10001/ui/views/controls... ui/views/controls/window_label.cc:18: // TODO(dmazzoni) Change role to "window" Add : after (dmazzoni), and add . at the end of the comment. Also make sure Dominic knows this TODO is in the code (and ideally there should be a bug tracking the work, too).
Code updated https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... File ash/wm/overview/window_overview.cc (right): https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.cc:131: main_grid_.root_window = secondary_grid_.root_window = NULL; On 2014/04/29 14:46:27, tdanderson wrote: > Initialize on two lines instead of one. Done. https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.cc:202: if (fade_out_direction) { On 2014/04/29 14:46:27, tdanderson wrote: > Is this check needed? Under which circumstances would |fade_out_direction| be > NULL? Yes, in alt-tabbing mode (through SetSelection) https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.cc:406: secondary_grid_.windows = main_grid_.windows = 0; On 2014/04/29 14:46:27, tdanderson wrote: > Two lines. Done. https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.cc:427: for (size_t i = 0; i < windows_->size() && selected_window_removed; ++i) { On 2014/04/29 14:46:27, tdanderson wrote: > No {} needed since the body of the for loop is a single line. Done. https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.cc:460: WindowGrid grid; On 2014/04/29 14:46:27, tdanderson wrote: > Declare this right before initialization (line 479). Also include a comment to > explain what you're doing (making a new grid which will be replacing either > main_grid_ or secondary_grid_). Perhaps also call it something more descriptive > than just |grid|. Done. https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.cc:578: if (arrow_cursor_.x >= current_grid->columns || On 2014/04/29 14:46:27, tdanderson wrote: > Don't align your boolean operators in this way. See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Boolea... Done. https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.cc:669: if (arrow_cursor_.primary_display) On 2014/04/29 14:46:27, tdanderson wrote: > Use {} here since the body of if/else exceeds one line. Done. https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... File ash/wm/overview/window_overview.h (right): https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.h:40: // Represents a grid of windows in OM. Useful for arrow key navigation. On 2014/04/29 14:46:27, tdanderson wrote: > "OM" -> "Overview Mode" Done. https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.h:101: }; On 2014/04/29 14:46:27, tdanderson wrote: > space Done. https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_overview.h:181: ArrowCursor arrow_cursor_; On 2014/04/29 14:46:27, tdanderson wrote: > When I hear "arrow cursor" I think of an arrow-shaped mouse cursor, so for that > reason I don't like this name. I'd prefer if we used the term "selection widget" > consistently. Done. https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... File ash/wm/overview/window_selector_item.cc (left): https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_selector_item.cc:51: params.visible_on_all_workspaces = true; On 2014/04/29 14:46:27, tdanderson wrote: > Why are you removing this? Actually, because I don't know how it got there. When I was working in the label stuff I tested that option… along with a dozen others. Not sure if it has an effect on the label. https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... File ash/wm/overview/window_selector_item.cc (right): https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_selector_item.cc:160: return window_label_.get(); On 2014/04/29 14:46:27, tdanderson wrote: > Since this is a simple accessor, you can move the implementation into the .h Done. https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... File ash/wm/overview/window_selector_item.h (right): https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_selector_item.h:75: // Returns the label widget On 2014/04/29 14:46:27, tdanderson wrote: > . at end of comment Done. https://chromiumcodereview.appspot.com/251103005/diff/10001/ash/wm/overview/w... ash/wm/overview/window_selector_item.h:76: views::Widget* get_window_label(); On 2014/04/29 14:46:27, tdanderson wrote: > Call this just window_label() instead of get_window_label() Done. https://chromiumcodereview.appspot.com/251103005/diff/10001/ui/views/controls... File ui/views/controls/window_label.cc (right): https://chromiumcodereview.appspot.com/251103005/diff/10001/ui/views/controls... ui/views/controls/window_label.cc:18: // TODO(dmazzoni) Change role to "window" On 2014/04/29 14:46:27, tdanderson wrote: > Add : after (dmazzoni), and add . at the end of the comment. Also make sure > Dominic knows this TODO is in the code (and ideally there should be a bug > tracking the work, too). Done, and he does know the TODO. We should probably file the bug when the patch gets landed.
Hey Terry, mind giving this revamped patch a first look? Please bear in mind that tests are still pending. Cheers!
Hey Nico, please see my comments below. https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_g... File ash/wm/overview/window_grid.h (right): https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_g... ash/wm/overview/window_grid.h:37: // Positions all the windows in the grid. If a window pointer is invalid, the Just reading this header file and not looking into window_grid.cc, I have no idea what your second sentence means. Also, behaviour should always be defined, even if that behaviour is "do nothing." https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_g... ash/wm/overview/window_grid.h:42: // case, false if |window| was not found. This comment needs to be updated since RemoveWindow() is returning a WSI* and not a bool. https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_g... ash/wm/overview/window_grid.h:56: const gfx::Rect GetSelectionBounds() const; Add documentation for this. https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_g... ash/wm/overview/window_grid.h:64: // Widget that indicates the user which is the selected window. to the user https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_g... ash/wm/overview/window_grid.h:71: size_t rows_; I would prefer num_rows_ and num_columns_ to make the identifier names more descriptive for people reading the code. When I see |rows_|, I assume it is some sort of collection (e.g., rows_[0] would give me a pointer to a linked list representing the items in the top row). https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... File ash/wm/overview/window_selector.cc (left): https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:379: PositionWindows(true); Why was this removed? https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:211: // Dummy value so that the compiler does not complain about uninitialized Comment not needed. https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:220: direction = WindowSelector::UP; Consider defining a private helper with a single parameter of type Direction, and moving lines 243-256 out of OnKeyEvent() and into the new helper. This would eliminate the need for the variables |direction| and |moving|, and would also eliminate the need to initialize something to a dummy value (which is nice to avoid whenever possible to reduce the risk of introducing new bugs). https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:243: if (moving) { It seems you can do a bit of simplification here by removing the |moving| flag and using return instead of break for each case that did not set |moving| to true. https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:244: bool change_root = false; |change_root| is not necessary. Just say if (grid_list_...) { ... } https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:247: if (change_root) { Your documentation for WindowGrid::Move() says that it "Moves the selection widget to the specified |direction|. Returns false if, after moving, the widget is out of bounds." Doesn't that imply that you should enter this conditional when !grid_list_[root_index_]->Move(direction) instead? (i.e., the function call to Move() returned false, which means we're switching root windows)? https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:254: grid_list_[root_index_]->Move(direction); I don't like the idea of calling Move() for the purpose of initializing the selection widget. Is it possible for you to take the logic that initializes the selection widget out of Move() and put it into its own helper function? Then you can assume Move() has the precondition that the selection widget has already been initialized (it wouldn't hurt to add a DCHECK() for that too). https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:330: removed_item = (*iter)->RemoveWindow(window); I prefer the previous approach: using find_if to determine whether or not the item is found within the grid_list_, and then only calling RemoveWindow() if it is. Is there a reason why you can't use a similar approach here? https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... File ash/wm/overview/window_selector.h (right): https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... ash/wm/overview/window_selector.h:145: // List of all the window overview grids, one for each window. One for each root window? https://codereview.chromium.org/251103005/diff/50001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/251103005/diff/50001/ui/views/controls/label.... ui/views/controls/label.h:175: // Sets the accessibility the label should report. accessibility role
Terry, please see my updated patch. Thanks! https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_g... File ash/wm/overview/window_grid.h (right): https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_g... ash/wm/overview/window_grid.h:37: // Positions all the windows in the grid. If a window pointer is invalid, the On 2014/05/28 16:01:19, tdanderson wrote: > Just reading this header file and not looking into window_grid.cc, I have no > idea what your second sentence means. Also, behaviour should always be defined, > even if that behaviour is "do nothing." I'll consider that for future changes. I removed the comment because it didn't make sense anymore. https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_g... ash/wm/overview/window_grid.h:42: // case, false if |window| was not found. On 2014/05/28 16:01:19, tdanderson wrote: > This comment needs to be updated since RemoveWindow() is returning a WSI* and > not a bool. Done. https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_g... ash/wm/overview/window_grid.h:56: const gfx::Rect GetSelectionBounds() const; On 2014/05/28 16:01:19, tdanderson wrote: > Add documentation for this. Done. https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_g... ash/wm/overview/window_grid.h:64: // Widget that indicates the user which is the selected window. On 2014/05/28 16:01:19, tdanderson wrote: > to the user Done. https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_g... ash/wm/overview/window_grid.h:71: size_t rows_; On 2014/05/28 16:01:19, tdanderson wrote: > I would prefer num_rows_ and num_columns_ to make the identifier names more > descriptive for people reading the code. When I see |rows_|, I assume it is some > sort of collection (e.g., rows_[0] would give me a pointer to a linked list > representing the items in the top row). Done. https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... File ash/wm/overview/window_selector.cc (left): https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:379: PositionWindows(true); On 2014/05/28 16:01:19, tdanderson wrote: > Why was this removed? Because RemoveWindow rearranges the windows on its grid if an item is removed. The previous approach rearranged on every grid for each window removal. I added a comment on RemoveWindow() to clarify this. https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:211: // Dummy value so that the compiler does not complain about uninitialized On 2014/05/28 16:01:19, tdanderson wrote: > Comment not needed. Removed. https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:220: direction = WindowSelector::UP; On 2014/05/28 16:01:19, tdanderson wrote: > Consider defining a private helper with a single parameter of type Direction, > and moving lines 243-256 out of OnKeyEvent() and into the new helper. This would > eliminate the need for the variables |direction| and |moving|, and would also > eliminate the need to initialize something to a dummy value (which is nice to > avoid whenever possible to reduce the risk of introducing new bugs). Yep, this is much nicer. Done. https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:243: if (moving) { On 2014/05/28 16:01:19, tdanderson wrote: > It seems you can do a bit of simplification here by removing the |moving| flag > and using return instead of break for each case that did not set |moving| to > true. Refactoring made this not necessary anymore. https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:244: bool change_root = false; On 2014/05/28 16:01:19, tdanderson wrote: > |change_root| is not necessary. Just say if (grid_list_...) { ... } I thought it was a bit harder to read this way, but a better placed comment solves this. Done. https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:247: if (change_root) { On 2014/05/28 16:01:19, tdanderson wrote: > Your documentation for WindowGrid::Move() says that it "Moves the selection > widget to the specified |direction|. Returns false if, after moving, the widget > is out of bounds." > > Doesn't that imply that you should enter this conditional when > !grid_list_[root_index_]->Move(direction) instead? (i.e., the function call to > Move() returned false, which means we're switching root windows)? Modified the comment. https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:254: grid_list_[root_index_]->Move(direction); On 2014/05/28 16:01:19, tdanderson wrote: > I don't like the idea of calling Move() for the purpose of initializing the > selection widget. Is it possible for you to take the logic that initializes the > selection widget out of Move() and put it into its own helper function? Then you > can assume Move() has the precondition that the selection widget has already > been initialized (it wouldn't hurt to add a DCHECK() for that too). Done. https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:330: removed_item = (*iter)->RemoveWindow(window); Well, if I do it like that I think I'll have to find the window two times on the grid. I will change it to search for the root_window, though, avoiding confusion and making it a little more efficient. https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... File ash/wm/overview/window_selector.h (right): https://codereview.chromium.org/251103005/diff/50001/ash/wm/overview/window_s... ash/wm/overview/window_selector.h:145: // List of all the window overview grids, one for each window. On 2014/05/28 16:01:19, tdanderson wrote: > One for each root window? Yes. Modified comment. https://codereview.chromium.org/251103005/diff/50001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/251103005/diff/50001/ui/views/controls/label.... ui/views/controls/label.h:175: // Sets the accessibility the label should report. On 2014/05/28 16:01:19, tdanderson wrote: > accessibility role Done.
LGTM, thanks for addressing my comments. Please send to Rob for a closer look when you're ready.
Rob, please see this patch, bearing in mind that it still lacks tests and the refactoring we had discussed to allow some form of widget to handle the keypresses.
https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... File ash/wm/overview/window_grid.cc (right): https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:108: gfx::Vector2d GetVectorFor(WindowSelector::Direction direction, nit: The name could better explain what the method does, perhaps call it GetSlideVectorForFadeIn? https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:132: : root_window_(root_window), nit: I believe the : should be indented 4 from the start of line. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:133: window_list_(window_list) { I believe this will create a copy when called and another copy to assign to local variable. The simplest approach might be to just have it build its windowselectoritem list from the complete list of window pointers. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:153: return NULL; This seems like odd behavior to sometimes not return the item. I'd prefer to see all of the removal of the WindowSelectorItem handled in WindowGrid and WindowSelector can just check if WindowGrid is empty after removing the item. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:165: if (selected_index_ != 0) { don't nest if's for a single condition, i.e. if (selected_index_ > 0 && selected_index >= removed_index) selected_index_ --; https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:276: InitSelectionWidget(direction); It looks like you're always destroying the old selection widget and creating a new one? You shouldn't have to do this if the selection is changing on the same root. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:280: selection_widget_.reset(); You've already passed off the selection_widget_ pointer on line 260, currently it should already be NULL. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:331: NotifyAccessibilityEvent(ui::AX_EVENT_FOCUS, true); Perhaps we could even focus the current window selector item, assuming it extends custombutton, rather than having a custombutton. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:339: out_of_bounds = true; Use early returns. It also feels strange that this leaves selected_index_ in a semi-random state. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:362: selected_index_--; This seems odd, up is the same as left on the top column? I'd expect it to always go up a row even if that meant changing displays. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... File ash/wm/overview/window_grid.h (right): https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.h:22: namespace ash { nit: blank line after non-trivial namespace block opening. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.h:51: // Returns true if the grid no more windows. nit: s/grid no more/grid has no more https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.h:52: bool empty() { return window_list_.empty(); } nit: const. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.h:55: bool is_selecting() { return selection_widget_ != NULL; } nit: const. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.h:58: const aura::Window* root_window() { return root_window_; } nit: const. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.h:84: size_t num_rows_; Remove this from the class, it's only used once outside of PositionWindows and can be easily calculated from num_columns. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:230: Move(WindowSelector::UP); I think any of these handled events should set handled or call StopPropagation on the event. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:336: windows_.erase(std::find(windows_.begin(), windows_.end(), removed_item)); So we have two vectors of WindowSelectorItem pointers which we need to keep in sync? Could we instead have the WindowGrid own the items? https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:338: if (grid_iter != grid_list_.end() && (*grid_iter)->empty()) { if we have a removed item, it must have come from grid_iter, why would it no longer be valid? I think the grid_iter != grid_list_.end() check is unnnecessary at this point. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:393: PositionWindows(true); Why remove the animate comment? https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:418: grid_list_.clear(); Instead of rebuilding grid_list_, can we construct it on creation of WindowSelector and then have each one manage their windows from that point on? https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:510: grid_list_[root_index_]->CreateSelectionWidget(direction); This seems like something calling GridList::Move could do if it's not created yet. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:520: root_index_++; root_index_ = (root_index + 1) % grid_list_.size() ? https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... File ash/wm/overview/window_selector.h (right): https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... ash/wm/overview/window_selector.h:163: size_t root_index_; nit: selected_grid_index_ or something like that. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:60: label->set_accessible_role(ui::AX_ROLE_BUTTON); I think it would be better if you used a LabelButton instead of changing the accessible role of a label.
Rob, please see my comments. Some changes were fairly substantial, but I think I was mostly able to solve the issues gracefully. Cheers! https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... File ash/wm/overview/window_grid.cc (right): https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:108: gfx::Vector2d GetVectorFor(WindowSelector::Direction direction, On 2014/05/29 20:04:25, flackr wrote: > nit: The name could better explain what the method does, perhaps call it > GetSlideVectorForFadeIn? Done. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:132: : root_window_(root_window), On 2014/05/29 20:04:25, flackr wrote: > nit: I believe the : should be indented 4 from the start of line. Done. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:133: window_list_(window_list) { On 2014/05/29 20:04:25, flackr wrote: > I believe this will create a copy when called and another copy to assign to > local variable. The simplest approach might be to just have it build its > windowselectoritem list from the complete list of window pointers. Done. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:153: return NULL; On 2014/05/29 20:04:25, flackr wrote: > This seems like odd behavior to sometimes not return the item. I'd prefer to see > all of the removal of the WindowSelectorItem handled in WindowGrid and > WindowSelector can just check if WindowGrid is empty after removing the item. Done. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:165: if (selected_index_ != 0) { On 2014/05/29 20:04:25, flackr wrote: > don't nest if's for a single condition, i.e. > if (selected_index_ > 0 && selected_index >= removed_index) > selected_index_ --; Done. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:276: InitSelectionWidget(direction); On 2014/05/29 20:04:25, flackr wrote: > It looks like you're always destroying the old selection widget and creating a > new one? You shouldn't have to do this if the selection is changing on the same > root. Well actually when you are leaving the boundaries of the grid but you are still in the same root, we do need to recreate the selection widget (think on the widget fading out in a row and in in another). Adding logic to decide if we recreate the widget or not would involve having an "out" parameter in MoveSelectedIndex(). I think this is a bit of an overkill. WDYT? https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:280: selection_widget_.reset(); On 2014/05/29 20:04:25, flackr wrote: > You've already passed off the selection_widget_ pointer on line 260, currently > it should already be NULL. Yep. Removed. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:331: NotifyAccessibilityEvent(ui::AX_EVENT_FOCUS, true); On 2014/05/29 20:04:25, flackr wrote: > Perhaps we could even focus the current window selector item, assuming it > extends custombutton, rather than having a custombutton. I changed it so that it focuses the transparent button. How do you think it looks? https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:339: out_of_bounds = true; On 2014/05/29 20:04:25, flackr wrote: > Use early returns. Done. > It also feels strange that this leaves selected_index_ in a > semi-random state. Yeah, but on the other hand returning true means that in the next call the index is going to be reset. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:362: selected_index_--; On 2014/05/29 20:04:25, flackr wrote: > This seems odd, up is the same as left on the top column? I'd expect it to > always go up a row even if that meant changing displays. Please see the new docs on the header file that explain how movement works. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... File ash/wm/overview/window_grid.h (right): https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.h:22: namespace ash { On 2014/05/29 20:04:25, flackr wrote: > nit: blank line after non-trivial namespace block opening. Done. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.h:51: // Returns true if the grid no more windows. On 2014/05/29 20:04:25, flackr wrote: > nit: s/grid no more/grid has no more Done. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.h:52: bool empty() { return window_list_.empty(); } On 2014/05/29 20:04:25, flackr wrote: > nit: const. Done. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.h:55: bool is_selecting() { return selection_widget_ != NULL; } On 2014/05/29 20:04:25, flackr wrote: > nit: const. Done. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.h:58: const aura::Window* root_window() { return root_window_; } On 2014/05/29 20:04:25, flackr wrote: > nit: const. Done. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.h:84: size_t num_rows_; On 2014/05/29 20:04:25, flackr wrote: > Remove this from the class, it's only used once outside of PositionWindows and > can be easily calculated from num_columns. Done. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:230: Move(WindowSelector::UP); On 2014/05/29 20:04:25, flackr wrote: > I think any of these handled events should set handled or call StopPropagation > on the event. Done. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:336: windows_.erase(std::find(windows_.begin(), windows_.end(), removed_item)); On 2014/05/29 20:04:25, flackr wrote: > So we have two vectors of WindowSelectorItem pointers which we need to keep in > sync? Could we instead have the WindowGrid own the items? I refactored the code so that the WindowGrid owns the items. WindowSelector is still the observer for changes - I think this is an ideal approach. Do you think the implementation is correct? https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:338: if (grid_iter != grid_list_.end() && (*grid_iter)->empty()) { On 2014/05/29 20:04:25, flackr wrote: > if we have a removed item, it must have come from grid_iter, why would it no > longer be valid? I think the grid_iter != grid_list_.end() check is unnnecessary > at this point. Done. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:393: PositionWindows(true); On 2014/05/29 20:04:25, flackr wrote: > Why remove the animate comment? Hmmm that slipped. Restored. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:418: grid_list_.clear(); On 2014/05/29 20:04:25, flackr wrote: > Instead of rebuilding grid_list_, can we construct it on creation of > WindowSelector and then have each one manage their windows from that point on? I believe it is still better to handle window activation and display metrics changes from window_selector, but we can call PositionWindows on the existing grids. What do you think? https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:510: grid_list_[root_index_]->CreateSelectionWidget(direction); On 2014/05/29 20:04:25, flackr wrote: > This seems like something calling GridList::Move could do if it's not created > yet. Done. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... ash/wm/overview/window_selector.cc:520: root_index_++; On 2014/05/29 20:04:25, flackr wrote: > root_index_ = (root_index + 1) % grid_list_.size() ? Done. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... File ash/wm/overview/window_selector.h (right): https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... ash/wm/overview/window_selector.h:163: size_t root_index_; On 2014/05/29 20:04:25, flackr wrote: > nit: selected_grid_index_ or something like that. Done. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:60: label->set_accessible_role(ui::AX_ROLE_BUTTON); On 2014/05/29 20:04:25, flackr wrote: > I think it would be better if you used a LabelButton instead of changing the > accessible role of a label. Modifying where we send the a11y alert made this code go away.
https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... File ash/wm/overview/window_grid.cc (right): https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:276: InitSelectionWidget(direction); On 2014/06/02 22:04:38, Nicolás wrote: > On 2014/05/29 20:04:25, flackr wrote: > > It looks like you're always destroying the old selection widget and creating a > > new one? You shouldn't have to do this if the selection is changing on the > same > > root. > > Well actually when you are leaving the boundaries of the grid but you are still > in the same root, we do need to recreate the selection widget (think on the > widget fading out in a row and in in another). Adding logic to decide if we > recreate the widget or not would involve having an "out" parameter in > MoveSelectedIndex(). I think this is a bit of an overkill. WDYT? I think this is necessary. The animations of the two windows will not be perfectly in sync and we don't want the double box effect that is currently happening. https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:331: NotifyAccessibilityEvent(ui::AX_EVENT_FOCUS, true); On 2014/06/02 22:04:38, Nicolás wrote: > On 2014/05/29 20:04:25, flackr wrote: > > Perhaps we could even focus the current window selector item, assuming it > > extends custombutton, rather than having a custombutton. > > I changed it so that it focuses the transparent button. How do you think it > looks? Looks better! It's not actually focused but pretending to be as far as I can tell. I think the alert should be delivered from Move though after we've determined the new selected window. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... File ash/wm/overview/window_grid.cc (right): https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:105: const gfx::Rect& bounds) { nit: align. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:128: : root_window_(root_window) { Sorry, I mean this should have only 4 spaces in front of it. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:160: if (!empty()) { How about early return here: if (empty()) return; https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:314: else No need for else after early return, here and below. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:327: selected_index_ += num_columns_; how about just always add num_columns, and if >= window_list_.size() (i.e. overflow) add 1 and % num_columns? https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:328: if (selected_index_ == 0 || selected_index_ >= window_list_.size()) The latter part of this condition isn't possible at this point since it can only overflow from 325 at which point it's already taken the remainder. I'd prefer checking for this condition in the condition on 324 which is the only case where it can happen. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:337: if (!(selected_index_ + num_columns_ >= window_list_.size())) { This looks like it will not work if there's only one row. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:362: } Can you move this to the top of the method and use an early return instead? https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... File ash/wm/overview/window_grid.h (right): https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.h:32: // | | | | | | nit: These boxes probably don't need the extra empty line. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.h:43: // - Going right->left nit: "Going left to right:" and "Going "top" to "bottom"" https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.h:63: // RemoveWindow() repositions the items in the grid if necessary. No need to mention method name since it's the comment for this method. Just "Repositions the items in the grid if necessary." https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:58: // A comparator for locating a WindowSelectorItem that contains a given target. Why would using WindowSelectorItemTargetComparator not work? This seems to be used for the gained active check, I think if any child of a selectable window gains activation we should also exit and select that window. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:114: const std::vector<WindowSelectorItem*>& window_list) { nit: Seems like you can use find_in with the WindowSelectorItemTargetComparator. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:134: if (restore_focus_window_) { nit: no curlies { } for single line block https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:169: } All of this building the list of overview items could be more easily done as part of constructing the windowgrid for a particular root window. Just pass in the const WindowList& into the WindowGrid constructor. Then we don't need to check for existing panel items on the current root since we're building for one root window at a time and you can just remove the grid_list_ if it ends up empty. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:191: Shell* shell = Shell::GetInstance(); Shell::GetInstance() is also used on 175, move before this too. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:192: shell->OnOverviewModeStarting(); OnOverviewModeStarting should be sent before we start initializing overview, i.e. as early as possible. At this point we've already removed focus, and tracked the current state of all of the windows in overview. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:306: CancelSelection(); TODO(nsatragno): Keep window selection active on remaining display(s)? https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:351: selected_grid_index_ = 0; TODO, For more than 2 displays / grids the new selected grid index should be based on the removed index. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:357: void WindowSelector::OnWindowBoundsChanged(aura::Window* window, Bounds changed should really be handled in the WindowSelectorItem since the WindowSelector and WindowGrid don't really need to do anything. This could be a TODO. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:476: // selection widget. TODO: If there are more than 2 monitors, move between grids in the requested direction (i.e. up/left or right/down).
Hi Rob, can you take another look at the patch? https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... File ash/wm/overview/window_grid.cc (right): https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:276: InitSelectionWidget(direction); On 2014/06/03 17:12:07, flackr wrote: > On 2014/06/02 22:04:38, Nicolás wrote: > > On 2014/05/29 20:04:25, flackr wrote: > > > It looks like you're always destroying the old selection widget and creating > a > > > new one? You shouldn't have to do this if the selection is changing on the > > same > > > root. > > > > Well actually when you are leaving the boundaries of the grid but you are > still > > in the same root, we do need to recreate the selection widget (think on the > > widget fading out in a row and in in another). Adding logic to decide if we > > recreate the widget or not would involve having an "out" parameter in > > MoveSelectedIndex(). I think this is a bit of an overkill. WDYT? > > I think this is necessary. The animations of the two windows will not be > perfectly in sync and we don't want the double box effect that is currently > happening. OK, done! https://codereview.chromium.org/251103005/diff/70001/ash/wm/overview/window_g... ash/wm/overview/window_grid.cc:331: NotifyAccessibilityEvent(ui::AX_EVENT_FOCUS, true); On 2014/06/03 17:12:07, flackr wrote: > On 2014/06/02 22:04:38, Nicolás wrote: > > On 2014/05/29 20:04:25, flackr wrote: > > > Perhaps we could even focus the current window selector item, assuming it > > > extends custombutton, rather than having a custombutton. > > > > I changed it so that it focuses the transparent button. How do you think it > > looks? > > Looks better! It's not actually focused but pretending to be as far as I can > tell. I think the alert should be delivered from Move though after we've > determined the new selected window. Yeah, we are artificially sending a "Focus" event to the a11y framework. Moved. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... File ash/wm/overview/window_grid.cc (right): https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:105: const gfx::Rect& bounds) { On 2014/06/03 17:12:07, flackr wrote: > nit: align. Done. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:128: : root_window_(root_window) { On 2014/06/03 17:12:07, flackr wrote: > Sorry, I mean this should have only 4 spaces in front of it. Done. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:160: if (!empty()) { On 2014/06/03 17:12:07, flackr wrote: > How about early return here: > if (empty()) > return; Done. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:314: else On 2014/06/03 17:12:07, flackr wrote: > No need for else after early return, here and below. Done. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:327: selected_index_ += num_columns_; On 2014/06/03 17:12:07, flackr wrote: > how about just always add num_columns, and if >= window_list_.size() (i.e. > overflow) add 1 and % num_columns? Done. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:328: if (selected_index_ == 0 || selected_index_ >= window_list_.size()) On 2014/06/03 17:12:07, flackr wrote: > The latter part of this condition isn't possible at this point since it can only > overflow from 325 at which point it's already taken the remainder. > > I'd prefer checking for this condition in the condition on 324 which is the only > case where it can happen. Done. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:337: if (!(selected_index_ + num_columns_ >= window_list_.size())) { On 2014/06/03 17:12:07, flackr wrote: > This looks like it will not work if there's only one row. But it will. If there is only one row, the only thing that happens is selected_index_-- (supposing selected_index_ != 0) because selected_index_ + num_columns_ >= window_list_.size() is true. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:362: } On 2014/06/03 17:12:07, flackr wrote: > Can you move this to the top of the method and use an early return instead? Done. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... File ash/wm/overview/window_grid.h (right): https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.h:32: // | | | | | | On 2014/06/03 17:12:07, flackr wrote: > nit: These boxes probably don't need the extra empty line. Done. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.h:43: // - Going right->left On 2014/06/03 17:12:07, flackr wrote: > nit: "Going left to right:" and "Going "top" to "bottom"" Done. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.h:63: // RemoveWindow() repositions the items in the grid if necessary. On 2014/06/03 17:12:07, flackr wrote: > No need to mention method name since it's the comment for this method. Just > "Repositions the items in the grid if necessary." Comment noted, this code has been removed. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:58: // A comparator for locating a WindowSelectorItem that contains a given target. On 2014/06/03 17:12:07, flackr wrote: > Why would using WindowSelectorItemTargetComparator not work? This seems to be > used for the gained active check, I think if any child of a selectable window > gains activation we should also exit and select that window. Yes, this is more reasonable. We still use this comparator to determine the WindowSelectorItem we are removing. This is OK, right? It would not make sense to do anything if the child of an overview window is destroyed. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:114: const std::vector<WindowSelectorItem*>& window_list) { On 2014/06/03 17:12:07, flackr wrote: > nit: Seems like you can use find_in with the WindowSelectorItemTargetComparator. Done. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:134: if (restore_focus_window_) { On 2014/06/03 17:12:07, flackr wrote: > nit: no curlies { } for single line block Done. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:169: } On 2014/06/03 17:12:07, flackr wrote: > All of this building the list of overview items could be more easily done as > part of constructing the windowgrid for a particular root window. Just pass in > the const WindowList& into the WindowGrid constructor. > > Then we don't need to check for existing panel items on the current root since > we're building for one root window at a time and you can just remove the > grid_list_ if it ends up empty. Done. I also moved the window removal code to the WindowGrid, now that it is easier to handle like that. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:191: Shell* shell = Shell::GetInstance(); On 2014/06/03 17:12:07, flackr wrote: > Shell::GetInstance() is also used on 175, move before this too. Done. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:192: shell->OnOverviewModeStarting(); On 2014/06/03 17:12:07, flackr wrote: > OnOverviewModeStarting should be sent before we start initializing overview, > i.e. as early as possible. At this point we've already removed focus, and > tracked the current state of all of the windows in overview. Done. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:306: CancelSelection(); On 2014/06/03 17:12:07, flackr wrote: > TODO(nsatragno): Keep window selection active on remaining display(s)? This does sound like a good idea. Adding TODO. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:351: selected_grid_index_ = 0; On 2014/06/03 17:12:07, flackr wrote: > TODO, For more than 2 displays / grids the new selected grid index should be > based on the removed index. Added TODO although this would not be a problem currently. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:357: void WindowSelector::OnWindowBoundsChanged(aura::Window* window, On 2014/06/03 17:12:07, flackr wrote: > Bounds changed should really be handled in the WindowSelectorItem since the > WindowSelector and WindowGrid don't really need to do anything. > > This could be a TODO. This should be relatively easy to implement. Added TODO on header file. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:476: // selection widget. On 2014/06/03 17:12:07, flackr wrote: > TODO: If there are more than 2 monitors, move between grids in the requested > direction (i.e. up/left or right/down). Added TODO. Again, this will not be a problem for now.
https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... File ash/wm/overview/window_grid.cc (right): https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:337: if (!(selected_index_ + num_columns_ >= window_list_.size())) { On 2014/06/04 20:21:13, Nicolás wrote: > On 2014/06/03 17:12:07, flackr wrote: > > This looks like it will not work if there's only one row. > > But it will. If there is only one row, the only thing that happens is > selected_index_-- (supposing selected_index_ != 0) because > selected_index_ + num_columns_ >= window_list_.size() > is true. Ah sorry, I was confused by the decrement being separate. Why is moving left a special case though? Can't we always do selected_index_ += num_columns_ * (window_list_.size() / num_columns_) - 1? https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... File ash/wm/overview/window_grid.h (right): https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.h:32: // | | | | | | On 2014/06/04 20:21:13, Nicolás wrote: > On 2014/06/03 17:12:07, flackr wrote: > > nit: These boxes probably don't need the extra empty line. > > Done. Sorry, to be specific, I meant you could remove lines 32, 36, and 40. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... File ash/wm/overview/window_grid.cc (right): https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:48: ui::LayerAnimationSequence* sequence) OVERRIDE; The ui::LayerAnimationObserver methods should not be overridden on ui::ImplicitAnimationObserver as ImplicitAnimationObserver uses these to determine when the implicit animation has completed. Currently I'd expect it to never be able to detect the completion. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:92: // A comparator for locating a selectable window given a targeted window. nit: For locating a WindowSelectorItem https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:173: CHECK(item->Contains(*iter)); I think this check is no longer necessary. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:180: (*iter)->PrepareForOverview(); I forget, but is there any reason we can't do this while building the list (except for the Panels item which we could call prepare on afterwards). https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:239: bool out_of_bounds = MoveSelectedIndex(direction, &recreate_selection_widget); Rather than passing recreate_selection_widget in to be modified by MoveSelectedIndex it would be cleaner for MoveSelectedIndex to call into a function to move the widget as part of updating the index. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:316: // behavior. I don't think this comment is necessary. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:331: if (selected_index_ >= removed_index && selected_index_ != 0) If the selection changes it should cause another accessibility alert for the new selection. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:340: selection_widget_->SetBounds(SelectedWindow()->target_bounds()); Seems like setting up an animated move to the selected window is done twice (here and in WindowGrid::Move), move to a helper function (MoveSelectionWidgetToTarget()) or something like that? https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:373: ::wm::SetWindowVisibilityAnimationTransition( Why ::wm? https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:383: selection_widget_->Show(); We actually don't allow calling show on a window with 0 TargetOpacity since it's considered to not be technically visible. You can on the other hand call show first and then set up the opacity animation or call show after the opacity animation is set up. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:411: } nit: newline before main switch. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:443: num_columns_ * (window_list_.size() / num_columns_); This should probably be num_columns_ * ((window_list_.size() - 1) / num_columns_). I.e. don't add num_columns_ * rows if there are exactly # rows since that will always overflow. Even better would be to avoid the following if by: selected_index_ += num_columns_ * ((window_list_.size() - selected_index_ - 1) / num_columns_) Then there should be no need to do the overflow check. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... File ash/wm/overview/window_grid.h (right): https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.h:114: std::vector<aura::Window*> observed_windows_; Why vector instead of set? It seems we require quick lookups / removals making it ideal to be a set. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:97: return iter != window_list.end(); No need to create/store iter variable, i.e. just return std::find_if(...) != window_list.end(); https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:123: } else { nit: Instead of else, use continue in first if statement. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:138: // Observe window activations and switchable containers on all root windows Observing switchable containers seems to be lines 127-133, not this line. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:326: root_iter != root_windows.end(); ++root_iter) { May as well change this to loop through the WindowGrids, we don't really need to hide windows on a root window for which we're not showing a windowgrid, or if we do we should probably keep empty windowgrids for displays with no switchable windows. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:333: overview_items = (*grid)->window_list(); Instead, add WindowGrid::Contains(Window*) and use this instead of ContainedIn on line 341. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:788: TEST_F(WindowSelectorTest, BasicArrowKeyNavigation) { Should probably have a test which tries complete cycles of each of the navigation directions in a non-trivial example. Build a vector/string representing the expected order and actual order.
Rob, can you please see my updated patch? Thanks! https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... File ash/wm/overview/window_grid.cc (right): https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:337: if (!(selected_index_ + num_columns_ >= window_list_.size())) { On 2014/06/04 22:25:06, flackr wrote: > On 2014/06/04 20:21:13, Nicolás wrote: > > On 2014/06/03 17:12:07, flackr wrote: > > > This looks like it will not work if there's only one row. > > > > But it will. If there is only one row, the only thing that happens is > > selected_index_-- (supposing selected_index_ != 0) because > > selected_index_ + num_columns_ >= window_list_.size() > > is true. > > Ah sorry, I was confused by the decrement being separate. Why is moving left a > special case though? Can't we always do selected_index_ += num_columns_ * > (window_list_.size() / num_columns_) - 1? Oh I didn't realize it would also work that way for one row. Done. https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... File ash/wm/overview/window_grid.h (right): https://codereview.chromium.org/251103005/diff/110001/ash/wm/overview/window_... ash/wm/overview/window_grid.h:32: // | | | | | | On 2014/06/04 22:25:06, flackr wrote: > On 2014/06/04 20:21:13, Nicolás wrote: > > On 2014/06/03 17:12:07, flackr wrote: > > > nit: These boxes probably don't need the extra empty line. > > > > Done. > > Sorry, to be specific, I meant you could remove lines 32, 36, and 40. Done. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... File ash/wm/overview/window_grid.cc (right): https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:48: ui::LayerAnimationSequence* sequence) OVERRIDE; On 2014/06/04 22:25:06, flackr wrote: > The ui::LayerAnimationObserver methods should not be overridden on > ui::ImplicitAnimationObserver as ImplicitAnimationObserver uses these to > determine when the implicit animation has completed. Currently I'd expect it to > never be able to detect the completion. Done. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:92: // A comparator for locating a selectable window given a targeted window. On 2014/06/04 22:25:06, flackr wrote: > nit: For locating a WindowSelectorItem Done. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:173: CHECK(item->Contains(*iter)); On 2014/06/04 22:25:06, flackr wrote: > I think this check is no longer necessary. Done. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:180: (*iter)->PrepareForOverview(); On 2014/06/04 22:25:06, flackr wrote: > I forget, but is there any reason we can't do this while building the list > (except for the Panels item which we could call prepare on afterwards). Well I did it for the panels, but now that we only have one pointer I can easily call it after checking for NULL. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:239: bool out_of_bounds = MoveSelectedIndex(direction, &recreate_selection_widget); On 2014/06/04 22:25:06, flackr wrote: > Rather than passing recreate_selection_widget in to be modified by > MoveSelectedIndex it would be cleaner for MoveSelectedIndex to call into a > function to move the widget as part of updating the index. While I preferred the other approach, this allows us to get rid of the out-parameter, which is worth it. Done. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:316: // behavior. On 2014/06/04 22:25:06, flackr wrote: > I don't think this comment is necessary. Removed. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:331: if (selected_index_ >= removed_index && selected_index_ != 0) On 2014/06/04 22:25:06, flackr wrote: > If the selection changes it should cause another accessibility alert for the new > selection. Good point. Done. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:340: selection_widget_->SetBounds(SelectedWindow()->target_bounds()); On 2014/06/04 22:25:06, flackr wrote: > Seems like setting up an animated move to the selected window is done twice > (here and in WindowGrid::Move), move to a helper function > (MoveSelectionWidgetToTarget()) or something like that? Done. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:373: ::wm::SetWindowVisibilityAnimationTransition( On 2014/06/04 22:25:06, flackr wrote: > Why ::wm? It's not in namespace ash. I think it wouldn't compile like that if it wasn't required though. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:383: selection_widget_->Show(); On 2014/06/04 22:25:06, flackr wrote: > We actually don't allow calling show on a window with 0 TargetOpacity since it's > considered to not be technically visible. You can on the other hand call show > first and then set up the opacity animation If by this you mean calling Show before setting the opacity to 0, done. > or call show after the opacity > animation is set up. I was not able to do this as show is scheduled after the animation ends, so the widget isn't shown until then. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:411: } On 2014/06/04 22:25:06, flackr wrote: > nit: newline before main switch. Now that we have an else there, it might not be necessary. In the diff it looks weird because the method has been moved "up" in the code and renamed. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:443: num_columns_ * (window_list_.size() / num_columns_); On 2014/06/04 22:25:06, flackr wrote: > This should probably be num_columns_ * ((window_list_.size() - 1) / > num_columns_). I.e. don't add num_columns_ * rows if there are exactly # rows > since that will always overflow. > > Even better would be to avoid the following if by: > selected_index_ += num_columns_ * ((window_list_.size() - selected_index_ - 1) / > num_columns_) > Then there should be no need to do the overflow check. I combined both this comment and the previous one to try to come up with the best solution. What do you think? https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... File ash/wm/overview/window_grid.h (right): https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_grid.h:114: std::vector<aura::Window*> observed_windows_; On 2014/06/04 22:25:06, flackr wrote: > Why vector instead of set? It seems we require quick lookups / removals making > it ideal to be a set. Changed to use a set. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:97: return iter != window_list.end(); On 2014/06/04 22:25:06, flackr wrote: > No need to create/store iter variable, i.e. just return std::find_if(...) != > window_list.end(); Done. Note that the method has been moved to WindowGrid. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:123: } else { On 2014/06/04 22:25:06, flackr wrote: > nit: Instead of else, use continue in first if statement. But then we wouldn't add the containers to the observed windows. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:138: // Observe window activations and switchable containers on all root windows On 2014/06/04 22:25:06, flackr wrote: > Observing switchable containers seems to be lines 127-133, not this line. Moved and modified comment. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:326: root_iter != root_windows.end(); ++root_iter) { On 2014/06/04 22:25:06, flackr wrote: > May as well change this to loop through the WindowGrids, we don't really need to > hide windows on a root window for which we're not showing a windowgrid, or if we > do we should probably keep empty windowgrids for displays with no switchable > windows. Sounds reasonable, done. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:333: overview_items = (*grid)->window_list(); On 2014/06/04 22:25:06, flackr wrote: > Instead, add WindowGrid::Contains(Window*) and use this instead of ContainedIn > on line 341. Done. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:788: TEST_F(WindowSelectorTest, BasicArrowKeyNavigation) { On 2014/06/04 22:25:06, flackr wrote: > Should probably have a test which tries complete cycles of each of the > navigation directions in a non-trivial example. Build a vector/string > representing the expected order and actual order. Check the new test out, I'm sure you're gonna like it.
Sorry for the comments across 3 patchsets. It looks great, just a few nit like changes. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:123: } else { On 2014/06/05 18:04:35, Nicolás wrote: > On 2014/06/04 22:25:06, flackr wrote: > > nit: Instead of else, use continue in first if statement. > > But then we wouldn't add the containers to the observed windows. You can move observing the containers before creating the window grid. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:788: TEST_F(WindowSelectorTest, BasicArrowKeyNavigation) { On 2014/06/05 18:04:36, Nicolás wrote: > On 2014/06/04 22:25:06, flackr wrote: > > Should probably have a test which tries complete cycles of each of the > > navigation directions in a non-trivial example. Build a vector/string > > representing the expected order and actual order. > > Check the new test out, I'm sure you're gonna like it. I do! https://codereview.chromium.org/251103005/diff/190001/ash/wm/overview/window_... File ash/wm/overview/window_grid.cc (right): https://codereview.chromium.org/251103005/diff/190001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:267: ((window_list_.size() - selected_index_) / num_columns_) - 1; Looks much cleaner now, thanks! https://codereview.chromium.org/251103005/diff/190001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:323: SelectedWindow()->SendFocusAlert(); Unless selected_index_ == removed_index, I don't think the selection actually changes in this case. This notification should occur when selected_index_ == removed_index even if selected_index_ == 0. https://codereview.chromium.org/251103005/diff/190001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:385: if (recreate_selection_widget || out_of_bounds) { Merge with if condition above. https://codereview.chromium.org/251103005/diff/190001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:408: InitSelectionWidget(direction); Remove this, instead rely on it being recreated later in this function if we need a new one. https://codereview.chromium.org/251103005/diff/190001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:411: InitSelectionWidget(direction); Move this to inside the if (!out_of_bounds) and do this if (!selection_widget_). https://codereview.chromium.org/251103005/diff/190001/ash/wm/overview/window_... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/251103005/diff/190001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:794: UpdateDisplay("400x300"); I think you need to add if (!SupportsHostWindowResize()) return; To not run this test on platforms which can't set the host window size. https://codereview.chromium.org/251103005/diff/190001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:805: // Expected index for each window during a full loop plus wrapping around. Can you add a comment with a table showing how the windows "should" be layed out. https://codereview.chromium.org/251103005/diff/190001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:818: index_path_for_direction[key_index][i]); nit: The error message on failure would be more readable if this constructed a string such as "Right navigation: 1 2 3 4 5 6 7 1" and compared the string with the expected string. That being said this can be done later, add a TODO maybe? https://codereview.chromium.org/251103005/diff/190001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:823: Can you add a simple multi monitor test? Even just one window on each root and navigate from one to the next? https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/transpa... File ash/wm/overview/transparent_activate_window_button.cc (right): https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/transpa... ash/wm/overview/transparent_activate_window_button.cc:66: event_handler_widget_->GetContentsView()-> // TODO(nsatragno): Actually focus the button. :-) https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... File ash/wm/overview/window_grid.cc (right): https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:182: // Find the minimum number of window_list per row that will fit all of the window_list per row? fit all of the window_list on screen? Should this be WindowSelectorItems? https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:216: // If the selection widget is active, reposition it without any animation. Maybe it should animate if animate is true? This would make it slide to the new selection on deleting windows right? https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:296: std::find_if(window_list_.begin(), window_list_.end(), nit: Wrapped line should be indented 4 from above line. https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:359: ::wm::SetWindowVisibilityAnimationTransition( Why the :: before wm namespace? https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... File ash/wm/overview/window_grid.h (right): https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... ash/wm/overview/window_grid.h:81: const std::vector<WindowSelectorItem*>& window_list() const { Is this still used? If not, remove. https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:136: DCHECK(!grid_list_.empty()); Redundant to line 127? https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:367: if (grid_list_[selected_grid_index_]->Move(direction)) { nit: I think this would be more readable as bool overflowed = grid_list_[selected_grid_index_]->Move(direction); if (overflowed) { ... } It makes it a little more obvious what it means. I know normally I would be against extra variables for a single use but in this case it's much more readable :-). https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... File ash/wm/overview/window_selector.h (right): https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... ash/wm/overview/window_selector.h:50: RIGHT, Can you change this order to match one of the more common direction orderings, perhaps matching the VKEY order, LEFT, UP, RIGHT, DOWN? https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/keycodes... https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... ash/wm/overview/window_selector.h:99: void CreateWindowGrids(); Function doesn't actually exist, remove from header. https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... ash/wm/overview/window_selector.h:111: // Returns the WindowSelectorItem Orphaned comment? https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:179: window_selector_->grid_list_[index]->window_list(); You can have the test friend class WindowSelectorTest to avoid the need to use window_list() here.
I think I flash-fixed everything. Please give it (another) look. https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/251103005/diff/150001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:123: } else { On 2014/06/05 20:16:32, flackr wrote: > On 2014/06/05 18:04:35, Nicolás wrote: > > On 2014/06/04 22:25:06, flackr wrote: > > > nit: Instead of else, use continue in first if statement. > > > > But then we wouldn't add the containers to the observed windows. > > You can move observing the containers before creating the window grid. Done. https://codereview.chromium.org/251103005/diff/190001/ash/wm/overview/window_... File ash/wm/overview/window_grid.cc (right): https://codereview.chromium.org/251103005/diff/190001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:323: SelectedWindow()->SendFocusAlert(); On 2014/06/05 20:16:32, flackr wrote: > Unless selected_index_ == removed_index, I don't think the selection actually > changes in this case. This notification should occur when selected_index_ == > removed_index even if selected_index_ == 0. Done. https://codereview.chromium.org/251103005/diff/190001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:385: if (recreate_selection_widget || out_of_bounds) { On 2014/06/05 20:16:32, flackr wrote: > Merge with if condition above. Done. https://codereview.chromium.org/251103005/diff/190001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:385: if (recreate_selection_widget || out_of_bounds) { On 2014/06/05 20:16:32, flackr wrote: > Merge with if condition above. Done. https://codereview.chromium.org/251103005/diff/190001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:408: InitSelectionWidget(direction); On 2014/06/05 20:16:32, flackr wrote: > Remove this, instead rely on it being recreated later in this function if we > need a new one. Yeah, this is some leftover from the way it worked a few patchsets ago. https://codereview.chromium.org/251103005/diff/190001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:411: InitSelectionWidget(direction); On 2014/06/05 20:16:32, flackr wrote: > Move this to inside the if (!out_of_bounds) and do this if (!selection_widget_). Done. https://codereview.chromium.org/251103005/diff/190001/ash/wm/overview/window_... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/251103005/diff/190001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:794: UpdateDisplay("400x300"); On 2014/06/05 20:16:32, flackr wrote: > I think you need to add > if (!SupportsHostWindowResize()) > return; > > To not run this test on platforms which can't set the host window size. Done. https://codereview.chromium.org/251103005/diff/190001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:805: // Expected index for each window during a full loop plus wrapping around. On 2014/06/05 20:16:32, flackr wrote: > Can you add a comment with a table showing how the windows "should" be layed > out. Done. https://codereview.chromium.org/251103005/diff/190001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:818: index_path_for_direction[key_index][i]); On 2014/06/05 20:16:32, flackr wrote: > nit: The error message on failure would be more readable if this constructed a > string such as "Right navigation: 1 2 3 4 5 6 7 1" and compared the string with > the expected string. That being said this can be done later, add a TODO maybe? Added TODO https://codereview.chromium.org/251103005/diff/190001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:823: On 2014/06/05 20:16:32, flackr wrote: > Can you add a simple multi monitor test? Even just one window on each root and > navigate from one to the next? Done. https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/transpa... File ash/wm/overview/transparent_activate_window_button.cc (right): https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/transpa... ash/wm/overview/transparent_activate_window_button.cc:66: event_handler_widget_->GetContentsView()-> On 2014/06/05 20:16:33, flackr wrote: > // TODO(nsatragno): Actually focus the button. > :-) According to what we discussed off-patch, this does not seem like a good idea for a TODO. https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... File ash/wm/overview/window_grid.cc (right): https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:182: // Find the minimum number of window_list per row that will fit all of the On 2014/06/05 20:16:33, flackr wrote: > window_list per row? fit all of the window_list on screen? Should this be > WindowSelectorItems? I think that was a search-and-replace gone bad. The original word was windows. https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:216: // If the selection widget is active, reposition it without any animation. On 2014/06/05 20:16:33, flackr wrote: > Maybe it should animate if animate is true? This would make it slide to the new > selection on deleting windows right? Actually right before reading this comment I found a bug related to this, I was moving the widget /twice/ when erasing windows. Updated. https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:296: std::find_if(window_list_.begin(), window_list_.end(), On 2014/06/05 20:16:33, flackr wrote: > nit: Wrapped line should be indented 4 from above line. Done. https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... File ash/wm/overview/window_grid.h (right): https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... ash/wm/overview/window_grid.h:81: const std::vector<WindowSelectorItem*>& window_list() const { On 2014/06/05 20:16:33, flackr wrote: > Is this still used? If not, remove. Yes, it is still being used by WindowSelector to handle window activations. https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:136: DCHECK(!grid_list_.empty()); On 2014/06/05 20:16:33, flackr wrote: > Redundant to line 127? Removed. https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:367: if (grid_list_[selected_grid_index_]->Move(direction)) { On 2014/06/05 20:16:33, flackr wrote: > nit: I think this would be more readable as > bool overflowed = grid_list_[selected_grid_index_]->Move(direction); > if (overflowed) { > ... > } > > It makes it a little more obvious what it means. I know normally I would be > against extra variables for a single use but in this case it's much more > readable :-). LOL my patch originally was like that (before uploading the CL) but I got exactly the opposite comment from Terry for another function so I decided to modify this to be consistent. Yes, it is more readable this way. Done. https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... File ash/wm/overview/window_selector.h (right): https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... ash/wm/overview/window_selector.h:50: RIGHT, On 2014/06/05 20:16:33, flackr wrote: > Can you change this order to match one of the more common direction orderings, > perhaps matching the VKEY order, LEFT, UP, RIGHT, DOWN? > https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/keycodes... Done. https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... ash/wm/overview/window_selector.h:99: void CreateWindowGrids(); On 2014/06/05 20:16:33, flackr wrote: > Function doesn't actually exist, remove from header. Done. https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... ash/wm/overview/window_selector.h:111: // Returns the WindowSelectorItem On 2014/06/05 20:16:33, flackr wrote: > Orphaned comment? Removed. https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/251103005/diff/210001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:179: window_selector_->grid_list_[index]->window_list(); On 2014/06/05 20:16:33, flackr wrote: > You can have the test friend class WindowSelectorTest to avoid the need to use > window_list() here. Done.
LGTM with nits. https://codereview.chromium.org/251103005/diff/230001/ash/wm/overview/window_... File ash/wm/overview/window_grid.cc (right): https://codereview.chromium.org/251103005/diff/230001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:319: if (selection_widget_) { nit: match indenting. https://codereview.chromium.org/251103005/diff/230001/ash/wm/overview/window_... ash/wm/overview/window_grid.cc:409: if (!out_of_bounds) { nit: Actually this would be better as an early return at this point. https://codereview.chromium.org/251103005/diff/230001/ash/wm/overview/window_... File ash/wm/overview/window_grid.h (right): https://codereview.chromium.org/251103005/diff/230001/ash/wm/overview/window_... ash/wm/overview/window_grid.h:47: class WindowGrid : public aura::WindowObserver { needs ASH_EXPORT to be used from ash_unittests I think. https://codereview.chromium.org/251103005/diff/230001/ash/wm/overview/window_... ash/wm/overview/window_grid.h:73: const size_t size() const { return window_list_.size(); } I don't think the returned value needs to be a const, just the method. https://codereview.chromium.org/251103005/diff/230001/ash/wm/overview/window_... File ash/wm/overview/window_selector.cc (right): https://codereview.chromium.org/251103005/diff/230001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:109: iter != root_windows.end(); iter++) { nit: Align with aura::Window. https://codereview.chromium.org/251103005/diff/230001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:118: WindowGrid* grid(new WindowGrid(*iter, windows, this)); nit: Because they're easy and safe, use a scoped_ptr<WindowGrid>, then you don't need to delete before continuing. use scoped_ptr<>::Pass to insert into grid_list_. https://codereview.chromium.org/251103005/diff/230001/ash/wm/overview/window_... ash/wm/overview/window_selector.cc:206: nit: Remove newline.
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/251103005/250001
Message was sent while issue was closed.
Change committed as 275489 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
