Clean up kAutofillIncludeFormElementsInShadowDom. [chromium/src : main]
0 views
Skip to first unread message
Jan Keitel (Gerrit)
Jul 23, 2024, 8:37:04 AM (4 days ago) Jul 23
to AyeAye, Chromium LUCI CQ, chromium...@chromium.org, android-web...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, browser-comp...@chromium.org, gcasto+w...@chromium.org, kinuko...@chromium.org, rouslan+au...@chromium.org, vasilii+watchlis...@chromium.org
Attention needed from Christoph Schwering
Jan Keitel added 1 comment![]( "Open in Gerrit")
File third_party/blink/renderer/core/html/forms/html_form_element_test.cc
Line 143, Patchset 3 (Parent):
EXPECT_EQ(form1->ListedElements(/*include_shadow_trees=*/true),
ListedElement::List{});Jan Keitel . unresolved
This is tested separately for Shadow DOM in tests below.
Related details
Attention is currently required from:
- Christoph Schwering
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
Jul 23, 2024, 9:21:52 AM (4 days ago) Jul 23
to Jan Keitel, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, android-web...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, browser-comp...@chromium.org, gcasto+w...@chromium.org, kinuko...@chromium.org, rouslan+au...@chromium.org, vasilii+watchlis...@chromium.org
Attention needed from Christoph Schwering
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Related details
Attention is currently required from:
- Christoph Schwering
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Jul 23, 2024, 9:27:11 AM (4 days ago) Jul 23
to Jan Keitel, findit...@appspot.gserviceaccount.com, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, android-web...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, browser-comp...@chromium.org, gcasto+w...@chromium.org, kinuko...@chromium.org, rouslan+au...@chromium.org, vasilii+watchlis...@chromium.org
Attention needed from Jan Keitel
Christoph Schwering voted and added 2 comments![]( "Open in Gerrit")
Votes added by Christoph Schwering
| Code-Review | +1 |
2 comments
File third_party/blink/renderer/core/html/forms/html_form_element.cc
Line 799, Patchset 4 (Latest):
// Autofill only considers top level forms - forms that have form
// ancestors are ignored. We therefore include all form control
// descendants of the form for which we collect the listed
// elements. Note that `elements` does not have this problem because itChristoph Schwering . unresolved
I find that hard to understand for two reasons:
- "forms that have form ancestors are ignored"
* Autofill ignores them, but the sentence sounds like the code below ignores them.
* I'd remove this part.
- "We include all form control descendants of the form"
* Include in what?
* I think it should be made explicit this includes descendants that have another form in between.
- Bonus bullet point: "for which we collect the listed elements" --> "whose listed elements we collect"?
File third_party/blink/renderer/core/html/forms/listed_element.cc
Line 63, Patchset 4 (Latest):
ContainerNode* starting_node = insertion_point.ParentOrShadowHostNode();
for (ContainerNode* parent = starting_node; parent;
parent = parent->ParentOrShadowHostNode()) {
if (HTMLFormElement* form = DynamicTo<HTMLFormElement>(parent)) {
form->InvalidateListedElementsIncludingShadowTrees();
}
}Christoph Schwering . unresolved
I refrain from leaving a comment that
```
ContainerNode* parent = &insertion_point;
while ((parent = parent->ParentOrShadowHostNode()) != nullptr) {
...
}
```
is much better because we both know it.
Related details
Attention is currently required from:
- Jan Keitel
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jan Keitel (Gerrit)
Jul 23, 2024, 1:12:27 PM (4 days ago) Jul 23
to findit...@appspot.gserviceaccount.com, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, android-web...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, browser-comp...@chromium.org, gcasto+w...@chromium.org, kinuko...@chromium.org, rouslan+au...@chromium.org, vasilii+watchlis...@chromium.org
Jan Keitel voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jan Keitel
| Commit-Queue | +0 |
2 comments
File third_party/blink/renderer/core/html/forms/html_form_element.cc
Line 799, Patchset 4:
// Autofill only considers top level forms - forms that have form
// ancestors are ignored. We therefore include all form control
// descendants of the form for which we collect the listed
// elements. Note that `elements` does not have this problem because itChristoph Schwering . resolved
I find that hard to understand for two reasons:
- "forms that have form ancestors are ignored"
* Autofill ignores them, but the sentence sounds like the code below ignores them.
* I'd remove this part.
- "We include all form control descendants of the form"
* Include in what?
* I think it should be made explicit this includes descendants that have another form in between.
- Bonus bullet point: "for which we collect the listed elements" --> "whose listed elements we collect"?
Jan Keitel
Done
File third_party/blink/renderer/core/html/forms/listed_element.cc
Line 63, Patchset 4:
ContainerNode* starting_node = insertion_point.ParentOrShadowHostNode();
for (ContainerNode* parent = starting_node; parent;
parent = parent->ParentOrShadowHostNode()) {
if (HTMLFormElement* form = DynamicTo<HTMLFormElement>(parent)) {
form->InvalidateListedElementsIncludingShadowTrees();
}
}Christoph Schwering . resolved
I refrain from leaving a comment that
```
ContainerNode* parent = &insertion_point;
while ((parent = parent->ParentOrShadowHostNode()) != nullptr) {
...
}
```
is much better because we both know it.
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jan Keitel (Gerrit)
Jul 23, 2024, 1:13:04 PM (4 days ago) Jul 23
to Joey Arhar, findit...@appspot.gserviceaccount.com, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, android-web...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, browser-comp...@chromium.org, gcasto+w...@chromium.org, kinuko...@chromium.org, rouslan+au...@chromium.org, vasilii+watchlis...@chromium.org
Attention needed from Joey Arhar
Jan Keitel added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 6 (Latest):
Jan Keitel . resolved
Hi Joey,
Here is another clean-up - would you PTAL?
Thanks,
Jan
Related details
Attention is currently required from:
- Joey Arhar
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
Jul 23, 2024, 1:56:09 PM (4 days ago) Jul 23
to Jan Keitel, Joey Arhar, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, android-web...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, browser-comp...@chromium.org, gcasto+w...@chromium.org, kinuko...@chromium.org, rouslan+au...@chromium.org, vasilii+watchlis...@chromium.org
Attention needed from Joey Arhar
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Related details
Attention is currently required from:
- Joey Arhar
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Jul 23, 2024, 2:44:11 PM (4 days ago) Jul 23
to Jan Keitel, findit...@appspot.gserviceaccount.com, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, android-web...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, browser-comp...@chromium.org, gcasto+w...@chromium.org, kinuko...@chromium.org, rouslan+au...@chromium.org, vasilii+watchlis...@chromium.org
Attention needed from Jan Keitel
Joey Arhar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joey Arhar
| Code-Review | +1 |
1 comment
File third_party/blink/renderer/core/html/forms/html_form_element_test.cc
Line 143, Patchset 3 (Parent):
EXPECT_EQ(form1->ListedElements(/*include_shadow_trees=*/true),
ListedElement::List{});Jan Keitel . resolved
This is tested separately for Shadow DOM in tests below.
Attention is currently required from:
- Jan Keitel
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jan Keitel (Gerrit)
Jul 23, 2024, 3:02:18 PM (4 days ago) Jul 23
to findit...@appspot.gserviceaccount.com, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, android-web...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, browser-comp...@chromium.org, gcasto+w...@chromium.org, kinuko...@chromium.org, rouslan+au...@chromium.org, vasilii+watchlis...@chromium.org
Jan Keitel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jan Keitel
| Commit-Queue | +2 |
1 comment
Patchset-level comments
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Jul 23, 2024, 3:05:39 PM (4 days ago) Jul 23
to Jan Keitel, Joey Arhar, findit...@appspot.gserviceaccount.com, AyeAye, chromium...@chromium.org, android-web...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, browser-comp...@chromium.org, gcasto+w...@chromium.org, kinuko...@chromium.org, rouslan+au...@chromium.org, vasilii+watchlis...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
Clean up kAutofillIncludeFormElementsInShadowDom.
Bug: 40204601
Change-Id: I3a34976407514eacca6d06f9609e7a50ebbb3211
Reviewed-by: Christoph Schwering <schw...@google.com>
Reviewed-by: Joey Arhar <jar...@chromium.org>
Commit-Queue: Jan Keitel <jke...@google.com>
Cr-Commit-Position: refs/heads/main@{#1331915}
Files:
- M android_webview/java/src/org/chromium/android_webview/common/ProductionSupportedFlagList.java
- M chrome/browser/autofill/form_structure_browsertest.cc
- M components/autofill/content/renderer/autofill_agent_browsertest.cc
- M components/autofill/content/renderer/form_autofill_issues.cc
- M components/autofill/content/renderer/form_autofill_util.cc
- M components/autofill/content/renderer/form_autofill_util_browsertest.cc
- M components/autofill/content/renderer/form_cache.cc
- M components/autofill/content/renderer/page_passwords_analyser.cc
- M components/autofill/content/renderer/password_autofill_agent.cc
- M third_party/blink/common/features.cc
- M third_party/blink/renderer/core/html/forms/html_form_element.cc
- M third_party/blink/renderer/core/html/forms/html_form_element_test.cc
- M third_party/blink/renderer/core/html/forms/listed_element.cc
Change size: L
Delta: 13 files changed, 39 insertions(+), 262 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Coverage: +1 by findit...@appspot.gserviceaccount.com
Code-Review: +1 by Joey Arhar, +1 by Christoph Schwering
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages