Commit 569e4c13 authored by Sebastien Marchand's avatar Sebastien Marchand Committed by Commit Bot

[TM] Don't freeze/discard tabs with Notifications permission

Tabs that are allowed to display notifications should never be frozen or
discarded as they clearly expressed that they'd like to use notifications
and the user allowed it.

The ukm.xml change simply add an enum value to a metric that has already
been approved here:
https://bugs.chromium.org/p/chromium/issues/detail?id=753486#c26

Change-Id: Iccdf1a026d59bbcc826a97388f8cf34fb50b44e8
Bug: 1026874
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1929087
Auto-Submit: Sébastien Marchand <sebmarchand@chromium.org>
Commit-Queue: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: default avatarRobert Kaplow <rkaplow@chromium.org>
Reviewed-by: default avatarFrançois Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#718657}
parent 67282e85
......@@ -39,6 +39,7 @@ const char* kDecisionFailureReasonStrings[] = {
"Tab is currently connected to a bluetooth device",
"Tab is currently holding a WebLock",
"Tab is currently holding an IndexedDB lock",
"Tab has notification permission ",
};
static_assert(base::size(kDecisionFailureReasonStrings) ==
static_cast<size_t>(DecisionFailureReason::MAX),
......@@ -152,6 +153,9 @@ void PopulateFailureReason(
case DecisionFailureReason::LIVE_STATE_USING_INDEXEDDB_LOCK:
ukm->SetFailureLiveStateUsingIndexedDBLock(1);
break;
case DecisionFailureReason::LIVE_STATE_HAS_NOTIFICATIONS_PERMISSION:
ukm->SetFailureLiveStateHasNotificationsPermission(1);
break;
case DecisionFailureReason::MAX:
NOTREACHED();
break;
......
......@@ -88,6 +88,9 @@ enum class DecisionFailureReason : int32_t {
// The tab is opted out of the intervention as it's currently holding at least
// one IndexedDB lock.
LIVE_STATE_USING_INDEXEDDB_LOCK,
// The tab is opted out of the intervention as it has the permission to use
// notifications.
LIVE_STATE_HAS_NOTIFICATIONS_PERMISSION,
// This must remain last.
MAX,
};
......
......@@ -16,6 +16,8 @@
#include "chrome/browser/devtools/devtools_window.h"
#include "chrome/browser/media/webrtc/media_capture_devices_dispatcher.h"
#include "chrome/browser/media/webrtc/media_stream_capture_indicator.h"
#include "chrome/browser/permissions/permission_manager.h"
#include "chrome/browser/permissions/permission_result.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/resource_coordinator/lifecycle_unit_state.mojom.h"
#include "chrome/browser/resource_coordinator/local_site_characteristics_data_store_factory.h"
......@@ -187,6 +189,7 @@ struct FeatureUsageEntry {
void CheckFeatureUsage(const SiteCharacteristicsDataReader* reader,
DecisionDetails* details) {
// TODO(sebmarchand): Remove the notification heuristic from this list.
const FeatureUsageEntry features[] = {
{reader->UsesAudioInBackground(), DecisionFailureReason::HEURISTIC_AUDIO},
{reader->UpdatesFaviconInBackground(),
......@@ -228,9 +231,10 @@ void CheckIfTabCanCommunicateWithUserWhileInBackground(
if (!URLShouldBeStoredInLocalDatabase(web_contents->GetLastCommittedURL()))
return;
auto* profile =
Profile::FromBrowserContext(web_contents->GetBrowserContext());
SiteCharacteristicsDataStore* data_store =
LocalSiteCharacteristicsDataStoreFactory::GetForProfile(
Profile::FromBrowserContext(web_contents->GetBrowserContext()));
LocalSiteCharacteristicsDataStoreFactory::GetForProfile(profile);
if (!data_store)
return;
......@@ -240,6 +244,14 @@ void CheckIfTabCanCommunicateWithUserWhileInBackground(
// TODO(sebmarchand): Add a failure reason for when the data isn't ready yet.
CheckFeatureUsage(reader.get(), details);
auto notif_permission = PermissionManager::Get(profile)->GetPermissionStatus(
ContentSettingsType::NOTIFICATIONS, web_contents->GetLastCommittedURL(),
web_contents->GetLastCommittedURL());
if (notif_permission.content_setting == CONTENT_SETTING_ALLOW) {
details->AddReason(
DecisionFailureReason::LIVE_STATE_HAS_NOTIFICATIONS_PERMISSION);
}
}
} // namespace
......
......@@ -18,6 +18,8 @@
#include "chrome/browser/media/webrtc/media_capture_devices_dispatcher.h"
#include "chrome/browser/media/webrtc/media_stream_capture_indicator.h"
#include "chrome/browser/metrics/desktop_session_duration/desktop_session_duration_tracker.h"
#include "chrome/browser/notifications/notification_permission_context.h"
#include "chrome/browser/permissions/permission_request_manager.h"
#include "chrome/browser/resource_coordinator/lifecycle_unit_observer.h"
#include "chrome/browser/resource_coordinator/local_site_characteristics_data_unittest_utils.h"
#include "chrome/browser/resource_coordinator/local_site_characteristics_webcontents_observer.h"
......@@ -956,4 +958,39 @@ TEST_F(TabLifecycleUnitTest, TabUnfreezeOnOriginTrialOptOut) {
tab_lifecycle_unit.GetState());
}
TEST_F(TabLifecycleUnitTest,
CannotFreezeOrDiscardTabWithNotificationsPermission) {
TabLifecycleUnit tab_lifecycle_unit(GetTabLifecycleUnitSource(), &observers_,
usage_clock_.get(), web_contents_,
tab_strip_model_.get());
TabLoadTracker::Get()->TransitionStateForTesting(web_contents_,
LoadingState::LOADED);
// Advance time enough that the tab is urgent discardable.
test_clock_.Advance(kBackgroundUrgentProtectionTime);
DecisionDetails decision_details;
ExpectCanDiscardTrueAllReasons(&tab_lifecycle_unit);
EXPECT_TRUE(tab_lifecycle_unit.CanFreeze(&decision_details));
NotificationPermissionContext::UpdatePermission(
profile(), web_contents_->GetLastCommittedURL().GetOrigin(),
CONTENT_SETTING_ALLOW);
decision_details.Clear();
EXPECT_FALSE(tab_lifecycle_unit.CanFreeze(&decision_details));
EXPECT_FALSE(decision_details.IsPositive());
EXPECT_EQ(DecisionFailureReason::LIVE_STATE_HAS_NOTIFICATIONS_PERMISSION,
decision_details.FailureReason());
decision_details.Clear();
EXPECT_FALSE(tab_lifecycle_unit.CanDiscard(
LifecycleUnitDiscardReason::PROACTIVE, &decision_details));
EXPECT_FALSE(decision_details.IsPositive());
EXPECT_EQ(DecisionFailureReason::LIVE_STATE_HAS_NOTIFICATIONS_PERMISSION,
decision_details.FailureReason());
NotificationPermissionContext::UpdatePermission(
profile(), web_contents_->GetLastCommittedURL().GetOrigin(),
CONTENT_SETTING_DEFAULT);
}
} // namespace resource_coordinator
......@@ -8339,6 +8339,12 @@ be describing additional metrics about the same event.
currently contains text form entry.
</summary>
</metric>
<metric name="FailureLiveStateHasNotificationsPermission" enum="Boolean">
<summary>
Boolean indicating that the intervention was disallowed because the tab
has the permission to use notifications.
</summary>
</metric>
<metric name="FailureLiveStateIsPDF" enum="Boolean">
<summary>
Boolean indicating that the intervention was disallowed because the tab is
......
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