cros: Correctly record AppListShowSource home button in tablet mode

This change ensures that the home button is not recorded as showing the
app list multiple times once the app list is already shown. Also correctly
records app list shown during transitions between tablet mode and
clamshell mode.

Tests have also been added to ensure that the AppListShowSource metric
is recorded correctly for the fixed cases.

Bug: 1013790
Change-Id: I67bb0f1de8b6bd20c03fd6ac586095d92255119b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1869123
Commit-Queue: Matthew Mourgos <mmourgos@chromium.org>
Reviewed-by: Alex Newcomer <newcomer@chromium.org>
Reviewed-by: Manu Cornet <manucornet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#709677}

ash/app_list/app_list_controller_impl.cc

@@ -131,6 +131,10 @@ bool HasVisibleWindows() {
   return false;
 }
 
+void LogAppListShowSource(AppListShowSource show_source) {
+  UMA_HISTOGRAM_ENUMERATION(kAppListToggleMethodHistogram, show_source);
+}
+
 }  // namespace
 
 AppListControllerImpl::AppListControllerImpl()
@@ -474,8 +478,10 @@ void AppListControllerImpl::OnActiveUserPrefServiceChanged(
     return;
   }
 
-  // Show the app list after signing in in tablet mode.
-  Show(GetDisplayIdToShowAppListOn(), AppListShowSource::kTabletMode,
+  // Show the app list after signing in in tablet mode. For metrics, the app
+  // list is not considered shown since the browser window is shown over app
+  // list upon login.
+  Show(GetDisplayIdToShowAppListOn(), base::nullopt /* no AppListShowSource */,
        base::TimeTicks());
 
   // The app list is not dismissed before switching user, suggestion chips will
@@ -530,9 +536,10 @@ bool AppListControllerImpl::GetTargetVisibility() const {
 }
 
 void AppListControllerImpl::Show(int64_t display_id,
-                                 AppListShowSource show_source,
+                                 base::Optional<AppListShowSource> show_source,
                                  base::TimeTicks event_time_stamp) {
-  UMA_HISTOGRAM_ENUMERATION(kAppListToggleMethodHistogram, show_source);
+  if (show_source.has_value())
+    LogAppListShowSource(show_source.value());
 
   presenter_.Show(display_id, event_time_stamp);
 
@@ -571,9 +578,8 @@ ash::ShelfAction AppListControllerImpl::ToggleAppList(
   ash::ShelfAction action =
       presenter_.ToggleAppList(display_id, show_source, event_time_stamp);
   UpdateExpandArrowVisibility();
-  if (action == SHELF_ACTION_APP_LIST_SHOWN) {
-    UMA_HISTOGRAM_ENUMERATION(kAppListToggleMethodHistogram, show_source);
-  }
+  if (action == SHELF_ACTION_APP_LIST_SHOWN)
+    LogAppListShowSource(show_source);
   return action;
 }
 
@@ -787,7 +793,13 @@ void AppListControllerImpl::OnHomeLauncherTargetPositionChanged(
 void AppListControllerImpl::ShowHomeScreenView() {
   DCHECK(IsTabletMode());
 
-  Show(GetDisplayIdToShowAppListOn(), kTabletMode, base::TimeTicks());
+  // App list is only considered shown for metrics if there are currently no
+  // other visible windows shown over the app list after the tablet transition.
+  base::Optional<AppListShowSource> show_source;
+  if (!HasVisibleWindows())
+    show_source = kTabletMode;
+
+  Show(GetDisplayIdToShowAppListOn(), show_source, base::TimeTicks());
 }
 
 aura::Window* AppListControllerImpl::GetHomeScreenWindow() {
@@ -851,9 +863,12 @@ ash::ShelfAction AppListControllerImpl::OnHomeButtonPressed(
   bool handled = Shell::Get()->home_screen_controller()->GoHome(display_id);
 
   // Perform the "back" action for the app list.
-  if (!handled)
+  if (!handled) {
     Back();
+    return ash::SHELF_ACTION_APP_LIST_BACK;
+  }
 
+  LogAppListShowSource(show_source);
   return ash::SHELF_ACTION_APP_LIST_SHOWN;
 }
 

ash/app_list/app_list_controller_impl.h

@@ -146,7 +146,7 @@ class ASH_EXPORT AppListControllerImpl
   // Methods used in ash:
   bool GetTargetVisibility() const;
   void Show(int64_t display_id,
-            AppListShowSource show_source,
+            base::Optional<AppListShowSource> show_source,
             base::TimeTicks event_time_stamp);
   void UpdateYPositionAndOpacity(int y_position_in_screen,
                                  float background_opacity);

ash/app_list/app_list_metrics_unittest.cc

@@ -6,6 +6,7 @@
 #include <vector>
 
 #include "ash/app_list/app_list_controller_impl.h"
+#include "ash/app_list/app_list_metrics.h"
 #include "ash/app_list/model/search/search_model.h"
 #include "ash/app_list/test/app_list_test_helper.h"
 #include "ash/app_list/test/app_list_test_model.h"
@@ -22,9 +23,11 @@
 #include "ash/public/cpp/app_list/app_list_types.h"
 #include "ash/public/cpp/shelf_item_delegate.h"
 #include "ash/public/cpp/shelf_model.h"
+#include "ash/shelf/home_button.h"
 #include "ash/shelf/shelf.h"
 #include "ash/shelf/shelf_view.h"
 #include "ash/shelf/shelf_view_test_api.h"
+#include "ash/shelf/shelf_widget.h"
 #include "ash/shell.h"
 #include "ash/test/ash_test_base.h"
 #include "ash/wm/tablet_mode/tablet_mode_controller.h"
@@ -494,4 +497,76 @@ TEST_F(AppListAppLaunchedMetricTest, HomecherSearchLaunchFromSearchBox) {
       1 /* Number of times launched from search box */);
 }
 
+class AppListShowSourceMetricTest : public AshTestBase {
+ public:
+  AppListShowSourceMetricTest() = default;
+  ~AppListShowSourceMetricTest() override = default;
+
+ protected:
+  void ClickHomeButton() {
+    HomeButton* home_button =
+        GetPrimaryShelf()->shelf_widget()->GetHomeButton();
+    gfx::Point center = home_button->GetCenterPoint();
+    views::View::ConvertPointToScreen(home_button, &center);
+    GetEventGenerator()->MoveMouseTo(center);
+    GetEventGenerator()->ClickLeftButton();
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(AppListShowSourceMetricTest);
+};
+
+// In tablet mode, test that AppListShowSource metric is only recorded when
+// pressing home button when not already home. Any presses on the home button
+// when already home should do nothing.
+TEST_F(AppListShowSourceMetricTest, TabletInAppToHome) {
+  base::HistogramTester histogram_tester;
+
+  std::unique_ptr<views::Widget> widget = CreateTestWidget();
+  Shell::Get()->tablet_mode_controller()->SetEnabledForTest(true);
+
+  ClickHomeButton();
+  histogram_tester.ExpectBucketCount(
+      kAppListToggleMethodHistogram, kShelfButton,
+      1 /* Number of times app list is shown with a shelf button */);
+  histogram_tester.ExpectBucketCount(
+      kAppListToggleMethodHistogram, kTabletMode,
+      0 /* Number of times app list is shown by tablet mode transition */);
+
+  GetAppListTestHelper()->CheckVisibility(true);
+
+  // Ensure that any subsequent clicks while already at home do not count as
+  // showing the app list.
+  ClickHomeButton();
+  histogram_tester.ExpectBucketCount(
+      kAppListToggleMethodHistogram, kShelfButton,
+      1 /* Number of times app list shown with a shelf button */);
+  histogram_tester.ExpectTotalCount(kAppListToggleMethodHistogram, 1);
+}
+
+// Ensure that app list is not recorded as shown when going to tablet mode with
+// a window open.
+TEST_F(AppListShowSourceMetricTest, TabletModeWithWindowOpen) {
+  base::HistogramTester histogram_tester;
+
+  std::unique_ptr<views::Widget> widget = CreateTestWidget();
+  Shell::Get()->tablet_mode_controller()->SetEnabledForTest(true);
+  GetAppListTestHelper()->CheckVisibility(false);
+
+  // Ensure that no AppListShowSource metric was recoreded.
+  histogram_tester.ExpectTotalCount(kAppListToggleMethodHistogram, 0);
+}
+
+// Ensure that app list is recorded as shown when going to tablet mode with no
+// other windows open.
+TEST_F(AppListShowSourceMetricTest, TabletModeWithNoWindowOpen) {
+  base::HistogramTester histogram_tester;
+
+  Shell::Get()->tablet_mode_controller()->SetEnabledForTest(true);
+  GetAppListTestHelper()->CheckVisibility(true);
+
+  histogram_tester.ExpectBucketCount(
+      kAppListToggleMethodHistogram, kTabletMode,
+      1 /* Number of times app list shown after entering tablet mode */);
+}
+
 }  // namespace ash

ash/public/cpp/shelf_types.h

@@ -123,6 +123,9 @@ enum ShelfAction {
 
   // The app list launcher menu was dismissed.
   SHELF_ACTION_APP_LIST_DISMISSED,
+
+  // The back action was performed on the app list.
+  SHELF_ACTION_APP_LIST_BACK,
 };
 
 // The type of a shelf item.

ash/shelf/shelf_button_pressed_metric_tracker.cc

@@ -63,6 +63,7 @@ void ShelfButtonPressedMetricTracker::RecordButtonPressedAction(
     case SHELF_ACTION_NONE:
     case SHELF_ACTION_APP_LIST_SHOWN:
     case SHELF_ACTION_APP_LIST_DISMISSED:
+    case SHELF_ACTION_APP_LIST_BACK:
       break;
     case SHELF_ACTION_NEW_WINDOW_CREATED:
       Shell::Get()->metrics()->RecordUserMetricsAction(

ash/shelf/shelf_layout_manager.cc

@@ -766,6 +766,7 @@ void ShelfLayoutManager::OnShelfItemSelected(ShelfAction action) {
     case SHELF_ACTION_NONE:
     case SHELF_ACTION_APP_LIST_SHOWN:
     case SHELF_ACTION_APP_LIST_DISMISSED:
+    case SHELF_ACTION_APP_LIST_BACK:
     case SHELF_ACTION_WINDOW_MINIMIZED:
       break;
     case SHELF_ACTION_NEW_WINDOW_CREATED: