Skip to content

Commit

Permalink
[SHv2] Move NotificationPermissionsReviewService to SafetyHub
Browse files Browse the repository at this point in the history
This CL moves the implementation of the service and associated factory
and tests to the general SafetyHub folder. No functional changes.

Bug: 1443466
Change-Id: I4db0e312a08c156db7ed3e7bbcf5013be569098b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4667392
Reviewed-by: Side YILMAZ <sideyilmaz@chromium.org>
Reviewed-by: Thomas Nguyen <tungnh@chromium.org>
Reviewed-by: Alex Ilin <alexilin@chromium.org>
Reviewed-by: Andy Paicu <andypaicu@chromium.org>
Commit-Queue: Tom Van Goethem <tov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1173449}
  • Loading branch information
tomvangoethem authored and Chromium LUCI CQ committed Jul 21, 2023
1 parent 6e34b6a commit 3e566ac
Show file tree
Hide file tree
Showing 13 changed files with 43 additions and 51 deletions.
2 changes: 0 additions & 2 deletions chrome/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -1138,8 +1138,6 @@ static_library("browser") {
"permissions/crowd_deny_preload_data.h",
"permissions/crowd_deny_safe_browsing_request.cc",
"permissions/crowd_deny_safe_browsing_request.h",
"permissions/notification_permission_review_service_factory.cc",
"permissions/notification_permission_review_service_factory.h",
"permissions/notifications_engagement_service_factory.cc",
"permissions/notifications_engagement_service_factory.h",
"permissions/notifications_permission_revocation_config.cc",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,6 @@
#include "chrome/browser/password_manager/password_reuse_manager_factory.h"
#include "chrome/browser/password_manager/password_store_factory.h"
#include "chrome/browser/permissions/adaptive_quiet_notification_permission_ui_enabler.h"
#include "chrome/browser/permissions/notification_permission_review_service_factory.h"
#include "chrome/browser/permissions/notifications_engagement_service_factory.h"
#include "chrome/browser/permissions/one_time_permissions_tracker_factory.h"
#include "chrome/browser/permissions/origin_keyed_permission_action_service_factory.h"
Expand Down Expand Up @@ -434,8 +433,9 @@
#include "chrome/browser/speech/speech_recognition_service_factory.h"
#include "chrome/browser/ui/global_media_controls/media_notification_service_factory.h"
#include "chrome/browser/ui/performance_controls/performance_controls_hats_service_factory.h"
#include "chrome/browser/usb/usb_connection_tracker_factory.h"
#include "chrome/browser/ui/safety_hub/notification_permission_review_service_factory.h"
#include "chrome/browser/ui/safety_hub/unused_site_permissions_service_factory.h"
#include "chrome/browser/usb/usb_connection_tracker_factory.h"
#include "chrome/browser/user_notes/user_note_service_factory.h"
#endif

Expand Down Expand Up @@ -803,7 +803,9 @@ void ChromeBrowserMainExtraPartsProfiles::
#endif
NotificationDisplayServiceFactory::GetInstance();
NotificationMetricsLoggerFactory::GetInstance();
#if !BUILDFLAG(IS_ANDROID)
NotificationPermissionsReviewServiceFactory::GetInstance();
#endif
NotificationsEngagementServiceFactory::GetInstance();
NotifierStateTrackerFactory::GetInstance();
#if BUILDFLAG(USE_NSS_CERTS)
Expand Down
4 changes: 4 additions & 0 deletions chrome/browser/ui/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -1298,6 +1298,10 @@ static_library("ui") {
"sad_tab_helper.cc",
"sad_tab_helper.h",
"sad_tab_types.h",
"safety_hub/notification_permission_review_service.cc",
"safety_hub/notification_permission_review_service.h",
"safety_hub/notification_permission_review_service_factory.cc",
"safety_hub/notification_permission_review_service_factory.h",
"safety_hub/password_status_check_service.cc",
"safety_hub/password_status_check_service.h",
"safety_hub/password_status_check_service_factory.cc",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// 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_review_service.h"
#include "chrome/browser/ui/safety_hub/notification_permission_review_service.h"

#include <map>
#include <set>
Expand All @@ -13,23 +13,23 @@
#include "components/content_settings/core/common/content_settings_utils.h"
#include "components/permissions/notifications_engagement_service.h"

namespace permissions {
namespace {

constexpr char kExcludedKey[] = "exempted";
constexpr char kDisplayedKey[] = "display_count";
// The daily average is calculated over the past this many days.
constexpr int kDays = 7;

namespace {

int ExtractNotificationCount(ContentSettingPatternSource item,
std::string date) {
if (!item.setting_value.is_dict())
if (!item.setting_value.is_dict()) {
return 0;
}

base::Value::Dict* bucket = item.setting_value.GetDict().FindDict(date);
if (!bucket)
if (!bucket) {
return 0;
}
return bucket->FindInt(kDisplayedKey).value_or(0);
}

Expand All @@ -40,8 +40,8 @@ int GetDailyAverageNotificationCount(ContentSettingPatternSource item) {

for (int day = 0; day < kDays; ++day) {
notification_count_total += ExtractNotificationCount(
item,
NotificationsEngagementService::GetBucketLabel(date - base::Days(day)));
item, permissions::NotificationsEngagementService::GetBucketLabel(
date - base::Days(day)));
}

return std::ceil(notification_count_total / kDays);
Expand Down Expand Up @@ -141,17 +141,20 @@ NotificationPermissionsReviewService::GetNotificationSiteListForReview() {
std::pair pair(item.primary_pattern, item.secondary_pattern);

// Blocklisted permissions should not be in the review list.
if (base::Contains(ignored_patterns_set, pair))
if (base::Contains(ignored_patterns_set, pair)) {
continue;
}

// Only granted permissions should be in the review list.
if (item.GetContentSetting() != CONTENT_SETTING_ALLOW)
if (item.GetContentSetting() != CONTENT_SETTING_ALLOW) {
continue;
}

// Only URLs that belong to a single origin should be in the review list.
if (!content_settings::PatternAppliesToSingleOrigin(item.primary_pattern,
item.secondary_pattern))
if (!content_settings::PatternAppliesToSingleOrigin(
item.primary_pattern, item.secondary_pattern)) {
continue;
}

int notification_count = notification_count_map[pair];
notification_permissions_list.emplace_back(
Expand Down Expand Up @@ -182,5 +185,3 @@ void NotificationPermissionsReviewService::
primary_pattern, secondary_pattern,
ContentSettingsType::NOTIFICATION_PERMISSION_REVIEW, {});
}

} // namespace permissions
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef COMPONENTS_PERMISSIONS_NOTIFICATION_PERMISSION_REVIEW_SERVICE_H_
#define COMPONENTS_PERMISSIONS_NOTIFICATION_PERMISSION_REVIEW_SERVICE_H_
#ifndef CHROME_BROWSER_UI_SAFETY_HUB_NOTIFICATION_PERMISSION_REVIEW_SERVICE_H_
#define CHROME_BROWSER_UI_SAFETY_HUB_NOTIFICATION_PERMISSION_REVIEW_SERVICE_H_

#include <vector>

Expand All @@ -13,8 +13,6 @@
#include "components/content_settings/core/common/content_settings_pattern.h"
#include "components/keyed_service/core/keyed_service.h"

namespace permissions {

struct NotificationPermissions {
ContentSettingsPattern primary_pattern;
ContentSettingsPattern secondary_pattern;
Expand Down Expand Up @@ -75,6 +73,4 @@ class NotificationPermissionsReviewService : public KeyedService,
content_settings_observation_{this};
};

} // namespace permissions

#endif // COMPONENTS_PERMISSIONS_NOTIFICATION_PERMISSION_REVIEW_SERVICE_H_
#endif // CHROME_BROWSER_UI_SAFETY_HUB_NOTIFICATION_PERMISSION_REVIEW_SERVICE_H_
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "chrome/browser/permissions/notification_permission_review_service_factory.h"
#include "chrome/browser/ui/safety_hub/notification_permission_review_service_factory.h"

#include "base/no_destructor.h"
#include "chrome/browser/content_settings/host_content_settings_map_factory.h"
Expand All @@ -17,9 +17,9 @@ NotificationPermissionsReviewServiceFactory::GetInstance() {
}

// static
permissions::NotificationPermissionsReviewService*
NotificationPermissionsReviewService*
NotificationPermissionsReviewServiceFactory::GetForProfile(Profile* profile) {
return static_cast<permissions::NotificationPermissionsReviewService*>(
return static_cast<NotificationPermissionsReviewService*>(
GetInstance()->GetServiceForBrowserContext(profile, true));
}

Expand All @@ -42,6 +42,6 @@ NotificationPermissionsReviewServiceFactory::
KeyedService*
NotificationPermissionsReviewServiceFactory::BuildServiceInstanceFor(
content::BrowserContext* context) const {
return new permissions::NotificationPermissionsReviewService(
return new NotificationPermissionsReviewService(
HostContentSettingsMapFactory::GetForProfile(context));
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef CHROME_BROWSER_PERMISSIONS_NOTIFICATION_PERMISSION_REVIEW_SERVICE_FACTORY_H_
#define CHROME_BROWSER_PERMISSIONS_NOTIFICATION_PERMISSION_REVIEW_SERVICE_FACTORY_H_
#ifndef CHROME_BROWSER_UI_SAFETY_HUB_NOTIFICATION_PERMISSION_REVIEW_SERVICE_FACTORY_H_
#define CHROME_BROWSER_UI_SAFETY_HUB_NOTIFICATION_PERMISSION_REVIEW_SERVICE_FACTORY_H_

#include "chrome/browser/profiles/profile_keyed_service_factory.h"
#include "components/permissions/notification_permission_review_service.h"
#include "chrome/browser/ui/safety_hub/notification_permission_review_service.h"

class Profile;

Expand All @@ -24,8 +24,7 @@ class NotificationPermissionsReviewServiceFactory
public:
static NotificationPermissionsReviewServiceFactory* GetInstance();

static permissions::NotificationPermissionsReviewService* GetForProfile(
Profile* profile);
static NotificationPermissionsReviewService* GetForProfile(Profile* profile);

// Non-copyable, non-moveable.
NotificationPermissionsReviewServiceFactory(
Expand All @@ -44,4 +43,4 @@ class NotificationPermissionsReviewServiceFactory
content::BrowserContext* context) const override;
};

#endif // CHROME_BROWSER_PERMISSIONS_NOTIFICATION_PERMISSION_REVIEW_SERVICE_FACTORY_H_
#endif // CHROME_BROWSER_UI_SAFETY_HUB_NOTIFICATION_PERMISSION_REVIEW_SERVICE_FACTORY_H_
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,19 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "chrome/browser/permissions/notification_permission_review_service_factory.h"
#include "chrome/browser/ui/safety_hub/notification_permission_review_service_factory.h"

#include "chrome/browser/content_settings/host_content_settings_map_factory.h"
#include "chrome/browser/ui/safety_hub/notification_permission_review_service.h"
#include "chrome/test/base/testing_profile.h"
#include "components/content_settings/core/browser/host_content_settings_map.h"
#include "components/content_settings/core/common/content_settings.h"
#include "components/content_settings/core/common/content_settings_pattern.h"
#include "components/content_settings/core/common/content_settings_utils.h"
#include "components/permissions/notification_permission_review_service.h"
#include "content/public/test/browser_task_environment.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "url/gurl.h"

namespace permissions {

#if !BUILDFLAG(IS_ANDROID)
class NotificationPermissionReviewServiceTest : public testing::Test {
protected:
TestingProfile* profile() { return &profile_; }
Expand Down Expand Up @@ -140,6 +137,3 @@ TEST_F(NotificationPermissionReviewServiceTest,
EXPECT_EQ(GURL(notification_permissions[0].primary_pattern.ToString()),
urls[0]);
}
#endif // !BUILDFLAG(IS_ANDROID)

} // namespace permissions
2 changes: 1 addition & 1 deletion chrome/browser/ui/webui/settings/safety_hub_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@
#include "base/time/time.h"
#include "base/values.h"
#include "chrome/browser/content_settings/host_content_settings_map_factory.h"
#include "chrome/browser/permissions/notification_permission_review_service_factory.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/safety_hub/notification_permission_review_service_factory.h"
#include "chrome/browser/ui/safety_hub/unused_site_permissions_service.h"
#include "chrome/browser/ui/safety_hub/unused_site_permissions_service_factory.h"
#include "chrome/browser/ui/webui/settings/site_settings_helper.h"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#include "base/time/clock.h"
#include "chrome/browser/content_settings/host_content_settings_map_factory.h"
#include "chrome/browser/history/history_service_factory.h"
#include "chrome/browser/permissions/notification_permission_review_service_factory.h"
#include "chrome/browser/ui/safety_hub/notification_permission_review_service_factory.h"
#include "chrome/browser/ui/safety_hub/unused_site_permissions_service.h"
#include "chrome/browser/ui/webui/settings/safety_hub_handler.h"
#include "chrome/browser/ui/webui/settings/site_settings_helper.h"
Expand Down
6 changes: 3 additions & 3 deletions chrome/browser/ui/webui/settings/site_settings_helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@
#include "chrome/browser/file_system_access/file_system_access_permission_context_factory.h"
#include "chrome/browser/hid/hid_chooser_context.h"
#include "chrome/browser/hid/hid_chooser_context_factory.h"
#include "chrome/browser/permissions/notification_permission_review_service_factory.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/serial/serial_chooser_context.h"
#include "chrome/browser/serial/serial_chooser_context_factory.h"
#include "chrome/browser/subresource_filter/subresource_filter_profile_context_factory.h"
#include "chrome/browser/ui/safety_hub/notification_permission_review_service_factory.h"
#include "chrome/browser/ui/url_identity.h"
#include "chrome/browser/usb/usb_chooser_context.h"
#include "chrome/browser/usb/usb_chooser_context_factory.h"
Expand Down Expand Up @@ -1104,8 +1104,8 @@ base::Value::List PopulateNotificationPermissionReviewData(Profile* profile) {

// Sort notification permissions by their priority for surfacing to the user.
auto notification_permission_ordering =
[](const permissions::NotificationPermissions& left,
const permissions::NotificationPermissions& right) {
[](const NotificationPermissions& left,
const NotificationPermissions& right) {
return left.notification_count > right.notification_count;
};
std::sort(notification_permissions.begin(), notification_permissions.end(),
Expand Down
2 changes: 1 addition & 1 deletion chrome/test/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -5895,7 +5895,6 @@ test("unit_tests") {
"../browser/permissions/contextual_notification_permission_ui_selector_unittest.cc",
"../browser/permissions/crowd_deny_preload_data_unittest.cc",
"../browser/permissions/crowd_deny_safe_browsing_request_unittest.cc",
"../browser/permissions/notification_permission_review_service_unittest.cc",
"../browser/permissions/notifications_engagement_service_unittest.cc",
"../browser/permissions/permission_context_base_permissions_policy_unittest.cc",
"../browser/permissions/permission_revocation_request_unittests.cc",
Expand Down Expand Up @@ -6109,6 +6108,7 @@ test("unit_tests") {
"../browser/policy/messaging_layer/util/reporting_server_connector_unittest.cc",
"../browser/segmentation_platform/client_util/local_tab_handler_unittest.cc",
"../browser/ssl/generated_https_first_mode_pref_unittest.cc",
"../browser/ui/safety_hub/notification_permission_review_service_unittest.cc",
"../browser/ui/safety_hub/safety_hub_service_unittest.cc",
]
}
Expand Down
2 changes: 0 additions & 2 deletions components/permissions/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,6 @@ source_set("permissions") {
"contexts/webxr_permission_context.h",
"contexts/window_management_permission_context.cc",
"contexts/window_management_permission_context.h",
"notification_permission_review_service.cc",
"notification_permission_review_service.h",
"notifications_engagement_service.cc",
"notifications_engagement_service.h",
"object_permission_context_base.cc",
Expand Down

0 comments on commit 3e566ac

Please sign in to comment.