Disallow toggling of shelf auto-hide with touch when there are no windows open

ash/shelf/shelf_layout_manager.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ash/shelf/shelf_layout_manager.h"
 
 #include <algorithm>
 #include <cmath>
 #include <vector>
 
@@ skipping 846 matching lines @@
   if (wm_shelf_->IsHorizontalAlignment())
     show_shelf_region_in_screen.set_height(kMaxAutoHideShowShelfRegionSize);
   else
     show_shelf_region_in_screen.set_width(kMaxAutoHideShowShelfRegionSize);
 
   // TODO: Figure out if we need any special handling when the keyboard is
   // visible.
   return show_shelf_region_in_screen;
 }
 
+bool ShelfLayoutManager::HasVisibleWindow() const {
+  const int64_t shelf_display_id =
+      WmWindow::Get(shelf_widget_->GetNativeWindow())
+          ->GetDisplayNearestWindow()
+          .id();
+  const std::vector<WmWindow*> windows =
+      Shell::Get()->mru_window_tracker()->BuildWindowListIgnoreModal();
+  // Process the window list and check if there are any visible windows.
+  // Ignore app list windows that may be animating to hide after dismissal.
+  for (size_t i = 0; i < windows.size(); ++i) {

[oshima, 2017/04/25 18:02:16]

auto* window : windows

+    if (windows[i] && windows[i]->IsVisible() && !IsAppListWindow(windows[i]) &&

[oshima, 2017/04/25 18:02:16]

the list shouldn't contain null. Was this  null check necessary?

[minch1, 2017/04/25 23:43:08]

Checked line 873 seems it will not contains null windows. Not sure when it will
has null windows. If it will has no null windows here, we don't need to check it
here.

[oshima, 2017/04/27 01:53:55]

It shouldn't have, as it only returns activatable ones.

+        !windows[i]->GetWindowState()->IsMinimized() &&

[oshima, 2017/04/25 18:02:16]

IIRC, minimized window isn't visible, no?

[minch1, 2017/04/25 23:43:08]

Checked BuildWindowListsIgnoredModal() in line 873. It focuses on the windows
that considered as activated. Seems that minimized windows can always be
activated? What we need to consider is not the visibility of the window but is
it included in the |windows| in line 872 or not?

[oshima, 2017/04/27 01:53:54]

Yes, a minimized window can be activated.
A minimized window is  such case, but there is no guarantee (from this code)
that that's only case. I think you should be able to just check visibility.

+        windows[i]->GetDisplayNearestWindow().id() == shelf_display_id) {
+      return true;
+    }
+  }
+  return false;
+}
+
 ShelfAutoHideState ShelfLayoutManager::CalculateAutoHideState(
     ShelfVisibilityState visibility_state) const {
   if (visibility_state != SHELF_AUTO_HIDE || !wm_shelf_->IsShelfInitialized())
     return SHELF_AUTO_HIDE_HIDDEN;
 
   if (shelf_widget_->IsShowingAppList())
     return SHELF_AUTO_HIDE_SHOWN;
 
   if (shelf_widget_->status_area_widget() &&
       shelf_widget_->status_area_widget()->ShouldShowShelf())
     return SHELF_AUTO_HIDE_SHOWN;
 
   if (shelf_widget_->IsShowingContextMenu())
     return SHELF_AUTO_HIDE_SHOWN;
 
   if (shelf_widget_->IsShowingOverflowBubble())
     return SHELF_AUTO_HIDE_SHOWN;
 
   if (shelf_widget_->IsActive() ||
       (shelf_widget_->status_area_widget() &&
        shelf_widget_->status_area_widget()->IsActive()))
     return SHELF_AUTO_HIDE_SHOWN;
 
-  const int64_t shelf_display_id =
-      WmWindow::Get(shelf_widget_->GetNativeWindow())
-          ->GetDisplayNearestWindow()
-          .id();
-  const std::vector<WmWindow*> windows =
-      Shell::Get()->mru_window_tracker()->BuildWindowListIgnoreModal();
-  // Process the window list and check if there are any visible windows.
-  // Ignore app list windows that may be animating to hide after dismissal.
-  bool visible_window = false;
-  for (size_t i = 0; i < windows.size(); ++i) {
-    if (windows[i] && windows[i]->IsVisible() && !IsAppListWindow(windows[i]) &&
-        !windows[i]->GetWindowState()->IsMinimized() &&
-        windows[i]->GetDisplayNearestWindow().id() == shelf_display_id) {
-      visible_window = true;
-      break;
-    }
-  }
   // If there are no visible windows do not hide the shelf.
-  if (!visible_window)
+  if (!HasVisibleWindow())
     return SHELF_AUTO_HIDE_SHOWN;
 
   if (gesture_drag_status_ == GESTURE_DRAG_COMPLETE_IN_PROGRESS)
     return gesture_drag_auto_hide_state_;
 
   // Don't show if the user is dragging the mouse.
   if (in_mouse_drag_)
     return SHELF_AUTO_HIDE_HIDDEN;
 
   // Ignore the mouse position if mouse events are disabled.
@@ skipping 203 matching lines @@
           : SHELF_AUTO_HIDE_SHOWN;
   ShelfAutoHideBehavior new_auto_hide_behavior =
       gesture_drag_auto_hide_state_ == SHELF_AUTO_HIDE_SHOWN
           ? SHELF_AUTO_HIDE_BEHAVIOR_NEVER
           : SHELF_AUTO_HIDE_BEHAVIOR_ALWAYS;
 
   // When in fullscreen and the shelf is forced to be auto hidden, the auto hide
   // behavior affects neither the visibility state nor the auto hide state. Set
   // |gesture_drag_status_| to GESTURE_DRAG_COMPLETE_IN_PROGRESS to set the auto
   // hide state to |gesture_drag_auto_hide_state_|.
+  // Since the shelf will always be shown when there are no windows opened.
+  // There is no point to change the auto hide behavior when swiping down / up
+  // on the shelf.

[tdanderson, 2017/04/24 16:57:23]

Based on this comment, shouldn't line 1139 instead be !HasVisibleWindow()?

nit: comma instead of period at the end of line 1134.

nit: start the comment at the end of line 1133 instead of on its own line.

[minch1, 2017/04/24 17:42:40]

Line 1140 will call SetAutoHideBehavior() which will change the shelf auto hide
state. But we don't want this happens when there are no windows open. Keep shelf
state as 'auto hide' if it is 'auto hide' regardless swiping actions. The same
as 'non auto hide'.

[tdanderson, 2017/04/25 13:43:06]

Ah yes, I was mis-reading the code, sorry. 

As an optional suggestion, consider a re-wording of the comment on line 1134 to
be "Only change the auto-hide behavior if there is at least one window visible."

   gesture_drag_status_ = GESTURE_DRAG_COMPLETE_IN_PROGRESS;
-  if (wm_shelf_->auto_hide_behavior() != new_auto_hide_behavior)
+  if (wm_shelf_->auto_hide_behavior() != new_auto_hide_behavior &&
+      HasVisibleWindow())

[tdanderson, 2017/04/24 16:57:23]

nit: use {} after the if and else since the if condition is now more than a
single line.

     wm_shelf_->SetAutoHideBehavior(new_auto_hide_behavior);
   else
     UpdateVisibilityState();
   gesture_drag_status_ = GESTURE_DRAG_NONE;
 }
 
 void ShelfLayoutManager::CancelGestureDrag() {
   gesture_drag_status_ = GESTURE_DRAG_CANCEL_IN_PROGRESS;
   UpdateVisibilityState();
   gesture_drag_status_ = GESTURE_DRAG_NONE;
 }
 
 }  // namespace ash