Commit e8290cd3 authored by Maxim Kolosovskiy's avatar Maxim Kolosovskiy Committed by Commit Bot

[Passwords] Report |DID_NOTHING| in automatic change metrics

Bug: 1132230
Change-Id: Ibae4da2522b4df619ad0848217cf3382b35c81f2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2438454
Commit-Queue: Maxim Kolosovskiy  <kolos@chromium.org>
Reviewed-by: default avatarFriedrich [CET] <fhorschig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812622}
parent 97c5b52f
...@@ -103,6 +103,7 @@ class PasswordCheckCoordinator implements PasswordCheckComponentUi, LifecycleObs ...@@ -103,6 +103,7 @@ class PasswordCheckCoordinator implements PasswordCheckComponentUi, LifecycleObs
@Override @Override
public void onResumeFragment() { public void onResumeFragment() {
mMediator.onResumeFragment();
mReauthenticationHelper.onReauthenticationMaybeHappened(); mReauthenticationHelper.onReauthenticationMaybeHappened();
} }
...@@ -110,6 +111,8 @@ class PasswordCheckCoordinator implements PasswordCheckComponentUi, LifecycleObs ...@@ -110,6 +111,8 @@ class PasswordCheckCoordinator implements PasswordCheckComponentUi, LifecycleObs
public void onDestroyFragment() { public void onDestroyFragment() {
mMediator.stopCheck(); mMediator.stopCheck();
if (mFragmentView.getActivity() == null || mFragmentView.getActivity().isFinishing()) { if (mFragmentView.getActivity() == null || mFragmentView.getActivity().isFinishing()) {
mMediator
.onUserLeavesCheckPage(); // Should be called only if the activity is finishing.
mMediator.destroy(); mMediator.destroy();
mModel = null; mModel = null;
} }
......
...@@ -59,6 +59,7 @@ class PasswordCheckMediator ...@@ -59,6 +59,7 @@ class PasswordCheckMediator
private HashSet<CompromisedCredential> mPreCheckSet; private HashSet<CompromisedCredential> mPreCheckSet;
private final PasswordCheckIconHelper mIconHelper; private final PasswordCheckIconHelper mIconHelper;
private long mLastStatusUpdate; private long mLastStatusUpdate;
private boolean mCctIsOpened;
PasswordCheckMediator(PasswordCheckChangePasswordHelper changePasswordDelegate, PasswordCheckMediator(PasswordCheckChangePasswordHelper changePasswordDelegate,
PasswordCheckReauthenticationHelper reauthenticationHelper, PasswordCheckReauthenticationHelper reauthenticationHelper,
...@@ -73,6 +74,7 @@ class PasswordCheckMediator ...@@ -73,6 +74,7 @@ class PasswordCheckMediator
mModel = model; mModel = model;
mDelegate = delegate; mDelegate = delegate;
mLaunchCheckupInAccount = launchCheckupInAccount; mLaunchCheckupInAccount = launchCheckupInAccount;
mCctIsOpened = false;
PasswordCheckMetricsRecorder.recordPasswordCheckReferrer(passwordCheckReferrer); PasswordCheckMetricsRecorder.recordPasswordCheckReferrer(passwordCheckReferrer);
...@@ -89,8 +91,27 @@ class PasswordCheckMediator ...@@ -89,8 +91,27 @@ class PasswordCheckMediator
} }
} }
void onResumeFragment() {
// If the fragment is resumed, a CCT is closed.
mCctIsOpened = false;
}
void onUserLeavesCheckPage() {
// A user can leave the page because they opened a CCT in browser. As a user is fixing a
// compromised credential, don't count such a case as a user |DID_NOTHING| for the remaining
// credentials.
if (!mCctIsOpened) {
// A user closes the check page.
ListModel<ListItem> items = mModel.get(ITEMS);
for (int i = 1; i < items.size(); i++) {
PasswordCheckMetricsRecorder.recordCheckResolutionAction(
PasswordCheckResolutionAction.DID_NOTHING,
items.get(i).model.get(COMPROMISED_CREDENTIAL));
}
}
}
void destroy() { void destroy() {
// TODO(crbug.com/): Report PasswordCheckResolutionAction.DID_NOTHING.
getPasswordCheck().removeObserver(this); getPasswordCheck().removeObserver(this);
} }
...@@ -282,6 +303,7 @@ class PasswordCheckMediator ...@@ -282,6 +303,7 @@ class PasswordCheckMediator
: PasswordCheckUserAction.CHANGE_PASSWORD); : PasswordCheckUserAction.CHANGE_PASSWORD);
PasswordCheckMetricsRecorder.recordCheckResolutionAction( PasswordCheckMetricsRecorder.recordCheckResolutionAction(
PasswordCheckResolutionAction.OPENED_SITE, credential); PasswordCheckResolutionAction.OPENED_SITE, credential);
mCctIsOpened = true;
mChangePasswordDelegate.launchAppOrCctWithChangePasswordUrl(credential); mChangePasswordDelegate.launchAppOrCctWithChangePasswordUrl(credential);
} }
...@@ -292,6 +314,7 @@ class PasswordCheckMediator ...@@ -292,6 +314,7 @@ class PasswordCheckMediator
PasswordCheckUserAction.CHANGE_PASSWORD_AUTOMATICALLY); PasswordCheckUserAction.CHANGE_PASSWORD_AUTOMATICALLY);
PasswordCheckMetricsRecorder.recordCheckResolutionAction( PasswordCheckMetricsRecorder.recordCheckResolutionAction(
PasswordCheckResolutionAction.STARTED_SCRIPT, credential); PasswordCheckResolutionAction.STARTED_SCRIPT, credential);
mCctIsOpened = true;
mChangePasswordDelegate.launchCctWithScript(credential); mChangePasswordDelegate.launchCctWithScript(credential);
} }
......
...@@ -691,6 +691,92 @@ public class PasswordCheckControllerTest { ...@@ -691,6 +691,92 @@ public class PasswordCheckControllerTest {
is(1)); is(1));
} }
@Test
public void testRecordsDidNothingOnLeavingPage() {
when(mPasswordCheck.getCompromisedCredentials())
.thenReturn(new CompromisedCredential[] {ANA, BOB, CHARLIE});
when(mPasswordCheck.areScriptsRefreshed()).thenReturn(true);
when(mChangePasswordDelegate.canManuallyChangeCredential(any(CompromisedCredential.class)))
.thenReturn(true);
mMediator.onPasswordCheckStatusChanged(IDLE);
mMediator.onCompromisedCredentialsFetchCompleted();
mMediator.onUserLeavesCheckPage();
assertThat(RecordHistogram.getHistogramValueCountForTesting(
PASSWORD_CHECK_RESOLUTION_HISTOGRAM_WITHOUT_AUTO_BUTTON,
PasswordCheckResolutionAction.DID_NOTHING),
is(2));
assertThat(RecordHistogram.getHistogramValueCountForTesting(
PASSWORD_CHECK_RESOLUTION_HISTOGRAM_WITH_AUTO_BUTTON,
PasswordCheckResolutionAction.DID_NOTHING),
is(1));
assertThat(RecordHistogram.getHistogramValueCountForTesting(
PASSWORD_CHECK_RESOLUTION_HISTOGRAM_FOR_SCRIPTED_SITES,
PasswordCheckResolutionAction.DID_NOTHING),
is(2));
}
@Test
public void testDoesntRecordDidNothingOnLeavingPageIfCctIsOpen() {
when(mPasswordCheck.getCompromisedCredentials())
.thenReturn(new CompromisedCredential[] {ANA, BOB, CHARLIE});
when(mPasswordCheck.areScriptsRefreshed()).thenReturn(true);
when(mChangePasswordDelegate.canManuallyChangeCredential(any(CompromisedCredential.class)))
.thenReturn(true);
mMediator.onPasswordCheckStatusChanged(IDLE);
mMediator.onCompromisedCredentialsFetchCompleted();
// A user opens a CCT and then open the tab in browser => a user leaves the check page.
mMediator.onChangePasswordWithScriptButtonClick(BOB);
mMediator.onUserLeavesCheckPage();
assertThat(RecordHistogram.getHistogramValueCountForTesting(
PASSWORD_CHECK_RESOLUTION_HISTOGRAM_WITHOUT_AUTO_BUTTON,
PasswordCheckResolutionAction.DID_NOTHING),
is(0));
assertThat(RecordHistogram.getHistogramValueCountForTesting(
PASSWORD_CHECK_RESOLUTION_HISTOGRAM_WITH_AUTO_BUTTON,
PasswordCheckResolutionAction.DID_NOTHING),
is(0));
assertThat(RecordHistogram.getHistogramValueCountForTesting(
PASSWORD_CHECK_RESOLUTION_HISTOGRAM_FOR_SCRIPTED_SITES,
PasswordCheckResolutionAction.DID_NOTHING),
is(0));
}
@Test
public void testRecordDidNothingOnLeavingPageIfCctIsClosed() {
when(mPasswordCheck.getCompromisedCredentials())
.thenReturn(new CompromisedCredential[] {ANA, BOB, CHARLIE});
when(mPasswordCheck.areScriptsRefreshed()).thenReturn(true);
when(mChangePasswordDelegate.canManuallyChangeCredential(any(CompromisedCredential.class)))
.thenReturn(true);
mMediator.onPasswordCheckStatusChanged(IDLE);
mMediator.onCompromisedCredentialsFetchCompleted();
// A user opens a CCT, closes it, and leaves the password check page.
mMediator.onChangePasswordWithScriptButtonClick(BOB);
mMediator.onResumeFragment();
mMediator.onUserLeavesCheckPage();
assertThat(RecordHistogram.getHistogramValueCountForTesting(
PASSWORD_CHECK_RESOLUTION_HISTOGRAM_WITHOUT_AUTO_BUTTON,
PasswordCheckResolutionAction.DID_NOTHING),
is(2));
assertThat(RecordHistogram.getHistogramValueCountForTesting(
PASSWORD_CHECK_RESOLUTION_HISTOGRAM_WITH_AUTO_BUTTON,
PasswordCheckResolutionAction.DID_NOTHING),
is(1));
assertThat(RecordHistogram.getHistogramValueCountForTesting(
PASSWORD_CHECK_RESOLUTION_HISTOGRAM_FOR_SCRIPTED_SITES,
PasswordCheckResolutionAction.DID_NOTHING),
is(2));
}
private void assertIdleHeader(MVCListAdapter.ListItem header) { private void assertIdleHeader(MVCListAdapter.ListItem header) {
assertHeaderTypeWithStatus(header, IDLE); assertHeaderTypeWithStatus(header, IDLE);
assertNull(header.model.get(CHECK_PROGRESS)); assertNull(header.model.get(CHECK_PROGRESS));
......
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