diff options
Diffstat (limited to 'chrome/browser/banners')
7 files changed, 21 insertions, 236 deletions
diff --git a/chrome/browser/banners/app_banner_data_fetcher.cc b/chrome/browser/banners/app_banner_data_fetcher.cc index 321e8d3..0090d10 100644 --- a/chrome/browser/banners/app_banner_data_fetcher.cc +++ b/chrome/browser/banners/app_banner_data_fetcher.cc @@ -188,9 +188,6 @@ void AppBannerDataFetcher::OnBannerPromptReply( return; } - AppBannerSettingsHelper::RecordMinutesFromFirstVisitToShow( - web_contents, validated_url_, GetAppIdentifier(), GetCurrentTime()); - // Definitely going to show the banner now. FOR_EACH_OBSERVER(Observer, observer_list_, OnDecidedWhetherToShow(this, true)); diff --git a/chrome/browser/banners/app_banner_data_fetcher_browsertest.cc b/chrome/browser/banners/app_banner_data_fetcher_browsertest.cc index c868268..b37df5f 100644 --- a/chrome/browser/banners/app_banner_data_fetcher_browsertest.cc +++ b/chrome/browser/banners/app_banner_data_fetcher_browsertest.cc @@ -9,10 +9,8 @@ #include "base/run_loop.h" #include "base/single_thread_task_runner.h" #include "base/task_runner.h" -#include "base/test/histogram_tester.h" #include "base/thread_task_runner_handle.h" #include "chrome/browser/banners/app_banner_data_fetcher_desktop.h" -#include "chrome/browser/banners/app_banner_metrics.h" #include "chrome/browser/banners/app_banner_settings_helper.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/tabs/tab_strip_model.h" @@ -99,7 +97,6 @@ class AppBannerDataFetcherBrowserTest : public InProcessBrowserTest, weak_factory_.GetWeakPtr(), 128, 128)); - base::HistogramTester histograms; base::RunLoop run_loop; quit_closure_ = run_loop.QuitClosure(); scoped_ptr<TestObserver> observer(new TestObserver(fetcher.get(), @@ -110,10 +107,6 @@ class AppBannerDataFetcherBrowserTest : public InProcessBrowserTest, EXPECT_EQ(expected_non_web_platform, non_web_platform_); EXPECT_EQ(expected_to_show, observer->will_show()); ASSERT_FALSE(fetcher->is_active()); - - // If showing the banner, ensure that the minutes histogram is recorded. - histograms.ExpectTotalCount(banners::kMinutesHistogram, - (observer->will_show() ? 1 : 0)); } void RunBannerTest(const std::string& manifest_page, diff --git a/chrome/browser/banners/app_banner_metrics.cc b/chrome/browser/banners/app_banner_metrics.cc index f9bbd34..5ab6f59 100644 --- a/chrome/browser/banners/app_banner_metrics.cc +++ b/chrome/browser/banners/app_banner_metrics.cc @@ -4,48 +4,32 @@ #include "chrome/browser/banners/app_banner_metrics.h" -#include "base/metrics/histogram_macros.h" #include "base/metrics/sparse_histogram.h" namespace banners { -const char kDismissEventHistogram[] = "AppBanners.DismissEvent"; -const char kDisplayEventHistogram[] = "AppBanners.DisplayEvent"; -const char kInstallEventHistogram[] = "AppBanners.InstallEvent"; -const char kMinutesHistogram[] = - "AppBanners.MinutesFromFirstVisitToBannerShown"; -const char kUserResponseHistogram[] = "AppBanners.UserResponse"; - void TrackDismissEvent(int event) { DCHECK_LT(DISMISS_EVENT_MIN, event); DCHECK_LT(event, DISMISS_EVENT_MAX); - UMA_HISTOGRAM_SPARSE_SLOWLY(kDismissEventHistogram, event); + UMA_HISTOGRAM_SPARSE_SLOWLY("AppBanners.DismissEvent", event); } void TrackDisplayEvent(int event) { DCHECK_LT(DISPLAY_EVENT_MIN, event); DCHECK_LT(event, DISPLAY_EVENT_MAX); - UMA_HISTOGRAM_SPARSE_SLOWLY(kDisplayEventHistogram, event); + UMA_HISTOGRAM_SPARSE_SLOWLY("AppBanners.DisplayEvent", event); } void TrackInstallEvent(int event) { DCHECK_LT(INSTALL_EVENT_MIN, event); DCHECK_LT(event, INSTALL_EVENT_MAX); - UMA_HISTOGRAM_SPARSE_SLOWLY(kInstallEventHistogram, event); -} - -void TrackMinutesFromFirstVisitToBannerShown(int minutes) { - // Histogram ranges from 1 minute to the number of minutes in 21 days. - // This is one more day than the decay length of time for site engagement, - // and seven more days than the expiry of visits for the app banner - // navigation heuristic. - UMA_HISTOGRAM_CUSTOM_COUNTS(kMinutesHistogram, minutes, 1, 30240, 100); + UMA_HISTOGRAM_SPARSE_SLOWLY("AppBanners.InstallEvent", event); } void TrackUserResponse(int event) { DCHECK_LT(USER_RESPONSE_MIN, event); DCHECK_LT(event, USER_RESPONSE_MAX); - UMA_HISTOGRAM_SPARSE_SLOWLY(kUserResponseHistogram, event); + UMA_HISTOGRAM_SPARSE_SLOWLY("AppBanners.UserResponse", event); } } // namespace banners diff --git a/chrome/browser/banners/app_banner_metrics.h b/chrome/browser/banners/app_banner_metrics.h index 0b0939e..7c78179 100644 --- a/chrome/browser/banners/app_banner_metrics.h +++ b/chrome/browser/banners/app_banner_metrics.h @@ -57,16 +57,9 @@ enum UserResponse { USER_RESPONSE_MAX = 7, }; -extern const char kDismissEventHistogram[]; -extern const char kDisplayEventHistogram[]; -extern const char kInstallEventHistogram[]; -extern const char kMinutesHistogram[]; -extern const char kUserResponseHistogram[]; - void TrackDismissEvent(int event); void TrackDisplayEvent(int event); void TrackInstallEvent(int event); -void TrackMinutesFromFirstVisitToBannerShown(int minutes); void TrackUserResponse(int event); }; // namespace banners diff --git a/chrome/browser/banners/app_banner_settings_helper.cc b/chrome/browser/banners/app_banner_settings_helper.cc index aca24f6..2f0bb43 100644 --- a/chrome/browser/banners/app_banner_settings_helper.cc +++ b/chrome/browser/banners/app_banner_settings_helper.cc @@ -8,14 +8,11 @@ #include <string> #include "base/command_line.h" -#include "base/metrics/field_trial.h" #include "base/strings/string_number_conversions.h" -#include "base/strings/string_util.h" #include "chrome/browser/banners/app_banner_data_fetcher.h" #include "chrome/browser/banners/app_banner_metrics.h" #include "chrome/browser/browser_process.h" #include "chrome/browser/content_settings/host_content_settings_map_factory.h" -#include "chrome/browser/engagement/site_engagement_service.h" #include "chrome/browser/profiles/profile.h" #include "chrome/common/chrome_switches.h" #include "components/content_settings/core/browser/host_content_settings_map.h" @@ -66,11 +63,9 @@ const char kBannerParamsDirectKey[] = "direct"; const char kBannerParamsIndirectKey[] = "indirect"; const char kBannerParamsTotalKey[] = "total"; const char kBannerParamsMinutesKey[] = "minutes"; -const char kBannerSiteEngagementParamsKey[] = "app_banner_triggering"; -const char kBannerSiteEngagementParamsTotalKey[] = - "app_banner_triggering_total"; -// Total engagement score required before a banner will actually be triggered. +// Total site engagements where a banner could have been shown before +// a banner will actually be triggered. double gTotalEngagementToTrigger = 2; // Engagement weight assigned to direct and indirect navigations. @@ -120,26 +115,10 @@ double GetEventEngagement(ui::PageTransition transition_type) { } } -// Queries variations for the maximum site engagement score required to trigger -// the banner showing. -void UpdateSiteEngagementToTrigger() { - std::string total_param = variations::GetVariationParamValue( - SiteEngagementService::kEngagementParams, - kBannerSiteEngagementParamsTotalKey); - - if (!total_param.empty()) { - double total_engagement = -1; - - if (base::StringToDouble(total_param, &total_engagement) && - total_engagement > 0) { - AppBannerSettingsHelper::SetTotalEngagementToTrigger(total_engagement); - } - } -} - // Queries variations for updates to the default engagement values assigned // to direct and indirect navigations. void UpdateEngagementWeights() { + std::map<std::string, std::string> params; std::string direct_param = variations::GetVariationParamValue( kBannerParamsKey, kBannerParamsDirectKey); std::string indirect_param = variations::GetVariationParamValue( @@ -181,16 +160,6 @@ void UpdateMinutesBetweenVisits() { } } -// Returns the site engagement karma score for the given origin URL under the -// current profile. -double GetSiteEngagementScoreForOrigin( - content::WebContents* web_contents, - const GURL& origin_url) { - SiteEngagementService* service = SiteEngagementService::Get( - Profile::FromBrowserContext(web_contents->GetBrowserContext())); - return service ? service->GetScore(origin_url) : 0; -} - } // namespace void AppBannerSettingsHelper::ClearHistoryForURLs( @@ -383,10 +352,6 @@ bool AppBannerSettingsHelper::ShouldShowBanner( return true; } - // Never show a banner when the package name or URL is empty. - if (package_name_or_start_url.empty()) - return false; - // Don't show if it has been added to the homescreen. base::Time added_time = GetSingleBannerEvent(web_contents, origin_url, package_name_or_start_url, @@ -417,17 +382,14 @@ bool AppBannerSettingsHelper::ShouldShowBanner( return false; } - double total_engagement = 0; - if (ShouldUseSiteEngagementScore()) { - total_engagement = - GetSiteEngagementScoreForOrigin(web_contents, origin_url); - } else { - std::vector<BannerEvent> could_show_events = GetCouldShowBannerEvents( - web_contents, origin_url, package_name_or_start_url); + std::vector<BannerEvent> could_show_events = GetCouldShowBannerEvents( + web_contents, origin_url, package_name_or_start_url); - for (const auto& event : could_show_events) - total_engagement += event.engagement; - } + // Return true if the total engagement of each applicable could show event + // meets the trigger threshold. + double total_engagement = 0; + for (const auto& event : could_show_events) + total_engagement += event.engagement; if (total_engagement < gTotalEngagementToTrigger) { banners::TrackDisplayEvent(banners::DISPLAY_EVENT_NOT_VISITED_ENOUGH); @@ -443,6 +405,8 @@ AppBannerSettingsHelper::GetCouldShowBannerEvents( const GURL& origin_url, const std::string& package_name_or_start_url) { std::vector<BannerEvent> result; + if (package_name_or_start_url.empty()) + return result; Profile* profile = Profile::FromBrowserContext(web_contents->GetBrowserContext()); @@ -490,6 +454,9 @@ base::Time AppBannerSettingsHelper::GetSingleBannerEvent( DCHECK(event != APP_BANNER_EVENT_COULD_SHOW); DCHECK(event < APP_BANNER_EVENT_NUM_EVENTS); + if (package_name_or_start_url.empty()) + return base::Time(); + Profile* profile = Profile::FromBrowserContext(web_contents->GetBrowserContext()); HostContentSettingsMap* settings = @@ -513,21 +480,6 @@ base::Time AppBannerSettingsHelper::GetSingleBannerEvent( return base::Time::FromInternalValue(internal_time); } -void AppBannerSettingsHelper::RecordMinutesFromFirstVisitToShow( - content::WebContents* web_contents, - const GURL& origin_url, - const std::string& package_name_or_start_url, - base::Time time) { - std::vector<BannerEvent> could_show_events = GetCouldShowBannerEvents( - web_contents, origin_url, package_name_or_start_url); - - int minutes = 0; - if (could_show_events.size()) - minutes = (time - could_show_events[0].time).InMinutes(); - - banners::TrackMinutesFromFirstVisitToBannerShown(minutes); -} - void AppBannerSettingsHelper::SetEngagementWeights(double direct_engagement, double indirect_engagement) { gDirectNavigationEngagement = direct_engagement; @@ -571,32 +523,6 @@ base::Time AppBannerSettingsHelper::BucketTimeToResolution( } void AppBannerSettingsHelper::UpdateFromFieldTrial() { - // If we are using the site engagement score, only extract the total - // engagement to trigger from the params variations. - if (ShouldUseSiteEngagementScore()) { - UpdateSiteEngagementToTrigger(); - } else { - UpdateEngagementWeights(); - UpdateMinutesBetweenVisits(); - } -} - -bool AppBannerSettingsHelper::ShouldUseSiteEngagementScore() { - if (base::CommandLine::ForCurrentProcess()->HasSwitch( - switches::kEnableSiteEngagementAppBanner)) { - return true; - } - - // This experiment is controlled under the same key as the broader site - // engagement experiment rather than the banner experiment. This avoids cross - // pollution with other site engagement experiments. However, this experiment - // must only be active when there is one singular group under the banner - // experiment, otherwise the banner and site engagement banner experiments - // will conflict. - // - // Making the experiment active when a variations key is present allows us - // to have experiments which enable multiple features under site engagement. - std::string param = variations::GetVariationParamValue( - SiteEngagementService::kEngagementParams, kBannerSiteEngagementParamsKey); - return !param.empty(); + UpdateEngagementWeights(); + UpdateMinutesBetweenVisits(); } diff --git a/chrome/browser/banners/app_banner_settings_helper.h b/chrome/browser/banners/app_banner_settings_helper.h index b46bc16..3ff877c 100644 --- a/chrome/browser/banners/app_banner_settings_helper.h +++ b/chrome/browser/banners/app_banner_settings_helper.h @@ -117,14 +117,6 @@ class AppBannerSettingsHelper { const std::string& package_name_or_start_url, AppBannerEvent event); - // Record a UMA statistic measuring the minutes between the first visit to the - // site and the first showing of the banner. - static void RecordMinutesFromFirstVisitToShow( - content::WebContents* web_contents, - const GURL& origin_url, - const std::string& package_name_or_start_url, - base::Time time); - // Set the engagement weights assigned to direct and indirect navigations. static void SetEngagementWeights(double direct_engagement, double indirect_engagement); @@ -145,10 +137,6 @@ class AppBannerSettingsHelper { // Updates all values from field trial. static void UpdateFromFieldTrial(); - // Returns true if the app banner trigger condition should use the site - // engagement score instead of the navigation-based heuristic. - static bool ShouldUseSiteEngagementScore(); - private: DISALLOW_IMPLICIT_CONSTRUCTORS(AppBannerSettingsHelper); }; diff --git a/chrome/browser/banners/app_banner_settings_helper_unittest.cc b/chrome/browser/banners/app_banner_settings_helper_unittest.cc index 254758a..82550f1 100644 --- a/chrome/browser/banners/app_banner_settings_helper_unittest.cc +++ b/chrome/browser/banners/app_banner_settings_helper_unittest.cc @@ -4,14 +4,8 @@ #include <vector> -#include "base/command_line.h" -#include "chrome/browser/banners/app_banner_metrics.h" #include "chrome/browser/banners/app_banner_settings_helper.h" -#include "chrome/browser/engagement/site_engagement_service.h" -#include "chrome/browser/engagement/site_engagement_service_factory.h" -#include "chrome/common/chrome_switches.h" #include "chrome/test/base/chrome_render_view_host_test_harness.h" -#include "chrome/test/base/testing_profile.h" #include "ui/base/page_transition_types.h" namespace { @@ -737,93 +731,3 @@ TEST_F(AppBannerSettingsHelperTest, OperatesOnOrigins) { EXPECT_TRUE(AppBannerSettingsHelper::ShouldShowBanner( web_contents(), url, kTestPackageName, reference_time)); } - -TEST_F(AppBannerSettingsHelperTest, ShouldShowWithHigherTotal) { - AppBannerSettingsHelper::SetEngagementWeights(1, 1); - AppBannerSettingsHelper::SetTotalEngagementToTrigger(5); - GURL url(kTestURL); - NavigateAndCommit(url); - - base::Time reference_time = GetReferenceTime(); - base::Time second_day = reference_time + base::TimeDelta::FromDays(1); - base::Time third_day = reference_time + base::TimeDelta::FromDays(2); - base::Time fourth_day = reference_time + base::TimeDelta::FromDays(3); - base::Time fifth_day = reference_time + base::TimeDelta::FromDays(4); - - EXPECT_FALSE(AppBannerSettingsHelper::ShouldShowBanner( - web_contents(), url, kTestPackageName, reference_time)); - - // It should take five visits to trigger the banner. - AppBannerSettingsHelper::RecordBannerCouldShowEvent( - web_contents(), url, kTestPackageName, reference_time, - ui::PAGE_TRANSITION_LINK); - EXPECT_FALSE(AppBannerSettingsHelper::ShouldShowBanner( - web_contents(), url, kTestPackageName, reference_time)); - - AppBannerSettingsHelper::RecordBannerCouldShowEvent( - web_contents(), url, kTestPackageName, second_day, - ui::PAGE_TRANSITION_TYPED); - EXPECT_FALSE(AppBannerSettingsHelper::ShouldShowBanner( - web_contents(), url, kTestPackageName, second_day)); - - AppBannerSettingsHelper::RecordBannerCouldShowEvent( - web_contents(), url, kTestPackageName, third_day, - ui::PAGE_TRANSITION_GENERATED); - EXPECT_FALSE(AppBannerSettingsHelper::ShouldShowBanner( - web_contents(), url, kTestPackageName, third_day)); - - AppBannerSettingsHelper::RecordBannerCouldShowEvent( - web_contents(), url, kTestPackageName, fourth_day, - ui::PAGE_TRANSITION_LINK); - EXPECT_FALSE(AppBannerSettingsHelper::ShouldShowBanner( - web_contents(), url, kTestPackageName, fourth_day)); - - // Visit the site again; now it should be shown. - AppBannerSettingsHelper::RecordBannerCouldShowEvent( - web_contents(), url, kTestPackageName, fifth_day, - ui::PAGE_TRANSITION_TYPED); - EXPECT_TRUE(AppBannerSettingsHelper::ShouldShowBanner( - web_contents(), url, kTestPackageName, fifth_day)); -} - -// Test that the banner triggers correctly using site engagement. -TEST_F(AppBannerSettingsHelperTest, SiteEngagementTrigger) { - AppBannerSettingsHelper::SetTotalEngagementToTrigger(2); - base::CommandLine* command_line = base::CommandLine::ForCurrentProcess(); - command_line->AppendSwitch(switches::kEnableSiteEngagementAppBanner); - - SiteEngagementService* service = - SiteEngagementServiceFactory::GetForProfile(profile()); - DCHECK(service); - - // Not used, but needed for method call. - base::Time reference_time = GetReferenceTime(); - GURL url(kTestURL); - - EXPECT_FALSE(AppBannerSettingsHelper::ShouldShowBanner( - web_contents(), url, kTestPackageName, reference_time)); - - service->AddPoints(url, 1); - EXPECT_FALSE(AppBannerSettingsHelper::ShouldShowBanner( - web_contents(), url, kTestPackageName, reference_time)); - - service->AddPoints(url, 1); - EXPECT_TRUE(AppBannerSettingsHelper::ShouldShowBanner( - web_contents(), url, kTestPackageName, reference_time)); - - AppBannerSettingsHelper::SetTotalEngagementToTrigger(5); - EXPECT_FALSE(AppBannerSettingsHelper::ShouldShowBanner( - web_contents(), url, kTestPackageName, reference_time)); - - service->AddPoints(url, 1.5); - EXPECT_FALSE(AppBannerSettingsHelper::ShouldShowBanner( - web_contents(), url, kTestPackageName, reference_time)); - - service->AddPoints(url, 0.5); - EXPECT_FALSE(AppBannerSettingsHelper::ShouldShowBanner( - web_contents(), url, kTestPackageName, reference_time)); - - service->AddPoints(url, 1.5); - EXPECT_TRUE(AppBannerSettingsHelper::ShouldShowBanner( - web_contents(), url, kTestPackageName, reference_time)); -} |