desktop pwas: Remove Webapp.Engagement.EngagementType histogram.
Erase
const char kPwaWindowEngagementTypeHistogram[] =
"Webapp.Engagement.EngagementType";
and all related browser tests in hosted_app_browsertest.cc
Do not observe SiteEngagement events in HostedAppBrowserController.
Bug: 930927, 918089
Change-Id: I7244b40ac0f6a9597f425057236edd8c159efa87
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1506674
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#639395}diff --git a/chrome/browser/ui/extensions/hosted_app_browser_controller.cc b/chrome/browser/ui/extensions/hosted_app_browser_controller.cc
index 5b49549..2941145 100644
--- a/chrome/browser/ui/extensions/hosted_app_browser_controller.cc
+++ b/chrome/browser/ui/extensions/hosted_app_browser_controller.cc
@@ -4,9 +4,7 @@
#include "chrome/browser/ui/extensions/hosted_app_browser_controller.h"
-#include "base/metrics/histogram_macros.h"
#include "base/strings/utf_string_conversions.h"
-#include "chrome/browser/engagement/site_engagement_service.h"
#include "chrome/browser/extensions/extension_util.h"
#include "chrome/browser/extensions/tab_helper.h"
#include "chrome/browser/profiles/profile.h"
@@ -112,10 +110,6 @@
profile, page_url, extensions::LAUNCH_CONTAINER_WINDOW);
}
-// TODO(loyso): Erase this histogram. crbug.com/918089.
-const char kPwaWindowEngagementTypeHistogram[] =
- "Webapp.Engagement.EngagementType";
-
// static
bool HostedAppBrowserController::IsForExperimentalHostedAppBrowser(
const Browser* browser) {
@@ -154,8 +148,7 @@
}
HostedAppBrowserController::HostedAppBrowserController(Browser* browser)
- : SiteEngagementObserver(SiteEngagementService::Get(browser->profile())),
- browser_(browser),
+ : browser_(browser),
extension_id_(web_app::GetAppIdFromApplicationName(browser->app_name())),
// If a bookmark app has a URL handler, then it is a PWA.
// TODO(https://crbug.com/774918): Replace once there is a more explicit
@@ -356,24 +349,6 @@
return GetExtension();
}
-void HostedAppBrowserController::OnEngagementEvent(
- content::WebContents* web_contents,
- const GURL& /*url*/,
- double /*score*/,
- SiteEngagementService::EngagementType type) {
- if (!created_for_installed_pwa_)
- return;
-
- // Check the event belongs to the controller's associated browser window.
- if (!web_contents ||
- web_contents != browser_->tab_strip_model()->GetActiveWebContents()) {
- return;
- }
-
- UMA_HISTOGRAM_ENUMERATION(kPwaWindowEngagementTypeHistogram, type,
- SiteEngagementService::ENGAGEMENT_LAST);
-}
-
void HostedAppBrowserController::OnTabStripModelChanged(
TabStripModel* tab_strip_model,
const TabStripModelChange& change,
diff --git a/chrome/browser/ui/extensions/hosted_app_browser_controller.h b/chrome/browser/ui/extensions/hosted_app_browser_controller.h
index 0f92234..645c0bb 100644
--- a/chrome/browser/ui/extensions/hosted_app_browser_controller.h
+++ b/chrome/browser/ui/extensions/hosted_app_browser_controller.h
@@ -10,7 +10,6 @@
#include "base/macros.h"
#include "base/optional.h"
#include "base/strings/string16.h"
-#include "chrome/browser/engagement/site_engagement_observer.h"
#include "chrome/browser/extensions/extension_uninstall_dialog.h"
#include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
#include "third_party/skia/include/core/SkColor.h"
@@ -30,14 +29,10 @@
const GURL& page_url,
content::BrowserContext* profile);
-// TODO(loyso): Erase this histogram. crbug.com/918089.
-extern const char kPwaWindowEngagementTypeHistogram[];
-
class Extension;
// Class to encapsulate logic to control the browser UI for hosted apps.
-class HostedAppBrowserController : public SiteEngagementObserver,
- public TabStripModelObserver,
+class HostedAppBrowserController : public TabStripModelObserver,
public ExtensionUninstallDialog::Delegate {
public:
// Returns whether |browser| uses the experimental hosted app experience.
@@ -115,12 +110,6 @@
// the lifetime of HostedAppBrowserController).
bool IsInstalled() const;
- // SiteEngagementObserver overrides.
- void OnEngagementEvent(content::WebContents* web_contents,
- const GURL& url,
- double score,
- SiteEngagementService::EngagementType type) override;
-
// TabStripModelObserver overrides.
void OnTabStripModelChanged(
TabStripModel* tab_strip_model,
diff --git a/chrome/browser/ui/extensions/hosted_app_browsertest.cc b/chrome/browser/ui/extensions/hosted_app_browsertest.cc
index b33d5ade..3a79644e 100644
--- a/chrome/browser/ui/extensions/hosted_app_browsertest.cc
+++ b/chrome/browser/ui/extensions/hosted_app_browsertest.cc
@@ -17,7 +17,6 @@
#include "base/strings/stringprintf.h"
#include "base/strings/utf_string_conversions.h"
#include "base/test/bind_test_util.h"
-#include "base/test/metrics/histogram_tester.h"
#include "base/test/scoped_feature_list.h"
#include "build/build_config.h"
#include "chrome/app/chrome_command_ids.h"
@@ -25,7 +24,6 @@
#include "chrome/browser/badging/badge_manager_delegate.h"
#include "chrome/browser/badging/badge_manager_factory.h"
#include "chrome/browser/banners/test_app_banner_manager_desktop.h"
-#include "chrome/browser/engagement/site_engagement_service.h"
#include "chrome/browser/extensions/extension_browsertest.h"
#include "chrome/browser/extensions/extension_service.h"
#include "chrome/browser/extensions/extension_util.h"
@@ -90,7 +88,6 @@
namespace {
constexpr const char kExampleURL[] = "http://example.org/";
-constexpr const char kExampleURL2[] = "http://example.com/";
constexpr const char kImagePath[] = "/ssl/google_files/logo.gif";
constexpr const char kAppDotComManifest[] =
"{"
@@ -1649,113 +1646,6 @@
NavigateAndCheckForToolbar(app_browser_, app_url, false);
}
-// TODO(loyso): crbug.com/918089. This test is deprecated in favor of
-// BookmarkAppTest.EngagementHistogramForAppInWindow and
-// BookmarkAppTest.EngagementHistogramForAppInTab.
-IN_PROC_BROWSER_TEST_P(HostedAppPWAOnlyTest, EngagementHistogram) {
- base::HistogramTester histograms;
- WebApplicationInfo web_app_info;
- web_app_info.app_url = GURL(kExampleURL);
- web_app_info.scope = GURL(kExampleURL);
- web_app_info.theme_color = base::Optional<SkColor>();
- const extensions::Extension* app = InstallBookmarkApp(web_app_info);
- Browser* app_browser = LaunchAppBrowser(app);
- NavigateToURLAndWait(app_browser, GURL(kExampleURL));
-
- // Test shortcut launch.
- EXPECT_EQ(web_app::GetAppIdFromApplicationName(app_browser->app_name()),
- app->id());
-
- histograms.ExpectUniqueSample(
- extensions::kPwaWindowEngagementTypeHistogram,
- SiteEngagementService::ENGAGEMENT_WEBAPP_SHORTCUT_LAUNCH, 1);
-
- // Test some other engagement events by directly calling into
- // SiteEngagementService.
- content::WebContents* web_contents =
- app_browser->tab_strip_model()->GetActiveWebContents();
- SiteEngagementService* site_engagement_service =
- SiteEngagementService::Get(app_browser->profile());
- site_engagement_service->HandleMediaPlaying(web_contents, false);
- site_engagement_service->HandleMediaPlaying(web_contents, true);
- site_engagement_service->HandleNavigation(web_contents,
- ui::PAGE_TRANSITION_TYPED);
- site_engagement_service->HandleUserInput(
- web_contents, SiteEngagementService::ENGAGEMENT_MOUSE);
-
- histograms.ExpectTotalCount(extensions::kPwaWindowEngagementTypeHistogram, 5);
- histograms.ExpectBucketCount(extensions::kPwaWindowEngagementTypeHistogram,
- SiteEngagementService::ENGAGEMENT_MEDIA_VISIBLE,
- 1);
- histograms.ExpectBucketCount(extensions::kPwaWindowEngagementTypeHistogram,
- SiteEngagementService::ENGAGEMENT_MEDIA_HIDDEN,
- 1);
- histograms.ExpectBucketCount(extensions::kPwaWindowEngagementTypeHistogram,
- SiteEngagementService::ENGAGEMENT_NAVIGATION, 1);
- histograms.ExpectBucketCount(extensions::kPwaWindowEngagementTypeHistogram,
- SiteEngagementService::ENGAGEMENT_MOUSE, 1);
-}
-
-// TODO(loyso): crbug.com/918089. This test is deprecated in favor of
-// BookmarkAppTest.EngagementHistogramAppWithoutScope and
-// BookmarkAppTest.EngagementHistogramRecordedForNonApps.
-IN_PROC_BROWSER_TEST_P(HostedAppPWAOnlyTest,
- EngagementHistogramNotRecordedIfNoScope) {
- base::HistogramTester histograms;
- WebApplicationInfo web_app_info;
- // App with no scope.
- web_app_info.app_url = GURL(kExampleURL);
- web_app_info.theme_color = base::Optional<SkColor>();
- const extensions::Extension* app = InstallBookmarkApp(web_app_info);
- Browser* app_browser = LaunchAppBrowser(app);
-
- EXPECT_EQ(web_app::GetAppIdFromApplicationName(app_browser->app_name()),
- app->id());
-
- histograms.ExpectTotalCount(extensions::kPwaWindowEngagementTypeHistogram, 0);
-}
-
-// TODO(loyso): crbug.com/918089. This test is deprecated in favor of
-// BookmarkAppTest.EngagementHistogramTwoApps.
-IN_PROC_BROWSER_TEST_P(HostedAppPWAOnlyTest, EngagementHistogramTwoApps) {
- base::HistogramTester histograms;
- const extensions::Extension *app1, *app2;
-
- // Install two apps.
- {
- WebApplicationInfo web_app_info;
- web_app_info.app_url = GURL(kExampleURL);
- web_app_info.scope = GURL(kExampleURL);
- web_app_info.theme_color = base::Optional<SkColor>();
- app1 = InstallBookmarkApp(web_app_info);
- }
- {
- WebApplicationInfo web_app_info;
- web_app_info.app_url = GURL(kExampleURL2);
- web_app_info.scope = GURL(kExampleURL2);
- web_app_info.theme_color = base::Optional<SkColor>();
- app2 = InstallBookmarkApp(web_app_info);
- }
-
- // Launch them three times. This ensures that each launch only logs once.
- // (Since all apps receive the notification on launch, there is a danger that
- // we might log too many times.)
- Browser* app_browser1 = LaunchAppBrowser(app1);
- Browser* app_browser2 = LaunchAppBrowser(app1);
- Browser* app_browser3 = LaunchAppBrowser(app2);
-
- EXPECT_EQ(web_app::GetAppIdFromApplicationName(app_browser1->app_name()),
- app1->id());
- EXPECT_EQ(web_app::GetAppIdFromApplicationName(app_browser2->app_name()),
- app1->id());
- EXPECT_EQ(web_app::GetAppIdFromApplicationName(app_browser3->app_name()),
- app2->id());
-
- histograms.ExpectUniqueSample(
- extensions::kPwaWindowEngagementTypeHistogram,
- SiteEngagementService::ENGAGEMENT_WEBAPP_SHORTCUT_LAUNCH, 3);
-}
-
// Common app manifest for HostedAppProcessModelTests.
constexpr const char kHostedAppProcessModelManifest[] =
R"( { "name": "Hosted App Process Model Test",
diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
index 40d25d3..a806ee7a 100644
--- a/tools/metrics/histograms/histograms.xml
+++ b/tools/metrics/histograms/histograms.xml
@@ -128183,6 +128183,10 @@
<histogram name="Webapp.Engagement.EngagementType"
enum="SiteEngagementServiceEngagementType" expires_after="2019-03-07">
+ <obsolete>
+ Removed 3/2019 because only counts PWAs. Superseded by the WebApp.Engagement
+ histogram with suffixes.
+ </obsolete>
<owner>calamity@chromium.org</owner>
<owner>mgiuca@chromium.org</owner>
<summary>
@@ -128190,9 +128194,7 @@
accumulation in site engagement within a PWA app window. Should be collected
for a subset of SiteEngagementService.EngagementType, which is triggered for
all browsing contexts. Recorded at the time of engagement accumulation
- (e.g., when mouse is clicked). Deprecated because only counts PWAs.
- Superseded by the WebApp.Engagement histogram with suffixes.
- http://crbug.com/918089.
+ (e.g., when mouse is clicked).
</summary>
</histogram>
