Fix bugs in scrollable shelf exposed by unit tests

Some bugs are exposed by enabling scrollable shelf during tests:
(1) In ScrollableShelfView::OnGestureEvent, event should be stopped from
propagation if it gets handled.
(2) In ScrollableShelfView::ShouldShowTooltipForView, it should check
that the shelf item is not removed from the shelf model,
(3) In FindFirstFocusableChild/FindLastFocusableChild, it should return
early if the view model is empty.

Bug: 1002576
Change-Id: I35a96d002bae88157e56622a880b6b3256e3e4fa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1807562Reviewed-by: Manu Cornet <manucornet@chromium.org>
Commit-Queue: Andrew Xu <andrewxu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#696993}

ash/shelf/scrollable_shelf_view.cc

@@ -568,8 +568,8 @@ void ScrollableShelfView::OnMouseEvent(ui::MouseEvent* event) {
 void ScrollableShelfView::OnGestureEvent(ui::GestureEvent* event) {
   if (ShouldHandleGestures(*event))
     HandleGestureEvent(event);
-  else
-    shelf_view_->HandleGestureEvent(event);
+  else if (shelf_view_->HandleGestureEvent(event))
+    event->StopPropagation();
 }
 
 const char* ScrollableShelfView::GetClassName() const {
@@ -606,6 +606,11 @@ bool ScrollableShelfView::ShouldShowTooltipForView(
   if (view->parent() != shelf_view_)
     return false;
 
+  // The shelf item corresponding to |view| may have been removed from the
+  // model.
+  if (!shelf_view_->ShouldShowTooltipForChildView(view))
+    return false;
+
   const gfx::Rect screen_bounds = view->GetBoundsInScreen();
   const gfx::Rect visible_bounds = shelf_container_view_->GetBoundsInScreen();
   return visible_bounds.Contains(screen_bounds);
@@ -943,10 +948,16 @@ void ScrollableShelfView::UpdateTappableIconIndices() {
 }
 
 views::View* ScrollableShelfView::FindFirstFocusableChild() {
+  if (shelf_view_->view_model()->view_size() == 0)
+    return nullptr;
+
   return shelf_view_->view_model()->view_at(shelf_view_->first_visible_index());
 }
 
 views::View* ScrollableShelfView::FindLastFocusableChild() {
+  if (shelf_view_->view_model()->view_size() == 0)
+    return nullptr;
+
   return shelf_view_->view_model()->view_at(shelf_view_->last_visible_index());
 }
 

ash/shelf/shelf_view.cc

@@ -969,6 +969,19 @@ bool ShelfView::HandleGestureEvent(const ui::GestureEvent* event) {
   return false;
 }
 
+bool ShelfView::ShouldShowTooltipForChildView(
+    const views::View* child_view) const {
+  DCHECK_EQ(this, child_view->parent());
+
+  if (child_view == overflow_button_)
+    return true;
+  // Don't show a tooltip for a view that's currently being dragged.
+  if (child_view == drag_view_)
+    return false;
+
+  return ShelfItemForView(child_view) && !IsShowingMenuForView(child_view);
+}
+
 // static
 void ShelfView::ConfigureChildView(views::View* view) {
   view->SetPaintToLayer();
@@ -2482,15 +2495,4 @@ base::string16 ShelfView::GetTitleForChildView(const views::View* view) const {
   return item ? item->title : base::string16();
 }
 
-bool ShelfView::ShouldShowTooltipForChildView(
-    const views::View* child_view) const {
-  if (child_view == overflow_button_)
-    return true;
-  // Don't show a tooltip for a view that's currently being dragged.
-  if (child_view == drag_view_)
-    return false;
-
-  return ShelfItemForView(child_view) && !IsShowingMenuForView(child_view);
-}
-
 }  // namespace ash

ash/shelf/shelf_view.h

@@ -283,6 +283,9 @@ class ASH_EXPORT ShelfView : public views::AccessiblePaneView,
   // Handles the gesture event. Returns true if |event| has been consumed.
   bool HandleGestureEvent(const ui::GestureEvent* event);
 
+  // Different from ShouldShowTooltipForView, |view| here must be a child view.
+  bool ShouldShowTooltipForChildView(const views::View* child_view) const;
+
   // Return the view model for test purposes.
   const views::ViewModel* view_model_for_test() const {
     return view_model_.get();
@@ -554,9 +557,6 @@ class ASH_EXPORT ShelfView : public views::AccessiblePaneView,
   // Different from GetTitleForView, |view| here must be a child view.
   base::string16 GetTitleForChildView(const views::View* view) const;
 
-  // Different from ShouldShowTooltipForView, |view| here must be a child view.
-  bool ShouldShowTooltipForChildView(const views::View* child_view) const;
-
   // The model; owned by Launcher.
   ShelfModel* model_;