DPWA: add tests for install icon IPH

Add unit tests for install In-Product Help recording and triggering
functions.

Bug: 1149670
Change-Id: Ibb91438b6a12787b9eac6794ab8747bd13136009
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2545782Reviewed-by: Phillis Tang <phillis@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Phillis Tang <phillis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#834970}

chrome/browser/ui/views/page_action/pwa_install_view.cc

@@ -11,6 +11,7 @@
 #include "base/metrics/user_metrics.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/time/time.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/banners/app_banner_manager.h"
 #include "chrome/browser/profiles/profile.h"
@@ -48,7 +49,8 @@ constexpr base::FeatureParam<ExperimentIcon> kInstallIconParam{
 // Add x_ prefix so the IPH feature engagement tracker can ignore this.
 constexpr base::FeatureParam<int> kIphSiteEngagementThresholdParam{
     &feature_engagement::kIPHDesktopPwaInstallFeature,
-    "x_site_engagement_threshold", 10};
+    "x_site_engagement_threshold",
+    web_app::kIphFieldTrialParamDefaultSiteEngagementThreshold};
 
 }  // namespace
 
@@ -127,8 +129,8 @@ void PwaInstallView::OnIphClosed() {
           ->GetPrefs();
   base::UmaHistogramEnumeration("WebApp.InstallIphPromo.Result",
                                 web_app::InstallIphResult::kIgnored);
-  web_app::RecordInstallIphIgnored(prefs,
-                                   web_app::GenerateAppIdFromURL(start_url));
+  web_app::RecordInstallIphIgnored(
+      prefs, web_app::GenerateAppIdFromURL(start_url), base::Time::Now());
 }
 
 void PwaInstallView::OnExecuting(PageActionIconView::ExecuteSource source) {

chrome/browser/ui/views/page_action/pwa_install_view_browsertest.cc

@@ -8,6 +8,7 @@
 #include "base/run_loop.h"
 #include "base/test/bind.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
 #include "build/chromeos_buildflags.h"
 #include "chrome/browser/banners/test_app_banner_manager_desktop.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
@@ -18,15 +19,20 @@
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 #include "chrome/browser/ui/views/location_bar/star_view.h"
 #include "chrome/browser/ui/views/page_action/page_action_icon_view.h"
+#include "chrome/browser/ui/views/user_education/feature_promo_controller_views.h"
 #include "chrome/browser/ui/web_applications/web_app_dialog_utils.h"
 #include "chrome/browser/web_applications/components/app_registry_controller.h"
 #include "chrome/browser/web_applications/components/install_bounce_metric.h"
 #include "chrome/browser/web_applications/components/install_finalizer.h"
 #include "chrome/browser/web_applications/components/os_integration_manager.h"
+#include "chrome/browser/web_applications/components/web_app_helpers.h"
+#include "chrome/browser/web_applications/components/web_app_prefs_utils.h"
 #include "chrome/browser/web_applications/web_app_provider.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "components/feature_engagement/public/feature_constants.h"
+#include "components/feature_engagement/public/feature_list.h"
 #include "components/omnibox/browser/omnibox_popup_model.h"
 #include "components/omnibox/browser/omnibox_view.h"
 #include "components/webapps/installable/installable_metrics.h"
@@ -88,6 +94,20 @@ class PwaInstallViewBrowserTest : public extensions::ExtensionBrowserTest {
  public:
   PwaInstallViewBrowserTest()
       : https_server_(net::EmbeddedTestServer::TYPE_HTTPS) {
+    base::FieldTrialParams iph_demo_params;
+    iph_demo_params[feature_engagement::kIPHDemoModeFeatureChoiceParam] =
+        feature_engagement::kIPHDesktopPwaInstallFeature.name;
+    base::test::ScopedFeatureList::FeatureAndParams iph_demo(
+        feature_engagement::kIPHDemoMode, iph_demo_params);
+
+    // kIPHDemoMode will bypass IPH framework's triggering validation so that
+    // we can test PWA specific triggering logic.
+    features_.InitWithFeaturesAndParameters(
+        {{feature_engagement::kIPHDemoMode,
+          {{feature_engagement::kIPHDemoModeFeatureChoiceParam,
+            feature_engagement::kIPHDesktopPwaInstallFeature.name}}},
+         {feature_engagement::kIPHDesktopPwaInstallFeature, {}}},
+        {});
   }
   ~PwaInstallViewBrowserTest() override = default;
 
@@ -262,6 +282,7 @@ class PwaInstallViewBrowserTest : public extensions::ExtensionBrowserTest {
 
  private:
   web_app::ScopedOsHooksSuppress os_hooks_suppress_;
+  base::test::ScopedFeatureList features_;
 };
 
 // Tests that the plus icon is not shown when an existing app is installed and
@@ -622,6 +643,45 @@ IN_PROC_BROWSER_TEST_F(PwaInstallViewBrowserTest,
       "Manifest listing related chrome app"));
 }
 
+IN_PROC_BROWSER_TEST_F(PwaInstallViewBrowserTest, PwaIntallIphSiteEngagement) {
+  GURL app_url = GetInstallableAppURL();
+  bool installable = OpenTab(app_url).installable;
+  ASSERT_TRUE(installable);
+
+  FeaturePromoControllerViews* controller =
+      FeaturePromoControllerViews::GetForView(pwa_install_view_);
+  // IPH is not shown when the site is not highly engaged.
+  EXPECT_FALSE(controller->BubbleIsShowing(
+      feature_engagement::kIPHDesktopPwaInstallFeature));
+
+  // Manually set engagement score to be above IPH triggering threshold.
+  SiteEngagementService::Get(profile())->AddPointsForTesting(
+      app_url, web_app::kIphFieldTrialParamDefaultSiteEngagementThreshold + 1);
+  OpenTab(app_url);
+  EXPECT_TRUE(controller->BubbleIsShowing(
+      feature_engagement::kIPHDesktopPwaInstallFeature));
+}
+
+IN_PROC_BROWSER_TEST_F(PwaInstallViewBrowserTest, PwaIntallIphIgnored) {
+  GURL app_url = GetInstallableAppURL();
+  SiteEngagementService::Get(profile())->AddPointsForTesting(
+      app_url, web_app::kIphFieldTrialParamDefaultSiteEngagementThreshold + 1);
+  // Manually set IPH ignored here, because the IPH demo mode only let IPH be
+  // shown once in an user session.
+  web_app::RecordInstallIphIgnored(
+      profile()->GetPrefs(),
+      web_app::GenerateAppIdFromURL(app_banner_manager_->GetManifestStartUrl()),
+      base::Time::Now());
+  bool installable = OpenTab(app_url).installable;
+  ASSERT_TRUE(installable);
+
+  FeaturePromoControllerViews* controller =
+      FeaturePromoControllerViews::GetForView(pwa_install_view_);
+  // IPH is not shown when the IPH is ignored recently.
+  EXPECT_FALSE(controller->BubbleIsShowing(
+      feature_engagement::kIPHDesktopPwaInstallFeature));
+}
+
 #if BUILDFLAG(IS_CHROMEOS_ASH)
 // Omnibox install promotion should not show if prefer_related_applications is
 // true and an ARC app listed as related.

chrome/browser/ui/views/web_apps/pwa_confirmation_bubble_view.cc

@@ -8,6 +8,8 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_util.h"
+#include "base/time/time.h"
+#include "build/build_config.h"
 #include "chrome/browser/feature_engagement/tracker_factory.h"
 #include "chrome/browser/ui/browser_dialogs.h"
 #include "chrome/browser/ui/browser_finder.h"
@@ -187,7 +189,7 @@ void PWAConfirmationBubbleView::WindowClosing() {
         web_app::GenerateAppIdFromURL(web_app_info_->start_url);
     UMA_HISTOGRAM_ENUMERATION("WebApp.InstallIphPromo.Result",
                               web_app::InstallIphResult::kCanceled);
-    web_app::RecordInstallIphIgnored(prefs_, app_id);
+    web_app::RecordInstallIphIgnored(prefs_, app_id, base::Time::Now());
   }
   if (callback_) {
     DCHECK(web_app_info_);

chrome/browser/ui/views/web_apps/pwa_confirmation_bubble_view_browsertest.cc

@@ -17,9 +17,12 @@
 #include "chrome/browser/web_applications/components/web_app_prefs_utils.h"
 #include "chrome/browser/web_applications/components/web_application_info.h"
 #include "chrome/common/chrome_features.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "components/feature_engagement/public/feature_constants.h"
 #include "content/public/test/browser_test.h"
+#include "services/preferences/public/cpp/dictionary_value_update.h"
+#include "services/preferences/public/cpp/scoped_pref_update.h"
 
 class PWAConfirmationBubbleViewBrowserTest : public InProcessBrowserTest {
  public:
@@ -112,13 +115,70 @@ IN_PROC_BROWSER_TEST_F(PWAConfirmationBubbleViewBrowserTest,
 
   bubble_dialog->CancelDialog();
   loop.Run();
-  PrefService* prefs = Profile::FromBrowserContext(browser()
-                                                       ->tab_strip_model()
-                                                       ->GetActiveWebContents()
-                                                       ->GetBrowserContext())
-                           ->GetPrefs();
+  PrefService* pref_service =
+      Profile::FromBrowserContext(browser()
+                                      ->tab_strip_model()
+                                      ->GetActiveWebContents()
+                                      ->GetBrowserContext())
+          ->GetPrefs();
   web_app::AppId app_id = web_app::GenerateAppIdFromURL(start_url);
-  EXPECT_EQ(web_app::GetIntWebAppPref(prefs, app_id, web_app::kIphIgnoreCount)
-                .value(),
-            1);
+  EXPECT_EQ(
+      web_app::GetIntWebAppPref(pref_service, app_id, web_app::kIphIgnoreCount)
+          .value(),
+      1);
+  EXPECT_TRUE(web_app::GetTimeWebAppPref(pref_service, app_id,
+                                         web_app::kIphLastIgnoreTime)
+                  .has_value());
+  {
+    auto* dict =
+        pref_service->GetDictionary(prefs::kWebAppsAppAgnosticIphState);
+    EXPECT_EQ(dict->FindIntKey(web_app::kIphIgnoreCount).value_or(0), 1);
+    EXPECT_NE(dict->FindKey(web_app::kIphLastIgnoreTime), nullptr);
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(PWAConfirmationBubbleViewBrowserTest,
+                       AcceptDialogResetIphCounters) {
+  auto app_info = GetAppInfo();
+  GURL start_url = app_info->start_url;
+  web_app::AppId app_id = web_app::GenerateAppIdFromURL(start_url);
+  PrefService* pref_service =
+      Profile::FromBrowserContext(browser()
+                                      ->tab_strip_model()
+                                      ->GetActiveWebContents()
+                                      ->GetBrowserContext())
+          ->GetPrefs();
+  web_app::UpdateIntWebAppPref(pref_service, app_id, web_app::kIphIgnoreCount,
+                               1);
+  {
+    prefs::ScopedDictionaryPrefUpdate update(
+        pref_service, prefs::kWebAppsAppAgnosticIphState);
+    update->SetInteger(web_app::kIphIgnoreCount, 1);
+  }
+  base::RunLoop loop;
+  // Show the PWA install dialog.
+  chrome::ShowPWAInstallBubble(
+      browser()->tab_strip_model()->GetActiveWebContents(), std::move(app_info),
+      base::BindLambdaForTesting(
+          [&](bool accepted,
+              std::unique_ptr<WebApplicationInfo> app_info_callback) {
+            loop.Quit();
+          }),
+      chrome::PwaInProductHelpState::kShown);
+
+  PWAConfirmationBubbleView* bubble_dialog =
+      PWAConfirmationBubbleView::GetBubbleForTesting();
+
+  bubble_dialog->AcceptDialog();
+  loop.Run();
+
+  EXPECT_EQ(
+      web_app::GetIntWebAppPref(pref_service, app_id, web_app::kIphIgnoreCount)
+          .value(),
+      0);
+  {
+    auto* dict =
+        pref_service->GetDictionary(prefs::kWebAppsAppAgnosticIphState);
+    EXPECT_EQ(dict->FindIntKey(web_app::kIphIgnoreCount).value_or(0), 0);
+  }
 }

chrome/browser/web_applications/components/BUILD.gn

@@ -175,6 +175,7 @@ source_set("unit_tests") {
     "web_app_icon_downloader_unittest.cc",
     "web_app_icon_generator_unittest.cc",
     "web_app_install_utils_unittest.cc",
+    "web_app_prefs_utils_unittest.cc",
     "web_app_shortcut_unittest.cc",
     "web_app_url_loader_unittest.cc",
     "web_app_utils_unittest.cc",
@@ -217,6 +218,7 @@ source_set("unit_tests") {
     "//components/webapps",
     "//components/webapps:test_support",
     "//content/public/browser",
+    "//services/preferences/public/cpp",
     "//skia",
     "//testing/gmock",
     "//testing/gtest",

chrome/browser/web_applications/components/web_app_constants.h

@@ -237,6 +237,9 @@ constexpr int kIphMuteAfterConsecutiveAppAgnosticIgnores = 4;
 constexpr int kIphAppSpecificMuteTimeSpanDays = 90;
 // Number of days to mute IPH after it's ignored for any app.
 constexpr int kIphAppAgnosticMuteTimeSpanDays = 14;
+// Default threshold for site engagement score if it's not set by field trial
+// param.
+constexpr int kIphFieldTrialParamDefaultSiteEngagementThreshold = 10;
 
 }  // namespace web_app
 

chrome/browser/web_applications/components/web_app_prefs_utils.cc

@@ -212,14 +212,15 @@ void RemoveWebAppPref(PrefService* pref_service,
   web_app_prefs->Remove(path, nullptr);
 }
 
-void RecordInstallIphIgnored(PrefService* pref_service, const AppId& app_id) {
+void RecordInstallIphIgnored(PrefService* pref_service,
+                             const AppId& app_id,
+                             base::Time time) {
   base::Optional<int> ignored_count =
       GetIntWebAppPref(pref_service, app_id, kIphIgnoreCount);
   int new_count = base::saturated_cast<int>(1 + ignored_count.value_or(0));
 
   UpdateIntWebAppPref(pref_service, app_id, kIphIgnoreCount, new_count);
-  UpdateTimeWebAppPref(pref_service, app_id, kIphLastIgnoreTime,
-                       base::Time::Now());
+  UpdateTimeWebAppPref(pref_service, app_id, kIphLastIgnoreTime, time);
 
   prefs::ScopedDictionaryPrefUpdate update(pref_service,
                                            prefs::kWebAppsAppAgnosticIphState);
@@ -227,8 +228,8 @@ void RecordInstallIphIgnored(PrefService* pref_service, const AppId& app_id) {
   update->GetInteger(kIphIgnoreCount, &global_count);
   update->SetInteger(kIphIgnoreCount,
                      base::saturated_cast<int>(global_count + 1));
-  update->Set(kIphLastIgnoreTime, std::make_unique<base::Value>(
-                                      util::TimeToValue(base::Time::Now())));
+  update->Set(kIphLastIgnoreTime,
+              std::make_unique<base::Value>(util::TimeToValue(time)));
 }
 
 void RecordInstallIphInstalled(PrefService* pref_service, const AppId& app_id) {

chrome/browser/web_applications/components/web_app_prefs_utils.h

@@ -75,7 +75,9 @@ void RemoveWebAppPref(PrefService* pref_service,
 void WebAppPrefsUtilsRegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry);
 
-void RecordInstallIphIgnored(PrefService* pref_service, const AppId& app_id);
+void RecordInstallIphIgnored(PrefService* pref_service,
+                             const AppId& app_id,
+                             base::Time time);
 
 void RecordInstallIphInstalled(PrefService* pref_service, const AppId& app_id);
 

chrome/browser/web_applications/components/web_app_prefs_utils_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/web_applications/components/web_app_prefs_utils.h"
+
+#include "base/run_loop.h"
+#include "base/test/bind.h"
+#include "base/test/task_environment.h"
+#include "base/time/time.h"
+#include "base/util/values/values_util.h"
+#include "chrome/browser/web_applications/components/web_app_constants.h"
+#include "chrome/browser/web_applications/components/web_app_id.h"
+#include "chrome/common/pref_names.h"
+#include "components/sync_preferences/testing_pref_service_syncable.h"
+#include "content/public/browser/browser_task_traits.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/test/browser_task_environment.h"
+#include "services/preferences/public/cpp/dictionary_value_update.h"
+#include "services/preferences/public/cpp/scoped_pref_update.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace web_app {
+
+namespace {
+const AppId app_id = "test_app";
+const AppId app_id_2 = "test_app_2";
+const base::Time time_before_app_mute =
+    base::Time::Now() -
+    base::TimeDelta::FromDays(kIphAppSpecificMuteTimeSpanDays) -
+    base::TimeDelta::FromHours(1);
+const base::Time time_before_global_mute =
+    base::Time::Now() -
+    base::TimeDelta::FromDays(kIphAppAgnosticMuteTimeSpanDays) -
+    base::TimeDelta::FromHours(1);
+}  // namespace
+
+class WebAppPrefsUtilsTest : public testing::Test {
+ public:
+  WebAppPrefsUtilsTest() {
+    WebAppPrefsUtilsRegisterProfilePrefs(prefs_.registry());
+  }
+
+  sync_preferences::TestingPrefServiceSyncable* prefs() { return &prefs_; }
+
+ protected:
+  content::BrowserTaskEnvironment task_environment_;
+
+ private:
+  sync_preferences::TestingPrefServiceSyncable prefs_;
+};
+
+TEST_F(WebAppPrefsUtilsTest, TestIphIgnoreRecorded) {
+  EXPECT_FALSE(GetIntWebAppPref(prefs(), kIphIgnoreCount, app_id).has_value());
+  EXPECT_FALSE(
+      GetTimeWebAppPref(prefs(), app_id, kIphLastIgnoreTime).has_value());
+
+  RecordInstallIphIgnored(prefs(), app_id, base::Time::Now());
+  EXPECT_EQ(GetIntWebAppPref(prefs(), app_id, kIphIgnoreCount).value_or(0), 1);
+  auto last_ignore_time =
+      GetTimeWebAppPref(prefs(), app_id, kIphLastIgnoreTime);
+  EXPECT_TRUE(last_ignore_time.has_value());
+  {
+    auto* dict = prefs()->GetDictionary(prefs::kWebAppsAppAgnosticIphState);
+    EXPECT_EQ(dict->FindIntKey(kIphIgnoreCount).value_or(0), 1);
+    EXPECT_EQ(util::ValueToTime(dict->FindKey(kIphLastIgnoreTime)),
+              last_ignore_time.value());
+  }
+}
+
+TEST_F(WebAppPrefsUtilsTest, TestIphIgnoreRecordUpdated) {
+  RecordInstallIphIgnored(prefs(), app_id, base::Time::Now());
+  auto last_ignore_time =
+      GetTimeWebAppPref(prefs(), app_id, kIphLastIgnoreTime);
+  EXPECT_TRUE(last_ignore_time.has_value());
+
+  RecordInstallIphIgnored(prefs(), app_id, base::Time::Now());
+  EXPECT_EQ(GetIntWebAppPref(prefs(), app_id, kIphIgnoreCount).value_or(0), 2);
+  EXPECT_NE(GetTimeWebAppPref(prefs(), app_id, kIphLastIgnoreTime).value(),
+            last_ignore_time.value());
+  {
+    auto* dict = prefs()->GetDictionary(prefs::kWebAppsAppAgnosticIphState);
+    EXPECT_EQ(dict->FindIntKey(kIphIgnoreCount).value_or(0), 2);
+    EXPECT_NE(util::ValueToTime(dict->FindKey(kIphLastIgnoreTime)),
+              last_ignore_time.value());
+  }
+}
+
+TEST_F(WebAppPrefsUtilsTest, TestIphInstallResetCounters) {
+  RecordInstallIphIgnored(prefs(), app_id, base::Time::Now());
+  EXPECT_EQ(GetIntWebAppPref(prefs(), app_id, kIphIgnoreCount).value_or(0), 1);
+  {
+    auto* dict = prefs()->GetDictionary(prefs::kWebAppsAppAgnosticIphState);
+    EXPECT_EQ(dict->FindIntKey(kIphIgnoreCount).value_or(0), 1);
+  }
+  RecordInstallIphInstalled(prefs(), app_id);
+  EXPECT_EQ(GetIntWebAppPref(prefs(), app_id, kIphIgnoreCount).value_or(0), 0);
+  {
+    auto* dict = prefs()->GetDictionary(prefs::kWebAppsAppAgnosticIphState);
+    EXPECT_EQ(dict->FindIntKey(kIphIgnoreCount).value_or(0), 0);
+  }
+}
+
+TEST_F(WebAppPrefsUtilsTest, TestIphAppIgnoredRecently) {
+  EXPECT_TRUE(ShouldShowIph(prefs(), app_id));
+  RecordInstallIphIgnored(prefs(), app_id, base::Time::Now());
+  EXPECT_FALSE(ShouldShowIph(prefs(), app_id));
+}
+
+TEST_F(WebAppPrefsUtilsTest, TestIphGlobalIgnoredRecently) {
+  EXPECT_TRUE(ShouldShowIph(prefs(), app_id));
+  RecordInstallIphIgnored(prefs(), app_id_2, base::Time::Now());
+  EXPECT_FALSE(ShouldShowIph(prefs(), app_id));
+}
+
+TEST_F(WebAppPrefsUtilsTest, TestIphGlobalIgnoredPassedMuteTime) {
+  RecordInstallIphIgnored(prefs(), app_id_2, time_before_global_mute);
+  EXPECT_TRUE(ShouldShowIph(prefs(), app_id));
+}
+
+TEST_F(WebAppPrefsUtilsTest, TestIphAppIgnoredPassedMuteTime) {
+  RecordInstallIphIgnored(prefs(), app_id, time_before_app_mute);
+  EXPECT_TRUE(ShouldShowIph(prefs(), app_id));
+}
+
+TEST_F(WebAppPrefsUtilsTest, TestIphConsecutiveAppIgnore) {
+  RecordInstallIphIgnored(prefs(), app_id, time_before_app_mute);
+  UpdateIntWebAppPref(prefs(), app_id, kIphIgnoreCount,
+                      kIphMuteAfterConsecutiveAppSpecificIgnores);
+  EXPECT_FALSE(ShouldShowIph(prefs(), app_id));
+}
+
+TEST_F(WebAppPrefsUtilsTest, TestGlobalConsecutiveAppIgnore) {
+  RecordInstallIphIgnored(prefs(), app_id_2, time_before_global_mute);
+  {
+    prefs::ScopedDictionaryPrefUpdate update(
+        prefs(), prefs::kWebAppsAppAgnosticIphState);
+    update->SetInteger(kIphIgnoreCount,
+                       kIphMuteAfterConsecutiveAppAgnosticIgnores);
+  }
+  EXPECT_FALSE(ShouldShowIph(prefs(), app_id));
+}
+}  // namespace web_app