Safety check: move parent status to backend (part 1)

* This is done as preparation for a subsequent refactoring of the
  safety check WebUi code. It moves the source of truth for the
  safety check parent status from the JS side to the C++ side. This
  removes some of the interdependencies of parent to children (and
  vice versa), and allows for a subsequent splitting of parent and
  children into individual parts on the WebUi side.
* This CL is part 1 of the change, which adds logic to handle the parent
  state in C++ and the communication from C++ to JS.

Bug: 1015841
Change-Id: I3716457964377ae3177884d5987281fd2c470a2a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2144022Reviewed-by: Esmael Elmoslimany <aee@chromium.org>
Commit-Queue: Rainhard Findling <rainhard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#759692}

chrome/browser/resources/settings/safety_check_page/BUILD.gn

@@ -92,7 +92,7 @@ polymer_modulizer("safety_check_page") {
                    "chrome/browser/resources/settings/hats_browser_proxy.html|HatsBrowserProxyImpl",
                    "chrome/browser/resources/settings/metrics_browser_proxy.html|SafetyCheckInteractions,MetricsBrowserProxy,MetricsBrowserProxyImpl",
                    "chrome/browser/resources/settings/open_window_proxy.html|OpenWindowProxyImpl",
-                   "chrome/browser/resources/settings/safety_check_page/safety_check_browser_proxy.html|SafetyCheckBrowserProxy,SafetyCheckBrowserProxyImpl,SafetyCheckExtensionsStatus,SafetyCheckPasswordsStatus,SafetyCheckUpdatesStatus,SafetyCheckSafeBrowsingStatus,SafetyCheckCallbackConstants",
+                   "chrome/browser/resources/settings/safety_check_page/safety_check_browser_proxy.html|SafetyCheckBrowserProxy,SafetyCheckBrowserProxyImpl,SafetyCheckExtensionsStatus,SafetyCheckParentStatus,SafetyCheckPasswordsStatus,SafetyCheckUpdatesStatus,SafetyCheckSafeBrowsingStatus,SafetyCheckCallbackConstants",
                    "ui/webui/resources/html/assert.html|assertNotReached",
                    "ui/webui/resources/html/polymer.html|Polymer,html,flush",
                  ]

chrome/browser/resources/settings/safety_check_page/safety_check_browser_proxy.js

@@ -19,12 +19,26 @@ cr.define('settings', function() {
    * @enum {string}
    */
   /* #export */ const SafetyCheckCallbackConstants = {
+    PARENT_CHANGED: 'safety-check-parent-status-changed',
     UPDATES_CHANGED: 'safety-check-updates-status-changed',
     PASSWORDS_CHANGED: 'safety-check-passwords-status-changed',
     SAFE_BROWSING_CHANGED: 'safety-check-safe-browsing-status-changed',
     EXTENSIONS_CHANGED: 'safety-check-extensions-status-changed',
   };
 
+  /**
+   * States of the safety check parent element.
+   * Needs to be kept in sync with ParentStatus in
+   * chrome/browser/ui/webui/settings/safety_check_handler.h
+   * @enum {number}
+   */
+  /* #export */
+  const SafetyCheckParentStatus = {
+    BEFORE: 0,
+    CHECKING: 1,
+    AFTER: 2,
+  };
+
   /**
    * States of the safety check updates element.
    * Needs to be kept in sync with UpdatesStatus in
@@ -124,6 +138,7 @@ cr.define('settings', function() {
   // #cr_define_end
   return {
     SafetyCheckCallbackConstants,
+    SafetyCheckParentStatus,
     SafetyCheckUpdatesStatus,
     SafetyCheckPasswordsStatus,
     SafetyCheckSafeBrowsingStatus,

chrome/browser/resources/settings/safety_check_page/safety_check_page.js

@@ -10,16 +10,6 @@
 
 (function() {
 
-/**
- * States of the safety check parent element.
- * @enum {number}
- */
-const ParentStatus = {
-  BEFORE: 0,
-  CHECKING: 1,
-  AFTER: 2,
-};
-
 /**
  * UI states a safety check child can be in. Defines the basic UI of the child.
  * @enum {number}
@@ -31,6 +21,14 @@ const ChildUiStatus = {
   WARNING: 3,
 };
 
+/**
+ * @typedef {{
+ *   newState: settings.SafetyCheckParentStatus,
+ *   displayString: string,
+ * }}
+ */
+let ParentChangedEvent;
+
 /**
  * @typedef {{
  *   newState: settings.SafetyCheckUpdatesStatus,
@@ -74,11 +72,11 @@ Polymer({
   properties: {
     /**
      * Current state of the safety check parent element.
-     * @private {!ParentStatus}
+     * @private {!settings.SafetyCheckParentStatus}
      */
     parentStatus_: {
       type: Number,
-      value: ParentStatus.BEFORE,
+      value: settings.SafetyCheckParentStatus.BEFORE,
     },
 
     /**
@@ -172,6 +170,9 @@ Polymer({
     this.metricsBrowserProxy_ = settings.MetricsBrowserProxyImpl.getInstance();
 
     // Register for safety check status updates.
+    this.addWebUIListener(
+        settings.SafetyCheckCallbackConstants.PARENT_CHANGED,
+        this.onSafetyCheckParentChanged_.bind(this));
     this.addWebUIListener(
         settings.SafetyCheckCallbackConstants.UPDATES_CHANGED,
         this.onSafetyCheckUpdatesChanged_.bind(this));
@@ -202,7 +203,7 @@ Polymer({
 
     // Update UI.
     this.parentDisplayString_ = this.i18n('safetyCheckRunning');
-    this.parentStatus_ = ParentStatus.CHECKING;
+    this.parentStatus_ = settings.SafetyCheckParentStatus.CHECKING;
     // Reset all children states.
     this.updatesStatus_ = settings.SafetyCheckUpdatesStatus.CHECKING;
     this.passwordsStatus_ = settings.SafetyCheckPasswordsStatus.CHECKING;
@@ -231,7 +232,7 @@ Polymer({
         this.extensionsStatus_ !=
             settings.SafetyCheckExtensionsStatus.CHECKING) {
       // Update UI.
-      this.parentStatus_ = ParentStatus.AFTER;
+      this.parentStatus_ = settings.SafetyCheckParentStatus.AFTER;
       // Start periodic safety check parent ran string updates.
       const timestamp = Date.now();
       const update = async () => {
@@ -250,6 +251,15 @@ Polymer({
     }
   },
 
+  /**
+   * @param {!ParentChangedEvent} event
+   * @private
+   */
+  onSafetyCheckParentChanged_: function(event) {
+    // TODO(crbug.com/1015841): Use parent state from backend instead of
+    // computing and storing parent state in WebUI.
+  },
+
   /**
    * @param {!UpdatesChangedEvent} event
    * @private
@@ -295,7 +305,7 @@ Polymer({
    * @return {boolean}
    */
   shouldShowParentButton_: function() {
-    return this.parentStatus_ === ParentStatus.BEFORE;
+    return this.parentStatus_ === settings.SafetyCheckParentStatus.BEFORE;
   },
 
   /**
@@ -303,7 +313,7 @@ Polymer({
    * @return {boolean}
    */
   shouldShowParentIconButton_: function() {
-    return this.parentStatus_ !== ParentStatus.BEFORE;
+    return this.parentStatus_ !== settings.SafetyCheckParentStatus.BEFORE;
   },
 
   /** @private */
@@ -327,7 +337,7 @@ Polymer({
    * @return {boolean}
    */
   shouldShowChildren_: function() {
-    return this.parentStatus_ != ParentStatus.BEFORE;
+    return this.parentStatus_ != settings.SafetyCheckParentStatus.BEFORE;
   },
 
   /**

chrome/browser/resources/settings/settings.gni

@@ -98,6 +98,7 @@ settings_namespace_rewrites = [
   "settings.SafetyCheckBrowserProxy|SafetyCheckBrowserProxy",
   "settings.SafetyCheckCallbackConstants|SafetyCheckCallbackConstants",
   "settings.SafetyCheckExtensionsStatus|SafetyCheckExtensionsStatus",
+  "settings.SafetyCheckParentStatus|SafetyCheckParentStatus",
   "settings.SafetyCheckPasswordsStatus|SafetyCheckPasswordsStatus",
   "settings.SafetyCheckSafeBrowsingStatus|SafetyCheckSafeBrowsingStatus",
   "settings.SafetyCheckUpdatesStatus|SafetyCheckUpdatesStatus",

chrome/browser/resources/settings/settings.js

@@ -35,6 +35,6 @@ export {PrivacyPageBrowserProxyImpl, SecureDnsMode, SecureDnsUiManagementMode} f
 export {ResetBrowserProxyImpl} from './reset_page/reset_browser_proxy.js';
 export {buildRouter, routes} from './route.m.js';
 export {Route, Router} from './router.m.js';
-export {SafetyCheckBrowserProxy, SafetyCheckBrowserProxyImpl, SafetyCheckCallbackConstants, SafetyCheckExtensionsStatus, SafetyCheckPasswordsStatus, SafetyCheckSafeBrowsingStatus, SafetyCheckUpdatesStatus} from './safety_check_page/safety_check_browser_proxy.m.js';
+export {SafetyCheckBrowserProxy, SafetyCheckBrowserProxyImpl, SafetyCheckCallbackConstants, SafetyCheckExtensionsStatus, SafetyCheckParentStatus, SafetyCheckPasswordsStatus, SafetyCheckSafeBrowsingStatus, SafetyCheckUpdatesStatus} from './safety_check_page/safety_check_browser_proxy.m.js';
 export {SearchEnginesBrowserProxyImpl} from './search_engines_page/search_engines_browser_proxy.m.js';
 export {getSearchManager, SearchRequest, setSearchManagerForTesting} from './search_settings.m.js';

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

@@ -33,6 +33,7 @@
 namespace {
 
 // Constants for communication with JS.
+constexpr char kParentEvent[] = "safety-check-parent-status-changed";
 constexpr char kUpdatesEvent[] = "safety-check-updates-status-changed";
 constexpr char kPasswordsEvent[] = "safety-check-passwords-status-changed";
 constexpr char kSafeBrowsingEvent[] =
@@ -84,6 +85,31 @@ SafetyCheckHandler::~SafetyCheckHandler() = default;
 void SafetyCheckHandler::PerformSafetyCheck() {
   AllowJavascript();
 
+  // Reset status of parent and children, which might have been set from a
+  // previous run of safety check.
+  parent_status_ = ParentStatus::kChecking;
+  update_status_ = UpdateStatus::kChecking;
+  passwords_status_ = PasswordsStatus::kChecking;
+  safe_browsing_status_ = SafeBrowsingStatus::kChecking;
+  extensions_status_ = ExtensionsStatus::kChecking;
+
+  // Update WebUi.
+  FireBasicSafetyCheckWebUiListener(kParentEvent,
+                                    static_cast<int>(parent_status_),
+                                    GetStringForParent(parent_status_));
+  FireBasicSafetyCheckWebUiListener(
+      kUpdatesEvent, static_cast<int>(update_status_), base::UTF8ToUTF16(""));
+  FireBasicSafetyCheckWebUiListener(kPasswordsEvent,
+                                    static_cast<int>(passwords_status_),
+                                    base::UTF8ToUTF16(""));
+  FireBasicSafetyCheckWebUiListener(kSafeBrowsingEvent,
+                                    static_cast<int>(safe_browsing_status_),
+                                    base::UTF8ToUTF16(""));
+  FireBasicSafetyCheckWebUiListener(kExtensionsEvent,
+                                    static_cast<int>(extensions_status_),
+                                    base::UTF8ToUTF16(""));
+
+  // Run safety check.
   if (!version_updater_) {
     version_updater_.reset(VersionUpdater::Create(web_ui()->GetWebContents()));
   }
@@ -245,27 +271,28 @@ void SafetyCheckHandler::OnUpdateCheckResult(VersionUpdater::Status status,
                                              const std::string& version,
                                              int64_t update_size,
                                              const base::string16& message) {
-  UpdateStatus update_status = ConvertToUpdateStatus(status);
-  base::DictionaryValue event;
-  event.SetIntKey(kNewState, static_cast<int>(update_status));
-  event.SetStringKey(kDisplayString, GetStringForUpdates(update_status));
-  FireWebUIListener(kUpdatesEvent, event);
-  if (update_status != UpdateStatus::kChecking) {
+  update_status_ = ConvertToUpdateStatus(status);
+  if (update_status_ != UpdateStatus::kChecking) {
     base::UmaHistogramEnumeration("Settings.SafetyCheck.UpdatesResult",
-                                  update_status);
+                                  update_status_);
   }
+  FireBasicSafetyCheckWebUiListener(kUpdatesEvent,
+                                    static_cast<int>(update_status_),
+                                    GetStringForUpdates(update_status_));
+  CompleteParentIfChildrenCompleted();
 }
 
 void SafetyCheckHandler::OnSafeBrowsingCheckResult(
     SafetyCheckHandler::SafeBrowsingStatus status) {
-  base::DictionaryValue event;
-  event.SetIntKey(kNewState, static_cast<int>(status));
-  event.SetStringKey(kDisplayString, GetStringForSafeBrowsing(status));
-  FireWebUIListener(kSafeBrowsingEvent, event);
-  if (status != SafeBrowsingStatus::kChecking) {
+  safe_browsing_status_ = status;
+  if (safe_browsing_status_ != SafeBrowsingStatus::kChecking) {
     base::UmaHistogramEnumeration("Settings.SafetyCheck.SafeBrowsingResult",
-                                  status);
+                                  safe_browsing_status_);
   }
+  FireBasicSafetyCheckWebUiListener(
+      kSafeBrowsingEvent, static_cast<int>(safe_browsing_status_),
+      GetStringForSafeBrowsing(safe_browsing_status_));
+  CompleteParentIfChildrenCompleted();
 }
 
 void SafetyCheckHandler::OnPasswordsCheckResult(PasswordsStatus status,
@@ -284,6 +311,8 @@ void SafetyCheckHandler::OnPasswordsCheckResult(PasswordsStatus status,
     base::UmaHistogramEnumeration("Settings.SafetyCheck.PasswordsResult",
                                   status);
   }
+  passwords_status_ = status;
+  CompleteParentIfChildrenCompleted();
 }
 
 void SafetyCheckHandler::OnExtensionsCheckResult(
@@ -309,6 +338,21 @@ void SafetyCheckHandler::OnExtensionsCheckResult(
     base::UmaHistogramEnumeration("Settings.SafetyCheck.ExtensionsResult",
                                   status);
   }
+  extensions_status_ = status;
+  CompleteParentIfChildrenCompleted();
+}
+
+base::string16 SafetyCheckHandler::GetStringForParent(ParentStatus status) {
+  switch (status) {
+    case ParentStatus::kBefore:
+      return l10n_util::GetStringUTF16(
+          IDS_SETTINGS_SAFETY_CHECK_PARENT_PRIMARY_LABEL_BEFORE);
+    case ParentStatus::kChecking:
+      return l10n_util::GetStringUTF16(IDS_SETTINGS_SAFETY_CHECK_RUNNING);
+    case ParentStatus::kAfter:
+      return l10n_util::GetStringUTF16(
+          IDS_SETTINGS_SAFETY_CHECK_PARENT_PRIMARY_LABEL_AFTER);
+  }
 }
 
 base::string16 SafetyCheckHandler::GetStringForUpdates(UpdateStatus status) {
@@ -601,3 +645,29 @@ void SafetyCheckHandler::RegisterMessages() {
       base::BindRepeating(&SafetyCheckHandler::HandleGetParentRanDisplayString,
                           base::Unretained(this)));
 }
+
+void SafetyCheckHandler::CompleteParentIfChildrenCompleted() {
+  if (update_status_ != UpdateStatus::kChecking &&
+      passwords_status_ != PasswordsStatus::kChecking &&
+      safe_browsing_status_ != SafeBrowsingStatus::kChecking &&
+      extensions_status_ != ExtensionsStatus::kChecking) {
+    // TODO(crbug.com/1015841): Minor improvement: also store the timestamp when
+    // safety check completed in the backend instead of in the frontend. This
+    // allows computing the safety check parent ran string on all platforms
+    // without the frontend handling the timestamp.
+    parent_status_ = ParentStatus::kAfter;
+    FireBasicSafetyCheckWebUiListener(kParentEvent,
+                                      static_cast<int>(parent_status_),
+                                      GetStringForParent(parent_status_));
+  }
+}
+
+void SafetyCheckHandler::FireBasicSafetyCheckWebUiListener(
+    const std::string& event_name,
+    int new_state,
+    const base::string16& display_string) {
+  base::DictionaryValue event;
+  event.SetIntKey(kNewState, new_state);
+  event.SetStringKey(kDisplayString, display_string);
+  FireWebUIListener(event_name, event);
+}

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

@@ -30,6 +30,14 @@ class SafetyCheckHandler
     : public settings::SettingsPageUIHandler,
       public password_manager::BulkLeakCheckService::Observer {
  public:
+  // The following enum represent the state of the safety check parent
+  // component and  should be kept in sync with the JS frontend
+  // (safety_check_browser_proxy.js).
+  enum class ParentStatus {
+    kBefore = 0,
+    kChecking = 1,
+    kAfter = 2,
+  };
   // The following enums represent the state of each component of the safety
   // check and should be kept in sync with the JS frontend
   // (safety_check_browser_proxy.js) and |SafetyCheck*| metrics enums in
@@ -164,6 +172,7 @@ class SafetyCheckHandler
 
   // Methods for building user-visible strings based on the safety check
   // state.
+  base::string16 GetStringForParent(ParentStatus status);
   base::string16 GetStringForUpdates(UpdateStatus status);
   base::string16 GetStringForSafeBrowsing(SafeBrowsingStatus status);
   base::string16 GetStringForPasswords(PasswordsStatus status,
@@ -197,6 +206,22 @@ class SafetyCheckHandler
   // WebUIMessageHandler implementation.
   void RegisterMessages() override;
 
+  // Updates the parent status from the children statuses.
+  void CompleteParentIfChildrenCompleted();
+
+  // Fire a safety check element WebUI update with a state and string.
+  void FireBasicSafetyCheckWebUiListener(const std::string& event_name,
+                                         int new_state,
+                                         const base::string16& display_string);
+
+  // The current status of the safety check elements. Before safety
+  // check is started, the parent is in the 'before' state.
+  ParentStatus parent_status_ = ParentStatus::kBefore;
+  UpdateStatus update_status_ = UpdateStatus::kChecking;
+  PasswordsStatus passwords_status_ = PasswordsStatus::kChecking;
+  SafeBrowsingStatus safe_browsing_status_ = SafeBrowsingStatus::kChecking;
+  ExtensionsStatus extensions_status_ = ExtensionsStatus::kChecking;
+
   std::unique_ptr<VersionUpdater> version_updater_;
   password_manager::BulkLeakCheckService* leak_service_ = nullptr;
   extensions::PasswordsPrivateDelegate* passwords_delegate_ = nullptr;