Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2129773002: [CrOS] Initial rough cut of alt-tab window cycling UI. (Closed)

Created:
3 years ago by Evan Stade
Modified:
2 years, 12 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+391 lines, -63 lines) Patch
M ash/ash.gyp View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M ash/common/ash_switches.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M ash/common/ash_switches.cc View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments Download
A ash/common/wm/forwarding_layer_delegate.h View 1 2 3 4 5 1 chunk +48 lines, -0 lines 0 comments Download
A ash/common/wm/forwarding_layer_delegate.cc View 1 2 3 4 5 1 chunk +56 lines, -0 lines 0 comments Download
M ash/wm/drag_window_controller.cc View 1 2 3 4 chunks +7 lines, -57 lines 0 comments Download
M ash/wm/window_cycle_list.h View 1 2 3 3 chunks +17 lines, -1 line 0 comments Download
M ash/wm/window_cycle_list.cc View 1 2 3 4 5 6 7 8 9 5 chunks +257 lines, -5 lines 0 comments Download

Messages

Total messages: 52 (19 generated)
Evan Stade
Who should be the primary reviewer for this series of patches?
3 years ago (2016-07-06 20:49:19 UTC) #2
Evan Stade
+derat is probably also a good reviewer
3 years ago (2016-07-06 21:50:48 UTC) #4
varkha
I think sky@ for implementation guidance. Indeed the visuals seem quite early at this stage ...
3 years ago (2016-07-06 22:03:06 UTC) #6
Daniel Erat
https://codereview.chromium.org/2129773002/diff/1/ash/wm/forwarding_layer_delegate.cc File ash/wm/forwarding_layer_delegate.cc (right): https://codereview.chromium.org/2129773002/diff/1/ash/wm/forwarding_layer_delegate.cc#newcode15 ash/wm/forwarding_layer_delegate.cc:15: : original_window_(original_window), original_delegate_(delegate) {} nit: do we still do ...
3 years ago (2016-07-06 22:04:48 UTC) #8
Evan Stade
> General comments, probably for a future iteration: > - Are you handling window deletion ...
3 years ago (2016-07-06 23:02:59 UTC) #9
Daniel Erat
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.h#newcode41 ash/common/ash_switches.h:41: ASH_EXPORT extern const char kAshEnableWindowCycleUi[]; On 2016/07/06 23:02:59, Evan ...
3 years ago (2016-07-06 23:47:30 UTC) #10
Daniel Erat
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.h#newcode41 ...
3 years ago (2016-07-07 00:01:20 UTC) #11
Evan Stade
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.h#newcode41 ash/common/ash_switches.h:41: ASH_EXPORT extern const char kAshEnableWindowCycleUi[]; On 2016/07/06 23:47:30, Daniel ...
3 years ago (2016-07-07 15:24:42 UTC) #12
Daniel Erat
lgtm but others should still review. also adding sky@ since he was mentioned and is ...
3 years ago (2016-07-07 16:43:28 UTC) #14
Evan Stade
sky@, should WindowCycle* and ForwardingLayerDelegate move to ash/common/wm?
3 years ago (2016-07-07 17:04:23 UTC) #15
sky
On 2016/07/07 17:04:23, Evan Stade wrote: > sky@, should WindowCycle* and ForwardingLayerDelegate move to ash/common/wm? ...
3 years ago (2016-07-07 17:27:41 UTC) #16
varkha
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.cc#newcode79 ash/common/ash_switches.cc:79: const char kAshEnableWindowCycleUi[] = "ash-enable-window-cycle-ui"; Not sure ...
3 years ago (2016-07-07 17:40:29 UTC) #17
Evan Stade
as I was investigating how to handle minimized windows I realized that the forwarding layer ...
3 years ago (2016-07-07 19:24:12 UTC) #18
Evan Stade
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.cc#newcode79 ash/common/ash_switches.cc:79: const char kAshEnableWindowCycleUi[] = ...
3 years ago (2016-07-09 00:35:06 UTC) #19
varkha
https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_list.cc File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/30001/ash/wm/window_cycle_list.cc#newcode293 ash/wm/window_cycle_list.cc:293: widget, kShellWindowId_OverlayContainer, &params); On 2016/07/09 00:35:06, Evan Stade wrote: ...
3 years ago (2016-07-09 08:44:22 UTC) #20
Evan Stade
+sadrul could you ptal since sky is ooo?
3 years ago (2016-07-12 16:30:05 UTC) #22
sadrul
I am not really an ash owner, but I do have opinions! :) https://codereview.chromium.org/2129773002/diff/70001/ash/common/wm/forwarding_layer_delegate.h File ...
3 years ago (2016-07-13 18:38:08 UTC) #23
Evan Stade
> I am not really an ash owner +oshima would perhaps be a good reviewer ...
3 years ago (2016-07-13 19:31:57 UTC) #25
sadrul
I have some comments, but none intended to block the CL. You should land if ...
3 years ago (2016-07-15 14:41:53 UTC) #26
Evan Stade
https://codereview.chromium.org/2129773002/diff/70001/ash/wm/window_cycle_list.cc File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/70001/ash/wm/window_cycle_list.cc#newcode88 ash/wm/window_cycle_list.cc:88: class WindowMirrorView : public views::View, public ::wm::LayerDelegateFactory { On ...
3 years ago (2016-07-15 17:37:16 UTC) #27
oshima
can you add a test? Or if you want to add test later for a ...
3 years ago (2016-07-15 18:28:05 UTC) #28
Evan Stade
> can you add a test? Or if you want to add test later for ...
3 years ago (2016-07-15 18:44:09 UTC) #29
oshima
I'm ok if you want to add test later since this isn't enabled yet. > ...
3 years ago (2016-07-15 19:47:52 UTC) #32
Evan Stade
https://codereview.chromium.org/2129773002/diff/90001/ash/wm/window_cycle_list.cc File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/90001/ash/wm/window_cycle_list.cc#newcode318 ash/wm/window_cycle_list.cc:318: widget, kShellWindowId_OverlayContainer, &params); On 2016/07/15 19:47:52, oshima wrote: > ...
3 years ago (2016-07-15 22:49:02 UTC) #35
oshima
thanks, lgtm https://codereview.chromium.org/2129773002/diff/150001/ash/wm/window_cycle_list.cc File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/150001/ash/wm/window_cycle_list.cc#newcode353 ash/wm/window_cycle_list.cc:353: cycle_view_->target_window()->Show(); Did you need this? Activating minimiezd ...
3 years ago (2016-07-15 23:33:01 UTC) #38
Evan Stade
thanks all for review https://codereview.chromium.org/2129773002/diff/150001/ash/wm/window_cycle_list.cc File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/150001/ash/wm/window_cycle_list.cc#newcode353 ash/wm/window_cycle_list.cc:353: cycle_view_->target_window()->Show(); On 2016/07/15 23:33:00, oshima ...
3 years ago (2016-07-15 23:39:29 UTC) #39
oshima
https://codereview.chromium.org/2129773002/diff/150001/ash/wm/window_cycle_list.cc File ash/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2129773002/diff/150001/ash/wm/window_cycle_list.cc#newcode353 ash/wm/window_cycle_list.cc:353: cycle_view_->target_window()->Show(); On 2016/07/15 23:39:29, Evan Stade wrote: > On ...
3 years ago (2016-07-15 23:40:22 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2129773002/150001
2 years, 12 months ago (2016-07-18 16:34:58 UTC) #43
commit-bot: I haz the power
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-generic_chromium_compile_only_ng/builds/168983) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, ...
2 years, 12 months ago (2016-07-18 16:47:11 UTC) #45
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2129773002/170001
2 years, 12 months ago (2016-07-18 20:52:57 UTC) #48
commit-bot: I haz the power
Committed patchset #10 (id:170001)
2 years, 12 months ago (2016-07-18 22:28:42 UTC) #49
commit-bot: I haz the power
CQ bit was unchecked.
2 years, 12 months ago (2016-07-18 22:28:48 UTC) #50
commit-bot: I haz the power
2 years, 12 months ago (2016-07-18 22:30:41 UTC) #52
Message was sent while issue was closed.
Patchset 10 (id:??) landed as
https://crrev.com/6d7ebab8c2afeca3e3776c2e288fffa485d80902
Cr-Commit-Position: refs/heads/master@{#406120}

Powered by Google App Engine
This is Rietveld 408576698