Cros: Dismiss app list when tapping on the shelf

Bug: 923481
Change-Id: I60cb8729b9d423858afe6c15b47da67b98b58e6f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2131043
Commit-Queue: Ana Salazar <anasalazar@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#755479}

ash/app_list/app_list_controller_impl.cc

@@ -1054,13 +1054,13 @@ void AppListControllerImpl::SetHomeLauncherAnimationCallbackForTesting(
   home_launcher_animation_callback_ = std::move(callback);
 }
 
-void AppListControllerImpl::RecordShelfAppLaunched(
-    base::Optional<AppListViewState> recorded_app_list_view_state,
-    base::Optional<bool> recorded_home_launcher_shown) {
+void AppListControllerImpl::RecordShelfAppLaunched() {
   RecordAppListAppLaunched(
       AppListLaunchedFrom::kLaunchedFromShelf,
-      recorded_app_list_view_state.value_or(GetAppListViewState()),
-      IsTabletMode(), recorded_home_launcher_shown.value_or(last_visible_));
+      recorded_app_list_view_state_.value_or(GetAppListViewState()),
+      IsTabletMode(), recorded_app_list_visibility_.value_or(last_visible_));
+  recorded_app_list_view_state_ = base::nullopt;
+  recorded_app_list_visibility_ = base::nullopt;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -1766,4 +1766,9 @@ void AppListControllerImpl::UpdateTrackedAppWindow() {
     tracked_app_window_->AddObserver(this);
 }
 
+void AppListControllerImpl::RecordAppListState() {
+  recorded_app_list_view_state_ = GetAppListViewState();
+  recorded_app_list_visibility_ = last_visible_;
+}
+
 }  // namespace ash

ash/app_list/app_list_controller_impl.h

@@ -333,9 +333,7 @@ class ASH_EXPORT AppListControllerImpl : public AppListController,
   void SetHomeLauncherAnimationCallbackForTesting(
       HomeLauncherAnimationCallback callback);
 
-  void RecordShelfAppLaunched(
-      base::Optional<AppListViewState> recorded_app_list_view_state,
-      base::Optional<bool> home_launcher_shown);
+  void RecordShelfAppLaunched();
 
   // Updates which container the launcher window should be in.
   void UpdateLauncherContainer(
@@ -351,6 +349,10 @@ class ASH_EXPORT AppListControllerImpl : public AppListController,
   aura::Window* GetContainerForDisplayId(
       base::Optional<int64_t> display_id = base::nullopt);
 
+  // Methods for recording the state of the app list before it changes in order
+  // to record metrics.
+  void RecordAppListState();
+
  private:
   // HomeScreenDelegate:
   void OnHomeLauncherDragStart() override;
@@ -444,6 +446,16 @@ class ASH_EXPORT AppListControllerImpl : public AppListController,
   // visibility animation to finish. Should only be used in tablet mode.
   HomeLauncherAnimationCallback home_launcher_animation_callback_;
 
+  // The AppListViewState at the moment it was recorded, used to record app
+  // launching metrics. This allows an accurate AppListViewState to be recorded
+  // before AppListViewState changes.
+  base::Optional<AppListViewState> recorded_app_list_view_state_;
+
+  // Whether the applist was shown at the moment it was recorded, used to record
+  // app launching metrics. This is recorded because AppList visibility can
+  // change before the metric is recorded.
+  base::Optional<bool> recorded_app_list_visibility_;
+
   // ScopedClosureRunner which while in scope keeps background blur in home
   // screen (in particular, apps container suggestion chips background)
   // disabled. Set while home screen transitions are in progress.

ash/app_list/app_list_presenter_delegate_impl.cc

@@ -250,18 +250,20 @@ void AppListPresenterDelegateImpl::ProcessLocatedEvent(
       !switches::ShouldNotDismissOnBlur() && !IsTabletMode()) {
     const aura::Window* status_window =
         shelf->shelf_widget()->status_area_widget()->GetNativeWindow();
-    // Don't dismiss the auto-hide shelf if event happened in status area. Then
-    // the event can still be propagated to the status area tray to open the
-    // corresponding tray bubble.
-    base::Optional<Shelf::ScopedAutoHideLock> auto_hide_lock;
-    if (status_window && status_window->Contains(target))
-      auto_hide_lock.emplace(shelf);
-
-    // Keep the app list open if the event happened in the shelf area.
     const aura::Window* hotseat_window =
         shelf->hotseat_widget()->GetNativeWindow();
-    if (!hotseat_window || !hotseat_window->Contains(target))
-      presenter_->Dismiss(event->time_stamp());
+    // Don't dismiss the auto-hide shelf if event happened in status area or the
+    // hotseat. Then the event can still be propagated.
+    base::Optional<Shelf::ScopedAutoHideLock> auto_hide_lock;
+    if ((status_window && status_window->Contains(target)) ||
+        (hotseat_window && hotseat_window->Contains(target))) {
+      auto_hide_lock.emplace(shelf);
+    }
+    // Record the current AppListViewState to be used later for metrics. The
+    // AppListViewState will change on app launch, so this will record the
+    // AppListViewState before the app was launched.
+    controller_->RecordAppListState();
+    presenter_->Dismiss(event->time_stamp());
   }
 }
 

ash/app_list/app_list_presenter_delegate_unittest.cc

@@ -2202,11 +2202,11 @@ TEST_F(AppListPresenterDelegateTest, TapAutoHideShelfWithAppListOpened) {
   GetAppListTestHelper()->CheckVisibility(true);
   EXPECT_EQ(SHELF_AUTO_HIDE_SHOWN, shelf->GetAutoHideState());
 
-  // Test that tapping the auto-hidden shelf keeps the app list and shelf
-  // visible.
+  // Test that tapping the auto-hidden shelf keeps shelf visible but dismiss the
+  // app list.
   generator->GestureTapAt(
       shelf->GetShelfViewForTesting()->GetBoundsInScreen().CenterPoint());
-  GetAppListTestHelper()->CheckVisibility(true);
+  GetAppListTestHelper()->CheckVisibility(false);
   EXPECT_EQ(SHELF_AUTO_HIDE_SHOWN, shelf->GetAutoHideState());
 }
 

ash/shelf/shelf_application_menu_model.cc

@@ -41,9 +41,7 @@ void ShelfApplicationMenuModel::ExecuteCommand(int command_id,
   if (delegate_) {
     // Record app launch when selecting window to open from disambiguation
     // menu.
-    Shell::Get()->app_list_controller()->RecordShelfAppLaunched(
-        base::nullopt /* recorded_app_list_view_state */,
-        base::nullopt /* recorded_home_launcher_shown */);
+    Shell::Get()->app_list_controller()->RecordShelfAppLaunched();
 
     // The display hosting the menu is irrelevant, windows activate in-place.
     delegate_->ExecuteCommand(false /*from_context_menu*/, command_id,

ash/shelf/shelf_context_menu_model.cc

@@ -117,9 +117,7 @@ void ShelfContextMenuModel::ExecuteCommand(int command_id, int event_flags) {
     default:
       if (delegate_) {
         if (IsCommandIdAnAppLaunch(command_id)) {
-          shell->app_list_controller()->RecordShelfAppLaunched(
-              base::nullopt /* recorded_app_list_view_state */,
-              base::nullopt /* recorded_home_launcher_shown */);
+          shell->app_list_controller()->RecordShelfAppLaunched();
         }
 
         delegate_->ExecuteCommand(true, command_id, event_flags, display_id_);

ash/shelf/shelf_view.cc

@@ -589,7 +589,6 @@ void ShelfView::OnShelfButtonAboutToRequestFocusFromTabTraversal(
 void ShelfView::ButtonPressed(views::Button* sender,
                               const ui::Event& event,
                               views::InkDrop* ink_drop) {
-
   if (!ShouldEventActivateButton(sender, event)) {
     ink_drop->SnapToHidden();
     return;
@@ -640,14 +639,6 @@ void ShelfView::ButtonPressed(views::Button* sender,
       break;
   }
 
-  // Record the current AppListViewState to be used later for metrics. The
-  // AppListViewState will change on app launch, so this will record the
-  // AppListViewState before the app was launched.
-  recorded_app_list_view_state_ =
-      Shell::Get()->app_list_controller()->GetAppListViewState();
-  app_list_visibility_before_app_launch_ =
-      Shell::Get()->app_list_controller()->IsVisible(GetDisplayIdForView(this));
-
   // Run AfterItemSelected directly if the item has no delegate (ie. in tests).
   const ShelfItem& item = model_->items()[last_pressed_index_];
   if (!model_->GetShelfItemDelegate(item.id)) {
@@ -1547,8 +1538,7 @@ void ShelfView::OnFadeOutAnimationEnded() {
   StartFadeInLastVisibleItem();
 }
 
-void ShelfView::StartFadeInLastVisibleItem() {
-}
+void ShelfView::StartFadeInLastVisibleItem() {}
 
 gfx::Rect ShelfView::GetMenuAnchorRect(const views::View& source,
                                        const gfx::Point& location,
@@ -1914,8 +1904,7 @@ void ShelfView::AfterItemSelected(const ShelfItem& item,
   // Record AppList metric for any action considered an app launch.
   if (action == SHELF_ACTION_NEW_WINDOW_CREATED ||
       action == SHELF_ACTION_WINDOW_ACTIVATED) {
-    Shell::Get()->app_list_controller()->RecordShelfAppLaunched(
-        recorded_app_list_view_state_, app_list_visibility_before_app_launch_);
+    Shell::Get()->app_list_controller()->RecordShelfAppLaunched();
   }
 
   // The app list handles its own ink drop effect state changes.

ash/shelf/shelf_view.h

@@ -611,16 +611,6 @@ class ASH_EXPORT ShelfView : public views::AccessiblePaneView,
   // Used to call SpeedUpDragScrolling.
   base::OneShotTimer speed_up_drag_scrolling_;
 
-  // The AppListViewState recorded before a button press, used to record app
-  // launching metrics. This allows an accurate AppListViewState to be recorded
-  // before AppListViewState changes.
-  AppListViewState recorded_app_list_view_state_;
-
-  // Whether the applist was shown before a button press, used to record app
-  // launching metrics. This is recorded because AppList visibility can change
-  // before the metric is recorded.
-  bool app_list_visibility_before_app_launch_ = false;
-
   // Whether this view should focus its last focusable child (instead of its
   // first) when focused.
   bool default_last_focusable_child_ = false;