Commit 4baf95f6 authored by Jan Wilken Dörrie's avatar Jan Wilken Dörrie Committed by Commit Bot

[Passwords] Add base::Feature for Additional Server Predictions

This change adds a new base::Feature to control processing of additional
server predictions during the form parsing stage. Populating these
additional fields is a recent server side change (http://cl/340884706)
only taking effect in Chrome M88+. This feature allows measuring the
impact of these new suggestions as part of a A/B experiment.

Bug: 1140493
Change-Id: I3741368e301c58c5adf7be21ac2947bf0d792e76
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2529552
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: default avatarVasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826247}
parent 05911d02
...@@ -4,9 +4,11 @@ ...@@ -4,9 +4,11 @@
#include "components/password_manager/core/browser/form_parsing/password_field_prediction.h" #include "components/password_manager/core/browser/form_parsing/password_field_prediction.h"
#include "base/feature_list.h"
#include "components/autofill/core/browser/form_structure.h" #include "components/autofill/core/browser/form_structure.h"
#include "components/autofill/core/common/form_data.h" #include "components/autofill/core/common/form_data.h"
#include "components/autofill/core/common/signatures.h" #include "components/autofill/core/common/signatures.h"
#include "components/password_manager/core/common/password_manager_features.h"
using autofill::AutofillField; using autofill::AutofillField;
using autofill::FieldSignature; using autofill::FieldSignature;
...@@ -18,22 +20,36 @@ namespace password_manager { ...@@ -18,22 +20,36 @@ namespace password_manager {
namespace { namespace {
bool AreSecondaryPredictionsEnabled() {
return base::FeatureList::IsEnabled(
features::kSecondaryServerFieldPredictions);
}
ServerFieldType GetServerType(const AutofillField& field) { ServerFieldType GetServerType(const AutofillField& field) {
// The main server predictions is in |field.server_type()| but the server can // The main server predictions is in `field.server_type()` but the server can
// send additional predictions in |field.server_predictions()|. This function // send additional predictions in `field.server_predictions()`. This function
// chooses relevant for Password Manager predictions. // chooses relevant for Password Manager predictions. Choosing additional
// predictions from `field.server_predictions()` is gated on the
// `kSecondaryServerFieldPredictions` flag. This is because the server only
// recently started to send down these predictions (http://cl/340884706).
// Having a feature flag here allows us to run experiments to measure the
// impact of these new predictions.
// 1. If there is cvc prediction returns it. // 1. If there is cvc prediction returns it.
for (const auto& predictions : field.server_predictions()) { for (const auto& predictions : field.server_predictions()) {
if (predictions.type() == autofill::CREDIT_CARD_VERIFICATION_CODE) if (predictions.type() == autofill::CREDIT_CARD_VERIFICATION_CODE &&
return ServerFieldType(predictions.type()); AreSecondaryPredictionsEnabled()) {
return autofill::CREDIT_CARD_VERIFICATION_CODE;
}
} }
// 2. If there is password related prediction returns it. // 2. If there is password related prediction returns it.
for (const auto& predictions : field.server_predictions()) { for (const auto& predictions : field.server_predictions()) {
ServerFieldType type = ServerFieldType(predictions.type()); auto type = static_cast<ServerFieldType>(predictions.type());
if (DeriveFromServerFieldType(type) != CredentialFieldType::kNone) if (DeriveFromServerFieldType(type) != CredentialFieldType::kNone &&
AreSecondaryPredictionsEnabled()) {
return type; return type;
}
} }
// 3. Returns the main prediction. // 3. Returns the main prediction.
......
...@@ -8,9 +8,11 @@ ...@@ -8,9 +8,11 @@
#include "base/stl_util.h" #include "base/stl_util.h"
#include "base/strings/utf_string_conversions.h" #include "base/strings/utf_string_conversions.h"
#include "base/test/scoped_feature_list.h"
#include "components/autofill/core/browser/form_structure.h" #include "components/autofill/core/browser/form_structure.h"
#include "components/autofill/core/common/form_data.h" #include "components/autofill/core/common/form_data.h"
#include "components/autofill/core/common/renderer_id.h" #include "components/autofill/core/common/renderer_id.h"
#include "components/password_manager/core/common/password_manager_features.h"
#include "testing/gmock/include/gmock/gmock.h" #include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h" #include "testing/gtest/include/gtest/gtest.h"
...@@ -39,7 +41,23 @@ namespace password_manager { ...@@ -39,7 +41,23 @@ namespace password_manager {
namespace { namespace {
TEST(FormPredictionsTest, ConvertToFormPredictions) { // The boolean parameter determines the feature state of
// `kSecondaryServerFieldPredictions`.
class FormPredictionsTest : public ::testing::TestWithParam<bool> {
public:
FormPredictionsTest() {
feature_list_.InitWithFeatureState(
features::kSecondaryServerFieldPredictions,
AreSecondaryPredictionsEnabled());
}
bool AreSecondaryPredictionsEnabled() const { return GetParam(); }
private:
base::test::ScopedFeatureList feature_list_;
};
TEST_P(FormPredictionsTest, ConvertToFormPredictions) {
struct TestField { struct TestField {
std::string name; std::string name;
std::string form_control_type; std::string form_control_type;
...@@ -55,13 +73,20 @@ TEST(FormPredictionsTest, ConvertToFormPredictions) { ...@@ -55,13 +73,20 @@ TEST(FormPredictionsTest, ConvertToFormPredictions) {
{"Password", "password", PASSWORD, PASSWORD, false}, {"Password", "password", PASSWORD, PASSWORD, false},
{"confirm_password", "password", CONFIRMATION_PASSWORD, {"confirm_password", "password", CONFIRMATION_PASSWORD,
CONFIRMATION_PASSWORD, true}, CONFIRMATION_PASSWORD, true},
// username in |additional_types| takes precedence. // username in |additional_types| takes precedence if the feature is
{"email", "text", EMAIL_ADDRESS, USERNAME, false, {USERNAME}}, // enabled.
// cvc in |additional_types| takes precedence. {"email",
"text",
EMAIL_ADDRESS,
AreSecondaryPredictionsEnabled() ? USERNAME : EMAIL_ADDRESS,
false,
{USERNAME}},
// cvc in |additional_types| takes precedence if the feature is enabled.
{"cvc", {"cvc",
"password", "password",
PASSWORD, PASSWORD,
CREDIT_CARD_VERIFICATION_CODE, AreSecondaryPredictionsEnabled() ? CREDIT_CARD_VERIFICATION_CODE
: PASSWORD,
false, false,
{CREDIT_CARD_VERIFICATION_CODE}}, {CREDIT_CARD_VERIFICATION_CODE}},
// non-password, non-cvc types in |additional_types| are ignored. // non-password, non-cvc types in |additional_types| are ignored.
...@@ -206,7 +231,7 @@ TEST(FormPredictionsTest, DeriveFromServerFieldType) { ...@@ -206,7 +231,7 @@ TEST(FormPredictionsTest, DeriveFromServerFieldType) {
DeriveFromServerFieldType(test_case.server_type)); DeriveFromServerFieldType(test_case.server_type));
} }
} }
INSTANTIATE_TEST_SUITE_P(All, FormPredictionsTest, testing::Bool());
} // namespace } // namespace
} // namespace password_manager } // namespace password_manager
...@@ -84,6 +84,10 @@ const base::Feature kPasswordsWeaknessCheck = { ...@@ -84,6 +84,10 @@ const base::Feature kPasswordsWeaknessCheck = {
const base::Feature kRecoverFromNeverSaveAndroid = { const base::Feature kRecoverFromNeverSaveAndroid = {
"RecoverFromNeverSaveAndroid", base::FEATURE_DISABLED_BY_DEFAULT}; "RecoverFromNeverSaveAndroid", base::FEATURE_DISABLED_BY_DEFAULT};
// Enables considering secondary server field predictions during form parsing.
const base::Feature kSecondaryServerFieldPredictions = {
"SecondaryServerFieldPredictions", base::FEATURE_DISABLED_BY_DEFAULT};
// Treat heuritistics to find new password fields as reliable. This enables // Treat heuritistics to find new password fields as reliable. This enables
// password generation on more forms, but could lead to false positives. // password generation on more forms, but could lead to false positives.
const base::Feature kTreatNewPasswordHeuristicsAsReliable = { const base::Feature kTreatNewPasswordHeuristicsAsReliable = {
......
...@@ -33,6 +33,7 @@ extern const base::Feature kPasswordImport; ...@@ -33,6 +33,7 @@ extern const base::Feature kPasswordImport;
extern const base::Feature kPasswordScriptsFetching; extern const base::Feature kPasswordScriptsFetching;
extern const base::Feature kPasswordsWeaknessCheck; extern const base::Feature kPasswordsWeaknessCheck;
extern const base::Feature kRecoverFromNeverSaveAndroid; extern const base::Feature kRecoverFromNeverSaveAndroid;
extern const base::Feature kSecondaryServerFieldPredictions;
extern const base::Feature kTreatNewPasswordHeuristicsAsReliable; extern const base::Feature kTreatNewPasswordHeuristicsAsReliable;
extern const base::Feature kUsernameFirstFlow; extern const base::Feature kUsernameFirstFlow;
extern const base::Feature kWellKnownChangePassword; extern const base::Feature kWellKnownChangePassword;
......
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