cros: consolidate uma record when executing app menu command

changes:
Consolidate uma recording when executing app menu command.
Both shelf and app list code record
Apps.ContextMenuExecuteCommand in AppMenuModelAdapter code
path. It was used to record in
SimpleMenuModel::RecordHistogram. However, app list doesn't
exercise through that path maybe due to the refactoring (it
calls delegate()'s code path now).

This CL fixes the regression. And it is more straightforward
to find where Apps.ContextMenuExecuteCommand UMA is recorded.

Bug: 846370
Test: manual test
Change-Id: Ib59ab96f2647d068af55118e150b81b3758a4a60
Reviewed-on: https://chromium-review.googlesource.com/1107720
Commit-Queue: Qiang Xu <warx@google.com>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569443}

ash/app_list/views/app_list_menu_model_adapter.cc

@@ -42,7 +42,7 @@ void AppListMenuModelAdapter::Build(
   menu_items_ = std::move(items);
 }
 
-void AppListMenuModelAdapter::RecordHistogram() {
+void AppListMenuModelAdapter::RecordHistogramOnMenuClosed() {
   const base::TimeDelta user_journey_time =
       base::TimeTicks::Now() - menu_open_time();
   switch (type_) {
@@ -97,6 +97,7 @@ bool AppListMenuModelAdapter::IsCommandEnabled(int id) const {
 
 void AppListMenuModelAdapter::ExecuteCommand(int id, int mouse_event_flags) {
   delegate_->ExecuteCommand(id, mouse_event_flags);
+  RecordExecuteCommandHistogram(id);
 }
 
 }  // namespace app_list

ash/app_list/views/app_list_menu_model_adapter.h

@@ -24,7 +24,8 @@ class APP_LIST_EXPORT AppListMenuModelAdapter
     : public ash::AppMenuModelAdapter {
  public:
   // The kinds of apps which show menus. This enum is used to record
-  // metrics, if a new value is added make sure to modify RecordHistogram().
+  // metrics, if a new value is added make sure to modify
+  // RecordHistogramOnMenuClosed().
   enum AppListViewAppType {
     FULLSCREEN_SEARCH_RESULT,
     FULLSCREEN_SUGGESTED,
@@ -54,7 +55,7 @@ class APP_LIST_EXPORT AppListMenuModelAdapter
   void Build(std::vector<ash::mojom::MenuItemPtr> items);
 
   // Overridden from AppMenuModelAdapter:
-  void RecordHistogram() override;
+  void RecordHistogramOnMenuClosed() override;
 
   // Overridden from views::MenuModelAdapter:
   bool IsItemChecked(int id) const override;

ash/app_menu/app_menu_model_adapter.cc

@@ -5,6 +5,7 @@
 #include "ash/app_menu/app_menu_model_adapter.h"
 
 #include "ash/app_menu/notification_menu_controller.h"
+#include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
 #include "ui/base/models/simple_menu_model.h"
 #include "ui/base/ui_base_features.h"
@@ -14,6 +15,20 @@
 
 namespace ash {
 
+namespace {
+
+// The UMA histogram that logs the commands which are executed on non-app
+// context menus.
+constexpr char kNonAppContextMenuExecuteCommand[] =
+    "Apps.ContextMenuExecuteCommand.NotFromApp";
+
+// The UMA histogram that logs the commands which are executed on app context
+// menus.
+constexpr char kAppContextMenuExecuteCommand[] =
+    "Apps.ContextMenuExecuteCommand.FromApp";
+
+}  // namespace
+
 AppMenuModelAdapter::AppMenuModelAdapter(
     const std::string& app_id,
     std::unique_ptr<ui::SimpleMenuModel> model,
@@ -66,12 +81,23 @@ base::TimeTicks AppMenuModelAdapter::GetClosingEventTime() {
   return menu_runner_->closing_event_time();
 }
 
+void AppMenuModelAdapter::ExecuteCommand(int id, int mouse_event_flags) {
+  views::MenuModelAdapter::ExecuteCommand(id, mouse_event_flags);
+  RecordExecuteCommandHistogram(id);
+}
+
 void AppMenuModelAdapter::OnMenuClosed(views::MenuItemView* menu) {
   DCHECK_NE(base::TimeTicks(), menu_open_time_);
-  RecordHistogram();
+  RecordHistogramOnMenuClosed();
 
   if (on_menu_closed_callback_)
     std::move(on_menu_closed_callback_).Run();
 }
 
+void AppMenuModelAdapter::RecordExecuteCommandHistogram(int command_id) {
+  base::UmaHistogramSparse(app_id().empty() ? kNonAppContextMenuExecuteCommand
+                                            : kAppContextMenuExecuteCommand,
+                           command_id);
+}
+
 }  // namespace ash

ash/app_menu/app_menu_model_adapter.h

@@ -48,12 +48,10 @@ class APP_MENU_EXPORT AppMenuModelAdapter : public views::MenuModelAdapter {
   // Closes the menu if one is being shown.
   void Cancel();
 
-  // Records the user journey time and show source histograms.
-  virtual void RecordHistogram() = 0;
-
   base::TimeTicks GetClosingEventTime();
 
   // Overridden from views::MenuModelAdapter:
+  void ExecuteCommand(int id, int mouse_event_flags) override;
   void OnMenuClosed(views::MenuItemView* menu) override;
 
  protected:
@@ -64,6 +62,13 @@ class APP_MENU_EXPORT AppMenuModelAdapter : public views::MenuModelAdapter {
   ui::SimpleMenuModel* model() { return model_.get(); }
   const ui::SimpleMenuModel* model() const { return model_.get(); }
 
+  // Helper method to record ExecuteCommand() histograms.
+  void RecordExecuteCommandHistogram(int command_id);
+
+  // Records histograms on menu closed, such as the user journey time and show
+  // source histograms.
+  virtual void RecordHistogramOnMenuClosed() = 0;
+
  private:
   // The application identifier used to fetch active notifications to display.
   const std::string app_id_;

ash/shelf/shelf_menu_model_adapter.cc

@@ -23,7 +23,7 @@ ShelfMenuModelAdapter::ShelfMenuModelAdapter(
 
 ShelfMenuModelAdapter::~ShelfMenuModelAdapter() = default;
 
-void ShelfMenuModelAdapter::RecordHistogram() {
+void ShelfMenuModelAdapter::RecordHistogramOnMenuClosed() {
   base::TimeDelta user_journey_time = base::TimeTicks::Now() - menu_open_time();
   // If the menu is for a ShelfButton.
   if (!app_id().empty()) {

ash/shelf/shelf_menu_model_adapter.h

@@ -22,7 +22,7 @@ class ASH_EXPORT ShelfMenuModelAdapter : public AppMenuModelAdapter {
   ~ShelfMenuModelAdapter() override;
 
   // Overridden from AppMenuModelAdapter:
-  void RecordHistogram() override;
+  void RecordHistogramOnMenuClosed() override;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ShelfMenuModelAdapter);

ash/shelf/shelf_view.cc

@@ -90,15 +90,6 @@ bool IsTabletModeEnabled() {
              ->tablet_mode_controller()
              ->IsTabletModeWindowManagerEnabled();
 }
-// The UMA histogram that logs the commands which are executed on non-app
-// context menus.
-constexpr char kNonAppContextMenuExecuteCommand[] =
-    "Apps.ContextMenuExecuteCommand.NotFromApp";
-
-// The UMA histogram that logs the commands which are executed on app context
-// menus.
-constexpr char kAppContextMenuExecuteCommand[] =
-    "Apps.ContextMenuExecuteCommand.FromApp";
 
 // A class to temporarily disable a given bounds animator.
 class BoundsAnimatorDisabler {
@@ -1941,9 +1932,6 @@ void ShelfView::AfterGetContextMenuItems(
       std::make_unique<ShelfContextMenuModel>(
           std::move(menu_items), model_->GetShelfItemDelegate(shelf_id),
           display_id);
-  menu_model->set_histogram_name(ShelfItemForView(source)
-                                     ? kAppContextMenuExecuteCommand
-                                     : kNonAppContextMenuExecuteCommand);
   ShowMenu(std::move(menu_model), source, point, true /* context_menu */,
            source_type);
 }
@@ -1959,7 +1947,6 @@ void ShelfView::ShowContextMenuForView(views::View* source,
     std::unique_ptr<ShelfContextMenuModel> menu_model =
         std::make_unique<ShelfContextMenuModel>(
             std::vector<mojom::MenuItemPtr>(), nullptr, display_id);
-    menu_model->set_histogram_name(kNonAppContextMenuExecuteCommand);
     ShowMenu(std::move(menu_model), source, point, true, source_type);
     return;
   }

chrome/browser/ui/app_list/app_context_menu.cc

@@ -31,7 +31,6 @@ AppContextMenu::~AppContextMenu() = default;
 void AppContextMenu::GetMenuModel(GetMenuModelCallback callback) {
   auto menu_model = std::make_unique<ui::SimpleMenuModel>(this);
   BuildMenu(menu_model.get());
-  menu_model->set_histogram_name("Apps.ContextMenuExecuteCommand.FromApp");
   std::move(callback).Run(std::move(menu_model));
 }
 

chrome/browser/ui/app_list/arc/arc_app_context_menu.cc

@@ -28,7 +28,6 @@ ArcAppContextMenu::~ArcAppContextMenu() = default;
 
 void ArcAppContextMenu::GetMenuModel(GetMenuModelCallback callback) {
   auto menu_model = std::make_unique<ui::SimpleMenuModel>(this);
-  menu_model->set_histogram_name("Apps.ContextMenuExecuteCommand.FromApp");
   BuildMenu(menu_model.get());
   if (!features::IsTouchableAppContextMenuEnabled()) {
     std::move(callback).Run(std::move(menu_model));