Skip to content

Commit

Permalink
[M116][UPMLocalPwd] Start the sync consent flow from the migration wa…
Browse files Browse the repository at this point in the history
…rning

The flow is started when the user chooses the option to sync passwords
in the password migration warning. This applies to users who are not
signed in, or are signed in, but still need to provide sync consent.
We determine whether the sync consent flow should be launched, by
first checking whether `isSyncFeatureEnabled` returns false.

(cherry picked from commit 967b0e4)

Bug: 1454770, 1456777
Change-Id: I2777697145b636260d601e8d6b94bd03d1a6052d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4620195
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Reviewed-by: Nohemi Fernandez <fernandex@chromium.org>
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Friedrich Horschig <fhorschig@chromium.org>
Commit-Queue: Ioana Pandele <ioanap@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1160647}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4637261
Cr-Commit-Position: refs/branch-heads/5845@{#34}
Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321}
  • Loading branch information
Ioana Pandele authored and Chromium LUCI CQ committed Jun 23, 2023
1 parent 1c22b6b commit 8b9cece
Show file tree
Hide file tree
Showing 10 changed files with 61 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,13 @@

package org.chromium.chrome.browser.password_manager;

import android.content.Context;

import org.chromium.base.annotations.CalledByNative;
import org.chromium.chrome.browser.profiles.Profile;
import org.chromium.chrome.browser.pwd_migration.PasswordMigrationWarningCoordinator;
import org.chromium.chrome.browser.settings.SettingsLauncherImpl;
import org.chromium.chrome.browser.signin.SyncConsentActivityLauncherImpl;
import org.chromium.chrome.browser.sync.settings.ManageSyncSettings;
import org.chromium.components.browser_ui.bottomsheet.BottomSheetController;
import org.chromium.components.browser_ui.bottomsheet.BottomSheetControllerProvider;
Expand All @@ -20,9 +23,11 @@ static void showWarning(WindowAndroid windowAndroid, Profile profile) {
BottomSheetController bottomSheetController =
BottomSheetControllerProvider.from(windowAndroid);
if (bottomSheetController == null) return;
Context context = windowAndroid.getContext().get();
if (context == null) return;
PasswordMigrationWarningCoordinator passwordMigrationWarningCoordinator =
new PasswordMigrationWarningCoordinator(windowAndroid.getContext().get(), profile,
bottomSheetController, new SettingsLauncherImpl(),
new PasswordMigrationWarningCoordinator(context, profile, bottomSheetController,
SyncConsentActivityLauncherImpl.get(), new SettingsLauncherImpl(),
ManageSyncSettings.class);
passwordMigrationWarningCoordinator.showWarning();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import org.chromium.chrome.browser.settings.ChromeManagedPreferenceDelegate;
import org.chromium.chrome.browser.settings.ProfileDependentSetting;
import org.chromium.chrome.browser.settings.SettingsLauncherImpl;
import org.chromium.chrome.browser.signin.SyncConsentActivityLauncherImpl;
import org.chromium.chrome.browser.sync.SyncServiceFactory;
import org.chromium.chrome.browser.sync.settings.ManageSyncSettings;
import org.chromium.chrome.browser.sync.settings.SyncSettingsUtils;
Expand Down Expand Up @@ -408,8 +409,8 @@ public void passwordListAvailable(int count) {
ChromeFeatureList.UNIFIED_PASSWORD_MANAGER_LOCAL_PWD_MIGRATION_WARNING)) {
PasswordMigrationWarningCoordinator passwordMigrationWarningCoordinator =
new PasswordMigrationWarningCoordinator(getContext(), mProfile,
mBottomSheetController, new SettingsLauncherImpl(),
ManageSyncSettings.class);
mBottomSheetController, SyncConsentActivityLauncherImpl.get(),
new SettingsLauncherImpl(), ManageSyncSettings.class);
passwordMigrationWarningCoordinator.showWarning();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ android_library("java") {
"//chrome/browser/profiles/android:java",
"//chrome/browser/signin/services/android:java",
"//chrome/browser/sync/android:java",
"//chrome/browser/ui/android/signin:java",
"//chrome/browser/ui/android/strings:ui_strings_grd",
"//components/browser_ui/bottomsheet/android:java",
"//components/browser_ui/settings/android:java",
Expand Down Expand Up @@ -59,6 +60,7 @@ robolectric_library("junit") {
"//chrome/browser/profiles/android:java",
"//chrome/browser/signin/services/android:java",
"//chrome/browser/sync/android:java",
"//chrome/browser/ui/android/signin:java",
"//chrome/test/android:chrome_java_test_support_common",
"//components/browser_ui/bottomsheet/android:java",
"//components/browser_ui/bottomsheet/android/test:java",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,22 +11,27 @@
import org.chromium.chrome.browser.profiles.Profile;
import org.chromium.chrome.browser.pwd_migration.PasswordMigrationWarningMediator.MigrationWarningOptionsHandler;
import org.chromium.chrome.browser.pwd_migration.PasswordMigrationWarningProperties.ScreenType;
import org.chromium.chrome.browser.ui.signin.SyncConsentActivityLauncher;
import org.chromium.components.browser_ui.bottomsheet.BottomSheetController;
import org.chromium.components.browser_ui.settings.SettingsLauncher;
import org.chromium.components.signin.metrics.SigninAccessPoint;
import org.chromium.ui.modelutil.PropertyModel;
import org.chromium.ui.modelutil.PropertyModelChangeProcessor;

/** The coordinator of the password migration warning. */
public class PasswordMigrationWarningCoordinator implements MigrationWarningOptionsHandler {
private final PasswordMigrationWarningMediator mMediator;
private final SyncConsentActivityLauncher mSyncConsentActivityLauncher;
private final SettingsLauncher mSettingsLauncher;
private final Context mContext;
private final Class<? extends Fragment> mSyncSettingsFragment;

public PasswordMigrationWarningCoordinator(Context context, Profile profile,
BottomSheetController sheetController, SettingsLauncher settingsLauncher,
Class<? extends Fragment> syncSettingsFragment) {
BottomSheetController sheetController,
SyncConsentActivityLauncher syncConsentActivityLauncher,
SettingsLauncher settingsLauncher, Class<? extends Fragment> syncSettingsFragment) {
mContext = context;
mSyncConsentActivityLauncher = syncConsentActivityLauncher;
mSettingsLauncher = settingsLauncher;
mSyncSettingsFragment = syncSettingsFragment;
mMediator = new PasswordMigrationWarningMediator(profile, this);
Expand All @@ -48,7 +53,8 @@ static void setUpModelChangeProcessors(PropertyModel model, PasswordMigrationWar

@Override
public void startSyncConsentFlow() {
// TODO(crbug.com/1454770): Launch the sync consent flow.
mSyncConsentActivityLauncher.launchActivityIfAllowed(
mContext, SigninAccessPoint.PASSWORD_MIGRATION_WARNING_ANDROID);
}

@Override
Expand Down
7 changes: 7 additions & 0 deletions components/signin/public/base/signin_metrics.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "base/metrics/histogram_functions.h"
#include "base/metrics/histogram_macros.h"
#include "base/metrics/user_metrics.h"
#include "base/metrics/user_metrics_action.h"
#include "base/numerics/safe_conversions.h"
#include "base/time/time.h"
#include "build/build_config.h"
Expand Down Expand Up @@ -474,6 +475,10 @@ void RecordSigninUserActionForAccessPoint(AccessPoint access_point) {
base::RecordAction(
base::UserMetricsAction("Signin_Signin_FromSetUpList"));
break;
case AccessPoint::ACCESS_POINT_PASSWORD_MIGRATION_WARNING_ANDROID:
base::RecordAction(base::UserMetricsAction(
"Signin_Signin_FromPasswordMigrationWarningAndroid"));
break;
case AccessPoint::ACCESS_POINT_MAX:
NOTREACHED();
break;
Expand Down Expand Up @@ -614,6 +619,7 @@ void RecordSigninImpressionUserActionForAccessPoint(AccessPoint access_point) {
case AccessPoint::ACCESS_POINT_FOR_YOU_FRE:
case signin_metrics::AccessPoint::ACCESS_POINT_REAUTH_INFO_BAR:
case signin_metrics::AccessPoint::ACCESS_POINT_ACCOUNT_CONSISTENCY_SERVICE:
case AccessPoint::ACCESS_POINT_PASSWORD_MIGRATION_WARNING_ANDROID:
case AccessPoint::ACCESS_POINT_MAX:
NOTREACHED() << "Signin_Impression_From* user actions"
<< " are not recorded for access point "
Expand Down Expand Up @@ -677,6 +683,7 @@ void RecordConsistencyPromoUserAction(AccountConsistencyPromoAction action,
case AccessPoint::ACCESS_POINT_ACCOUNT_CONSISTENCY_SERVICE:
case AccessPoint::ACCESS_POINT_SEARCH_COMPANION:
case AccessPoint::ACCESS_POINT_SET_UP_LIST:
case AccessPoint::ACCESS_POINT_PASSWORD_MIGRATION_WARNING_ANDROID:
case AccessPoint::ACCESS_POINT_MAX:
NOTREACHED() << "Signin.AccountConsistencyPromoAction histogram is not"
<< "recorded for access point "
Expand Down
2 changes: 2 additions & 0 deletions components/signin/public/base/signin_metrics.h
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,8 @@ enum class AccessPoint : int {
ACCESS_POINT_SEARCH_COMPANION = 50,
// Access point for the IOS Set Up List on the NTP.
ACCESS_POINT_SET_UP_LIST = 51,
// Access point for the local password migration warning on Android.
ACCESS_POINT_PASSWORD_MIGRATION_WARNING_ANDROID = 52,

// Add values above this line with a corresponding label to the
// "SigninAccessPoint" enum in tools/metrics/histograms/enums.xml
Expand Down
2 changes: 2 additions & 0 deletions components/signin/public/base/signin_metrics_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,8 @@ class SigninMetricsTest : public ::testing::Test {
return "SearchCompanion";
case AccessPoint::ACCESS_POINT_SET_UP_LIST:
return "SetUpList";
case AccessPoint::ACCESS_POINT_PASSWORD_MIGRATION_WARNING_ANDROID:
return "PasswordMigrationWarning";
case AccessPoint::ACCESS_POINT_MAX:
NOTREACHED();
return "";
Expand Down
12 changes: 12 additions & 0 deletions ios/chrome/browser/ui/authentication/signin_promo_view_mediator.mm
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,8 @@ bool IsSupportedAccessPoint(signin_metrics::AccessPoint access_point) {
case signin_metrics::AccessPoint::ACCESS_POINT_ACCOUNT_CONSISTENCY_SERVICE:
case signin_metrics::AccessPoint::ACCESS_POINT_SEARCH_COMPANION:
case signin_metrics::AccessPoint::ACCESS_POINT_SET_UP_LIST:
case signin_metrics::AccessPoint::
ACCESS_POINT_PASSWORD_MIGRATION_WARNING_ANDROID:
case signin_metrics::AccessPoint::ACCESS_POINT_MAX:
return false;
}
Expand Down Expand Up @@ -187,6 +189,8 @@ void RecordImpressionsTilSigninButtonsHistogramForAccessPoint(
case signin_metrics::AccessPoint::ACCESS_POINT_ACCOUNT_CONSISTENCY_SERVICE:
case signin_metrics::AccessPoint::ACCESS_POINT_SEARCH_COMPANION:
case signin_metrics::AccessPoint::ACCESS_POINT_SET_UP_LIST:
case signin_metrics::AccessPoint::
ACCESS_POINT_PASSWORD_MIGRATION_WARNING_ANDROID:
NOTREACHED() << "Unexpected value for access point "
<< static_cast<int>(access_point);
break;
Expand Down Expand Up @@ -267,6 +271,8 @@ void RecordImpressionsTilDismissHistogramForAccessPoint(
case signin_metrics::AccessPoint::ACCESS_POINT_ACCOUNT_CONSISTENCY_SERVICE:
case signin_metrics::AccessPoint::ACCESS_POINT_SEARCH_COMPANION:
case signin_metrics::AccessPoint::ACCESS_POINT_SET_UP_LIST:
case signin_metrics::AccessPoint::
ACCESS_POINT_PASSWORD_MIGRATION_WARNING_ANDROID:
NOTREACHED() << "Unexpected value for access point "
<< static_cast<int>(access_point);
break;
Expand Down Expand Up @@ -346,6 +352,8 @@ void RecordImpressionsTilXButtonHistogramForAccessPoint(
case signin_metrics::AccessPoint::ACCESS_POINT_ACCOUNT_CONSISTENCY_SERVICE:
case signin_metrics::AccessPoint::ACCESS_POINT_SEARCH_COMPANION:
case signin_metrics::AccessPoint::ACCESS_POINT_SET_UP_LIST:
case signin_metrics::AccessPoint::
ACCESS_POINT_PASSWORD_MIGRATION_WARNING_ANDROID:
case signin_metrics::AccessPoint::ACCESS_POINT_MAX:
NOTREACHED() << "Unexpected value for access point "
<< static_cast<int>(access_point);
Expand Down Expand Up @@ -413,6 +421,8 @@ void RecordImpressionsTilXButtonHistogramForAccessPoint(
case signin_metrics::AccessPoint::ACCESS_POINT_ACCOUNT_CONSISTENCY_SERVICE:
case signin_metrics::AccessPoint::ACCESS_POINT_SEARCH_COMPANION:
case signin_metrics::AccessPoint::ACCESS_POINT_SET_UP_LIST:
case signin_metrics::AccessPoint::
ACCESS_POINT_PASSWORD_MIGRATION_WARNING_ANDROID:
return nullptr;
}
}
Expand Down Expand Up @@ -477,6 +487,8 @@ void RecordImpressionsTilXButtonHistogramForAccessPoint(
case signin_metrics::AccessPoint::ACCESS_POINT_ACCOUNT_CONSISTENCY_SERVICE:
case signin_metrics::AccessPoint::ACCESS_POINT_SEARCH_COMPANION:
case signin_metrics::AccessPoint::ACCESS_POINT_SET_UP_LIST:
case signin_metrics::AccessPoint::
ACCESS_POINT_PASSWORD_MIGRATION_WARNING_ANDROID:
return nullptr;
}
}
Expand Down
16 changes: 16 additions & 0 deletions tools/metrics/actions/actions.xml
Original file line number Diff line number Diff line change
Expand Up @@ -32387,6 +32387,22 @@ should be able to be added at any place in this file.
</description>
</action>

<action name="Signin_Signin_FromPasswordMigrationWarning">
<owner>ioanap@chromium.org</owner>
<owner>izuzic@google.com</owner>
<owner>chrome-signin-team@chromium.org</owner>
<description>
Recorded on sign in/sync consent flow start from access point
signin_metrics::AccessPoint::ACCESS_POINT_PASSWORD_MIGRATION_WARNING.
Android only.
</description>
</action>

<action name="Signin_Signin_FromPasswordMigrationWarningAndroid">
<owner>Please list the metric's owners. Add more owner tags as needed.</owner>
<description>Please enter the description of the metric.</description>
</action>

<action name="Signin_Signin_FromPostDeviceRestoreSigninPromo">
<owner>scottyoder@google.com</owner>
<owner>bling-get-set-up@google.com</owner>
Expand Down
1 change: 1 addition & 0 deletions tools/metrics/histograms/enums.xml
Original file line number Diff line number Diff line change
Expand Up @@ -97437,6 +97437,7 @@ https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_2.7.1.pdf
<int value="49" label="Account consistency service"/>
<int value="50" label="Search companion"/>
<int value="51" label="Set Up List on iOS only"/>
<int value="52" label="Password migration warning on Android only"/>
</enum>

<enum name="SigninAccountReconcilorState">
Expand Down

0 comments on commit 8b9cece

Please sign in to comment.