Fix bug between id() and package_name in ArcAppReinstallAppResult.

This results in impression counts and open records for packages being
mis-recorded.

I've tested this fix to the bug and note that impressions of >5 are
indeed hidden now.

Bug: 960468
Change-Id: If0047fb0b9f8eb7a888032b778f374eff136c5a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1638200Reviewed-by: Jenny Zhang <jennyz@chromium.org>
Commit-Queue: Rob Schonberger <robsc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#665051}

chrome/browser/ui/app_list/search/arc/arc_app_reinstall_app_result.cc

@@ -27,7 +27,7 @@ ArcAppReinstallAppResult::ArcAppReinstallAppResult(
     const arc::mojom::AppReinstallCandidatePtr& mojom_data,
     const gfx::ImageSkia& skia_icon,
     Observer* observer)
-    : observer_(observer) {
+    : observer_(observer), package_name_(mojom_data->package_name) {
   DCHECK(observer_);
   set_id(kPlayStoreAppUrlPrefix + mojom_data->package_name);
   SetResultType(ash::SearchResultType::kPlayStoreReinstallApp);
@@ -49,12 +49,12 @@ ArcAppReinstallAppResult::~ArcAppReinstallAppResult() = default;
 void ArcAppReinstallAppResult::Open(int /*event_flags*/) {
   RecordAction(base::UserMetricsAction("ArcAppReinstall_Clicked"));
   arc::LaunchPlayStoreWithUrl(id());
-  observer_->OnOpened(id());
+  observer_->OnOpened(package_name_);
 }
 
 void ArcAppReinstallAppResult::OnVisibilityChanged(bool visibility) {
   ChromeSearchResult::OnVisibilityChanged(visibility);
-  observer_->OnVisibilityChanged(id(), visibility);
+  observer_->OnVisibilityChanged(package_name_, visibility);
 }
 
 SearchResultType ArcAppReinstallAppResult::GetSearchResultType() const {

chrome/browser/ui/app_list/search/arc/arc_app_reinstall_app_result.h

@@ -22,8 +22,8 @@ class ArcAppReinstallAppResult : public ChromeSearchResult {
  public:
   class Observer {
    public:
-    virtual void OnOpened(const std::string& package_id) = 0;
-    virtual void OnVisibilityChanged(const std::string& package_id,
+    virtual void OnOpened(const std::string& package_name) = 0;
+    virtual void OnVisibilityChanged(const std::string& package_name,
                                      bool visibility) = 0;
 
    protected:
@@ -44,7 +44,7 @@ class ArcAppReinstallAppResult : public ChromeSearchResult {
  private:
   // Observer passed in constructor. not owned.
   Observer* const observer_;
-
+  const std::string package_name_;
   DISALLOW_COPY_AND_ASSIGN(ArcAppReinstallAppResult);
 };
 

chrome/browser/ui/app_list/search/arc/arc_app_reinstall_search_provider.cc

@@ -464,16 +464,18 @@ void ArcAppReinstallSearchProvider::SetTimerForTesting(
   app_fetch_timer_ = std::move(timer);
 }
 
-void ArcAppReinstallSearchProvider::OnOpened(const std::string& id) {
-  UpdateStateTime(profile_, id, kOpenTime);
+void ArcAppReinstallSearchProvider::OnOpened(const std::string& package_name) {
+  UpdateStateTime(profile_, package_name, kOpenTime);
   int64_t impression_count;
-  if (GetStateInt64(profile_, id, kImpressionCount, &impression_count)) {
+  if (GetStateInt64(profile_, package_name, kImpressionCount,
+                    &impression_count)) {
     UMA_HISTOGRAM_COUNTS_100(kAppListImpressionsBeforeOpen, impression_count);
   }
   UpdateResults();
 }
 
-void ArcAppReinstallSearchProvider::OnVisibilityChanged(const std::string& id,
+void ArcAppReinstallSearchProvider::OnVisibilityChanged(
+    const std::string& package_name,
     bool visibility) {
   if (!visibility) {
     // do not update state when showing, update when we hide.
@@ -485,17 +487,21 @@ void ArcAppReinstallSearchProvider::OnVisibilityChanged(const std::string& id,
   const base::TimeDelta now = base::Time::Now().ToDeltaSinceWindowsEpoch();
   base::TimeDelta latest_impression;
   int64_t impression_count;
-  if (!GetStateInt64(profile_, id, kImpressionCount, &impression_count)) {
+  if (!GetStateInt64(profile_, package_name, kImpressionCount,
+                     &impression_count)) {
     impression_count = 0;
   }
   // Get impression count and time. If neither is set, set them.
   // If they're set, update if appropriate.
-  if (!GetStateTime(profile_, id, kImpressionTime, &latest_impression) ||
+  if (!GetStateTime(profile_, package_name, kImpressionTime,
+                    &latest_impression) ||
       impression_count == 0 ||
       (now - latest_impression >
        base::TimeDelta::FromSeconds(kNewImpressionTime.Get()))) {
-    UpdateStateTime(profile_, id, kImpressionTime);
-    SetStateInt64(profile_, id, kImpressionCount, impression_count + 1);
+    UpdateStateTime(profile_, package_name, kImpressionTime);
+    SetStateInt64(profile_, package_name, kImpressionCount,
+                  impression_count + 1);
+    UpdateResults();
   }
 }
 

chrome/browser/ui/app_list/search/arc/arc_app_reinstall_search_provider.h

@@ -80,9 +80,10 @@ class ArcAppReinstallSearchProvider
   void SetTimerForTesting(std::unique_ptr<base::RepeatingTimer> timer);
 
   // ArcAppReinstallAppResult::Observer:
-  void OnOpened(const std::string& id) override;
+  void OnOpened(const std::string& package_name) override;
 
-  void OnVisibilityChanged(const std::string& id, bool visibility) override;
+  void OnVisibilityChanged(const std::string& package_name,
+                           bool visibility) override;
 
   static void RegisterProfilePrefs(PrefRegistrySimple* registry);
  private: