Safety check settings page: added update status enum to pass to JS

Also removed the version updater callback argument as it does not need
to be created outside of CheckUpdates().

Bug: 1015841
Change-Id: I4a2bb4a23ce1c24e1a8358c641b797be0a79a21e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2049982
Commit-Queue: Andrey Zaytsev <andzaytsev@google.com>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#743060}

chrome/browser/ui/webui/settings/safety_check_handler.cc

@@ -10,6 +10,44 @@
 #include "components/prefs/pref_service.h"
 #include "components/safe_browsing/core/common/safe_browsing_prefs.h"
 
+namespace {
+
+// Constants for communication with JS.
+static constexpr char kStatusChanged[] = "safety-check-status-changed";
+static constexpr char kInitialize[] = "initializeSafetyCheck";
+static constexpr char kPerformSafetyCheck[] = "performSafetyCheck";
+static constexpr char kSafetyCheckComponent[] = "safetyCheckComponent";
+static constexpr char kNewState[] = "newState";
+
+// Converts the VersionUpdater::Status to the UpdateStatus enum to be passed
+// to the safety check frontend. Note: if the VersionUpdater::Status gets
+// changed, this will fail to compile. That is done intentionally to ensure
+// that the states of the safety check are always in sync with the
+// VersionUpdater ones.
+SafetyCheckHandler::UpdateStatus ConvertToUpdateStatus(
+    VersionUpdater::Status status) {
+  switch (status) {
+    case VersionUpdater::CHECKING:
+      return SafetyCheckHandler::UpdateStatus::kChecking;
+    case VersionUpdater::UPDATED:
+      return SafetyCheckHandler::UpdateStatus::kUpdated;
+    case VersionUpdater::UPDATING:
+      return SafetyCheckHandler::UpdateStatus::kUpdating;
+    case VersionUpdater::NEED_PERMISSION_TO_UPDATE:
+    case VersionUpdater::NEARLY_UPDATED:
+      return SafetyCheckHandler::UpdateStatus::kRelaunch;
+    case VersionUpdater::DISABLED:
+    case VersionUpdater::DISABLED_BY_ADMIN:
+      return SafetyCheckHandler::UpdateStatus::kDisabledByAdmin;
+    case VersionUpdater::FAILED:
+    case VersionUpdater::FAILED_CONNECTION_TYPE_DISALLOWED:
+      return SafetyCheckHandler::UpdateStatus::kFailed;
+    case VersionUpdater::FAILED_OFFLINE:
+      return SafetyCheckHandler::UpdateStatus::kFailedOffline;
+  }
+}
+}  // namespace
+
 SafetyCheckHandler::SafetyCheckHandler()
     : SafetyCheckHandler(nullptr, nullptr) {}
 
@@ -19,29 +57,32 @@ void SafetyCheckHandler::PerformSafetyCheck() {
   if (!version_updater_) {
     version_updater_.reset(VersionUpdater::Create(web_ui()->GetWebContents()));
   }
-  CheckUpdates(base::Bind(&SafetyCheckHandler::OnUpdateCheckResult,
-                          base::Unretained(this)));
+  CheckUpdates();
   CheckSafeBrowsing();
 }
 
-void SafetyCheckHandler::OnJavascriptAllowed() {}
-
-void SafetyCheckHandler::OnJavascriptDisallowed() {}
-
-void SafetyCheckHandler::RegisterMessages() {}
-
 SafetyCheckHandler::SafetyCheckHandler(
     std::unique_ptr<VersionUpdater> version_updater,
     SafetyCheckHandlerObserver* observer)
     : version_updater_(std::move(version_updater)), observer_(observer) {}
 
-void SafetyCheckHandler::CheckUpdates(
-    const VersionUpdater::StatusCallback& update_callback) {
+void SafetyCheckHandler::HandleInitialize(const base::ListValue* /*args*/) {
+  AllowJavascript();
+}
+
+void SafetyCheckHandler::HandlePerformSafetyCheck(
+    const base::ListValue* /*args*/) {
+  PerformSafetyCheck();
+}
+
+void SafetyCheckHandler::CheckUpdates() {
   if (observer_) {
     observer_->OnUpdateCheckStart();
   }
-  version_updater_->CheckForUpdate(update_callback,
-                                   VersionUpdater::PromoteCallback());
+  version_updater_->CheckForUpdate(
+      base::Bind(&SafetyCheckHandler::OnUpdateCheckResult,
+                 base::Unretained(this)),
+      VersionUpdater::PromoteCallback());
 }
 
 void SafetyCheckHandler::CheckSafeBrowsing() {
@@ -54,24 +95,30 @@ void SafetyCheckHandler::CheckSafeBrowsing() {
   if (!enabled) {
     bool disabled_by_admin =
         pref_service->IsManagedPreference(prefs::kSafeBrowsingEnabled);
-    status = disabled_by_admin ? SafeBrowsingStatus::DISABLED_BY_ADMIN
-                               : SafeBrowsingStatus::DISABLED;
+    status = disabled_by_admin ? SafeBrowsingStatus::kDisabledByAdmin
+                               : SafeBrowsingStatus::kDisabled;
   } else {
-    status = SafeBrowsingStatus::ENABLED;
+    status = SafeBrowsingStatus::kEnabled;
   }
   OnSafeBrowsingCheckResult(status);
 }
 
-void SafetyCheckHandler::OnUpdateCheckResult(VersionUpdater::Status status,
-                                             int progress,
-                                             bool rollback,
-                                             const std::string& version,
-                                             int64_t update_size,
-                                             const base::string16& message) {
+void SafetyCheckHandler::OnUpdateCheckResult(
+    VersionUpdater::Status status,
+    int /*progress*/,
+    bool /*rollback*/,
+    const std::string& /*version*/,
+    int64_t /*update_size*/,
+    const base::string16& /*message*/) {
+  UpdateStatus update_status = ConvertToUpdateStatus(status);
   if (observer_) {
-    observer_->OnUpdateCheckResult(status, progress, rollback, version,
-                                   update_size, message);
+    observer_->OnUpdateCheckResult(update_status);
   }
+  auto event = std::make_unique<base::DictionaryValue>();
+  event->SetInteger(kSafetyCheckComponent,
+                    static_cast<int>(SafetyCheckComponent::kUpdates));
+  event->SetInteger(kNewState, static_cast<int>(update_status));
+  FireWebUIListener(kStatusChanged, *event);
 }
 
 void SafetyCheckHandler::OnSafeBrowsingCheckResult(
@@ -79,4 +126,23 @@ void SafetyCheckHandler::OnSafeBrowsingCheckResult(
   if (observer_) {
     observer_->OnSafeBrowsingCheckResult(status);
   }
+  auto event = std::make_unique<base::DictionaryValue>();
+  event->SetInteger(kSafetyCheckComponent,
+                    static_cast<int>(SafetyCheckComponent::kSafeBrowsing));
+  event->SetInteger(kNewState, static_cast<int>(status));
+  FireWebUIListener(kStatusChanged, *event);
+}
+
+void SafetyCheckHandler::OnJavascriptAllowed() {}
+
+void SafetyCheckHandler::OnJavascriptDisallowed() {}
+
+void SafetyCheckHandler::RegisterMessages() {
+  web_ui()->RegisterMessageCallback(
+      kInitialize, base::BindRepeating(&SafetyCheckHandler::HandleInitialize,
+                                       base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      kPerformSafetyCheck,
+      base::BindRepeating(&SafetyCheckHandler::HandlePerformSafetyCheck,
+                          base::Unretained(this)));
 }

chrome/browser/ui/webui/settings/safety_check_handler.h

@@ -23,7 +23,30 @@ class SafetyCheckHandlerObserver;
 // software.
 class SafetyCheckHandler : public settings::SettingsPageUIHandler {
  public:
-  enum SafeBrowsingStatus { ENABLED, DISABLED_BY_ADMIN, DISABLED };
+  // Used in communication with the frontend to indicate which component the
+  // communicated status update applies to.
+  enum class SafetyCheckComponent {
+    kUpdates,
+    kPasswords,
+    kSafeBrowsing,
+    kExtensions,
+  };
+  enum class UpdateStatus {
+    kChecking,
+    kUpdated,
+    kUpdating,
+    kRelaunch,
+    kDisabledByAdmin,
+    kFailedOffline,
+    kFailed,
+  };
+  enum class SafeBrowsingStatus {
+    kChecking,
+    kEnabled,
+    kDisabled,
+    kDisabledByAdmin,
+    kDisabledByExtension,
+  };
 
   SafetyCheckHandler();
   ~SafetyCheckHandler() override;
@@ -38,9 +61,16 @@ class SafetyCheckHandler : public settings::SettingsPageUIHandler {
                      SafetyCheckHandlerObserver* observer);
 
  private:
+  // Handles page initialization. Should be called when page is loaded.
+  void HandleInitialize(const base::ListValue* args);
+
+  // Handles triggering the safety check from the frontend (by user pressing a
+  // button).
+  void HandlePerformSafetyCheck(const base::ListValue* args);
+
   // Triggers an update check and invokes the provided callback once results
   // are available.
-  void CheckUpdates(const VersionUpdater::StatusCallback& update_callback);
+  void CheckUpdates();
 
   // Gets the status of Safe Browsing from the PrefService and invokes
   // OnSafeBrowsingCheckResult with results.

chrome/browser/ui/webui/settings/safety_check_handler_observer.h

@@ -18,12 +18,7 @@ class SafetyCheckHandlerObserver {
   virtual ~SafetyCheckHandlerObserver() = default;
 
   virtual void OnUpdateCheckStart() = 0;
-  virtual void OnUpdateCheckResult(VersionUpdater::Status status,
-                                   int progress,
-                                   bool rollback,
-                                   const std::string& version,
-                                   int64_t update_size,
-                                   const base::string16& message) = 0;
+  virtual void OnUpdateCheckResult(SafetyCheckHandler::UpdateStatus status) = 0;
   virtual void OnSafeBrowsingCheckStart() = 0;
   virtual void OnSafeBrowsingCheckResult(
       SafetyCheckHandler::SafeBrowsingStatus status) = 0;

chrome/browser/ui/webui/settings/safety_check_handler_unittest.cc

@@ -19,12 +19,7 @@ class TestSafetyCheckObserver : public SafetyCheckHandlerObserver {
  public:
   void OnUpdateCheckStart() override { update_check_started_ = true; }
 
-  void OnUpdateCheckResult(VersionUpdater::Status status,
-                           int progress,
-                           bool rollback,
-                           const std::string& version,
-                           int64_t update_size,
-                           const base::string16& message) override {
+  void OnUpdateCheckResult(SafetyCheckHandler::UpdateStatus status) override {
     update_check_status_ = status;
   }
 
@@ -38,7 +33,7 @@ class TestSafetyCheckObserver : public SafetyCheckHandlerObserver {
   }
 
   bool update_check_started_ = false;
-  base::Optional<VersionUpdater::Status> update_check_status_;
+  base::Optional<SafetyCheckHandler::UpdateStatus> update_check_status_;
   bool safe_browsing_check_started_ = false;
   base::Optional<SafetyCheckHandler::SafeBrowsingStatus>
       safe_browsing_check_status_;
@@ -46,6 +41,7 @@ class TestSafetyCheckObserver : public SafetyCheckHandlerObserver {
 
 class TestingSafetyCheckHandler : public SafetyCheckHandler {
  public:
+  using SafetyCheckHandler::AllowJavascript;
   using SafetyCheckHandler::set_web_ui;
 
   TestingSafetyCheckHandler(std::unique_ptr<VersionUpdater> version_updater,
@@ -55,27 +51,63 @@ class TestingSafetyCheckHandler : public SafetyCheckHandler {
 
 class SafetyCheckHandlerTest : public ChromeRenderViewHostTestHarness {
  public:
-  void SetUp() override {
-    ChromeRenderViewHostTestHarness::SetUp();
-
-    // The unique pointer to a TestVersionUpdater gets moved to
-    // SafetyCheckHandler, but a raw pointer is retained here to change its
-    // state.
-    auto version_updater = std::make_unique<TestVersionUpdater>();
-    version_updater_ = version_updater.get();
-    test_web_ui_.set_web_contents(web_contents());
-    safety_check_ = std::make_unique<TestingSafetyCheckHandler>(
-        std::move(version_updater), &observer_);
-    safety_check_->set_web_ui(&test_web_ui_);
-  }
+  void SetUp() override;
+
+  // Returns whether one of the WebUI callbacks for safety check status changes
+  // has the specified |safetyCheckComponent| and |newState|.
+  bool HasSafetyCheckStatusChangedWithData(int component, int new_state);
 
  protected:
-  TestVersionUpdater* version_updater_;
+  TestVersionUpdater* version_updater_ = nullptr;
   TestSafetyCheckObserver observer_;
   content::TestWebUI test_web_ui_;
   std::unique_ptr<TestingSafetyCheckHandler> safety_check_;
 };
 
+void SafetyCheckHandlerTest::SetUp() {
+  ChromeRenderViewHostTestHarness::SetUp();
+
+  // The unique pointer to a TestVersionUpdater gets moved to
+  // SafetyCheckHandler, but a raw pointer is retained here to change its
+  // state.
+  auto version_updater = std::make_unique<TestVersionUpdater>();
+  version_updater_ = version_updater.get();
+  test_web_ui_.set_web_contents(web_contents());
+  safety_check_ = std::make_unique<TestingSafetyCheckHandler>(
+      std::move(version_updater), &observer_);
+  test_web_ui_.ClearTrackedCalls();
+  safety_check_->set_web_ui(&test_web_ui_);
+  safety_check_->AllowJavascript();
+}
+
+bool SafetyCheckHandlerTest::HasSafetyCheckStatusChangedWithData(
+    int component,
+    int new_state) {
+  for (const auto& it : test_web_ui_.call_data()) {
+    const content::TestWebUI::CallData& data = *it;
+    if (data.function_name() != "cr.webUIListenerCallback") {
+      continue;
+    }
+    std::string event;
+    if ((!data.arg1()->GetAsString(&event)) ||
+        event != "safety-check-status-changed") {
+      continue;
+    }
+    const base::DictionaryValue* dictionary = nullptr;
+    if (!data.arg2()->GetAsDictionary(&dictionary)) {
+      continue;
+    }
+    int cur_component, cur_new_state;
+    if (dictionary->GetInteger("safetyCheckComponent", &cur_component) &&
+        cur_component == component &&
+        dictionary->GetInteger("newState", &cur_new_state) &&
+        cur_new_state == new_state) {
+      return true;
+    }
+  }
+  return false;
+}
+
 TEST_F(SafetyCheckHandlerTest, PerformSafetyCheck_AllChecksInvoked) {
   safety_check_->PerformSafetyCheck();
   EXPECT_TRUE(observer_.update_check_started_);
@@ -86,14 +118,23 @@ TEST_F(SafetyCheckHandlerTest, CheckUpdates_Updated) {
   version_updater_->SetReturnedStatus(VersionUpdater::Status::UPDATED);
   safety_check_->PerformSafetyCheck();
   ASSERT_TRUE(observer_.update_check_status_.has_value());
-  EXPECT_EQ(VersionUpdater::Status::UPDATED, observer_.update_check_status_);
+  EXPECT_EQ(SafetyCheckHandler::UpdateStatus::kUpdated,
+            observer_.update_check_status_);
+  EXPECT_TRUE(HasSafetyCheckStatusChangedWithData(
+      static_cast<int>(SafetyCheckHandler::SafetyCheckComponent::kUpdates),
+      static_cast<int>(SafetyCheckHandler::UpdateStatus::kUpdated)));
 }
 
 TEST_F(SafetyCheckHandlerTest, CheckUpdates_NotUpdated) {
-  version_updater_->SetReturnedStatus(VersionUpdater::Status::DISABLED);
+  version_updater_->SetReturnedStatus(
+      VersionUpdater::Status::DISABLED_BY_ADMIN);
   safety_check_->PerformSafetyCheck();
   ASSERT_TRUE(observer_.update_check_status_.has_value());
-  EXPECT_EQ(VersionUpdater::Status::DISABLED, observer_.update_check_status_);
+  EXPECT_EQ(SafetyCheckHandler::UpdateStatus::kDisabledByAdmin,
+            observer_.update_check_status_);
+  EXPECT_TRUE(HasSafetyCheckStatusChangedWithData(
+      static_cast<int>(SafetyCheckHandler::SafetyCheckComponent::kUpdates),
+      static_cast<int>(SafetyCheckHandler::UpdateStatus::kDisabledByAdmin)));
 }
 
 TEST_F(SafetyCheckHandlerTest, CheckSafeBrowsing_Enabled) {
@@ -102,8 +143,11 @@ TEST_F(SafetyCheckHandlerTest, CheckSafeBrowsing_Enabled) {
       ->SetBoolean(prefs::kSafeBrowsingEnabled, true);
   safety_check_->PerformSafetyCheck();
   ASSERT_TRUE(observer_.safe_browsing_check_status_.has_value());
-  EXPECT_EQ(SafetyCheckHandler::SafeBrowsingStatus::ENABLED,
+  EXPECT_EQ(SafetyCheckHandler::SafeBrowsingStatus::kEnabled,
             observer_.safe_browsing_check_status_);
+  EXPECT_TRUE(HasSafetyCheckStatusChangedWithData(
+      static_cast<int>(SafetyCheckHandler::SafetyCheckComponent::kSafeBrowsing),
+      static_cast<int>(SafetyCheckHandler::SafeBrowsingStatus::kEnabled)));
 }
 
 TEST_F(SafetyCheckHandlerTest, CheckSafeBrowsing_Disabled) {
@@ -112,8 +156,11 @@ TEST_F(SafetyCheckHandlerTest, CheckSafeBrowsing_Disabled) {
       ->SetBoolean(prefs::kSafeBrowsingEnabled, false);
   safety_check_->PerformSafetyCheck();
   ASSERT_TRUE(observer_.safe_browsing_check_status_.has_value());
-  EXPECT_EQ(SafetyCheckHandler::SafeBrowsingStatus::DISABLED,
+  EXPECT_EQ(SafetyCheckHandler::SafeBrowsingStatus::kDisabled,
             observer_.safe_browsing_check_status_);
+  EXPECT_TRUE(HasSafetyCheckStatusChangedWithData(
+      static_cast<int>(SafetyCheckHandler::SafetyCheckComponent::kSafeBrowsing),
+      static_cast<int>(SafetyCheckHandler::SafeBrowsingStatus::kDisabled)));
 }
 
 TEST_F(SafetyCheckHandlerTest, CheckSafeBrowsing_DisabledByAdmin) {
@@ -124,6 +171,10 @@ TEST_F(SafetyCheckHandlerTest, CheckSafeBrowsing_DisabledByAdmin) {
                        std::make_unique<base::Value>(false));
   safety_check_->PerformSafetyCheck();
   ASSERT_TRUE(observer_.safe_browsing_check_status_.has_value());
-  EXPECT_EQ(SafetyCheckHandler::SafeBrowsingStatus::DISABLED_BY_ADMIN,
+  EXPECT_EQ(SafetyCheckHandler::SafeBrowsingStatus::kDisabledByAdmin,
             observer_.safe_browsing_check_status_);
+  EXPECT_TRUE(HasSafetyCheckStatusChangedWithData(
+      static_cast<int>(SafetyCheckHandler::SafetyCheckComponent::kSafeBrowsing),
+      static_cast<int>(
+          SafetyCheckHandler::SafeBrowsingStatus::kDisabledByAdmin)));
 }