[EVP] Verify emails and dispatch "verifiedemail" events in autofill [chromium/src : main]
Sam Goto (Gerrit)
Sam Goto added 2 comments![]( "Open in Gerrit")
void EmailVerifierObserver::OnFillOrPreviewForm(TODO: add unit tests in this CL once I get a signal from reviewers that this is the right code structure.
EmailVerifier* EmailVerifier::GetOrCreateForFrame(TODO: move this to another and independent CL once I get the signal from reviewers that this is the right code structure.
Related details
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Goto (Gerrit)
Sam Goto added 1 comment![]( "Open in Gerrit")
manager->AddObserver(email_verifier_observer_.get());Note to self: we should probably put this behind a flag.
Related details
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jihad Hanna (Gerrit)
Jihad Hanna added 6 comments![]( "Open in Gerrit")
auto manager = std::make_unique<BrowserAutofillManager>(&driver);
manager->AddObserver(email_verifier_observer_.get());
return manager;Check out `components/android_autofill/browser/android_autofill_manager.h` to see how we're initializing observers (with `base::ScopedObservation`)
class EmailVerifierObserver : public AutofillManager::Observer {I think this should be called `EmailVerifierDelegate` (the current name sounds like an interface for observers of `EmailVerifier`).
const AutofillProfile* profile =
*std::get_if<const AutofillProfile*>(&filling_payload);
std::u16string email = profile->GetRawInfo(EMAIL_ADDRESS);
if (email.empty()) {
return;
}I think this shouldn't be necessary, the check right before it and the fact that we filled an email field would imply this, right?
field->autocomplete_attribute() == "email";Are you sure you want this (vs. `field->Type().GetAddressType() == EMAIL_ADDRESS`)
Yours is true only when the developper annotates the field as an email field.
The alternative is true whenever Autofill classifies a field as an email field, and we use many indicators for that, one of them is the HTML `autocomplete` attribute but we have other signals too.
const AutofillField* field_with_nonce = form->GetFieldById(*it);nit: &
EmailVerifier* EmailVerifier::GetOrCreateForFrame(Are you sure you wanna have such a verfier for every frame from the beginning? (vs. just the main frame)
Supporting iframes comes with security risks (the user usually willingly visits the origin of the main frame, but usually doesn't know about the iframes. Automatically verifying the emails there might not be a good idea (just thinking out loud)
Related details
- Sam Goto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Goto (Gerrit)
Sam Goto added 6 comments![]( "Open in Gerrit")
auto manager = std::make_unique<BrowserAutofillManager>(&driver);
manager->AddObserver(email_verifier_observer_.get());
return manager;Check out `components/android_autofill/browser/android_autofill_manager.h` to see how we're initializing observers (with `base::ScopedObservation`)
Yeah, that makes a lot of sense and simplifies a lot (not having to unregister manually).
However, EmailVerifierDelegate is owned by ChromeAutofillClient (1:1 with WebContents), so not 1:1 for every BrowserAutofillManager (which is 1:1 with RendererFrameHost).
I like the idea of making EmailVerifierDelegate use ScopedObservation and be 1:1 with BrowserAutofillManager, but I wasn't sure who should own it.
I considered making EmailVerifierDelegate hang on the RenderFrameHostImpl UserData's structure, but I think that would mean I'd have to move it out of components/autofill.
I found ContentAutofillDriver, which is 1:1 with RenderFrameHost and accessible here in this method, so I made it own a EmailVerifierDelegate, made it a ScopedObservation and made the Observation() happen here.
I wasn't sure if conceptually speaking it made sense for ContentAutofillDriver to own the EmailVerifierDelegate, so I could use some guidance here if this is appropriate or not. Happy to change if you think there is a better place to hang it.
WDYT?
class EmailVerifierObserver : public AutofillManager::Observer {I think this should be called `EmailVerifierDelegate` (the current name sounds like an interface for observers of `EmailVerifier`).
Done
const AutofillProfile* profile =
*std::get_if<const AutofillProfile*>(&filling_payload);
std::u16string email = profile->GetRawInfo(EMAIL_ADDRESS);
if (email.empty()) {
return;
}I think this shouldn't be necessary, the check right before it and the fact that we filled an email field would imply this, right?
Isn't it possible that we filled a "name" WITHOUT an "email"? Or are you saying that the check that "fielled_fields_ids" (and that one of the fields that we filled was an "email" field) is sufficient for this code to know that an EMAIL_ADDRESS was filled?
If that's the case, done, I removed this check here and moved the variable declaration further down when it is used.
Are you sure you want this (vs. `field->Type().GetAddressType() == EMAIL_ADDRESS`)
Yours is true only when the developper annotates the field as an email field.
The alternative is true whenever Autofill classifies a field as an email field, and we use many indicators for that, one of them is the HTML `autocomplete` attribute but we have other signals too.
The alternative is true whenever Autofill classifies a field as an email field, and we use many indicators for that
That seems broader than what I was aiming for, but seems robust and OK too. I'll take that suggestion and see if I can come up with any reason why that might not work later.
Thanks!
const AutofillField* field_with_nonce = form->GetFieldById(*it);Sam Gotonit: &
Done
Are you sure you wanna have such a verfier for every frame from the beginning? (vs. just the main frame)
Supporting iframes comes with security risks (the user usually willingly visits the origin of the main frame, but usually doesn't know about the iframes. Automatically verifying the emails there might not be a good idea (just thinking out loud)
I believe so ... I was planning to add some permission policy to make sure that the main framing was explicitly and manually opting-into this, so that there is no way to abuse this without the main frame knowing about it ... So yeah, would be good to support all frames from the get go.
Related details
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jihad Hanna (Gerrit)
Jihad Hanna added 2 comments![]( "Open in Gerrit")
The static casting is making me think that the architecture isn't optimal.
As I haven't reviewed the addition of new observers before, I'll ask Chris to review this one because I'm not sure what the best path forward is.
auto manager = std::make_unique<BrowserAutofillManager>(&driver);
manager->AddObserver(email_verifier_observer_.get());
return manager;Sam GotoCheck out `components/android_autofill/browser/android_autofill_manager.h` to see how we're initializing observers (with `base::ScopedObservation`)
Yeah, that makes a lot of sense and simplifies a lot (not having to unregister manually).
However, EmailVerifierDelegate is owned by ChromeAutofillClient (1:1 with WebContents), so not 1:1 for every BrowserAutofillManager (which is 1:1 with RendererFrameHost).
I like the idea of making EmailVerifierDelegate use ScopedObservation and be 1:1 with BrowserAutofillManager, but I wasn't sure who should own it.
I considered making EmailVerifierDelegate hang on the RenderFrameHostImpl UserData's structure, but I think that would mean I'd have to move it out of components/autofill.
I found ContentAutofillDriver, which is 1:1 with RenderFrameHost and accessible here in this method, so I made it own a EmailVerifierDelegate, made it a ScopedObservation and made the Observation() happen here.
I wasn't sure if conceptually speaking it made sense for ContentAutofillDriver to own the EmailVerifierDelegate, so I could use some guidance here if this is appropriate or not. Happy to change if you think there is a better place to hang it.
WDYT?
You should only observe the `AutofillManager` of the main frame, as the other ones don't really do anything as they just forward events to the main frame. so it's fine to have this class owned by the client.
Related details
- Christoph Schwering
- Sam Goto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Goto (Gerrit)
Sam Goto added 1 comment![]( "Open in Gerrit")
auto manager = std::make_unique<BrowserAutofillManager>(&driver);
manager->AddObserver(email_verifier_observer_.get());
return manager;Sam GotoCheck out `components/android_autofill/browser/android_autofill_manager.h` to see how we're initializing observers (with `base::ScopedObservation`)
Jihad HannaYeah, that makes a lot of sense and simplifies a lot (not having to unregister manually).
However, EmailVerifierDelegate is owned by ChromeAutofillClient (1:1 with WebContents), so not 1:1 for every BrowserAutofillManager (which is 1:1 with RendererFrameHost).
I like the idea of making EmailVerifierDelegate use ScopedObservation and be 1:1 with BrowserAutofillManager, but I wasn't sure who should own it.
I considered making EmailVerifierDelegate hang on the RenderFrameHostImpl UserData's structure, but I think that would mean I'd have to move it out of components/autofill.
I found ContentAutofillDriver, which is 1:1 with RenderFrameHost and accessible here in this method, so I made it own a EmailVerifierDelegate, made it a ScopedObservation and made the Observation() happen here.
I wasn't sure if conceptually speaking it made sense for ContentAutofillDriver to own the EmailVerifierDelegate, so I could use some guidance here if this is appropriate or not. Happy to change if you think there is a better place to hang it.
WDYT?
You should only observe the `AutofillManager` of the main frame, as the other ones don't really do anything as they just forward events to the main frame. so it's fine to have this class owned by the client.
Ah, that (having ChromeAutofillDriver own Email verifier delegate) makes it much simpler then, because I can make the static cast inside of the content/browser/webid/delegation package in the EmailVerifierImpl, right?
Related details
- Christoph Schwering
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 1 comment![]( "Open in Gerrit")
auto manager = std::make_unique<BrowserAutofillManager>(&driver);
manager->AddObserver(email_verifier_observer_.get());
return manager;Sam GotoCheck out `components/android_autofill/browser/android_autofill_manager.h` to see how we're initializing observers (with `base::ScopedObservation`)
Jihad HannaYeah, that makes a lot of sense and simplifies a lot (not having to unregister manually).
However, EmailVerifierDelegate is owned by ChromeAutofillClient (1:1 with WebContents), so not 1:1 for every BrowserAutofillManager (which is 1:1 with RendererFrameHost).
I like the idea of making EmailVerifierDelegate use ScopedObservation and be 1:1 with BrowserAutofillManager, but I wasn't sure who should own it.
I considered making EmailVerifierDelegate hang on the RenderFrameHostImpl UserData's structure, but I think that would mean I'd have to move it out of components/autofill.
I found ContentAutofillDriver, which is 1:1 with RenderFrameHost and accessible here in this method, so I made it own a EmailVerifierDelegate, made it a ScopedObservation and made the Observation() happen here.
I wasn't sure if conceptually speaking it made sense for ContentAutofillDriver to own the EmailVerifierDelegate, so I could use some guidance here if this is appropriate or not. Happy to change if you think there is a better place to hang it.
WDYT?
Sam GotoYou should only observe the `AutofillManager` of the main frame, as the other ones don't really do anything as they just forward events to the main frame. so it's fine to have this class owned by the client.
Ah, that (having ChromeAutofillDriver own Email verifier delegate) makes it much simpler then, because I can make the static cast inside of the content/browser/webid/delegation package in the EmailVerifierImpl, right?
I'm not sure yet I understand the requirements (haven't spent much time on the CL yet), so perhaps I'm missing the point:
I wasn't sure if conceptually speaking it made sense for ContentAutofillDriver to own the EmailVerifierDelegate, so I could use some guidance here if this is appropriate or not.
Having this in AutofillDriver seems wrong to me. AutofillDrivers only purpose is to only pass messages between AutofillAgents and AutofillManagers. I'd like te keep it that way 😊. For observations, we have `AutofillManager::Observer`.
Also, `CreateManager()` should not call into the driver ([documentation](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/content/browser/content_autofill_client.h;l=37;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18)). `CreateManager()` is called during the construction of `AutofillDriver`, so this is a tricky point in time.
EmailVerifierDelegate is owned by ChromeAutofillClient
Would [`ScopedAutofillManagersObservation`](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/scoped_autofill_managers_observation.h;drc=02fd48f9f8b8035ec6771bb079f87cb276816992) help?
Related details
- Jihad Hanna
- Sam Goto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Goto (Gerrit)
Sam Goto added 1 comment![]( "Open in Gerrit")
Having this in AutofillDriver seems wrong to me. AutofillDrivers only purpose is to only pass messages between AutofillAgents and AutofillManagers. I'd like te keep it that way 😊.
Yeah, that didn't feel right to me either, conceptually speaking.
Would ScopedAutofillManagersObservation help?
Oh, ScopedAutofillManagersObservation might help indeed! It seems like it needs to be one per WebContents, which is great.
Since it is one per WebContents, would it make sense for me to make the ChromeAutofillClient own the EmailVerifierDelegate (one per WebContents too) and make it (the EmailVerifierDelegate) be a ScopedAutofillManagerObservation?
Does that sound right?
Related details
- Christoph Schwering
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Goto (Gerrit)
I took this path and I think it looks much better now.
PTAL?
Interestingly, the OtpFieldDetector (also recently introduced and owned by the ChromeAutofillClient) also uses this pattern here [1].
Related details
- Christoph Schwering
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 12 comments![]( "Open in Gerrit")
I took this path and I think it looks much better now.
I agree, that looks neat 😊
Thanks!
~EmailVerifierDelegate() override;nit: move below other constructors and assignment operators (go/cstyle#Declaration_Order)
// A handler for email verification logic that is triggered from Autofill.Could you add more color what it handles etc.?
if (!std::get_if<const AutofillProfile*>(&filling_payload)) {
return;
}
const AutofillProfile* profile =
*std::get_if<const AutofillProfile*>(&filling_payload);
nit: either only do one `std::get_if()` call or do `std::holds_alternative()` + `std::get()`?
std::find_if(filled_field_ids.begin(), filled_field_ids.end(),nit: `std::ranges::find_if(std::find_if(filled_field_ids, ...`
field->Type().GetAddressType() == EMAIL_ADDRESS;Since this isn't about addresses, this should probably be `GetIdentityCredentialType()`.
auto it =
std::find_if(filled_field_ids.begin(), filled_field_ids.end(),
[&form](const FieldGlobalId& field_id) {
const AutofillField* field = form->GetFieldById(field_id);
return field && !field->nonce().empty() &&
field->Type().GetAddressType() == EMAIL_ADDRESS;
});Since `GetFieldById()` does linear lookup, flip the order?
```
const std::vector<std::unique_ptr<AutofillField>>& fields = form->fields();
auto it = std::ranges::find_if(
fields, [&](const std::unique_ptr<AutofillField>& field) {
return !field->nonce().empty() &&
field->Type().GetIdentityCredentialType()() == EMAIL_ADDRESS &&
filled_field_ids.contains(field->global_id());
});
if (it == form.fields() {
return;
}
```
if (!rfh) {
return;
}`rfh` is guaranteed to be non-null.
base::Unretained(this), manager.GetWeakPtr(),Why is this safe?
Since `OnEmailVerified()` doesn't use any state of `EmailVerifierDelegate`, do you want to make it a lambda or a free function?
std::optional<std::string> verification) {Could you document what this is?
static_cast<BrowserAutofillManager*>(manager.get())
->driver()nitty nit: `&>(*driver).`
EmailVerifier::~EmailVerifier() = default;nit: Move to `content/public/browser/webid/email_verifier.cc`?
Related details
- Jihad Hanna
- Sam Goto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Goto (Gerrit)
Sam Goto added 12 comments![]( "Open in Gerrit")
auto manager = std::make_unique<BrowserAutofillManager>(&driver);
manager->AddObserver(email_verifier_observer_.get());
return manager;Acknowledged
nit: move below other constructors and assignment operators (go/cstyle#Declaration_Order)
Done
// A handler for email verification logic that is triggered from Autofill.Could you add more color what it handles etc.?
Done, added more context.
if (!std::get_if<const AutofillProfile*>(&filling_payload)) {
return;
}
const AutofillProfile* profile =
*std::get_if<const AutofillProfile*>(&filling_payload);
nit: either only do one `std::get_if()` call or do `std::holds_alternative()` + `std::get()`?
Done, picked doing only one `std::get_if()`.
std::find_if(filled_field_ids.begin(), filled_field_ids.end(),nit: `std::ranges::find_if(std::find_if(filled_field_ids, ...`
ah, so much better, thanks, done!
field->Type().GetAddressType() == EMAIL_ADDRESS;Since this isn't about addresses, this should probably be `GetIdentityCredentialType()`.
This is actually about email addresses, and independent from IdentityCredentialType.
Am I missing something?
auto it =
std::find_if(filled_field_ids.begin(), filled_field_ids.end(),
[&form](const FieldGlobalId& field_id) {
const AutofillField* field = form->GetFieldById(field_id);
return field && !field->nonce().empty() &&
field->Type().GetAddressType() == EMAIL_ADDRESS;
});Since `GetFieldById()` does linear lookup, flip the order?
```
const std::vector<std::unique_ptr<AutofillField>>& fields = form->fields();
auto it = std::ranges::find_if(
fields, [&](const std::unique_ptr<AutofillField>& field) {
return !field->nonce().empty() &&
field->Type().GetIdentityCredentialType()() == EMAIL_ADDRESS &&
filled_field_ids.contains(field->global_id());
});
if (it == form.fields() {
return;
}
```
Makes sense. Done.
`rfh` is guaranteed to be non-null.
Acknowledged
base::Unretained(this), manager.GetWeakPtr(),Why is this safe?
Since `OnEmailVerified()` doesn't use any state of `EmailVerifierDelegate`, do you want to make it a lambda or a free function?
Ah, you are right that OnEmailVerified doesn't use any member variable of the EmailVerifierDelegate. I initially moved the callback into a method just to make it more readable, but I agree it if unnecessary.
I made the callback a lambda now, is this what you were looking for?
Could you document what this is?
Done
static_cast<BrowserAutofillManager*>(manager.get())
->driver()Sam Gotonitty nit: `&>(*driver).`
Done
nit: Move to `content/public/browser/webid/email_verifier.cc`?
- Christoph Schwering
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 2 comments![]( "Open in Gerrit")
field->Type().GetAddressType() == EMAIL_ADDRESS;Sam GotoSince this isn't about addresses, this should probably be `GetIdentityCredentialType()`.
This is actually about email addresses, and independent from IdentityCredentialType.
Am I missing something?
Oh, I thought this was related. I should take a look at the design doc 😊
I'm not sure what the right answer here is. There are ~3 options:
- `field->Type().GetTypes().contains(EMAIL_ADDRESS)`
- `field->Type().GetTypes().contains_any({EMAIL_ADDRESS, USERNAME_AND_EMAIL_ADDRESS, EMAIL_OR_LOYALTY_MEMBERSHIP_ID})`
- `field->autofilled_type() == EMAIL_ADDRESS`
I guess the last option is what you want.
Some background to help with the decision:
We introduced those `AutofillType::GetFooType()` as part of making `AutofillType` a union type. For example, a field type can have the types `PASSPORT_NUMBER` and `NATIONAL_ID_CARD_NUMBER` at the same time to express that the field accepts both values. For now this is only used for Autofill AI but we expect to use this also for PWM.
For example, in the long term a field could also have types `USERNAME` and `EMAIL_ADDRESS` at once. Currently that's not possible yet (because of [these constraints](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/autofill_type.h;l=26-31;drc=2468ad347c8302e00208e933d5215c17e84f824b)).
You're not interested in addresses in the sense of physical addresses, right? If that's correct, I'd suggest you do `field->Type().GetTypes().contains(EMAIL_ADDRESS)`. (I know, `Type().GetTypes()` looks strange -- to be fixed soon.)
If false positives are no problem, you may want to also `USERNAME_AND_EMAIL_ADDRESS` and maybe others. (I'm not familiar with the exact semantics of these types.)
But I guess what you really want to know is the **filled** value, right? In that case, `AutofillField::autofilled_type()` should be the right function.
base::Unretained(this), manager.GetWeakPtr(),Sam GotoWhy is this safe?
Since `OnEmailVerified()` doesn't use any state of `EmailVerifierDelegate`, do you want to make it a lambda or a free function?
Ah, you are right that OnEmailVerified doesn't use any member variable of the EmailVerifierDelegate. I initially moved the callback into a method just to make it more readable, but I agree it if unnecessary.
I made the callback a lambda now, is this what you were looking for?
Thanks, that's what I had in mind.
(My point about `base::Unretained(this)` was there's non protection if `this` has died by the time the callback is called.)
Related details
- Jihad Hanna
- Sam Goto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Goto (Gerrit)
Sam Goto added 5 comments![]( "Open in Gerrit")
The static casting is making me think that the architecture isn't optimal.
Sam GotoAs I haven't reviewed the addition of new observers before, I'll ask Chris to review this one because I'm not sure what the best path forward is.
Acknowledged
Note to self: we should probably put this behind a flag.
Done
field->Type().GetAddressType() == EMAIL_ADDRESS;Sam GotoSince this isn't about addresses, this should probably be `GetIdentityCredentialType()`.
Christoph SchweringThis is actually about email addresses, and independent from IdentityCredentialType.
Am I missing something?
Oh, I thought this was related. I should take a look at the design doc 😊
I'm not sure what the right answer here is. There are ~3 options:
- `field->Type().GetTypes().contains(EMAIL_ADDRESS)`
- `field->Type().GetTypes().contains_any({EMAIL_ADDRESS, USERNAME_AND_EMAIL_ADDRESS, EMAIL_OR_LOYALTY_MEMBERSHIP_ID})`
- `field->autofilled_type() == EMAIL_ADDRESS`
I guess the last option is what you want.
Some background to help with the decision:
We introduced those `AutofillType::GetFooType()` as part of making `AutofillType` a union type. For example, a field type can have the types `PASSPORT_NUMBER` and `NATIONAL_ID_CARD_NUMBER` at the same time to express that the field accepts both values. For now this is only used for Autofill AI but we expect to use this also for PWM.
For example, in the long term a field could also have types `USERNAME` and `EMAIL_ADDRESS` at once. Currently that's not possible yet (because of [these constraints](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/autofill_type.h;l=26-31;drc=2468ad347c8302e00208e933d5215c17e84f824b)).
You're not interested in addresses in the sense of physical addresses, right? If that's correct, I'd suggest you do `field->Type().GetTypes().contains(EMAIL_ADDRESS)`. (I know, `Type().GetTypes()` looks strange -- to be fixed soon.)
If false positives are no problem, you may want to also `USERNAME_AND_EMAIL_ADDRESS` and maybe others. (I'm not familiar with the exact semantics of these types.)
But I guess what you really want to know is the **filled** value, right? In that case, `AutofillField::autofilled_type()` should be the right function.
You're not interested in addresses in the sense of physical addresses, right?
Right
But I guess what you really want to know is the filled value, right? In that case, AutofillField::autofilled_type() should be the right function.
Right, I think I want to make sure that the user has selected an email in the autofill UI and **filled** it with an email, **not** what the developer has announced or we classified the field as.
If `autofilled_type()` reflects that, the third option you mentioned `field->autofilled_type() == EMAIL_ADDRESS` seems to be most applicable here.
Done, changed.
void EmailVerifierObserver::OnFillOrPreviewForm(TODO: add unit tests in this CL once I get a signal from reviewers that this is the right code structure.
I'm still working on the unittests, please ignore the ones that I added in this last patch.
EmailVerifier* EmailVerifier::GetOrCreateForFrame(TODO: move this to another and independent CL once I get the signal from reviewers that this is the right code structure.
Christoph, I'd be happy to move this to another CL if you'd like to keep this CL self-contained with the autofill files. LMK if you'd prefer that, but otherwise, I'll just add more reviewers to this CL once you are satisfied with the autofill changes.
Related details
- Christoph Schwering
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jihad Hanna (Gerrit)
Jihad Hanna added 5 comments![]( "Open in Gerrit")
+1 for `field->autofilled_type()`, this way we also support filling that comes from AddressOnTyping (% what I said in the other comment).
const AutofillProfile* const* profile =
std::get_if<const AutofillProfile*>(&filling_payload);
if (!profile) {
return;
}
nit: Move down next to usage?
const std::unique_ptr<AutofillField>& field = *it;`filled_email_field`?
const std::unique_ptr<AutofillField>& field = *it;nit:
`const AutofillField& field = *it->get();`
std::u16string email = (*profile)->GetRawInfo(EMAIL_ADDRESS);Please add a TODO that this isn't how it should be done.
Ideally, you should just do `filled_email_field->value()`, but this currently wouldn't work, because `AutofillManager::Observer::OnFillOrPreviewForm` is unfortunately called before the cache (and hence the `FormStructure` you're using here) is updated.
This means that in reality we cannot yet support AddressOnTyping, which is yet another way to autofill an email address, but where the `filling_payload` isn't an `AutofillProfile`.
---
Apologies is this seems like too much context, TLDR is just to add a TODO so that we remember to fix this when we fix the `OnFillOrPreviewForm` callback.
Related details
- Christoph Schwering
- Sam Goto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jihad Hanna (Gerrit)
Jihad Hanna added 1 comment![]( "Open in Gerrit")
const AutofillProfile* profile =
*std::get_if<const AutofillProfile*>(&filling_payload);
std::u16string email = profile->GetRawInfo(EMAIL_ADDRESS);
if (email.empty()) {
return;
}Sam GotoI think this shouldn't be necessary, the check right before it and the fact that we filled an email field would imply this, right?
Isn't it possible that we filled a "name" WITHOUT an "email"? Or are you saying that the check that "fielled_fields_ids" (and that one of the fields that we filled was an "email" field) is sufficient for this code to know that an EMAIL_ADDRESS was filled?
If that's the case, done, I removed this check here and moved the variable declaration further down when it is used.
Thanks!
Christoph Schwering (Gerrit)
Christoph Schwering added 6 comments![]( "Open in Gerrit")
Thanks, this looks good to me % tests and a few nits.
const AutofillProfile* const* profile =
std::get_if<const AutofillProfile*>(&filling_payload);
if (!profile) {
return;
}
nit: Move down next to usage?
It makes sense to early-return before the form and field lookup, right?
ContentAutofillDriver* content_driver =
static_cast<ContentAutofillDriver*>(&manager.driver());nit:
```
ContentAutofillDriver& content_driver =
static_cast<ContentAutofillDriver&>(manager.driver());
```
nit: We use less vertical whitespace in Autofill. The style guide's phrasing has been weakened lately it's general recommendation is to use it sparingly.
EXPECT_CALL(*email_verifier, Verify(_, _, _, _))nit: Just `Verify`?
EmailVerifier* EmailVerifier::GetOrCreateForFrame(Sam GotoTODO: move this to another and independent CL once I get the signal from reviewers that this is the right code structure.
Christoph, I'd be happy to move this to another CL if you'd like to keep this CL self-contained with the autofill files. LMK if you'd prefer that, but otherwise, I'll just add more reviewers to this CL once you are satisfied with the autofill changes.
- Sam Goto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jihad Hanna (Gerrit)
Jihad Hanna added 1 comment![]( "Open in Gerrit")
const AutofillProfile* const* profile =
std::get_if<const AutofillProfile*>(&filling_payload);
if (!profile) {
return;
}
Christoph Schweringnit: Move down next to usage?
It makes sense to early-return before the form and field lookup, right?
As a small optimization, yes. But since I would like to get rid of the profile completely (see my other comment about the TODO) I thought I'd have all the logic together.
Keeping it here also SGTM, just a suggestion.
Sam Goto (Gerrit)
Sam Goto added 10 comments![]( "Open in Gerrit")
field->Type().GetAddressType() == EMAIL_ADDRESS;SGTM.
I'm going to resolve this, since I"m assuming I got this right. Feel free to re-open if I'm still missing something.
const AutofillProfile* const* profile =
std::get_if<const AutofillProfile*>(&filling_payload);
if (!profile) {
return;
}
Christoph Schweringnit: Move down next to usage?
Jihad HannaIt makes sense to early-return before the form and field lookup, right?
As a small optimization, yes. But since I would like to get rid of the profile completely (see my other comment about the TODO) I thought I'd have all the logic together.
Keeping it here also SGTM, just a suggestion.
Ok, I'll add the TODO and leave this here.
const std::unique_ptr<AutofillField>& field = *it;Sam Goto`filled_email_field`?
Done, used email_field instead to be a bit shorter, LMK if you feel strongly about the "filled" suffix.
nit:
`const AutofillField& field = *it->get();`
Done
ContentAutofillDriver* content_driver =
static_cast<ContentAutofillDriver*>(&manager.driver());nit:
```
ContentAutofillDriver& content_driver =
static_cast<ContentAutofillDriver&>(manager.driver());
```
Done
nit: We use less vertical whitespace in Autofill. The style guide's phrasing has been weakened lately it's general recommendation is to use it sparingly.
Was the extra new line that you wanted me to remove? If so, done. If not, feel free to reopen and clarity.
std::u16string email = (*profile)->GetRawInfo(EMAIL_ADDRESS);Please add a TODO that this isn't how it should be done.
Ideally, you should just do `filled_email_field->value()`, but this currently wouldn't work, because `AutofillManager::Observer::OnFillOrPreviewForm` is unfortunately called before the cache (and hence the `FormStructure` you're using here) is updated.
This means that in reality we cannot yet support AddressOnTyping, which is yet another way to autofill an email address, but where the `filling_payload` isn't an `AutofillProfile`.
---
Apologies is this seems like too much context, TLDR is just to add a TODO so that we remember to fix this when we fix the `OnFillOrPreviewForm` callback.
Done. Text LGTY?
EXPECT_CALL(*email_verifier, Verify(_, _, _, _))Sam Gotonit: Just `Verify`?
Ha, I didn't know you could just use the method's name when using _ parameters! Neat, thanks!
I've actually added expectations here that check that the right values are passed in this specific test, but I've changed to use the simpler version in other tests.
void EmailVerifierObserver::OnFillOrPreviewForm(Sam GotoTODO: add unit tests in this CL once I get a signal from reviewers that this is the right code structure.
Sam GotoI'm still working on the unittests, please ignore the ones that I added in this last patch.
I have added really good unittests now. Resolving.
EmailVerifier* EmailVerifier::GetOrCreateForFrame(Sam GotoTODO: move this to another and independent CL once I get the signal from reviewers that this is the right code structure.
Christoph SchweringChristoph, I'd be happy to move this to another CL if you'd like to keep this CL self-contained with the autofill files. LMK if you'd prefer that, but otherwise, I'll just add more reviewers to this CL once you are satisfied with the autofill changes.
That's fine by me.
I have actually changed some of the constructors of the classes that this relies on to make this class more testable. I think there are enough changes now that I'm probably better off breaking that into a separate CL.
I'll break the extra changes unrelated to the autofill code into another CL momentarily.
Related details
- Christoph Schwering
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jihad Hanna (Gerrit)
Jihad Hanna added 1 comment![]( "Open in Gerrit")
std::u16string email = (*profile)->GetRawInfo(EMAIL_ADDRESS);Sam GotoPlease add a TODO that this isn't how it should be done.
Ideally, you should just do `filled_email_field->value()`, but this currently wouldn't work, because `AutofillManager::Observer::OnFillOrPreviewForm` is unfortunately called before the cache (and hence the `FormStructure` you're using here) is updated.
This means that in reality we cannot yet support AddressOnTyping, which is yet another way to autofill an email address, but where the `filling_payload` isn't an `AutofillProfile`.
---
Apologies is this seems like too much context, TLDR is just to add a TODO so that we remember to fix this when we fix the `OnFillOrPreviewForm` callback.
Done. Text LGTY?
- Christoph Schwering
- Sam Goto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 3 comments![]( "Open in Gerrit")
namespace content {Please fix this WARNING reported by ClangTidy: check: modernize-concat-nested-namespaces
nested namespaces can be concatenated...
check: modernize-concat-nested-namespaces
nested namespaces can be concatenated (https://clang.llvm.org/extra/clang-tidy/checks/modernize/concat-nested-namespaces.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-concat-nested-namespaces` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel` and `linux-clang-tidy-rel`)
TEST_F(EmailVerifierDelegateTest, VerificationTriggered) {Please add a brief (1-2 sentence) comment above each test.
FormData form_data;
FormFieldData field;
field.set_label(u"Email");
field.set_name(u"email");
field.set_form_control_type(FormControlType::kInputEmail);
field.set_nonce(u"test_nonce");
form_data.set_fields({field});Please use test util functions to create the form and field `Form[Field]Data` objects because they also set internal fields. Unfortunately there's a zoo of such functions. Probably the best one for your use-case is the mechanism used [here](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/filling/form_filler_unittest.cc;l=371-386;drc=2468ad347c8302e00208e933d5215c17e84f824b).
Related details
- Sam Goto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Goto (Gerrit)
Sam Goto added 4 comments![]( "Open in Gerrit")
Please fix this WARNING reported by ClangTidy: check: modernize-concat-nested-namespaces
nested namespaces can be concatenated...
check: modernize-concat-nested-namespaces
nested namespaces can be concatenated (https://clang.llvm.org/extra/clang-tidy/checks/modernize/concat-nested-namespaces.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-concat-nested-namespaces` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel` and `linux-clang-tidy-rel`)
Done
std::u16string email = (*profile)->GetRawInfo(EMAIL_ADDRESS);Sam GotoPlease add a TODO that this isn't how it should be done.
Ideally, you should just do `filled_email_field->value()`, but this currently wouldn't work, because `AutofillManager::Observer::OnFillOrPreviewForm` is unfortunately called before the cache (and hence the `FormStructure` you're using here) is updated.
This means that in reality we cannot yet support AddressOnTyping, which is yet another way to autofill an email address, but where the `filling_payload` isn't an `AutofillProfile`.
---
Apologies is this seems like too much context, TLDR is just to add a TODO so that we remember to fix this when we fix the `OnFillOrPreviewForm` callback.
Jihad HannaDone. Text LGTY?
Yes, thank you!
Small nit: `TODO(crbug.com/446288895): `
Done
TEST_F(EmailVerifierDelegateTest, VerificationTriggered) {Please add a brief (1-2 sentence) comment above each test.
Done
FormData form_data;
FormFieldData field;
field.set_label(u"Email");
field.set_name(u"email");
field.set_form_control_type(FormControlType::kInputEmail);
field.set_nonce(u"test_nonce");
form_data.set_fields({field});Please use test util functions to create the form and field `Form[Field]Data` objects because they also set internal fields. Unfortunately there's a zoo of such functions. Probably the best one for your use-case is the mechanism used [here](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/filling/form_filler_unittest.cc;l=371-386;drc=2468ad347c8302e00208e933d5215c17e84f824b).
Thanks! I added test::GetFormData() for one of the tests, but I couldn't switch for all of the tests because I can't set the "nonce" in it yet.
I considered adding setting the "nonce" in GetFormData() but that seemed to increase to scope of this CL more than I should, so I left it as a TODO.
Does that make sense?
Otherwise, I can add support for adding "nonce" in GetFormData() and the FormDescription and rebase this CL in that.
I also considered using the mutable_field() in the FormData to set_nonce(), but that seemed very Passkey-specific.
I added a TDOO against this cover bug to make sure it doesn't fall through the tracks, but if you feel strongly I should address this before hand, I'd be happy to kick off another CL.
Related details
- Christoph Schwering
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Goto (Gerrit)
Sam Goto voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sam Goto
| Commit-Queue | +1 |
1 comment
EmailVerifier* EmailVerifier::GetOrCreateForFrame(Sam GotoTODO: move this to another and independent CL once I get the signal from reviewers that this is the right code structure.
Christoph SchweringChristoph, I'd be happy to move this to another CL if you'd like to keep this CL self-contained with the autofill files. LMK if you'd prefer that, but otherwise, I'll just add more reviewers to this CL once you are satisfied with the autofill changes.
Sam GotoThat's fine by me.
I have actually changed some of the constructors of the classes that this relies on to make this class more testable. I think there are enough changes now that I'm probably better off breaking that into a separate CL.
I'll break the extra changes unrelated to the autofill code into another CL momentarily.
Done, I broke the non-autofill parts of this CL into a separate CL, so that they can be reviewed independently. I rebased this CL on top of the other one.
Resolving this now.
Related details
- Christoph Schwering
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Goto (Gerrit)
Sam Goto added 1 comment![]( "Open in Gerrit")
FormData form_data;
FormFieldData field;
field.set_label(u"Email");
field.set_name(u"email");
field.set_form_control_type(FormControlType::kInputEmail);
field.set_nonce(u"test_nonce");
form_data.set_fields({field});Sam GotoPlease use test util functions to create the form and field `Form[Field]Data` objects because they also set internal fields. Unfortunately there's a zoo of such functions. Probably the best one for your use-case is the mechanism used [here](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/filling/form_filler_unittest.cc;l=371-386;drc=2468ad347c8302e00208e933d5215c17e84f824b).
Thanks! I added test::GetFormData() for one of the tests, but I couldn't switch for all of the tests because I can't set the "nonce" in it yet.
I considered adding setting the "nonce" in GetFormData() but that seemed to increase to scope of this CL more than I should, so I left it as a TODO.
Does that make sense?
Otherwise, I can add support for adding "nonce" in GetFormData() and the FormDescription and rebase this CL in that.
I also considered using the mutable_field() in the FormData to set_nonce(), but that seemed very Passkey-specific.
Sam GotoI added a TDOO against this cover bug to make sure it doesn't fall through the tracks, but if you feel strongly I should address this before hand, I'd be happy to kick off another CL.
Ok, I added the support for test::GetFormData() to set "nonce" in fields and branched from this CL so that it can be reviewed/submitted independently:
https://chromium-review.googlesource.com/c/chromium/src/+/6973355
I'm going mark this as resolved, LMK if you feel strongly about inverting the dependencies (order in which they get submitted), but otherwise I'll assume that this order that I'm suggesting is acceptable.
Related details
- Christoph Schwering
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering voted and added 3 comments![]( "Open in Gerrit")
Votes added by Christoph Schwering
| Code-Review | +1 |
3 comments
// Verifies that the success test case works as expected: the form comforms toPlease fix this WARNING reported by Spellchecker: "comforms" is a possible misspelling of "conforms" or "comforts".
To bypass Spe...
"comforms" is a possible misspelling of "conforms" or "comforts".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
FormData form_data;
FormFieldData field;
field.set_label(u"Email");
field.set_name(u"email");
field.set_form_control_type(FormControlType::kInputEmail);
field.set_nonce(u"test_nonce");
form_data.set_fields({field});Sam GotoPlease use test util functions to create the form and field `Form[Field]Data` objects because they also set internal fields. Unfortunately there's a zoo of such functions. Probably the best one for your use-case is the mechanism used [here](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/filling/form_filler_unittest.cc;l=371-386;drc=2468ad347c8302e00208e933d5215c17e84f824b).
Sam GotoThanks! I added test::GetFormData() for one of the tests, but I couldn't switch for all of the tests because I can't set the "nonce" in it yet.
I considered adding setting the "nonce" in GetFormData() but that seemed to increase to scope of this CL more than I should, so I left it as a TODO.
Does that make sense?
Otherwise, I can add support for adding "nonce" in GetFormData() and the FormDescription and rebase this CL in that.
I also considered using the mutable_field() in the FormData to set_nonce(), but that seemed very Passkey-specific.
I added a TDOO against this cover bug to make sure it doesn't fall through the tracks, but if you feel strongly I should address this before hand, I'd be happy to kick off another CL.
Ok, I added the support for test::GetFormData() to set "nonce" in fields and branched from this CL so that it can be reviewed/submitted independently:
https://chromium-review.googlesource.com/c/chromium/src/+/6973355
I'm going mark this as resolved, LMK if you feel strongly about inverting the dependencies (order in which they get submitted), but otherwise I'll assume that this order that I'm suggesting is acceptable.
- Jihad Hanna
- Sam Goto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Goto (Gerrit)
Sam Goto added 1 comment![]( "Open in Gerrit")
// Verifies that the success test case works as expected: the form comforms toPlease fix this WARNING reported by Spellchecker: "comforms" is a possible misspelling of "conforms" or "comforts".
To bypass Spe...
"comforms" is a possible misspelling of "conforms" or "comforts".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Done
Related details
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Goto (Gerrit)
Sam Goto added 1 comment![]( "Open in Gerrit")
AutofillDriverTestApi(driver_.get())
.SetLifecycleState(AutofillDriver::LifecycleState::kPendingDeletion);Christoph, I had to add this to address tests failing in the CQ with this error [1].
Since this is a mock, the internal lifecycle state isn't set internally, so I had to set it externally before it gets destroyed.
Does this look right to you? Any other way I can/should accomplish this?
[1] [ RUN ] EmailVerifierDelegateTest.FeatureDisabled
[915122:915122:FATAL:components/autofill/core/browser/foundations/autofill_driver.cc:42] Check failed: lifecycle_state_ == LifecycleState::kPendingDeletion (0 vs. 3)
#0 0x5b8b9e7587d2 base::debug::CollectStackTrace()
Related details
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |