Safety check on Android: fixed disappearing icons and UI logic

* Added logic to delay setting status icons on initialization
* Removed unnecessary logic to invoke setInitialState in the Mediator constructor

Bug: 1070620
Change-Id: I21174d0c874f56eb6ee218fa844bead1fe4a0451
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2346285
Commit-Queue: Natalie Chouinard <chouinard@chromium.org>
Reviewed-by: Natalie Chouinard <chouinard@chromium.org>
Auto-Submit: Andrey Zaytsev <andzaytsev@google.com>
Cr-Commit-Position: refs/heads/master@{#796897}

chrome/browser/safety_check/android/java/src/org/chromium/chrome/browser/safety_check/SafetyCheckElementPreference.java

@@ -12,6 +12,7 @@ import android.widget.ImageView;
 import androidx.annotation.DrawableRes;
 import androidx.preference.PreferenceViewHolder;
 
+import org.chromium.base.Callback;
 import org.chromium.components.browser_ui.settings.ChromeBasePreference;
 
 /**
@@ -23,12 +24,21 @@ public class SafetyCheckElementPreference extends ChromeBasePreference {
     private View mProgressBar;
     private ImageView mStatusView;
 
+    /**
+     * Represents an action to take once the view elements are available.
+     * This is needed because |SafetyCheckMediator::setInitialState()| is invoked before all the
+     * nested views are available, so setting icons should be delayed.
+     */
+    private Callback<Void> mDelayedAction;
+
     /**
      * Creates a new object and sets the widget layout.
      */
     public SafetyCheckElementPreference(Context context, AttributeSet attrs) {
         super(context, attrs);
         setWidgetLayoutResource(R.layout.safety_check_status);
+        // No delayed action to take.
+        mDelayedAction = null;
     }
 
     /**
@@ -39,14 +49,21 @@ public class SafetyCheckElementPreference extends ChromeBasePreference {
         super.onBindViewHolder(holder);
         mProgressBar = holder.findViewById(R.id.progress);
         mStatusView = (ImageView) holder.findViewById(R.id.status_view);
+        // If there is a delayed action - take it.
+        if (mDelayedAction != null) {
+            mDelayedAction.onResult(null);
+        }
+        // Reset the delayed action.
+        mDelayedAction = null;
     }
 
     /**
      * Displays the progress bar.
      */
     void showProgressBar() {
-        // Ignore if this gets invoked before onBindViewHolder.
+        // Delay if this gets invoked before onBindViewHolder.
         if (mStatusView == null || mProgressBar == null) {
+            mDelayedAction = (ignored) -> showProgressBar();
             return;
         }
         mStatusView.setVisibility(View.GONE);
@@ -58,8 +75,9 @@ public class SafetyCheckElementPreference extends ChromeBasePreference {
      * @param icon An icon to display.
      */
     void showStatusIcon(@DrawableRes int icon) {
-        // Ignore if this gets invoked before onBindViewHolder.
+        // Delay if this gets invoked before onBindViewHolder.
         if (mStatusView == null || mProgressBar == null) {
+            mDelayedAction = (ignored) -> showStatusIcon(icon);
             return;
         }
         mStatusView.setImageResource(icon);
@@ -71,8 +89,9 @@ public class SafetyCheckElementPreference extends ChromeBasePreference {
      * Hides anything in the status area.
      */
     void clearStatusIndicator() {
-        // Ignore if this gets invoked before onBindViewHolder.
+        // Delay if this gets invoked before onBindViewHolder.
         if (mStatusView == null || mProgressBar == null) {
+            mDelayedAction = (ignored) -> clearStatusIndicator();
             return;
         }
         mStatusView.setVisibility(View.GONE);

chrome/browser/safety_check/android/java/src/org/chromium/chrome/browser/safety_check/SafetyCheckMediator.java

@@ -115,8 +115,6 @@ class SafetyCheckMediator implements PasswordCheck.Observer, SafetyCheckCommonOb
         this(model, client, settingsLauncher, null, new Handler());
         // Have to initialize this after the constructor call, since a "this" instance is needed.
         mSafetyCheckBridge = new SafetyCheckBridge(SafetyCheckMediator.this);
-        // Determine and set the initial state.
-        setInitialState();
     }
 
     @VisibleForTesting
@@ -127,8 +125,6 @@ class SafetyCheckMediator implements PasswordCheck.Observer, SafetyCheckCommonOb
         mSafetyCheckBridge = bridge;
         mHandler = handler;
         mPreferenceManager = SharedPreferencesManager.getInstance();
-        mPasswordsLoaded = false;
-        mLeaksLoaded = false;
         // Set the listener for clicking the updates element.
         mModel.set(SafetyCheckProperties.UPDATES_CLICK_LISTENER,
                 (Preference.OnPreferenceClickListener) (p) -> {
@@ -176,10 +172,6 @@ class SafetyCheckMediator implements PasswordCheck.Observer, SafetyCheckCommonOb
         mModel.set(SafetyCheckProperties.LAST_RUN_TIMESTAMP,
                 mPreferenceManager.readLong(
                         ChromePreferenceKeys.SETTINGS_SAFETY_CHECK_LAST_RUN_TIMESTAMP, 0));
-        if (mSafetyCheckBridge != null) {
-            // Determine and set the initial state.
-            setInitialState();
-        }
     }
 
     /**
@@ -204,6 +196,10 @@ class SafetyCheckMediator implements PasswordCheck.Observer, SafetyCheckCommonOb
         }
         mModel.set(SafetyCheckProperties.PASSWORDS_STATE, PasswordsState.CHECKING);
         mLoadStage = PasswordCheckLoadStage.INITIAL_WAIT_FOR_LOAD;
+        // Reset the status of the password disk loads. If it's loaded, PasswordCheck will invoke
+        // the callbacks again (the |callImmediatelyIfReady| argument to |addObserver| is true).
+        mPasswordsLoaded = false;
+        mLeaksLoaded = false;
         // Refresh the PasswordCheck instance, since it's not guaranteed to be the same.
         mPasswordCheck = PasswordCheckFactory.getOrCreate();
         mPasswordCheck.addObserver(this, true);

chrome/browser/safety_check/android/javatests/src/org/chromium/chrome/browser/safety_check/SafetyCheckMediatorTest.java

@@ -381,7 +381,7 @@ public class SafetyCheckMediatorTest {
     @Test
     public void testPasswordsInitialLoadDuringInitialState() {
         // Order: initial state -> load completed -> done.
-        // setInitialState was invoked on Mediator creation.
+        mMediator.setInitialState();
         assertEquals(PasswordsState.CHECKING, mModel.get(PASSWORDS_STATE));
 
         mMediator.onCompromisedCredentialsFetchCompleted();
@@ -398,7 +398,7 @@ public class SafetyCheckMediatorTest {
     @Test
     public void testPasswordsInitialLoadDuringRunningCheck() {
         // Order: initial state -> safety check triggered -> load completed -> check done.
-        // setInitialState was invoked on Mediator creation.
+        mMediator.setInitialState();
         assertEquals(PasswordsState.CHECKING, mModel.get(PASSWORDS_STATE));
 
         mMediator.performSafetyCheck();
@@ -419,7 +419,7 @@ public class SafetyCheckMediatorTest {
     @Test
     public void testPasswordsInitialLoadAfterRunningCheck() {
         // Order: initial state -> safety check triggered -> check done -> load completed.
-        // setInitialState was invoked on Mediator creation.
+        mMediator.setInitialState();
         assertEquals(PasswordsState.CHECKING, mModel.get(PASSWORDS_STATE));
 
         mMediator.performSafetyCheck();
@@ -440,7 +440,7 @@ public class SafetyCheckMediatorTest {
     @Test
     public void testPasswordsInitialLoadCheckReturnsError() {
         // Order: initial state -> safety check triggered -> check error -> load ignored.
-        // setInitialState was invoked on Mediator creation.
+        mMediator.setInitialState();
         assertEquals(PasswordsState.CHECKING, mModel.get(PASSWORDS_STATE));
 
         mMediator.performSafetyCheck();

chrome/browser/safety_check/android/javatests/src/org/chromium/chrome/browser/safety_check/SafetyCheckSettingsFragmentTest.java

@@ -7,6 +7,10 @@ package org.chromium.chrome.browser.safety_check;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.when;
 
 import android.content.Context;
 import android.support.test.InstrumentationRegistry;
@@ -69,6 +73,18 @@ public class SafetyCheckSettingsFragmentTest {
     public void setUp() {
         MockitoAnnotations.initMocks(this);
         PasswordCheckFactory.setPasswordCheckForTesting(mPasswordCheck);
+        // Make the passwords initial status return immediately to avoid spinning animation.
+        when(mPasswordCheck.getSavedPasswordsCount()).thenReturn(0);
+        when(mPasswordCheck.getCompromisedCredentialsCount()).thenReturn(0);
+        doAnswer(invocation -> {
+            PasswordCheck.Observer observer =
+                    (PasswordCheck.Observer) (invocation.getArguments()[0]);
+            observer.onCompromisedCredentialsFetchCompleted();
+            observer.onSavedPasswordsFetchCompleted();
+            return null;
+        })
+                .when(mPasswordCheck)
+                .addObserver(any(SafetyCheckMediator.class), eq(true));
     }
 
     @Test