Tablet WM : Swiping on system tray bubble.

ash/system/tray/system_tray.cc

Index: ash/system/tray/system_tray.cc
diff --git a/ash/system/tray/system_tray.cc b/ash/system/tray/system_tray.cc
index 78f2929147dab3951ce59e4d5e0d922ef7483b9c..bf8761fd10dd11b884485f5efbd88fc392c502bc 100644
--- a/ash/system/tray/system_tray.cc
+++ b/ash/system/tray/system_tray.cc
@@ -50,6 +50,7 @@
 #include "ash/system/user/tray_user.h"
 #include "ash/system/web_notification/web_notification_tray.h"
 #include "ash/wm/container_finder.h"
+#include "ash/wm/maximize_mode/maximize_mode_controller.h"
 #include "ash/wm/widget_finder.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
@@ -172,8 +173,10 @@ class SystemTray::ActivationObserver : public ::wm::ActivationChangeObserver {
     int container_id = wm::GetContainerForWindow(gained_active)->id();
 
     // Don't close the bubble if a popup notification is activated.
-    if (container_id == kShellWindowId_StatusContainer)
+    if (container_id == kShellWindowId_StatusContainer ||
+        container_id == kShellWindowId_SettingBubbleContainer) {
       return;
+    }
 
     views::Widget* bubble_widget =
         tray_->GetSystemBubble()->bubble_view()->GetWidget();
@@ -198,7 +201,8 @@ class SystemTray::ActivationObserver : public ::wm::ActivationChangeObserver {
 
 // SystemTray
 
-SystemTray::SystemTray(Shelf* shelf) : TrayBackgroundView(shelf) {
+SystemTray::SystemTray(Shelf* shelf)
+    : TrayBackgroundView(shelf), shelf_(shelf) {
   SetInkDropMode(InkDropMode::ON);
 
   // Since user avatar is on the right hand side of System tray of a
@@ -630,6 +634,21 @@ void SystemTray::ActivateBubble() {
   bubble_view->GetWidget()->Activate();
 }
 
+void SystemTray::OnGestureEvent(ui::GestureEvent* event) {
+  if (Shell::Get()
+          ->maximize_mode_controller()
+          ->IsMaximizeModeWindowManagerEnabled() &&
+      shelf_->IsHorizontalAlignment() && ProcessGestureEvent(*event)) {
+    event->StopPropagation();

[tdanderson, 2017/06/20 22:42:27]

I think that event->SetHandled() should do the trick here, is there a reason why
you need to use StopPropagation() specifically?

[minch1, 2017/06/22 17:42:11]

Done.

+  } else {
+    TrayBackgroundView::OnGestureEvent(event);
+  }
+}
+
+gfx::Rect SystemTray::GetWorkAreaBoundsInScreen() const {
+  return shelf_->GetUserWorkAreaBounds();
+}
+
 bool SystemTray::PerformAction(const ui::Event& event) {
   // If we're already showing a full system tray menu, either default or
   // detailed menu, hide it; otherwise, show it (and hide any popup that's
@@ -644,6 +663,91 @@ bool SystemTray::PerformAction(const ui::Event& event) {
   return true;
 }
 
+bool SystemTray::ProcessGestureEvent(const ui::GestureEvent& event) {
+  if (event.type() == ui::ET_GESTURE_SCROLL_BEGIN)
+    return StartGestureDrag(event);
+
+  if (!HasSystemBubble())
+    return false;

[xdai1, 2017/06/20 17:00:31]

Why don't put this if-clause at the top?

[xdai1, 2017/06/20 17:02:53]

I think I get it...

+
+  if (event.type() == ui::ET_GESTURE_SCROLL_UPDATE) {
+    UpdateGestureDrag(event);
+    return true;
+  }
+
+  if (event.type() == ui::ET_GESTURE_SCROLL_END ||
+      event.type() == ui::ET_SCROLL_FLING_START) {
+    CompleteGestureDrag(event);
+    return true;
+  }
+  return false;
+}
+
+bool SystemTray::StartGestureDrag(const ui::GestureEvent& gesture) {
+  if ((HasSystemBubble() && full_system_tray_menu_) ||
+      IsDraggingDownOnShelf(gesture)) {

[tdanderson, 2017/06/20 22:42:24]

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.

[minch1, 2017/06/22 17:42:11]

Yes, you are right, thanks.

+    return false;
+  }
+
+  gesture_drag_amount_ = 0.f;

[tdanderson, 2017/06/20 22:42:23]

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.

[minch1, 2017/06/22 17:42:11]

Done.

+  ShowDefaultView(BUBBLE_CREATE_NEW);
+  system_tray_bubble_bounds_ =
+      system_bubble_->bubble_view()->GetWidget()->GetWindowBoundsInScreen();
+  UpdateBoundsOnDragging(gesture);
+  return true;
+}
+
+void SystemTray::UpdateGestureDrag(const ui::GestureEvent& gesture) {
+  UpdateBoundsOnDragging(gesture);
+  gesture_drag_amount_ += gesture.details().scroll_y();
+}
+
+void SystemTray::CompleteGestureDrag(const ui::GestureEvent& gesture) {
+  gfx::Rect target_bounds = system_tray_bubble_bounds_;

[tdanderson, 2017/06/20 22:42:25]

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);
  

[minch1, 2017/06/22 17:42:11]

Done.

+  if (ShouldShowSystemBubbleForDragging(gesture)) {
+    system_bubble_->bubble()->UpdateBounds(target_bounds, false);
+  } else {
+    target_bounds.set_y(shelf_->GetIdealBounds().y());
+    system_bubble_->bubble()->UpdateBounds(target_bounds, true);
+  }
+}
+
+bool SystemTray::IsDraggingDownOnShelf(const ui::GestureEvent& gesture) {
+  gfx::Point gesture_location = gesture.location();
+  View::ConvertPointToScreen(this, &gesture_location);
+
+  if (gesture_location.y() >= shelf_->GetIdealBounds().y() &&

[tdanderson, 2017/06/20 22:42:23]

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;

[minch1, 2017/06/22 17:42:11]

Acknowledged.

+      gesture.details().scroll_y_hint() > 0) {
+    return true;
+  }
+  return false;
+}
+
+void SystemTray::UpdateBoundsOnDragging(const ui::GestureEvent& gesture) {

[tdanderson, 2017/06/20 22:42:26]

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.

[minch1, 2017/06/22 17:42:11]

Done.

+  gfx::Point gesture_location = gesture.location();
+  View::ConvertPointToScreen(this, &gesture_location);
+
+  // System tray bubble should not be dragged higher than its original height.
+  if (gesture_location.y() < system_tray_bubble_bounds_.y())
+    gesture_location.set_y(system_tray_bubble_bounds_.y());
+
+  gfx::Rect bounds_on_gesture(
+      system_tray_bubble_bounds_.x(), gesture_location.y(),
+      system_tray_bubble_bounds_.width(), system_tray_bubble_bounds_.height());
+  system_bubble_->bubble_view()->GetWidget()->SetBounds(bounds_on_gesture);
+}
+
+bool SystemTray::ShouldShowSystemBubbleForDragging(
+    const ui::GestureEvent& gesture) {
+  const float kFlingVelocity = 100.0f;
+  if (gesture.type() == ui::ET_SCROLL_FLING_START &&

[tdanderson, 2017/06/20 22:42:20]

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."

[minch1, 2017/06/22 17:42:11]

Done.

+      fabs(gesture.details().velocity_y()) > kFlingVelocity) {
+    return gesture.details().velocity_y() < 0;
+  }
+

[tdanderson, 2017/06/20 22:42:20]

Consider adding a DCHECK here to verify that the gesture type is a
GESTURE_SCROLL_END.

[minch1, 2017/06/22 17:42:11]

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.

[tdanderson, 2017/06/22 20:21:42]

Ah yes, you're right. Then perhaps add a DCHECK to verify (type == SCROLL_END ||
type == SCROLL_FLING_START).

[minch1, 2017/06/23 22:39:56]

Done.

+  return -gesture_drag_amount_ >= system_tray_bubble_bounds_.height() / 3.0;

[tdanderson, 2017/06/20 22:42:21]

Suggestion for inline documentation here: "Show the system menu if it is already
at least one-third visible."

[minch1, 2017/06/22 17:42:11]

Done.

+}
+
 void SystemTray::CloseSystemBubbleAndDeactivateSystemTray() {
   activation_observer_.reset();
   key_event_watcher_.reset();