Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2930123002:
Tablet WM : Swiping on system tray bubble. (Closed)
DescriptionTablet WM : Swiping on system tray bubble.
Swiping up on the system tray to open the system tray bubble.
1.This works only for tablet and horizontal shelf.
2.Update the bounds of the bubble according to the position of the gesture.
3.For SCROLL_END event. To show or hide the bubble depends on
the position of the gesture. Bubble is split into two parts.
End in the lower part (one-third position) will hide the bubble;
Otherwise, should always show the bubble.
4.For FLING event. To show or hide the bubble depends on
gesture position and velocity.
If the fling velocity larger than |kSwipingVelocity|, show or hide
according to the fling direction;
Otherwise, show or hide depends on the position.
BUG=734814
Review-Url: https://codereview.chromium.org/2930123002
Cr-Commit-Position: refs/heads/master@{#482696}
Committed: https://chromium.googlesource.com/chromium/src/+/e6131f3b26bf5d8904d266f9f826fb811b28f875
Patch Set 1 #
Total comments: 26
Patch Set 2 : Add animation and addressed xiyuan's comments. #Patch Set 3 : Add tests and rebased. #Patch Set 4 : . #Patch Set 5 : . #Patch Set 6 : . #
Total comments: 29
Patch Set 7 : Addressed xiyuan's comments. #
Total comments: 43
Patch Set 8 : Addressed comments. #
Total comments: 40
Patch Set 9 : Fixed tdanderson's comments. #
Total comments: 16
Patch Set 10 : Fixed tdanderson's comments. #Patch Set 11 : Fixed UT. #
Total comments: 19
Patch Set 12 : Fixed comments. #
Total comments: 1
Patch Set 13 : Fixed comments. #
Messages
Total messages: 88 (64 generated)
The CQ bit was checked by minch@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...
Description was changed from ========== Swiping on system tray bubble. BUG=725977 ========== to ========== Swiping on system tray bubble. Swiping up on the system tray should open the system tray bubble. 1.Change the bounds of the bubble according to the position of the gesture. 2.For SCROLL_END event. Whether to show or hide the bubble depends on the position of the gesture. Bubble is split into three parts. End in the lowest part will hide the bubble; End in the highest part will show the bubble; End in the middle part depends on the scroll delta of the final gesture. 3.For FLING event. Whether to show or hide the bubble depends on gesture position and velocity. If the fling velocity larger than |kSwipingVelocity|, show or hide according to the fling direction; Otherwise, show or hide depends on the position. 4.The rules maybe changed later. No animation currently. BUG=725977 ==========
minch@chromium.org changed reviewers: + tdanderson@chromium.org, xdai@chromium.org, xiyuan@chromium.org
On 2017/06/09 20:29:05, minch1 wrote: > mailto:minch@chromium.org changed reviewers: > + mailto:tdanderson@chromium.org, mailto:xdai@chromium.org, mailto:xiyuan@chromium.org I find it difficult to understand the intended behavior of this CL based on the description alone. Are there specific mocks you can point me to? All I can find is a bullet point "Dragging up on quick settings opens quick settings" with no mock or explanation about points (1)-(3) in your CL description. Also, please look into adding test coverage for this new functionality.
Could you please help review? Thanks.
On 2017/06/09 21:53:21, tdanderson wrote: > On 2017/06/09 20:29:05, minch1 wrote: > > mailto:minch@chromium.org changed reviewers: > > + mailto:tdanderson@chromium.org, mailto:xdai@chromium.org, > mailto:xiyuan@chromium.org > > I find it difficult to understand the intended behavior > of this CL based on the description alone. Are there specific > mocks you can point me to? All I can find is a bullet point > "Dragging up on quick settings opens quick settings" with no > mock or explanation about points (1)-(3) in your CL description. > > Also, please look into adding test coverage for this new functionality. Maybe you can take a look of this https://docs.google.com/presentation/d/1c97J5hI-4lgSf4TNA7uNsZy2y21G9RqOXR1D8... Looks like they have no mocks for the system tray bubble yet. But they have the mock for swiping up on the shelf to show the launcher.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray.cc File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.cc:82: constexpr float kSwipingVelocity = 100.0; nit: move this to where it is used since it is referenced only in one place. https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.cc:179: // on maximize mode. Where is the relevant code for "on maximize mode."? If maximize mode does not matter, removing this part in the comment. https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.cc:216: Shell::Get()->AddShellObserver(this); What if Shell starts in maximize mode? |on_maximize_mode_| is initialized to false and would be wrong in this case. https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.cc:603: on_maximize_mode_ = true; on_maximize_mode_ -> in_maximize_mode_ (or is_maximize_mode_) https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.cc:719: system_bubble_->bubble_view()->GetWidget()->Show(); Can we restrict to call Show() only when necessary? I would recommend to calculate initial bounds in StartGestureDrag() and call Show() there. https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.cc:730: system_tray_bubble_bounds_); nit: wrap with {} since it takes more than one line. And wrap "else" branch too. https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.cc:748: float one_third = system_bubble_height / 3.0; move this two lines down to where they are used. https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.cc:755: else if (fabs(gesture_drag_amount_) > 2 * one_third) no "else" after "return". See https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.cc:765: } else { no "else" after "return". https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray.h File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.h:226: Shelf* shelf_; nit: Shelf* -> Shelf* const since |shelf_| should never change during the lifetime of this class. Also add a brief comment to document it. https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.h:234: float gesture_drag_amount_; nit: initialize it to some value. https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.h:237: float final_scroll_y_; nit: initialize it https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... File ash/system/tray/system_tray_bubble.cc (right): https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray_bubble.cc:205: aura::Window* clipping_window = new aura::Window(nullptr); Re-use existing |clipping_window| if it has been created before. Otherwise, we are kind of leaking it.
The CQ bit was checked by minch@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...
Description was changed from ========== Swiping on system tray bubble. Swiping up on the system tray should open the system tray bubble. 1.Change the bounds of the bubble according to the position of the gesture. 2.For SCROLL_END event. Whether to show or hide the bubble depends on the position of the gesture. Bubble is split into three parts. End in the lowest part will hide the bubble; End in the highest part will show the bubble; End in the middle part depends on the scroll delta of the final gesture. 3.For FLING event. Whether to show or hide the bubble depends on gesture position and velocity. If the fling velocity larger than |kSwipingVelocity|, show or hide according to the fling direction; Otherwise, show or hide depends on the position. 4.The rules maybe changed later. No animation currently. BUG=725977 ========== to ========== Swiping on system tray bubble. Swiping up on the system tray should open the system tray bubble. 1.Change the bounds of the bubble according to the position of the gesture. 2.For SCROLL_END event. Whether to show or hide the bubble depends on the position of the gesture. Bubble is split into two parts. End in the lower part (one-third position) will hide the bubble; Otherwise, should always show the bubble. 3.For FLING event. Whether to show or hide the bubble depends on gesture position and velocity. If the fling velocity larger than |kSwipingVelocity|, show or hide according to the fling direction; Otherwise, show or hide depends on the position. 4.The rules maybe changed later. BUG=725977 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 minch@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...
Description was changed from ========== Swiping on system tray bubble. Swiping up on the system tray should open the system tray bubble. 1.Change the bounds of the bubble according to the position of the gesture. 2.For SCROLL_END event. Whether to show or hide the bubble depends on the position of the gesture. Bubble is split into two parts. End in the lower part (one-third position) will hide the bubble; Otherwise, should always show the bubble. 3.For FLING event. Whether to show or hide the bubble depends on gesture position and velocity. If the fling velocity larger than |kSwipingVelocity|, show or hide according to the fling direction; Otherwise, show or hide depends on the position. 4.The rules maybe changed later. BUG=725977 ========== to ========== Tablet WM : Swiping on system tray bubble. Swiping up on the system tray should open the system tray bubble. 1.Change the bounds of the bubble according to the position of the gesture. 2.For SCROLL_END event. Whether to show or hide the bubble depends on the position of the gesture. Bubble is split into two parts. End in the lower part (one-third position) will hide the bubble; Otherwise, should always show the bubble. 3.For FLING event. Whether to show or hide the bubble depends on gesture position and velocity. If the fling velocity larger than |kSwipingVelocity|, show or hide according to the fling direction; Otherwise, show or hide depends on the position. 4.The rules maybe changed later. BUG=725977 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 minch@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...
The CQ bit was checked by minch@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 minch@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Could you please help review? Thanks. https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray.cc File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.cc:82: constexpr float kSwipingVelocity = 100.0; On 2017/06/12 18:00:51, xiyuan wrote: > nit: move this to where it is used since it is referenced only in one place. Done. https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.cc:179: // on maximize mode. On 2017/06/12 18:00:50, xiyuan wrote: > Where is the relevant code for "on maximize mode."? If maximize mode does not > matter, removing this part in the comment. Since we added a clipping window in maximize mode. When we click the items in the default menu it will trigger OnWindowActivated here. We should make sure the menu will not be closed in this situation though the comment here maybe not so good. https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.cc:216: Shell::Get()->AddShellObserver(this); On 2017/06/12 18:00:51, xiyuan wrote: > What if Shell starts in maximize mode? |on_maximize_mode_| is initialized to > false and would be wrong in this case. Done. https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.cc:603: on_maximize_mode_ = true; On 2017/06/12 18:00:51, xiyuan wrote: > on_maximize_mode_ -> in_maximize_mode_ (or is_maximize_mode_) Done. https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.cc:719: system_bubble_->bubble_view()->GetWidget()->Show(); On 2017/06/12 18:00:50, xiyuan wrote: > Can we restrict to call Show() only when necessary? I would recommend to > calculate initial bounds in StartGestureDrag() and call Show() there. Done. https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.cc:730: system_tray_bubble_bounds_); On 2017/06/12 18:00:51, xiyuan wrote: > nit: wrap with {} since it takes more than one line. And wrap "else" branch too. Acknowledged. https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.cc:748: float one_third = system_bubble_height / 3.0; On 2017/06/12 18:00:51, xiyuan wrote: > move this two lines down to where they are used. Done. https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.cc:755: else if (fabs(gesture_drag_amount_) > 2 * one_third) On 2017/06/12 18:00:51, xiyuan wrote: > no "else" after "return". > > See > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.cc:765: } else { On 2017/06/12 18:00:50, xiyuan wrote: > no "else" after "return". Done. https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray.h File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.h:226: Shelf* shelf_; On 2017/06/12 18:00:51, xiyuan wrote: > nit: Shelf* -> Shelf* const since |shelf_| should never change during the > lifetime of this class. > > Also add a brief comment to document it. Done. https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.h:234: float gesture_drag_amount_; On 2017/06/12 18:00:51, xiyuan wrote: > nit: initialize it to some value. Done. https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray.h:237: float final_scroll_y_; On 2017/06/12 18:00:51, xiyuan wrote: > nit: initialize it Done. https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... File ash/system/tray/system_tray_bubble.cc (right): https://codereview.chromium.org/2930123002/diff/1/ash/system/tray/system_tray... ash/system/tray/system_tray_bubble.cc:205: aura::Window* clipping_window = new aura::Window(nullptr); On 2017/06/12 18:00:51, xiyuan wrote: > Re-use existing |clipping_window| if it has been created before. Otherwise, we > are kind of leaking it. Done.
Description was changed from ========== Tablet WM : Swiping on system tray bubble. Swiping up on the system tray should open the system tray bubble. 1.Change the bounds of the bubble according to the position of the gesture. 2.For SCROLL_END event. Whether to show or hide the bubble depends on the position of the gesture. Bubble is split into two parts. End in the lower part (one-third position) will hide the bubble; Otherwise, should always show the bubble. 3.For FLING event. Whether to show or hide the bubble depends on gesture position and velocity. If the fling velocity larger than |kSwipingVelocity|, show or hide according to the fling direction; Otherwise, show or hide depends on the position. 4.The rules maybe changed later. BUG=725977 ========== to ========== Tablet WM : Swiping on system tray bubble. Swiping up on the system tray should open the system tray bubble. 1.Change the bounds of the bubble according to the position of the gesture. 2.For SCROLL_END event. Whether to show or hide the bubble depends on the position of the gesture. Bubble is split into two parts. End in the lower part (one-third position) will hide the bubble; Otherwise, should always show the bubble. 3.For FLING event. Whether to show or hide the bubble depends on gesture position and velocity. If the fling velocity larger than |kSwipingVelocity|, show or hide according to the fling direction; Otherwise, show or hide depends on the position. 4.The rules maybe change later. BUG=725977 ==========
https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray.cc:217: in_maximize_mode_ = true; nit: just do in_maximize_mode_ = Shell::Get() ->maximize_mode_controller() ->IsMaximizeModeWindowManagerEnabled(); https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray.cc:760: return true; nit: Line 758-760 can be this return fabs(gesture_drag_amount_) >= one_third; https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray.cc:768: const float kSwipingVelocity = 100.0; nit: 100.0 -> 100.0f since default is double, appending "f" to ensure correct type for float. https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray.h:161: friend class test::SystemTrayTest; Can you extend SystemTrayTestApi to allow setting |in_maximize_mode_| instead of adding new friends? https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray.h:231: // dragging. This comment seems to be just for ShouldShowSystemBubbleForSwiping. Move it there and document ShouldShowSystemBubbleBasedOnPosition properly. https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray.h:232: bool ShouldShowSystemBubbleBasedOnPosition(); nit: ShouldShowSystemBubbleBasedOnPosition -> ShouldShowSystemBubbleBasedOnDragAmount https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... File ash/system/tray/system_tray_bubble.cc (right): https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray_bubble.cc:81: if (system_tray_bubble_) What if |system_tray_bubble_| go away before the animation finishes? Testing |system_tray_bubble_| probably does not help on that. If OnImplicitAnimationsCompleted() is called when bubble widget is closed before animation finishes, we can drop the "if (system_tray_bubble_)" since it is not useful. If it is not called, then we might need to also observe the bubble widget so that we clean up properly for such situation. Think you can verify this by extending the animation to some long time (e.g. 5 seconds), then try to dismiss the tray (clicking on desktop, on the tray again etc) while animation runs. https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray_bubble.cc:227: clipping_window_ = new aura::Window(nullptr); When would |clipping_window_| be released? We probably should do delete clipping_window_; in SystemTrayBubble's dtor to balance this new. https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray_bubble.cc:232: clipping_window_->Show(); Line 228-232 (except line 230 that updates bounds to match workarea) should be included in "if (!clipping_window_)" as well since they only need to run once. https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... File ash/system/tray/system_tray_bubble.h (right): https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray_bubble.h:65: // is hidden behind the shelf. The comment seems not exactly correct since UpdateBounds does not test whether bubble is hidden behind the shelf. Please update. e.g. // Update the bounds of the system tray bubble. Close the bubble if // |close_bubble| is set. https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:94: float height() { nit: height -> GetSystemBubbleHeight https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:129: // in masimize mode. nit: masimize -> maximize https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:132: ASSERT_TRUE(system_tray()->HasSystemBubble()); nit: ASSERT_TRUE -> EXPECT_TRUE Use ASSERT_xxx when the rest of test code does not run or makes no sense when it fails. Most of the time, we use EXPECT_xxx to see how many cases fail. https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:133: nit: move this empty line after CloseSystemBubble() below to separate test cases.
The CQ bit was checked by minch@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...
Description was changed from ========== Tablet WM : Swiping on system tray bubble. Swiping up on the system tray should open the system tray bubble. 1.Change the bounds of the bubble according to the position of the gesture. 2.For SCROLL_END event. Whether to show or hide the bubble depends on the position of the gesture. Bubble is split into two parts. End in the lower part (one-third position) will hide the bubble; Otherwise, should always show the bubble. 3.For FLING event. Whether to show or hide the bubble depends on gesture position and velocity. If the fling velocity larger than |kSwipingVelocity|, show or hide according to the fling direction; Otherwise, show or hide depends on the position. 4.The rules maybe change later. BUG=725977 ========== to ========== Tablet WM : Swiping on system tray bubble. Swiping up on the system tray should open the system tray bubble. 1.Change the bounds of the bubble according to the position of the gesture. 2.For SCROLL_END event. To show or hide the bubble depends on the position of the gesture. Bubble is split into two parts. End in the lower part (one-third position) will hide the bubble; Otherwise, should always show the bubble. 3.For FLING event. To show or hide the bubble depends on gesture position and velocity. If the fling velocity larger than |kSwipingVelocity|, show or hide according to the fling direction; Otherwise, show or hide depends on the position. BUG=725977 ==========
Description was changed from ========== Tablet WM : Swiping on system tray bubble. Swiping up on the system tray should open the system tray bubble. 1.Change the bounds of the bubble according to the position of the gesture. 2.For SCROLL_END event. To show or hide the bubble depends on the position of the gesture. Bubble is split into two parts. End in the lower part (one-third position) will hide the bubble; Otherwise, should always show the bubble. 3.For FLING event. To show or hide the bubble depends on gesture position and velocity. If the fling velocity larger than |kSwipingVelocity|, show or hide according to the fling direction; Otherwise, show or hide depends on the position. BUG=725977 ========== to ========== Tablet WM : Swiping on system tray bubble. Swiping up on the system tray should open the system tray bubble. 1.This works only for tablet and horizontal shelf. 2.Update the bounds of the bubble according to the position of the gesture. 3.For SCROLL_END event. To show or hide the bubble depends on the position of the gesture. Bubble is split into two parts. End in the lower part (one-third position) will hide the bubble; Otherwise, should always show the bubble. 4.For FLING event. To show or hide the bubble depends on gesture position and velocity. If the fling velocity larger than |kSwipingVelocity|, show or hide according to the fling direction; Otherwise, show or hide depends on the position. BUG=725977 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray.cc:217: in_maximize_mode_ = true; On 2017/06/16 17:33:42, xiyuan wrote: > nit: just do > > in_maximize_mode_ = Shell::Get() > ->maximize_mode_controller() > ->IsMaximizeModeWindowManagerEnabled(); Done. https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray.cc:760: return true; On 2017/06/16 17:33:42, xiyuan wrote: > nit: Line 758-760 can be this > > return fabs(gesture_drag_amount_) >= one_third; Done. https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray.cc:768: const float kSwipingVelocity = 100.0; On 2017/06/16 17:33:42, xiyuan wrote: > nit: 100.0 -> 100.0f since default is double, appending "f" to ensure correct > type for float. Done. https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray.h:161: friend class test::SystemTrayTest; On 2017/06/16 17:33:42, xiyuan wrote: > Can you extend SystemTrayTestApi to allow setting |in_maximize_mode_| instead of > adding new friends? Done. https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray.h:231: // dragging. On 2017/06/16 17:33:42, xiyuan wrote: > This comment seems to be just for ShouldShowSystemBubbleForSwiping. Move it > there and document ShouldShowSystemBubbleBasedOnPosition properly. Done. https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray.h:232: bool ShouldShowSystemBubbleBasedOnPosition(); On 2017/06/16 17:33:42, xiyuan wrote: > nit: ShouldShowSystemBubbleBasedOnPosition -> > ShouldShowSystemBubbleBasedOnDragAmount Done. https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... File ash/system/tray/system_tray_bubble.cc (right): https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray_bubble.cc:81: if (system_tray_bubble_) On 2017/06/16 17:33:42, xiyuan wrote: > What if |system_tray_bubble_| go away before the animation finishes? Testing > |system_tray_bubble_| probably does not help on that. > > If OnImplicitAnimationsCompleted() is called when bubble widget is closed before > animation finishes, we can drop the "if (system_tray_bubble_)" since it is not > useful. > > If it is not called, then we might need to also observe the bubble widget so > that we clean up properly for such situation. > > Think you can verify this by extending the animation to some long time (e.g. 5 > seconds), then try to dismiss the tray (clicking on desktop, on the tray again > etc) while animation runs. Verified that OnImplicitAnimationsCompleted() will be called if bubble_widget is closed before animation finishes (click the desktop before the animation time exhausted). Remove "if (system_tray_bubble_)" directly here. https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray_bubble.cc:227: clipping_window_ = new aura::Window(nullptr); On 2017/06/16 17:33:43, xiyuan wrote: > When would |clipping_window_| be released? > > We probably should do > delete clipping_window_; > in SystemTrayBubble's dtor to balance this new. Done. https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray_bubble.cc:232: clipping_window_->Show(); On 2017/06/16 17:33:43, xiyuan wrote: > Line 228-232 (except line 230 that updates bounds to match workarea) should be > included in "if (!clipping_window_)" as well since they only need to run once. Done. https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... File ash/system/tray/system_tray_bubble.h (right): https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray_bubble.h:65: // is hidden behind the shelf. On 2017/06/16 17:33:43, xiyuan wrote: > The comment seems not exactly correct since UpdateBounds does not test whether > bubble is hidden behind the shelf. Please update. > > e.g. > // Update the bounds of the system tray bubble. Close the bubble if > // |close_bubble| is set. Done. https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:94: float height() { On 2017/06/16 17:33:43, xiyuan wrote: > nit: height -> GetSystemBubbleHeight Done. https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:129: // in masimize mode. On 2017/06/16 17:33:43, xiyuan wrote: > nit: masimize -> maximize Done. https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:132: ASSERT_TRUE(system_tray()->HasSystemBubble()); On 2017/06/16 17:33:43, xiyuan wrote: > nit: ASSERT_TRUE -> EXPECT_TRUE > > Use ASSERT_xxx when the rest of test code does not run or makes no sense when it > fails. Most of the time, we use EXPECT_xxx to see how many cases fail. Done. https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:133: On 2017/06/16 17:33:43, xiyuan wrote: > nit: move this empty line after CloseSystemBubble() below to separate test > cases. Done.
lgtm https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... File ash/system/tray/system_tray_bubble.cc (right): https://codereview.chromium.org/2930123002/diff/100001/ash/system/tray/system... ash/system/tray/system_tray_bubble.cc:81: if (system_tray_bubble_) On 2017/06/16 22:11:51, minch1 wrote: > On 2017/06/16 17:33:42, xiyuan wrote: > > What if |system_tray_bubble_| go away before the animation finishes? Testing > > |system_tray_bubble_| probably does not help on that. > > > > If OnImplicitAnimationsCompleted() is called when bubble widget is closed > before > > animation finishes, we can drop the "if (system_tray_bubble_)" since it is not > > useful. > > > > If it is not called, then we might need to also observe the bubble widget so > > that we clean up properly for such situation. > > > > Think you can verify this by extending the animation to some long time (e.g. 5 > > seconds), then try to dismiss the tray (clicking on desktop, on the tray again > > etc) while animation runs. > > Verified that OnImplicitAnimationsCompleted() will be called if bubble_widget is > closed before animation finishes (click the desktop before the animation time > exhausted). Remove "if (system_tray_bubble_)" directly here. Ack. Thanks.
Hi Min, I have left a round of comments below. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.cc:651: void SystemTray::OnGestureEvent(ui::GestureEvent* event) { Why are you restricting this to just a horizontally-aligned shelf? https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.cc:652: if (in_maximize_mode_ && shelf_->IsHorizontalAlignment() && Side question to ask UX (OK to handle in a follow-on CL): since we're letting the user open the system menu with a swipe-up, should we also be letting them close the system menu with a swipe-down on a menu that is already open? https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.cc:656: CustomButton::OnGestureEvent(event); I think you'd instead want to call up to TrayBackrgroundView::OnGestureEvent() since SystemTray inherits directly from TrayBackgroundView. If you were to call up to CustomButton::OnGestureEvent() and at some point in the future someone added a TrayBackgroundView::OnGestureEvent() or ActionableView::OnGestureEvent() override, these overrides would be skipped when a gesture targeted SystemTray. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.cc:678: bool SystemTray::ProcessGestureEvent(const ui::GestureEvent& event) { When building and trying out this CL locally, I can get a crash when I do the following when the system menu is closed: without your finger leaving the screen, very quickly swipe up on the system menu, then down, then up, then down again. One possible cause for this crash could be that you are not handling all relevant possibilities for gesture events. For instance you may be receiving ET_SCROLL_FLING_CANCEL, ET_GESTURE_SWIPE, ET_GESTURE_END, etc. I suggest adding logging to this function to see which event types and sequences can be seen. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.cc:697: IsDraggingDownOnShelf(gesture)) { I don't understand what the check IsDraggingDownOnShelf() does and why you need it. Can you please explain? https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.cc:765: if (fabs(gesture.details().velocity_y()) > kSwipingVelocity) Consider setting kSwipingVelocity to -100 and inlining ShouldShowSystemBubbleBasedOnDragAmount(). Then you could simplify this function to just: const float kSwipingVelocity = -100.0f; if (gesture.type() == FLING_START && gesture.details.velocity_y() < kSwipingVelocity()) { return true; } return -gesture_drag_amount_ >= system_tray_bubble_bounds_.height() / 3.0; https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.h:10: #include <vector> I see you are using bug 725977 to track many different changes. As a suggestion, consider splitting each surface into its own bug, and mark these as blocking the 725977 tracking bug. In my opinion there are many advantages to doing this: * It makes it easier to track what has already been done and what is left to do. * If the feature work will span across different milestones, it's easier to tell which parts were landed in which milestones since each bug will have its own M- label. * It is easier to track regressions, in case any individual CL needs to be reverted. * When a bug is used to track only a single piece of feature work, it makes it simpler for the test team to perform manual verification. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.h:48: class ASH_EXPORT SystemTray : public TrayBackgroundView, Note that I have left a comment here listing the other overrides of TrayBackgroundView: https://bugs.chromium.org/p/chromium/issues/detail?id=725977#c4 It looks like you are planning to add "swipe up" functionality to many of these. I'm OK with this as the first CL, but in a subsequent CLs consider opportunities to move any common code into the TrayBackgroundView class. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.h:151: // Overridden from ui::EventHandler: nit: just "ui::EventHandler:" https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.h:236: // Swiping of the system tray bubble is only for maximize mode. Can you clarify why you want to restrict this functionality to just maximize mode? For consistency I would expect the same behavior to work when I am in clamshell (non-maximized) mode with a touchscreen device. Removing the restriction would also simplify your CL :) https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... File ash/system/tray/system_tray_bubble.cc (right): https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray_bubble.cc:72: // animation completes. When I build and try out this CL locally, I run into a crash when I do the following: (1) Start swiping up on the system tray to partially expose the system menu (keep finger on the screen) (2) With a second finger, tap elsewhere (e.g., on the desktop, the overview mode button, etc) https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray_bubble.cc:108: if (clipping_window_) Have you considered making |clipping_window_| a unique_ptr to avoid having to manually delete here? https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:102: TEST_F(SystemTrayTest, SwipingOnSystemTray) { Can you also add a test to verify the correct behavior on fling? https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:120: SendGestureEvent(start, delta); Consider adding a test case where |delta| is past the one-third threshhold but is not the full height of the system menu.
https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.cc:679: if (event.type() == ui::ET_GESTURE_SCROLL_BEGIN) I think "switch case" might make the code cleaner and easier to follow. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.h:50: public ShellObserver { You don't need observe Shell to get the maximize mode state. It can always calculated by Shell::Get()->maximize_mode_controller()->IsMaximizeModeWindowManagerEnabled() https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.h:240: gfx::Rect system_tray_bubble_bounds_ = gfx::Rect(); nit: I don't think you need this initialization.
The CQ bit was checked by minch@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.cc:651: void SystemTray::OnGestureEvent(ui::GestureEvent* event) { On 2017/06/19 16:19:38, tdanderson wrote: > Why are you restricting this to just a horizontally-aligned shelf? Since tablet will has horizontal shelf only in the future. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.cc:652: if (in_maximize_mode_ && shelf_->IsHorizontalAlignment() && On 2017/06/19 16:19:38, tdanderson wrote: > Side question to ask UX (OK to handle in a follow-on CL): since we're letting > the user open the system menu with a swipe-up, should we also be letting them > close the system menu with a swipe-down on a menu that is already open? Yes, that's right. We definitely need that, I will do that in next CL. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.cc:656: CustomButton::OnGestureEvent(event); On 2017/06/19 16:19:38, tdanderson wrote: > I think you'd instead want to call up to TrayBackrgroundView::OnGestureEvent() > since SystemTray inherits directly from TrayBackgroundView. > > If you were to call up to CustomButton::OnGestureEvent() and at some point in > the future someone added a TrayBackgroundView::OnGestureEvent() or > ActionableView::OnGestureEvent() override, these overrides would be skipped when > a gesture targeted SystemTray. Done. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.cc:678: bool SystemTray::ProcessGestureEvent(const ui::GestureEvent& event) { On 2017/06/19 16:19:38, tdanderson wrote: > When building and trying out this CL locally, I can get a crash when I do the > following when the system menu is closed: without your finger leaving the > screen, very quickly swipe up on the system menu, then down, then up, then down > again. > > One possible cause for this crash could be that you are not handling all > relevant possibilities for gesture events. For instance you may be receiving > ET_SCROLL_FLING_CANCEL, ET_GESTURE_SWIPE, ET_GESTURE_END, etc. I suggest adding > logging to this function to see which event types and sequences can be seen. I am sorry I cannot repro this locally. But I guess this maybe the same reason as another crash following (one finger tap the bubble then another finger tap elsewhere). Quickly swiping maybe touch the desktop by accident. All the other gesture events will return false here directly. Let me know if you can still repro the crash after my change. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.cc:679: if (event.type() == ui::ET_GESTURE_SCROLL_BEGIN) On 2017/06/19 19:01:30, xdai1 wrote: > I think "switch case" might make the code cleaner and easier to follow. I think "switch case" is not so good after update. Please check. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.cc:697: IsDraggingDownOnShelf(gesture)) { On 2017/06/19 16:19:38, tdanderson wrote: > I don't understand what the check IsDraggingDownOnShelf() does and why you need > it. Can you please explain? When there is no bubble. Swiping down on the system tray should actor the same as if swiping down on the other positions on the shelf to hide the shelf. If I didn't check this, swiping down on the system tray at the beginning will not hide the shelf. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.cc:765: if (fabs(gesture.details().velocity_y()) > kSwipingVelocity) On 2017/06/19 16:19:38, tdanderson wrote: > Consider setting kSwipingVelocity to -100 and inlining > ShouldShowSystemBubbleBasedOnDragAmount(). Then you could simplify this function > to just: > > const float kSwipingVelocity = -100.0f; > if (gesture.type() == FLING_START && > gesture.details.velocity_y() < kSwipingVelocity()) { > return true; > } > > return -gesture_drag_amount_ >= system_tray_bubble_bounds_.height() / 3.0; Thanks. Done the change for ShouldShowSystemBubbleBasedOnDragAmount. But keep line 764-766. Since should return false (close the bubble) if velocity > 100.0f. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.h:10: #include <vector> On 2017/06/19 16:19:38, tdanderson wrote: > I see you are using bug 725977 to track many different changes. As a suggestion, > consider splitting each surface into its own bug, and mark these as blocking the > 725977 tracking bug. In my opinion there are many advantages to doing this: > > * It makes it easier to track what has already been done and what is left to do. > * If the feature work will span across different milestones, it's easier to tell > which parts were landed in which milestones since each bug will have its own M- > label. > * It is easier to track regressions, in case any individual CL needs to be > reverted. > * When a bug is used to track only a single piece of feature work, it makes it > simpler for the test team to perform manual verification. Thanks for your suggestion Terry. I will split this issue into multiple small ones. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.h:48: class ASH_EXPORT SystemTray : public TrayBackgroundView, On 2017/06/19 16:19:38, tdanderson wrote: > Note that I have left a comment here listing the other overrides of > TrayBackgroundView: > https://bugs.chromium.org/p/chromium/issues/detail?id=725977#c4 > > It looks like you are planning to add "swipe up" functionality to many of these. > I'm OK with this as the first CL, but in a subsequent CLs consider opportunities > to move any common code into the TrayBackgroundView class. Yes, I think it will has some code reuse in the following CLs after this feature applied to some other parts. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.h:50: public ShellObserver { On 2017/06/19 19:01:30, xdai1 wrote: > You don't need observe Shell to get the maximize mode state. It can always > calculated by > Shell::Get()->maximize_mode_controller()->IsMaximizeModeWindowManagerEnabled() Done. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.h:151: // Overridden from ui::EventHandler: On 2017/06/19 16:19:38, tdanderson wrote: > nit: just "ui::EventHandler:" Done. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.h:236: // Swiping of the system tray bubble is only for maximize mode. On 2017/06/19 16:19:38, tdanderson wrote: > Can you clarify why you want to restrict this functionality to just maximize > mode? For consistency I would expect the same behavior to work when I am in > clamshell (non-maximized) mode with a touchscreen device. Removing the > restriction would also simplify your CL :) I think the touch gestures here is only for tablet and horizontal shelf. Since tablet will has horizontal shelf only in the future. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.h:240: gfx::Rect system_tray_bubble_bounds_ = gfx::Rect(); On 2017/06/19 19:01:30, xdai1 wrote: > nit: I don't think you need this initialization. Done. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... File ash/system/tray/system_tray_bubble.cc (right): https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray_bubble.cc:72: // animation completes. On 2017/06/19 16:19:38, tdanderson wrote: > When I build and try out this CL locally, I run into a crash when I do the > following: > > (1) Start swiping up on the system tray to partially expose the system menu > (keep finger on the screen) > (2) With a second finger, tap elsewhere (e.g., on the desktop, the overview mode > button, etc) This is because tap elsewhere will dismiss the menu, then |system_bubble_| is nullptr in this situation. I will check it first before update the bounds. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray_bubble.cc:108: if (clipping_window_) On 2017/06/19 16:19:38, tdanderson wrote: > Have you considered making |clipping_window_| a unique_ptr to avoid having to > manually delete here? Done. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:102: TEST_F(SystemTrayTest, SwipingOnSystemTray) { On 2017/06/19 16:19:38, tdanderson wrote: > Can you also add a test to verify the correct behavior on fling? Done. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:120: SendGestureEvent(start, delta); On 2017/06/19 16:19:39, tdanderson wrote: > Consider adding a test case where |delta| is past the one-third threshhold but > is not the full height of the system menu. Done. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:19: #include "ash/system/tray/system_tray_test_api.h" Will remove this in next PS.
https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.cc:671: return false; Why don't put this if-clause at the top?
lgtm Thanks. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.cc:671: return false; On 2017/06/20 17:00:31, xdai1 wrote: > Why don't put this if-clause at the top? I think I get it...
Hi Min, thanks for addressing one of the crashers and for adding extra tests! Please see my latest comments: https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.cc:651: void SystemTray::OnGestureEvent(ui::GestureEvent* event) { On 2017/06/20 16:45:34, minch1 wrote: > On 2017/06/19 16:19:38, tdanderson wrote: > > Why are you restricting this to just a horizontally-aligned shelf? > > Since tablet will has horizontal shelf only in the future. Acknowledged. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.cc:652: if (in_maximize_mode_ && shelf_->IsHorizontalAlignment() && On 2017/06/20 16:45:34, minch1 wrote: > On 2017/06/19 16:19:38, tdanderson wrote: > > Side question to ask UX (OK to handle in a follow-on CL): since we're letting > > the user open the system menu with a swipe-up, should we also be letting them > > close the system menu with a swipe-down on a menu that is already open? > > Yes, that's right. We definitely need that, I will do that in next CL. Acknowledged. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.cc:678: bool SystemTray::ProcessGestureEvent(const ui::GestureEvent& event) { On 2017/06/20 16:45:34, minch1 wrote: > On 2017/06/19 16:19:38, tdanderson wrote: > > When building and trying out this CL locally, I can get a crash when I do the > > following when the system menu is closed: without your finger leaving the > > screen, very quickly swipe up on the system menu, then down, then up, then > down > > again. > > > > One possible cause for this crash could be that you are not handling all > > relevant possibilities for gesture events. For instance you may be receiving > > ET_SCROLL_FLING_CANCEL, ET_GESTURE_SWIPE, ET_GESTURE_END, etc. I suggest > adding > > logging to this function to see which event types and sequences can be seen. > > I am sorry I cannot repro this locally. But I guess this maybe the same reason > as another crash following (one finger tap the bubble then another finger tap > elsewhere). Quickly swiping maybe touch the desktop by accident. All the other > gesture events will return false here directly. > Let me know if you can still repro the crash after my change. I can still repro locally with Patch Set 8. I will share/send you a video of what I'm seeing. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.cc:765: if (fabs(gesture.details().velocity_y()) > kSwipingVelocity) On 2017/06/20 16:45:34, minch1 wrote: > On 2017/06/19 16:19:38, tdanderson wrote: > > Consider setting kSwipingVelocity to -100 and inlining > > ShouldShowSystemBubbleBasedOnDragAmount(). Then you could simplify this > function > > to just: > > > > const float kSwipingVelocity = -100.0f; > > if (gesture.type() == FLING_START && > > gesture.details.velocity_y() < kSwipingVelocity()) { > > return true; > > } > > > > return -gesture_drag_amount_ >= system_tray_bubble_bounds_.height() / 3.0; > > Thanks. Done the change for ShouldShowSystemBubbleBasedOnDragAmount. But keep > line 764-766. Since should return false (close the bubble) if velocity > 100.0f. Acknowledged. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.h:10: #include <vector> On 2017/06/20 16:45:34, minch1 wrote: > On 2017/06/19 16:19:38, tdanderson wrote: > > I see you are using bug 725977 to track many different changes. As a > suggestion, > > consider splitting each surface into its own bug, and mark these as blocking > the > > 725977 tracking bug. In my opinion there are many advantages to doing this: > > > > * It makes it easier to track what has already been done and what is left to > do. > > * If the feature work will span across different milestones, it's easier to > tell > > which parts were landed in which milestones since each bug will have its own > M- > > label. > > * It is easier to track regressions, in case any individual CL needs to be > > reverted. > > * When a bug is used to track only a single piece of feature work, it makes it > > simpler for the test team to perform manual verification. > > Thanks for your suggestion Terry. I will split this issue into > multiple small ones. Acknowledged. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.h:48: class ASH_EXPORT SystemTray : public TrayBackgroundView, On 2017/06/20 16:45:34, minch1 wrote: > On 2017/06/19 16:19:38, tdanderson wrote: > > Note that I have left a comment here listing the other overrides of > > TrayBackgroundView: > > https://bugs.chromium.org/p/chromium/issues/detail?id=725977#c4 > > > > It looks like you are planning to add "swipe up" functionality to many of > these. > > I'm OK with this as the first CL, but in a subsequent CLs consider > opportunities > > to move any common code into the TrayBackgroundView class. > > Yes, I think it will has some code reuse in the following CLs after this feature > applied to some other parts. Acknowledged. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.h:236: // Swiping of the system tray bubble is only for maximize mode. On 2017/06/20 16:45:34, minch1 wrote: > On 2017/06/19 16:19:38, tdanderson wrote: > > Can you clarify why you want to restrict this functionality to just maximize > > mode? For consistency I would expect the same behavior to work when I am in > > clamshell (non-maximized) mode with a touchscreen device. Removing the > > restriction would also simplify your CL :) > > I think the touch gestures here is only for tablet and horizontal shelf. Since > tablet will has horizontal shelf only in the future. OK as discussed in the other email thread, in a follow-on CL please add user actions metrics to track how often the swipe-up feature is used. https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... File ash/system/tray/system_tray_bubble.cc (right): https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray_bubble.cc:72: // animation completes. On 2017/06/20 16:45:34, minch1 wrote: > On 2017/06/19 16:19:38, tdanderson wrote: > > When I build and try out this CL locally, I run into a crash when I do the > > following: > > > > (1) Start swiping up on the system tray to partially expose the system menu > > (keep finger on the screen) > > (2) With a second finger, tap elsewhere (e.g., on the desktop, the overview > mode > > button, etc) > > This is because tap elsewhere will dismiss the menu, then |system_bubble_| is > nullptr in this situation. I will check it first before update the bounds. Thanks, I do not see this crash locally with Patch Set 8. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.cc:642: event->StopPropagation(); I think that event->SetHandled() should do the trick here, is there a reason why you need to use StopPropagation() specifically? https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.cc:688: IsDraggingDownOnShelf(gesture)) { Now I understand what IsDraggingDownOnShelf() is doing. Since that function is only called in one place and its implementation is very short (just a few lines), for readability I suggest just inlining the implementation of StartGestureDrag() as follows: bool SystemTray::StartGestureDrag(...) { DCHECK_EQ(gesture.type(), ui::ET_GESTURE_SCROLL_BEGIN); // <add some comment here> if (HasSystemBubble() && full_system_tray_menu_) return false; // If the scroll sequence begins to scroll downward, return false // so that the event will instead by handled by the shelf. gfx::Point gesture_location = ...; View::ConvertPointToScreen(...); if (gesture_location.y() >= shelf_->GetIdealBounds().y() && gesture.details().scroll_y_hint() > 0) { return false; } gesture_drag_amount_ = 0.f; ... return true; } NOTE: I think it should be sufficient to just check that gesture.details().scroll_y_hint() > 0, which checks that scrolling has started in the downward direction. I don't think you need to compare against the shelf location. Can you please verify this? If so, then your code will be simpler since you don't need to store the |shelf_| member variable or convert the event location to a different coordinate space. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.cc:692: gesture_drag_amount_ = 0.f; It looks like you are only resetting |gesture_drag_amount_| once a new scroll-sequence begins, which means that this member could be storing a meaningless value whenever a scroll is not in progress. This is potentially dangerous, as someone could query this value and infer that a drag was in progress when it wasn't. For this CL, I suggest that you also reset this member to 0 in CompleteGestureDrag() as well as right above line 683. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.cc:706: gfx::Rect target_bounds = system_tray_bubble_bounds_; Consider simplifying the implementation of this function as follows: const bool hide_bubble = !ShouldShowSystemBubbleForDragging(gesture); gfx::Rect target_bounds = system_tray_bubble_bounds_; if (hide_bubble) target_bounds.set_y(shelf_->GetIdealBounds().y()); system_bubble_->bubble()->UpdateBounds(target_bounds, hide_bubble); https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.cc:719: if (gesture_location.y() >= shelf_->GetIdealBounds().y() && In StartGestureDrag() I suggest just inlining this implementation. However if you do keep this as a separate named helper function, lines 719-723 can just be replaced with: return gesture_location.y() >= shelf_->GetIdealBounds().y() && gesture.details().scroll_y_hint() > 0; https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.cc:726: void SystemTray::UpdateBoundsOnDragging(const ui::GestureEvent& gesture) { Since the only property of |gesture| you use is its location, I suggest just passing in the gfx::Point of the location instead of the whole GestureEvent. Then you can re-name to something such as SetBubbleBounds() or similar. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.cc:743: if (gesture.type() == ui::ET_SCROLL_FLING_START && Suggestion for inline documentation here: "If the scroll sequence terminates with a fling, show the system menu if the fling was fast enough and in the correct direction." https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.cc:747: Consider adding a DCHECK here to verify that the gesture type is a GESTURE_SCROLL_END. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.cc:748: return -gesture_drag_amount_ >= system_tray_bubble_bounds_.height() / 3.0; Suggestion for inline documentation here: "Show the system menu if it is already at least one-third visible." https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.h:220: bool ShouldShowSystemBubbleForDragging(const ui::GestureEvent& gesture); With the current implementation, once the system menu is already open, you cannot close it by swiping (you said you would address that in a future CL). So as-is, I find it a bit misleading to use the term "close" here. How about the following suggestion for documentation and a function name which I think makes this a bit clearer: // Returns true if the system bubble should be shown (i.e., // animated upward to be made fully visible) after a // sequence of scroll events terminated by |sequence_end|. Otherwise // return false, indicating that the partially-visible system bubble // should be animated downward and made fully hidden. bool ShouldShowSystemBubbleAfterScrollSequence(const ui::GestureEvent& sequence_end); I have made suggestions for inline documentation in the .cc. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... File ash/system/tray/system_tray_bubble.cc (right): https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray_bubble.cc:341: void SystemTrayBubble::UpdateBounds(const gfx::Rect& target_bounds, For clarity, how about naming this AnimateToTargetBounds() (or similar)? https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:19: #include "ash/system/tray/system_tray_test_api.h" On 2017/06/20 16:45:35, minch1 wrote: > Will remove this in next PS. Acknowledged. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:66: void SendGestureEvent(gfx::Point& start, nit: can you please add brief documentation for each of the helper functions you are introducing here (above lines 66, 88, 100). https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:74: if (is_fling) { To simplify this, you could replace lines 74-85 with just the three lines: ui::GestureEventDetails details = is_fling ? ... : ...; ui::GestureEvent event = ui::GestureEvent(..., details); system_tray->OnGestureEvent(&event); https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:88: float GetSystemBubbleHeight() { Does this function always return the same value? You call this many times in your tests, and it seems like a lot of unnecessary overhead to show the view, capture the bounds, and then close the view every time. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:121: TEST_F(SystemTrayTest, FlingOnSystemTray) { nit: can you please add brief documentation above each of the tests you are adding so that readers can get a general sense of what the test does without needing to look at its implementation. See for instance what is done on line 266. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:126: float kFlingVelocity = 100.0f; I think it would be a good idea to define this constant in a single place (system_tray.h/cc is appropriate) and then reference it from both SystemTray::ShouldShowSystemBubbleForDragging() and from this line. That way if the constant is ever changed, your test gets updated for free. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:188: TEST_F(SystemTrayTest, TapOutsideCloseBubble) { Thanks for adding this test after fixing the crash! Very helpful in guarding against regressions.
Description was changed from ========== Tablet WM : Swiping on system tray bubble. Swiping up on the system tray should open the system tray bubble. 1.This works only for tablet and horizontal shelf. 2.Update the bounds of the bubble according to the position of the gesture. 3.For SCROLL_END event. To show or hide the bubble depends on the position of the gesture. Bubble is split into two parts. End in the lower part (one-third position) will hide the bubble; Otherwise, should always show the bubble. 4.For FLING event. To show or hide the bubble depends on gesture position and velocity. If the fling velocity larger than |kSwipingVelocity|, show or hide according to the fling direction; Otherwise, show or hide depends on the position. BUG=725977 ========== to ========== Tablet WM : Swiping on system tray bubble. Swiping up on the system tray should open the system tray bubble. 1.This works only for tablet and horizontal shelf. 2.Update the bounds of the bubble according to the position of the gesture. 3.For SCROLL_END event. To show or hide the bubble depends on the position of the gesture. Bubble is split into two parts. End in the lower part (one-third position) will hide the bubble; Otherwise, should always show the bubble. 4.For FLING event. To show or hide the bubble depends on gesture position and velocity. If the fling velocity larger than |kSwipingVelocity|, show or hide according to the fling direction; Otherwise, show or hide depends on the position. BUG=734814 ==========
The CQ bit was checked by minch@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
minch@chromium.org changed reviewers: + jwd@chromium.org
https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2930123002/diff/120001/ash/system/tray/system... ash/system/tray/system_tray.h:236: // Swiping of the system tray bubble is only for maximize mode. On 2017/06/20 22:42:20, tdanderson wrote: > On 2017/06/20 16:45:34, minch1 wrote: > > On 2017/06/19 16:19:38, tdanderson wrote: > > > Can you clarify why you want to restrict this functionality to just maximize > > > mode? For consistency I would expect the same behavior to work when I am in > > > clamshell (non-maximized) mode with a touchscreen device. Removing the > > > restriction would also simplify your CL :) > > > > I think the touch gestures here is only for tablet and horizontal shelf. Since > > tablet will has horizontal shelf only in the future. > > OK as discussed in the other email thread, in a follow-on CL please add user > actions metrics to track how often the swipe-up feature is used. Done. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.cc:642: event->StopPropagation(); On 2017/06/20 22:42:27, tdanderson wrote: > I think that event->SetHandled() should do the trick here, is there a reason why > you need to use StopPropagation() specifically? Done. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.cc:688: IsDraggingDownOnShelf(gesture)) { On 2017/06/20 22:42:24, tdanderson wrote: > Now I understand what IsDraggingDownOnShelf() is doing. Since that function is > only called in one place and its implementation is very short (just a few > lines), for readability I suggest just inlining the implementation of > StartGestureDrag() as follows: > > bool SystemTray::StartGestureDrag(...) { > DCHECK_EQ(gesture.type(), ui::ET_GESTURE_SCROLL_BEGIN); > > // <add some comment here> > if (HasSystemBubble() && full_system_tray_menu_) > return false; > > // If the scroll sequence begins to scroll downward, return false > // so that the event will instead by handled by the shelf. > gfx::Point gesture_location = ...; > View::ConvertPointToScreen(...); > if (gesture_location.y() >= shelf_->GetIdealBounds().y() && > gesture.details().scroll_y_hint() > 0) { > return false; > } > > gesture_drag_amount_ = 0.f; > ... > return true; > } > > > NOTE: I think it should be sufficient to just check that > gesture.details().scroll_y_hint() > 0, which checks that scrolling has started > in the downward direction. I don't think you need to compare against the shelf > location. Can you please verify this? If so, then your code will be simpler > since you don't need to store the |shelf_| member variable or convert the event > location to a different coordinate space. Yes, you are right, thanks. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.cc:692: gesture_drag_amount_ = 0.f; On 2017/06/20 22:42:23, tdanderson wrote: > It looks like you are only resetting |gesture_drag_amount_| once a new > scroll-sequence begins, which means that this member could be storing a > meaningless value whenever a scroll is not in progress. This is potentially > dangerous, as someone could query this value and infer that a drag was in > progress when it wasn't. > > For this CL, I suggest that you also reset this member to 0 in > CompleteGestureDrag() as well as right above line 683. Done. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.cc:706: gfx::Rect target_bounds = system_tray_bubble_bounds_; On 2017/06/20 22:42:25, tdanderson wrote: > Consider simplifying the implementation of this function as follows: > > const bool hide_bubble = !ShouldShowSystemBubbleForDragging(gesture); > gfx::Rect target_bounds = system_tray_bubble_bounds_; > > if (hide_bubble) > target_bounds.set_y(shelf_->GetIdealBounds().y()); > > system_bubble_->bubble()->UpdateBounds(target_bounds, hide_bubble); > Done. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.cc:719: if (gesture_location.y() >= shelf_->GetIdealBounds().y() && On 2017/06/20 22:42:23, tdanderson wrote: > In StartGestureDrag() I suggest just inlining this implementation. However if > you do keep this as a separate named helper function, lines 719-723 can just be > replaced with: > > return gesture_location.y() >= shelf_->GetIdealBounds().y() && > gesture.details().scroll_y_hint() > 0; Acknowledged. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.cc:726: void SystemTray::UpdateBoundsOnDragging(const ui::GestureEvent& gesture) { On 2017/06/20 22:42:26, tdanderson wrote: > Since the only property of |gesture| you use is its location, I suggest just > passing in the gfx::Point of the location instead of the whole GestureEvent. > Then you can re-name to something such as SetBubbleBounds() or similar. Done. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.cc:743: if (gesture.type() == ui::ET_SCROLL_FLING_START && On 2017/06/20 22:42:20, tdanderson wrote: > Suggestion for inline documentation here: "If the scroll sequence terminates > with a fling, show the system menu if the fling was fast enough and in the > correct direction." Done. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.cc:747: On 2017/06/20 22:42:20, tdanderson wrote: > Consider adding a DCHECK here to verify that the gesture type is a > GESTURE_SCROLL_END. It is not correct to add a DCHECK to verify that the gesture type is GESTURE_SCROLL_END here. Since gesture type here can be both GESTURE_SCROLL_END and SCROLL_FLING_START. If the velocity of the fling event is not large enough it should consider the position of the gesture. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.cc:748: return -gesture_drag_amount_ >= system_tray_bubble_bounds_.height() / 3.0; On 2017/06/20 22:42:21, tdanderson wrote: > Suggestion for inline documentation here: "Show the system menu if it is already > at least one-third visible." Done. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.h:220: bool ShouldShowSystemBubbleForDragging(const ui::GestureEvent& gesture); On 2017/06/20 22:42:28, tdanderson wrote: > With the current implementation, once the system menu is already open, you > cannot close it by swiping (you said you would address that in a future CL). So > as-is, I find it a bit misleading to use the term "close" here. How about the > following suggestion for documentation and a function name which I think makes > this a bit clearer: > > // Returns true if the system bubble should be shown (i.e., > // animated upward to be made fully visible) after a > // sequence of scroll events terminated by |sequence_end|. Otherwise > // return false, indicating that the partially-visible system bubble > // should be animated downward and made fully hidden. > bool ShouldShowSystemBubbleAfterScrollSequence(const ui::GestureEvent& > sequence_end); > > I have made suggestions for inline documentation in the .cc. Done. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... File ash/system/tray/system_tray_bubble.cc (right): https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray_bubble.cc:341: void SystemTrayBubble::UpdateBounds(const gfx::Rect& target_bounds, On 2017/06/20 22:42:29, tdanderson wrote: > For clarity, how about naming this AnimateToTargetBounds() (or similar)? Done. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:66: void SendGestureEvent(gfx::Point& start, On 2017/06/20 22:42:31, tdanderson wrote: > nit: can you please add brief documentation for each of the helper functions you > are introducing here (above lines 66, 88, 100). Done. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:74: if (is_fling) { On 2017/06/20 22:42:31, tdanderson wrote: > To simplify this, you could replace lines 74-85 with just the three lines: > > ui::GestureEventDetails details = is_fling ? ... : ...; > ui::GestureEvent event = ui::GestureEvent(..., details); > system_tray->OnGestureEvent(&event); Done. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:88: float GetSystemBubbleHeight() { On 2017/06/20 22:42:29, tdanderson wrote: > Does this function always return the same value? You call this many times in > your tests, and it seems like a lot of unnecessary overhead to show the view, > capture the bounds, and then close the view every time. It is not always the same value when you open the bubble. So I think it is necessary to get the height of the bubble for each action. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:121: TEST_F(SystemTrayTest, FlingOnSystemTray) { On 2017/06/20 22:42:31, tdanderson wrote: > nit: can you please add brief documentation above each of the tests you are > adding so that readers can get a general sense of what the test does without > needing to look at its implementation. See for instance what is done on line > 266. Done. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:126: float kFlingVelocity = 100.0f; On 2017/06/20 22:42:30, tdanderson wrote: > I think it would be a good idea to define this constant in a single place > (system_tray.h/cc is appropriate) and then reference it from both > SystemTray::ShouldShowSystemBubbleForDragging() and from this line. That way if > the constant is ever changed, your test gets updated for free. Done.
lgtm
Hi Min, please take a look at my comments. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.cc:747: On 2017/06/22 17:42:11, minch1 wrote: > On 2017/06/20 22:42:20, tdanderson wrote: > > Consider adding a DCHECK here to verify that the gesture type is a > > GESTURE_SCROLL_END. > > It is not correct to add a DCHECK to verify that the gesture type is > GESTURE_SCROLL_END here. Since gesture type here can be both GESTURE_SCROLL_END > and SCROLL_FLING_START. If the velocity of the fling event is not large enough > it should consider the position of the gesture. Ah yes, you're right. Then perhaps add a DCHECK to verify (type == SCROLL_END || type == SCROLL_FLING_START). https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:88: float GetSystemBubbleHeight() { On 2017/06/22 17:42:11, minch1 wrote: > On 2017/06/20 22:42:29, tdanderson wrote: > > Does this function always return the same value? You call this many times in > > your tests, and it seems like a lot of unnecessary overhead to show the view, > > capture the bounds, and then close the view every time. > > It is not always the same value when you open the bubble. So I think it is > necessary to get the height of the bubble for each action. Acknowledged. https://codereview.chromium.org/2930123002/diff/160001/ash/system/tray/system... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2930123002/diff/160001/ash/system/tray/system... ash/system/tray/system_tray.cc:683: gesture_drag_amount_ = 0.f; You should also reset this member upon completing a gesture drag, I believe (so, right after line 680). After thinking about this some more, I think the 'safer' thing to do would be to have a |is_in_drag_| member or similar. But we can come back to that idea in a future CL since you will presumably be refactoring this code in the near future anyway. In the meanwhile can you please add a "// TODO(minch|tdanderson): Consider adding a boolean member to track whether or not a drag is in progress" https://codereview.chromium.org/2930123002/diff/160001/ash/system/tray/system... ash/system/tray/system_tray.cc:690: if ((HasSystemBubble() && full_system_tray_menu_)) { nit: unnecessary extra () https://codereview.chromium.org/2930123002/diff/160001/ash/system/tray/system... ash/system/tray/system_tray.cc:691: system_bubble_->bubble()->Close(); I tried this patch set locally, and as far as I can tell adding just this line is sufficient to fix the crash. I don't think you need to make any changes to tray_event_filter.cc. Can you please verify? https://codereview.chromium.org/2930123002/diff/160001/ash/system/tray/system... ash/system/tray/system_tray.cc:729: void SystemTray::SetBubbleBounds(gfx::Point* location) { There doesn't seem to be any need for this parameter to be a pointer type, since the call sites never access the mutated point after SetBubbleBounds() terminates. I suggest instead: Call sites just look like: SetBubbleBounds(gesture.location()); Then: void SystemTray::SetBubbleBounds(const gfx::Point& location) { gfx::Point* location_in_screen_coordinates(location); ConvertPointToScreen(this, &location_in_screen_coordinates); ... } https://codereview.chromium.org/2930123002/diff/160001/ash/system/tray/system... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2930123002/diff/160001/ash/system/tray/system... ash/system/tray/system_tray.h:42: const float kFlingVelocity = 100.0f; as-is, this constant is being exposed to the entire ash namespace. Instead consider placing inside the SystemTray class. Have a look at 'static const int kDesktopBorderSize' in ash/wm/window_positioner.h as an example. https://codereview.chromium.org/2930123002/diff/160001/ash/system/tray/system... ash/system/tray/system_tray.h:210: // Update the bounds of the system tray bubble according to the location. nit: also specify that |location| is in the local coordinate space of |this|. https://codereview.chromium.org/2930123002/diff/160001/tools/metrics/actions/... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2930123002/diff/160001/tools/metrics/actions/... tools/metrics/actions/actions.xml:16152: close the system tray bubble. This description is not entirely accurate since the metric is only recording instances where the user has swiped up on the tray in an attempt to open the bubble. My suggestions here would be to: * Re-name UMA_STATUS_AREA_SYSTEM_TRAY_SWIPING to UMA_STATUS_AREA_SYSTEM_TRAY_SWIPE_TO_OPEN or similar * Re-name StatusArea_SystemTray_Swiping to StatusArea_SystemTray_SwipeToOpen or similar * Re-word the description as "Records when the user swipes up on the system tray in an attempt to open the system tray bubble. Note this is recorded even if the bubble is not opened, i.e., if the swipe was too short." [1] In a follow-on CL you can add a separate metric StatusArea_SystemTray_SwipeToClose. [1] If you wanted to be super-precise, you could count two things separately: instances where the user started to swipe up but the menu didn't open, and instances where the user started to swipe up and the menu opened. I think it is ok as-is, but perhaps run this by a PM to see exactly what metrics they care about.
The CQ bit was checked by minch@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 minch@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi, tdanderson@, please help review patch 11. Thanks. https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2930123002/diff/140001/ash/system/tray/system... ash/system/tray/system_tray.cc:747: On 2017/06/22 20:21:42, tdanderson wrote: > On 2017/06/22 17:42:11, minch1 wrote: > > On 2017/06/20 22:42:20, tdanderson wrote: > > > Consider adding a DCHECK here to verify that the gesture type is a > > > GESTURE_SCROLL_END. > > > > It is not correct to add a DCHECK to verify that the gesture type is > > GESTURE_SCROLL_END here. Since gesture type here can be both > GESTURE_SCROLL_END > > and SCROLL_FLING_START. If the velocity of the fling event is not large enough > > it should consider the position of the gesture. > > Ah yes, you're right. Then perhaps add a DCHECK to verify (type == SCROLL_END || > type == SCROLL_FLING_START). Done. https://codereview.chromium.org/2930123002/diff/160001/ash/system/tray/system... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2930123002/diff/160001/ash/system/tray/system... ash/system/tray/system_tray.cc:683: gesture_drag_amount_ = 0.f; On 2017/06/22 20:21:42, tdanderson wrote: > You should also reset this member upon completing a gesture drag, I believe (so, > right after line 680). > > After thinking about this some more, I think the 'safer' thing to do would be to > have a |is_in_drag_| member or similar. But we can come back to that idea in a > future CL since you will presumably be refactoring this code in the near future > anyway. In the meanwhile can you please add a "// TODO(minch|tdanderson): > Consider adding a boolean member to track whether or not a drag is in progress" I am not sure following cl will change this or not. Let me do it in this cl. Actually, I found it is not correct to set get_drag_amount_= 0.f here. Since if there is an event happened between SCROLL_UPDATE and SCROLL_END / FLING_START (this is possible, eg.ET_GESTURE_DRAG). It will reset this value which will lead to wrong completion of the dragging. I think it is enough to reset is_in_drag_ in CompleteGestureDrag. Since there is always SCROLL_END / FLING_START for a SCROLL_START. https://codereview.chromium.org/2930123002/diff/160001/ash/system/tray/system... ash/system/tray/system_tray.cc:690: if ((HasSystemBubble() && full_system_tray_menu_)) { On 2017/06/22 20:21:42, tdanderson wrote: > nit: unnecessary extra () Done. https://codereview.chromium.org/2930123002/diff/160001/ash/system/tray/system... ash/system/tray/system_tray.cc:691: system_bubble_->bubble()->Close(); On 2017/06/22 20:21:42, tdanderson wrote: > I tried this patch set locally, and as far as I can tell adding just this line > is sufficient to fix the crash. I don't think you need to make any changes to > tray_event_filter.cc. Can you please verify? I think that should because you only swiping on the system tray area. If you swiping on other overlap area of system tray bubble and shelf during animation, it will cause crash. If you want to repro this, I think you can try to 1.set |kAnimationDurationMS| longer (eg, 2000). 2.swiping on the system tray ends with aiming close the bubble, eg, ends with fling down with enough velocity. 3.before animation finish, swiping on the overlap area of the bubble and shelf. Crash happened. Another case is that if you keep dragging the bubble (not release the finger), and then swiping / touch the overlap area of the bubble and shelf with another finger at the same time, you will find that this will not dismiss the bubble. This is not consistent when you touch elsewhere (eg.desktop) in the same situation. I think it should close the bubble in this situation. So I changed some logic in tray_event_filter.cc I also added a UT for this, please check. https://codereview.chromium.org/2930123002/diff/160001/ash/system/tray/system... ash/system/tray/system_tray.cc:729: void SystemTray::SetBubbleBounds(gfx::Point* location) { On 2017/06/22 20:21:42, tdanderson wrote: > There doesn't seem to be any need for this parameter to be a pointer type, since > the call sites never access the mutated point after SetBubbleBounds() > terminates. I suggest instead: > > Call sites just look like: > SetBubbleBounds(gesture.location()); > > Then: > void SystemTray::SetBubbleBounds(const gfx::Point& location) { > gfx::Point* location_in_screen_coordinates(location); > ConvertPointToScreen(this, &location_in_screen_coordinates); > ... > } Done. https://codereview.chromium.org/2930123002/diff/160001/ash/system/tray/system... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2930123002/diff/160001/ash/system/tray/system... ash/system/tray/system_tray.h:42: const float kFlingVelocity = 100.0f; On 2017/06/22 20:21:43, tdanderson wrote: > as-is, this constant is being exposed to the entire ash namespace. Instead > consider placing inside the SystemTray class. Have a look at 'static const int > kDesktopBorderSize' in ash/wm/window_positioner.h as an example. Done. https://codereview.chromium.org/2930123002/diff/160001/ash/system/tray/system... ash/system/tray/system_tray.h:210: // Update the bounds of the system tray bubble according to the location. On 2017/06/22 20:21:42, tdanderson wrote: > nit: also specify that |location| is in the local coordinate space of |this|. Done. https://codereview.chromium.org/2930123002/diff/160001/tools/metrics/actions/... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2930123002/diff/160001/tools/metrics/actions/... tools/metrics/actions/actions.xml:16152: close the system tray bubble. On 2017/06/22 20:21:43, tdanderson wrote: > This description is not entirely accurate since the metric is only recording > instances where the user has swiped up on the tray in an attempt to open the > bubble. My suggestions here would be to: > > * Re-name UMA_STATUS_AREA_SYSTEM_TRAY_SWIPING to > UMA_STATUS_AREA_SYSTEM_TRAY_SWIPE_TO_OPEN or similar > > * Re-name StatusArea_SystemTray_Swiping to StatusArea_SystemTray_SwipeToOpen or > similar > > * Re-word the description as "Records when the user swipes up on the system tray > in an attempt to open the system tray bubble. Note this is recorded even if the > bubble is not opened, i.e., if the swipe was too short." [1] > > In a follow-on CL you can add a separate metric > StatusArea_SystemTray_SwipeToClose. > > [1] If you wanted to be super-precise, you could count two things separately: > instances where the user started to swipe up but the menu didn't open, and > instances where the user started to swipe up and the menu opened. I think it is > ok as-is, but perhaps run this by a PM to see exactly what metrics they care > about. OK. Let's keep it as-is first. Just waiting for the response from the PM.
Hi, tdanderson@, please help review patch 11. Thanks.
Patch set 11 is looking great! I have a few last questions/comments, and have also made the (optional) suggestion to revert the metrics code while we wait on the PMs - that way you can land this patch now and always add in the desired metrics in a follow-on. https://codereview.chromium.org/2930123002/diff/160001/ash/system/tray/system... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2930123002/diff/160001/ash/system/tray/system... ash/system/tray/system_tray.cc:683: gesture_drag_amount_ = 0.f; On 2017/06/23 22:39:56, minch1 wrote: > On 2017/06/22 20:21:42, tdanderson wrote: > > You should also reset this member upon completing a gesture drag, I believe > (so, > > right after line 680). > > > > After thinking about this some more, I think the 'safer' thing to do would be > to > > have a |is_in_drag_| member or similar. But we can come back to that idea in a > > future CL since you will presumably be refactoring this code in the near > future > > anyway. In the meanwhile can you please add a "// TODO(minch|tdanderson): > > Consider adding a boolean member to track whether or not a drag is in > progress" > > I am not sure following cl will change this or not. Let me do it in this cl. > Actually, I found it is not correct to set get_drag_amount_= 0.f here. Since if > there is an event happened between SCROLL_UPDATE and SCROLL_END / FLING_START > (this is possible, eg.ET_GESTURE_DRAG). It will reset this value which will lead > to wrong completion of the dragging. > I think it is enough to reset is_in_drag_ in CompleteGestureDrag. Since there is > always SCROLL_END / FLING_START for a SCROLL_START. Acknowledged. https://codereview.chromium.org/2930123002/diff/160001/ash/system/tray/system... ash/system/tray/system_tray.cc:691: system_bubble_->bubble()->Close(); On 2017/06/23 22:39:56, minch1 wrote: > On 2017/06/22 20:21:42, tdanderson wrote: > > I tried this patch set locally, and as far as I can tell adding just this line > > is sufficient to fix the crash. I don't think you need to make any changes to > > tray_event_filter.cc. Can you please verify? > > I think that should because you only swiping on the system tray area. If you > swiping on other overlap area of system tray bubble and shelf during animation, > it will cause crash. > If you want to repro this, I think you can try to > 1.set |kAnimationDurationMS| longer (eg, 2000). > 2.swiping on the system tray ends with aiming close the bubble, eg, ends with > fling down with enough velocity. > 3.before animation finish, swiping on the overlap area of the bubble and shelf. > Crash happened. > Another case is that if you keep dragging the bubble (not release the finger), > and then swiping / touch the overlap area of the bubble and shelf with another > finger at the same time, you will find that this will not dismiss the bubble. > This is not consistent when you touch elsewhere (eg.desktop) in the same > situation. > I think it should close the bubble in this situation. So I changed some logic in > tray_event_filter.cc > I also added a UT for this, please check. OK, this makes sense, thank you for the explanation and following through with this. https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/system... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/system... ash/system/tray/system_tray.cc:689: is_in_drag_ = true; Should this be placed just above line 703 (past the early returns)? If we hit any of the early returns then I don't think we should consider ourself as being in a drag, though I could be missing something here. https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/system... ash/system/tray/system_tray.cc:711: void SystemTray::UpdateGestureDrag(const ui::GestureEvent& gesture) { Consider adding DCHECK(is_in_drag_); https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/system... ash/system/tray/system_tray.cc:716: void SystemTray::CompleteGestureDrag(const ui::GestureEvent& gesture) { Consider adding DCHECK(is_in_drag_); https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/system... ash/system/tray/system_tray.cc:724: is_in_drag_ = false; What do you think of doing the following: In StartGestureDrag(): DCHECK_EQ(gesture_drag_amount_, 0.f); is_in_drag_ = true; Then in CompleteGestureDrag(): is_in_drag_ = false; gesture_drag_amount_ = 0.f; This will trigger the DCHECK in any cases where, for some reason, a previous drag didn't complete and you're trying to start a new one. Offhand I can't think of any specific case where this could happen, but I am not 100% certain it's impossible given the many different types of events we have and the possible combinations of event streams. (For instance, could you run into a ET_SCROLL_FLING_CANCEL instead of a ET_SCROLL_FLING_START?) https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/system... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/system... ash/system/tray/system_tray.h:216: // |sequence_end|. Otherwise return false, indicating that the nit: re-name the |gesture| parameter here to |sequence_end| to match the doc. https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/system... ash/system/tray/system_tray.h:232: // during gesture dragging. Suggestion for a reword here: "True if the user is in the process of gesture-dragging to open the system tray bubble, false otherwise." https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/system... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:126: TEST_F(SystemTrayTest, SwipingOnShelfDuringAnimation) { Awesome, thanks for adding this https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:156: gfx::Rect current_bounds = GetSystemBubbleBoundsInScreen(); nit: newlines after lines 156, 164, 168 (before commenting a block/line of code) https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/tray_e... File ash/system/tray/tray_event_filter.cc (right): https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/tray_e... ash/system/tray/tray_event_filter.cc:80: // happened outside the bubble. Suggestion to re-word the second sentence: "During the drag, the bubble's logical bounds can extend outside of the work area, but its visual bounds are only within the work area. Restrict |bounds| so that events located outside the bubble's visual bounds are treated as outside of the bubble." Does that make sense / reflect what is happening here? https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/tray_e... ash/system/tray/tray_event_filter.cc:83: ->IsMaximizeModeWindowManagerEnabled()) { Do you intentionally not check container_id == kShellWindowId_SettingBubbleContainer here since for all other containers the intersection on line 84 would just be a no-op? https://codereview.chromium.org/2930123002/diff/200001/tools/metrics/actions/... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2930123002/diff/200001/tools/metrics/actions/... tools/metrics/actions/actions.xml:16241: <action name="StatusArea_SystemTray_SwipeToOpen"> Feel free to remove the metrics collection from this CL if you're still waiting on PM feedback. We can always add metrics in a follow-on CL.
The CQ bit was checked by minch@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Tablet WM : Swiping on system tray bubble. Swiping up on the system tray should open the system tray bubble. 1.This works only for tablet and horizontal shelf. 2.Update the bounds of the bubble according to the position of the gesture. 3.For SCROLL_END event. To show or hide the bubble depends on the position of the gesture. Bubble is split into two parts. End in the lower part (one-third position) will hide the bubble; Otherwise, should always show the bubble. 4.For FLING event. To show or hide the bubble depends on gesture position and velocity. If the fling velocity larger than |kSwipingVelocity|, show or hide according to the fling direction; Otherwise, show or hide depends on the position. BUG=734814 ========== to ========== Tablet WM : Swiping on system tray bubble. Swiping up on the system tray to open the system tray bubble. 1.This works only for tablet and horizontal shelf. 2.Update the bounds of the bubble according to the position of the gesture. 3.For SCROLL_END event. To show or hide the bubble depends on the position of the gesture. Bubble is split into two parts. End in the lower part (one-third position) will hide the bubble; Otherwise, should always show the bubble. 4.For FLING event. To show or hide the bubble depends on gesture position and velocity. If the fling velocity larger than |kSwipingVelocity|, show or hide according to the fling direction; Otherwise, show or hide depends on the position. BUG=734814 ==========
Hi, tdanderson@, could you help review the patch 12? Thanks. https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/system... File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/system... ash/system/tray/system_tray.cc:689: is_in_drag_ = true; On 2017/06/26 18:13:02, tdanderson wrote: > Should this be placed just above line 703 (past the early returns)? If we hit > any of the early returns then I don't think we should consider ourself as being > in a drag, though I could be missing something here. Yeah, you are right, thanks. https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/system... ash/system/tray/system_tray.cc:724: is_in_drag_ = false; On 2017/06/26 18:13:02, tdanderson wrote: > What do you think of doing the following: > > In StartGestureDrag(): > DCHECK_EQ(gesture_drag_amount_, 0.f); > is_in_drag_ = true; > > Then in CompleteGestureDrag(): > is_in_drag_ = false; > gesture_drag_amount_ = 0.f; > > > This will trigger the DCHECK in any cases where, for some reason, a previous > drag didn't complete and you're trying to start a new one. Offhand I can't think > of any specific case where this could happen, but I am not 100% certain it's > impossible given the many different types of events we have and the possible > combinations of event streams. (For instance, could you run into a > ET_SCROLL_FLING_CANCEL instead of a ET_SCROLL_FLING_START?) Sorry, I think I made a mistake here. It should always has a SCROLL_END / FLING_START with a normal dragging sequence. I think it is possible the sequence ends with like FLING_CANCEL. How about set |is_in_drag_| to false and just close the bubble in this case? And after this, I think we don't need to DCHECK(is_in_drag_) in UpdateGestureDrag and CompleteGestureDrag? And also the |gesture_drag_amount_|? https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/system... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/system... ash/system/tray/system_tray.h:216: // |sequence_end|. Otherwise return false, indicating that the On 2017/06/26 18:13:02, tdanderson wrote: > nit: re-name the |gesture| parameter here to |sequence_end| to match the doc. Done. https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/system... ash/system/tray/system_tray.h:232: // during gesture dragging. On 2017/06/26 18:13:02, tdanderson wrote: > Suggestion for a reword here: "True if the user is in the process of > gesture-dragging to open the system tray bubble, false otherwise." Done. https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/system... File ash/system/tray/system_tray_unittest.cc (right): https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/system... ash/system/tray/system_tray_unittest.cc:156: gfx::Rect current_bounds = GetSystemBubbleBoundsInScreen(); On 2017/06/26 18:13:02, tdanderson wrote: > nit: newlines after lines 156, 164, 168 (before commenting a block/line of code) Done. https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/tray_e... File ash/system/tray/tray_event_filter.cc (right): https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/tray_e... ash/system/tray/tray_event_filter.cc:80: // happened outside the bubble. On 2017/06/26 18:13:02, tdanderson wrote: > Suggestion to re-word the second sentence: "During the drag, the bubble's > logical bounds can extend outside of the work area, but its visual bounds are > only within the work area. Restrict |bounds| so that events located outside the > bubble's visual bounds are treated as outside of the bubble." > > Does that make sense / reflect what is happening here? Yes, thanks. https://codereview.chromium.org/2930123002/diff/200001/ash/system/tray/tray_e... ash/system/tray/tray_event_filter.cc:83: ->IsMaximizeModeWindowManagerEnabled()) { On 2017/06/26 18:13:02, tdanderson wrote: > Do you intentionally not check container_id == > kShellWindowId_SettingBubbleContainer here since for all other containers the > intersection on line 84 would just be a no-op? I don't think that for all the other containers the intersection with the work area is just the original |bounds| (all is in work area). So I think it should be better to add the container_id restriction here.Thanks. https://codereview.chromium.org/2930123002/diff/200001/tools/metrics/actions/... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2930123002/diff/200001/tools/metrics/actions/... tools/metrics/actions/actions.xml:16241: <action name="StatusArea_SystemTray_SwipeToOpen"> On 2017/06/26 18:13:02, tdanderson wrote: > Feel free to remove the metrics collection from this CL if you're still waiting > on PM feedback. We can always add metrics in a follow-on CL. OK. I will remove it first and add it back in a follow-on CL.
Patch set 12 LGTM https://codereview.chromium.org/2930123002/diff/220001/ash/system/tray/system... File ash/system/tray/system_tray.h (right): https://codereview.chromium.org/2930123002/diff/220001/ash/system/tray/system... ash/system/tray/system_tray.h:228: // Tracks the amount of the drag. nit: Add "Only valid if |is_in_drag_| is true".
The CQ bit was checked by minch@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by minch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, xdai@chromium.org, jwd@chromium.org, tdanderson@chromium.org Link to the patchset: https://codereview.chromium.org/2930123002/#ps240001 (title: "Fixed comments.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1498589033351160,
"parent_rev": "cd8a5a3348315e3527c27bacca9cc1b65fdb3848", "commit_rev":
"e6131f3b26bf5d8904d266f9f826fb811b28f875"}
Message was sent while issue was closed.
Description was changed from ========== Tablet WM : Swiping on system tray bubble. Swiping up on the system tray to open the system tray bubble. 1.This works only for tablet and horizontal shelf. 2.Update the bounds of the bubble according to the position of the gesture. 3.For SCROLL_END event. To show or hide the bubble depends on the position of the gesture. Bubble is split into two parts. End in the lower part (one-third position) will hide the bubble; Otherwise, should always show the bubble. 4.For FLING event. To show or hide the bubble depends on gesture position and velocity. If the fling velocity larger than |kSwipingVelocity|, show or hide according to the fling direction; Otherwise, show or hide depends on the position. BUG=734814 ========== to ========== Tablet WM : Swiping on system tray bubble. Swiping up on the system tray to open the system tray bubble. 1.This works only for tablet and horizontal shelf. 2.Update the bounds of the bubble according to the position of the gesture. 3.For SCROLL_END event. To show or hide the bubble depends on the position of the gesture. Bubble is split into two parts. End in the lower part (one-third position) will hide the bubble; Otherwise, should always show the bubble. 4.For FLING event. To show or hide the bubble depends on gesture position and velocity. If the fling velocity larger than |kSwipingVelocity|, show or hide according to the fling direction; Otherwise, show or hide depends on the position. BUG=734814 Review-Url: https://codereview.chromium.org/2930123002 Cr-Commit-Position: refs/heads/master@{#482696} Committed: https://chromium.googlesource.com/chromium/src/+/e6131f3b26bf5d8904d266f9f826... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/e6131f3b26bf5d8904d266f9f826... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
