Commit d04e4609 authored by Maxim Kolosovskiy's avatar Maxim Kolosovskiy Committed by Chromium LUCI CQ

[Passwords] Enable button titles scraping based on channel, not a flag

Button titles scraping for unowned forms might be heavy. So, it should
be enabled only in Dev and Canary.

Before this CL, channel-based filtering was implemented with the
AutofillFieldMetadata feature. As the experiment spans for many years,
it is better to hardcode the behavior on the client side. Different
behaviour in different channels is to be deprecated once we deprecate
the old metadata and use the rich metadata pipeline.

Bug: 1111809
Change-Id: I0cd31b4961338a2b07b10c37164b6bc75f8d8808
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2526083Reviewed-by: default avatarChristos Froussios <cfroussios@chromium.org>
Reviewed-by: default avatarRobert Sesek <rsesek@chromium.org>
Reviewed-by: default avatarJochen Eisinger <jochen@chromium.org>
Auto-Submit: Maxim Kolosovskiy  <kolos@chromium.org>
Commit-Queue: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#832310}
parent 95aeb1c5
...@@ -34,6 +34,7 @@ static_library("browser") { ...@@ -34,6 +34,7 @@ static_library("browser") {
"//components/resources", "//components/resources",
"//components/strings", "//components/strings",
"//components/user_prefs", "//components/user_prefs",
"//components/version_info",
"//content/public/browser", "//content/public/browser",
"//content/public/common", "//content/public/common",
"//gpu/config", "//gpu/config",
...@@ -77,6 +78,7 @@ source_set("unit_tests") { ...@@ -77,6 +78,7 @@ source_set("unit_tests") {
"//components/autofill/core/browser", "//components/autofill/core/browser",
"//components/autofill/core/browser:test_support", "//components/autofill/core/browser:test_support",
"//components/autofill/core/common", "//components/autofill/core/common",
"//components/version_info",
"//content/public/browser", "//content/public/browser",
"//content/public/common", "//content/public/common",
"//content/test:test_support", "//content/test:test_support",
......
include_rules = [ include_rules = [
"+content/public/browser", "+content/public/browser",
"+components/keyed_service/content", "+components/keyed_service/content",
"+components/version_info",
"+crypto/random.h", "+crypto/random.h",
"+gpu/config/gpu_info.h", "+gpu/config/gpu_info.h",
"+services/device/public", "+services/device/public",
......
...@@ -18,6 +18,7 @@ ...@@ -18,6 +18,7 @@
#include "components/autofill/core/browser/form_structure.h" #include "components/autofill/core/browser/form_structure.h"
#include "components/autofill/core/browser/payments/payments_service_url.h" #include "components/autofill/core/browser/payments/payments_service_url.h"
#include "components/autofill/core/common/autofill_features.h" #include "components/autofill/core/common/autofill_features.h"
#include "components/version_info/channel.h"
#include "content/public/browser/back_forward_cache.h" #include "content/public/browser/back_forward_cache.h"
#include "content/public/browser/browser_context.h" #include "content/public/browser/browser_context.h"
#include "content/public/browser/navigation_controller.h" #include "content/public/browser/navigation_controller.h"
...@@ -36,6 +37,24 @@ ...@@ -36,6 +37,24 @@
#include "ui/gfx/geometry/size_f.h" #include "ui/gfx/geometry/size_f.h"
#include "url/origin.h" #include "url/origin.h"
namespace {
bool ShouldEnableHeavyFormDataScraping(const version_info::Channel channel) {
switch (channel) {
case version_info::Channel::CANARY:
case version_info::Channel::DEV:
return true;
case version_info::Channel::STABLE:
case version_info::Channel::BETA:
case version_info::Channel::UNKNOWN:
return false;
}
NOTREACHED();
return false;
}
} // namespace
namespace autofill { namespace autofill {
ContentAutofillDriver::ContentAutofillDriver( ContentAutofillDriver::ContentAutofillDriver(
...@@ -56,6 +75,9 @@ ContentAutofillDriver::ContentAutofillDriver( ...@@ -56,6 +75,9 @@ ContentAutofillDriver::ContentAutofillDriver(
SetAutofillManager(std::make_unique<AutofillManager>( SetAutofillManager(std::make_unique<AutofillManager>(
this, client, app_locale, enable_download_manager)); this, client, app_locale, enable_download_manager));
} }
if (client && ShouldEnableHeavyFormDataScraping(client->GetChannel())) {
GetAutofillAgent()->EnableHeavyFormDataScraping();
}
} }
ContentAutofillDriver::~ContentAutofillDriver() = default; ContentAutofillDriver::~ContentAutofillDriver() = default;
......
...@@ -21,6 +21,7 @@ ...@@ -21,6 +21,7 @@
#include "components/autofill/core/browser/test_autofill_client.h" #include "components/autofill/core/browser/test_autofill_client.h"
#include "components/autofill/core/common/autofill_switches.h" #include "components/autofill/core/common/autofill_switches.h"
#include "components/autofill/core/common/form_data_predictions.h" #include "components/autofill/core/common/form_data_predictions.h"
#include "components/version_info/version_info.h"
#include "content/public/browser/browser_context.h" #include "content/public/browser/browser_context.h"
#include "content/public/browser/ssl_status.h" #include "content/public/browser/ssl_status.h"
#include "content/public/browser/storage_partition.h" #include "content/public/browser/storage_partition.h"
...@@ -141,6 +142,7 @@ class FakeAutofillAgent : public mojom::AutofillAgent { ...@@ -141,6 +142,7 @@ class FakeAutofillAgent : public mojom::AutofillAgent {
// Mocked mojom::AutofillAgent methods: // Mocked mojom::AutofillAgent methods:
MOCK_METHOD0(FirstUserGestureObservedInTab, void()); MOCK_METHOD0(FirstUserGestureObservedInTab, void());
MOCK_METHOD0(EnableHeavyFormDataScraping, void());
private: private:
void CallDone() { void CallDone() {
...@@ -456,4 +458,30 @@ TEST_F(ContentAutofillDriverTest, PreviewFieldWithValue) { ...@@ -456,4 +458,30 @@ TEST_F(ContentAutofillDriverTest, PreviewFieldWithValue) {
EXPECT_EQ(input_value, output_value); EXPECT_EQ(input_value, output_value);
} }
TEST_F(ContentAutofillDriverTest, EnableHeavyFormDataScraping) {
struct TestCase {
version_info::Channel channel;
bool heavy_scraping_enabled;
} kTestCases[] = {{version_info::Channel::CANARY, true},
{version_info::Channel::DEV, true},
{version_info::Channel::UNKNOWN, false},
{version_info::Channel::BETA, false},
{version_info::Channel::STABLE, false}};
for (auto test_case : kTestCases) {
SCOPED_TRACE(testing::Message()
<< "channel: "
<< version_info::GetChannelString(test_case.channel));
test_autofill_client_->set_channel_for_testing(test_case.channel);
EXPECT_CALL(fake_agent_, EnableHeavyFormDataScraping())
.Times(test_case.heavy_scraping_enabled ? 1 : 0);
std::unique_ptr<ContentAutofillDriver> driver(new TestContentAutofillDriver(
web_contents()->GetMainFrame(), test_autofill_client_.get()));
base::RunLoop().RunUntilIdle();
testing::Mock::VerifyAndClearExpectations(&fake_agent_);
}
}
} // namespace autofill } // namespace autofill
...@@ -78,6 +78,10 @@ interface AutofillAgent { ...@@ -78,6 +78,10 @@ interface AutofillAgent {
// Set whether or not an assistant action is currently running an action. // Set whether or not an assistant action is currently running an action.
SetAssistantActionState(bool running); SetAssistantActionState(bool running);
// Allows heavy scraping of form data (e.g., button titles for
// unowned forms).
EnableHeavyFormDataScraping();
}; };
// There is one instance of this interface per render frame in the render // There is one instance of this interface per render frame in the render
......
...@@ -758,6 +758,10 @@ void AutofillAgent::SetAssistantActionState(bool running) { ...@@ -758,6 +758,10 @@ void AutofillAgent::SetAssistantActionState(bool running) {
} }
} }
void AutofillAgent::EnableHeavyFormDataScraping() {
is_heavy_form_data_scraping_enabled_ = true;
}
void AutofillAgent::QueryAutofillSuggestions( void AutofillAgent::QueryAutofillSuggestions(
const WebFormControlElement& element, const WebFormControlElement& element,
bool autoselect_first_suggestion) { bool autoselect_first_suggestion) {
......
...@@ -100,6 +100,7 @@ class AutofillAgent : public content::RenderFrameObserver, ...@@ -100,6 +100,7 @@ class AutofillAgent : public content::RenderFrameObserver,
const std::vector<std::string>& selectors, const std::vector<std::string>& selectors,
GetElementFormAndFieldDataCallback callback) override; GetElementFormAndFieldDataCallback callback) override;
void SetAssistantActionState(bool running) override; void SetAssistantActionState(bool running) override;
void EnableHeavyFormDataScraping() override;
void FormControlElementClicked(const blink::WebFormControlElement& element, void FormControlElementClicked(const blink::WebFormControlElement& element,
bool was_focused); bool was_focused);
...@@ -124,6 +125,10 @@ class AutofillAgent : public content::RenderFrameObserver, ...@@ -124,6 +125,10 @@ class AutofillAgent : public content::RenderFrameObserver,
FormTracker* form_tracker_for_testing() { return &form_tracker_; } FormTracker* form_tracker_for_testing() { return &form_tracker_; }
bool is_heavy_form_data_scraping_enabled() {
return is_heavy_form_data_scraping_enabled_;
}
void SelectWasUpdated(const blink::WebFormControlElement& element); void SelectWasUpdated(const blink::WebFormControlElement& element);
protected: protected:
...@@ -367,6 +372,10 @@ class AutofillAgent : public content::RenderFrameObserver, ...@@ -367,6 +372,10 @@ class AutofillAgent : public content::RenderFrameObserver,
// is. // is.
bool is_screen_reader_enabled_ = false; bool is_screen_reader_enabled_ = false;
// Whether agents should enable heavy scraping of form data (e.g., button
// titles for unowned forms).
bool is_heavy_form_data_scraping_enabled_ = false;
const scoped_refptr<FieldDataManager> field_data_manager_; const scoped_refptr<FieldDataManager> field_data_manager_;
base::WeakPtrFactory<AutofillAgent> weak_ptr_factory_{this}; base::WeakPtrFactory<AutofillAgent> weak_ptr_factory_{this};
......
...@@ -99,15 +99,6 @@ enum FieldFilterMask { ...@@ -99,15 +99,6 @@ enum FieldFilterMask {
FILTER_NON_FOCUSABLE_ELEMENTS, FILTER_NON_FOCUSABLE_ELEMENTS,
}; };
// Returns whether sending autofill field metadata to the server is enabled.
// TODO(crbug.com/938804): Remove this when button titles are crowdsourced in
// all channels.
bool IsAutofillFieldMetadataEnabled() {
static base::NoDestructor<std::string> kGroupName(
base::FieldTrialList::FindFullName("AutofillFieldMetadata"));
return base::StartsWith(*kGroupName, "Enabled", base::CompareCase::SENSITIVE);
}
void TruncateString(base::string16* str, size_t max_length) { void TruncateString(base::string16* str, size_t max_length) {
if (str->length() > max_length) if (str->length() > max_length)
str->resize(max_length); str->resize(max_length);
...@@ -2211,9 +2202,10 @@ base::string16 FindChildText(const WebNode& node) { ...@@ -2211,9 +2202,10 @@ base::string16 FindChildText(const WebNode& node) {
ButtonTitleList GetButtonTitles(const WebFormElement& web_form, ButtonTitleList GetButtonTitles(const WebFormElement& web_form,
const WebDocument& document, const WebDocument& document,
ButtonTitlesCache* button_titles_cache) { ButtonTitlesCache* button_titles_cache) {
DCHECK(button_titles_cache); if (!button_titles_cache) {
if (!IsAutofillFieldMetadataEnabled() && web_form.IsNull()) // Button titles scraping is disabled for this form.
return ButtonTitleList(); return ButtonTitleList();
}
// True if the cache has no entry for |web_form|. // True if the cache has no entry for |web_form|.
bool cache_miss = true; bool cache_miss = true;
......
...@@ -360,12 +360,6 @@ TEST_F(FormAutofillUtilsTest, GetButtonTitles_TooLongTitle) { ...@@ -360,12 +360,6 @@ TEST_F(FormAutofillUtilsTest, GetButtonTitles_TooLongTitle) {
} }
TEST_F(FormAutofillUtilsTest, GetButtonTitles_Formless) { TEST_F(FormAutofillUtilsTest, GetButtonTitles_Formless) {
// Button titles computation and crowdsourcing for <form>less forms are
// enabled only if |AutofillFieldMetadata| (Dev and Canary) is enabled.
base::test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.Init();
base::FieldTrialList::CreateFieldTrial("AutofillFieldMetadata", "Enabled");
constexpr char kNoFormHtml[] = constexpr char kNoFormHtml[] =
"<div class='reg-form'>" "<div class='reg-form'>"
" <input type='button' value='\n Show\t password '>" " <input type='button' value='\n Show\t password '>"
...@@ -400,13 +394,10 @@ TEST_F(FormAutofillUtilsTest, GetButtonTitles_Formless) { ...@@ -400,13 +394,10 @@ TEST_F(FormAutofillUtilsTest, GetButtonTitles_Formless) {
VerifyButtonTitleCache(form_target, expected, cache); VerifyButtonTitleCache(form_target, expected, cache);
} }
TEST_F(FormAutofillUtilsTest, GetButtonTitles_Formless_DisabledByDefault) { TEST_F(FormAutofillUtilsTest, GetButtonTitles_DisabledIfNoCache) {
// Button titles computation and crowdsourcing for <form>less forms should be // Button titles scraping for unowned forms can be time-consuming and disabled
// disabled if |AutofillFieldMetadata| is disabled. // in Beta and Stable. To disable button titles computation, |buttons_cache|
base::test::ScopedFeatureList scoped_feature_list; // should be null.
scoped_feature_list.Init();
base::FieldTrialList::CreateFieldTrial("AutofillFieldMetadata", "Disabled");
constexpr char kNoFormHtml[] = constexpr char kNoFormHtml[] =
"<div class='reg-form'>" "<div class='reg-form'>"
" <input type='button' value='\n Show\t password '>" " <input type='button' value='\n Show\t password '>"
...@@ -425,13 +416,11 @@ TEST_F(FormAutofillUtilsTest, GetButtonTitles_Formless_DisabledByDefault) { ...@@ -425,13 +416,11 @@ TEST_F(FormAutofillUtilsTest, GetButtonTitles_Formless_DisabledByDefault) {
ASSERT_NE(nullptr, web_frame); ASSERT_NE(nullptr, web_frame);
WebFormElement form_target; WebFormElement form_target;
ASSERT_FALSE(web_frame->GetDocument().Body().IsNull()); ASSERT_FALSE(web_frame->GetDocument().Body().IsNull());
ButtonTitlesCache cache;
autofill::ButtonTitleList actual = autofill::ButtonTitleList actual =
GetButtonTitles(form_target, web_frame->GetDocument(), &cache); GetButtonTitles(form_target, web_frame->GetDocument(), nullptr);
EXPECT_TRUE(actual.empty()); EXPECT_TRUE(actual.empty());
EXPECT_TRUE(cache.empty());
} }
TEST_F(FormAutofillUtilsTest, IsEnabled) { TEST_F(FormAutofillUtilsTest, IsEnabled) {
......
...@@ -1370,7 +1370,9 @@ PasswordAutofillAgent::GetFormDataFromUnownedInputElements() { ...@@ -1370,7 +1370,9 @@ PasswordAutofillAgent::GetFormDataFromUnownedInputElements() {
return nullptr; return nullptr;
return CreateFormDataFromUnownedInputElements( return CreateFormDataFromUnownedInputElements(
*web_frame, field_data_manager_.get(), &username_detector_cache_, *web_frame, field_data_manager_.get(), &username_detector_cache_,
&button_titles_cache_); autofill_agent_ && autofill_agent_->is_heavy_form_data_scraping_enabled()
? &button_titles_cache_
: nullptr);
} }
void PasswordAutofillAgent::InformAboutFormClearing( void PasswordAutofillAgent::InformAboutFormClearing(
......
...@@ -546,6 +546,7 @@ static_library("test_support") { ...@@ -546,6 +546,7 @@ static_library("test_support") {
"//components/translate/core/browser:test_support", "//components/translate/core/browser:test_support",
"//components/ukm", "//components/ukm",
"//components/ukm:test_support", "//components/ukm:test_support",
"//components/version_info:version_info",
"//google_apis:test_support", "//google_apis:test_support",
"//services/network:test_support", "//services/network:test_support",
"//services/network/public/cpp", "//services/network/public/cpp",
......
...@@ -9,6 +9,7 @@ ...@@ -9,6 +9,7 @@
#include "components/autofill/core/browser/autofill_test_utils.h" #include "components/autofill/core/browser/autofill_test_utils.h"
#include "components/autofill/core/browser/payments/local_card_migration_manager.h" #include "components/autofill/core/browser/payments/local_card_migration_manager.h"
#include "components/autofill/core/browser/webdata/autofill_webdata_service.h" #include "components/autofill/core/browser/webdata/autofill_webdata_service.h"
#include "components/version_info/channel.h"
#if !defined(OS_IOS) #if !defined(OS_IOS)
#include "components/autofill/core/browser/payments/test_internal_authenticator.h" #include "components/autofill/core/browser/payments/test_internal_authenticator.h"
...@@ -22,6 +23,10 @@ TestAutofillClient::TestAutofillClient() ...@@ -22,6 +23,10 @@ TestAutofillClient::TestAutofillClient()
TestAutofillClient::~TestAutofillClient() {} TestAutofillClient::~TestAutofillClient() {}
version_info::Channel TestAutofillClient::GetChannel() const {
return channel_for_testing_;
}
PersonalDataManager* TestAutofillClient::GetPersonalDataManager() { PersonalDataManager* TestAutofillClient::GetPersonalDataManager() {
return &test_personal_data_manager_; return &test_personal_data_manager_;
} }
......
...@@ -28,6 +28,7 @@ ...@@ -28,6 +28,7 @@
#include "components/translate/core/browser/language_state.h" #include "components/translate/core/browser/language_state.h"
#include "components/translate/core/browser/mock_translate_driver.h" #include "components/translate/core/browser/mock_translate_driver.h"
#include "components/ukm/test_ukm_recorder.h" #include "components/ukm/test_ukm_recorder.h"
#include "components/version_info/channel.h"
#include "services/metrics/public/cpp/delegating_ukm_recorder.h" #include "services/metrics/public/cpp/delegating_ukm_recorder.h"
#if !defined(OS_IOS) #if !defined(OS_IOS)
...@@ -43,6 +44,7 @@ class TestAutofillClient : public AutofillClient { ...@@ -43,6 +44,7 @@ class TestAutofillClient : public AutofillClient {
~TestAutofillClient() override; ~TestAutofillClient() override;
// AutofillClient: // AutofillClient:
version_info::Channel GetChannel() const override;
PersonalDataManager* GetPersonalDataManager() override; PersonalDataManager* GetPersonalDataManager() override;
AutocompleteHistoryManager* GetAutocompleteHistoryManager() override; AutocompleteHistoryManager* GetAutocompleteHistoryManager() override;
PrefService* GetPrefs() override; PrefService* GetPrefs() override;
...@@ -230,6 +232,10 @@ class TestAutofillClient : public AutofillClient { ...@@ -230,6 +232,10 @@ class TestAutofillClient : public AutofillClient {
autofill_offer_manager_ = std::move(autofill_offer_manager); autofill_offer_manager_ = std::move(autofill_offer_manager);
} }
void set_channel_for_testing(const version_info::Channel channel) {
channel_for_testing_ = channel;
}
GURL form_origin() { return form_origin_; } GURL form_origin() { return form_origin_; }
ukm::TestUkmRecorder* GetTestUkmRecorder(); ukm::TestUkmRecorder* GetTestUkmRecorder();
...@@ -265,6 +271,8 @@ class TestAutofillClient : public AutofillClient { ...@@ -265,6 +271,8 @@ class TestAutofillClient : public AutofillClient {
// otherwise. // otherwise.
base::Optional<bool> credit_card_name_fix_flow_bubble_was_shown_; base::Optional<bool> credit_card_name_fix_flow_bubble_was_shown_;
version_info::Channel channel_for_testing_ = version_info::Channel::UNKNOWN;
// Populated if local save or upload was offered. // Populated if local save or upload was offered.
base::Optional<SaveCreditCardOptions> save_credit_card_options_; base::Optional<SaveCreditCardOptions> save_credit_card_options_;
......
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