Skip to content

Commit

Permalink
[Autofill] Merge FormFieldData::datalist_{values,labels}
Browse files Browse the repository at this point in the history
This CL merges the two vectors FormFieldData::datalist_{values,labels}
into one FormFieldData::datalist_options, analogous to the
existing FormFieldData::options that had been merged earlier.

Bug: 1213313
Change-Id: I58b6a89f99e4df9eb80eb85b8f617f3928927b44
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4935534
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Dominic Battre <battre@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Mike Dougherty <michaeldo@chromium.org>
Commit-Queue: Christoph Schwering <schwering@google.com>
Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1212643}
  • Loading branch information
schwering authored and Chromium LUCI CQ committed Oct 20, 2023
1 parent 06bad52 commit 1672129
Show file tree
Hide file tree
Showing 37 changed files with 194 additions and 270 deletions.
3 changes: 1 addition & 2 deletions android_webview/browser/aw_autofill_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -284,8 +284,7 @@ void AwAutofillClient::ShowAutofillPopup(
}

void AwAutofillClient::UpdateAutofillPopupDataListValues(
const std::vector<std::u16string>& values,
const std::vector<std::u16string>& labels) {
base::span<const autofill::SelectOption> datalist) {
// Leaving as an empty method since updating autofill popup window
// dynamically does not seem to be a useful feature for android webview.
// See crrev.com/18102002 if need to implement.
Expand Down
3 changes: 1 addition & 2 deletions android_webview/browser/aw_autofill_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,7 @@ class AwAutofillClient : public autofill::ContentAutofillClient {
const autofill::AutofillClient::PopupOpenArgs& open_args,
base::WeakPtr<autofill::AutofillPopupDelegate> delegate) override;
void UpdateAutofillPopupDataListValues(
const std::vector<std::u16string>& values,
const std::vector<std::u16string>& labels) override;
base::span<const autofill::SelectOption> datalist) override;
std::vector<autofill::Suggestion> GetPopupSuggestions() const override;
void PinPopupView() override;
autofill::AutofillClient::PopupOpenArgs GetReopenPopupArgs(
Expand Down
13 changes: 6 additions & 7 deletions chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -252,8 +252,7 @@ bool AutofillPopupControllerImpl::
}

void AutofillPopupControllerImpl::UpdateDataListValues(
const std::vector<std::u16string>& values,
const std::vector<std::u16string>& labels) {
base::span<const SelectOption> options) {
// Remove all the old data list values, which should always be at the top of
// the list if they are present.
while (!suggestions_.empty() &&
Expand All @@ -263,7 +262,7 @@ void AutofillPopupControllerImpl::UpdateDataListValues(

// If there are no new data list values, exit (clearing the separator if there
// is one).
if (values.empty()) {
if (options.empty()) {
if (!suggestions_.empty() &&
suggestions_[0].popup_item_id == PopupItemId::kSeparator) {
suggestions_.erase(suggestions_.begin());
Expand All @@ -286,11 +285,11 @@ void AutofillPopupControllerImpl::UpdateDataListValues(
}

// Prepend the parameters to the suggestions we already have.
suggestions_.insert(suggestions_.begin(), values.size(), Suggestion());
for (size_t i = 0; i < values.size(); i++) {
suggestions_.insert(suggestions_.begin(), options.size(), Suggestion());
for (size_t i = 0; i < options.size(); i++) {
suggestions_[i].main_text =
Suggestion::Text(values[i], Suggestion::Text::IsPrimary(true));
suggestions_[i].labels = {{Suggestion::Text(labels[i])}};
Suggestion::Text(options[i].value, Suggestion::Text::IsPrimary(true));
suggestions_[i].labels = {{Suggestion::Text(options[i].content)}};
suggestions_[i].popup_item_id = PopupItemId::kDatalistEntry;
}

Expand Down
3 changes: 1 addition & 2 deletions chrome/browser/ui/autofill/autofill_popup_controller_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,7 @@ class AutofillPopupControllerImpl
AutoselectFirstSuggestion autoselect_first_suggestion);

// Updates the data list values currently shown with the popup.
virtual void UpdateDataListValues(const std::vector<std::u16string>& values,
const std::vector<std::u16string>& labels);
virtual void UpdateDataListValues(base::span<const SelectOption> options);

// Informs the controller that the popup may not be hidden by stale data or
// interactions with native Chrome UI. This state remains active until the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,7 @@ class AutofillPopupControllerImplMac : public AutofillPopupControllerImpl {

// Updates the data list values currently shown with the popup. Calls
// -invalidateTouchBar from |touchBarController_|.
void UpdateDataListValues(const std::vector<std::u16string>& values,
const std::vector<std::u16string>& labels) override;
void UpdateDataListValues(base::span<const SelectOption> options) override;

protected:
// Hides the popup and destroys the controller. This also invalidates
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,11 @@
}

void AutofillPopupControllerImplMac::UpdateDataListValues(
const std::vector<std::u16string>& values,
const std::vector<std::u16string>& labels) {
base::span<const SelectOption> options) {
if (touch_bar_controller_)
[touch_bar_controller_ invalidateTouchBar];

AutofillPopupControllerImpl::UpdateDataListValues(values, labels);
AutofillPopupControllerImpl::UpdateDataListValues(options);
// No code below this line!
// |UpdateDataListValues| may hide the popup and destroy |this|, so
// |UpdateDataListValues| should be the last line.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -528,30 +528,24 @@ TEST_F(AutofillPopupControllerImplTest,

TEST_F(AutofillPopupControllerImplTest, UpdateDataListValues) {
ShowSuggestions(manager(), {PopupItemId::kAddressEntry});

// Add one data list entry.
std::u16string value1 = u"data list value 1";
std::vector<std::u16string> data_list_values{value1};
std::u16string label1 = u"data list label 1";
std::vector<std::u16string> data_list_labels{label1};

client().popup_controller(manager()).UpdateDataListValues(data_list_values,
data_list_labels);
std::vector<SelectOption> options = {
{.value = u"data list value 1", .content = u"data list label 1"}};
client().popup_controller(manager()).UpdateDataListValues(options);

ASSERT_EQ(3, client().popup_controller(manager()).GetLineCount());

Suggestion result0 = client().popup_controller(manager()).GetSuggestionAt(0);
EXPECT_EQ(value1, result0.main_text.value);
EXPECT_EQ(value1,
EXPECT_EQ(options[0].value, result0.main_text.value);
EXPECT_EQ(options[0].value,
client().popup_controller(manager()).GetSuggestionMainTextAt(0));
ASSERT_EQ(1u, result0.labels.size());
ASSERT_EQ(1u, result0.labels[0].size());
EXPECT_EQ(label1, result0.labels[0][0].value);
EXPECT_EQ(options[0].content, result0.labels[0][0].value);
EXPECT_EQ(std::u16string(), result0.additional_label);
EXPECT_EQ(label1, client()
.popup_controller(manager())
.GetSuggestionLabelsAt(0)[0][0]
.value);
EXPECT_EQ(options[0].content, client()
.popup_controller(manager())
.GetSuggestionLabelsAt(0)[0][0]
.value);
EXPECT_EQ(PopupItemId::kDatalistEntry, result0.popup_item_id);

Suggestion result1 = client().popup_controller(manager()).GetSuggestionAt(1);
Expand All @@ -567,51 +561,47 @@ TEST_F(AutofillPopupControllerImplTest, UpdateDataListValues) {
EXPECT_EQ(PopupItemId::kAddressEntry, result2.popup_item_id);

// Add two data list entries (which should replace the current one).
std::u16string value2 = u"data list value 2";
data_list_values.push_back(value2);
std::u16string label2 = u"data list label 2";
data_list_labels.push_back(label2);

client().popup_controller(manager()).UpdateDataListValues(data_list_values,
data_list_labels);
options.push_back(
{.value = u"data list value 1", .content = u"data list label 1"});
client().popup_controller(manager()).UpdateDataListValues(options);
ASSERT_EQ(4, client().popup_controller(manager()).GetLineCount());

// Original one first, followed by new one, then separator.
EXPECT_EQ(
value1,
options[0].value,
client().popup_controller(manager()).GetSuggestionAt(0).main_text.value);
EXPECT_EQ(value1,
EXPECT_EQ(options[0].value,
client().popup_controller(manager()).GetSuggestionMainTextAt(0));
ASSERT_EQ(
1u,
client().popup_controller(manager()).GetSuggestionAt(0).labels.size());
ASSERT_EQ(
1u,
client().popup_controller(manager()).GetSuggestionAt(0).labels[0].size());
EXPECT_EQ(label1, client()
.popup_controller(manager())
.GetSuggestionAt(0)
.labels[0][0]
.value);
EXPECT_EQ(options[0].content, client()
.popup_controller(manager())
.GetSuggestionAt(0)
.labels[0][0]
.value);
EXPECT_EQ(
std::u16string(),
client().popup_controller(manager()).GetSuggestionAt(0).additional_label);
EXPECT_EQ(
value2,
options[1].value,
client().popup_controller(manager()).GetSuggestionAt(1).main_text.value);
EXPECT_EQ(value2,
EXPECT_EQ(options[1].value,
client().popup_controller(manager()).GetSuggestionMainTextAt(1));
ASSERT_EQ(
1u,
client().popup_controller(manager()).GetSuggestionAt(1).labels.size());
ASSERT_EQ(
1u,
client().popup_controller(manager()).GetSuggestionAt(1).labels[0].size());
EXPECT_EQ(label2, client()
.popup_controller(manager())
.GetSuggestionAt(1)
.labels[0][0]
.value);
EXPECT_EQ(options[1].content, client()
.popup_controller(manager())
.GetSuggestionAt(1)
.labels[0][0]
.value);
EXPECT_EQ(
std::u16string(),
client().popup_controller(manager()).GetSuggestionAt(1).additional_label);
Expand All @@ -620,9 +610,8 @@ TEST_F(AutofillPopupControllerImplTest, UpdateDataListValues) {
client().popup_controller(manager()).GetSuggestionAt(2).popup_item_id);

// Clear all data list values.
data_list_values.clear();
client().popup_controller(manager()).UpdateDataListValues(data_list_values,
data_list_labels);
options.clear();
client().popup_controller(manager()).UpdateDataListValues(options);

ASSERT_EQ(1, client().popup_controller(manager()).GetLineCount());
EXPECT_EQ(
Expand All @@ -635,29 +624,25 @@ TEST_F(AutofillPopupControllerImplTest, PopupsWithOnlyDataLists) {
ShowSuggestions(manager(), {PopupItemId::kDatalistEntry});

// Replace the datalist element with a new one.
std::u16string value1 = u"data list value 1";
std::vector<std::u16string> data_list_values{value1};
std::u16string label1 = u"data list label 1";
std::vector<std::u16string> data_list_labels{label1};

client().popup_controller(manager()).UpdateDataListValues(data_list_values,
data_list_labels);
std::vector<SelectOption> options = {
{.value = u"data list value 1", .content = u"data list label 1"}};
client().popup_controller(manager()).UpdateDataListValues(options);

ASSERT_EQ(1, client().popup_controller(manager()).GetLineCount());
EXPECT_EQ(
value1,
options[0].value,
client().popup_controller(manager()).GetSuggestionAt(0).main_text.value);
ASSERT_EQ(
1u,
client().popup_controller(manager()).GetSuggestionAt(0).labels.size());
ASSERT_EQ(
1u,
client().popup_controller(manager()).GetSuggestionAt(0).labels[0].size());
EXPECT_EQ(label1, client()
.popup_controller(manager())
.GetSuggestionAt(0)
.labels[0][0]
.value);
EXPECT_EQ(options[0].content, client()
.popup_controller(manager())
.GetSuggestionAt(0)
.labels[0][0]
.value);
EXPECT_EQ(
std::u16string(),
client().popup_controller(manager()).GetSuggestionAt(0).additional_label);
Expand All @@ -668,9 +653,8 @@ TEST_F(AutofillPopupControllerImplTest, PopupsWithOnlyDataLists) {
// Clear datalist values and check that the popup becomes hidden.
EXPECT_CALL(client().popup_controller(manager()),
Hide(PopupHidingReason::kNoSuggestions));
data_list_values.clear();
client().popup_controller(manager()).UpdateDataListValues(data_list_values,
data_list_values);
options.clear();
client().popup_controller(manager()).UpdateDataListValues(options);
}

TEST_F(AutofillPopupControllerImplTest, GetOrCreateAndroid) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,15 +209,17 @@ IN_PROC_BROWSER_TEST_F(AutofillPopupControllerBrowserTest, ResetSelectedLine) {
ASSERT_TRUE(controller);

// Push some suggestions and select the line #3.
std::vector<std::u16string> rows = {u"suggestion1", u"suggestion2",
u"suggestion3", u"suggestion4"};
client->UpdateAutofillPopupDataListValues(rows, rows);
std::vector<SelectOption> rows = {{u"suggestion1", u"suggestion1"},
{u"suggestion2", u"suggestion2"},
{u"suggestion3", u"suggestion3"},
{u"suggestion4", u"suggestion4"}};
client->UpdateAutofillPopupDataListValues(rows);
int original_suggestions_count = controller->GetLineCount();
controller->SelectSuggestion(3u);

// Replace the list with the smaller one.
rows = {u"suggestion1"};
client->UpdateAutofillPopupDataListValues(rows, rows);
rows = {{u"suggestion1", u"suggestion1"}};
client->UpdateAutofillPopupDataListValues(rows);
// Make sure that previously selected line #3 doesn't exist.
ASSERT_LT(controller->GetLineCount(), original_suggestions_count);
// Selecting a new line should not crash.
Expand Down
5 changes: 2 additions & 3 deletions chrome/browser/ui/autofill/chrome_autofill_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1044,10 +1044,9 @@ void ChromeAutofillClient::ShowAutofillPopup(
}

void ChromeAutofillClient::UpdateAutofillPopupDataListValues(
const std::vector<std::u16string>& values,
const std::vector<std::u16string>& labels) {
base::span<const SelectOption> options) {
if (popup_controller_.get())
popup_controller_->UpdateDataListValues(values, labels);
popup_controller_->UpdateDataListValues(options);
}

std::vector<Suggestion> ChromeAutofillClient::GetPopupSuggestions() const {
Expand Down
3 changes: 1 addition & 2 deletions chrome/browser/ui/autofill/chrome_autofill_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -244,8 +244,7 @@ class ChromeAutofillClient : public ContentAutofillClient,
const PopupOpenArgs& open_args,
base::WeakPtr<AutofillPopupDelegate> delegate) override;
void UpdateAutofillPopupDataListValues(
const std::vector<std::u16string>& values,
const std::vector<std::u16string>& labels) override;
base::span<const SelectOption> datalist) override;
std::vector<Suggestion> GetPopupSuggestions() const override;
void PinPopupView() override;
PopupOpenArgs GetReopenPopupArgs(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,10 +120,10 @@ void AutofillProviderAndroid::OnAskForValuesToFill(
StartNewSession(manager, form, field, bounding_box);
}

if (field.datalist_values.empty()) {
if (field.datalist_options.empty()) {
return;
}
bridge_->ShowDatalistPopup(field.datalist_values, field.datalist_labels,
bridge_->ShowDatalistPopup(field.datalist_options,
field.text_direction == base::i18n::RIGHT_TO_LEFT);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <string>

#include "base/containers/span.h"
#include "components/autofill/core/common/form_field_data.h"
#include "components/autofill/core/common/mojom/autofill_types.mojom.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

Expand Down Expand Up @@ -67,9 +68,9 @@ class AutofillProviderAndroidBridge {
virtual void OnServerPredictionQueryDone(bool success) = 0;

// Shows a Datalist popup.
virtual void ShowDatalistPopup(base::span<const std::u16string> values,
base::span<const std::u16string> labels,
bool is_rtl) = 0;
virtual void ShowDatalistPopup(
base::span<const autofill::SelectOption> options,
bool is_rtl) = 0;

// Hides the Datalist popup, if any is showing.
virtual void HideDatalistPopup() = 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,15 +107,23 @@ void AutofillProviderAndroidBridgeImpl::OnFocusChanged(
}

void AutofillProviderAndroidBridgeImpl::ShowDatalistPopup(
base::span<const std::u16string> values,
base::span<const std::u16string> labels,
base::span<const SelectOption> options,
bool is_rtl) {
JNIEnv* env = AttachCurrentThread();
ScopedJavaLocalRef<jobject> obj = java_ref_.get(env);
if (obj.is_null()) {
return;
}

std::vector<std::u16string> values;
std::vector<std::u16string> labels;
values.reserve(options.size());
labels.reserve(options.size());
for (const SelectOption& option : options) {
values.push_back(option.value);
labels.push_back(option.content);
}

Java_AutofillProvider_showDatalistPopup(
env, obj, ToJavaArrayOfStrings(env, values),
ToJavaArrayOfStrings(env, labels), is_rtl);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,7 @@ class AutofillProviderAndroidBridgeImpl : public AutofillProviderAndroidBridge {
const FieldInfo& field,
bool has_server_predictions) override;
void OnServerPredictionQueryDone(bool success) override;
void ShowDatalistPopup(base::span<const std::u16string> values,
base::span<const std::u16string> labels,
void ShowDatalistPopup(base::span<const SelectOption> options,
bool is_rtl) override;
void HideDatalistPopup() override;
void OnFocusChanged(const absl::optional<FieldInfo>& field) override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,7 @@ class MockAutofillProviderAndroidBridge : public AutofillProviderAndroidBridge {
MOCK_METHOD(void, OnServerPredictionQueryDone, (bool success), (override));
MOCK_METHOD(void,
ShowDatalistPopup,
(base::span<const std::u16string>,
base::span<const std::u16string>,
bool),
(base::span<const SelectOption>, bool),
(override));
MOCK_METHOD(void, HideDatalistPopup, (), (override));
MOCK_METHOD(void,
Expand Down

0 comments on commit 1672129

Please sign in to comment.