Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
690103008:
Implemented swipe to close in overview mode. (Closed)
|
Created:
6 years, 1 month ago by bruthig Modified:
5 years, 10 months ago CC:
chromium-reviews, tdanderson+overview_chromium.org, kalyank, sadrul, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplemented swipe to close in overview mode.
BUG=393668
Committed: https://crrev.com/7ec5114efa5eada853c2ece5d6edb0fa4ce5611a
Cr-Commit-Position: refs/heads/master@{#316217}
Patch Set 1 #
Total comments: 31
Patch Set 2 : Rebased on to master. #Patch Set 3 : Removed ET_SCROLL_FLING_CANCEL handling from TransparentButton and removed WindowSelector.MultiWind… #
Total comments: 3
Patch Set 4 : Implemented swipe to close on top of other changes in overview mode #
Total comments: 22
Patch Set 5 : Rebase on master. #Patch Set 6 : Addressed flackrs comments from Patch Set 4. #
Total comments: 11
Patch Set 7 : Addressed flackr's comments from Patch Set 6. #
Total comments: 4
Patch Set 8 : Addressed flackr's comments from Patch Set 7. #
Messages
Total messages: 44 (14 generated)
bruthig@chromium.org changed reviewers: + derat@chromium.org, flackr@chromium.org, jhawkins@chromium.org, jwd@chromium.org
jwd@chromium.org: Please review changes in tools/metrics/histograms/* derat@chromium.org: Please review changes in ash/ash_switches.(h|cc) jhawkins@chromium.org: Please review changes in chrome/* flackr@chromium.org: Please review changes in ash/wm/*
is it typical to add a disable switch for a new feature like this, or should it first be behind an enable switch?
We often land as disabled with an enable flag, then switch the flag to be a disable flag for broad testing but still being able to switch it off easily for branch as necessary. Landing with a disable flag is just skipping that first step, although we might lose out on testing that the disable state works. https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/transparent_... File ash/wm/overview/transparent_activate_window_button.cc (right): https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/transparent_... ash/wm/overview/transparent_activate_window_button.cc:25: const int kDefaultCloseWindowDistanceMinimum = 100; Looks like this will never actually be used since the first thing we do is override it. https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/transparent_... ash/wm/overview/transparent_activate_window_button.cc:65: close_window_distance_minimum_ = distance; I think setting the close distance to a proportion of the button's size is reasonable and would save us the complication of passing down the value from WindowSelector. https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/transparent_... ash/wm/overview/transparent_activate_window_button.cc:80: const bool swipe_to_close_disabled_; Follow the pattern used by kEnableTouchFeedback to avoid having lots of copies of the flag value: https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/ui_base_sw... https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/transparent_... ash/wm/overview/transparent_activate_window_button.cc:155: case ui::ET_SCROLL_FLING_CANCEL: I don't think we need to worry about fling cancel since as I understand fling_start is after the finger lifts and fling cancel would be if they again put their finger down to stop the fling. I think closing as soon as the finger lifts up rather than worrying about a re-tap is simpler and more intuitive. https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/transparent_... ash/wm/overview/transparent_activate_window_button.cc:158: fling_velocity_x_ = fabs(event->details().velocity_x()); Why do we need to save the fling velocity? If I remember correctly the fling is generated after the finger lifts up so we can immediately evaluate at that point whether it translates into a close or not and immediately respond to it. https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/window_selec... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector_item.cc:39: const float kMinimumOpacity = 0.2f; 0.f seems like a decent close opacity. If you drag and the window becomes invisible it's obvious that a release will kill it. https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector_item.cc:42: const float kMinimumOpacityDistance = 200.0f; Shouldn't this be the same as the close distance? https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/window_selec... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector_unittest.cc:1303: TEST_F(WindowSelectorSwipeToCloseDisabledTest, WindowTapDragFarDistance) { I'm not sure testing the lack of a feature is something we want to be doing. For example, when removing features we don't test that those features aren't there anymore. https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector_unittest.cc:1380: TEST_F(WindowSelectorTest, MultiWindowTapDragShortDistance) { Is this exercising a different code path than WindowTapDragShortDistance? https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector_unittest.cc:1466: TEST_F(WindowSelectorTest, MultiWindowTapDragLabelFarDistance) { Aren't these multi window drag tests a superset of some of the above tests (the single window tests)? i.e. can we remove some of the above tests? https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector_unittest.cc:1494: TEST_F(WindowSelectorTest, LowVelocityFling) { Can probably merge this with the short distance drag as a bunch of things to try which don't close the window.
bruthig@chromium.org changed reviewers: + jonross@chromium.org
https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/transparent_... File ash/wm/overview/transparent_activate_window_button.cc (right): https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/transparent_... ash/wm/overview/transparent_activate_window_button.cc:29: const float kMinimumFlingVelocity = 4000.0f; Won't be needed if we decide to just close on fling events. https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/transparent_... ash/wm/overview/transparent_activate_window_button.cc:158: fling_velocity_x_ = fabs(event->details().velocity_x()); On 2014/11/07 19:10:52, flackr wrote: > Why do we need to save the fling velocity? If I remember correctly the fling is > generated after the finger lifts up so we can immediately evaluate at that point > whether it translates into a close or not and immediately respond to it. Currently used on line 162 to determine if fling is fast enough to close. Do we actually get slow flings? https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/window_selec... File ash/wm/overview/window_selector_item.h (right): https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector_item.h:105: virtual void Close() override; nit: remove virtual from all of the overrides.
pkotwicz@chromium.org changed reviewers: + pkotwicz@chromium.org
https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/transparent_... File ash/wm/overview/transparent_activate_window_button.cc (right): https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/transparent_... ash/wm/overview/transparent_activate_window_button.cc:80: const bool swipe_to_close_disabled_; Drive by: I dislike the pattern used with kEnableTouchFeedback because it is impossible to test the behavior with both return values of switches::IsTouchFeedbackEnabled() in the same test suite It is possible to test the behavior with both return values of base::CommandLine::ForCurrentProcess()->HasSwitch() because the command line is reset for each test case. TestClientInitializer does the resetting. There are ways of avoiding having a copy of the flag in each TransparentButton class (e.g. store the value in WindowSelectorController). I don't find any of these ways clearly better than a member variable in TransparentButton though.
this looks like it still needs a review, but lgtm for ash_switches.*.
Made changes for some comments and continued discussions for others. flackr@, jonross@ can you please take another look. I will also need to figure out how make the OverviewCloseButton to behave nicely when a window is being scrolled but this would best be done with an offline discussion with flackr@. https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/transparent_... File ash/wm/overview/transparent_activate_window_button.cc (right): https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/transparent_... ash/wm/overview/transparent_activate_window_button.cc:25: const int kDefaultCloseWindowDistanceMinimum = 100; On 2014/11/07 19:10:52, flackr (OOO) wrote: > Looks like this will never actually be used since the first thing we do is > override it. That is correct. I'm not sure if you are suggesting a change here or not? https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/transparent_... ash/wm/overview/transparent_activate_window_button.cc:29: const float kMinimumFlingVelocity = 4000.0f; On 2014/11/12 23:57:03, jonross wrote: > Won't be needed if we decide to just close on fling events. Windows are closed too easily if using all flings. I consulted stevet@ and we decided that a minimum was necessary. https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/transparent_... ash/wm/overview/transparent_activate_window_button.cc:65: close_window_distance_minimum_ = distance; On 2014/11/07 19:10:52, flackr (OOO) wrote: > I think setting the close distance to a proportion of the button's size is > reasonable and would save us the complication of passing down the value from > WindowSelector. Not 100% sure I understand but I will take a shot. I will assume you are suggesting that the close_window_distance_minimum should be calculated within the TransparentActivateWindowButton. Keep in mind the only size that the TransparentActivateWindowButton has access to is of the transparent overlay and when there are multiple windows this overlay's size is not the same as the entire WindowSelectorItem's size. I think it would be kind of odd if a smaller panel window closed at a different absolute distance than a larger window that took up the entire WindowSelectorItem's space when in the exact same overview layout. I'm not sure why passing in values like this is considered a complication? https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/transparent_... ash/wm/overview/transparent_activate_window_button.cc:80: const bool swipe_to_close_disabled_; On 2014/11/07 19:10:52, flackr (OOO) wrote: > Follow the pattern used by kEnableTouchFeedback to avoid having lots of copies > of the flag value: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/ui_base_sw... This is a very good example of how statics are problematic for testing ;) I will defer changing this until we have agreement. https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/transparent_... ash/wm/overview/transparent_activate_window_button.cc:155: case ui::ET_SCROLL_FLING_CANCEL: On 2014/11/07 19:10:52, flackr (OOO) wrote: > I don't think we need to worry about fling cancel since as I understand > fling_start is after the finger lifts and fling cancel would be if they again > put their finger down to stop the fling. I think closing as soon as the finger > lifts up rather than worrying about a re-tap is simpler and more intuitive. Done. https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/transparent_... ash/wm/overview/transparent_activate_window_button.cc:158: fling_velocity_x_ = fabs(event->details().velocity_x()); On 2014/11/07 19:10:52, flackr (OOO) wrote: > Why do we need to save the fling velocity? If I remember correctly the fling is > generated after the finger lifts up so we can immediately evaluate at that point > whether it translates into a close or not and immediately respond to it. It's easier IMO to only evaluate for one event type whether the window should close or not. If we also evaluate and possibly close during a ET_GESTURE_FLING_START event then we would need to keep track of whether the window was closed or not when we are evaluating during the ET_GESTURE_END event because we shouldn't attempt to close (or cancel scroll) on a window which has already been closed. https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/window_selec... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector_item.cc:39: const float kMinimumOpacity = 0.2f; On 2014/11/07 19:10:52, flackr wrote: > 0.f seems like a decent close opacity. If you drag and the window becomes > invisible it's obvious that a release will kill it. I consulted with stevet@ with the 0.2f value. If we want to change it we should loop him in to the conversation. I would prefer that we don't delay getting this CL in based on this and we can always change it later very easily. https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector_item.cc:42: const float kMinimumOpacityDistance = 200.0f; On 2014/11/07 19:10:52, flackr (OOO) wrote: > Shouldn't this be the same as the close distance? I agree, this should be in sync with the actual close distance. I will defer this until we agree on how that distance is calculated. https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/window_selec... File ash/wm/overview/window_selector_item.h (right): https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector_item.h:105: virtual void Close() override; On 2014/11/12 23:57:03, jonross (OOO Dec 20-Jan4) wrote: > nit: remove virtual from all of the overrides. Done. https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/window_selec... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector_unittest.cc:1303: TEST_F(WindowSelectorSwipeToCloseDisabledTest, WindowTapDragFarDistance) { On 2014/11/07 19:10:52, flackr wrote: > I'm not sure testing the lack of a feature is something we want to be doing. For > example, when removing features we don't test that those features aren't there > anymore. I would argue that testing a non-existent feature/code-path is completely different than testing a feature which is "turned off". The main purpose of this test isn't verifying that a window is not selected, it is to make sure that nothing blows up. I could make it more clear in the docs or test name as to the real purpose. https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector_unittest.cc:1380: TEST_F(WindowSelectorTest, MultiWindowTapDragShortDistance) { On 2014/11/07 19:10:52, flackr (OOO) wrote: > Is this exercising a different code path than WindowTapDragShortDistance? Removed. https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector_unittest.cc:1466: TEST_F(WindowSelectorTest, MultiWindowTapDragLabelFarDistance) { On 2014/11/07 19:10:52, flackr wrote: > Aren't these multi window drag tests a superset of some of the above tests (the > single window tests)? i.e. can we remove some of the above tests? I will remove the MultiWindowTapDragShortDistance test but I think all the others add value. The touch events in the MultiWindowTapDragLabelShortDistance and MultiWindowTapDragLabelFarDistance will be targeting the WindowSelectorItem's transparent overlay whereas in the single window tests above the event will be handled by the actual window's overlay. https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector_unittest.cc:1494: TEST_F(WindowSelectorTest, LowVelocityFling) { On 2014/11/07 19:10:52, flackr wrote: > Can probably merge this with the short distance drag as a bunch of things to try > which don't close the window. Unless you feel super strongly about this I would like to leave it as is. IMO it is much easier and better to divide tests based on the input behaviour compared to the expected out come. Doing it this way also makes the tests more stand alone (which is a good property) and if some tap+dragging behaviour changes then the developer wouldn't need to have to fully understand what the tap+fling behaviour is in order to update the test. It's also a lot easier for a new developer to learn expected outcome behaviour by looking up tests by good 'input' driven test names instead of looking up tests by the expected outcome.
https://codereview.chromium.org/690103008/diff/40001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/690103008/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:6089: + Disables swipe gestures to close windows in Overview Mode. This description doesn't seem to add much value to the title. Where did this string come from?
Responded to jhawkins@ comment. https://codereview.chromium.org/690103008/diff/40001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/690103008/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:6089: + Disables swipe gestures to close windows in Overview Mode. On 2015/01/05 19:47:59, James Hawkins wrote: > This description doesn't seem to add much value to the title. Where did this > string come from? I authored it but it definitely has room for improvement. Any suggestions?
On 2015/01/05 19:54:58, bruthig wrote: > Responded to jhawkins@ comment. > > https://codereview.chromium.org/690103008/diff/40001/chrome/app/generated_res... > File chrome/app/generated_resources.grd (right): > > https://codereview.chromium.org/690103008/diff/40001/chrome/app/generated_res... > chrome/app/generated_resources.grd:6089: + Disables swipe gestures to > close windows in Overview Mode. > On 2015/01/05 19:47:59, James Hawkins wrote: > > This description doesn't seem to add much value to the title. Where did this > > string come from? > > I authored it but it definitely has room for improvement. Any suggestions? I suggest asking a PM or UX. I don't remember exactly what the rule is, but if we can't find an informative string to use, I'm not sure we should have one at all.
https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/transparent_... File ash/wm/overview/transparent_activate_window_button.cc (right): https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/transparent_... ash/wm/overview/transparent_activate_window_button.cc:65: close_window_distance_minimum_ = distance; On 2015/01/02 16:49:17, bruthig wrote: > On 2014/11/07 19:10:52, flackr (OOO) wrote: > > I think setting the close distance to a proportion of the button's size is > > reasonable and would save us the complication of passing down the value from > > WindowSelector. > > Not 100% sure I understand but I will take a shot. I will assume you are > suggesting that the close_window_distance_minimum should be calculated within > the TransparentActivateWindowButton. > > Keep in mind the only size that the TransparentActivateWindowButton has access > to is of the transparent overlay and when there are multiple windows this > overlay's size is not the same as the entire WindowSelectorItem's size. I think > it would be kind of odd if a smaller panel window closed at a different absolute > distance than a larger window that took up the entire WindowSelectorItem's space > when in the exact same overview layout. > > I'm not sure why passing in values like this is considered a complication? Can the size of the WindowSelectionItem be accessed by the TransparentActivateWindowButton? If so this could be determined within this class if passing it around is a concern. I wouldn't want different sized panels to close at different fling distances. https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/transparent_... ash/wm/overview/transparent_activate_window_button.cc:80: const bool swipe_to_close_disabled_; On 2015/01/02 16:49:17, bruthig wrote: > On 2014/11/07 19:10:52, flackr (OOO) wrote: > > Follow the pattern used by kEnableTouchFeedback to avoid having lots of copies > > of the flag value: > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/ui_base_sw... > > This is a very good example of how statics are problematic for testing ;) > > I will defer changing this until we have agreement. It does add a complexity, however you can have one test class for the main behaviour, and then extend it to provide the other command line flag. Definitely not optimal though. https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/window_selec... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/690103008/diff/1/ash/wm/overview/window_selec... ash/wm/overview/window_selector_unittest.cc:1303: TEST_F(WindowSelectorSwipeToCloseDisabledTest, WindowTapDragFarDistance) { On 2015/01/02 16:49:17, bruthig wrote: > On 2014/11/07 19:10:52, flackr wrote: > > I'm not sure testing the lack of a feature is something we want to be doing. > For > > example, when removing features we don't test that those features aren't there > > anymore. > > I would argue that testing a non-existent feature/code-path is completely > different than testing a feature which is "turned off". The main purpose of > this test isn't verifying that a window is not selected, it is to make sure that > nothing blows up. I could make it more clear in the docs or test name as to the > real purpose. Could you clarify in the name+description?
bruthig@chromium.org changed reviewers: + nsatragno@chromium.org
bruthig@chromium.org changed reviewers: + tdanderson@chromium.org
I've re-implemented the swipe-to-close feature over in-flight changes that can be found here: https://codereview.chromium.org/872113004/ If all goes well I am trying to land this by feature freeze on Fri Feb 6. jwd@chromium.org jhawkins@chromium.org flackr@chromium.org Can you please have a look? Thanks :) https://codereview.chromium.org/690103008/diff/40001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/690103008/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:6089: + Disables swipe gestures to close windows in Overview Mode. On 2015/01/05 19:54:58, bruthig wrote: > On 2015/01/05 19:47:59, James Hawkins wrote: > > This description doesn't seem to add much value to the title. Where did this > > string come from? > > I authored it but it definitely has room for improvement. Any suggestions? The updated strings came from kuscher@.
On 2015/02/04 22:23:28, bruthig wrote: > I've re-implemented the swipe-to-close feature over in-flight changes that can > be found here: https://codereview.chromium.org/872113004/ > > If all goes well I am trying to land this by feature freeze on Fri Feb 6. > > mailto:jwd@chromium.org > mailto:jhawkins@chromium.org > mailto:flackr@chromium.org > > Can you please have a look? Thanks :) > > https://codereview.chromium.org/690103008/diff/40001/chrome/app/generated_res... > File chrome/app/generated_resources.grd (right): > > https://codereview.chromium.org/690103008/diff/40001/chrome/app/generated_res... > chrome/app/generated_resources.grd:6089: + Disables swipe gestures to > close windows in Overview Mode. > On 2015/01/05 19:54:58, bruthig wrote: > > On 2015/01/05 19:47:59, James Hawkins wrote: > > > This description doesn't seem to add much value to the title. Where did > this > > > string come from? > > > > I authored it but it definitely has room for improvement. Any suggestions? > > The updated strings came from kuscher@. Please list which owners are to review which files.
jwd@chromium.org: Please review changes in tools/metrics/histograms/* jhawkins@chromium.org: Please review changes in chrome/* flackr@chromium.org: Please review changes in ash/wm/*
https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:98: ui::Layer* layer = window->layer(); layer is only used once, remove this and access as window->layer()->SetOpacity(0.0f) below. https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:110: 1.0 - static_cast<float>(abs(distance)) / min_opacity_distance; nit: 1.0 is a double, use 1.0f for float calculation. https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:111: return std::min(1.0f, std::max(kMinimumOpacity, opacity)); Why std::min with 1.0f? opacity should always be strictly <= 1 from the caculation above, should it not? https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:153: // local coordinate space as |this|. s/as |this|/of |this| ? https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:167: int delta_x = event->x() - scroll_x_origin_; nit: While the variable is not used in this case, this really should be 0 on the SCROLL_BEGIN. I think it would be cleaner to do something like this at the start of the method: int delta_x = 0; if (event->type() == ui::ET_SCROLL_BEGIN) scroll_x_origin_ = event->x(); else delta_x = event->x() - scroll_x_origin_; https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:182: selector_item_->OnFling(delta_x, fabs(event->details().velocity_x())); I think rather than delegating these events to the selector item the WindowSelectorItem should just be the views::LabelButton. https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:317: OverviewAnimationType::OVERVIEW_ANIMATION_SCROLL_SELECTOR_ITEM, It confuses me that we have an animation type for this. i.e. I don't think that we would ever want an animation making the movement lag behind your finger. https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:421: swipe_to_close_disabled()) You should check the flag directly here. https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_unittest.cc:1296: // We need a widget for the close button to work, a bare window will crash. Probably worth mentioning why. Is a bare window missing a layer? https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_unittest.cc:1337: start, end, base::TimeDelta::FromMilliseconds(10), 5); Should probably also verify the window moves with the scroll and fades out.
I've addressed flackr's comments from the previous patch set. flackr@: Can you please have another look at files in ash/wm/* jwd@chromium.org: Can you please review changes in tools/metrics/histograms/* jhawkins@chromium.org: Can you please review changes in chrome/* https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:98: ui::Layer* layer = window->layer(); On 2015/02/10 17:49:49, flackr wrote: > layer is only used once, remove this and access as > window->layer()->SetOpacity(0.0f) below. Done. https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:110: 1.0 - static_cast<float>(abs(distance)) / min_opacity_distance; On 2015/02/10 17:49:49, flackr wrote: > nit: 1.0 is a double, use 1.0f for float calculation. Done. https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:111: return std::min(1.0f, std::max(kMinimumOpacity, opacity)); On 2015/02/10 17:49:49, flackr wrote: > Why std::min with 1.0f? opacity should always be strictly <= 1 from the > caculation above, should it not? Not if min_opacity_distance < 0. Alternatively I could DCHECK(min_opacity_distance > 0). Thoughts? https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:153: // local coordinate space as |this|. On 2015/02/10 17:49:49, flackr wrote: > s/as |this|/of |this| ? Done. https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:167: int delta_x = event->x() - scroll_x_origin_; On 2015/02/10 17:49:49, flackr wrote: > nit: While the variable is not used in this case, this really should be 0 on the > SCROLL_BEGIN. I think it would be cleaner to do something like this at the start > of the method: > int delta_x = 0; > if (event->type() == ui::ET_SCROLL_BEGIN) > scroll_x_origin_ = event->x(); > else > delta_x = event->x() - scroll_x_origin_; Done. https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:182: selector_item_->OnFling(delta_x, fabs(event->details().velocity_x())); On 2015/02/10 17:49:49, flackr wrote: > I think rather than delegating these events to the selector item the > WindowSelectorItem should just be the views::LabelButton. Discussed offline but will summarize here. It would be problematic for the WindowSelectorItem to be the contents view of a widget that it owned. Thus it was decided to leave as is. https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:317: OverviewAnimationType::OVERVIEW_ANIMATION_SCROLL_SELECTOR_ITEM, On 2015/02/10 17:49:49, flackr wrote: > It confuses me that we have an animation type for this. i.e. I don't think that > we would ever want an animation making the movement lag behind your finger. It was a while ago and I can't remember the exact steps to repro but I remember manipulating a window without creating a ScopedLayerAnimationSettings and assumed that the changes would be immediate however there was some rogue ScopedLayerAnimationSettings object in scope that caused the changes to actually be animated. Actually having an animation type for this is ensuring that it will indeed be instant. https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:421: swipe_to_close_disabled()) On 2015/02/10 17:49:49, flackr wrote: > You should check the flag directly here. The flag check has been moved in to the OverviewLabelButton::OnGestureEvent method which is called with high frequency so it was decided to not check the flag directly because it is not optimized for lookup. https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_unittest.cc:1296: // We need a widget for the close button to work, a bare window will crash. On 2015/02/10 17:49:50, flackr wrote: > Probably worth mentioning why. Is a bare window missing a layer? I've moved the comment to the CreateWindowWidget method documentation. Hopefully that is more understandable. https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_unittest.cc:1337: start, end, base::TimeDelta::FromMilliseconds(10), 5); On 2015/02/10 17:49:50, flackr wrote: > Should probably also verify the window moves with the scroll and fades out. Done.
On 2015/02/12 18:08:55, bruthig wrote: > I've addressed flackr's comments from the previous patch set. > > flackr@: Can you please have another look at files in ash/wm/* > > mailto:jwd@chromium.org: Can you please review changes in tools/metrics/histograms/* > > mailto:jhawkins@chromium.org: Can you please review changes in chrome/* > > https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... > File ash/wm/overview/window_selector_item.cc (right): > > https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... > ash/wm/overview/window_selector_item.cc:98: ui::Layer* layer = window->layer(); > On 2015/02/10 17:49:49, flackr wrote: > > layer is only used once, remove this and access as > > window->layer()->SetOpacity(0.0f) below. > > Done. > > https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... > ash/wm/overview/window_selector_item.cc:110: 1.0 - > static_cast<float>(abs(distance)) / min_opacity_distance; > On 2015/02/10 17:49:49, flackr wrote: > > nit: 1.0 is a double, use 1.0f for float calculation. > > Done. > > https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... > ash/wm/overview/window_selector_item.cc:111: return std::min(1.0f, > std::max(kMinimumOpacity, opacity)); > On 2015/02/10 17:49:49, flackr wrote: > > Why std::min with 1.0f? opacity should always be strictly <= 1 from the > > caculation above, should it not? > > Not if min_opacity_distance < 0. Alternatively I could > DCHECK(min_opacity_distance > 0). Thoughts? > > https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... > ash/wm/overview/window_selector_item.cc:153: // local coordinate space as > |this|. > On 2015/02/10 17:49:49, flackr wrote: > > s/as |this|/of |this| ? > > Done. > > https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... > ash/wm/overview/window_selector_item.cc:167: int delta_x = event->x() - > scroll_x_origin_; > On 2015/02/10 17:49:49, flackr wrote: > > nit: While the variable is not used in this case, this really should be 0 on > the > > SCROLL_BEGIN. I think it would be cleaner to do something like this at the > start > > of the method: > > int delta_x = 0; > > if (event->type() == ui::ET_SCROLL_BEGIN) > > scroll_x_origin_ = event->x(); > > else > > delta_x = event->x() - scroll_x_origin_; > > Done. > > https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... > ash/wm/overview/window_selector_item.cc:182: selector_item_->OnFling(delta_x, > fabs(event->details().velocity_x())); > On 2015/02/10 17:49:49, flackr wrote: > > I think rather than delegating these events to the selector item the > > WindowSelectorItem should just be the views::LabelButton. > > Discussed offline but will summarize here. It would be problematic for the > WindowSelectorItem to be the contents view of a widget that it owned. Thus it > was decided to leave as is. > > https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... > ash/wm/overview/window_selector_item.cc:317: > OverviewAnimationType::OVERVIEW_ANIMATION_SCROLL_SELECTOR_ITEM, > On 2015/02/10 17:49:49, flackr wrote: > > It confuses me that we have an animation type for this. i.e. I don't think > that > > we would ever want an animation making the movement lag behind your finger. > > It was a while ago and I can't remember the exact steps to repro but I remember > manipulating a window without creating a ScopedLayerAnimationSettings and > assumed that the changes would be immediate however there was some rogue > ScopedLayerAnimationSettings object in scope that caused the changes to actually > be animated. Actually having an animation type for this is ensuring that it > will indeed be instant. > > https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... > ash/wm/overview/window_selector_item.cc:421: swipe_to_close_disabled()) > On 2015/02/10 17:49:49, flackr wrote: > > You should check the flag directly here. > > The flag check has been moved in to the OverviewLabelButton::OnGestureEvent > method which is called with high frequency so it was decided to not check the > flag directly because it is not optimized for lookup. > > https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... > File ash/wm/overview/window_selector_unittest.cc (right): > > https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... > ash/wm/overview/window_selector_unittest.cc:1296: // We need a widget for the > close button to work, a bare window will crash. > On 2015/02/10 17:49:50, flackr wrote: > > Probably worth mentioning why. Is a bare window missing a layer? > > I've moved the comment to the CreateWindowWidget method documentation. > Hopefully that is more understandable. > > https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... > ash/wm/overview/window_selector_unittest.cc:1337: start, end, > base::TimeDelta::FromMilliseconds(10), 5); > On 2015/02/10 17:49:50, flackr wrote: > > Should probably also verify the window moves with the scroll and fades out. > > Done. LGTM
https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:111: return std::min(1.0f, std::max(kMinimumOpacity, opacity)); On 2015/02/12 18:08:55, bruthig wrote: > On 2015/02/10 17:49:49, flackr wrote: > > Why std::min with 1.0f? opacity should always be strictly <= 1 from the > > caculation above, should it not? > > Not if min_opacity_distance < 0. Alternatively I could > DCHECK(min_opacity_distance > 0). Thoughts? I'd prefer DCHECK, min_opacity_distance < 0 doesn't make sense. https://codereview.chromium.org/690103008/diff/60001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:317: OverviewAnimationType::OVERVIEW_ANIMATION_SCROLL_SELECTOR_ITEM, On 2015/02/12 18:08:55, bruthig wrote: > On 2015/02/10 17:49:49, flackr wrote: > > It confuses me that we have an animation type for this. i.e. I don't think > that > > we would ever want an animation making the movement lag behind your finger. > > It was a while ago and I can't remember the exact steps to repro but I remember > manipulating a window without creating a ScopedLayerAnimationSettings and > assumed that the changes would be immediate however there was some rogue > ScopedLayerAnimationSettings object in scope that caused the changes to actually > be animated. Actually having an animation type for this is ensuring that it > will indeed be instant. Acknowledged, though seems like something worth investigating as now we're violating the expected animation of whatever is calling this. It should just be on the call stack right? Add a TODO comment explaining this. https://codereview.chromium.org/690103008/diff/100001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/690103008/diff/100001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:168: event->SetHandled(); Only the scroll begin is handled, but not the scroll updates or end? Any reason? https://codereview.chromium.org/690103008/diff/100001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.h (right): https://codereview.chromium.org/690103008/diff/100001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.h:44: void OnGestureEvent(ui::GestureEvent* event) override; nit: indentation. https://codereview.chromium.org/690103008/diff/100001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.h:120: void OnFling(int delta_x, float velocity_x); nit: I kind of feel like we should just have an OnGestureEvent on this class, rather than splitting it up in OverviewLabelEvent. That would allow easily changing it if we had a better way of intercepting the gesture events (e.g. could probably clean this up by instantiating an event targeter or pretarget event handler to handle the scroll related gesture events on that window). https://codereview.chromium.org/690103008/diff/100001/ash/wm/overview/window_... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/690103008/diff/100001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1332: window->GetBoundsInScreen().x()); I didn't realize the window get bounds routines included the transform, perhaps we don't need the "GetTransformed(Target)?BoundsInRootWindow" helper methods which have been added in a few places? https://codereview.chromium.org/690103008/diff/100001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1335: EXPECT_GT(0.5f, window->layer()->opacity()); nit: I think with comparison EXPECT operators we reverse the order to put them in natural english order as it gives a more natural error on failure. i.e. this should be EXECT_LT(window->layer()->opacity(), 0.5f); https://codereview.chromium.org/690103008/diff/100001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1351: RunAllPendingInMessageLoop(); This should happen implicitly when control leaves the test and it shuts down.
New patchsets have been uploaded after l-g-t-m from jonross@chromium.org
flackr@ can you have another look? https://codereview.chromium.org/690103008/diff/100001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/690103008/diff/100001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:168: event->SetHandled(); On 2015/02/12 18:52:43, flackr wrote: > Only the scroll begin is handled, but not the scroll updates or end? Any reason? Done. https://codereview.chromium.org/690103008/diff/100001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.h (right): https://codereview.chromium.org/690103008/diff/100001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.h:44: void OnGestureEvent(ui::GestureEvent* event) override; On 2015/02/12 18:52:43, flackr wrote: > nit: indentation. Done. https://codereview.chromium.org/690103008/diff/100001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.h:120: void OnFling(int delta_x, float velocity_x); On 2015/02/12 18:52:43, flackr wrote: > nit: I kind of feel like we should just have an OnGestureEvent on this class, > rather than splitting it up in OverviewLabelEvent. That would allow easily > changing it if we had a better way of intercepting the gesture events (e.g. > could probably clean this up by instantiating an event targeter or pretarget > event handler to handle the scroll related gesture events on that window). Done. https://codereview.chromium.org/690103008/diff/100001/ash/wm/overview/window_... File ash/wm/overview/window_selector_unittest.cc (right): https://codereview.chromium.org/690103008/diff/100001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1335: EXPECT_GT(0.5f, window->layer()->opacity()); On 2015/02/12 18:52:43, flackr wrote: > nit: I think with comparison EXPECT operators we reverse the order to put them > in natural english order as it gives a more natural error on failure. i.e. this > should be EXECT_LT(window->layer()->opacity(), 0.5f); Done. https://codereview.chromium.org/690103008/diff/100001/ash/wm/overview/window_... ash/wm/overview/window_selector_unittest.cc:1351: RunAllPendingInMessageLoop(); On 2015/02/12 18:52:43, flackr wrote: > This should happen implicitly when control leaves the test and it shuts down. Done.
lgtm with nits. https://codereview.chromium.org/690103008/diff/120001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/690103008/diff/120001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:267: swipe_to_close_disabled()) { Return early instead. https://codereview.chromium.org/690103008/diff/120001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:283: case ui::ET_GESTURE_SCROLL_UPDATE: { I still think it would be correct to mark all of the scroll related events as handled so that nothing else listening to these events thinks that they haven't been handled yet.
lgtm
New patchsets have been uploaded after l-g-t-m from flackr@chromium.org,jwd@chromium.org
The CQ bit was checked by bruthig@chromium.org
Fixed nits, no need to review. Thanks all! :) https://codereview.chromium.org/690103008/diff/120001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/690103008/diff/120001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:267: swipe_to_close_disabled()) { On 2015/02/12 20:52:38, flackr wrote: > Return early instead. Done. https://codereview.chromium.org/690103008/diff/120001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:283: case ui::ET_GESTURE_SCROLL_UPDATE: { On 2015/02/12 20:52:38, flackr wrote: > I still think it would be correct to mark all of the scroll related events as > handled so that nothing else listening to these events thinks that they haven't > been handled yet. Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/690103008/140001
The CQ bit was unchecked by bruthig@chromium.org
The CQ bit was checked by bruthig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/690103008/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bruthig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/690103008/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bruthig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/690103008/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/7ec5114efa5eada853c2ece5d6edb0fa4ce5611a Cr-Commit-Position: refs/heads/master@{#316217} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
