Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2129773002:
[CrOS] Initial rough cut of alt-tab window cycling UI. (Closed)
|
Created:
4 years, 5 months ago by Evan Stade Modified:
4 years, 5 months ago CC:
chromium-reviews, kalyank, sadrul, dcheng Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[CrOS] Initial rough cut of alt-tab window cycling UI.
A bit ugly, but mostly functional. I think most changes going forward
should be aesthetic, so this seems like a decent place for an initial
commit.
Enabled with --ash-enable-window-cycle-ui flag.
BUG=626111
Committed: https://crrev.com/6d7ebab8c2afeca3e3776c2e288fffa485d80902
Cr-Commit-Position: refs/heads/master@{#406120}
Patch Set 1 #
Total comments: 6
Patch Set 2 : self review #
Total comments: 11
Patch Set 3 : derat comments #
Total comments: 28
Patch Set 4 : varkha feedback, handle minimized windows #Patch Set 5 : one more todo #
Total comments: 13
Patch Set 6 : sadrul review #
Total comments: 14
Patch Set 7 : oshima #Patch Set 8 : handle last window death #Patch Set 9 : lint #
Total comments: 3
Patch Set 10 : function rename #
Messages
Total messages: 52 (19 generated)
estade@chromium.org changed reviewers: + tdanderson@chromium.org, varkha@chromium.org
Who should be the primary reviewer for this series of patches?
estade@chromium.org changed reviewers: + derat@chromium.org
+derat is probably also a good reviewer
varkha@chromium.org changed reviewers: - derat@chromium.org
I think sky@ for implementation guidance. Indeed the visuals seem quite early at this stage but we can iterate under a flag. General comments, probably for a future iteration: - Are you handling window deletion while the cycler is active? - Are you handling resizing of windows while the cycler is active? Not sure that we need to, as long as this doesn't cause problems, i.e. the previews in Alt+Tab stay scaled with same height. - Are you handling minimized windows? - Alt+Tab tiles scaling - seems like WIP. https://codereview.chromium.org/2129773002/diff/1/ash/common/ash_switches.cc File ash/common/ash_switches.cc (right): https://codereview.chromium.org/2129773002/diff/1/ash/common/ash_switches.cc#... ash/common/ash_switches.cc:79: const char kAshEnableWindowCycleUi[] = "ash-enable-window-cycle-ui"; I think this will need to be tied into --ash-md=experimental using MDC::IsWindowCycleMaterial similar to https://cs.chromium.org/chromium/src/ash/common/material_design/material_desi...
derat@chromium.org changed reviewers: + derat@chromium.org
https://codereview.chromium.org/2129773002/diff/1/ash/wm/forwarding_layer_del... File ash/wm/forwarding_layer_delegate.cc (right): https://codereview.chromium.org/2129773002/diff/1/ash/wm/forwarding_layer_del... ash/wm/forwarding_layer_delegate.cc:15: : original_window_(original_window), original_delegate_(delegate) {} nit: do we still do one-per-line here or is this what clang-format recommends now? https://codereview.chromium.org/2129773002/diff/1/ash/wm/forwarding_layer_del... File ash/wm/forwarding_layer_delegate.h (right): https://codereview.chromium.org/2129773002/diff/1/ash/wm/forwarding_layer_del... ash/wm/forwarding_layer_delegate.h:17: // the paint request to the original delegate. It checks if the orignal delegate nit: s/orignal/original/ https://codereview.chromium.org/2129773002/diff/20001/ash/common/ash_switches.h File ash/common/ash_switches.h (right): https://codereview.chromium.org/2129773002/diff/20001/ash/common/ash_switches... ash/common/ash_switches.h:41: ASH_EXPORT extern const char kAshEnableWindowCycleUi[]; nit: highly unscientific, but "UI" looks like it's probably much more common than "Ui" in class (and presumably also constant) names: % git grep 'class .*UI.* {' | wc -l 720 % git grep 'class .*Ui.* {' | wc -l 33 https://codereview.chromium.org/2129773002/diff/20001/ash/wm/window_cycle_lis... File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/20001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:145: WindowCycleView(const WindowCycleList::WindowList& windows) add 'explicit' https://codereview.chromium.org/2129773002/diff/20001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:191: views::View* destroying_view = window_view_map_[destroying_window]; maybe DCHECK that it's present in the map? https://codereview.chromium.org/2129773002/diff/20001/ash/wm/window_cycle_list.h File ash/wm/window_cycle_list.h (right): https://codereview.chromium.org/2129773002/diff/20001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.h:53: bool ShouldShowUi(); nit: same comment about "UI" vs. "Ui"
> General comments, probably for a future iteration: > - Are you handling window deletion while the cycler is active? Yes. Well, I wrote code for it, but I didn't spend the time to figure out how to actually test it. > - Are you handling resizing of windows while the cycler is active? Not sure that > we need to, as long as this doesn't cause problems, i.e. the previews in Alt+Tab > stay scaled with same height. Not explicitly, although all we'd have to do is re-layout. > - Are you handling minimized windows? functionally, yes, aesthetically no. That is, you can alt tab to a minimized window, it just won't currently paint anything in the window list. > - Alt+Tab tiles scaling - seems like WIP. Not sure what you mean? I didn't know the exact size Sebastien would want to limit the tiles to, so I hazarded a guess. Aside from that, the scaling algorithm seems fairly straightforward. We only scale down, never up, and always keep the same aspect ratio. https://codereview.chromium.org/2129773002/diff/1/ash/common/ash_switches.cc File ash/common/ash_switches.cc (right): https://codereview.chromium.org/2129773002/diff/1/ash/common/ash_switches.cc#... ash/common/ash_switches.cc:79: const char kAshEnableWindowCycleUi[] = "ash-enable-window-cycle-ui"; On 2016/07/06 22:03:06, varkha wrote: > I think this will need to be tied into --ash-md=experimental using > MDC::IsWindowCycleMaterial similar to > https://cs.chromium.org/chromium/src/ash/common/material_design/material_desi... This feature seems somewhat orthogonal to material design. We could even launch it separately (before or after ash MD) if we needed to. Is there anything especially material about it? https://codereview.chromium.org/2129773002/diff/1/ash/wm/forwarding_layer_del... File ash/wm/forwarding_layer_delegate.cc (right): https://codereview.chromium.org/2129773002/diff/1/ash/wm/forwarding_layer_del... ash/wm/forwarding_layer_delegate.cc:15: : original_window_(original_window), original_delegate_(delegate) {} On 2016/07/06 22:04:47, Daniel Erat wrote: > nit: do we still do one-per-line here or is this what clang-format recommends > now? this patch is git cl formatted, yes https://codereview.chromium.org/2129773002/diff/1/ash/wm/forwarding_layer_del... File ash/wm/forwarding_layer_delegate.h (right): https://codereview.chromium.org/2129773002/diff/1/ash/wm/forwarding_layer_del... ash/wm/forwarding_layer_delegate.h:17: // the paint request to the original delegate. It checks if the orignal delegate On 2016/07/06 22:04:47, Daniel Erat wrote: > nit: s/orignal/original/ Done. https://codereview.chromium.org/2129773002/diff/20001/ash/common/ash_switches.h File ash/common/ash_switches.h (right): https://codereview.chromium.org/2129773002/diff/20001/ash/common/ash_switches... ash/common/ash_switches.h:41: ASH_EXPORT extern const char kAshEnableWindowCycleUi[]; On 2016/07/06 22:04:47, Daniel Erat wrote: > nit: highly unscientific, but "UI" looks like it's probably much more common > than "Ui" in class (and presumably also constant) names: > > % git grep 'class .*UI.* {' | wc -l > 720 > % git grep 'class .*Ui.* {' | wc -l > 33 Yea, Chrome does it wrong in most places. The google style guide specifies this should be Ui so I always use Ui (or Md, or Api, etc.) in new code. https://codereview.chromium.org/2129773002/diff/20001/ash/wm/window_cycle_lis... File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/20001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:145: WindowCycleView(const WindowCycleList::WindowList& windows) On 2016/07/06 22:04:47, Daniel Erat wrote: > add 'explicit' Done. https://codereview.chromium.org/2129773002/diff/20001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:191: views::View* destroying_view = window_view_map_[destroying_window]; On 2016/07/06 22:04:47, Daniel Erat wrote: > maybe DCHECK that it's present in the map? Wouldn't the deref on the next line crash if it weren't present in the map? (Also the code makes it apparent that |destroying_view| is expected to be valid.) https://codereview.chromium.org/2129773002/diff/20001/ash/wm/window_cycle_list.h File ash/wm/window_cycle_list.h (right): https://codereview.chromium.org/2129773002/diff/20001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.h:53: bool ShouldShowUi(); On 2016/07/06 22:04:48, Daniel Erat wrote: > nit: same comment about "UI" vs. "Ui" ditto
https://codereview.chromium.org/2129773002/diff/20001/ash/common/ash_switches.h File ash/common/ash_switches.h (right): https://codereview.chromium.org/2129773002/diff/20001/ash/common/ash_switches... ash/common/ash_switches.h:41: ASH_EXPORT extern const char kAshEnableWindowCycleUi[]; On 2016/07/06 23:02:59, Evan Stade wrote: > On 2016/07/06 22:04:47, Daniel Erat wrote: > > nit: highly unscientific, but "UI" looks like it's probably much more common > > than "Ui" in class (and presumably also constant) names: > > > > % git grep 'class .*UI.* {' | wc -l > > 720 > > % git grep 'class .*Ui.* {' | wc -l > > 33 > > Yea, Chrome does it wrong in most places. The google style guide specifies this > should be Ui so I always use Ui (or Md, or Api, etc.) in new code. fair enough. i actually checked the chromium style guide today but got a cached version (looked like http://web.archive.org/web/20160601170106/https://www.chromium.org/developers...), which has some example functions with names like "OK3ReallyLongFunctionName", which made me think that we don't lowercase the trailing letters in abbreviations. (the google c++ guide still has an "IsOK" example at https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D..., but that one's probably hard to change in the underlying code.) https://codereview.chromium.org/2129773002/diff/20001/ash/wm/window_cycle_lis... File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/20001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:191: views::View* destroying_view = window_view_map_[destroying_window]; On 2016/07/06 23:02:59, Evan Stade wrote: > On 2016/07/06 22:04:47, Daniel Erat wrote: > > maybe DCHECK that it's present in the map? > > Wouldn't the deref on the next line crash if it weren't present in the map? > (Also the code makes it apparent that |destroying_view| is expected to be > valid.) yeah, i just prefer failed checks to segfaults since they're much easier to debug (which i guess is an argument for CHECK instead of DCHECK). the style guide suggests that CHECK is just to prevent vulnerabilities and DCHECK just provides a hint of an assertion, though, so feel free to ignore this suggestion.
On 2016/07/06 23:47:30, Daniel Erat wrote: > https://codereview.chromium.org/2129773002/diff/20001/ash/common/ash_switches.h > File ash/common/ash_switches.h (right): > > https://codereview.chromium.org/2129773002/diff/20001/ash/common/ash_switches... > ash/common/ash_switches.h:41: ASH_EXPORT extern const char > kAshEnableWindowCycleUi[]; > On 2016/07/06 23:02:59, Evan Stade wrote: > > On 2016/07/06 22:04:47, Daniel Erat wrote: > > > nit: highly unscientific, but "UI" looks like it's probably much more common > > > than "Ui" in class (and presumably also constant) names: > > > > > > % git grep 'class .*UI.* {' | wc -l > > > 720 > > > % git grep 'class .*Ui.* {' | wc -l > > > 33 > > > > Yea, Chrome does it wrong in most places. The google style guide specifies > this > > should be Ui so I always use Ui (or Md, or Api, etc.) in new code. > > fair enough. i actually checked the chromium style guide today but got a cached > version (looked like > http://web.archive.org/web/20160601170106/https://www.chromium.org/developers...), > which has some example functions with names like "OK3ReallyLongFunctionName", > which made me think that we don't lowercase the trailing letters in > abbreviations. (the google c++ guide still has an "IsOK" example at > https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D..., > but that one's probably hard to change in the underlying code.) > > https://codereview.chromium.org/2129773002/diff/20001/ash/wm/window_cycle_lis... > File ash/wm/window_cycle_list.cc (right): > > https://codereview.chromium.org/2129773002/diff/20001/ash/wm/window_cycle_lis... > ash/wm/window_cycle_list.cc:191: views::View* destroying_view = > window_view_map_[destroying_window]; > On 2016/07/06 23:02:59, Evan Stade wrote: > > On 2016/07/06 22:04:47, Daniel Erat wrote: > > > maybe DCHECK that it's present in the map? > > > > Wouldn't the deref on the next line crash if it weren't present in the map? > > (Also the code makes it apparent that |destroying_view| is expected to be > > valid.) > > yeah, i just prefer failed checks to segfaults since they're much easier to > debug (which i guess is an argument for CHECK instead of DCHECK). the style > guide suggests that CHECK is just to prevent vulnerabilities and DCHECK just > provides a hint of an assertion, though, so feel free to ignore this suggestion. (i meant s/assertion/invariant/ in that last sentence)
https://codereview.chromium.org/2129773002/diff/20001/ash/common/ash_switches.h File ash/common/ash_switches.h (right): https://codereview.chromium.org/2129773002/diff/20001/ash/common/ash_switches... ash/common/ash_switches.h:41: ASH_EXPORT extern const char kAshEnableWindowCycleUi[]; On 2016/07/06 23:47:30, Daniel Erat wrote: > On 2016/07/06 23:02:59, Evan Stade wrote: > > On 2016/07/06 22:04:47, Daniel Erat wrote: > > > nit: highly unscientific, but "UI" looks like it's probably much more common > > > than "Ui" in class (and presumably also constant) names: > > > > > > % git grep 'class .*UI.* {' | wc -l > > > 720 > > > % git grep 'class .*Ui.* {' | wc -l > > > 33 > > > > Yea, Chrome does it wrong in most places. The google style guide specifies > this > > should be Ui so I always use Ui (or Md, or Api, etc.) in new code. > > fair enough. i actually checked the chromium style guide today but got a cached > version (looked like > http://web.archive.org/web/20160601170106/https://www.chromium.org/developers...), > which has some example functions with names like "OK3ReallyLongFunctionName", > which made me think that we don't lowercase the trailing letters in > abbreviations. (the google c++ guide still has an "IsOK" example at > https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D..., > but that one's probably hard to change in the underlying code.) I guess that might be a bug in the guide? The part I was referring to says "(i.e. StartRpc(), not StartRPC())."
derat@chromium.org changed reviewers: + sky@chromium.org
lgtm but others should still review. also adding sky@ since he was mentioned and is a views wizard
sky@, should WindowCycle* and ForwardingLayerDelegate move to ash/common/wm?
On 2016/07/07 17:04:23, Evan Stade wrote: > sky@, should WindowCycle* and ForwardingLayerDelegate move to ash/common/wm? The layer cloning you have isn't going to work the same for mash, but I think it's better to start with putting the code in ash/common and we can refactor later on when animation/cloning support is added to mus.
Nice discovery! https://codereview.chromium.org/2129773002/diff/30001/ash/common/ash_switches.cc File ash/common/ash_switches.cc (right): https://codereview.chromium.org/2129773002/diff/30001/ash/common/ash_switches... ash/common/ash_switches.cc:79: const char kAshEnableWindowCycleUi[] = "ash-enable-window-cycle-ui"; Not sure if we need this or could just use ash-md=experimental. https://codereview.chromium.org/2129773002/diff/30001/ash/wm/forwarding_layer... File ash/wm/forwarding_layer_delegate.h (right): https://codereview.chromium.org/2129773002/diff/30001/ash/wm/forwarding_layer... ash/wm/forwarding_layer_delegate.h:3: // found in the LICENSE file. Talked offline about moving this to ash/common/wm. https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:75: WindowMirrorView() : target_(nullptr) {} Would it be simpler to initialize |target_| in ctor and leave Init() with no parameters? https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:98: target_size.height() <= kMaxHeight) { I think we may need to scale up in this case to maintain a neat appearance (future). https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:104: kMaxHeight / static_cast<float>(target_size.height())); This should probably favor keeping the target height fixed and be a function of number of windows (future). https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:113: gfx::Size allotted_size = size(); Do you need this temporary? https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:116: float scale = width() / static_cast<float>(target_->GetBounds().width()); nit: const. https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:132: ui::Layer* mirror_layer() { return layer_owner_->root(); } Should this be GetMirrorLayer() because it is not a simple accessor? It was a bit confusing seeing mirror_layer() used in Init() but not seeing |mirror_layer_| being initialized. https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:214: std::map<WmWindow*, WindowMirrorView*> window_view_map_; Do you need include <map>? https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:293: widget, kShellWindowId_OverlayContainer, ¶ms); Does it play well with the screenshot capture using shortcuts? Doesn't need to be functional, just as long as the results are expected. https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:322: delete cycle_ui_widget_; nit: should cycle_ui_widget_ be auto managed? https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:348: cycle_view_->SetTargetWindow(windows_[current_index_]); nit: early return? https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:370: cycle_view_->HandleWindowDestruction(window, windows_[current_index_]); Should something happen when a last window gets destroyed? https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_list.h File ash/wm/window_cycle_list.h (right): https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.h:53: bool ShouldShowUi(); Maybe keep this in ash::MaterialDesignController (or at least the flag part)?
as I was investigating how to handle minimized windows I realized that the forwarding layer delegate wasn't working as expected. That is, it wasn't painting (or forwarding paint calls) because the underlying layer was never invalidated. It appeared to work because the underlying layer already had the right thing painted on it. Also, I don't know how to get a minimized window to paint. I suppose we could unminimize and move offscreen but that seems like a terrible hack. I'm going to have to go back to the drawing board for a bit to figure out what's going on, so please hold off on reviewing for now.
minimized windows now work correctly! https://codereview.chromium.org/2129773002/diff/30001/ash/common/ash_switches.cc File ash/common/ash_switches.cc (right): https://codereview.chromium.org/2129773002/diff/30001/ash/common/ash_switches... ash/common/ash_switches.cc:79: const char kAshEnableWindowCycleUi[] = "ash-enable-window-cycle-ui"; On 2016/07/07 17:40:28, varkha wrote: > Not sure if we need this or could just use ash-md=experimental. I think this is useful for when we want to launch (add to about:flags, roll out via finch, etc.) https://codereview.chromium.org/2129773002/diff/30001/ash/wm/forwarding_layer... File ash/wm/forwarding_layer_delegate.h (right): https://codereview.chromium.org/2129773002/diff/30001/ash/wm/forwarding_layer... ash/wm/forwarding_layer_delegate.h:3: // found in the LICENSE file. On 2016/07/07 17:40:28, varkha wrote: > Talked offline about moving this to ash/common/wm. Done. https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:75: WindowMirrorView() : target_(nullptr) {} On 2016/07/07 17:40:28, varkha wrote: > Would it be simpler to initialize |target_| in ctor and leave Init() with no > parameters? Done. https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:98: target_size.height() <= kMaxHeight) { On 2016/07/07 17:40:28, varkha wrote: > I think we may need to scale up in this case to maintain a neat appearance > (future). perhaps, although I kinda doubt scaling up would ever look good https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:104: kMaxHeight / static_cast<float>(target_size.height())); On 2016/07/07 17:40:28, varkha wrote: > This should probably favor keeping the target height fixed and be a function of > number of windows (future). Acknowledged. https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:113: gfx::Size allotted_size = size(); On 2016/07/07 17:40:28, varkha wrote: > Do you need this temporary? nope, removed https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:116: float scale = width() / static_cast<float>(target_->GetBounds().width()); On 2016/07/07 17:40:28, varkha wrote: > nit: const. Done. https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:132: ui::Layer* mirror_layer() { return layer_owner_->root(); } On 2016/07/07 17:40:28, varkha wrote: > Should this be GetMirrorLayer() because it is not a simple accessor? It was a > bit confusing seeing mirror_layer() used in Init() but not seeing > |mirror_layer_| being initialized. Done. https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:214: std::map<WmWindow*, WindowMirrorView*> window_view_map_; On 2016/07/07 17:40:29, varkha wrote: > Do you need include <map>? need? no. Should? perhaps. Done. https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:293: widget, kShellWindowId_OverlayContainer, ¶ms); On 2016/07/07 17:40:29, varkha wrote: > Does it play well with the screenshot capture using shortcuts? Doesn't need to > be functional, just as long as the results are expected. I tried to test it but pressing ctrl-f5 while holding alt switched me to a different virtual terminal -_- https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:322: delete cycle_ui_widget_; On 2016/07/07 17:40:28, varkha wrote: > nit: should cycle_ui_widget_ be auto managed? Done. https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:348: cycle_view_->SetTargetWindow(windows_[current_index_]); On 2016/07/07 17:40:28, varkha wrote: > nit: early return? Done. https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:370: cycle_view_->HandleWindowDestruction(window, windows_[current_index_]); On 2016/07/07 17:40:28, varkha wrote: > Should something happen when a last window gets destroyed? probably. TODO added.
https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:293: widget, kShellWindowId_OverlayContainer, ¶ms); On 2016/07/09 00:35:06, Evan Stade wrote: > On 2016/07/07 17:40:29, varkha wrote: > > Does it play well with the screenshot capture using shortcuts? Doesn't need to > > be functional, just as long as the results are expected. > > I tried to test it but pressing ctrl-f5 while holding alt switched me to a > different virtual terminal -_- Yeah, may need to try mapping different accelerators instead of Alt+Tab. https://cs.chromium.org/chromium/src/ash/accelerators/accelerator_table.cc?q=... and https://cs.chromium.org/chromium/src/ash/accelerators/accelerator_controller.... and maybe here: https://cs.chromium.org/chromium/src/ash/wm/window_cycle_controller.cc?dr=Ss&... I've tried setting Alt+Tab UI to be tied to Ctrl+F3 VKEY_BROWSER_REFRESH + ui::EF_CONTROL_DOWN and that works with Ctrl+Shift+F5 to take the region screenshot, so maybe this is all there is. I was just worrying what stacking order we will get in kShellWindowId_OverlayContainer.
estade@chromium.org changed reviewers: + sadrul@chromium.org
+sadrul could you ptal since sky is ooo?
I am not really an ash owner, but I do have opinions! :) https://codereview.chromium.org/2129773002/diff/70001/ash/common/wm/forwardin... File ash/common/wm/forwarding_layer_delegate.h (right): https://codereview.chromium.org/2129773002/diff/70001/ash/common/wm/forwardin... ash/common/wm/forwarding_layer_delegate.h:5: #include "base/macros.h" include guards https://codereview.chromium.org/2129773002/diff/70001/ash/common/wm/forwardin... ash/common/wm/forwarding_layer_delegate.h:34: bool IsDelegateValid(ui::Layer* layer); non-overrides before override This could be a const method? Document. (looks like you are moving existing code, but since you are touching, fix at the same time?) https://codereview.chromium.org/2129773002/diff/70001/ash/wm/window_cycle_lis... File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/70001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:88: class WindowMirrorView : public views::View, public ::wm::LayerDelegateFactory { Can you move this into a separate file? We should consolidate this with whatever we use for the overview mode. It doesn't make sense to have two very similar things. (presumably, we will be able to use this for thumbnail-in-tooltip thing too) https://codereview.chromium.org/2129773002/diff/70001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:229: views::View* target_view = window_view_map_[target_window_]; What happens if target_window_ is null? https://codereview.chromium.org/2129773002/diff/70001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:398: cycle_view_->HandleWindowDestruction(window, windows_[current_index_]); windows_.empty() ? nullptr : windows_[current_index_]
estade@chromium.org changed reviewers: + oshima@chromium.org
> I am not really an ash owner +oshima would perhaps be a good reviewer for ash? https://codereview.chromium.org/2129773002/diff/70001/ash/common/wm/forwardin... File ash/common/wm/forwarding_layer_delegate.h (right): https://codereview.chromium.org/2129773002/diff/70001/ash/common/wm/forwardin... ash/common/wm/forwarding_layer_delegate.h:5: #include "base/macros.h" On 2016/07/13 18:38:07, sadrul wrote: > include guards Done. https://codereview.chromium.org/2129773002/diff/70001/ash/common/wm/forwardin... ash/common/wm/forwarding_layer_delegate.h:34: bool IsDelegateValid(ui::Layer* layer); On 2016/07/13 18:38:07, sadrul wrote: > non-overrides before override > > This could be a const method? > > Document. > > (looks like you are moving existing code, but since you are touching, fix at the > same time?) Done. https://codereview.chromium.org/2129773002/diff/70001/ash/wm/window_cycle_lis... File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/70001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:88: class WindowMirrorView : public views::View, public ::wm::LayerDelegateFactory { On 2016/07/13 18:38:07, sadrul wrote: > Can you move this into a separate file? > > We should consolidate this with whatever we use for the overview mode. It > doesn't make sense to have two very similar things. (presumably, we will be able > to use this for thumbnail-in-tooltip thing too) are you suggesting a separate file for every class here? In general I prefer avoiding an explosion of files because imho it's harder to read small classes that are spread between many .h and .cc files. Also it's harder to realize that this class /isn't/ reused anywhere. If and when the new overview mode wants to use this (which I suspect you are right about), it can at that point easily be factored into its own file. (The current overview mode doesn't use this because it moves the actual windows into the overview grid.) https://codereview.chromium.org/2129773002/diff/70001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:229: views::View* target_view = window_view_map_[target_window_]; On 2016/07/13 18:38:07, sadrul wrote: > What happens if target_window_ is null? It should not be possible. Added a dcheck on |windows| in the ctor. https://codereview.chromium.org/2129773002/diff/70001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:398: cycle_view_->HandleWindowDestruction(window, windows_[current_index_]); On 2016/07/13 18:38:07, sadrul wrote: > windows_.empty() ? nullptr : windows_[current_index_] I thought about it but that alone is not a satisfying way to handle this case. I think we want to abort the alt-tab preview altogether, but I'm punting on that for now. Yes, this might crash currently, but it's behind a flag.
I have some comments, but none intended to block the CL. You should land if ash owners are happy. https://codereview.chromium.org/2129773002/diff/70001/ash/wm/window_cycle_lis... File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/70001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:88: class WindowMirrorView : public views::View, public ::wm::LayerDelegateFactory { On 2016/07/13 19:31:56, Evan Stade wrote: > On 2016/07/13 18:38:07, sadrul wrote: > > Can you move this into a separate file? > > > > We should consolidate this with whatever we use for the overview mode. It > > doesn't make sense to have two very similar things. (presumably, we will be > able > > to use this for thumbnail-in-tooltip thing too) > > are you suggesting a separate file for every class here? In general I prefer > avoiding an explosion of files because imho it's harder to read small classes > that are spread between many .h and .cc files. Also it's harder to realize that > this class /isn't/ reused anywhere. If and when the new overview mode wants to > use this (which I suspect you are right about), it can at that point easily be > factored into its own file. (The current overview mode doesn't use this because > it moves the actual windows into the overview grid.) I am suggesting moving this WindowMirrorView class into a separate file. That would make it easier to test too (e.g. a test to make sure minimized windows are still drawn correctly in a WindowMirrorView). https://codereview.chromium.org/2129773002/diff/70001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:398: cycle_view_->HandleWindowDestruction(window, windows_[current_index_]); On 2016/07/13 19:31:56, Evan Stade wrote: > On 2016/07/13 18:38:07, sadrul wrote: > > windows_.empty() ? nullptr : windows_[current_index_] > > I thought about it but that alone is not a satisfying way to handle this case. I > think we want to abort the alt-tab preview altogether, but I'm punting on that > for now. Yes, this might crash currently, but it's behind a flag. Acknowledged. https://codereview.chromium.org/2129773002/diff/90001/ash/wm/window_cycle_lis... File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/90001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:108: EnsureAllChildrenAreVisible(GetMirrorLayer()); Where do we restore the opacity/visibility?
https://codereview.chromium.org/2129773002/diff/70001/ash/wm/window_cycle_lis... File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/70001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:88: class WindowMirrorView : public views::View, public ::wm::LayerDelegateFactory { On 2016/07/15 14:41:53, sadrul wrote: > On 2016/07/13 19:31:56, Evan Stade wrote: > > On 2016/07/13 18:38:07, sadrul wrote: > > > Can you move this into a separate file? > > > > > > We should consolidate this with whatever we use for the overview mode. It > > > doesn't make sense to have two very similar things. (presumably, we will be > > able > > > to use this for thumbnail-in-tooltip thing too) > > > > are you suggesting a separate file for every class here? In general I prefer > > avoiding an explosion of files because imho it's harder to read small classes > > that are spread between many .h and .cc files. Also it's harder to realize > that > > this class /isn't/ reused anywhere. If and when the new overview mode wants to > > use this (which I suspect you are right about), it can at that point easily be > > factored into its own file. (The current overview mode doesn't use this > because > > it moves the actual windows into the overview grid.) > > I am suggesting moving this WindowMirrorView class into a separate file. That > would make it easier to test too (e.g. a test to make sure minimized windows are > still drawn correctly in a WindowMirrorView). OK. If you don't mind, I'll leave this here for now and move it to a new file when the reasons you've outlined necessitate it. https://codereview.chromium.org/2129773002/diff/90001/ash/wm/window_cycle_lis... File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/90001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:108: EnsureAllChildrenAreVisible(GetMirrorLayer()); On 2016/07/15 14:41:53, sadrul wrote: > Where do we restore the opacity/visibility? we don't. The mirror layer is owned by |this| (RecreateLayers gives us ownership of the existing layers, then recreates them for the real window) so once the alt tab preview is closed, these layers will be disposed. I'll admit that it's a little bit sketchy that we set all layers to visible, but it doesn't seem to cause any harm and it's necessary for at least the content area layer in the minimized window case.
can you add a test? Or if you want to add test later for a reason, let me know. https://codereview.chromium.org/2129773002/diff/90001/ash/wm/window_cycle_lis... File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/90001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:115: const int kMaxHeight = 600; This looked pretty big. Is this temporary, or I'm misreading something? https://codereview.chromium.org/2129773002/diff/90001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:219: views::View* destroying_view = window_view_map_[destroying_window]; don't you have to (or want to) erase? https://codereview.chromium.org/2129773002/diff/90001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:394: current_index_--; what if the 1st window is destroyed?
> can you add a test? Or if you want to add test later for a reason, let me know. Is there any specific functionality you would like to see tested? In general I think tests are more useful slightly later in the life cycle of a feature because it's easy to spend a lot of time making tests that verify behavior which you end up wanting to change. This takes more time now (to write the tests) and later (to fix/rewrite tests). Definitely by the time the code is more stabilized and we get close to being ready to launch tests will be very useful for catching regressions. It looks like there are some tests in window_cycle_controller_unittest which could serve as a good starting point when it's time to test this new feature. https://codereview.chromium.org/2129773002/diff/90001/ash/wm/window_cycle_lis... File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/90001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:115: const int kMaxHeight = 600; On 2016/07/15 18:28:04, oshima wrote: > This looked pretty big. Is this temporary, or I'm misreading something? the visual details like this one are temporary. I suspect sizing logic will get a little more complicated (e.g. max size could depend on the number of windows. Too many windows, and the max size gets smaller, eventually we start scrolling instead of squeezing more previews in.) https://codereview.chromium.org/2129773002/diff/90001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:219: views::View* destroying_view = window_view_map_[destroying_window]; On 2016/07/15 18:28:04, oshima wrote: > don't you have to (or want to) erase? technically I don't think it's doing any harm by leaving it there, but sure we can erase. https://codereview.chromium.org/2129773002/diff/90001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:394: current_index_--; On 2016/07/15 18:28:04, oshima wrote: > what if the 1st window is destroyed? I don't think that would be a problem? If the 1st window is the current window, current_index_ will not be greater than removed_index. If the 1st window is not the current window, then we'll correctly decrement current_index to maintain the same current window. It would be a problem if there were only one window (see below).
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I'm ok if you want to add test later since this isn't enabled yet. > Is there any specific functionality you would like to see tested? * Adding/removing windows while UI is shown * display metrics change. * screen lock ,modal case. would be good ones. https://codereview.chromium.org/2129773002/diff/90001/ash/wm/window_cycle_lis... File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/90001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:318: widget, kShellWindowId_OverlayContainer, ¶ms); Overlay container is above lock screen and system modal containers, so you need to make sure hiding them when screen is locked or modal window is shown. You can just add TODO for now. (current approach that relies on containers is a bit problematic, but that's a separate issue) https://codereview.chromium.org/2129773002/diff/90001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:330: widget->SetBounds(widget_rect); you should also handle the case when display metrics changes. Simply canceling the UI is probably good enough. Another question is what to do when you have two or more displays. Are we going to group then per display like over view mode? https://codereview.chromium.org/2129773002/diff/90001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:394: current_index_--; On 2016/07/15 18:44:08, Evan Stade wrote: > On 2016/07/15 18:28:04, oshima wrote: > > what if the 1st window is destroyed? > > I don't think that would be a problem? If the 1st window is the current window, > current_index_ will not be greater than removed_index. If the 1st window is not > the current window, then we'll correctly decrement current_index to maintain the > same current window. > > It would be a problem if there were only one window (see below). Ah yes, that's what I was wondering. If current_index_ == 0, and that's the last one, then windows_ will be empty and windows_[current_index_] will result in out of range. Why not fixing it now?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2129773002/diff/90001/ash/wm/window_cycle_lis... File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/90001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:318: widget, kShellWindowId_OverlayContainer, ¶ms); On 2016/07/15 19:47:52, oshima wrote: > Overlay container is above lock screen and system modal containers, > so you need to make sure hiding them when screen is locked or modal window is > shown. > > You can just add TODO for now. > > (current approach that relies on containers is a bit problematic, but that's a > separate issue) Done. https://codereview.chromium.org/2129773002/diff/90001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:330: widget->SetBounds(widget_rect); On 2016/07/15 19:47:52, oshima wrote: > you should also handle the case when display metrics changes. Simply canceling > the UI is probably good enough. > > Another question is what to do when you have two or more displays. > Are we going to group then per display like over view mode? I don't know what would currently happen with multiple displays and I don't know what the plan is --- it hasn't been mentioned in the design discussions afaik. I probably should use GetTargetRootWindow() above instead of PrimaryRootWindow() (fixed). Do you know how I can manually test this? https://codereview.chromium.org/2129773002/diff/90001/ash/wm/window_cycle_lis... ash/wm/window_cycle_list.cc:394: current_index_--; On 2016/07/15 19:47:52, oshima wrote: > On 2016/07/15 18:44:08, Evan Stade wrote: > > On 2016/07/15 18:28:04, oshima wrote: > > > what if the 1st window is destroyed? > > > > I don't think that would be a problem? If the 1st window is the current > window, > > current_index_ will not be greater than removed_index. If the 1st window is > not > > the current window, then we'll correctly decrement current_index to maintain > the > > same current window. > > > > It would be a problem if there were only one window (see below). > > Ah yes, that's what I was wondering. If current_index_ == 0, and that's the last > one, then windows_ will be empty and windows_[current_index_] will result in out > of range. Why not > fixing it now? I just thought this was a good stopping point because too much more work and the patch becomes a bit unwieldy. I thought it would be harder to cancel the UI, but it seems we can do so pretty easily. Also I wanted to punt on coming up with a test case but I just went ahead and did it in this patch (here is the test case: https://jsfiddle.net/evanstade/q4p5mL08/ )
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
thanks, lgtm https://codereview.chromium.org/2129773002/diff/150001/ash/wm/window_cycle_li... File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/150001/ash/wm/window_cycle_li... ash/wm/window_cycle_list.cc:353: cycle_view_->target_window()->Show(); Did you need this? Activating minimiezd window should show the window. Plese ignore my comment if it's necessary.
thanks all for review https://codereview.chromium.org/2129773002/diff/150001/ash/wm/window_cycle_li... File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/150001/ash/wm/window_cycle_li... ash/wm/window_cycle_list.cc:353: cycle_view_->target_window()->Show(); On 2016/07/15 23:33:00, oshima wrote: > Did you need this? Activating minimiezd window should show the window. > Plese ignore my comment if it's necessary. I think it's necessary (90% sure I tried without using Show()). Note that this matches L367.
https://codereview.chromium.org/2129773002/diff/150001/ash/wm/window_cycle_li... File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/150001/ash/wm/window_cycle_li... ash/wm/window_cycle_list.cc:353: cycle_view_->target_window()->Show(); On 2016/07/15 23:39:29, Evan Stade wrote: > On 2016/07/15 23:33:00, oshima wrote: > > Did you need this? Activating minimiezd window should show the window. > > Plese ignore my comment if it's necessary. > > I think it's necessary (90% sure I tried without using Show()). Note that this > matches L367. Acknowledged.
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org Link to the patchset: https://codereview.chromium.org/2129773002/#ps150001 (title: "lint")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2129773002/#ps170001 (title: "function rename")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #10 (id:170001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [CrOS] Initial rough cut of alt-tab window cycling UI. A bit ugly, but mostly functional. I think most changes going forward should be aesthetic, so this seems like a decent place for an initial commit. Enabled with --ash-enable-window-cycle-ui flag. BUG=626111 ========== to ========== [CrOS] Initial rough cut of alt-tab window cycling UI. A bit ugly, but mostly functional. I think most changes going forward should be aesthetic, so this seems like a decent place for an initial commit. Enabled with --ash-enable-window-cycle-ui flag. BUG=626111 Committed: https://crrev.com/6d7ebab8c2afeca3e3776c2e288fffa485d80902 Cr-Commit-Position: refs/heads/master@{#406120} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/6d7ebab8c2afeca3e3776c2e288fffa485d80902 Cr-Commit-Position: refs/heads/master@{#406120} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
