Commit 551bbd27 authored by Ioana Pandele's avatar Ioana Pandele Committed by Commit Bot

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: default avatarIoana Pandele <ioanap@chromium.org>
Cr-Commit-Position: refs/heads/master@{#805385}
parent dc952b0b
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,7 +67,6 @@ android_library("internal_java") { ...@@ -67,7 +67,6 @@ 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,15 +25,12 @@ import static org.chromium.chrome.browser.password_check.PasswordCheckProperties ...@@ -25,15 +25,12 @@ 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;
...@@ -49,8 +46,6 @@ import java.util.List; ...@@ -49,8 +46,6 @@ import java.util.List;
*/ */
class PasswordCheckMediator class PasswordCheckMediator
implements PasswordCheckCoordinator.CredentialEventHandler, PasswordCheck.Observer { implements PasswordCheckCoordinator.CredentialEventHandler, PasswordCheck.Observer {
private static long sStatusUpdateDelayMillis = 1000; // 1 second
private final PasswordCheckReauthenticationHelper mReauthenticationHelper; private final PasswordCheckReauthenticationHelper mReauthenticationHelper;
private final PasswordCheckChangePasswordHelper mChangePasswordDelegate; private final PasswordCheckChangePasswordHelper mChangePasswordDelegate;
private PropertyModel mModel; private PropertyModel mModel;
...@@ -58,7 +53,6 @@ class PasswordCheckMediator ...@@ -58,7 +53,6 @@ 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,
...@@ -112,7 +106,6 @@ class PasswordCheckMediator ...@@ -112,7 +106,6 @@ 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);
...@@ -127,21 +120,6 @@ class PasswordCheckMediator ...@@ -127,21 +120,6 @@ class PasswordCheckMediator
@Override @Override
public void onPasswordCheckStatusChanged(@PasswordCheckUIStatus int status) { public void onPasswordCheckStatusChanged(@PasswordCheckUIStatus int status) {
long currentTime = System.currentTimeMillis();
ListModel<ListItem> items = mModel.get(ITEMS);
if (items.size() > 0
&& items.get(0).model.get(CHECK_STATUS) == PasswordCheckUIStatus.RUNNING
&& mLastStatusUpdate + sStatusUpdateDelayMillis > 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;
...@@ -161,9 +139,8 @@ class PasswordCheckMediator ...@@ -161,9 +139,8 @@ class PasswordCheckMediator
header = items.get(0).model; header = items.get(0).model;
} }
header.set(CHECK_STATUS, status); header.set(CHECK_STATUS, status);
Pair<Integer, Integer> progress = header.get(CHECK_PROGRESS); header.set(
if (progress == null) progress = UNKNOWN_PROGRESS; CHECK_PROGRESS, status == PasswordCheckUIStatus.RUNNING ? UNKNOWN_PROGRESS : null);
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) {
...@@ -187,7 +164,7 @@ class PasswordCheckMediator ...@@ -187,7 +164,7 @@ class PasswordCheckMediator
assert remainingInQueue >= 0; assert remainingInQueue >= 0;
PropertyModel header = items.get(0).model; PropertyModel header = items.get(0).model;
assert header.get(CHECK_STATUS) == PasswordCheckUIStatus.RUNNING; 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);
...@@ -383,9 +360,4 @@ class PasswordCheckMediator ...@@ -383,9 +360,4 @@ class PasswordCheckMediator
return originComparisonResult == 0 ? usernameComparisonResult : originComparisonResult; return originComparisonResult == 0 ? usernameComparisonResult : originComparisonResult;
}); });
} }
@VisibleForTesting
protected static void setStatusUpdateDelayMillis(long statusUpdateDelayMillis) {
sStatusUpdateDelayMillis = statusUpdateDelayMillis;
}
} }
...@@ -129,7 +129,6 @@ public class PasswordCheckControllerTest { ...@@ -129,7 +129,6 @@ 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