Commit 9fa4ad1b authored by Ioana Pandele's avatar Ioana Pandele Committed by Commit Bot

Reland "[PwdCheckAndroid] Add delay between check status header updates"

This reverts commit 551bbd27.

Reason for revert: Reland with fix for the status update.

TBR=tedchoc@chromium.org

TBR-ing since the DEPS file has already been reviewed and hasn't changed.

Original change's description:
> Revert "[PwdCheckAndroid] Add delay between check status header updates"
>
> This reverts commit 849b3b5d.
>
> Reason for revert: The assumption that the progress signal comes after
> status signal was wrong, so this change causes a bug when re-running the
> check.
>
> Since this was meant to be merged, reverting + fix is easier than fixing
> in a follow-up CL.
>
> Original change's description:
> > [PwdCheckAndroid] Add delay between check status header updates
> >
> > This CL adds a delay to status updates whenever multiple updates are
> > notified to the mediator in a short time window.
> > The minimum time interval between updates is set to 1 second.
> >
> > Bug: 1122026, 1092444
> > Change-Id: I64d9c95a11a5ed9b9d6fcf8941c260bf8b3a64ac
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2390642
> > Reviewed-by: Ted Choc <tedchoc@chromium.org>
> > Reviewed-by: Friedrich [CET] <fhorschig@chromium.org>
> > Commit-Queue: Ioana Pandele <ioanap@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#805236}
>
> TBR=tedchoc@chromium.org,fhorschig@chromium.org,ioanap@chromium.org,erocchi@google.com
>
> Change-Id: I737eee5e355033542dc9ba8393eedc3390baf5de
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 1122026
> Bug: 1092444
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2401620
> Commit-Queue: Ioana Pandele <ioanap@chromium.org>
> Reviewed-by: Ioana Pandele <ioanap@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#805385}

TBR=tedchoc@chromium.org,fhorschig@chromium.org,ioanap@chromium.org,erocchi@google.com

# Not skipping CQ checks because this is a reland.

Bug: 1122026
Bug: 1092444
Change-Id: Ib43b92f88393634d958582270d5dd9a1edba5829
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2403281Reviewed-by: default avatarFriedrich [CET] <fhorschig@chromium.org>
Commit-Queue: Ioana Pandele <ioanap@chromium.org>
Cr-Commit-Position: refs/heads/master@{#805750}
parent 4313f090
include_rules = [ include_rules = [
"+chrome/android", "+chrome/android",
"+chrome/browser/ui/android", "+chrome/browser/ui/android",
"+content/public/android",
] ]
\ No newline at end of file
...@@ -67,6 +67,7 @@ android_library("internal_java") { ...@@ -67,6 +67,7 @@ android_library("internal_java") {
"//chrome/browser/ui/android/strings:ui_strings_grd", "//chrome/browser/ui/android/strings:ui_strings_grd",
"//components/browser_ui/widget/android:java", "//components/browser_ui/widget/android:java",
"//components/embedder_support/android:util_java", "//components/embedder_support/android:util_java",
"//content/public/android:content_java",
"//third_party/android_deps:androidx_annotation_annotation_java", "//third_party/android_deps:androidx_annotation_annotation_java",
"//third_party/android_deps:androidx_appcompat_appcompat_java", "//third_party/android_deps:androidx_appcompat_appcompat_java",
"//third_party/android_deps:androidx_appcompat_appcompat_resources_java", "//third_party/android_deps:androidx_appcompat_appcompat_resources_java",
......
...@@ -25,12 +25,15 @@ import static org.chromium.chrome.browser.password_check.PasswordCheckProperties ...@@ -25,12 +25,15 @@ import static org.chromium.chrome.browser.password_check.PasswordCheckProperties
import android.content.DialogInterface; import android.content.DialogInterface;
import android.util.Pair; import android.util.Pair;
import androidx.annotation.VisibleForTesting;
import androidx.appcompat.app.AlertDialog; import androidx.appcompat.app.AlertDialog;
import org.chromium.base.task.PostTask;
import org.chromium.chrome.browser.password_check.helper.PasswordCheckChangePasswordHelper; import org.chromium.chrome.browser.password_check.helper.PasswordCheckChangePasswordHelper;
import org.chromium.chrome.browser.password_check.helper.PasswordCheckIconHelper; import org.chromium.chrome.browser.password_check.helper.PasswordCheckIconHelper;
import org.chromium.chrome.browser.password_check.helper.PasswordCheckReauthenticationHelper; import org.chromium.chrome.browser.password_check.helper.PasswordCheckReauthenticationHelper;
import org.chromium.chrome.browser.password_check.helper.PasswordCheckReauthenticationHelper.ReauthReason; import org.chromium.chrome.browser.password_check.helper.PasswordCheckReauthenticationHelper.ReauthReason;
import org.chromium.content_public.browser.UiThreadTaskTraits;
import org.chromium.ui.modelutil.ListModel; import org.chromium.ui.modelutil.ListModel;
import org.chromium.ui.modelutil.MVCListAdapter.ListItem; import org.chromium.ui.modelutil.MVCListAdapter.ListItem;
import org.chromium.ui.modelutil.PropertyModel; import org.chromium.ui.modelutil.PropertyModel;
...@@ -46,6 +49,8 @@ import java.util.List; ...@@ -46,6 +49,8 @@ import java.util.List;
*/ */
class PasswordCheckMediator class PasswordCheckMediator
implements PasswordCheckCoordinator.CredentialEventHandler, PasswordCheck.Observer { implements PasswordCheckCoordinator.CredentialEventHandler, PasswordCheck.Observer {
private static long sStatusUpdateDelayMillis = 1000;
private final PasswordCheckReauthenticationHelper mReauthenticationHelper; private final PasswordCheckReauthenticationHelper mReauthenticationHelper;
private final PasswordCheckChangePasswordHelper mChangePasswordDelegate; private final PasswordCheckChangePasswordHelper mChangePasswordDelegate;
private PropertyModel mModel; private PropertyModel mModel;
...@@ -53,6 +58,7 @@ class PasswordCheckMediator ...@@ -53,6 +58,7 @@ class PasswordCheckMediator
private Runnable mLaunchCheckupInAccount; private Runnable mLaunchCheckupInAccount;
private HashSet<CompromisedCredential> mPreCheckSet; private HashSet<CompromisedCredential> mPreCheckSet;
private final PasswordCheckIconHelper mIconHelper; private final PasswordCheckIconHelper mIconHelper;
private long mLastStatusUpdate;
PasswordCheckMediator(PasswordCheckChangePasswordHelper changePasswordDelegate, PasswordCheckMediator(PasswordCheckChangePasswordHelper changePasswordDelegate,
PasswordCheckReauthenticationHelper reauthenticationHelper, PasswordCheckReauthenticationHelper reauthenticationHelper,
...@@ -106,6 +112,7 @@ class PasswordCheckMediator ...@@ -106,6 +112,7 @@ class PasswordCheckMediator
.with(LAUNCH_ACCOUNT_CHECKUP_ACTION, mLaunchCheckupInAccount) .with(LAUNCH_ACCOUNT_CHECKUP_ACTION, mLaunchCheckupInAccount)
.with(RESTART_BUTTON_ACTION, this::startCheckManually) .with(RESTART_BUTTON_ACTION, this::startCheckManually)
.build())); .build()));
mLastStatusUpdate = System.currentTimeMillis();
} }
if (items.size() > 1) items.removeRange(1, items.size() - 1); if (items.size() > 1) items.removeRange(1, items.size() - 1);
...@@ -120,6 +127,19 @@ class PasswordCheckMediator ...@@ -120,6 +127,19 @@ class PasswordCheckMediator
@Override @Override
public void onPasswordCheckStatusChanged(@PasswordCheckUIStatus int status) { public void onPasswordCheckStatusChanged(@PasswordCheckUIStatus int status) {
long currentTime = System.currentTimeMillis();
if (shouldDelayStatusChange(status, currentTime)) {
mLastStatusUpdate += sStatusUpdateDelayMillis;
PostTask.postDelayedTask(UiThreadTaskTraits.DEFAULT,
() -> changePasswordCheckStatus(status), mLastStatusUpdate - currentTime);
} else {
mLastStatusUpdate = currentTime;
changePasswordCheckStatus(status);
}
}
private void changePasswordCheckStatus(@PasswordCheckUIStatus int status) {
// There is no UI representation of a canceled check. This status can be sent when // There is no UI representation of a canceled check. This status can be sent when
// the bridge and the password check UI are being torn down while a check is running. // the bridge and the password check UI are being torn down while a check is running.
if (status == PasswordCheckUIStatus.CANCELED) return; if (status == PasswordCheckUIStatus.CANCELED) return;
...@@ -139,8 +159,9 @@ class PasswordCheckMediator ...@@ -139,8 +159,9 @@ class PasswordCheckMediator
header = items.get(0).model; header = items.get(0).model;
} }
header.set(CHECK_STATUS, status); header.set(CHECK_STATUS, status);
header.set( Pair<Integer, Integer> progress = header.get(CHECK_PROGRESS);
CHECK_PROGRESS, status == PasswordCheckUIStatus.RUNNING ? UNKNOWN_PROGRESS : null); if (progress == null) progress = UNKNOWN_PROGRESS;
header.set(CHECK_PROGRESS, status == PasswordCheckUIStatus.RUNNING ? progress : null);
Long checkTimestamp = null; Long checkTimestamp = null;
Integer compromisedCredentialCount = null; Integer compromisedCredentialCount = null;
if (status == PasswordCheckUIStatus.IDLE) { if (status == PasswordCheckUIStatus.IDLE) {
...@@ -164,7 +185,10 @@ class PasswordCheckMediator ...@@ -164,7 +185,10 @@ class PasswordCheckMediator
assert remainingInQueue >= 0; assert remainingInQueue >= 0;
PropertyModel header = items.get(0).model; PropertyModel header = items.get(0).model;
header.set(CHECK_STATUS, PasswordCheckUIStatus.RUNNING); if (header.get(CHECK_STATUS) != PasswordCheckUIStatus.RUNNING) {
mLastStatusUpdate = System.currentTimeMillis();
header.set(CHECK_STATUS, PasswordCheckUIStatus.RUNNING);
}
header.set( header.set(
CHECK_PROGRESS, new Pair<>(alreadyProcessed, alreadyProcessed + remainingInQueue)); CHECK_PROGRESS, new Pair<>(alreadyProcessed, alreadyProcessed + remainingInQueue));
header.set(CHECK_TIMESTAMP, null); header.set(CHECK_TIMESTAMP, null);
...@@ -304,6 +328,16 @@ class PasswordCheckMediator ...@@ -304,6 +328,16 @@ class PasswordCheckMediator
&& mModel.get(ITEMS).get(0).model.get(CHECK_STATUS) && mModel.get(ITEMS).get(0).model.get(CHECK_STATUS)
== PasswordCheckUIStatus.RUNNING; == PasswordCheckUIStatus.RUNNING;
} }
private boolean shouldDelayStatusChange(
@PasswordCheckUIStatus int newStatus, long currentTime) {
ListModel<ListItem> items = mModel.get(ITEMS);
return items.size() > 0
&& items.get(0).model.get(CHECK_STATUS) == PasswordCheckUIStatus.RUNNING
&& newStatus != PasswordCheckUIStatus.RUNNING
&& mLastStatusUpdate + sStatusUpdateDelayMillis > currentTime;
}
private ListItem createEntryForCredential(CompromisedCredential credential) { private ListItem createEntryForCredential(CompromisedCredential credential) {
PropertyModel credentialModel = PropertyModel credentialModel =
new PropertyModel new PropertyModel
...@@ -360,4 +394,9 @@ class PasswordCheckMediator ...@@ -360,4 +394,9 @@ class PasswordCheckMediator
return originComparisonResult == 0 ? usernameComparisonResult : originComparisonResult; return originComparisonResult == 0 ? usernameComparisonResult : originComparisonResult;
}); });
} }
@VisibleForTesting
protected static void setStatusUpdateDelayMillis(long statusUpdateDelayMillis) {
sStatusUpdateDelayMillis = statusUpdateDelayMillis;
}
} }
...@@ -129,6 +129,7 @@ public class PasswordCheckControllerTest { ...@@ -129,6 +129,7 @@ public class PasswordCheckControllerTest {
mChangePasswordDelegate, mReauthenticationHelper, mIconHelper); mChangePasswordDelegate, mReauthenticationHelper, mIconHelper);
PasswordCheckFactory.setPasswordCheckForTesting(mPasswordCheck); PasswordCheckFactory.setPasswordCheckForTesting(mPasswordCheck);
mMediator.initialize(mModel, mDelegate, PasswordCheckReferrer.PASSWORD_SETTINGS, () -> {}); mMediator.initialize(mModel, mDelegate, PasswordCheckReferrer.PASSWORD_SETTINGS, () -> {});
PasswordCheckMediator.setStatusUpdateDelayMillis(0);
} }
@Test @Test
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment