[Launcher impressions] Refactor ChromeSearchResult::GetSearchResulType

Most search result data is stored in a SearchResultMetadata object, which is synced between the ash and chrome objects representing results. One exception is the ash::SearchResultType enum which, confusingly named, is only used for metrics. Currently this enum is available via a virtual method on the chrome-side only. To fix a race condition in the impressions system, we now need to treat it like other search result data. This CL adds methods to SearchResultMetadata, deletes the old chrome getter, updates all ChromeSearchResult subclasses to correctly set the metadata. The ChromeSearchResult subclass updates are mostly trivial, these have slightly more logic: - chrome_search_result itself - arc_app_data_search_result - omnibox_result Bug: 1076270 Change-Id: I1b1da0e3248031df3f31c2cd8b622dfb0487e390 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2352256 Reviewed-by: Xiyuan Xia <xiyuan@chromium.org> Reviewed-by: Thanh Nguyen <thanhdng@chromium.org> Commit-Queue: Tony Yeoman <tby@chromium.org> Cr-Commit-Position: refs/heads/master@{#799446}
marcosholgado · Aug 19, 2020 · aca16d8 · aca16d8
1 parent a39ef6a
commit aca16d8
Show file tree

Hide file tree

Showing 39 changed files with 41 additions and 104 deletions.
diff --git a/ash/public/cpp/app_list/app_list_types.h b/ash/public/cpp/app_list/app_list_types.h
@@ -8,6 +8,7 @@
 #include <string>
 #include <vector>
 
+#include "ash/public/cpp/app_list/app_list_metrics.h"
 #include "ash/public/cpp/ash_public_export.h"
 #include "base/optional.h"
 #include "base/strings/string16.h"
@@ -275,6 +276,9 @@ struct ASH_PUBLIC_EXPORT SearchResultMetadata {
   // indicates no subtype has been set.
   int result_subtype = -1;
 
+  // A search result type used for metrics.
+  ash::SearchResultType metrics_type = ash::SEARCH_RESULT_TYPE_BOUNDARY;
+
   // Which UI container(s) the result should be displayed in.
   SearchResultDisplayType display_type = SearchResultDisplayType::kList;
 

diff --git a/chrome/browser/ui/app_list/app_list_client_impl.cc b/chrome/browser/ui/app_list/app_list_client_impl.cc
@@ -129,15 +129,15 @@ void AppListClientImpl::OpenSearchResult(const std::string& result_id,
   // Send training signal to search controller.
   search_controller_->Train(std::move(app_launch_data));
 
-  RecordSearchResultOpenTypeHistogram(
-      launched_from, result->GetSearchResultType(), IsTabletMode());
+  RecordSearchResultOpenTypeHistogram(launched_from, result->metrics_type(),
+                                      IsTabletMode());
 
   if (launch_as_default)
-    RecordDefaultSearchResultOpenTypeHistogram(result->GetSearchResultType());
+    RecordDefaultSearchResultOpenTypeHistogram(result->metrics_type());
 
   if (!search_controller_->GetLastQueryLength() &&
       launched_from == ash::AppListLaunchedFrom::kLaunchedFromSearchBox)
-    RecordZeroStateSuggestionOpenTypeHistogram(result->GetSearchResultType());
+    RecordZeroStateSuggestionOpenTypeHistogram(result->metrics_type());
 
   // OpenResult may cause |result| to be deleted.
   search_controller_->OpenResult(result, event_flags);

diff --git a/chrome/browser/ui/app_list/search/answer_card/answer_card_result.cc b/chrome/browser/ui/app_list/search/answer_card/answer_card_result.cc
@@ -19,6 +19,7 @@ AnswerCardResult::AnswerCardResult(Profile* profile,
   DCHECK(!stripped_search_result_url.is_empty());
   SetDisplayType(ash::SearchResultDisplayType::kCard);
   SetResultType(ash::AppListSearchResultType::kAnswerCard);
+  SetMetricsType(ash::ANSWER_CARD);
   SetQueryUrl(potential_card_url);
   SetEquivalentResutlId(stripped_search_result_url.spec());
   set_id(potential_card_url.spec());
@@ -33,8 +34,4 @@ void AnswerCardResult::Open(int event_flags) {
                             ui::DispositionFromEventFlags(event_flags));
 }
 
-ash::SearchResultType AnswerCardResult::GetSearchResultType() const {
-  return ash::ANSWER_CARD;
-}
-
 }  // namespace app_list
diff --git a/chrome/browser/ui/app_list/search/answer_card/answer_card_result.h b/chrome/browser/ui/app_list/search/answer_card/answer_card_result.h
@@ -30,8 +30,6 @@ class AnswerCardResult : public ChromeSearchResult {
 
   void Open(int event_flags) override;
 
-  ash::SearchResultType GetSearchResultType() const override;
-
   const GURL& search_result_url() const { return search_result_url_; }
 
  private:

diff --git a/chrome/browser/ui/app_list/search/app_result.cc b/chrome/browser/ui/app_list/search/app_result.cc
@@ -18,6 +18,7 @@ AppResult::AppResult(Profile* profile,
       controller_(controller) {
   SetDisplayType(ash::SearchResultDisplayType::kTile);
   SetResultType(ash::AppListSearchResultType::kInstalledApp);
+  SetMetricsType(ash::SEARCH_RESULT_TYPE_BOUNDARY);
   SetIsRecommendation(is_recommendation);
 }
 
@@ -41,8 +42,4 @@ void AppResult::UpdateFromLastLaunchedOrInstalledTime(
   set_relevance(1 / (weeks + 1));
 }
 
-ash::SearchResultType AppResult::GetSearchResultType() const {
-  return ash::SEARCH_RESULT_TYPE_BOUNDARY;
-}
-
 }  // namespace app_list
diff --git a/chrome/browser/ui/app_list/search/app_result.h b/chrome/browser/ui/app_list/search/app_result.h
@@ -34,8 +34,6 @@ class AppResult : public ChromeSearchResult, public AppContextMenuDelegate {
 
   const std::string& app_id() const { return app_id_; }
 
-  ash::SearchResultType GetSearchResultType() const override;
-
   base::WeakPtr<AppResult> GetWeakPtr() {
     return weak_ptr_factory_.GetWeakPtr();
   }

diff --git a/chrome/browser/ui/app_list/search/app_service_app_result.cc b/chrome/browser/ui/app_list/search/app_service_app_result.cc
@@ -58,6 +58,8 @@ AppServiceAppResult::AppServiceAppResult(Profile* profile,
     CallLoadIcon(true, allow_placeholder_icon);
   }
 
+  SetMetricsType(GetSearchResultType());
+
   switch (app_type_) {
     case apps::mojom::AppType::kBuiltIn:
       set_id(app_id);

diff --git a/chrome/browser/ui/app_list/search/app_service_app_result.h b/chrome/browser/ui/app_list/search/app_service_app_result.h
@@ -43,12 +43,12 @@ class AppServiceAppResult : public AppResult {
   void Open(int event_flags) override;
   void GetContextMenuModel(GetMenuModelCallback callback) override;
   void OnVisibilityChanged(bool visibility) override;
-  ash::SearchResultType GetSearchResultType() const override;
   AppContextMenu* GetAppContextMenu() override;
 
   // AppContextMenuDelegate overrides:
   void ExecuteLaunchCommand(int event_flags) override;
 
+  ash::SearchResultType GetSearchResultType() const;
   void Launch(int event_flags, apps::mojom::LaunchSource launch_source);
 
   void CallLoadIcon(bool chip, bool allow_placeholder_icon);

diff --git a/chrome/browser/ui/app_list/search/arc/arc_app_data_search_result.cc b/chrome/browser/ui/app_list/search/arc/arc_app_data_search_result.cc
@@ -86,9 +86,14 @@ ArcAppDataSearchResult::ArcAppDataSearchResult(
   set_id(kAppDataSearchPrefix + launch_intent_uri());
   if (data_->type == arc::mojom::AppDataResultType::PERSON) {
     SetDisplayType(ash::SearchResultDisplayType::kTile);
+    SetMetricsType(ash::APP_DATA_RESULT_PERSON);
   } else if (data_->type == arc::mojom::AppDataResultType::NOTE_DOCUMENT) {
     SetDetails(base::UTF8ToUTF16(data_->text));
     SetDisplayType(ash::SearchResultDisplayType::kList);
+    SetMetricsType(ash::APP_DATA_RESULT_NOTE_DOCUMENT);
+  } else {
+    NOTREACHED();
+    SetMetricsType(ash::SEARCH_RESULT_TYPE_BOUNDARY);
   }
 
   // TODO(warx): set default images when icon_png_data() is not available.
@@ -144,16 +149,4 @@ void ArcAppDataSearchResult::ApplyIcon(const gfx::ImageSkia& icon) {
   SetIcon(icon);
 }
 
-ash::SearchResultType ArcAppDataSearchResult::GetSearchResultType() const {
-  switch (data_->type) {
-    case arc::mojom::AppDataResultType::PERSON:
-      return ash::APP_DATA_RESULT_PERSON;
-    case arc::mojom::AppDataResultType::NOTE_DOCUMENT:
-      return ash::APP_DATA_RESULT_NOTE_DOCUMENT;
-    default:
-      NOTREACHED();
-      return ash::SEARCH_RESULT_TYPE_BOUNDARY;
-  }
-}
-
 }  // namespace app_list
diff --git a/chrome/browser/ui/app_list/search/arc/arc_app_data_search_result.h b/chrome/browser/ui/app_list/search/arc/arc_app_data_search_result.h
@@ -32,7 +32,6 @@ class ArcAppDataSearchResult : public ChromeSearchResult {
   // ChromeSearchResult:
   void GetContextMenuModel(GetMenuModelCallback callback) override;
   void Open(int event_flags) override;
-  ash::SearchResultType GetSearchResultType() const override;
 
  private:
   const std::string& launch_intent_uri() const {

diff --git a/chrome/browser/ui/app_list/search/arc/arc_app_reinstall_app_result.cc b/chrome/browser/ui/app_list/search/arc/arc_app_reinstall_app_result.cc
@@ -33,6 +33,7 @@ ArcAppReinstallAppResult::ArcAppReinstallAppResult(
   SetResultType(ash::AppListSearchResultType::kPlayStoreReinstallApp);
   SetTitle(base::UTF8ToUTF16(mojom_data->title));
   SetDisplayType(ash::SearchResultDisplayType::kTile);
+  SetMetricsType(ash::PLAY_STORE_REINSTALL_APP);
   SetDisplayIndex(ash::SearchResultDisplayIndex::kSixthIndex);
   SetIsRecommendation(true);
   set_relevance(kAppReinstallRelevance);
@@ -59,8 +60,4 @@ void ArcAppReinstallAppResult::OnVisibilityChanged(bool visibility) {
   observer_->OnVisibilityChanged(package_name_, visibility);
 }
 
-ash::SearchResultType ArcAppReinstallAppResult::GetSearchResultType() const {
-  return ash::PLAY_STORE_REINSTALL_APP;
-}
-
 }  // namespace app_list
diff --git a/chrome/browser/ui/app_list/search/arc/arc_app_reinstall_app_result.h b/chrome/browser/ui/app_list/search/arc/arc_app_reinstall_app_result.h
@@ -39,7 +39,6 @@ class ArcAppReinstallAppResult : public ChromeSearchResult {
   // ChromeSearchResult:
   void Open(int event_flags) override;
   void OnVisibilityChanged(bool visibility) override;
-  ash::SearchResultType GetSearchResultType() const override;
 
  private:
   // Observer passed in constructor. not owned.

diff --git a/chrome/browser/ui/app_list/search/arc/arc_app_reinstall_search_provider_unittest.cc b/chrome/browser/ui/app_list/search/arc/arc_app_reinstall_search_provider_unittest.cc
@@ -155,9 +155,6 @@ class TestSearchResult : public ChromeSearchResult {
     // set_id is protected in chromesearchresult.
     ChromeSearchResult::set_id(str);
   }
-  ash::SearchResultType GetSearchResultType() const override {
-    return ash::SEARCH_RESULT_TYPE_BOUNDARY;
-  }
 };
 }  // namespace
 

diff --git a/chrome/browser/ui/app_list/search/arc/arc_app_shortcut_search_result.cc b/chrome/browser/ui/app_list/search/arc/arc_app_shortcut_search_result.cc
@@ -41,6 +41,7 @@ ArcAppShortcutSearchResult::ArcAppShortcutSearchResult(
   SetAccessibleName(ComputeAccessibleName());
   SetResultType(ash::AppListSearchResultType::kArcAppShortcut);
   SetDisplayType(ash::SearchResultDisplayType::kTile);
+  SetMetricsType(ash::PLAY_STORE_APP_SHORTCUT);
   SetIsRecommendation(is_recommendation);
 
   const int icon_dimension =
@@ -81,10 +82,6 @@ void ArcAppShortcutSearchResult::Open(int event_flags) {
                              list_controller_->GetAppListDisplayId());
 }
 
-ash::SearchResultType ArcAppShortcutSearchResult::GetSearchResultType() const {
-  return ash::PLAY_STORE_APP_SHORTCUT;
-}
-
 void ArcAppShortcutSearchResult::OnAppImageUpdated(
     const std::string& app_id,
     const gfx::ImageSkia& image) {

diff --git a/chrome/browser/ui/app_list/search/arc/arc_app_shortcut_search_result.h b/chrome/browser/ui/app_list/search/arc/arc_app_shortcut_search_result.h
@@ -38,7 +38,6 @@ class ArcAppShortcutSearchResult : public ChromeSearchResult,
 
   // ChromeSearchResult:
   void Open(int event_flags) override;
-  ash::SearchResultType GetSearchResultType() const override;
 
  private:
   // AppIconLoaderDelegate:

diff --git a/chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc b/chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.cc
@@ -122,6 +122,8 @@ ArcPlayStoreSearchResult::ArcPlayStoreSearchResult(
   SetRating(review_score());
   SetResultType(is_instant_app() ? ash::AppListSearchResultType::kInstantApp
                                  : ash::AppListSearchResultType::kPlayStoreApp);
+  SetMetricsType(is_instant_app() ? ash::PLAY_STORE_INSTANT_APP
+                                  : ash::PLAY_STORE_UNINSTALLED_APP);
 
   if (base::FeatureList::IsEnabled(features::kAppServiceAdaptiveIcon)) {
     apps::ArcRawIconPngDataToImageSkia(
@@ -160,11 +162,6 @@ void ArcPlayStoreSearchResult::Open(int event_flags) {
                list_controller_->GetAppListDisplayId());
 }
 
-ash::SearchResultType ArcPlayStoreSearchResult::GetSearchResultType() const {
-  return is_instant_app() ? ash::PLAY_STORE_INSTANT_APP
-                          : ash::PLAY_STORE_UNINSTALLED_APP;
-}
-
 void ArcPlayStoreSearchResult::GetContextMenuModel(
     GetMenuModelCallback callback) {
   context_menu_ = std::make_unique<ArcPlayStoreAppContextMenu>(

diff --git a/chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.h b/chrome/browser/ui/app_list/search/arc/arc_playstore_search_result.h
@@ -37,7 +37,6 @@ class ArcPlayStoreSearchResult : public ChromeSearchResult,
   // ChromeSearchResult overrides:
   void GetContextMenuModel(GetMenuModelCallback callback) override;
   void Open(int event_flags) override;
-  ash::SearchResultType GetSearchResultType() const override;
 
   // app_list::AppContextMenuDelegate overrides:
   void ExecuteLaunchCommand(int event_flags) override;

diff --git a/chrome/browser/ui/app_list/search/assistant_search_provider.cc b/chrome/browser/ui/app_list/search/assistant_search_provider.cc
@@ -79,6 +79,7 @@ class AssistantSearchResult : public ChromeSearchResult {
     SetDisplayIndex(ash::SearchResultDisplayIndex::kFirstIndex);
     SetDisplayType(ash::SearchResultDisplayType::kChip);
     SetResultType(ash::AppListSearchResultType::kAssistantChip);
+    SetMetricsType(ash::SearchResultType::ASSISTANT);
     SetTitle(base::UTF8ToUTF16(conversation_starter.text));
     SetChipIcon(gfx::CreateVectorIcon(
         ash::kAssistantIcon,
@@ -97,11 +98,6 @@ class AssistantSearchResult : public ChromeSearchResult {
   ~AssistantSearchResult() override = default;
 
  private:
-  // ChromeSearchResult:
-  ash::SearchResultType GetSearchResultType() const override {
-    return ash::SearchResultType::ASSISTANT;
-  }
-
   void Open(int event_flags) override {
     // Opening of |action_url_| is delegated to the Assistant controller as only
     // the Assistant controller knows how to handle Assistant deep links.

diff --git a/chrome/browser/ui/app_list/search/assistant_search_provider_unittest.cc b/chrome/browser/ui/app_list/search/assistant_search_provider_unittest.cc
@@ -45,7 +45,7 @@ class Expect {
     EXPECT_EQ(r_.display_index(), ash::SearchResultDisplayIndex::kFirstIndex);
     EXPECT_EQ(r_.display_type(), ash::SearchResultDisplayType::kChip);
     EXPECT_EQ(r_.result_type(), ash::AppListSearchResultType::kAssistantChip);
-    EXPECT_EQ(r_.GetSearchResultType(), ash::SearchResultType::ASSISTANT);
+    EXPECT_EQ(r_.metrics_type(), ash::SearchResultType::ASSISTANT);
     EXPECT_TRUE(r_.chip_icon().BackedBySameObjectAs(gfx::CreateVectorIcon(
         ash::kAssistantIcon,
         ash::AppListConfig::instance().suggestion_chip_icon_dimension(),

diff --git a/chrome/browser/ui/app_list/search/chrome_search_result.cc b/chrome/browser/ui/app_list/search/chrome_search_result.cc
@@ -84,6 +84,11 @@ void ChromeSearchResult::SetResultType(ResultType result_type) {
   SetSearchResultMetadata();
 }
 
+void ChromeSearchResult::SetMetricsType(MetricsType metrics_type) {
+  metadata_->metrics_type = metrics_type;
+  SetSearchResultMetadata();
+}
+
 void ChromeSearchResult::SetDisplayIndex(DisplayIndex display_index) {
   metadata_->display_index = display_index;
   SetSearchResultMetadata();

diff --git a/chrome/browser/ui/app_list/search/chrome_search_result.h b/chrome/browser/ui/app_list/search/chrome_search_result.h
@@ -34,6 +34,7 @@ class ChromeSearchResult {
  public:
   using ResultType = ash::AppListSearchResultType;
   using DisplayType = ash::SearchResultDisplayType;
+  using MetricsType = ash::SearchResultType;
   using Tag = ash::SearchResultTag;
   using Tags = ash::SearchResultTags;
   using Action = ash::SearchResultAction;
@@ -59,6 +60,7 @@ class ChromeSearchResult {
   ash::AppListSearchResultType result_type() const {
     return metadata_->result_type;
   }
+  MetricsType metrics_type() const { return metadata_->metrics_type; }
   DisplayIndex display_index() const { return metadata_->display_index; }
   float position_priority() const { return metadata_->position_priority; }
   const Actions& actions() const { return metadata_->actions; }
@@ -88,6 +90,7 @@ class ChromeSearchResult {
   void SetFormattedPrice(const base::string16& formatted_price);
   void SetDisplayType(DisplayType display_type);
   void SetResultType(ResultType result_type);
+  void SetMetricsType(MetricsType metrics_type);
   void SetDisplayIndex(DisplayIndex display_index);
   void SetPositionPriority(float position_priority);
   void SetDisplayScore(double display_score);
@@ -155,9 +158,6 @@ class ChromeSearchResult {
   // subtype after construction is not reflected in ash.
   int result_subtype() const { return metadata_->result_subtype; }
 
-  // Get the type of the result, used in metrics.
-  virtual ash::SearchResultType GetSearchResultType() const = 0;
-
  protected:
   // These id setters should be called in derived class constructors only.
   void set_id(const std::string& id) { metadata_->id = id; }

diff --git a/chrome/browser/ui/app_list/search/drive_quick_access_result.cc b/chrome/browser/ui/app_list/search/drive_quick_access_result.cc
@@ -22,10 +22,7 @@ DriveQuickAccessResult::DriveQuickAccessResult(const base::FilePath& filepath,
     : ZeroStateFileResult(filepath, relevance, profile) {
   set_id(kDriveQuickAccessResultPrefix + filepath.value());
   SetResultType(ResultType::kDriveQuickAccess);
-}
-
-ash::SearchResultType DriveQuickAccessResult::GetSearchResultType() const {
-  return ash::DRIVE_QUICK_ACCESS;
+  SetMetricsType(ash::DRIVE_QUICK_ACCESS);
 }
 
 }  // namespace app_list
diff --git a/chrome/browser/ui/app_list/search/drive_quick_access_result.h b/chrome/browser/ui/app_list/search/drive_quick_access_result.h
@@ -17,9 +17,6 @@ class DriveQuickAccessResult : public ZeroStateFileResult {
   DriveQuickAccessResult(const base::FilePath& filepath,
                          float relevance,
                          Profile* profile);
-
-  // ZeroStateFileResult:
-  ash::SearchResultType GetSearchResultType() const override;
 };
 
 }  // namespace app_list

diff --git a/chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc b/chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.cc
@@ -56,10 +56,6 @@ void LauncherSearchResult::Open(int event_flags) {
   service->OnOpenResult(extension_->id(), item_id_);
 }
 
-ash::SearchResultType LauncherSearchResult::GetSearchResultType() const {
-  return ash::LAUNCHER_SEARCH_PROVIDER_RESULT;
-}
-
 LauncherSearchResult::LauncherSearchResult(
     const std::string& item_id,
     const std::string& icon_type,
@@ -81,6 +77,7 @@ void LauncherSearchResult::Initialize() {
                     chromeos::launcher_search_provider::kMaxSearchResultScore));
   SetDetails(base::UTF8ToUTF16(extension_->name()));
   SetResultType(ResultType::kLauncher);
+  SetMetricsType(ash::LAUNCHER_SEARCH_PROVIDER_RESULT);
 
   SetIcon(GetIconFromType(icon_type_));
 }

diff --git a/chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.h b/chrome/browser/ui/app_list/search/launcher_search/launcher_search_result.h
@@ -32,7 +32,6 @@ class LauncherSearchResult : public ChromeSearchResult {
 
   // ChromeSearchResult overrides:
   void Open(int event_flags) override;
-  ash::SearchResultType GetSearchResultType() const override;
 
  private:
   // Constructor for duplicating a result.

diff --git a/chrome/browser/ui/app_list/search/omnibox_result.cc b/chrome/browser/ui/app_list/search/omnibox_result.cc
@@ -144,6 +144,7 @@ OmniboxResult::OmniboxResult(Profile* profile,
   set_id(match_.stripped_destination_url.spec());
   SetResultType(ash::AppListSearchResultType::kOmnibox);
   set_result_subtype(static_cast<int>(match_.type));
+  SetMetricsType(GetSearchResultType());
 
   // Derive relevance from omnibox relevance and normalize it to [0, 1].
   // The magic number 1500 is the highest score of an omnibox result.

diff --git a/chrome/browser/ui/app_list/search/omnibox_result.h b/chrome/browser/ui/app_list/search/omnibox_result.h
@@ -40,7 +40,6 @@ class OmniboxResult : public ChromeSearchResult {
   // ChromeSearchResult overrides:
   void Open(int event_flags) override;
   void InvokeAction(int action_index, int event_flags) override;
-  ash::SearchResultType GetSearchResultType() const override;
 
   // Returns the URL that will be navigated to by this search result.
   GURL DestinationURL() const;
@@ -59,6 +58,8 @@ class OmniboxResult : public ChromeSearchResult {
 
   void RecordOmniboxResultHistogram();
 
+  ash::SearchResultType GetSearchResultType() const;
+
   Profile* profile_;
   AppListControllerDelegate* list_controller_;
   AutocompleteController* autocomplete_controller_;