[Autofill] Trigger email verification on typed email address [chromium/src : main]
0 views
Skip to first unread message
Mohamed Amir Yosef (Gerrit)
Jun 23, 2026, 3:53:41 PM (yesterday) Jun 23
to android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, gcasto+w...@chromium.org, vasilii+watchlis...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, browser-comp...@chromium.org, ipc-securi...@chromium.org
Attention needed from Christoph Schwering
Mohamed Amir Yosef added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 4 (Latest):
Mohamed Amir Yosef . resolved
Hi Chris,
Could you please take a look?
Thank you,
Mohamed
Related details
Attention is currently required from:
- Christoph Schwering
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
6:09 AM (17 hours ago) 6:09 AM
to Mohamed Amir Yosef, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, gcasto+w...@chromium.org, vasilii+watchlis...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, browser-comp...@chromium.org, ipc-securi...@chromium.org
Attention needed from Mohamed Amir Yosef
Christoph Schwering added 3 comments![]( "Open in Gerrit")
File components/autofill/content/renderer/autofill_agent.h
Line 617, Patchset 4 (Latest):
// email verification values per field, preventing IPC spam on alternating
// focus.Christoph Schwering . unresolved
We do that anyway with `FocusOn[Non]FormField()` already, don't we?
File components/autofill/content/renderer/autofill_agent.cc
Line 970, Patchset 4 (Latest):
autofill_driver->StartEmailVerification(form, field_id, value);Christoph Schwering . unresolved
Can we instead
- retrofit DidEndTextFieldEditing() with a `FormData` + `FieldGlobalId`
- parse the form
- retrieve the `AutofillField` for that `FieldGlobalId`
- look at the `AutofillField::value()` and `AutofillField::last_modifier()`
?
That'd avoid a specialized event and makes an existing event more useful and keep EVP-specific logic out of the renderer and out of `AutofillAgent` in particular (if it has to live in the renderer, it should probably go in the helper class).
File components/autofill/core/browser/foundations/autofill_manager.cc
Line 283, Patchset 4 (Latest):
const FormData& form,Christoph Schwering . unresolved
The form should likely be parsed. (Mostly note to self.)
Related details
Attention is currently required from:
- Mohamed Amir Yosef
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohamed Amir Yosef (Gerrit)
9:52 AM (13 hours ago) 9:52 AM
to android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, gcasto+w...@chromium.org, vasilii+watchlis...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, browser-comp...@chromium.org, ipc-securi...@chromium.org
Attention needed from Christoph Schwering
Mohamed Amir Yosef added 5 comments![]( "Open in Gerrit")
Patchset-level comments
Mohamed Amir Yosef . resolved
Thanks a lot Chris!
Could you please take another look?
File components/autofill/content/browser/integrators/email_verifier/email_verifier_delegate_unittest.cc
Line 948, Patchset 6:
// Flush the message loop to ensure nothing was posted.
base::RunLoop run_loop;
base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(
FROM_HERE, run_loop.QuitClosure());
run_loop.Run();File components/autofill/content/renderer/autofill_agent.h
Line 617, Patchset 4:
// email verification values per field, preventing IPC spam on alternating
// focus.Christoph Schwering . resolved
We do that anyway with `FocusOn[Non]FormField()` already, don't we?
Mohamed Amir Yosef
Acknowledged
File components/autofill/content/renderer/autofill_agent.cc
Line 970, Patchset 4:
autofill_driver->StartEmailVerification(form, field_id, value);Christoph Schwering . resolved
Can we instead
- retrofit DidEndTextFieldEditing() with a `FormData` + `FieldGlobalId`
- parse the form
- retrieve the `AutofillField` for that `FieldGlobalId`
- look at the `AutofillField::value()` and `AutofillField::last_modifier()`
?
That'd avoid a specialized event and makes an existing event more useful and keep EVP-specific logic out of the renderer and out of `AutofillAgent` in particular (if it has to live in the renderer, it should probably go in the helper class).
Mohamed Amir Yosef
Tried a slightly different approach that was discussed over chat!
File components/autofill/core/browser/foundations/autofill_manager.cc
The form should likely be parsed. (Mostly note to self.)
Attention is currently required from:
- Christoph Schwering
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Biesinger (Gerrit)
5:48 PM (5 hours ago) 5:48 PM
to Mohamed Amir Yosef, Christian Biesinger, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, gcasto+w...@chromium.org, vasilii+watchlis...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, browser-comp...@chromium.org, ipc-securi...@chromium.org
Attention needed from Christoph Schwering and Mohamed Amir Yosef
Christian Biesinger added 1 comment![]( "Open in Gerrit")
File components/autofill/content/browser/integrators/email_verifier/email_verifier_delegate.cc
Line 401, Patchset 9 (Latest):
void EmailVerifierDelegate::OnFieldLostFocus(AutofillManager& manager,Christian Biesinger . unresolved
Is this better than using something based on AutofillManager::OnDidEndTextFieldEditing?
Related details
Attention is currently required from:
- Christoph Schwering
- Mohamed Amir Yosef
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
9:32 PM (2 hours ago) 9:32 PM
to Mohamed Amir Yosef, Christian Biesinger, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, gcasto+w...@chromium.org, vasilii+watchlis...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, browser-comp...@chromium.org, ipc-securi...@chromium.org
Attention needed from Mohamed Amir Yosef
Christoph Schwering added 3 comments![]( "Open in Gerrit")
File components/autofill/content/browser/integrators/email_verifier/email_verifier_delegate.h
Line 160, Patchset 9 (Latest):
void OnFieldLostFocus(AutofillManager& manager,
const FieldGlobalId& field_id);
Christoph Schwering . unresolved
Member functions go above member variables 😜
File components/autofill/content/browser/integrators/email_verifier/email_verifier_delegate.cc
Line 427, Patchset 9 (Latest):
if (value.empty() || value.length() > 256 ||Christoph Schwering . unresolved
Can a reasonable email address be shorter than `x@y.z` or at least `x@y`? I.e., check `value.size() <= 3` or `value.size() <= 5`?
Line 428, Patchset 9 (Latest):
value.find(u'@') == std::u16string::npos) {Christoph Schwering . unresolved
Should this also require a domain (i.e., a `u'.'` after the `u'@'`)?
Related details
Attention is currently required from:
- Mohamed Amir Yosef
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages