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

ash/shelf/shelf_layout_manager.cc

Index: ash/shelf/shelf_layout_manager.cc
diff --git a/ash/shelf/shelf_layout_manager.cc b/ash/shelf/shelf_layout_manager.cc
index 895a31528497d9b50422a1b13337acd0b5c6968a..79beef49b94cea99d6ecac24493af1b9a22df506 100644
--- a/ash/shelf/shelf_layout_manager.cc
+++ b/ash/shelf/shelf_layout_manager.cc
@@ -864,6 +864,25 @@ gfx::Rect ShelfLayoutManager::GetAutoHideShowShelfRegionInScreen() const {
   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())
@@ -887,25 +906,8 @@ ShelfAutoHideState ShelfLayoutManager::CalculateAutoHideState(
        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)
@@ -1129,8 +1131,12 @@ void ShelfLayoutManager::CompleteGestureDrag(const ui::GestureEvent& gesture) {
   // 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();