[autofill in devtools] Creates a new class to handle form issues that are emitted to devtools. [chromium/src : main]
Bruno Braga (Gerrit)
Attention is currently required from: Christoph Schwering, Florian Leimgruber.
17 comments:
File components/autofill/content/renderer/form_autofill_util.cc:
PTAL. […]
Done
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-labelledby#values […]
Done
Patch Set #2, Line 749:
MaybeDo you mean the autocomplete issue? updated.
Done
base::ranges::any_of(
GetWebElementsFromIdList(element.GetDocument(),
arial_label_attribute),
[](const WebElement& node) { return node.IsNull(); })I meant something like […]
I see. Good point
base::flat_map<WebString, int> id_count;
for (const WebFormControlElement& element : control_elements) {
if (IsAutofillableElement(element) && !element.GetIdAttribute().IsEmpty()) {
id_count[element.GetIdAttribute()]++;
}
}I think it is. I actually don't think it's more complex. […]
Also updated the names in the new class to keep it consistent.
Patch Set #2, Line 1615:
static base::NoDestructor<WebString> kName("name");NoDescrutor is needed because ~WebString() destructor is non-trivial because ~scoped_refptr() is non […]
I updated the new class not to clash with your current CL, ptal.
Patch Set #2, Line 1624:
violatingAttrI meant that "violating" isn't accurate. […]
We can call it violating_attr_value. However a violating attribute means an attribute that does not exist? a typo?
I think it is implicit in this case that we are talking about the value since we cannot even know if a literally violating attribute exists. I can rename the declarations in this file to be violating_attribute_value if you prefer.
element.GetDocument().GetFrame()->AddGenericIssue(
blink::mojom::GenericIssueErrorType::
kFormAutocompleteAttributeEmptyError,
element.GetDevToolsNodeId(), WebString("autocomplete"));
form_issues->emplace_back(
GenericIssueErrorType::kFormAutocompleteAttributeEmptyError,
element.GetDevToolsNodeId(), WebString("autocomplete"));Obsolete with recent recent refactor.
Done
Patch Set #2, Line 1908:
const WebInputElement input_element = element.DynamicTo<WebInputElement>();surprised lint does not catch this.
Done
Patch Set #2, Line 2315:
field->aria_label = GetAriaLabel(host.GetDocument(), host);This looks like it should be outside of the `if`.
Done
File components/autofill/content/renderer/form_autofill_util.cc:
Patch Set #8, Line 1551:
violatingAttr`violating_attr`
Done
Patch Set #8, Line 1815:
WebString("aria-labelledby")kAriaLabelledBy
Done
File components/autofill/content/renderer/form_autofill_util_browsertest.cc:
=* […]
Done
File components/autofill/content/renderer/form_cache.cc:
Patch Set #2, Line 181:
std::vector<blink::WebAutofillClient::FormIssue> form_issues;As of now we need form_field_data which is inside the cache. […]
Done
Patch Set #2, Line 186:
&form_issuesI added the array in the cache to make sure the tests would pass and ended up forgetting to delete. […]
Done
File components/autofill/core/common/form_field_data.h:
Patch Set #8, Line 319:
// Whether the label source `for` attribute linked to the elements name.Add a comment that this is not serialized.
Done. When you say serialized do you mean to send it to the browser or is it actually stored somewhere?
Patch Set #8, Line 320:
label_source_for_is_field_nameI think given the gigantic size of a FormFieldData this is OK. […]
This makes sense. Do we have a bug for this?
To view, visit change 4543002. To unsubscribe, or for help writing mail filters, visit settings.
Florian Leimgruber (Gerrit)
Attention is currently required from: Bruno Braga, Christoph Schwering.
9 comments:
File components/autofill/content/renderer/form_autofill_issues.h:
e.g. instead of i.e.?
File components/autofill/content/renderer/form_autofill_issues.cc:
Patch Set #12, Line 1:
// Copyright 2023 The Chromium AuthorsI suppose most of the implementation of this class is moved here from form_autofill_util? Is the implementation there kept so that issues are continued to be emitted once this CL lands?
Are there any changes in this file that need to be properly reviewed?
File components/autofill/content/renderer/form_autofill_util.h:
nit: Comment
File components/autofill/content/renderer/form_autofill_util.cc:
base::flat_map<WebString, int> id_count;
for (const WebFormControlElement& element : control_elements) {
if (IsAutofillableElement(element) && !element.GetIdAttribute().IsEmpty()) {
id_count[element.GetIdAttribute()]++;
}
}
Also updated the names in the new class to keep it consistent.
You could also keep the second half of the implementation as-is and create the flat map using [MakeFlatMap()](https://source.chromium.org/chromium/chromium/src/+/main:base/containers/flat_map.h;l=373;drc=e4622aaeccea84652488d1822c28c78b7115684f). This would solve the runtime problem too.
File components/autofill/content/renderer/form_autofill_util.cc:
Patch Set #12, Line 1525:
// Create copies of |elements| that can be modifiedInstead of copying all `elements`, you can just create a vector of element IDs and use that.
base::ranges::sort(elements_copies, {},
&WebFormControlElement::GetIdAttribute);You're sorting an empty vector here.
for (++it; it != elements.end() &&
std::prev(it)->GetIdAttribute() == it->GetIdAttribute();
++it) {This will not emit the issue for the last element in sorted order. E.g. if you have 3 elements with the same ID, the issue is only emitted twice.
Patch Set #12, Line 1542:
prev(it)This `prev()` is unnecessary. All elements should point to the same document.
File components/autofill/core/common/form_field_data.h:
Patch Set #8, Line 320:
label_source_for_is_field_nameThis makes sense. […]
I think we should either:
- Make this of type [`AssignedLabelSource`](https://source.chromium.org/chromium/chromium/src/+/refs/heads/main:components/autofill/content/renderer/form_autofill_util.h;l=76-85;drc=f2c32198ce5251791dc0a79ad63404509c22fee1), which captures all the possible label-for inferences cases.
- Don't add another member, but add a new `LabelSource` enum value.
To view, visit change 4543002. To unsubscribe, or for help writing mail filters, visit settings.
Christoph Schwering (Gerrit)
Attention is currently required from: Bruno Braga.
2 comments:
Patchset:
I'll wait until Florian's comments are addressed :)
File components/autofill/core/common/form_field_data.h:
Patch Set #8, Line 320:
label_source_for_is_field_nameThis makes sense. […]
crbug.com/1345089 subsumes this
(I don't remember writing this comment, but maybe it's useful.)
To view, visit change 4543002. To unsubscribe, or for help writing mail filters, visit settings.
Findit (Gerrit)
Attention is currently required from: Bruno Braga.
Bruno Braga (Gerrit)
Attention is currently required from: Christoph Schwering, Florian Leimgruber.
9 comments:
File components/autofill/content/renderer/form_autofill_issues.h:
e.g. instead of i.e. […]
Done
File components/autofill/content/renderer/form_autofill_issues.cc:
Patch Set #12, Line 1:
// Copyright 2023 The Chromium Authors
I suppose most of the implementation of this class is moved here from form_autofill_util? Is the imp […]
Yes. I will start emitting the issues from this file on a follow up CL, eventually I will remove the emission from there (I will possibly have a flag). Added a todo.
This files essentially copies the what we are doing inside form_autofill_util.cc, you can review code styling but the functionality is essentially the same.
File components/autofill/content/renderer/form_autofill_util.h:
nit: Comment
Done
File components/autofill/content/renderer/form_autofill_util.cc:
base::flat_map<WebString, int> id_count;
for (const WebFormControlElement& element : control_elements) {
if (IsAutofillableElement(element) && !element.GetIdAttribute().IsEmpty()) {
id_count[element.GetIdAttribute()]++;
}
}
You could also keep the second half of the implementation as-is and create the flat map using [MakeF […]
From reading MakeFlatMap documentation and its usages it does not look like we can create a counter by key and only map 1:1.
File components/autofill/content/renderer/form_autofill_util.cc:
Patch Set #12, Line 1525:
// Create copies of |elements| that can be modifiedInstead of copying all `elements`, you can just create a vector of element IDs and use that.
how? I need the frame and the node id to create the issue.
base::ranges::sort(elements_copies, {},
&WebFormControlElement::GetIdAttribute);You're sorting an empty vector here.
Done
for (++it; it != elements.end() &&
std::prev(it)->GetIdAttribute() == it->GetIdAttribute();
++it) {
This will not emit the issue for the last element in sorted order. E.g. […]
It would not be emitted for the first one. This why I have the prev check. Unless I am missing something, you can see that the test is covering it.
Patch Set #12, Line 1542:
prev(it)This `prev()` is unnecessary. All elements should point to the same document.
I know, but it is necessary to get the nodeId. I think for a reader it makes sense to keep it consistent other than using prev in one place but not in the other. Removed and added a comment.
File components/autofill/core/common/form_field_data.h:
Patch Set #8, Line 320:
label_source_for_is_field_name
crbug.com/1345089 subsumes this […]
Done
To view, visit change 4543002. To unsubscribe, or for help writing mail filters, visit settings.
Florian Leimgruber (Gerrit)
Attention is currently required from: Bruno Braga, Christoph Schwering.
9 comments:
File components/autofill/content/renderer/form_autofill_issues.cc:
Patch Set #12, Line 1:
// Copyright 2023 The Chromium Authors
This files essentially copies the what we are doing inside form_autofill_util.cc, you can review code styling but the functionality is essentially the same.
Given that the code already starts to diverge (see my comment in `MaybeEmitDuplicateIdForInputDevtoolsIssue()`), I wonder if we can avoid the duplication at all.
Could we for example remove the code from form_autofill_util and just call the new functions in form_autofill_issues? The implementation in form_autofill_issues can then decide to either emit the issue directly if the `form_issues` parameter is null, or add it to the vector otherwise.
More generally, to make the review easier, it would be nice to have one CL that removes the code into the file 1:1 and another CL that makes the actual changes. This CL does both, which makes it really hard for reviewers to spot the differences with a diff of +764. WDYT?
File components/autofill/content/renderer/form_autofill_util.cc:
Patch Set #2, Line 2788:
if (base::FeatureList::IsEnabled(features::kAutofillEnableDevtoolsIssues)) {Obsolete with new refactor, but yes. That was the reason.
Acknowledged
base::flat_map<WebString, int> id_count;
for (const WebFormControlElement& element : control_elements) {
if (IsAutofillableElement(element) && !element.GetIdAttribute().IsEmpty()) {
id_count[element.GetIdAttribute()]++;
}
}
From reading MakeFlatMap documentation and its usages it does not look like we can create a counter […]
Upps, that makes sense. Sorry
File components/autofill/content/renderer/form_autofill_util.cc:
Patch Set #12, Line 1525:
// Create copies of |elements| that can be modified
how? I need the frame and the node id to create the issue.
You're right, I misread the code.
for (++it; it != elements.end() &&
std::prev(it)->GetIdAttribute() == it->GetIdAttribute();
++it) {
It would not be emitted for the first one. This why I have the prev check. […]
The implementation in form_autofill_util and form_autofill_issues differs. In the latter file you've added this snippet inside the main loop:
```
if (previous_element_not_added) {
form_issues->emplace_back(
GenericIssueErrorType::kFormDuplicateIdForInputError,
prev(it)->GetDevToolsNodeId(), id_attr);
}
```This ensures that you don't miss the first one. The code in form_autofill_util still has the bug.
Patch Set #12, Line 1542:
prev(it)
I know, but it is necessary to get the nodeId. […]
Acknowledged
File components/autofill/content/renderer/form_autofill_util.cc:
Patch Set #17, Line 1528:
WebVector<WebFormControlElement> elements_copies;nit: Maybe rename, since this is not really a copy, but only the autofillable elements with an ID.
Patch Set #17, Line 1548:
// Please not that all elements are pointing to the same document.*note
There's a word missing
To view, visit change 4543002. To unsubscribe, or for help writing mail filters, visit settings.
Bruno Braga (Gerrit)
Attention is currently required from: Christoph Schwering, Florian Leimgruber.
4 comments:
File components/autofill/content/renderer/form_autofill_issues.cc:
Patch Set #12, Line 1:
// Copyright 2023 The Chromium Authors
> This files essentially copies the what we are doing inside form_autofill_util. […]
Resolved offline. Chained the suggestion approach n a follow up cl.
File components/autofill/content/renderer/form_autofill_util.cc:
for (++it; it != elements.end() &&
std::prev(it)->GetIdAttribute() == it->GetIdAttribute();
++it) {
The implementation in form_autofill_util and form_autofill_issues differs. […]
we are not pushing to an array in there, we are always emitting the prev issue, so there is no bug (the issue is deduped on the devtools side). Updated this test as well for clarity
File components/autofill/content/renderer/form_autofill_util.cc:
Patch Set #17, Line 1528:
WebVector<WebFormControlElement> elements_copies;nit: Maybe rename, since this is not really a copy, but only the autofillable elements with an ID.
Done
Patch Set #17, Line 1548:
// Please not that all elements are pointing to the same document.*note
Done
To view, visit change 4543002. To unsubscribe, or for help writing mail filters, visit settings.
Florian Leimgruber (Gerrit)
Attention is currently required from: Bruno Braga, Christoph Schwering.
21 comments:
Commit Message:
[autofill in devtools] Creates a new class to handle form issues that are emitted to devtools.
next step: Either inside form cache or the agent (TBD) we will call the methods inside form_autofill_issues.h to create the vector of FormIssues. This will only be done in the case where devtools is open only.
notes:
1. I removed the frame from FormIssue because it turns out we do not use it.
2. I made GetAutocompleteAttribute, HasAutocompleteAttribute and GetWebElementsFromIdList public so that they can be reused inside the new class.
2. I refactored some parts of the existing code. With the refactor that came from the initial review, things changes quite a bit. However there were still comments regarding the existing code, I addressed them as well. I can break into another CL if you prefer.nit: The CL has changed quite a bit since the original description. Could you update it, stating that/why the current implementation is duplicated? And please mention which parts are new and which aren't.
File components/autofill/content/renderer/form_autofill_issues.h:
emit devtools
// issues for themShould this rather read "add them to `form_issues`"?
void GetFormIssues(
const blink::WebFormElement& form_element,
std::vector<blink::WebAutofillClient::FormIssue>* form_issues);
// Same as above, however instead of receiving a form, it received a list of
// control elements that were found outside a form tag.
void GetFormIssuesFromUnownedControlElements(
const std::vector<blink::WebFormControlElement>& unowned_control_elements,
std::vector<blink::WebAutofillClient::FormIssue>* form_issues);The implementation of `GetFormIssues()` basically just calls `GetFormIssuesFromUnownedControlElements(form_element.GetFormControlElements(), form_issues)`. Should we just have a single function?
```
GetFormIssues(
const std::vector<blink::WebFormControlElement>& control_elements,
std::vector<blink::WebAutofillClient::FormIssue>* form_issues)
```
File components/autofill/content/renderer/form_autofill_issues.cc:
Patch Set #12, Line 1:
// Copyright 2023 The Chromium Authors
Resolved offline. Chained the suggestion approach n a follow up cl.
Acknowledged
File components/autofill/content/renderer/form_autofill_issues.cc:
// Realistically devtools already dedupes issues. However we make sure not
// to push the same one here for readability.
bool previous_element_not_added =
form_issues->empty() ||
(form_issues->back().issue_type ==
GenericIssueErrorType::kFormDuplicateIdForInputError &&
form_issues->back().violating_node != prev(it)->GetDevToolsNodeId());
if (previous_element_not_added) {
form_issues->emplace_back(
GenericIssueErrorType::kFormDuplicateIdForInputError,
prev(it)->GetDevToolsNodeId(), id_attr);
}
I think it suffices to do this once before the inner for loop?
Patch Set #20, Line 148:
if (!form_issues) {This is inconsistent across this file. Some functions are conditioned on `form_issues` being non-null, others aren't. Is this intentional?
Patch Set #20, Line 228:
DCHECK(!labels.IsNull());nit: Use CHECK. See [tip of the week](https://g3doc.corp.google.com/devtools/library_club/g3doc/totw/labs/avoid-dcheck.md).
Patch Set #20, Line 234:
!label.CorrespondingControl().IsNull()Since the `label` is one of the parameters, I think we should get rid of this third parameter.
Patch Set #20, Line 272:
WebElementCollection labels = document.GetElementsByHTMLTagName(label_attr);nit: Move this down to where it's actually used.
Patch Set #20, Line 276:
field.assigned_label_source == AssignedLabelSource::kNameI think you should check that the `label_source` is `kFor`. Otherwise `assigned_label_source` is not set.
Patch Set #20, Line 294:
// TODO(crbug.com/1339277): Use `root` once the feature is launched.This comment doesn't make any sense here, `root` is not in the scope :) (and the feature was launched several months ago)
Patch Set #20, Line 301:
kFormLabelForNameErrorkFormLabelForMatchesNonExistingIdError?
File components/autofill/content/renderer/form_autofill_issues_browsertest.cc:
Patch Set #20, Line 26:
bool formIssuesContainIssue(nit:
- Upper case F.
- Maybe "ContainsIssueType" instead of "ContainsIssue"
- Comment explaining what `violating_attr` does.
for (const blink::WebAutofillClient::FormIssue& form_issue : form_issues) {
if (form_issue.issue_type == expected_issue) {
return violating_attr.empty()
? true
: violating_attr == form_issue.violating_node_attribute.Utf8();
}
}
return false;nit: Use base::ranges::find. Something like this should work
```
if (auto it = base::ranges::find(form_issues, expected_issue, [](auto issue) {
return issue.issue_type;
}); it != form_issues.end()) {
return ...
}
return false
```Patch Set #20, Line 45:
WebFormElement WebFormElementFromHTML(const char* html) {nit: Maybe make ID of the form element a parameter (with default value). Then it's obvious from the interface what the function does.
"<form id='target'>"
" <input />"
" <label> A label</label>"
"</form>";Nits for the HTML code here and below:
- You can use raw string literals like this:
```
constexpr char kHTML[] = R"(
<form>
<input>
</form>
)";
```
- You don't need quotes when the attribute value doesn't contain spaces. E.g. `id=target` works. If you want to keep the quotes, please be consistent (here you use single quotes. The next test uses double quotes).
- `<input>` instead of `<input />`.int duplicated_ids_issue_count = 0;
for (const blink::WebAutofillClient::FormIssue& form_issue : form_issues) {
if (form_issue.issue_type ==
GenericIssueErrorType::kFormDuplicateIdForInputError &&
form_issue.violating_node_attribute.Utf8() == "id") {
duplicated_ids_issue_count++;
}
}Simplify using `base::ranges::count()` (similar to my suggestion for `formIssuesContainIssue()`)
std::vector<FormFieldData> fields;
for (size_t i = 0; i < 3; ++i) {
FormFieldData expected;
expected.id_attribute = u"id_" + base::NumberToString16(i);
expected.name_attribute = u"name_" + base::NumberToString16(i);
expected.assigned_label_source = AssignedLabelSource::kName;
fields.push_back(expected);
}Can you declare these as <input>s in the HTML and then extract them from the loaded DOM? Same below.
File components/autofill/content/renderer/form_autofill_util.h:
Patch Set #20, Line 414:
bool HasAutocompleteAttribute(const blink::WebElement& element);nit: This function has a one line implementation. Do we really need to expose it through the header? Maybe it's simpler to just duplicate this line in the form issues file.
File components/autofill/content/renderer/form_autofill_util.cc:
for (++it; it != elements.end() &&
std::prev(it)->GetIdAttribute() == it->GetIdAttribute();
++it) {
we are not pushing to an array in there, we are always emitting the prev issue, so there is no bug ( […]
Ack. Please add a comment stating that DevTools does the dedup for us.
File components/autofill/content/renderer/form_autofill_util.cc:
auto it = elements_with_id_attr.begin();
while ((it = base::ranges::adjacent_find(
it, elements_with_id_attr.end(), {},
&WebFormControlElement::GetIdAttribute)) !=
elements_with_id_attr.end()) {
for (++it; it != elements_with_id_attr.end() &&
std::prev(it)->GetIdAttribute() == it->GetIdAttribute();
++it) {
I think you can merge both loops into a single loop like this:
```
for (auto it = elements_with_id_attr.begin();
(it = base::ranges::adjacent_find(
it, elements_with_id_attr.end(), {},
&WebFormControlElement::GetIdAttribute)) !=
elements_with_id_attr.end(); it++) {
// emit issues for it and it+1
}
```
To view, visit change 4543002. To unsubscribe, or for help writing mail filters, visit settings.
Christoph Schwering (Gerrit)
Attention is currently required from: Bruno Braga.
7 comments:
File components/autofill/content/renderer/form_autofill_issues.h:
Looks, go/cstyle#function_declarations
e.g. should be preceded by a comma according all and succeed by a comma according to most style guides as far as I'm aware (https://english.stackexchange.com/a/16215, https://www.dailywritingtips.com/comma-after-i-e-and-e-g/)
Patch Set #20, Line 22:
GetFormIssuesIs this only called in tests?
Patch Set #20, Line 24:
std::vector<blink::WebAutofillClient::FormIssue>*Do we need an out parameter here and elsewhere? What does "Get" mean in this context? go/totw/176
File components/autofill/content/renderer/form_autofill_util.cc:
Patch Set #20, Line 1586:
Please note that allnit: "All"
go/cstyle#Vertical_Whitespace
while ((it = base::ranges::adjacent_find(
it, elements_with_id_attr.end(), {},
&WebFormControlElement::GetIdAttribute)) !=
elements_with_id_attr.end()) {
for (++it; it != elements_with_id_attr.end() &&
std::prev(it)->GetIdAttribute() == it->GetIdAttribute();
++it) {
// Please note that all elements are pointing to the same document.
// Therefore we can it simply use `it` and not `prev`.
it->GetDocument().GetFrame()->AddGenericIssue(
GenericIssueErrorType::kFormDuplicateIdForInputError,
prev(it)->GetDevToolsNodeId(), id_attr);
it->GetDocument().GetFrame()->AddGenericIssue(
GenericIssueErrorType::kFormDuplicateIdForInputError,
it->GetDevToolsNodeId(), id_attr);
}
}I think the inner loop is worth eliminating.
If there are three consecutive fields with the IDs, this emits (1), (2), (2), (3), doesn't it? Removing the `for (...)` entirely wouldn't change anything, right?
To view, visit change 4543002. To unsubscribe, or for help writing mail filters, visit settings.
Bruno Braga (Gerrit)
Attention is currently required from: Bruno Braga.
Bruno Braga uploaded patch set #22 to this change.
[autofill in devtools] Creates a new class to handle form issues that are emitted to devtools.
This class has the following behaviour right now:
1. It handles the exact same issues form_utils handles today, therefore does not add any new user visible behaviour.
3. Appends issues to a provided vector if the array is not null.
3. If the vector is null it simply emits the issues.
next steps:
1. Remove the implementation from form_utils. We will call the methods introduced in this class with a nullptr vector so that issues are directly emitted and there is no behaviour change.
2. Once we create the new devtools event that will receive the array of issues, default to always pushing issues to the array.
notes:
1. I removed the frame from FormIssue because it turns out we do not use it.
Bug: 1399414
Change-Id: Id52ca7ed87755751281f9ba65ea3f62c9569e085
---
M components/BUILD.gn
M components/autofill/content/renderer/BUILD.gn
M components/autofill/content/renderer/autofill_agent.cc
A components/autofill/content/renderer/form_autofill_issues.cc
A components/autofill/content/renderer/form_autofill_issues.h
A components/autofill/content/renderer/form_autofill_issues_browsertest.cc
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/html_based_username_detector_browsertest.cc
M components/autofill/content/renderer/password_autofill_agent.cc
M components/autofill/content/renderer/password_form_conversion_utils.cc
M components/autofill/core/common/form_field_data.h
M third_party/blink/public/web/web_autofill_client.h
M third_party/blink/renderer/core/inspector/inspector_audits_agent.cc
15 files changed, 754 insertions(+), 113 deletions(-)
To view, visit change 4543002. To unsubscribe, or for help writing mail filters, visit settings.
Bruno Braga (Gerrit)
Attention is currently required from: Christoph Schwering, Florian Leimgruber.
28 comments:
Commit Message:
[autofill in devtools] Creates a new class to handle form issues that are emitted to devtools.
next step: Either inside form cache or the agent (TBD) we will call the methods inside form_autofill_issues.h to create the vector of FormIssues. This will only be done in the case where devtools is open only.
notes:
1. I removed the frame from FormIssue because it turns out we do not use it.
2. I made GetAutocompleteAttribute, HasAutocompleteAttribute and GetWebElementsFromIdList public so that they can be reused inside the new class.
2. I refactored some parts of the existing code. With the refactor that came from the initial review, things changes quite a bit. However there were still comments regarding the existing code, I addressed them as well. I can break into another CL if you prefer.
nit: The CL has changed quite a bit since the original description. […]
Done
File components/autofill/content/renderer/form_autofill_issues.h:
e.g. […]
Done
Looks, go/cstyle#function_declarations
Done
emit devtools
// issues for them
Should this rather read "add them to `form_issues`"?
Yeah. Also mentioned that we are emitting in case form_issues is null
Patch Set #20, Line 22:
GetFormIssuesIs this only called in tests?
For now yes. The chained CL will actually use it.
Patch Set #20, Line 24:
std::vector<blink::WebAutofillClient::FormIssue>*Do we need an out parameter here and elsewhere? What does "Get" mean in this context? go/totw/176
It will append form_issues (get them) to the parameter provided, as the documentation states. It is better to have it as a param because we will want to use this class 3 times.
1. For each form in the page.
2. For elements that are not part of a form
3. After extraction to look for incorrect label usages.If we make it return an array we will have to be always merging it to an existing one in the caller.
void GetFormIssues(
const blink::WebFormElement& form_element,
std::vector<blink::WebAutofillClient::FormIssue>* form_issues);
// Same as above, however instead of receiving a form, it received a list of
// control elements that were found outside a form tag.
void GetFormIssuesFromUnownedControlElements(
const std::vector<blink::WebFormControlElement>& unowned_control_elements,
std::vector<blink::WebAutofillClient::FormIssue>* form_issues);
The implementation of `GetFormIssues()` basically just calls `GetFormIssuesFromUnownedControlElement […]
Done
File components/autofill/content/renderer/form_autofill_issues.cc:
// Realistically devtools already dedupes issues. However we make sure not
// to push the same one here for readability.
bool previous_element_not_added =
form_issues->empty() ||
(form_issues->back().issue_type ==
GenericIssueErrorType::kFormDuplicateIdForInputError &&
form_issues->back().violating_node != prev(it)->GetDevToolsNodeId());
if (previous_element_not_added) {
form_issues->emplace_back(
GenericIssueErrorType::kFormDuplicateIdForInputError,
prev(it)->GetDevToolsNodeId(), id_attr);
}
I think it suffices to do this once before the inner for loop?
Done
Patch Set #20, Line 148:
if (!form_issues) {
This is inconsistent across this file. […]
Done
Patch Set #20, Line 228:
DCHECK(!labels.IsNull());
nit: Use CHECK. See [tip of the week](https://g3doc.corp.google. […]
Done
Patch Set #20, Line 234:
!label.CorrespondingControl().IsNull()Since the `label` is one of the parameters, I think we should get rid of this third parameter.
Done
Patch Set #20, Line 272:
WebElementCollection labels = document.GetElementsByHTMLTagName(label_attr);nit: Move this down to where it's actually used.
Done
Patch Set #20, Line 276:
field.assigned_label_source == AssignedLabelSource::kName
I think you should check that the `label_source` is `kFor`. […]
Done
Patch Set #20, Line 294:
// TODO(crbug.com/1339277): Use `root` once the feature is launched.
This comment doesn't make any sense here, `root` is not in the scope :) (and the feature was launche […]
Done
Patch Set #20, Line 301:
kFormLabelForNameErrorkFormLabelForMatchesNonExistingIdError?
No, the phrasing was confusing. It is actually kFormLabelForNameError
File components/autofill/content/renderer/form_autofill_issues_browsertest.cc:
Patch Set #20, Line 26:
bool formIssuesContainIssue(
nit: […]
Done
for (const blink::WebAutofillClient::FormIssue& form_issue : form_issues) {
if (form_issue.issue_type == expected_issue) {
return violating_attr.empty()
? true
: violating_attr == form_issue.violating_node_attribute.Utf8();
}
}
return false;
nit: Use base::ranges::find. Something like this should work […]
Thanks for the suggestion but I do not see how this is making the code more readable? It looks like a different way of writing the same thing with some more boilerplate.
Patch Set #20, Line 45:
WebFormElement WebFormElementFromHTML(const char* html) {
nit: Maybe make ID of the form element a parameter (with default value). […]
Done
"<form id='target'>"
" <input />"
" <label> A label</label>"
"</form>";
Nits for the HTML code here and below: […]
Done
int duplicated_ids_issue_count = 0;
for (const blink::WebAutofillClient::FormIssue& form_issue : form_issues) {
if (form_issue.issue_type ==
GenericIssueErrorType::kFormDuplicateIdForInputError &&
form_issue.violating_node_attribute.Utf8() == "id") {
duplicated_ids_issue_count++;
}
}Simplify using `base::ranges::count()` (similar to my suggestion for `formIssuesContainIssue()`)
Done
std::vector<FormFieldData> fields;
for (size_t i = 0; i < 3; ++i) {
FormFieldData expected;
expected.id_attribute = u"id_" + base::NumberToString16(i);
expected.name_attribute = u"name_" + base::NumberToString16(i);
expected.assigned_label_source = AssignedLabelSource::kName;
fields.push_back(expected);
}Can you declare these as <input>s in the HTML and then extract them from the loaded DOM? Same below.
Done
File components/autofill/content/renderer/form_autofill_util.h:
Patch Set #20, Line 414:
bool HasAutocompleteAttribute(const blink::WebElement& element);
nit: This function has a one line implementation. […]
Done
File components/autofill/content/renderer/form_autofill_util.cc:
for (++it; it != elements.end() &&
std::prev(it)->GetIdAttribute() == it->GetIdAttribute();
++it) {
Ack. Please add a comment stating that DevTools does the dedup for us.
Done
File components/autofill/content/renderer/form_autofill_util.cc:
There's a word missing
Done
File components/autofill/content/renderer/form_autofill_util.cc:
auto it = elements_with_id_attr.begin();
while ((it = base::ranges::adjacent_find(
it, elements_with_id_attr.end(), {},
&WebFormControlElement::GetIdAttribute)) !=
elements_with_id_attr.end()) {
for (++it; it != elements_with_id_attr.end() &&
std::prev(it)->GetIdAttribute() == it->GetIdAttribute();
++it) {
I think you can merge both loops into a single loop like this: […]
Done
Patch Set #20, Line 1586:
Please note that allnit: "All"
Done
go/cstyle#Vertical_Whitespace
Done
while ((it = base::ranges::adjacent_find(
it, elements_with_id_attr.end(), {},
&WebFormControlElement::GetIdAttribute)) !=
elements_with_id_attr.end()) {
for (++it; it != elements_with_id_attr.end() &&
std::prev(it)->GetIdAttribute() == it->GetIdAttribute();
++it) {
// Please note that all elements are pointing to the same document.
// Therefore we can it simply use `it` and not `prev`.
it->GetDocument().GetFrame()->AddGenericIssue(
GenericIssueErrorType::kFormDuplicateIdForInputError,
prev(it)->GetDevToolsNodeId(), id_attr);
it->GetDocument().GetFrame()->AddGenericIssue(
GenericIssueErrorType::kFormDuplicateIdForInputError,
it->GetDevToolsNodeId(), id_attr);
}
}
I think the inner loop is worth eliminating. […]
Done
To view, visit change 4543002. To unsubscribe, or for help writing mail filters, visit settings.
Christoph Schwering (Gerrit)
Attention is currently required from: Bruno Braga, Florian Leimgruber.
2 comments:
Patchset:
Just one comment, didn't review yet.
File components/autofill/content/renderer/form_autofill_issues_browsertest.cc:
for (const blink::WebAutofillClient::FormIssue& form_issue : form_issues) {
if (form_issue.issue_type == expected_issue) {
return violating_attr.empty()
? true
: violating_attr == form_issue.violating_node_attribute.Utf8();
}
}
return false;
Thanks for the suggestion but I do not see how this is making the code more readable? It looks like […]
IMO the short answer is "because it makes the code more declarative".
https://www.youtube.com/watch?v=W2tWOdzgXHA is on this specific topic (though it predates C++20).This particular function is just an instance of `base::ranges::any_of()`:
```
return base::ranges::any_of(form_issues, [&](const auto& form_issue) {
return form_issue.issue_type == expected_issue &&
(violating_attr.empty() ||
violating_attr == form_issue.violating_node_attribute.Utf8());
});
```
To view, visit change 4543002. To unsubscribe, or for help writing mail filters, visit settings.
Bruno Braga (Gerrit)
Attention is currently required from: Christoph Schwering, Florian Leimgruber.
1 comment:
File components/autofill/content/renderer/form_autofill_issues_browsertest.cc:
for (const blink::WebAutofillClient::FormIssue& form_issue : form_issues) {
if (form_issue.issue_type == expected_issue) {
return violating_attr.empty()
? true
: violating_attr == form_issue.violating_node_attribute.Utf8();
}
}
return false;
IMO the short answer is "because it makes the code more declarative". […]
Well, indeed I agree that in this case base::any_of does make it simpler.
To view, visit change 4543002. To unsubscribe, or for help writing mail filters, visit settings.
Florian Leimgruber (Gerrit)
Attention is currently required from: Bruno Braga, Christoph Schwering.
11 comments:
Commit Message:
Patch Set #23, Line 16:
1. Remove the implementation from form_utils. We will call the methods introduced in this class with a nullptr vector so that issues are directly emitted and there is no behaviour change.Remind me, what was the reason for doing this in a separate CL? It seems like it would be easier to remove it immediately. Could you mention that in the description (I remember you told me offline, but unfortunately I forgot. Sorry)
Patchset:
Almost there :)
File components/autofill/content/renderer/form_autofill_issues.h:
This needs to
// be called after label extraction.Can we CHECK that, e.g. using something like this at the beginning of the function?
```
CHECK(base::ranges::all_of(fields, [](const FormFieldData& field) {
return field.label_source != LabelSource::kUnknown;
}));
```
(Or perhaps DCHECK, since this is rather expensive)
File components/autofill/content/renderer/form_autofill_issues.cc:
Patch Set #23, Line 125:
bool current_element_not_added =I think the current element has never been added already. Shouldn't you check for the `std::next()` element?
(form_issues->back().issue_type ==
GenericIssueErrorType::kFormDuplicateIdForInputError &&
form_issues->back().violating_node != it->GetDevToolsNodeId())
Shouldn't this be
```
(form_issues->back().issue_type !=
GenericIssueErrorType::kFormDuplicateIdForInputError ||
form_issues->back().violating_node != it->GetDevToolsNodeId())
```
(i.e. it was not already added if the previous issue wasn't a duplicate ID - or if it was, but for a different field)Given the apparent complexity introduced by this check, and the fact that "Realistically devtools already dedupes issues" - should we just get rid of the check?
if (next(it) != elements_with_id_attr.end() &&
next(it)->GetIdAttribute() == it->GetIdAttribute()) {Like in form_autofill_util.cc, this is guaranteed to be true.
Patch Set #23, Line 216:
const WebString for_attr = GetWebString<kFor>();This is unused
File components/autofill/content/renderer/form_autofill_issues_browsertest.cc:
Patch Set #23, Line 60:
<form id=target>optional nit here and below: Newline, so the <form> open and closing tag have the same indentation?
File components/autofill/content/renderer/form_autofill_util.cc:
Patch Set #23, Line 1544:
// Create copies of |elements| with ids that can be modifiednit: New code should prefer backticks \`\` instead of pipes || - [style guide](https://google.github.io/styleguide/cppguide.html#Function_Comments).
Patch Set #23, Line 1565:
if (next(it) != elements_with_id_attr.end() &&Both conditions of this if are [guaranteed](https://source.chromium.org/chromium/chromium/src/+/main:base/ranges/algorithm.h;l=690-692;drc=6d1cf699abe0ca8158015acc39b77606f327f972) to be true by `adjacent_find()`. That's the point of the function :)
Patch Set #23, Line 1569:
next(it)->GetDevToolsNodeId(), id_attr);(just curious) Why does `next()` compile? I thought specifying the `std` namespace was required.
To view, visit change 4543002. To unsubscribe, or for help writing mail filters, visit settings.
Bruno Braga (Gerrit)
Attention is currently required from: Christoph Schwering, Florian Leimgruber.
12 comments:
Commit Message:
Patch Set #23, Line 16:
1. Remove the implementation from form_utils. We will call the methods introduced in this class with a nullptr vector so that issues are directly emitted and there is no behaviour change.
Remind me, what was the reason for doing this in a separate CL? It seems like it would be easier to […]
Just to keep things separate and make this CL a bit shorter i.e fist move then delete, specially because this CL got quite large. The remove will have to come with an update in form_cache to start using this method, so I prefer to have a specific CL for that. That being said I see your point specially because this CL is keeping form_utils in sync.
File components/autofill/content/renderer/form_autofill_issues.h:
This needs to
// be called after label extraction.
Can we CHECK that, e.g. using something like this at the beginning of the function? […]
Done, please note that I had to update some tests.
also, can label_source really not be unknown tho, is that a guarantee?
File components/autofill/content/renderer/form_autofill_issues.cc:
Patch Set #23, Line 125:
bool current_element_not_added =
I think the current element has never been added already. […]
resolved offline, the current element can be added more than once.
(form_issues->back().issue_type ==
GenericIssueErrorType::kFormDuplicateIdForInputError &&
form_issues->back().violating_node != it->GetDevToolsNodeId())
Shouldn't this be […]
Resolved offline.
the code snipped makes sense, however we are not removing the check to make sure the array itself does not have duplicates. If we allow duplicated here we would have to dedupe inside devtools. I am removing the comment for clarity.
if (next(it) != elements_with_id_attr.end() &&
next(it)->GetIdAttribute() == it->GetIdAttribute()) {Like in form_autofill_util.cc, this is guaranteed to be true.
Done
Patch Set #23, Line 216:
const WebString for_attr = GetWebString<kFor>();This is unused
Done
File components/autofill/content/renderer/form_autofill_issues_browsertest.cc:
for (const blink::WebAutofillClient::FormIssue& form_issue : form_issues) {
if (form_issue.issue_type == expected_issue) {
return violating_attr.empty()
? true
: violating_attr == form_issue.violating_node_attribute.Utf8();
}
}
return false;
Well, indeed I agree that in this case base::any_of does make it simpler.
Thanks for the suggestion.
File components/autofill/content/renderer/form_autofill_issues_browsertest.cc:
Patch Set #23, Line 60:
<form id=target>optional nit here and below: Newline, so the <form> open and closing tag have the same indentation?
Done
File components/autofill/content/renderer/form_autofill_util.cc:
base::flat_map<WebString, int> id_count;
for (const WebFormControlElement& element : control_elements) {
if (IsAutofillableElement(element) && !element.GetIdAttribute().IsEmpty()) {
id_count[element.GetIdAttribute()]++;
}
}
Upps, that makes sense. […]
Done
File components/autofill/content/renderer/form_autofill_util.cc:
Patch Set #23, Line 1544:
// Create copies of |elements| with ids that can be modified
nit: New code should prefer backticks \`\` instead of pipes || - [style guide](https://google. […]
Done
Patch Set #23, Line 1565:
if (next(it) != elements_with_id_attr.end() &&
Both conditions of this if are [guaranteed](https://source.chromium. […]
Done
Patch Set #23, Line 1569:
next(it)->GetDevToolsNodeId(), id_attr);(just curious) Why does `next()` compile? I thought specifying the `std` namespace was required.
this is a fair point. I am not sure either, I can try to find out and update here for learning.
To view, visit change 4543002. To unsubscribe, or for help writing mail filters, visit settings.
Florian Leimgruber (Gerrit)
Attention is currently required from: Bruno Braga, Christoph Schwering.
Patch set 25:Code-Review +1
7 comments:
Patchset:
I haven't taken a super detailed look, but given all the revisions this went through and the fact that it mostly just moves code, it LGTM now.
File components/autofill/content/renderer/form_autofill_issues.h:
This needs to
// be called after label extraction.
Done, please note that I had to update some tests. […]
Oh, you're right - kUnknown actually represents "we didn't find a label". According to [UMA](https://uma.googleplex.com/p/chrome/timeline_v2?sid=c2bdb42178a366bd4a8ee0c078d1cb38), that's the case for about 5% of the fields. Please remove, sorry 😐
File components/autofill/content/renderer/form_autofill_issues.cc:
Patch Set #20, Line 301:
kFormLabelForNameErrorNo, the phrasing was confusing. […]
Acknowledged
File components/autofill/content/renderer/form_autofill_issues.cc:
Patch Set #23, Line 125:
bool current_element_not_added =
resolved offline, the current element can be added more than once.
Acknowledged
(form_issues->back().issue_type ==
GenericIssueErrorType::kFormDuplicateIdForInputError &&
form_issues->back().violating_node != it->GetDevToolsNodeId())
Resolved offline. […]
Acknowledged
File components/autofill/content/renderer/form_autofill_issues_browsertest.cc:
for (const blink::WebAutofillClient::FormIssue& form_issue : form_issues) {
if (form_issue.issue_type == expected_issue) {
return violating_attr.empty()
? true
: violating_attr == form_issue.violating_node_attribute.Utf8();
}
}
return false;
Thanks for the suggestion.
Acknowledged
File components/autofill/content/renderer/form_autofill_util.cc:
Patch Set #25, Line 1567:
Please note that AllRemove "Please note that" or use a lower case a for "All".
Perhaps also note that you're relying on the DevTools-side deduplication here.
To view, visit change 4543002. To unsubscribe, or for help writing mail filters, visit settings.
Findit (Gerrit)
Attention is currently required from: Bruno Braga, Christoph Schwering.
Bruno Braga (Gerrit)
Attention is currently required from: Christoph Schwering, Mike West.
Bruno Braga would like Mike West to review this change.
[autofill in devtools] Creates a new class to handle form issues that are emitted to devtools.
This class has the following behaviour right now:
1. It handles the exact same issues form_utils handles today, therefore does not add any new user visible behaviour.
3. Appends issues to a provided vector if the array is not null.
3. If the vector is null it simply emits the issues.
next steps:
1. Remove the implementation from form_utils. We will call the methods introduced in this class with a nullptr vector so that issues are directly emitted and there is no behaviour change.
2. Once we create the new devtools event that will receive the array of issues, default to always pushing issues to the array.
notes:
1. I removed the frame from FormIssue because it turns out we do not use it.
Bug: 1399414
Change-Id: Id52ca7ed87755751281f9ba65ea3f62c9569e085
---
M components/BUILD.gn
M components/autofill/content/renderer/BUILD.gn
M components/autofill/content/renderer/autofill_agent.cc
A components/autofill/content/renderer/form_autofill_issues.cc
A components/autofill/content/renderer/form_autofill_issues.h
A components/autofill/content/renderer/form_autofill_issues_browsertest.cc
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/html_based_username_detector_browsertest.cc
M components/autofill/content/renderer/password_autofill_agent.cc
M components/autofill/content/renderer/password_form_conversion_utils.cc
M components/autofill/core/common/form_field_data.h
M third_party/blink/public/web/web_autofill_client.h
M third_party/blink/renderer/core/inspector/inspector_audits_agent.cc
15 files changed, 745 insertions(+), 116 deletions(-)
Bruno Braga (Gerrit)
Attention is currently required from: Christoph Schwering, Mike West.
This needs to
// be called after label extraction.
Oh, you're right - kUnknown actually represents "we didn't find a label". […]
Done
File components/autofill/content/renderer/form_autofill_util.cc:
Patch Set #25, Line 1567:
Please note that All
Remove "Please note that" or use a lower case a for "All". […]
Done
To view, visit change 4543002. To unsubscribe, or for help writing mail filters, visit settings.
Bruno Braga (Gerrit)
Attention is currently required from: Christoph Schwering, Danil Somsikov, Mike West.
Bruno Braga would like Danil Somsikov to review this change.
To view, visit change 4543002. To unsubscribe, or for help writing mail filters, visit settings.
Bruno Braga (Gerrit)
Attention is currently required from: Christoph Schwering, Mike West.
Bruno Braga removed Danil Somsikov from this change.
Christoph Schwering (Gerrit)
Attention is currently required from: Bruno Braga, Florian Leimgruber, Mike West.
21 comments:
Commit Message:
Patch Set #26, Line 7:
new classDoes it introduce a new class?
Patch Set #26, Line 11:
1. It handles the exact same issues form_utils handles today, therefore does not add any new user visible behaviour.nit: wrap at 72 as per https://chromium.googlesource.com/chromium/src/+/master/docs/contributing.md#Uploading-a-change-for-review
Patch Set #26, Line 12:
vector if the array is not null.That's hard to understand without knowing the CL.
Edit after the review: I still don't understand it :-). I guess by "array is not null" you mean that the vector isn't nullptr? That's obvious thought because otherwise we'd crash, right? I think a the description should be more high-level to make it more accessible.
Patch Set #26, Line 13:
3. If the vector is null it simply emits the issues.I forgot, what's the difference between the two and what's the reason for the vector again?
IIRC we had some discussions in the context of this CL about minimizing the overhead for users who don't have DevTools open. I don't recall our conclusion but I also don't see this addressed in the CL (perhaps because I don't find where the new code is called). Could you remind me? :-)
Patch Set #26, Line 13:
If the vector is null it simply emits the issuesI can't find this in the CL. The new code from components/autofill/content/renderer/form_autofill_issues.{h,cc} isn't called anywhere yet, is it?
File components/autofill/content/renderer/form_autofill_issues.cc:
Patch Set #26, Line 55:
std::vector<blink::WebAutofillClient::FormIssue>*Taking go/totw/176's "avoid whenever possible" literally, I guess the recommended approach is to take a vector by value or rvalue reference and to return it, and call it with `std::move()` where appropriate.
Patch Set #26, Line 153:
const WebInputElement input_element = element.DynamicTo<WebInputElement>();I meant to comment about the surrounding whitespace w.r.t. go/c-style#Vertical_Whitespace, but the variable actually seems to be unused, so it's just an uncontroversial "Remove" :-)
File components/autofill/content/renderer/form_autofill_util.cc:
Patch Set #2, Line 1615:
static base::NoDestructor<WebString> kName("name");I updated the new class not to clash with your current CL, ptal.
Done
Patch Set #2, Line 1624:
violatingAttrWe can call it violating_attr_value. […]
My point wasn't about "attribute" vs "attribute value". I meant they're not *violating* anything.
The two things that come to my mind what they could be violating are the HTML spec and the autocomplete attribute's grammar. But they're even matching the latter, and there's a good chance they're conforming with the former because all it [says](https://html.spec.whatwg.org/multipage/dom.html#global-attributes:the-id-attribute-2) is that an `id` attribute value must not contain spaces.
File components/autofill/content/renderer/form_autofill_util.cc:
Patch Set #23, Line 1569:
next(it)->GetDevToolsNodeId(), id_attr);
this is a fair point. I am not sure either, I can try to find out and update here for learning.
It works because of [argument-dependent lookup](https://en.cppreference.com/w/cpp/language/adl). It breaks when a `next()` function is defined inside `autofill::form_util` because the unqualified lookup finds that `next()` function then (though that may be intentional as in the `using std::swap; swap(x, y);` pattern.
File components/autofill/content/renderer/form_autofill_util.cc:
Patch Set #26, Line 1552:
form_util::Remove?
// Create copies of `elements` with ids that can be modified
WebVector<WebFormControlElement> elements_with_id_attr;
elements_with_id_attr.reserve(elements.size());
for (const auto& element : elements) {
if (form_util::IsAutofillableElement(element) &&
!element.GetIdAttribute().IsEmpty()) {
elements_with_id_attr.push_back(element);
}
}nit: instead pass `elements` by value and do `base::EraseIf(elements, base::not_fn(&IsAutofillableElement))`?
Patch Set #26, Line 1567:
Please note that allI guess this is personal taste, but I think the short version "All" is more common for such a comment. From a quick look at the codebase we seem to use "please" when we're afraid that somebody would break something (as in "please read the documentation" or "please don't use this function").
Patch Set #26, Line 1618:
violatingAttr`snake_case`
Patch Set #26, Line 1867:
KEEP_WHITESPACEI assume this doesn't have an effect because the separators are whitespace, correct?
typo: labelled_
File components/autofill/core/common/form_field_data.h:
Patch Set #26, Line 57:
x its id/namegrammar
Patch Set #26, Line 59:
practisepractice
nit:
```
`kFor`
```Patch Set #26, Line 344:
When the `label_source` is kForAnd otherwise?
Patch Set #26, Line 346:
assigned_label_sourceAssign a default value (go/totw/146)?
To view, visit change 4543002. To unsubscribe, or for help writing mail filters, visit settings.