Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2237703003:
Ash Window Cycle UI - add initial show delay and fade in. (Closed)
|
Created:
4 years, 4 months ago by Evan Stade Modified:
4 years, 4 months ago Reviewers:
sky CC:
chromium-reviews, kalyank, sadrul, Sebastien Gabriel Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAsh Window Cycle UI - add initial show delay and fade in.
UI doesn't show till 150ms have elapsed (or a second Tab press). UI fades in over 100ms.
BUG=635758, 626111
Committed: https://crrev.com/7a701c4452ebcec88acce2b347f61ab8786e2ab1
Cr-Commit-Position: refs/heads/master@{#411421}
Patch Set 1 #Patch Set 2 : adjust timings #
Total comments: 8
Patch Set 3 : add widget name #
Messages
Total messages: 20 (11 generated)
Description was changed from ========== Ash Window Cycle UI - add initial show delay and fade in. I resisted making this change because I thought it would look and feel janky. Now that I've written this CL, I have confirmed my suspicion that it would feel sluggish (to me). I am submitting it for review anyway so that others (sgabriel, mgreenwald, etc.) can try it out for themselves. BUG= ========== to ========== Ash Window Cycle UI - add initial show delay and fade in. I resisted making this change because I thought it would look and feel janky. Now that I've written this CL, I have confirmed my suspicion that it would feel sluggish (to me, in its current incarnation). I am submitting it for review anyway so that others (sgabriel, mgreenwald, etc.) can try it out for themselves. BUG=635758,626111 ==========
estade@chromium.org changed reviewers: + sky@chromium.org
it would be nice if I could just show this off in person rather than putting it through review before we know we want to keep it. Alas, I don't know how to get a live version of this in sgabriel's hands without either getting it into canary or boarding a plane.
On 2016/08/11 01:26:04, Evan Stade wrote: > it would be nice if I could just show this off in person rather than putting it > through review before we know we want to keep it. Alas, I don't know how to get > a live version of this in sgabriel's hands without either getting it into canary > or boarding a plane. @sky would you mind holding off on reviewing this one for the moment? I'm going to save Evan the plane trip and show sgabriel in person tomorrow. I'll follow up after with next steps - if we're okay going with 0ms delay, need to find an initial delay or I'm unable to get it in front of sgabriel in a timely manner. Personally am starting to think 0ms is the right move.
ok Scott, please review after all.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Description was changed from ========== Ash Window Cycle UI - add initial show delay and fade in. I resisted making this change because I thought it would look and feel janky. Now that I've written this CL, I have confirmed my suspicion that it would feel sluggish (to me, in its current incarnation). I am submitting it for review anyway so that others (sgabriel, mgreenwald, etc.) can try it out for themselves. BUG=635758,626111 ========== to ========== Ash Window Cycle UI - add initial show delay and fade in. UI doesn't show till 150ms have elapsed (or a second Tab press). UI fades in over 100ms. BUG=635758,626111 ==========
Dry run: 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
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2237703003/diff/20001/ash/common/wm/window_cy... File ash/common/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2237703003/diff/20001/ash/common/wm/window_cy... ash/common/wm/window_cycle_list.cc:238: base::TimeDelta::FromMilliseconds(100)); Please document why this uses a shorter time than is typical. https://codereview.chromium.org/2237703003/diff/20001/ash/common/wm/window_cy... ash/common/wm/window_cycle_list.cc:411: show_ui_timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(150), Document why the delay. https://codereview.chromium.org/2237703003/diff/20001/ash/common/wm/window_cy... ash/common/wm/window_cycle_list.cc:506: params.delegate = cycle_view_; It's very common in ash code to set a name on these widgets to help identify them later on. I'm suggesting you do something like: params.name = "WindowCycleList"; This way if you dump the window tree you'll see WindowCycleList.
https://codereview.chromium.org/2237703003/diff/20001/ash/common/wm/window_cy... File ash/common/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2237703003/diff/20001/ash/common/wm/window_cy... ash/common/wm/window_cycle_list.cc:238: base::TimeDelta::FromMilliseconds(100)); On 2016/08/11 19:36:27, sky wrote: > Please document why this uses a shorter time than is typical. I don't know if there's really a reason beyond "this is what Sebastien thought felt good". I guess you could go by the old axiom that any action should take at most 250ms to happen, and with the 150ms initial delay this transition only has 100ms left to work with. What kind of comment would you like to see for this admittedly arbitrary value? https://codereview.chromium.org/2237703003/diff/20001/ash/common/wm/window_cy... ash/common/wm/window_cycle_list.cc:411: show_ui_timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(150), On 2016/08/11 19:36:27, sky wrote: > Document why the delay. It is documented in the header file above the member declaration, is that comment insufficient or do you think it should be moved here? https://codereview.chromium.org/2237703003/diff/20001/ash/common/wm/window_cy... ash/common/wm/window_cycle_list.cc:506: params.delegate = cycle_view_; On 2016/08/11 19:36:26, sky wrote: > It's very common in ash code to set a name on these widgets to help identify > them later on. I'm suggesting you do something like: > > params.name = "WindowCycleList"; > > This way if you dump the window tree you'll see WindowCycleList. Done.
https://codereview.chromium.org/2237703003/diff/20001/ash/common/wm/window_cy... File ash/common/wm/window_cycle_list.cc (right): https://codereview.chromium.org/2237703003/diff/20001/ash/common/wm/window_cy... ash/common/wm/window_cycle_list.cc:238: base::TimeDelta::FromMilliseconds(100)); On 2016/08/11 20:00:11, Evan Stade wrote: > On 2016/08/11 19:36:27, sky wrote: > > Please document why this uses a shorter time than is typical. > > I don't know if there's really a reason beyond "this is what Sebastien thought > felt good". I guess you could go by the old axiom that any action should take at > most 250ms to happen, and with the 150ms initial delay this transition only has > 100ms left to work with. > > What kind of comment would you like to see for this admittedly arbitrary value? Fair enough. https://codereview.chromium.org/2237703003/diff/20001/ash/common/wm/window_cy... ash/common/wm/window_cycle_list.cc:411: show_ui_timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(150), On 2016/08/11 20:00:11, Evan Stade wrote: > On 2016/08/11 19:36:27, sky wrote: > > Document why the delay. > > It is documented in the header file above the member declaration, is that > comment insufficient or do you think it should be moved here? What you have is fine. I clearly didn't look enough.
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2237703003/#ps40001 (title: "add widget name")
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.
Description was changed from ========== Ash Window Cycle UI - add initial show delay and fade in. UI doesn't show till 150ms have elapsed (or a second Tab press). UI fades in over 100ms. BUG=635758,626111 ========== to ========== Ash Window Cycle UI - add initial show delay and fade in. UI doesn't show till 150ms have elapsed (or a second Tab press). UI fades in over 100ms. BUG=635758,626111 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Ash Window Cycle UI - add initial show delay and fade in. UI doesn't show till 150ms have elapsed (or a second Tab press). UI fades in over 100ms. BUG=635758,626111 ========== to ========== Ash Window Cycle UI - add initial show delay and fade in. UI doesn't show till 150ms have elapsed (or a second Tab press). UI fades in over 100ms. BUG=635758,626111 Committed: https://crrev.com/7a701c4452ebcec88acce2b347f61ab8786e2ab1 Cr-Commit-Position: refs/heads/master@{#411421} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7a701c4452ebcec88acce2b347f61ab8786e2ab1 Cr-Commit-Position: refs/heads/master@{#411421} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
