[SHv2] Add Safe Browsing result

This CL creates a result class for the Safe Browsing module of Safety
Hub. Additionally, it introduces a number of improvements to the Safety
Hub service in general, in particular how results are created based on
dict values. This now uses a simple factory method instead of a lot of
boilerplate code. Furthermore, the menu notification service now uses a
bound method to obtain the latest result from services (or other places)
instead of requiring these need to originate from a Safety Hub service.

Bug: 1443466

Change-Id: I1276d9c266dc73c5bfd865013c53c7820ad8dce3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4953199
Reviewed-by: Side YILMAZ <sideyilmaz@chromium.org>
Commit-Queue: Tom Van Goethem <tov@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1215408}

chrome/app/settings_strings.grdp

@@ -4488,6 +4488,9 @@
   <message name="IDS_SETTINGS_SAFETY_HUB_MODULE_NAME_SEPARATOR" desc="The entry point shown on the screenshot leads to Safety Check settings page (chrome://settings/safetyCheck) when at least on of the Safety Check modules have recommendations. The subheader of the entry point lists the modules with recommendations using a separator between them (in English it is a comma used between all items in a series). This string is the separator between the module names and is used like 'Passwords, extensions, notifications'.">
     ,
   </message>
+  <message name="IDS_SETTINGS_SAFETY_HUB_SAFE_BROWSING_MENU_NOTIFICATION" desc="When Safe Browsing is disabled, the proactive Safety Hub feature encourages users to turn on the feature. This label is for the notification in Chrome's three-dot menu.">
+    Turn on Safe Browsing
+  </message>
 
   <!-- Experimental enterprise plus_addresses settings strings -->
   <message name="IDS_PLUS_ADDRESS_SETTINGS_LABEL" desc="A button in settings for management of experimental enterprise plus addresses" translateable="false">

chrome/app/settings_strings_grdp/IDS_SETTINGS_SAFETY_HUB_SAFE_BROWSING_MENU_NOTIFICATION.png.sha1

@@ -0,0 +1 @@
+32ba9088e34d6306ce1387a1504fd1acca3c1d76

chrome/browser/ui/BUILD.gn

@@ -1375,6 +1375,8 @@ static_library("ui") {
       "safety_hub/password_status_check_service.h",
       "safety_hub/password_status_check_service_factory.cc",
       "safety_hub/password_status_check_service_factory.h",
+      "safety_hub/safe_browsing_result.cc",
+      "safety_hub/safe_browsing_result.h",
       "safety_hub/safety_hub_constants.cc",
       "safety_hub/safety_hub_constants.h",
       "safety_hub/safety_hub_prefs.cc",

chrome/browser/ui/safety_hub/menu_notification.cc

@@ -8,7 +8,11 @@
 #include <string>
 
 #include "base/json/values_util.h"
+#include "chrome/browser/ui/safety_hub/notification_permission_review_service.h"
+#include "chrome/browser/ui/safety_hub/safe_browsing_result.h"
+#include "chrome/browser/ui/safety_hub/safety_hub_constants.h"
 #include "chrome/browser/ui/safety_hub/safety_hub_service.h"
+#include "chrome/browser/ui/safety_hub/unused_site_permissions_service.h"
 
 SafetyHubMenuNotification::SafetyHubMenuNotification()
     : is_currently_active_(false),
@@ -19,7 +23,8 @@ SafetyHubMenuNotification::SafetyHubMenuNotification()
 SafetyHubMenuNotification::~SafetyHubMenuNotification() = default;
 
 SafetyHubMenuNotification::SafetyHubMenuNotification(
-    const base::Value::Dict& dict) {
+    const base::Value::Dict& dict,
+    safety_hub::SafetyHubModuleType type) {
   is_currently_active_ =
       dict.FindBool(kSafetyHubMenuNotificationActiveKey).value();
   impression_count_ =
@@ -28,6 +33,10 @@ SafetyHubMenuNotification::SafetyHubMenuNotification(
       dict.Find(kSafetyHubMenuNotificationFirstImpressionKey));
   last_impression_time_ =
       base::ValueToTime(dict.Find(kSafetyHubMenuNotificationLastImpressionKey));
+  if (dict.contains(kSafetyHubMenuNotificationResultKey)) {
+    result_ = GetResultFromDict(
+        *dict.FindDict(kSafetyHubMenuNotificationResultKey), type);
+  }
 }
 
 base::Value::Dict SafetyHubMenuNotification::ToDictValue() const {
@@ -134,18 +143,6 @@ void SafetyHubMenuNotification::UpdateResult(
   result_ = std::move(result);
 }
 
-// static
-std::unique_ptr<SafetyHubMenuNotification>
-SafetyHubMenuNotification::FromDictValue(const base::Value::Dict& dict,
-                                         SafetyHubService* service) {
-  auto notification = std::make_unique<SafetyHubMenuNotification>(dict);
-  if (dict.contains(kSafetyHubMenuNotificationResultKey)) {
-    notification->result_ = service->GetResultFromDictValue(
-        *dict.FindDict(kSafetyHubMenuNotificationResultKey));
-  }
-  return notification;
-}
-
 std::u16string SafetyHubMenuNotification::GetNotificationString() const {
   CHECK(result_);
   return result_->GetNotificationString();
@@ -160,3 +157,21 @@ SafetyHubService::Result* SafetyHubMenuNotification::GetResultForTesting()
     const {
   return result_.get();
 }
+
+// static
+std::unique_ptr<SafetyHubService::Result>
+SafetyHubMenuNotification::GetResultFromDict(
+    const base::Value::Dict& dict,
+    safety_hub::SafetyHubModuleType type) {
+  switch (type) {
+    case safety_hub::SafetyHubModuleType::UNUSED_SITE_PERMISSIONS:
+      return std::make_unique<
+          UnusedSitePermissionsService::UnusedSitePermissionsResult>(dict);
+    case safety_hub::SafetyHubModuleType::NOTIFICATION_PERMISSIONS:
+      return std::make_unique<
+          NotificationPermissionsReviewService::NotificationPermissionsResult>(
+          dict);
+    case safety_hub::SafetyHubModuleType::SAFE_BROWSING:
+      return std::make_unique<SafetyHubSafeBrowsingResult>(dict);
+  }
+}

chrome/browser/ui/safety_hub/menu_notification.h

@@ -10,6 +10,7 @@
 
 #include "base/time/time.h"
 #include "base/values.h"
+#include "chrome/browser/ui/safety_hub/safety_hub_constants.h"
 #include "chrome/browser/ui/safety_hub/safety_hub_service.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
 
@@ -31,7 +32,8 @@ constexpr int kSafetyHubMenuNotificationMinImpressionCount = 5;
 class SafetyHubMenuNotification {
  public:
   SafetyHubMenuNotification();
-  explicit SafetyHubMenuNotification(const base::Value::Dict& dict);
+  explicit SafetyHubMenuNotification(const base::Value::Dict& dict,
+                                     safety_hub::SafetyHubModuleType type);
 
   SafetyHubMenuNotification(const SafetyHubMenuNotification&) = delete;
   SafetyHubMenuNotification& operator=(const SafetyHubMenuNotification&) =
@@ -41,10 +43,6 @@ class SafetyHubMenuNotification {
 
   base::Value::Dict ToDictValue() const;
 
-  static std::unique_ptr<SafetyHubMenuNotification> FromDictValue(
-      const base::Value::Dict& dict,
-      SafetyHubService* service);
-
   // Called when the menu notification will be shown. This will make the
   // notification the currently active one.
   void Show();
@@ -88,6 +86,10 @@ class SafetyHubMenuNotification {
   // Returns whether any notification for the same type of result has been
   // shown.
   bool HasAnyNotificationBeenShown() const;
+  // Returns a result based on the values defined in the provided dictionary.
+  static std::unique_ptr<SafetyHubService::Result> GetResultFromDict(
+      const base::Value::Dict& dict,
+      safety_hub::SafetyHubModuleType type);
 
   // Indicates whether the notification is actively being shown.
   bool is_currently_active_ = false;

chrome/browser/ui/safety_hub/menu_notification_service.cc

@@ -7,22 +7,31 @@
 #include <memory>
 #include <utility>
 
+#include "base/functional/bind.h"
+#include "base/functional/callback_forward.h"
 #include "base/values.h"
+#include "chrome/browser/ui/safety_hub/menu_notification.h"
+#include "chrome/browser/ui/safety_hub/notification_permission_review_service.h"
+#include "chrome/browser/ui/safety_hub/safe_browsing_result.h"
 #include "chrome/browser/ui/safety_hub/safety_hub_prefs.h"
+#include "chrome/browser/ui/safety_hub/safety_hub_service.h"
+#include "chrome/browser/ui/safety_hub/unused_site_permissions_service.h"
 #include "components/prefs/pref_service.h"
 
 namespace {
-SafetyHubServiceInfoElement::SafetyHubServiceInfoElement() = default;
-SafetyHubServiceInfoElement::~SafetyHubServiceInfoElement() = default;
+SafetyHubModuleInfoElement::SafetyHubModuleInfoElement() = default;
+SafetyHubModuleInfoElement::~SafetyHubModuleInfoElement() = default;
 
-SafetyHubServiceInfoElement::SafetyHubServiceInfoElement(
+SafetyHubModuleInfoElement::SafetyHubModuleInfoElement(
     MenuNotificationPriority priority,
     base::TimeDelta interval,
-    raw_ptr<SafetyHubService> service,
+    base::RepeatingCallback<
+        std::optional<std::unique_ptr<SafetyHubService::Result>>()>
+        result_getter,
     std::unique_ptr<SafetyHubMenuNotification> notification)
     : priority(priority),
       interval(interval),
-      service(service),
+      result_getter(result_getter),
       notification(std::move(notification)) {}
 }  // namespace
 
@@ -36,27 +45,40 @@ SafetyHubMenuNotificationService::SafetyHubMenuNotificationService(
 
   // TODO(crbug.com/1443466): Make the interval for each service finch
   // configurable.
-  SetServiceInfoElement(SafetyHubServiceType::UNUSED_SITE_PERMISSIONS,
-                        MenuNotificationPriority::LOW, base::Days(10),
-                        unused_site_permissions_service, stored_notifications);
-  SetServiceInfoElement(SafetyHubServiceType::NOTIFICATION_PERMISSIONS,
-                        MenuNotificationPriority::LOW, base::Days(10),
-                        notification_permissions_service, stored_notifications);
+  // The Safety Hub services will be available whenever the |GetCachedResult|
+  // method is called, so it is safe to use |base::Unretained| here.
+  SetInfoElement(
+      safety_hub::SafetyHubModuleType::UNUSED_SITE_PERMISSIONS,
+      MenuNotificationPriority::LOW, base::Days(10),
+      base::BindRepeating(&SafetyHubService::GetCachedResult,
+                          base::Unretained(unused_site_permissions_service)),
+      stored_notifications);
+  SetInfoElement(
+      safety_hub::SafetyHubModuleType::NOTIFICATION_PERMISSIONS,
+      MenuNotificationPriority::LOW, base::Days(10),
+      base::BindRepeating(&SafetyHubService::GetCachedResult,
+                          base::Unretained(notification_permissions_service)),
+      stored_notifications);
+  SetInfoElement(safety_hub::SafetyHubModuleType::SAFE_BROWSING,
+                 MenuNotificationPriority::MEDIUM, base::Days(90),
+                 base::BindRepeating(&SafetyHubSafeBrowsingResult::GetResult,
+                                     base::Unretained(pref_service)),
+                 stored_notifications);
 }
 
 SafetyHubMenuNotificationService::~SafetyHubMenuNotificationService() = default;
 
 absl::optional<MenuNotificationEntry>
 SafetyHubMenuNotificationService::GetNotificationToShow() {
-  absl::optional<ResultMap> result_map = GetResultsFromAllServices();
+  absl::optional<ResultMap> result_map = GetResultsFromAllModules();
   if (!result_map.has_value()) {
     return absl::nullopt;
   }
   std::list<SafetyHubMenuNotification*> notifications_to_be_shown;
   MenuNotificationPriority cur_highest_priority = MenuNotificationPriority::LOW;
   for (auto const& item : result_map.value()) {
-    SafetyHubServiceInfoElement* info_element =
-        service_info_map_[item.first].get();
+    SafetyHubModuleInfoElement* info_element =
+        module_info_map_[item.first].get();
     SafetyHubMenuNotification* notification = info_element->notification.get();
     notification->UpdateResult(std::move(result_map.value()[item.first]));
     if (notification->ShouldBeShown(info_element->interval)) {
@@ -97,10 +119,12 @@ SafetyHubMenuNotificationService::GetNotificationToShow() {
 }
 
 absl::optional<ResultMap>
-SafetyHubMenuNotificationService::GetResultsFromAllServices() {
+SafetyHubMenuNotificationService::GetResultsFromAllModules() {
   ResultMap result_map;
-  for (auto const& item : service_info_map_) {
-    auto result = item.second->service->GetCachedResult();
+  for (auto const& item : module_info_map_) {
+    CHECK(item.second->result_getter);
+    absl::optional<std::unique_ptr<SafetyHubService::Result>> result =
+        item.second->result_getter.Run();
     // If one of the cached results is unavailable, no notification is shown.
     if (!result.has_value()) {
       return absl::nullopt;
@@ -113,8 +137,8 @@ SafetyHubMenuNotificationService::GetResultsFromAllServices() {
 void SafetyHubMenuNotificationService::SaveNotificationsToPrefs() const {
   base::Value::Dict notifications;
   for (auto const& it : pref_dict_key_map_) {
-    SafetyHubServiceInfoElement* info_element =
-        service_info_map_.find(it.first)->second.get();
+    SafetyHubModuleInfoElement* info_element =
+        module_info_map_.find(it.first)->second.get();
     notifications.Set(it.second, info_element->notification->ToDictValue());
   }
   pref_service_->SetDict(safety_hub_prefs::kMenuNotificationsPrefsKey,
@@ -123,44 +147,33 @@ void SafetyHubMenuNotificationService::SaveNotificationsToPrefs() const {
 
 SafetyHubMenuNotification*
 SafetyHubMenuNotificationService::GetNotificationForTesting(
-    SafetyHubServiceType service_type) {
-  return service_info_map_.find(service_type)->second.get()->notification.get();
+    safety_hub::SafetyHubModuleType service_type) {
+  return module_info_map_.find(service_type)->second.get()->notification.get();
 }
 
 std::unique_ptr<SafetyHubMenuNotification>
 SafetyHubMenuNotificationService::GetNotificationFromDict(
     const base::Value::Dict& dict,
-    SafetyHubServiceType type,
-    SafetyHubService* service) const {
-  // It can be assumed that all `SafetyHubServiceType`s are in
+    safety_hub::SafetyHubModuleType& type) const {
+  // It can be assumed that all `safety_hub::SafetyHubModuleType`s are in
   // `pref_dict_key_map_`.
   const base::Value::Dict* notification_dict =
       dict.FindDict(pref_dict_key_map_.find(type)->second);
-  std::unique_ptr<SafetyHubMenuNotification> result_notification;
   if (!notification_dict) {
-    result_notification = std::make_unique<SafetyHubMenuNotification>();
-  } else {
-    result_notification =
-        SafetyHubMenuNotification::FromDictValue(*notification_dict, service);
+    return std::make_unique<SafetyHubMenuNotification>();
   }
-  return result_notification;
+  return std::make_unique<SafetyHubMenuNotification>(*notification_dict, type);
 }
 
-void SafetyHubMenuNotificationService::Shutdown() {
-  for (auto const& item : service_info_map_) {
-    // Setting to nullptr to avoid dangling pointers when services are
-    // deconstructed.
-    item.second->service = nullptr;
-  }
-}
-
-void SafetyHubMenuNotificationService::SetServiceInfoElement(
-    SafetyHubServiceType type,
+void SafetyHubMenuNotificationService::SetInfoElement(
+    safety_hub::SafetyHubModuleType type,
     MenuNotificationPriority priority,
     base::TimeDelta interval,
-    SafetyHubService* service,
+    base::RepeatingCallback<
+        std::optional<std::unique_ptr<SafetyHubService::Result>>()>
+        result_getter,
     const base::Value::Dict& stored_notifications) {
-  service_info_map_[type] = std::make_unique<SafetyHubServiceInfoElement>(
-      priority, interval, service,
-      GetNotificationFromDict(stored_notifications, type, service));
+  module_info_map_[type] = std::make_unique<SafetyHubModuleInfoElement>(
+      priority, interval, result_getter,
+      GetNotificationFromDict(stored_notifications, type));
 }