Skip to content

Commit

Permalink
Remove deprecated PropagateAutofillPredictions() in Android Autofill.
Browse files Browse the repository at this point in the history 
AutofillManager is transitioning to emitting Observer events on having
received type predictions to make it easier for consumers outside of
Autofill to use them.

This CL makes that transition for Android Autofill. It introduces no
functional changes.

Bug: 1466435
Change-Id: I5623bca5781ef87b86fb2202c6eff3d00bbb7054
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4804326
Commit-Queue: Jan Keitel <jkeitel@google.com>
Reviewed-by: Christoph Schwering <schwering@google.com>
Reviewed-by: Dominic Battre <battre@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1188191}
  • Loading branch information
Jan Keitel authored and Chromium LUCI CQ committed Aug 25, 2023
1 parent df917da commit bde4757
Show file tree
Hide file tree
Showing 7 changed files with 94 additions and 44 deletions.
32 changes: 23 additions & 9 deletions components/android_autofill/browser/android_autofill_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@

#include "components/android_autofill/browser/android_autofill_manager.h"

#include <memory>
#include <string>
#include <vector>

#include "base/check_op.h"
#include "base/containers/contains.h"
#include "base/memory/ptr_util.h"
#include "components/android_autofill/browser/autofill_provider.h"
Expand All @@ -30,6 +35,7 @@ AndroidAutofillManager::AndroidAutofillManager(AutofillDriver* driver,
AutofillClient* client)
: AutofillManager(driver, client) {
StartNewLoggingSession();
autofill_manager_observation.Observe(this);
}

AndroidAutofillManager::~AndroidAutofillManager() = default;
Expand Down Expand Up @@ -163,13 +169,6 @@ void AndroidAutofillManager::OnHidePopupImpl() {
provider->OnHidePopup(this);
}

void AndroidAutofillManager::PropagateAutofillPredictionsDeprecated(
const std::vector<FormStructure*>& forms) {
has_server_prediction_ = true;
if (auto* provider = GetAutofillProvider())
provider->OnServerPredictionsAvailable(this);
}

void AndroidAutofillManager::OnFormProcessed(
const FormData& form,
const FormStructure& form_structure) {
Expand All @@ -191,9 +190,10 @@ void AndroidAutofillManager::OnServerRequestError(

void AndroidAutofillManager::Reset() {
AutofillManager::Reset();
has_server_prediction_ = false;
if (auto* provider = GetAutofillProvider())
forms_with_server_predictions_.clear();
if (auto* provider = GetAutofillProvider()) {
provider->Reset(this);
}
StartNewLoggingSession();
}

Expand All @@ -204,6 +204,20 @@ void AndroidAutofillManager::OnContextMenuShownInField(
NOTREACHED();
}

void AndroidAutofillManager::OnFieldTypesDetermined(AutofillManager& manager,
FormGlobalId form,
FieldTypeSource source) {
CHECK_EQ(&manager, this);
if (source != FieldTypeSource::kAutofillServer) {
return;
}

forms_with_server_predictions_.insert(form);
if (auto* provider = GetAutofillProvider()) {
provider->OnServerPredictionsAvailable(form);
}
}

AutofillProvider* AndroidAutofillManager::GetAutofillProvider() {
if (auto* rfh =
static_cast<ContentAutofillDriver&>(driver()).render_frame_host()) {
Expand Down
25 changes: 21 additions & 4 deletions components/android_autofill/browser/android_autofill_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@
#ifndef COMPONENTS_ANDROID_AUTOFILL_BROWSER_ANDROID_AUTOFILL_MANAGER_H_
#define COMPONENTS_ANDROID_AUTOFILL_BROWSER_ANDROID_AUTOFILL_MANAGER_H_

#include <memory>
#include <string>
#include <vector>

#include "base/containers/flat_set.h"
#include "base/memory/raw_ptr.h"
#include "base/memory/weak_ptr.h"
#include "components/autofill/core/browser/autofill_manager.h"
Expand All @@ -28,7 +33,8 @@ void AndroidDriverInitHook(AutofillClient* client,
ContentAutofillDriver* driver);

// This class forwards AutofillManager calls to AutofillProvider.
class AndroidAutofillManager : public AutofillManager {
class AndroidAutofillManager : public AutofillManager,
public AutofillManager::Observer {
public:
AndroidAutofillManager(const AndroidAutofillManager&) = delete;
AndroidAutofillManager& operator=(const AndroidAutofillManager&) = delete;
Expand Down Expand Up @@ -73,7 +79,9 @@ class AndroidAutofillManager : public AutofillManager {

void ReportAutofillWebOTPMetrics(bool used_web_otp) override {}

bool has_server_prediction() const { return has_server_prediction_; }
bool has_server_prediction(FormGlobalId form) const {
return forms_with_server_predictions_.contains(form);
}

FieldTypeGroup ComputeFieldTypeGroupForField(const FormData& form,
const FormFieldData& field);
Expand Down Expand Up @@ -137,13 +145,18 @@ class AndroidAutofillManager : public AutofillManager {
const DenseSet<FormType>& form_types) override {}

void PropagateAutofillPredictionsDeprecated(
const std::vector<FormStructure*>& forms) override;
const std::vector<FormStructure*>& forms) override {}

void OnServerRequestError(FormSignature form_signature,
AutofillDownloadManager::RequestType request_type,
int http_error) override;

private:
// AutofillManager::Observer:
void OnFieldTypesDetermined(AutofillManager& manager,
FormGlobalId form,
FieldTypeSource source) override;

AutofillProvider* GetAutofillProvider();

// Records metrics for loggers and creates new logging session.
Expand All @@ -161,11 +174,15 @@ class AndroidAutofillManager : public AutofillManager {
// Returns logger associated with the passed-in `form_type`.
FormEventLoggerWeblayerAndroid* GetEventFormLogger(FormType form_type);

bool has_server_prediction_ = false;
// The forms that have received server predictions.
base::flat_set<FormGlobalId> forms_with_server_predictions_;
std::unique_ptr<FormEventLoggerWeblayerAndroid> address_logger_;
std::unique_ptr<FormEventLoggerWeblayerAndroid> payments_logger_;
std::unique_ptr<FormEventLoggerWeblayerAndroid> password_logger_;

base::ScopedObservation<AutofillManager, AutofillManager::Observer>
autofill_manager_observation{this};

base::WeakPtrFactory<AndroidAutofillManager> weak_ptr_factory_{this};
};

Expand Down
3 changes: 1 addition & 2 deletions components/android_autofill/browser/autofill_provider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,7 @@ class AutofillProvider : public content::WebContentsUserData<AutofillProvider> {

virtual void OnHidePopup(AndroidAutofillManager* manager) = 0;

virtual void OnServerPredictionsAvailable(
AndroidAutofillManager* manager) = 0;
virtual void OnServerPredictionsAvailable(FormGlobalId form) = 0;

virtual void OnServerQueryRequestError(AndroidAutofillManager* manager,
FormSignature form_signature) = 0;
Expand Down
29 changes: 16 additions & 13 deletions components/android_autofill/browser/autofill_provider_android.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,8 @@ void AutofillProviderAndroid::StartNewSession(AndroidAutofillManager* manager,
Java_AutofillProvider_startAutofillSession(
env, obj, form_obj, index, transformed_bounding.x(),
transformed_bounding.y(), transformed_bounding.width(),
transformed_bounding.height(), manager->has_server_prediction());
transformed_bounding.height(),
manager->has_server_prediction(form.global_id()));
}

void AutofillProviderAndroid::OnAutofillAvailable(JNIEnv* env,
Expand Down Expand Up @@ -376,23 +377,25 @@ void AutofillProviderAndroid::OnHidePopup(AndroidAutofillManager* manager) {
}
}

void AutofillProviderAndroid::OnServerPredictionsAvailable(
AndroidAutofillManager* manager) {
void AutofillProviderAndroid::OnServerPredictionsAvailable(FormGlobalId form) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
if (manager != manager_.get() || !form_.get())
if (!form_ || form_->form().global_id() != form) {
return;
}

if (auto* form_structure =
manager_->FindCachedFormById(form_->form().global_id())) {
form_->UpdateFieldTypes(*form_structure);

JNIEnv* env = AttachCurrentThread();
ScopedJavaLocalRef<jobject> obj = java_ref_.get(env);
if (obj.is_null())
return;
CHECK(manager_);
const FormStructure* form_structure = manager_->FindCachedFormById(form);
if (!form_structure) {
return;
}

Java_AutofillProvider_onQueryDone(env, obj, /*success=*/true);
form_->UpdateFieldTypes(*form_structure);
JNIEnv* env = AttachCurrentThread();
ScopedJavaLocalRef<jobject> obj = java_ref_.get(env);
if (obj.is_null()) {
return;
}
Java_AutofillProvider_onQueryDone(env, obj, /*success=*/true);
}

void AutofillProviderAndroid::OnServerQueryRequestError(
Expand Down
2 changes: 1 addition & 1 deletion components/android_autofill/browser/autofill_provider_android.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ class AutofillProviderAndroid : public AutofillProvider {
const FormData& form,
base::TimeTicks timestamp) override;
void OnHidePopup(AndroidAutofillManager* manager) override;
void OnServerPredictionsAvailable(AndroidAutofillManager* manager) override;
void OnServerPredictionsAvailable(FormGlobalId form) override;
void OnServerQueryRequestError(AndroidAutofillManager* manager,
FormSignature form_signature) override;

Expand Down
45 changes: 31 additions & 14 deletions components/android_autofill/browser/autofill_provider_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,17 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "components/android_autofill/browser/autofill_provider.h"

#include "base/memory/raw_ptr.h"
#include "components/android_autofill/browser/android_autofill_manager.h"
#include "components/android_autofill/browser/test_autofill_provider.h"
#include "components/autofill/content/browser/test_autofill_client_injector.h"
#include "components/autofill/content/browser/test_autofill_driver_injector.h"
#include "components/autofill/content/browser/test_autofill_manager_injector.h"
#include "components/autofill/content/browser/test_content_autofill_client.h"
#include "components/autofill/core/common/autofill_test_utils.h"
#include "components/autofill/core/common/unique_ids.h"
#include "content/public/test/test_renderer_host.h"
#include "testing/gtest/include/gtest/gtest.h"

Expand All @@ -20,13 +24,21 @@ class TestAndroidAutofillManager : public AndroidAutofillManager {
ContentAutofillClient* client)
: AndroidAutofillManager(driver, client) {}

void SimulatePropagateAutofillPredictions() {
PropagateAutofillPredictionsDeprecated({});
void SimulatePropagateAutofillPredictions(FormGlobalId form_id) {
NotifyObservers(&Observer::OnFieldTypesDetermined, form_id,
Observer::FieldTypeSource::kAutofillServer);
}

void SimulateOnAskForValuesToFillImpl() {
void SimulateOnAskForValuesToFillImpl(FormGlobalId form_id) {
FormData form;
form.host_frame = form_id.frame_token;
form.unique_renderer_id = form_id.renderer_id;

FormFieldData field;
field.host_frame = form.host_frame;
field.unique_renderer_id = test::MakeFieldRendererId();
OnAskForValuesToFillImpl(
FormData(), FormFieldData(), gfx::RectF(),
form, field, gfx::RectF(),
AutofillSuggestionTriggerSource::kTextFieldDidChange);
}
};
Expand All @@ -35,7 +47,9 @@ class FakeAutofillProvider : public TestAutofillProvider {
public:
using TestAutofillProvider::TestAutofillProvider;

bool HasServerPrediction() const { return manager_->has_server_prediction(); }
bool HasServerPrediction(FormGlobalId form_id) const {
return manager_->has_server_prediction(form_id);
}

private:
// AutofillProvider:
Expand Down Expand Up @@ -80,6 +94,7 @@ class AutofillProviderTest : public content::RenderViewHostTestHarness {
ASSERT_TRUE(FakeAutofillProvider::FromWebContents(web_contents()));
}

test::AutofillUnitTestEnvironment autofill_environment_;
TestAutofillClientInjector<TestContentAutofillClient>
autofill_client_injector_;
TestAutofillManagerInjector<TestAndroidAutofillManager>
Expand All @@ -88,21 +103,23 @@ class AutofillProviderTest : public content::RenderViewHostTestHarness {

TEST_F(AutofillProviderTest, HasServerPredictionAfterQuery) {
// Simulate the result arrives after starting autofill.
android_autofill_manager().SimulateOnAskForValuesToFillImpl();
EXPECT_FALSE(autofill_provider().HasServerPrediction());
android_autofill_manager().SimulatePropagateAutofillPredictions();
EXPECT_TRUE(autofill_provider().HasServerPrediction());
FormGlobalId form_id = test::MakeFormGlobalId();
android_autofill_manager().SimulateOnAskForValuesToFillImpl(form_id);
EXPECT_FALSE(autofill_provider().HasServerPrediction(form_id));
android_autofill_manager().SimulatePropagateAutofillPredictions(form_id);
EXPECT_TRUE(autofill_provider().HasServerPrediction(form_id));
android_autofill_manager().Reset();
EXPECT_FALSE(autofill_provider().HasServerPrediction());
EXPECT_FALSE(autofill_provider().HasServerPrediction(form_id));
}

TEST_F(AutofillProviderTest, HasServerPredictionBeforeQuery) {
// Simulate the result arrives before starting autofill.
android_autofill_manager().SimulatePropagateAutofillPredictions();
android_autofill_manager().SimulateOnAskForValuesToFillImpl();
EXPECT_TRUE(autofill_provider().HasServerPrediction());
FormGlobalId form_id = test::MakeFormGlobalId();
android_autofill_manager().SimulatePropagateAutofillPredictions(form_id);
android_autofill_manager().SimulateOnAskForValuesToFillImpl(form_id);
EXPECT_TRUE(autofill_provider().HasServerPrediction(form_id));
android_autofill_manager().Reset();
EXPECT_FALSE(autofill_provider().HasServerPrediction());
EXPECT_FALSE(autofill_provider().HasServerPrediction(form_id));
}

} // namespace autofill
2 changes: 1 addition & 1 deletion components/android_autofill/browser/test_autofill_provider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class TestAutofillProvider : public AutofillProvider {
const FormData& form,
base::TimeTicks timestamp) override {}
void OnHidePopup(AndroidAutofillManager* manager) override {}
void OnServerPredictionsAvailable(AndroidAutofillManager* manager) override {}
void OnServerPredictionsAvailable(FormGlobalId form) override {}
void OnServerQueryRequestError(AndroidAutofillManager* manager,
FormSignature form_signature) override {}
void Reset(AndroidAutofillManager* manager) override {}
Expand Down

0 comments on commit bde4757

Please sign in to comment.