Hook up clearing of autofill state for <selectmenu> [chromium/src : main]
Peter Kotwicz (Gerrit)
Attention is currently required from: Stephen McGruer.
Peter Kotwicz would like Stephen McGruer to review this change.
Hook up clearing of autofill state for <selectmenu>
This CL:
- Makes clearing autofill state for <selectmenu> work
- Makes HTMLSelectMenuElement update
HTMLFormControlElement::autofill_state_ and
HTMLFormControlElementWithState::user_has_edited_the_field_
BUG=1420414,1424116
TEST=AutofillChromeClientTest.*
FormAutofillUtilsTest.*
FormCacheBrowserTest.*
Change-Id: Ibaa320aba94f796780564d5d3098de5105c9e824
---
M components/autofill/content/renderer/form_autofill_util.cc
M components/autofill/content/renderer/form_autofill_util.h
M components/autofill/content/renderer/form_autofill_util_browsertest.cc
M components/autofill/content/renderer/form_cache.cc
M components/autofill/content/renderer/form_cache.h
M components/autofill/content/renderer/form_cache_browsertest.cc
M components/autofill/content/renderer/form_cache_test_api.h
M third_party/blink/renderer/core/BUILD.gn
M third_party/blink/renderer/core/exported/web_form_control_element.cc
M third_party/blink/renderer/core/html/forms/html_select_menu_element.cc
M third_party/blink/renderer/core/html/forms/html_select_menu_element.h
M third_party/blink/renderer/core/page/DEPS
M third_party/blink/renderer/core/page/chrome_client_impl_test.cc
13 files changed, 399 insertions(+), 119 deletions(-)
To view, visit change 4364438. To unsubscribe, or for help writing mail filters, visit settings.
Peter Kotwicz (Gerrit)
Attention is currently required from: Stephen McGruer.
1 comment:
Patchset:
Stephen can you please take a look?
To view, visit change 4364438. To unsubscribe, or for help writing mail filters, visit settings.
Stephen McGruer (Gerrit)
Attention is currently required from: Peter Kotwicz.
9 comments:
Patchset:
Hey, sorry, I've been swamped so I didn't manage to finish the full review by EoD. Still to review the below, but wanted to give you some feedback to start with.
Still to review:
- components/autofill/content/renderer/form_cache_browsertest.cc
- third_party/blink/renderer/core/page/chrome_client_impl_test.cc
File components/autofill/content/renderer/form_autofill_util.h:
Patch Set #4, Line 186:
bool IsCheckableElement(const blink::WebInputElement& element);There are a few callsites (2 I found) that call `IsCheckableElement` with an inline DynamicTo call. These could be replaced to call the new version without the DynamicTo now I believe.
Patch Set #4, Line 187:
bool IsCheckableElement(const blink::WebFormControlElement& element);I wonder if we should just replace the WebInputElement version with the new WebFormControlElement one, since WebInputElement is-a WebFormControlElement (and so all existing call-sites should just work).
WDYT?
(I guess the question might be how expensive is an unnecessary DynamicTo call, for the objects that are already WebInputElements. I would hope near-free, but maybe not?)
File components/autofill/content/renderer/form_autofill_util_browsertest.cc:
Patch Set #4, Line 86:
bool AreRendererIdsEqual(const WebElement& element,Nit: when initially reading this, I thought that it checked the ID of the renderer for the element and the field. That isn't the case, it checks the *field* ID (which is only unique for this renderer). Can we think of a name that reflects that, perhaps:
HaveSameFormControlId
(Not sure if the unique-per-renderer part is relevant for the method name)
Patch Set #4, Line 88:
if (!element.IsFormControlElement()) {I think that this should ASSERT that the element is a WebFormControlElement, or just take one as input, rather than check and return false. The current behavior makes it harder to know in the case of:
`EXPECT_FALSE(AreRendererIdsEqual(element, field))`
Did this pass because they didn't have equivalent renderer IDs, or because element wasn't actually a WebFormControlElement.
Patch Set #4, Line 103:
~FormAutofillUtilsTest() override {}use '= default' to define a trivial destructor (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html)
(Note: building this file or its dependencies failed; this diagnostic might be incorrect as a result. Potentially relevant: clang-tidy currently runs in c++17 mode. Use of c++20-only features is currently discouraged. See crbug.com/1406869.)
(Lint observed on `android-clang-tidy-rel`, `linux-chromeos-clang-tidy-rel`, and `linux-clang-tidy-rel`)
Please fix.
Patch Set #4, Line 1522:
class SelectMenuEnableParamTest : public FormAutofillUtilsTest,I think this name could be better, though I'm struggling to come up with one immediately. Perhaps this just needs to somehow indicate that it is about SelectMenuAutofilling rather than just SelectMenu?
SelectMenuAutofillParamTest ?
File components/autofill/content/renderer/form_cache_browsertest.cc:
Patch Set #4, Line 416:
fill_dataThis isn't a parameter; meant to be `form_data`? (Or `form_fill_data`??)
File third_party/blink/renderer/core/html/forms/html_select_menu_element.cc:
Patch Set #4, Line 831:
SetAutofillState(autofill_state);Is this in line with how other autofilled elements work? I was trying to track down the same code for HTMLSelectElement, but failed to find equivalent code.
To view, visit change 4364438. To unsubscribe, or for help writing mail filters, visit settings.
Peter Kotwicz (Gerrit)
Attention is currently required from: Stephen McGruer.
9 comments:
Patchset:
Stephen can you please take another look?
File components/autofill/content/renderer/form_autofill_util.h:
Patch Set #4, Line 186:
bool IsCheckableElement(const blink::WebInputElement& element);
There are a few callsites (2 I found) that call `IsCheckableElement` with an inline DynamicTo call. […]
Replaced those other call sites.
Patch Set #4, Line 187:
bool IsCheckableElement(const blink::WebFormControlElement& element);
I wonder if we should just replace the WebInputElement version with the new WebFormControlElement on […]
I will defer to someone with more autofill experience.
File components/autofill/content/renderer/form_autofill_util_browsertest.cc:
Patch Set #4, Line 86:
bool AreRendererIdsEqual(const WebElement& element,
Nit: when initially reading this, I thought that it checked the ID of the renderer for the element a […]
HaveSameFormControlId() it is.
Patch Set #4, Line 88:
if (!element.IsFormControlElement()) {
I think that this should ASSERT that the element is a WebFormControlElement, or just take one as inp […]
The function now takes a WebFormControlElement as a parameter.
Adding an ASSERT() to this function does not compile because the method returns a bool instead of being void.
Patch Set #4, Line 103:
~FormAutofillUtilsTest() override {}
> use '= default' to define a trivial destructor (https://clang.llvm. […]
Done
Patch Set #4, Line 1522:
class SelectMenuEnableParamTest : public FormAutofillUtilsTest,
I think this name could be better, though I'm struggling to come up with one immediately. […]
Changed to SelectMenuAutofillParamTest.
As SelectMenuAutofillParamTest is used in a parameterized test, the base class is part of the test name.
Ex: FormAutofillUtilsTest/SelectMenuAutofillParamTest.WebFormElementToFormData/0
File components/autofill/content/renderer/form_cache_browsertest.cc:
Patch Set #4, Line 416:
fill_dataThis isn't a parameter; meant to be `form_data`? (Or `form_fill_data`??)
I meant `form_fill_data`. Thanks for catching the documentation inconsistency. Fixed.
File third_party/blink/renderer/core/html/forms/html_select_menu_element.cc:
Patch Set #4, Line 831:
SetAutofillState(autofill_state);
Is this in line with how other autofilled elements work? I was trying to track down the same code fo […]
Yes it is in line with how HTMLSelectElement works - HTMLSelectElement::SelectOption()
The general rule of "if you see a large comment in a CL it likely was copied verbatim from somewhere else" applies 😊
This line of code is tested in AutofillChromeClientTest.
To view, visit change 4364438. To unsubscribe, or for help writing mail filters, visit settings.
Stephen McGruer (Gerrit)
Attention is currently required from: Stephen McGruer.
7 comments:
Patchset:
(Just resolving the finished comments, then will take another look)
File components/autofill/content/renderer/form_autofill_util.h:
Patch Set #4, Line 186:
bool IsCheckableElement(const blink::WebInputElement& element);
Replaced those other call sites.
(Marking resolved; please resolve comments where you have completed the requested action 😊).
File components/autofill/content/renderer/form_autofill_util_browsertest.cc:
Patch Set #4, Line 86:
bool AreRendererIdsEqual(const WebElement& element,
HaveSameFormControlId() it is.
(Resolving comment)
Patch Set #4, Line 88:
if (!element.IsFormControlElement()) {
The function now takes a WebFormControlElement as a parameter. […]
(Resolving comment.)
Patch Set #4, Line 1522:
class SelectMenuEnableParamTest : public FormAutofillUtilsTest,
Changed to SelectMenuAutofillParamTest. […]
Thanks!
(Resolving comment.)
File components/autofill/content/renderer/form_cache_browsertest.cc:
Patch Set #4, Line 416:
fill_data
I meant `form_fill_data`. Thanks for catching the documentation inconsistency. Fixed.
(Resolving comment)
File third_party/blink/renderer/core/html/forms/html_select_menu_element.cc:
Patch Set #4, Line 831:
SetAutofillState(autofill_state);
Yes it is in line with how HTMLSelectElement works - HTMLSelectElement::SelectOption() […]
Thanks!
(Resolving comment)
To view, visit change 4364438. To unsubscribe, or for help writing mail filters, visit settings.
Stephen McGruer (Gerrit)
Attention is currently required from: Peter Kotwicz.
Patch set 6:Code-Review +1
1 comment:
Patchset:
LGTM, thanks!
To view, visit change 4364438. To unsubscribe, or for help writing mail filters, visit settings.
Peter Kotwicz (Gerrit)
Attention is currently required from: Florian Leimgruber.
Peter Kotwicz would like Florian Leimgruber to review this change.
13 files changed, 406 insertions(+), 130 deletions(-)
Peter Kotwicz (Gerrit)
Attention is currently required from: Florian Leimgruber.
1 comment:
Patchset:
Florian can you please take a look?
To view, visit change 4364438. To unsubscribe, or for help writing mail filters, visit settings.
Florian Leimgruber (Gerrit)
Attention is currently required from: Peter Kotwicz.
Patch set 7:Code-Review +1
10 comments:
Patchset:
I didn't look at the blink code. Autofill code LGTM.
File components/autofill/content/renderer/form_autofill_util.h:
Patch Set #4, Line 187:
bool IsCheckableElement(const blink::WebFormControlElement& element);I will defer to someone with more autofill experience.
I don't know how expensive it is either. But it just seems to be an if + static_cast<>. So I vote for removing the WebInputElement version.
File components/autofill/content/renderer/form_autofill_util_browsertest.cc:
Patch Set #7, Line 100:
base::test::ScopedFeatureList scoped_feature_list_;nit: private
if (IsAutofillingSelectMenuEnabled()) {
scoped_feature_list_.InitAndEnableFeature(
features::kAutofillEnableSelectMenu);
} else {
scoped_feature_list_.InitAndDisableFeature(
features::kAutofillEnableSelectMenu);
}```
scoped_feature_list.InitWithFeatureState(
features::kAutofillEnableSelectMenu,
IsAutofillingSelectMenuEnabled());
```const
TODO(crbug.com/1422114): Make test work without explicit <selectmenu>
// tabindexWhy is the tabindex necessary right now?
File components/autofill/content/renderer/form_cache.h:
std::map<FieldRendererId, std::u16string> initial_select_values_;
std::map<FieldRendererId, std::u16string> initial_selectmenu_values_;Is the distinction between select and selectmenu necessary here? Could we just have a single `initial_select_or_selectmenu_values_` map?
File components/autofill/content/renderer/form_cache.cc:
Patch Set #7, Line 496:
// TODO(crbug.com/1336051): Handle selectmenu case.Remove?
File components/autofill/content/renderer/form_cache_browsertest.cc:
for (FormFieldData& field : form_data.fields) {
if (field.name == search_field_name.Utf16()) {
return &field;
}
}
return nullptr;```
auto it = base::ranges::find(form_data.fields,
search_field_name.Utf16(), &FormFieldData::name);
return it != form_data.fields.end() ? &*it : nullptr;
```Patch Set #7, Line 428:
FormFieldData* value_to_fill = FindFieldByName(optional nit: I find `field_to_fill` a more intuitive name.
To view, visit change 4364438. To unsubscribe, or for help writing mail filters, visit settings.
Peter Kotwicz (Gerrit)
Attention is currently required from: Florian Leimgruber, Joey Arhar, Stephen McGruer.
Peter Kotwicz would like Joey Arhar to review this change.
13 files changed, 399 insertions(+), 134 deletions(-)
Peter Kotwicz (Gerrit)
Attention is currently required from: Florian Leimgruber, Joey Arhar, Stephen McGruer.
10 comments:
Patchset:
Joey can you please take a look at the blink changes?
File components/autofill/content/renderer/form_autofill_util.h:
Patch Set #4, Line 187:
bool IsCheckableElement(const blink::WebFormControlElement& element);
I don't know how expensive it is either. But it just seems to be an if + static_cast<>. […]
Removed the WebInputElement version.
File components/autofill/content/renderer/form_autofill_util_browsertest.cc:
Patch Set #7, Line 100:
base::test::ScopedFeatureList scoped_feature_list_;nit: private
Done
if (IsAutofillingSelectMenuEnabled()) {
scoped_feature_list_.InitAndEnableFeature(
features::kAutofillEnableSelectMenu);
} else {
scoped_feature_list_.InitAndDisableFeature(
features::kAutofillEnableSelectMenu);
}
``` […]
Thanks. I was unaware of InitWithFeatureState()!
const
Done
TODO(crbug.com/1422114): Make test work without explicit <selectmenu>
// tabindexWhy is the tabindex necessary right now?
Because HTMLSelectMenuElement::SupportsFocus() has a custom override.
File components/autofill/content/renderer/form_cache.h:
std::map<FieldRendererId, std::u16string> initial_select_values_;
std::map<FieldRendererId, std::u16string> initial_selectmenu_values_;
Is the distinction between select and selectmenu necessary here? Could we just have a single `initia […]
I added a 2nd map for the sake of legibility. There is no difference.
File components/autofill/content/renderer/form_cache.cc:
Patch Set #7, Line 496:
// TODO(crbug.com/1336051): Handle selectmenu case.Remove?
The comment is still needed because I haven't dealt with selectmenus in ScanFormControlElements(). Handling selectmenus in ScanFormControlElements() is trivial. I am trying to make each CL as small as possible to prove that each change is tested.
File components/autofill/content/renderer/form_cache_browsertest.cc:
for (FormFieldData& field : form_data.fields) {
if (field.name == search_field_name.Utf16()) {
return &field;
}
}
return nullptr;
``` […]
That's super cool! I did not know that the base::ranges utility functions existed! Changed.
Patch Set #7, Line 428:
FormFieldData* value_to_fill = FindFieldByName(optional nit: I find `field_to_fill` a more intuitive name.
Changed.
To view, visit change 4364438. To unsubscribe, or for help writing mail filters, visit settings.
Joey Arhar (Gerrit)
Attention is currently required from: Florian Leimgruber, Peter Kotwicz, Stephen McGruer.
Patch set 9:Code-Review +1
Florian Leimgruber (Gerrit)
Attention is currently required from: Peter Kotwicz, Stephen McGruer.
Patch set 9:Code-Review +1
7 comments:
Patchset:
Still LGTM
File components/autofill/content/renderer/form_autofill_util_browsertest.cc:
if (IsAutofillingSelectMenuEnabled()) {
scoped_feature_list_.InitAndEnableFeature(
features::kAutofillEnableSelectMenu);
} else {
scoped_feature_list_.InitAndDisableFeature(
features::kAutofillEnableSelectMenu);
}
Thanks. […]
Acknowledged
TODO(crbug.com/1422114): Make test work without explicit <selectmenu>
// tabindex
Because HTMLSelectMenuElement::SupportsFocus() has a custom override.
Acknowledged
File components/autofill/content/renderer/form_cache.h:
std::map<FieldRendererId, std::u16string> initial_select_values_;
std::map<FieldRendererId, std::u16string> initial_selectmenu_values_;
I added a 2nd map for the sake of legibility. There is no difference.
Acknowledged
File components/autofill/content/renderer/form_cache.cc:
Patch Set #7, Line 496:
// TODO(crbug.com/1336051): Handle selectmenu case.
The comment is still needed because I haven't dealt with selectmenus in ScanFormControlElements(). […]
Ok, thanks!
File components/autofill/content/renderer/form_cache_browsertest.cc:
for (FormFieldData& field : form_data.fields) {
if (field.name == search_field_name.Utf16()) {
return &field;
}
}
return nullptr;That's super cool! I did not know that the base::ranges utility functions existed! Changed.
Acknowledged
Patch Set #7, Line 428:
FormFieldData* value_to_fill = FindFieldByName(
Changed.
Acknowledged
To view, visit change 4364438. To unsubscribe, or for help writing mail filters, visit settings.
Stephen McGruer (Gerrit)
Attention is currently required from: Peter Kotwicz.
To view, visit change 4364438. To unsubscribe, or for help writing mail filters, visit settings.
Peter Kotwicz (Gerrit)
Patch set 10:Commit-Queue +1
Peter Kotwicz (Gerrit)
Patch set 10:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
9 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: components/autofill/content/renderer/form_autofill_util.cc
Insertions: 2, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: components/autofill/content/renderer/form_cache.h
Insertions: 2, Deletions: 1.
The diff is too large to show. Please review the diff.
```
Hook up clearing of autofill state for <selectmenu>
This CL:
- Makes clearing autofill state for <selectmenu> work
- Makes HTMLSelectMenuElement update
HTMLFormControlElement::autofill_state_ and
HTMLFormControlElementWithState::user_has_edited_the_field_
BUG=1420414,1424116
TEST=AutofillChromeClientTest.*
FormAutofillUtilsTest.*
FormCacheBrowserTest.*
Change-Id: Ibaa320aba94f796780564d5d3098de5105c9e824
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4364438
Reviewed-by: Joey Arhar <jar...@chromium.org>
Reviewed-by: Florian Leimgruber <fleim...@google.com>
Reviewed-by: Stephen McGruer <smcg...@chromium.org>
Commit-Queue: Peter Kotwicz <pkot...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1124883}
---
M components/autofill/content/renderer/form_autofill_util.cc
M components/autofill/content/renderer/form_autofill_util.h
M components/autofill/content/renderer/form_autofill_util_browsertest.cc
M components/autofill/content/renderer/form_cache.cc
M components/autofill/content/renderer/form_cache.h
M components/autofill/content/renderer/form_cache_browsertest.cc
M components/autofill/content/renderer/form_cache_test_api.h
M third_party/blink/renderer/core/BUILD.gn
M third_party/blink/renderer/core/exported/web_form_control_element.cc
M third_party/blink/renderer/core/html/forms/html_select_menu_element.cc
M third_party/blink/renderer/core/html/forms/html_select_menu_element.h
M third_party/blink/renderer/core/page/DEPS
M third_party/blink/renderer/core/page/chrome_client_impl_test.cc
13 files changed, 401 insertions(+), 134 deletions(-)