Reland "[Autofill] Simplify ContentAutofillDriver initialization"
This is a reland of commit 3b90869541da848d1791a4ca8b44bc0fd4af8347
The CL did not cause the breakage. The tests seem to be
failing quite consistently on ci/mac12-arm64-rel-tests. The
first failure [1] does include the reverted CL, but the
failures continue after the revert [2].
I suspect [3] is the culprit.
[1] https://ci.chromium.org/ui/p/chromium/builders/ci/mac12-arm64-rel-tests/18253/blamelist
[2] https://ci.chromium.org/ui/p/chromium/builders/ci/mac12-arm64-rel-tests/18262/blamelist
[3] https://chromium-review.googlesource.com/c/chromium/src/+/5224247
Original change's description:
> [Autofill] Simplify ContentAutofillDriver initialization
>
> The DriverInitHooks have two jobs:
> (1) create a {Browser,Android}AutofillManager;
> (2) call some setters on AutofillAgent.
>
> Since the introduction of ContentAutofillClient, the DriverInitHook
> can be conveniently turned into a member function of the client.
>
> This CL introduces *two* new methods in CAC:
> (1) CreateManager(), to be called by CAD's constructor.
> (2) InitAgent(), to be called by the CAD factory.
>
> The point in time they're called is important:
> - CreateManager() is called before OnContentAutofillDriverCreated(),
> which is necessary for TestAutofillManagerInjector;
> - InitAgent() is called after OnContentAutofillDriverCreated(),
> which avoids trouble with double binding mojo remotes when
> TestAutofillDriverInjector creates a new driver.
>
> Alternatives I considered are:
> - Calling CreateManager() from the factory. This would avoid the
> potential problem of the manager calling into the driver during
> the driver's construction. Rejected because this requires
> CAD::set_autofill_manager(), and since driver and manager
> have to point to each other, there will always be some kind of
> chicken-egg problem.
> - Calling InitAgent() from CAD's constructor. Rejected because
> - tests that mock an AutofillAgent would attempt to bind *two*
> remotes, which isn't allowed;
> - unbinding and rebinding remotes
> - makes writing tests harder;
> - seems not to work for the renderer -> browser remote.
> - Calling CreateManager() and InitAgent() or only InitAgent() from
> a virtual CAD::Init() function, which is be called by the CAD
> factory. Tests would override Init() and only call CreateDriver(),
> not InitAgent(). Rejected because that'd make writing tests harder.
>
> The DriverInitHook model worked similarly, but it probably worked
> out of luck: the DriverInitHooks didn't actually call
> GetAutofillAgent() in most tests, which happened to avoid the
> double bind in all relevant tests.
>
> Bug: 1200511
> Change-Id: I339a8b0e7531a76395e5e50dba16f77e3385f56a
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5224630
> Reviewed-by: Jan Keitel <[email protected]>
> Commit-Queue: Christoph Schwering <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#1252040}
Bug: 1200511
Change-Id: Ib923916a0bd31c920ee089569584c1728289731d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5237331
Commit-Queue: Christoph Schwering <[email protected]>
Auto-Submit: Christoph Schwering <[email protected]>
Reviewed-by: Muyao Xu <[email protected]>
Owners-Override: Muyao Xu <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1252350}
diff --git a/chrome/browser/autofill/BUILD.gn b/chrome/browser/autofill/BUILD.gn
index a44c8274..4cff643d 100644
--- a/chrome/browser/autofill/BUILD.gn
+++ b/chrome/browser/autofill/BUILD.gn
@@ -129,6 +129,8 @@
sources = [
"autofill_uitest_util.cc",
"autofill_uitest_util.h",
+ "mock_autofill_agent.cc",
+ "mock_autofill_agent.h",
"mock_autofill_popup_controller.cc",
"mock_autofill_popup_controller.h",
"mock_manual_filling_controller.cc",
@@ -139,6 +141,7 @@
"//chrome/browser/autofill:autofill",
"//chrome/browser/profiles:profile",
"//components/autofill/content/browser:browser",
+ "//components/autofill/content/common/mojom",
"//components/autofill/core/browser:test_support",
"//content/test:test_support",
"//testing/gmock",
diff --git a/chrome/browser/autofill/mock_autofill_agent.cc b/chrome/browser/autofill/mock_autofill_agent.cc
new file mode 100644
index 0000000..afe0878
--- /dev/null
+++ b/chrome/browser/autofill/mock_autofill_agent.cc
@@ -0,0 +1,30 @@
+// Copyright 2024 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/autofill/mock_autofill_agent.h"
+
+#include "content/public/browser/render_frame_host.h"
+#include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h"
+
+namespace autofill {
+
+MockAutofillAgent::MockAutofillAgent() = default;
+MockAutofillAgent::~MockAutofillAgent() = default;
+
+void MockAutofillAgent::BindForTesting(content::RenderFrameHost* rfh) {
+ blink::AssociatedInterfaceProvider* remote_interfaces =
+ rfh->GetRemoteAssociatedInterfaces();
+ remote_interfaces->OverrideBinderForTesting(
+ mojom::AutofillAgent::Name_,
+ base::BindRepeating(&MockAutofillAgent::BindPendingReceiver,
+ weak_ptr_factory_.GetWeakPtr()));
+}
+
+void MockAutofillAgent::BindPendingReceiver(
+ mojo::ScopedInterfaceEndpointHandle handle) {
+ receivers_.Add(this, mojo::PendingAssociatedReceiver<mojom::AutofillAgent>(
+ std::move(handle)));
+}
+
+} // namespace autofill
diff --git a/chrome/browser/autofill/mock_autofill_agent.h b/chrome/browser/autofill/mock_autofill_agent.h
new file mode 100644
index 0000000..aa385c0
--- /dev/null
+++ b/chrome/browser/autofill/mock_autofill_agent.h
@@ -0,0 +1,99 @@
+// Copyright 2024 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_AUTOFILL_MOCK_AUTOFILL_AGENT_H_
+#define CHROME_BROWSER_AUTOFILL_MOCK_AUTOFILL_AGENT_H_
+
+#include "base/memory/weak_ptr.h"
+#include "components/autofill/content/common/mojom/autofill_agent.mojom.h"
+#include "mojo/public/cpp/bindings/associated_receiver_set.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace content {
+class RenderFrameHost;
+}
+
+namespace autofill {
+
+class MockAutofillAgent : public mojom::AutofillAgent {
+ public:
+ MockAutofillAgent();
+ MockAutofillAgent(const MockAutofillAgent&) = delete;
+ MockAutofillAgent& operator=(const MockAutofillAgent&) = delete;
+ ~MockAutofillAgent() override;
+
+ void BindForTesting(content::RenderFrameHost* rfh);
+ void BindPendingReceiver(mojo::ScopedInterfaceEndpointHandle handle);
+
+ MOCK_METHOD(void, TriggerFormExtraction, (), (override));
+ MOCK_METHOD(void,
+ TriggerFormExtractionWithResponse,
+ (base::OnceCallback<void(bool)>),
+ (override));
+ MOCK_METHOD(
+ void,
+ ExtractForm,
+ (FormRendererId form,
+ base::OnceCallback<void(const std::optional<FormData>&)> callback),
+ (override));
+ MOCK_METHOD(void,
+ ApplyFormAction,
+ (mojom::ActionType action_type,
+ mojom::ActionPersistence action_persistence,
+ const FormData::FillData& form),
+ (override));
+ MOCK_METHOD(void,
+ ApplyFieldAction,
+ (mojom::ActionPersistence action_persistence,
+ mojom::TextReplacement text_replacement,
+ FieldRendererId field,
+ const std::u16string& value),
+ (override));
+ MOCK_METHOD(void,
+ FieldTypePredictionsAvailable,
+ (const std::vector<FormDataPredictions>& forms),
+ (override));
+ MOCK_METHOD(void, ClearSection, (), (override));
+ MOCK_METHOD(void, ClearPreviewedForm, (), (override));
+ MOCK_METHOD(void,
+ TriggerSuggestions,
+ (FieldRendererId field_id,
+ AutofillSuggestionTriggerSource trigger_source),
+ (override));
+ MOCK_METHOD(void,
+ SetSuggestionAvailability,
+ (FieldRendererId field,
+ mojom::AutofillSuggestionAvailability suggestion_availability),
+ (override));
+ MOCK_METHOD(void,
+ AcceptDataListSuggestion,
+ (FieldRendererId field, const ::std::u16string& value),
+ (override));
+ MOCK_METHOD(void,
+ PreviewPasswordSuggestion,
+ (const ::std::u16string& username,
+ const ::std::u16string& password),
+ (override));
+ MOCK_METHOD(void,
+ PreviewPasswordGenerationSuggestion,
+ (const ::std::u16string& password),
+ (override));
+ MOCK_METHOD(void, SetUserGestureRequired, (bool required), (override));
+ MOCK_METHOD(void, SetSecureContextRequired, (bool required), (override));
+ MOCK_METHOD(void, SetFocusRequiresScroll, (bool require), (override));
+ MOCK_METHOD(void, SetQueryPasswordSuggestion, (bool query), (override));
+ MOCK_METHOD(void, EnableHeavyFormDataScraping, (), (override));
+ MOCK_METHOD(void,
+ GetPotentialLastFourCombinationsForStandaloneCvc,
+ (base::OnceCallback<void(const std::vector<std::string>&)>),
+ (override));
+
+ private:
+ mojo::AssociatedReceiverSet<mojom::AutofillAgent> receivers_;
+ base::WeakPtrFactory<MockAutofillAgent> weak_ptr_factory_{this};
+};
+
+} // namespace autofill
+
+#endif // CHROME_BROWSER_AUTOFILL_MOCK_AUTOFILL_AGENT_H_
diff --git a/chrome/browser/fast_checkout/fast_checkout_client_impl_unittest.cc b/chrome/browser/fast_checkout/fast_checkout_client_impl_unittest.cc
index aaad224..eba165d 100644
--- a/chrome/browser/fast_checkout/fast_checkout_client_impl_unittest.cc
+++ b/chrome/browser/fast_checkout/fast_checkout_client_impl_unittest.cc
@@ -678,14 +678,9 @@
web_contents()->GetPrimaryMainFrame(),
autofill::ContentAutofillClient::FromWebContents(web_contents())
->GetAutofillDriverFactory());
- auto browser_autofill_manager =
- std::make_unique<autofill::BrowserAutofillManager>(
- autofill_driver.get(),
- autofill::ContentAutofillClient::FromWebContents(web_contents()),
- "en-US");
- autofill::BrowserAutofillManager* autofill_manager =
- browser_autofill_manager.get();
- autofill_driver->set_autofill_manager(std::move(browser_autofill_manager));
+ autofill::BrowserAutofillManager& autofill_manager =
+ static_cast<autofill::BrowserAutofillManager&>(
+ autofill_driver->GetAutofillManager());
// `FastCheckoutClientImpl::autofill_manager_` is `nullptr` initially.
EXPECT_FALSE(fast_checkout_client()->autofill_manager_);
@@ -694,7 +689,7 @@
// Starting the run successfully.
EXPECT_TRUE(fast_checkout_client()->TryToStart(
GURL(kUrl), autofill::FormData(), autofill::FormFieldData(),
- autofill_manager->GetWeakPtr()));
+ autofill_manager.GetWeakPtr()));
OnAfterAskForValuesToFill();
// `FastCheckoutClientImpl::autofill_manager_` is not `nullptr` anymore.
diff --git a/chrome/browser/ui/autofill/chrome_autofill_client.cc b/chrome/browser/ui/autofill/chrome_autofill_client.cc
index 156f9c1..2acba35 100644
--- a/chrome/browser/ui/autofill/chrome_autofill_client.cc
+++ b/chrome/browser/ui/autofill/chrome_autofill_client.cc
@@ -181,11 +181,26 @@
namespace autofill {
namespace {
+
AutoselectFirstSuggestion ShouldAutofillPopupAutoselectFirstSuggestion(
AutofillSuggestionTriggerSource source) {
return AutoselectFirstSuggestion(
source == AutofillSuggestionTriggerSource::kTextFieldDidReceiveKeyDown);
}
+
+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_NORETURN();
+}
+
} // namespace
// static
@@ -1322,11 +1337,7 @@
}
ChromeAutofillClient::ChromeAutofillClient(content::WebContents* web_contents)
- : ContentAutofillClient(
- web_contents,
- base::BindRepeating(&BrowserDriverInitHook,
- this,
- g_browser_process->GetApplicationLocale())),
+ : ContentAutofillClient(web_contents),
content::WebContentsObserver(web_contents),
log_manager_(
// TODO(crbug.com/928595): Replace the closure with a callback to the
@@ -1415,4 +1426,19 @@
}
#endif
+std::unique_ptr<AutofillManager> ChromeAutofillClient::CreateManager(
+ base::PassKey<ContentAutofillDriver> pass_key,
+ ContentAutofillDriver& driver) {
+ return std::make_unique<BrowserAutofillManager>(
+ &driver, this, g_browser_process->GetApplicationLocale());
+}
+
+void ChromeAutofillClient::InitAgent(
+ base::PassKey<ContentAutofillDriverFactory> pass_key,
+ const mojo::AssociatedRemote<mojom::AutofillAgent>& agent) {
+ if (ShouldEnableHeavyFormDataScraping(GetChannel())) {
+ agent->EnableHeavyFormDataScraping();
+ }
+}
+
} // namespace autofill
diff --git a/chrome/browser/ui/autofill/chrome_autofill_client.h b/chrome/browser/ui/autofill/chrome_autofill_client.h
index c73ed43..3b0c5a1 100644
--- a/chrome/browser/ui/autofill/chrome_autofill_client.h
+++ b/chrome/browser/ui/autofill/chrome_autofill_client.h
@@ -289,6 +289,14 @@
return autofill_progress_dialog_controller_.get();
}
+ // ContentAutofillClient:
+ std::unique_ptr<AutofillManager> CreateManager(
+ base::PassKey<ContentAutofillDriver> pass_key,
+ ContentAutofillDriver& driver) override;
+ void InitAgent(
+ base::PassKey<ContentAutofillDriverFactory> pass_key,
+ const mojo::AssociatedRemote<mojom::AutofillAgent>& agent) override;
+
protected:
explicit ChromeAutofillClient(content::WebContents* web_contents);
#if BUILDFLAG(IS_ANDROID)
diff --git a/chrome/browser/ui/autofill/chrome_autofill_client_unittest.cc b/chrome/browser/ui/autofill/chrome_autofill_client_unittest.cc
index 66cd909..fe747da 100644
--- a/chrome/browser/ui/autofill/chrome_autofill_client_unittest.cc
+++ b/chrome/browser/ui/autofill/chrome_autofill_client_unittest.cc
@@ -10,6 +10,7 @@
#include "base/test/scoped_feature_list.h"
#include "build/branding_buildflags.h"
#include "build/build_config.h"
+#include "chrome/browser/autofill/mock_autofill_agent.h"
#include "chrome/browser/autofill/personal_data_manager_factory.h"
#include "chrome/browser/fast_checkout/fast_checkout_client_impl.h"
#include "chrome/browser/plus_addresses/plus_address_service_factory.h"
@@ -27,8 +28,11 @@
#include "components/plus_addresses/features.h"
#include "components/prefs/pref_service.h"
#include "components/unified_consent/pref_names.h"
+#include "mojo/public/cpp/bindings/associated_receiver_set.h"
+#include "mojo/public/cpp/bindings/associated_remote.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h"
#if BUILDFLAG(IS_ANDROID)
#include "chrome/browser/ui/android/autofill/autofill_cvc_save_message_delegate.h"
@@ -76,11 +80,9 @@
};
#endif
-// Exposes the protected constructor.
class TestChromeAutofillClient : public ChromeAutofillClient {
public:
- explicit TestChromeAutofillClient(content::WebContents* web_contents)
- : ChromeAutofillClient(web_contents) {}
+ using ChromeAutofillClient::ChromeAutofillClient;
#if BUILDFLAG(IS_ANDROID)
MockFastCheckoutClient* GetFastCheckoutClient() override {
@@ -98,7 +100,6 @@
}
MockFastCheckoutClient fast_checkout_client_;
- base::test::ScopedFeatureList scoped_feature_list_;
#endif
};
@@ -135,14 +136,6 @@
return personal_data_manager_;
}
- TestContentAutofillDriver* autofill_driver() {
- return test_autofill_driver_injector_[web_contents()];
- }
-
- TestBrowserAutofillManager* autofill_manager() {
- return test_autofill_manager_injector_[web_contents()];
- }
-
#if !BUILDFLAG(IS_ANDROID)
MockSaveCardBubbleController& save_card_bubble_controller() {
return static_cast<MockSaveCardBubbleController&>(
@@ -170,13 +163,8 @@
}
raw_ptr<TestPersonalDataManager> personal_data_manager_ = nullptr;
- TestAutofillClientInjector<TestChromeAutofillClient>
+ TestAutofillClientInjector<testing::NiceMock<TestChromeAutofillClient>>
test_autofill_client_injector_;
- TestAutofillDriverInjector<TestContentAutofillDriver>
- test_autofill_driver_injector_;
- TestAutofillManagerInjector<TestBrowserAutofillManager>
- test_autofill_manager_injector_;
-
base::OnceCallback<void()> setup_flags_;
};
