Commit 825e89b7 authored by Balazs Engedy's avatar Balazs Engedy Committed by Commit Bot

Refactor ContextualNotificationPermissionUiSelector.

This CL introduces no behavior changes, it just moves from a model with:

  (UiToUse + QuietUiReason)

to:

  Decision = (QuietUiReason + WarningReason),

where either being base::nullopt indicates that the normal UI should be
used, or no warning should be printed, respectively.

Bug: 1087005, 1081233
Change-Id: I90563d2fc245f2f25a132566f394d74dd22c2f70
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2218309
Commit-Queue: Balazs Engedy <engedy@chromium.org>
Reviewed-by: default avatarBalazs Engedy <engedy@chromium.org>
Reviewed-by: default avatarEvan Stade <estade@chromium.org>
Reviewed-by: default avatarAndy Paicu <andypaicu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#772654}
parent c997c869
......@@ -28,9 +28,9 @@
namespace {
using UiToUse = ContextualNotificationPermissionUiSelector::UiToUse;
using QuietUiReason = ContextualNotificationPermissionUiSelector::QuietUiReason;
using UiToUseWithReason = std::pair<UiToUse, base::Optional<QuietUiReason>>;
using WarningReason = ContextualNotificationPermissionUiSelector::WarningReason;
using Decision = ContextualNotificationPermissionUiSelector::Decision;
// Records a histogram sample for NotificationUserExperienceQuality.
void RecordNotificationUserExperienceQuality(
......@@ -53,8 +53,9 @@ void RecordWarningOnlyState(bool value) {
// Attempts to decide which UI to use based on preloaded site reputation data,
// or returns base::nullopt if not possible. |site_reputation| can be nullptr.
base::Optional<UiToUseWithReason> GetUiToUseBasedOnSiteReputation(
base::Optional<Decision> GetDecisionBasedOnSiteReputation(
const CrowdDenyPreloadData::SiteReputation* site_reputation) {
using Config = QuietNotificationPermissionUiConfig;
if (!site_reputation) {
RecordNotificationUserExperienceQuality(
CrowdDenyPreloadData::SiteReputation::UNKNOWN);
......@@ -66,26 +67,28 @@ base::Optional<UiToUseWithReason> GetUiToUseBasedOnSiteReputation(
RecordWarningOnlyState(site_reputation->warning_only());
switch (site_reputation->notification_ux_quality()) {
case CrowdDenyPreloadData::SiteReputation::ACCEPTABLE:
return std::make_pair(UiToUse::kNormalUi, base::nullopt);
case CrowdDenyPreloadData::SiteReputation::UNSOLICITED_PROMPTS:
if (!QuietNotificationPermissionUiConfig::IsCrowdDenyTriggeringEnabled())
return base::nullopt;
case CrowdDenyPreloadData::SiteReputation::ACCEPTABLE: {
return Decision::UseNormalUiAndShowNoWarning();
}
case CrowdDenyPreloadData::SiteReputation::UNSOLICITED_PROMPTS: {
if (site_reputation->warning_only())
return std::make_pair(UiToUse::kNormalUi, base::nullopt);
return std::make_pair(UiToUse::kQuietUi,
QuietUiReason::kTriggeredByCrowdDeny);
case CrowdDenyPreloadData::SiteReputation::ABUSIVE_PROMPTS:
if (!QuietNotificationPermissionUiConfig::
IsAbusiveRequestBlockingEnabled()) {
return Decision::UseNormalUiAndShowNoWarning();
if (!Config::IsCrowdDenyTriggeringEnabled())
return base::nullopt;
}
return Decision(QuietUiReason::kTriggeredByCrowdDeny,
Decision::ShowNoWarning());
}
case CrowdDenyPreloadData::SiteReputation::ABUSIVE_PROMPTS: {
if (site_reputation->warning_only())
return std::make_pair(UiToUse::kNormalUi, base::nullopt);
return std::make_pair(UiToUse::kQuietUi,
QuietUiReason::kTriggeredDueToAbusiveRequests);
case CrowdDenyPreloadData::SiteReputation::UNKNOWN:
return Decision::UseNormalUiAndShowNoWarning();
if (!Config::IsAbusiveRequestBlockingEnabled())
return base::nullopt;
return Decision(QuietUiReason::kTriggeredDueToAbusiveRequests,
Decision::ShowNoWarning());
}
case CrowdDenyPreloadData::SiteReputation::UNKNOWN: {
return base::nullopt;
}
}
NOTREACHED();
......@@ -125,7 +128,7 @@ void ContextualNotificationPermissionUiSelector::SelectUiToUse(
DCHECK(callback_);
if (!base::FeatureList::IsEnabled(features::kQuietNotificationPrompts)) {
Notify(UiToUse::kNormalUi, base::nullopt);
Notify(Decision::UseNormalUiAndShowNoWarning());
return;
}
......@@ -146,15 +149,16 @@ ContextualNotificationPermissionUiSelector::
void ContextualNotificationPermissionUiSelector::EvaluatePerSiteTriggers(
const url::Origin& origin) {
auto ui_to_use_with_reason = GetUiToUseBasedOnSiteReputation(
base::Optional<Decision> decision = GetDecisionBasedOnSiteReputation(
CrowdDenyPreloadData::GetInstance()->GetReputationDataForSite(origin));
if (!ui_to_use_with_reason ||
ui_to_use_with_reason->first == UiToUse::kNormalUi) {
OnPerSiteTriggersEvaluated(UiToUse::kNormalUi, base::nullopt);
// If the PreloadData suggests this is an unacceptable site, ping Safe
// Browsing to verify; but do not ping if it is not warranted.
if (!decision || (!decision->quiet_ui_reason && !decision->warning_reason)) {
OnPerSiteTriggersEvaluated(Decision::UseNormalUiAndShowNoWarning());
return;
}
// PreloadData suggests an unacceptable site, ping Safe Browsing to verify.
DCHECK(!safe_browsing_request_);
DCHECK(g_browser_process->safe_browsing_service());
......@@ -165,11 +169,11 @@ void ContextualNotificationPermissionUiSelector::EvaluatePerSiteTriggers(
base::DefaultClock::GetInstance(), origin,
base::BindOnce(&ContextualNotificationPermissionUiSelector::
OnSafeBrowsingVerdictReceived,
base::Unretained(this), *ui_to_use_with_reason->second));
base::Unretained(this), *decision));
}
void ContextualNotificationPermissionUiSelector::OnSafeBrowsingVerdictReceived(
QuietUiReason candidate_quiet_ui_reason,
Decision candidate_decision,
CrowdDenySafeBrowsingRequest::Verdict verdict) {
DCHECK(safe_browsing_request_);
DCHECK(callback_);
......@@ -178,39 +182,34 @@ void ContextualNotificationPermissionUiSelector::OnSafeBrowsingVerdictReceived(
switch (verdict) {
case CrowdDenySafeBrowsingRequest::Verdict::kAcceptable:
OnPerSiteTriggersEvaluated(UiToUse::kNormalUi, base::nullopt);
OnPerSiteTriggersEvaluated(Decision::UseNormalUiAndShowNoWarning());
break;
case CrowdDenySafeBrowsingRequest::Verdict::kUnacceptable:
OnPerSiteTriggersEvaluated(UiToUse::kQuietUi, candidate_quiet_ui_reason);
if (candidate_decision.quiet_ui_reason &&
ShouldHoldBackQuietUI(*(candidate_decision.quiet_ui_reason))) {
candidate_decision.quiet_ui_reason.reset();
}
OnPerSiteTriggersEvaluated(candidate_decision);
break;
}
}
void ContextualNotificationPermissionUiSelector::OnPerSiteTriggersEvaluated(
UiToUse ui_to_use,
base::Optional<QuietUiReason> quiet_ui_reason) {
if (ui_to_use == UiToUse::kQuietUi &&
!ShouldHoldBackQuietUI(*quiet_ui_reason)) {
Notify(UiToUse::kQuietUi, quiet_ui_reason);
return;
}
Decision decision) {
// Still show the quiet UI if it is enabled for all sites, even if per-site
// conditions did not trigger showing the quiet UI on this origin.
if (profile_->GetPrefs()->GetBoolean(
if (!decision.quiet_ui_reason &&
profile_->GetPrefs()->GetBoolean(
prefs::kEnableQuietNotificationPermissionUi)) {
Notify(UiToUse::kQuietUi, QuietUiReason::kEnabledInPrefs);
return;
decision.quiet_ui_reason = QuietUiReason::kEnabledInPrefs;
}
Notify(UiToUse::kNormalUi, base::nullopt);
Notify(decision);
}
void ContextualNotificationPermissionUiSelector::Notify(
UiToUse ui_to_use,
base::Optional<QuietUiReason> quiet_ui_reason) {
DCHECK_EQ(ui_to_use == UiToUse::kQuietUi, !!quiet_ui_reason);
std::move(callback_).Run(ui_to_use, quiet_ui_reason);
const Decision& decision) {
std::move(callback_).Run(decision);
}
// static
......@@ -52,13 +52,10 @@ class ContextualNotificationPermissionUiSelector
void EvaluatePerSiteTriggers(const url::Origin& origin);
void OnSafeBrowsingVerdictReceived(
QuietUiReason candidate_quiet_ui_reason,
Decision candidate_decision,
CrowdDenySafeBrowsingRequest::Verdict verdict);
void OnPerSiteTriggersEvaluated(
UiToUse ui_to_use,
base::Optional<QuietUiReason> quiet_ui_reason);
void Notify(UiToUse ui_to_use, base::Optional<QuietUiReason> quiet_ui_reason);
void OnPerSiteTriggersEvaluated(Decision decision);
void Notify(const Decision& decision);
Profile* profile_;
......
......@@ -56,7 +56,8 @@ class TestQuietNotificationPermissionUiSelector
// permissions::NotificationPermissionUiSelector:
void SelectUiToUse(permissions::PermissionRequest* request,
DecisionMadeCallback callback) override {
std::move(callback).Run(UiToUse::kQuietUi, simulated_reason_for_quiet_ui_);
std::move(callback).Run(
Decision(simulated_reason_for_quiet_ui_, base::nullopt));
}
private:
......
......@@ -12,6 +12,7 @@ source_set("permissions") {
"contexts/window_placement_permission_context.h",
"features.cc",
"features.h",
"notification_permission_ui_selector.cc",
"notification_permission_ui_selector.h",
"permission_context_base.cc",
"permission_context_base.h",
......
// 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 "components/permissions/notification_permission_ui_selector.h"
namespace permissions {
NotificationPermissionUiSelector::Decision::Decision(
base::Optional<QuietUiReason> quiet_ui_reason,
base::Optional<WarningReason> warning_reason)
: quiet_ui_reason(quiet_ui_reason), warning_reason(warning_reason) {}
NotificationPermissionUiSelector::Decision::~Decision() = default;
NotificationPermissionUiSelector::Decision::Decision(const Decision&) = default;
NotificationPermissionUiSelector::Decision&
NotificationPermissionUiSelector::Decision::operator=(const Decision&) =
default;
// static
NotificationPermissionUiSelector::Decision
NotificationPermissionUiSelector::Decision::UseNormalUiAndShowNoWarning() {
return Decision(UseNormalUi(), ShowNoWarning());
}
} // namespace permissions
......@@ -5,30 +5,58 @@
#ifndef COMPONENTS_PERMISSIONS_NOTIFICATION_PERMISSION_UI_SELECTOR_H_
#define COMPONENTS_PERMISSIONS_NOTIFICATION_PERMISSION_UI_SELECTOR_H_
#include "base/callback_forward.h"
#include "base/optional.h"
#include "components/permissions/permission_request.h"
namespace permissions {
// The interface for implementations that decide if the quiet prompt UI should
// be used to display a notification permission |request|.
// be used to display a notification permission |request|, whether a warning
// should be printed to the Dev Tools console, and the reasons for both.
//
// Implementations of interface are expected to have long-lived instances that
// can support multiple requests, but only one at a time.
class NotificationPermissionUiSelector {
public:
enum class UiToUse {
kNormalUi,
kQuietUi,
};
enum class QuietUiReason {
kEnabledInPrefs,
kTriggeredByCrowdDeny,
kTriggeredDueToAbusiveRequests,
};
using DecisionMadeCallback =
base::OnceCallback<void(UiToUse, base::Optional<QuietUiReason>)>;
enum class WarningReason {
kAbusiveRequests,
};
struct Decision {
Decision(base::Optional<QuietUiReason> quiet_ui_reason,
base::Optional<WarningReason> warning_reason);
~Decision();
Decision(const Decision&);
Decision& operator=(const Decision&);
static constexpr base::Optional<QuietUiReason> UseNormalUi() {
return base::nullopt;
}
static constexpr base::Optional<WarningReason> ShowNoWarning() {
return base::nullopt;
}
static Decision UseNormalUiAndShowNoWarning();
// The reason for showing the quiet UI, or `base::nullopt` if the normal UI
// should be used.
base::Optional<QuietUiReason> quiet_ui_reason;
// The reason for printing a warning to the console, or `base::nullopt` if
// no warning should be printed.
base::Optional<WarningReason> warning_reason;
};
using DecisionMadeCallback = base::OnceCallback<void(const Decision&)>;
virtual ~NotificationPermissionUiSelector() {}
......
......@@ -371,7 +371,8 @@ void PermissionRequestManager::DequeueRequestIfNeeded() {
&PermissionRequestManager::OnSelectedUiToUseForNotifications,
weak_factory_.GetWeakPtr()));
} else {
current_request_ui_to_use_ = UiToUse::kNormalUi;
current_request_ui_to_use_ =
UiDecision(UiDecision::UseNormalUi(), UiDecision::ShowNoWarning());
ScheduleShowBubble();
}
}
......@@ -479,7 +480,6 @@ void PermissionRequestManager::FinalizeBubble(
current_request_view_shown_to_user_ = false;
current_request_ui_to_use_.reset();
current_request_quiet_ui_reason_.reset();
if (view_)
DeleteBubble();
......@@ -579,12 +579,12 @@ bool PermissionRequestManager::ShouldCurrentRequestUseQuietUI() const {
// ContentSettingImageModel might call into this method if the user switches
// between tabs while the |notification_permission_ui_selector_| is pending.
return current_request_ui_to_use_ &&
*current_request_ui_to_use_ == UiToUse::kQuietUi;
current_request_ui_to_use_->quiet_ui_reason;
}
PermissionRequestManager::QuietUiReason
PermissionRequestManager::ReasonForUsingQuietUi() const {
return *current_request_quiet_ui_reason_;
return *(current_request_ui_to_use_->quiet_ui_reason);
}
bool PermissionRequestManager::IsRequestInProgress() const {
......@@ -602,10 +602,8 @@ void PermissionRequestManager::NotifyBubbleRemoved() {
}
void PermissionRequestManager::OnSelectedUiToUseForNotifications(
UiToUse ui_to_use,
base::Optional<QuietUiReason> quiet_ui_reason) {
current_request_ui_to_use_ = ui_to_use;
current_request_quiet_ui_reason_ = quiet_ui_reason;
const UiDecision& decision) {
current_request_ui_to_use_ = decision;
ScheduleShowBubble();
}
......
......@@ -54,8 +54,9 @@ class PermissionRequestManager
enum AutoResponseType { NONE, ACCEPT_ALL, DENY_ALL, DISMISS };
using UiToUse = NotificationPermissionUiSelector::UiToUse;
using UiDecision = NotificationPermissionUiSelector::Decision;
using QuietUiReason = NotificationPermissionUiSelector::QuietUiReason;
using WarningReason = NotificationPermissionUiSelector::WarningReason;
~PermissionRequestManager() override;
......@@ -80,6 +81,7 @@ class PermissionRequestManager
// directly by the user in notifications settings, or via automatic logic that
// might trigger the current request to use the quiet UI.
bool ShouldCurrentRequestUseQuietUI() const;
// If |ShouldCurrentRequestUseQuietUI| return true, this will provide a reason
// as to why the quiet UI needs to be used.
QuietUiReason ReasonForUsingQuietUi() const;
......@@ -180,9 +182,7 @@ class PermissionRequestManager
void NotifyBubbleAdded();
void NotifyBubbleRemoved();
void OnSelectedUiToUseForNotifications(
UiToUse ui_to_use,
base::Optional<QuietUiReason> quiet_ui_reason);
void OnSelectedUiToUseForNotifications(const UiDecision& decision);
PermissionPromptDisposition DetermineCurrentRequestUIDispositionForUMA();
......@@ -226,14 +226,9 @@ class PermissionRequestManager
bool current_request_view_shown_to_user_ = false;
// Whether to use the normal or quiet UI to display the current permission
// |requests_|, or nullopt if we are still waiting on the result from the
// |notification_permission_ui_selector_|.
base::Optional<UiToUse> current_request_ui_to_use_;
// The reason for using the quiet UI to display the current permission
// |requests_|, or nullopt if we are still waiting for the response from the
// |notification_permission_ui_selector_| or we are using the normal UI.
base::Optional<QuietUiReason> current_request_quiet_ui_reason_;
// |requests_|, and whether to show warnings. This will be nullopt if we are
// still waiting on the result from |notification_permission_ui_selector_|.
base::Optional<UiDecision> current_request_ui_to_use_;
// Whether the bubble is being destroyed by this class, rather than in
// response to a UI event. In this case, callbacks from the bubble itself
......
......@@ -23,6 +23,10 @@
namespace permissions {
namespace {
using QuietUiReason = NotificationPermissionUiSelector::QuietUiReason;
}
class PermissionRequestManagerTest : public content::RenderViewHostTestHarness {
public:
PermissionRequestManagerTest()
......@@ -541,34 +545,34 @@ TEST_F(PermissionRequestManagerTest, UMAForTabSwitching) {
class MockNotificationPermissionUiSelector
: public NotificationPermissionUiSelector {
public:
explicit MockNotificationPermissionUiSelector(UiToUse ui_to_use, bool async) {
ui_to_use_ = ui_to_use;
explicit MockNotificationPermissionUiSelector(
base::Optional<QuietUiReason> quiet_ui_reason,
bool async) {
quiet_ui_reason_ = quiet_ui_reason;
async_ = async;
}
void SelectUiToUse(PermissionRequest* request,
DecisionMadeCallback callback) override {
base::Optional<QuietUiReason> reason;
if (ui_to_use_ == UiToUse::kQuietUi)
reason = QuietUiReason::kEnabledInPrefs;
Decision decision(quiet_ui_reason_, Decision::ShowNoWarning());
if (async_) {
base::SequencedTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(std::move(callback), ui_to_use_, reason));
FROM_HERE, base::BindOnce(std::move(callback), decision));
} else {
std::move(callback).Run(ui_to_use_, reason);
std::move(callback).Run(decision);
}
}
static void CreateForManager(PermissionRequestManager* manager,
UiToUse ui_to_use,
base::Optional<QuietUiReason> quiet_ui_reason,
bool async) {
manager->set_notification_permission_ui_selector_for_testing(
std::make_unique<MockNotificationPermissionUiSelector>(ui_to_use,
std::make_unique<MockNotificationPermissionUiSelector>(quiet_ui_reason,
async));
}
private:
UiToUse ui_to_use_;
base::Optional<QuietUiReason> quiet_ui_reason_;
bool async_;
};
......@@ -576,7 +580,8 @@ TEST_F(PermissionRequestManagerTest,
UiSelectorNotUsedForPermissionsOtherThanNotification) {
for (auto* request : {&request_mic_, &request_camera_, &request_ptz_}) {
MockNotificationPermissionUiSelector::CreateForManager(
manager_, NotificationPermissionUiSelector::UiToUse::kQuietUi,
manager_,
NotificationPermissionUiSelector::QuietUiReason::kEnabledInPrefs,
false /* async */);
manager_->AddRequest(request);
......@@ -593,20 +598,20 @@ TEST_F(PermissionRequestManagerTest,
}
TEST_F(PermissionRequestManagerTest, UiSelectorUsedForNotifications) {
using UiToUse = NotificationPermissionUiSelector::UiToUse;
const struct {
NotificationPermissionUiSelector::UiToUse ui_to_use;
base::Optional<NotificationPermissionUiSelector::QuietUiReason>
quiet_ui_reason;
bool async;
} kTests[] = {
{UiToUse::kQuietUi, true},
{UiToUse::kNormalUi, true},
{UiToUse::kQuietUi, false},
{UiToUse::kNormalUi, false},
{QuietUiReason::kEnabledInPrefs, true},
{NotificationPermissionUiSelector::Decision::UseNormalUi(), true},
{QuietUiReason::kEnabledInPrefs, false},
{NotificationPermissionUiSelector::Decision::UseNormalUi(), false},
};
for (const auto& test : kTests) {
MockNotificationPermissionUiSelector::CreateForManager(
manager_, test.ui_to_use, test.async);
manager_, test.quiet_ui_reason, test.async);
MockPermissionRequest request(
"foo", PermissionRequestType::PERMISSION_NOTIFICATIONS,
......@@ -618,7 +623,7 @@ TEST_F(PermissionRequestManagerTest, UiSelectorUsedForNotifications) {
EXPECT_TRUE(prompt_factory_->is_visible());
EXPECT_TRUE(
prompt_factory_->RequestTypeSeen(request.GetPermissionRequestType()));
EXPECT_EQ(test.ui_to_use == UiToUse::kQuietUi,
EXPECT_EQ(!!test.quiet_ui_reason,
manager_->ShouldCurrentRequestUseQuietUI());
Accept();
......@@ -628,9 +633,9 @@ TEST_F(PermissionRequestManagerTest, UiSelectorUsedForNotifications) {
TEST_F(PermissionRequestManagerTest,
UiSelectionHappensSeparatelyForEachRequest) {
using UiToUse = NotificationPermissionUiSelector::UiToUse;
using QuietUiReason = NotificationPermissionUiSelector::QuietUiReason;
MockNotificationPermissionUiSelector::CreateForManager(
manager_, UiToUse::kQuietUi, true);
manager_, QuietUiReason::kEnabledInPrefs, true);
MockPermissionRequest request1(
"request1", PermissionRequestType::PERMISSION_NOTIFICATIONS,
PermissionRequestGestureType::GESTURE);
......@@ -643,7 +648,8 @@ TEST_F(PermissionRequestManagerTest,
"request2", PermissionRequestType::PERMISSION_NOTIFICATIONS,
PermissionRequestGestureType::GESTURE);
MockNotificationPermissionUiSelector::CreateForManager(
manager_, UiToUse::kNormalUi, true);
manager_, NotificationPermissionUiSelector::Decision::UseNormalUi(),
true);
manager_->AddRequest(&request2);
WaitForBubbleToBeShown();
EXPECT_FALSE(manager_->ShouldCurrentRequestUseQuietUI());
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment