Centralize AutofillAI authentication and server-side fetching logic. [chromium/src : main]
Karol Sygiet (Gerrit)
Karol Sygiet added 1 comment![]( "Open in Gerrit")
Related details
- Florian Leimgruber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Florian Leimgruber (Gerrit)
Florian Leimgruber added 1 comment![]( "Open in Gerrit")
MaybeAuthenticateAndFetchForFilling(Can we split this into two function, one that authenticates and one that does fetching? I feel like doing two things in one function creates quite a overloaded interface with three callback parameter.
Especially the `show_loading_state_callback` feels really weird.
Related details
- Karol Sygiet
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. |
Karol Sygiet (Gerrit)
Karol Sygiet added 1 comment![]( "Open in Gerrit")
Can we split this into two function, one that authenticates and one that does fetching? I feel like doing two things in one function creates quite a overloaded interface with three callback parameter.
Especially the `show_loading_state_callback` feels really weird.
- Florian Leimgruber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Karol Sygiet (Gerrit)
Karol Sygiet added 1 comment![]( "Open in Gerrit")
Hi Jihad, Since florian is OOO, could you take a look instead?
Related details
- Florian Leimgruber
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jihad Hanna (Gerrit)
Jihad Hanna added 10 comments![]( "Open in Gerrit")
const Suggestion& suggestion,super nit: Move up, as usually we leave bools and enums in the end.
CreateAutofillAiReauthConverter(const FieldTypeSet& ai_field_types,document
return auth_succeeded ? std::move(entity)
: std::optional<EntityInstance>();optional nit:
```
return auth_succeeded ? std::optional<EntityInstance>(std::move(entity))
: std::nullopt;
```
seemed more intuitive to me.
std::unique_ptr<device_reauth::DeviceAuthenticator> StartAutofillAiReauth(document
#if BUILDFLAG(IS_MAC) || BUILDFLAG(IS_WIN) || BUILDFLAG(IS_CHROMEOS) || \
BUILDFLAG(IS_IOS)
const std::u16string origin = base::UTF8ToUTF16(field.origin().host());
message = l10n_util::GetStringFUTF16(IDS_AUTOFILL_AI_FILLING_REAUTH, origin);
#endifCould you document why this is different on android?
std::move(callback).Run(/*auth_succeeded=*/true);Since authentication didn't really succeed, add a comment explaining why we're passing true.
return base::BindOnce(
[](base::WeakPtr<AutofillClient> client,
base::OnceCallback<void(std::optional<EntityInstance>)>
fill_form_callback,
std::optional<EntityInstance> masked_entity) {
if (!masked_entity || !client ||
!client->GetWalletPassAccessManager()) {
std::move(fill_form_callback).Run(std::nullopt);
return;
}
auto maybe_notify_of_unmasking_error = base::BindOnce(
[](base::WeakPtr<AutofillClient> client,
std::optional<EntityInstance> entity) {
if (client && !entity) {
client->ShowAutofillAiFetchFromWalletFailureNotification();
}
return entity;
},
client);
client->GetWalletPassAccessManager()->GetUnmaskedWalletEntityInstance(
masked_entity->guid(), std::move(maybe_notify_of_unmasking_error)
.Then(std::move(fill_form_callback)));
},
client.GetWeakPtr(), std::move(fill_form_callback));What do you think about creating two lambdas `maybe_notify_of_unmasking_error` and `fetch_callback` before `return base::BindOnce(...)` so that the code is more readable?
if (!entity || !autofill_field) {`|| !form_structure`? We shouldn't implicitly assume that one implies the other.
Related details
- Florian Leimgruber
- Karol Sygiet
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. |
Karol Sygiet (Gerrit)
Karol Sygiet added 10 comments![]( "Open in Gerrit")
super nit: Move up, as usually we leave bools and enums in the end.
Done
AutofillClient& client,const?
In the implementation `StartAutofillAiReauth` calls `client.GetDeviceAuthenticator(` which is not const.
AutofillClient& client,const?
This function uses WeakPtr and IIUC typically we don't want const `GetWeakPtr` methods
CreateAutofillAiReauthConverter(const FieldTypeSet& ai_field_types,Karol Sygietdocument
Done
return auth_succeeded ? std::move(entity)
: std::optional<EntityInstance>();optional nit:
```
return auth_succeeded ? std::optional<EntityInstance>(std::move(entity))
: std::nullopt;
```seemed more intuitive to me.
Done
std::unique_ptr<device_reauth::DeviceAuthenticator> StartAutofillAiReauth(Karol Sygietdocument
Done
#if BUILDFLAG(IS_MAC) || BUILDFLAG(IS_WIN) || BUILDFLAG(IS_CHROMEOS) || \
BUILDFLAG(IS_IOS)
const std::u16string origin = base::UTF8ToUTF16(field.origin().host());
message = l10n_util::GetStringFUTF16(IDS_AUTOFILL_AI_FILLING_REAUTH, origin);
#endifCould you document why this is different on android?
Done
std::move(callback).Run(/*auth_succeeded=*/true);Since authentication didn't really succeed, add a comment explaining why we're passing true.
Added. I think this was the state of things before this CL, added a todo to evaluate whether it is the correct behavior.
return base::BindOnce(
[](base::WeakPtr<AutofillClient> client,
base::OnceCallback<void(std::optional<EntityInstance>)>
fill_form_callback,
std::optional<EntityInstance> masked_entity) {
if (!masked_entity || !client ||
!client->GetWalletPassAccessManager()) {
std::move(fill_form_callback).Run(std::nullopt);
return;
}
auto maybe_notify_of_unmasking_error = base::BindOnce(
[](base::WeakPtr<AutofillClient> client,
std::optional<EntityInstance> entity) {
if (client && !entity) {
client->ShowAutofillAiFetchFromWalletFailureNotification();
}
return entity;
},
client);
client->GetWalletPassAccessManager()->GetUnmaskedWalletEntityInstance(
masked_entity->guid(), std::move(maybe_notify_of_unmasking_error)
.Then(std::move(fill_form_callback)));
},
client.GetWeakPtr(), std::move(fill_form_callback));What do you think about creating two lambdas `maybe_notify_of_unmasking_error` and `fetch_callback` before `return base::BindOnce(...)` so that the code is more readable?
Let me know if it looks better now :)
`|| !form_structure`? We shouldn't implicitly assume that one implies the other.
- Florian Leimgruber
- Jihad Hanna
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. |
Jihad Hanna (Gerrit)
Jihad Hanna added 7 comments![]( "Open in Gerrit")
AutofillClient& client,Karol Sygietconst?
In the implementation `StartAutofillAiReauth` calls `client.GetDeviceAuthenticator(` which is not const.
That's not [what I see](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/autofill_client.h;l=665-668;drc=868ed9b7d285b249a3b6dddde4f5fb0f80f608db). Am I missing something?
AutofillClient& client,Karol Sygietconst?
This function uses WeakPtr and IIUC typically we don't want const `GetWeakPtr` methods
Oh that is true, my bad 👍
std::move(callback).Run(/*auth_succeeded=*/true);Karol SygietSince authentication didn't really succeed, add a comment explaining why we're passing true.
Added. I think this was the state of things before this CL, added a todo to evaluate whether it is the correct behavior.
Acknowledged
auto maybe_notify_of_unmasking_error = base::BindOnce(
[](base::WeakPtr<AutofillClient> client,
std::optional<EntityInstance> entity) {
if (client && !entity) {
client->ShowAutofillAiFetchFromWalletFailureNotification();
}
return entity;
},
client.GetWeakPtr());Since this does not mutate the input, I believe the `.Then()` below is just added complexity.
If we make this:
```
// Notifies the client in case of fetch failure, and calls `fill_form_callback`.
auto unmasking_callback = base::BindOnce(
[](base::WeakPtr<AutofillClient> client,
base::OnceCallback<void(std::optional<EntityInstance>)>
fill_form_callback,
std::optional<EntityInstance> entity) {
if (client && !entity) {
client->ShowAutofillAiFetchFromWalletFailureNotification();
}
std::move(fill_form_callback).Run(entity);
},
client.GetWeakPtr(), std::move(fill_form_callback));
```
And then we could remove one of the callbacks from the parameters of the other one.
(If this seems useless to you, feel free to close this comment, I'm just finding the second callback a little cumbersome and this way felt a little easier to parse to me),
return base::BindOnce(
[](base::WeakPtr<AutofillClient> client,
base::OnceCallback<void(std::optional<EntityInstance>)>
fill_form_callback,
std::optional<EntityInstance> masked_entity) {
if (!masked_entity || !client ||
!client->GetWalletPassAccessManager()) {
std::move(fill_form_callback).Run(std::nullopt);
return;
}
auto maybe_notify_of_unmasking_error = base::BindOnce(
[](base::WeakPtr<AutofillClient> client,
std::optional<EntityInstance> entity) {
if (client && !entity) {
client->ShowAutofillAiFetchFromWalletFailureNotification();
}
return entity;
},
client);
client->GetWalletPassAccessManager()->GetUnmaskedWalletEntityInstance(
masked_entity->guid(), std::move(maybe_notify_of_unmasking_error)
.Then(std::move(fill_form_callback)));
},
client.GetWeakPtr(), std::move(fill_form_callback));Karol SygietWhat do you think about creating two lambdas `maybe_notify_of_unmasking_error` and `fetch_callback` before `return base::BindOnce(...)` so that the code is more readable?
Let me know if it looks better now :)
Yes thank you 👍
convert_auth_response = CreateAutofillAiReauthConverter(Optional nit: I'd inline this.
if (should_fetch_from_server) {
fill_and_hide =
CreateAutofillAiFetchingCallback(client, std::move(fill_and_hide));
}We pass both `should_fetch_from_server` and `fill_and_hide` to `HandleAutofillAiReauthAndLoadingState()` below, can we move this to the function, and move `CreateAutofillAiFetchingCallback()` to the anonymous namespace?
Related details
- Florian Leimgruber
- Karol Sygiet
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. |
Karol Sygiet (Gerrit)
Karol Sygiet added 4 comments![]( "Open in Gerrit")
AutofillClient& client,Karol Sygietconst?
Jihad HannaIn the implementation `StartAutofillAiReauth` calls `client.GetDeviceAuthenticator(` which is not const.
That's not [what I see](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/autofill_client.h;l=665-668;drc=868ed9b7d285b249a3b6dddde4f5fb0f80f608db). Am I missing something?
Ah right, but still I'm not sure it will work since it now the non-const CreateAutofillAiFetchingCallback is called by this function.
auto maybe_notify_of_unmasking_error = base::BindOnce(
[](base::WeakPtr<AutofillClient> client,
std::optional<EntityInstance> entity) {
if (client && !entity) {
client->ShowAutofillAiFetchFromWalletFailureNotification();
}
return entity;
},
client.GetWeakPtr());Since this does not mutate the input, I believe the `.Then()` below is just added complexity.
If we make this:
```
// Notifies the client in case of fetch failure, and calls `fill_form_callback`.
auto unmasking_callback = base::BindOnce(
[](base::WeakPtr<AutofillClient> client,
base::OnceCallback<void(std::optional<EntityInstance>)>
fill_form_callback,
std::optional<EntityInstance> entity) {
if (client && !entity) {
client->ShowAutofillAiFetchFromWalletFailureNotification();
}
std::move(fill_form_callback).Run(entity);
},
client.GetWeakPtr(), std::move(fill_form_callback));
```And then we could remove one of the callbacks from the parameters of the other one.
(If this seems useless to you, feel free to close this comment, I'm just finding the second callback a little cumbersome and this way felt a little easier to parse to me),
Done. PTAL if it looks better to you now
convert_auth_response = CreateAutofillAiReauthConverter(Optional nit: I'd inline this.
Done
if (should_fetch_from_server) {
fill_and_hide =
CreateAutofillAiFetchingCallback(client, std::move(fill_and_hide));
}We pass both `should_fetch_from_server` and `fill_and_hide` to `HandleAutofillAiReauthAndLoadingState()` below, can we move this to the function, and move `CreateAutofillAiFetchingCallback()` to the anonymous namespace?
- Florian Leimgruber
- Jihad Hanna
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. |
Jihad Hanna (Gerrit)
Jihad Hanna added 5 comments![]( "Open in Gerrit")
base::OnceCallback<void(std::optional<EntityInstance>)> fill_form_callback,Can we call this `fill_form_and_hide_popup`?
base::OnceCallback<void(std::optional<EntityInstance>)> fill_and_hide =`fill_form_and_hide_popup`
.Then(base::BindOnce(&AutofillClient::HideAutofillSuggestions,
client.GetWeakPtr(),
SuggestionHidingReason::kAcceptSuggestion));Just call the function in the lambda?
const bool should_fetch_from_server =
suggestion.GetPayload<Suggestion::AutofillAiPayload>()
.requires_server_fetch;This can be moved to the function as well.
auto show_loading_suggestions_callback = base::BindOnce(
[](base::WeakPtr<AutofillExternalDelegate> delegate,
std::vector<Suggestion> suggestions) {
if (delegate) {
delegate->AttemptToDisplayAutofillSuggestions(
std::move(suggestions), delegate->trigger_source_,
/*is_update=*/true, AutofillSuggestionsIgnoreFocusLoss(true));
}
},
GetWeakPtr());
This makes it feel like this will happen somehow asynchronously, whereas it doesn't.
Can the newly created function just be a private method in AED instead, so that we don't have to define this here altogether?
Related details
- Karol Sygiet
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. |
Karol Sygiet (Gerrit)
Karol Sygiet added 5 comments![]( "Open in Gerrit")
base::OnceCallback<void(std::optional<EntityInstance>)> fill_form_callback,Can we call this `fill_form_and_hide_popup`?
Done
base::OnceCallback<void(std::optional<EntityInstance>)> fill_and_hide =Karol Sygiet`fill_form_and_hide_popup`
Done
.Then(base::BindOnce(&AutofillClient::HideAutofillSuggestions,
client.GetWeakPtr(),
SuggestionHidingReason::kAcceptSuggestion));Just call the function in the lambda?
Done
const bool should_fetch_from_server =
suggestion.GetPayload<Suggestion::AutofillAiPayload>()
.requires_server_fetch;This can be moved to the function as well.
Done
auto show_loading_suggestions_callback = base::BindOnce(
[](base::WeakPtr<AutofillExternalDelegate> delegate,
std::vector<Suggestion> suggestions) {
if (delegate) {
delegate->AttemptToDisplayAutofillSuggestions(
std::move(suggestions), delegate->trigger_source_,
/*is_update=*/true, AutofillSuggestionsIgnoreFocusLoss(true));
}
},
GetWeakPtr());
This makes it feel like this will happen somehow asynchronously, whereas it doesn't.
Can the newly created function just be a private method in AED instead, so that we don't have to define this here altogether?
Made it a `base::FunctionRef` instead, does it look better?
Related details
- Jihad Hanna
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. |
Jihad Hanna (Gerrit)
Jihad Hanna added 3 comments![]( "Open in Gerrit")
AutofillClient& client,Karol Sygietconst?
Jihad HannaIn the implementation `StartAutofillAiReauth` calls `client.GetDeviceAuthenticator(` which is not const.
Karol SygietThat's not [what I see](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/autofill_client.h;l=665-668;drc=868ed9b7d285b249a3b6dddde4f5fb0f80f608db). Am I missing something?
Ah right, but still I'm not sure it will work since it now the non-const CreateAutofillAiFetchingCallback is called by this function.
Acknowledged
auto maybe_notify_of_unmasking_error = base::BindOnce(
[](base::WeakPtr<AutofillClient> client,
std::optional<EntityInstance> entity) {
if (client && !entity) {
client->ShowAutofillAiFetchFromWalletFailureNotification();
}
return entity;
},
client.GetWeakPtr());Since this does not mutate the input, I believe the `.Then()` below is just added complexity.
If we make this:
```
// Notifies the client in case of fetch failure, and calls `fill_form_callback`.
auto unmasking_callback = base::BindOnce(
[](base::WeakPtr<AutofillClient> client,
base::OnceCallback<void(std::optional<EntityInstance>)>
fill_form_callback,
std::optional<EntityInstance> entity) {
if (client && !entity) {
client->ShowAutofillAiFetchFromWalletFailureNotification();
}
std::move(fill_form_callback).Run(entity);
},
client.GetWeakPtr(), std::move(fill_form_callback));
```And then we could remove one of the callbacks from the parameters of the other one.
(If this seems useless to you, feel free to close this comment, I'm just finding the second callback a little cumbersome and this way felt a little easier to parse to me),
Done. PTAL if it looks better to you now
Acknowledged
auto show_loading_suggestions_callback = base::BindOnce(
[](base::WeakPtr<AutofillExternalDelegate> delegate,
std::vector<Suggestion> suggestions) {
if (delegate) {
delegate->AttemptToDisplayAutofillSuggestions(
std::move(suggestions), delegate->trigger_source_,
/*is_update=*/true, AutofillSuggestionsIgnoreFocusLoss(true));
}
},
GetWeakPtr());
Karol SygietThis makes it feel like this will happen somehow asynchronously, whereas it doesn't.
Can the newly created function just be a private method in AED instead, so that we don't have to define this here altogether?
Made it a `base::FunctionRef` instead, does it look better?
Yes 👍
Related details
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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")
auto unmasking_callback = base::BindOnce(
[](base::WeakPtr<AutofillClient> client,
base::OnceCallback<void(std::optional<EntityInstance>)>
fill_form_and_hide_popup,
std::optional<EntityInstance> entity) {
if (client && !entity) {
client->ShowAutofillAiFetchFromWalletFailureNotification();
}
std::move(fill_form_and_hide_popup).Run(entity);
},
client, std::move(fill_form_and_hide_popup));We agreed on moving this out of the other lambda
if ((should_fetch_from_server || should_reauth) &&
base::FeatureList::IsEnabled(features::kAutofillAiWalletPrivatePasses)) {
show_loading_suggestions_callback(PrepareLoadingStateSuggestions(
base::ToVector(client.GetAutofillSuggestions()), suggestion));
}WDYT about calling this regardless whether `(should_fetch_from_server || should_reauth)` is true or false? I imagine that otherwise it wouldn't even show to the user because it would be very fast. We could then call it directly in AED and not pass it at all to this function, making things simpler.
Related details
- Karol Sygiet
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. |
Florian Leimgruber (Gerrit)
Florian Leimgruber added 1 comment![]( "Open in Gerrit")
MaybeAuthenticateAndFetchForFilling(Karol SygietCan we split this into two function, one that authenticates and one that does fetching? I feel like doing two things in one function creates quite a overloaded interface with three callback parameter.
Especially the `show_loading_state_callback` feels really weird.
Done
Drive-by: Why did we end up with a single function again? :)
Jihad Hanna (Gerrit)
Jihad Hanna added 1 comment![]( "Open in Gerrit")
MaybeAuthenticateAndFetchForFilling(Karol SygietCan we split this into two function, one that authenticates and one that does fetching? I feel like doing two things in one function creates quite a overloaded interface with three callback parameter.
Especially the `show_loading_state_callback` feels really weird.
Florian LeimgruberDone
Drive-by: Why did we end up with a single function again? :)
I believe ideally we have:
- `StartFillingProcess()` --> Pretty much does nothing besides calling the next function.
- `MaybeAuthenticateForFilling()` --> takes the callback, possibly augments it for authentication and then either directly call or authenticate and call the next function.
- `MaybeFetchForFilling()` --> takes the callback, possibly augments it for fetching and then either directly call or fetch and call `fill_form_and_hide_popup`.
Karol Sygiet (Gerrit)
Karol Sygiet added 3 comments![]( "Open in Gerrit")
MaybeAuthenticateAndFetchForFilling(Karol SygietCan we split this into two function, one that authenticates and one that does fetching? I feel like doing two things in one function creates quite a overloaded interface with three callback parameter.
Especially the `show_loading_state_callback` feels really weird.
Florian LeimgruberDone
Jihad HannaDrive-by: Why did we end up with a single function again? :)
I believe ideally we have:
- `StartFillingProcess()` --> Pretty much does nothing besides calling the next function.
- `MaybeAuthenticateForFilling()` --> takes the callback, possibly augments it for authentication and then either directly call or authenticate and call the next function.
- `MaybeFetchForFilling()` --> takes the callback, possibly augments it for fetching and then either directly call or fetch and call `fill_form_and_hide_popup`.
Yeah, I first did what you've suggested Florian (or at least tried) and then changed the approach.
Jihad, could you take a look whether it looks more like your proposal now?
auto unmasking_callback = base::BindOnce(
[](base::WeakPtr<AutofillClient> client,
base::OnceCallback<void(std::optional<EntityInstance>)>
fill_form_and_hide_popup,
std::optional<EntityInstance> entity) {
if (client && !entity) {
client->ShowAutofillAiFetchFromWalletFailureNotification();
}
std::move(fill_form_and_hide_popup).Run(entity);
},
client, std::move(fill_form_and_hide_popup));We agreed on moving this out of the other lambda
I tired moving this out of lambda but caused problems and test failures, as this requires moving the `fill_form_and_hide_popup` which the return callback may need on its own.
I've tried designing this better with Gemini but it believes this is the cleanest approach. LMK if you have some idea to move it out of here.
if ((should_fetch_from_server || should_reauth) &&
base::FeatureList::IsEnabled(features::kAutofillAiWalletPrivatePasses)) {
show_loading_suggestions_callback(PrepareLoadingStateSuggestions(
base::ToVector(client.GetAutofillSuggestions()), suggestion));
}WDYT about calling this regardless whether `(should_fetch_from_server || should_reauth)` is true or false? I imagine that otherwise it wouldn't even show to the user because it would be very fast. We could then call it directly in AED and not pass it at all to this function, making things simpler.
I'd prefer to avoid modifying the prod logic as this CL ideally should be a noop.
Related details
- Florian Leimgruber
- Jihad Hanna
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. |
Jihad Hanna (Gerrit)
Jihad Hanna added 7 comments![]( "Open in Gerrit")
std::unique_ptr<device_reauth::DeviceAuthenticator> StartAutofillAiReauth(Should we just inline this now?
The authenticate function does not do much and so inlining might reduce the hopping factor while trying to understand this flow.
if (!masked_entity) {
// Re-auth failed, do not show notification.
std::move(fill_form_and_hide_popup).Run(std::nullopt);
return;
}This is not needed anymore? It is already treated in the Authenticate phase.
auto unmasking_callback = base::BindOnce(
[](base::WeakPtr<AutofillClient> client,
base::OnceCallback<void(std::optional<EntityInstance>)>
fill_form_and_hide_popup,
std::optional<EntityInstance> entity) {
if (client && !entity) {
client->ShowAutofillAiFetchFromWalletFailureNotification();
}
std::move(fill_form_and_hide_popup).Run(entity);
},
client, std::move(fill_form_and_hide_popup));Karol SygietWe agreed on moving this out of the other lambda
I tired moving this out of lambda but caused problems and test failures, as this requires moving the `fill_form_and_hide_popup` which the return callback may need on its own.
I've tried designing this better with Gemini but it believes this is the cleanest approach. LMK if you have some idea to move it out of here.
I think with the comment above this wouldn't be a problem anymore.
std::move(fill_form_and_hide_popup).Run(entity);You don't need to write stuff this way anymore, you can just define the `fetch_callback` as what you call `unmasking_callback` above and simply call `GetUnmaskedWalletEntityInstance()` here.
const bool should_reauth =(just curious) Why "re"auth? wouldn't `should_authenticate` be more accurate here?
const bool should_reauth =
WillFillSensitiveAttributes(entity, form, field.section(),
client.GetAppLocale()) &&
prefs::IsAutofillAiReauthBeforeFillingEnabled(client.GetPrefs());```
if (const bool should_reauth =
WillFillSensitiveAttributes(entity, form, field.section(),
client.GetAppLocale()) &&
prefs::IsAutofillAiReauthBeforeFillingEnabled(client.GetPrefs());
!should_reauth){
// No authentication is needed, simply forward the signal to the next phase.
MaybeFetchForFilling(entity, client, suggestion,
std::move(fill_form_and_hide_popup));
return nullptr;
}
```
Then you could call the thing below `authentication_callback` and we wouldn't have to fake successful authentication when the boolean is false.
Related details
- Florian Leimgruber
- Karol Sygiet
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. |
Florian Leimgruber (Gerrit)
Florian Leimgruber added 1 comment![]( "Open in Gerrit")
MaybeAuthenticateAndFetchForFilling(Karol SygietCan we split this into two function, one that authenticates and one that does fetching? I feel like doing two things in one function creates quite a overloaded interface with three callback parameter.
Especially the `show_loading_state_callback` feels really weird.
Florian LeimgruberDone
Jihad HannaDrive-by: Why did we end up with a single function again? :)
Karol SygietI believe ideally we have:
- `StartFillingProcess()` --> Pretty much does nothing besides calling the next function.
- `MaybeAuthenticateForFilling()` --> takes the callback, possibly augments it for authentication and then either directly call or authenticate and call the next function.
- `MaybeFetchForFilling()` --> takes the callback, possibly augments it for fetching and then either directly call or fetch and call `fill_form_and_hide_popup`.
Yeah, I first did what you've suggested Florian (or at least tried) and then changed the approach.
Jihad, could you take a look whether it looks more like your proposal now?
Wait, why `StartFillingProcess()`? The function name sounds super generic, it returns a `DeviceAuthenticator` and takes a `show_loading_suggestions_callback`. I'm definitely not a fan of this interface :)
What speaks against having just two functions `MaybeAuthenticateForFilling()` and `MaybeFetchForFilling()`?
Related details
- Karol Sygiet
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. |
Jihad Hanna (Gerrit)
Jihad Hanna added 1 comment![]( "Open in Gerrit")
MaybeAuthenticateAndFetchForFilling(Karol SygietCan we split this into two function, one that authenticates and one that does fetching? I feel like doing two things in one function creates quite a overloaded interface with three callback parameter.
Especially the `show_loading_state_callback` feels really weird.
Florian LeimgruberDone
Jihad HannaDrive-by: Why did we end up with a single function again? :)
Karol SygietI believe ideally we have:
- `StartFillingProcess()` --> Pretty much does nothing besides calling the next function.
- `MaybeAuthenticateForFilling()` --> takes the callback, possibly augments it for authentication and then either directly call or authenticate and call the next function.
- `MaybeFetchForFilling()` --> takes the callback, possibly augments it for fetching and then either directly call or fetch and call `fill_form_and_hide_popup`.
Florian LeimgruberYeah, I first did what you've suggested Florian (or at least tried) and then changed the approach.
Jihad, could you take a look whether it looks more like your proposal now?
Wait, why `StartFillingProcess()`? The function name sounds super generic, it returns a `DeviceAuthenticator` and takes a `show_loading_suggestions_callback`. I'm definitely not a fan of this interface :)
What speaks against having just two functions `MaybeAuthenticateForFilling()` and `MaybeFetchForFilling()`?
I have made a suggestion on how to get rid of `show_loading_suggestions_callback` somewhere below.
My reasoning for this is that having a generic function gives us a single place to document the whole flow in the header file, while the individual steps are hidden in the anonymous namespace.
I like `Maybe[Authenticate|Fetch]ForFilling`, but in the public API their names would not be ideal, as they don't reflect that more is happening (Authenticate calls Fetch and Fetch calls filling, but that's not visible in the names)
Florian Leimgruber (Gerrit)
Florian Leimgruber added 1 comment![]( "Open in Gerrit")
MaybeAuthenticateAndFetchForFilling(Karol SygietCan we split this into two function, one that authenticates and one that does fetching? I feel like doing two things in one function creates quite a overloaded interface with three callback parameter.
Especially the `show_loading_state_callback` feels really weird.
Florian LeimgruberDone
Jihad HannaDrive-by: Why did we end up with a single function again? :)
Karol SygietI believe ideally we have:
- `StartFillingProcess()` --> Pretty much does nothing besides calling the next function.
- `MaybeAuthenticateForFilling()` --> takes the callback, possibly augments it for authentication and then either directly call or authenticate and call the next function.
- `MaybeFetchForFilling()` --> takes the callback, possibly augments it for fetching and then either directly call or fetch and call `fill_form_and_hide_popup`.
Florian LeimgruberYeah, I first did what you've suggested Florian (or at least tried) and then changed the approach.
Jihad, could you take a look whether it looks more like your proposal now?
Jihad HannaWait, why `StartFillingProcess()`? The function name sounds super generic, it returns a `DeviceAuthenticator` and takes a `show_loading_suggestions_callback`. I'm definitely not a fan of this interface :)
What speaks against having just two functions `MaybeAuthenticateForFilling()` and `MaybeFetchForFilling()`?
I have made a suggestion on how to get rid of `show_loading_suggestions_callback` somewhere below.
My reasoning for this is that having a generic function gives us a single place to document the whole flow in the header file, while the individual steps are hidden in the anonymous namespace.
I like `Maybe[Authenticate|Fetch]ForFilling`, but in the public API their names would not be ideal, as they don't reflect that more is happening (Authenticate calls Fetch and Fetch calls filling, but that's not visible in the names)
My concern is that a function shouldn't have 8 parameters and do so many different things at once.
Jihad Hanna (Gerrit)
Jihad Hanna added 1 comment![]( "Open in Gerrit")
// Starts the filling process. It is responsible for authentication and fetching
// when needed by calling the subsequent functions.Please document the flow with bullets for clarity.
Jihad Hanna (Gerrit)
Jihad Hanna added 2 comments![]( "Open in Gerrit")
AutofillTriggerSource trigger_source,This is unused, remove
My concern is that a function shouldn't have 8 parameters
I understand, and I made a suggestion on how to get rid of the loading callback, and one parameter was unused, bringing the number down to 7 or 6, but besides that, every parameter is needed.
What can be done, is that we could replace some parameters with simpler ones:
- `form_structure` can be replaced with `requires_authentication` boolean. That would also help Karol getting rid of the cache dependency.
- `autofill_field` can be replaced with `origin` if we make the `requires_authentication` boolean a parameter. It will also help Karol getting rid of the cache dependency. A slight problem here is that some metric needs the field types deep in the callbacks, I would suggest discussing with some leads and removing this metric which is a little invasive.
- `suggestion` can be replaced with `requires_server_fetch` boolean.
---
My concern is that a function shouldn't do so many different things at once.
Besides the cumbersome `show_loading_suggestions_callback`, I don't see how the function is doing many things at once.
I think I initially saw `show_loading_suggestions_callback` inside the function and suggested trying to move the stuff it needs from AED to inside the function, maybe that was bad and I should have instead suggested moving `show_loading_suggestions_callback` out of the function altogether.
---
If my suggestions can all be implemented, we can reach the following signature:
```
// Starts the filling process. It is responsible for authentication and fetching
// when needed by calling the subsequent functions.
std::unique_ptr<device_reauth::DeviceAuthenticator> StartFillingProcess(
const EntityInstance& entity,
AutofillClient& client,
const url::Origin& origin,
bool requires_authentication,
bool requires_server_fetch,
base::OnceCallback<void(std::optional<EntityInstance>)>
fill_form_and_hide_popup);
```
Which seems lighter and more relevant to the function. (We might still need `autofill_field` though if we don't make progress on getting rid of the per-field-type authentication metric).
Florian Leimgruber (Gerrit)
Florian Leimgruber added 1 comment![]( "Open in Gerrit")
While this interface would definitely be an improvement, I'm still not a fan of it. Just form looking at the interface, I'm asking myself:
- `StartFillingProcess()` can mean a lot of things. Just from the name, I wouldn't be surprised if it just sets up some loggers for metrics :) Something like `AuthenticateAndFetchForFilling()` seems more precise?
- Why would `StartFillingProcess()` return a `DeviceAuthenticator`? Since I'm providing a `requires_authentication` boolean, wouldn't I expect that the function handles authentication? Why does the caller need it?
- What about error handling? From the interface, the nullopt callback parameter can only distinguish between success/failure. It has no way to distinguish between "re-auth failed" vs "fetch failed", even though these cases are treated differently (when fetching fails, we show a toast).
- `fill_form_and_hide_popup`: Why is hiding the popup even relevant in this context? Shouldn't this be a detail on the caller side? The caller knows the status of the popup. A filling related function shouldn't care.
- `requires_authentication` and `requires_server_fetch`: Shouldn't both of these properties be derivable from the entity + client?
---
I don't see how the function is doing many things at once
It does at least two things - re-auth and filling? My preference is still splitting this into two steps.
[[not very relevant] This will also make testing easier. Otherwise, tests for re-auth need to set up fetching and vice versa]
Regarding your earlier comment:
> My reasoning for this is that having a generic function gives us a single place to document the whole flow in the header file, while the individual steps are hidden in the anonymous namespace.
There's nothing preventing is from leaving a comment above these two functions, describing how the two functions interact?
Related details
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. |
Jihad Hanna (Gerrit)
Jihad Hanna added 1 comment![]( "Open in Gerrit")
Something like `AuthenticateAndFetchForFilling()` seems more precise?
I do not mind, the function starts an async flow, and it could be that the flow becomes longer and does more things in the future, which is why I suggested a generic name. Your suggested name SGTM too.
Why would `StartFillingProcess()` return a `DeviceAuthenticator`?
The `DeviceAuthenticator` needs to be persisted somewhere while authentication is happening, and there is no clear place to persist it given that it was chosen to implement this as free functions instead of classes (not that classes would be better, CCAccessManager currently looks very ugly as well).
What about error handling?
The client is passed around to be able to notify of different errors. What is the problem?
Why is hiding the popup even relevant in this context?
I agree it's not relevant.
`requires_authentication` and `requires_server_fetch`: Shouldn't both of these properties be derivable from the entity + client?
I have no idea why `requries_server_fetch` was part of the `Suggestion` and not derivable from the `EntityInstance`, and I agree that ideally you would be able to tell from the entity, but for the authentication one you would also need the form and field.
There's nothing preventing is from leaving a comment
True, personally as a matter of taste I would've prefered to document it in a single public function that hides the details from the public API, but I wouldn't mind otherwise.
---
Overall, I think we shouldn't try to solve all problems at once, because this logic grew to become hard to understand and interpret. Unless Karol or you have a suggestion on how to cleanly resolve everything in a single CL, I suggest we first start by converting this logic into a process like this CL is doing, and leaving TODOs around the places we dislike and then Karol, you or me can follow up on these.
Related details
- Karol Sygiet
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. |
Florian Leimgruber (Gerrit)
Florian Leimgruber added 1 comment![]( "Open in Gerrit")
I do not mind, the function starts an async flow, and it could be that the flow becomes longer and does more things in the future, which is why I suggested a generic name. Your suggested name SGTM too.
Agreed, the flow might become more complicated in the future. With a generic function name, people are invited to just add it to this one huge function and it might grow into hard to maintain code. I think that's another reason why I generally prefer separate functions.
The DeviceAuthenticator needs to be persisted somewhere while authentication is happening, and there is no clear place to persist it given that it was chosen to implement this as free functions instead of classes (not that classes would be better, CCAccessManager currently looks very ugly as well).
Have we actually considered a class? Maybe this would give us the best of both worlds:
- Separate (maybe private) functions for each step, separately testable.
- One public function orchestrating the flow.
- No strange return values, since the class can have members.
The client is passed around to be able to notify of different errors. What is the problem?
Ah, so the function does error handling. That works, but then I find it strange that the callback still does error handling as well. Would it suffice to have a callback that takes a non-optional entity, which is only called when filling should happen?
I have no idea why requries_server_fetch was part of the Suggestion
I think because Android UI needs it - see [this CL](https://chromium-review.git.corp.google.com/c/chromium/src/+/7705838).
and not derivable from the EntityInstance
It is derivable from the EntityInstance. But I guess given that it's in the suggestion, we want to reuse that :/
and I agree that ideally you would be able to tell from the entity, but for the authentication one you would also need the form and field.
That'd still be my preference, I think. Otherwise, we'll have the logic whether authentication is required in two places.
Overall, I think we shouldn't try to solve all problems at once, because this logic grew to become hard to understand and interpret. Unless Karol or you have a suggestion on how to cleanly resolve everything in a single CL, I suggest we first start by converting this logic into a process like this CL is doing, and leaving TODOs around the places we dislike and then Karol, you or me can follow up on these.
Generally agreed. But looking at the current implementation, it is split into multiple steps: First `MaybeAuthenticateBeforeFilling()`, which executes a callback that fetches, which executes a callback that fills.
Couldn't we just mirror this structure in the free functions?
Jihad Hanna (Gerrit)
Jihad Hanna added 3 comments![]( "Open in Gerrit")
Have we actually considered a class?
We could give it a try creating an `AutofillAiAccessManager` 👍
Would it suffice to have a callback that takes a non-optional entity, which is only called when filling should happen?
I think it would work 👍
That'd still be my preference
I don't think I agree with this, authentication for normal filling is about whether or not form filling will fill anything sensitive. For AtMemory, it's about whether the value to fill in the trigger field is sensitive (which is slightly different).
Generally agreed. But ... Couldn't we just mirror this structure in the free functions?
I did not understand this part.
if (!should_fetch_from_server) {Get the access manager from now and add the `|| !access_manager` case here
Florian Leimgruber (Gerrit)
Florian Leimgruber added 1 comment![]( "Open in Gerrit")
I don't think I agree with this, authentication for normal filling is about whether or not form filling will fill anything sensitive. For AtMemory, it's about whether the value to fill in the trigger field is sensitive (which is slightly different).
Ah, I didn't realise that AtMemory has different re-auth requirements. In this case, the parameter makes sense to me as well.
I did not understand this part.
IIUC, we have multiple options:
1. [current CL] Create one free function that does re-auth + fetching and split it later.
2. Create one free function, one for re-auth and one for fetching [more closely mirroring the current code structure], and later try to refactor it into a class.
3. Try creating a class now.
I meant that my preferences are 3 > 2 > 1.
Karol Sygiet (Gerrit)
Karol Sygiet added 1 comment![]( "Open in Gerrit")
I can try implementing the `AutofillAiAccessManager` (similar to `CreditCardAccessManager`), and try mimicking the CC's logic but for AutofillAi there. Does that sound good to both of you?
Related details
- Florian Leimgruber
- Jihad Hanna
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. |
Florian Leimgruber (Gerrit)
Florian Leimgruber added 1 comment![]( "Open in Gerrit")
- Jihad Hanna
- Karol Sygiet
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. |
Jihad Hanna (Gerrit)
Jihad Hanna added 1 comment![]( "Open in Gerrit")
MaybeAuthenticateAndFetchForFilling(- Florian Leimgruber
- Karol Sygiet
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. |
Karol Sygiet (Gerrit)
Karol Sygiet added 1 comment![]( "Open in Gerrit")
MaybeAuthenticateAndFetchForFilling(@fleim...@google.com @jihad...@google.com could you take a look if the new architecture looks good?
Related details
- Florian Leimgruber
- Jihad Hanna
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. |
Florian Leimgruber (Gerrit)
Florian Leimgruber added 7 comments![]( "Open in Gerrit")
client->ShowAutofillAiFetchFromWalletFailureNotification();(for discussion) Would it make sense to instead introduce a "FailureReason" enum and have a callback with a variant of EntityInstance and FailureReason. When, depending on whether the failure reason is re-auth failure or fetch failure, the caller can act accordingly. Showing the right error seems like a strange responsbility for the access manager.
const Suggestion& suggestion,Can we have some CHECK() that this is an AutofillAi suggestion?
base::OnceCallback<void(std::optional<EntityInstance>)> on_fetch_complete =Why not just stick with `on_entity_fetched`?
I think ideally, all the logic below should be split up into two private functions. But I guess that can be a follow-up, given that you're mostly just moving code.
client.UpdateAutofillSuggestions((for discussion) It does seem a little weird that the access manager has to update the suggestion state. Is this what we want?
std::unique_ptr<device_reauth::DeviceAuthenticator> authenticator =Can we initialise the authenticator in the constructor and always just always have it be non-null?
- Jihad Hanna
- Karol Sygiet
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. |
Jihad Hanna (Gerrit)
Jihad Hanna added 8 comments![]( "Open in Gerrit")
const raw_ref<BrowserAutofillManager> manager_;I think we can store the client instead if some cleanups are done.
void MaybeAuthenticateBeforeFilling(const std::u16string& reauth_message,Fetching
// Extracts `EntityInstance` from suggestion payload.
base::optional_ref<const EntityInstance> GetEntityInstance(
const Suggestion& suggestion) const;We should rather pass the masked entity directly and not need to fetch it here.
// Manages authentication and fetching for Autofill AI suggestions prior to
// filling a form.entities (no need to mention suggestions and filling here, you could consider reusing this for other purposes requiring authentication and server fetching)
void AutofillAiAccessManager::FetchEntityInstance(Why did we get rid of the two phases introduced previously? We seemed to have agreed on that
const bool should_fetch_from_server =
suggestion.GetPayload<Suggestion::AutofillAiPayload>()
.requires_server_fetch;This doesn't belong here too this should be passed to the class directly so that the class doesn't care about the concept of `Suggestion`.
This was mentioned before.
// Before running `on_fetch_complete`, potentially ask for a re-auth.
const bool is_sensitive = WillFillSensitiveAttributes(
*entity, *form_structure, autofill_field->section(),
client.GetAppLocale());
const bool should_reauth =
is_sensitive &&
prefs::IsAutofillAiReauthBeforeFillingEnabled(client.GetPrefs());This doesn't belong here.
You're extracting this logic to reuse it for AtMemory, but AtMemory will have a different check for reauth since you cannot assume having a cached form/field.
This was mentioned before.
// Show a loading state during fetching or reauth.
if ((should_fetch_from_server || should_reauth) &&
base::FeatureList::IsEnabled(features::kAutofillAiWalletPrivatePasses)) {
client.UpdateAutofillSuggestions(
PrepareLoadingStateSuggestions(
base::ToVector(client.GetAutofillSuggestions()), suggestion),
FillingProduct::kAutofillAi, trigger_source,
AutofillSuggestionsIgnoreFocusLoss(true));
}This logic belongs to the delegate.
This was mentioned before.
Related details
- Karol Sygiet
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. |
Karol Sygiet (Gerrit)
Karol Sygiet added 14 comments![]( "Open in Gerrit")
I think we can store the client instead if some cleanups are done.
Done
void MaybeAuthenticateBeforeFilling(const std::u16string& reauth_message,Karol SygietFetching
Done
// Extracts `EntityInstance` from suggestion payload.
base::optional_ref<const EntityInstance> GetEntityInstance(
const Suggestion& suggestion) const;We should rather pass the masked entity directly and not need to fetch it here.
Done
// Manages authentication and fetching for Autofill AI suggestions prior to
// filling a form.entities (no need to mention suggestions and filling here, you could consider reusing this for other purposes requiring authentication and server fetching)
Done
client->ShowAutofillAiFetchFromWalletFailureNotification();(for discussion) Would it make sense to instead introduce a "FailureReason" enum and have a callback with a variant of EntityInstance and FailureReason. When, depending on whether the failure reason is re-auth failure or fetch failure, the caller can act accordingly. Showing the right error seems like a strange responsbility for the access manager.
I modeled `AutofillAiAccessManager` to closely mirror the existing payments `CreditCardAccessManager` and IbanAccessManager. In those classes, the access controller is considered the central hub that orchestrates the unmasking flow, including instructing the client to show specific error notifications if FIDO/CVC/Wallet fetching fails.
Keeping the error trigger here preserves code encapsulation for the access orchestration flow, though returning an expected/FailureReason could definitely be considered if we refactor the other access managers down the road.
Why did we get rid of the two phases introduced previously? We seemed to have agreed on that
Done
Can we have some CHECK() that this is an AutofillAi suggestion?
Done
base::OnceCallback<void(std::optional<EntityInstance>)> on_fetch_complete =Why not just stick with `on_entity_fetched`?
Done
I think ideally, all the logic below should be split up into two private functions. But I guess that can be a follow-up, given that you're mostly just moving code.
Yes, I planned to do that in a followup which brings the memory support as this will touch them.
const bool should_fetch_from_server =
suggestion.GetPayload<Suggestion::AutofillAiPayload>()
.requires_server_fetch;This doesn't belong here too this should be passed to the class directly so that the class doesn't care about the concept of `Suggestion`.
This was mentioned before.
Since having the `Suggestion` is needed for the UI update sake (following the CCAM patterns). there's no point in passing the bool directly (especially since (also following the CCAM) the method is named `FetchEntityInstance` following the pattern from payments access managers
// Before running `on_fetch_complete`, potentially ask for a re-auth.
const bool is_sensitive = WillFillSensitiveAttributes(
*entity, *form_structure, autofill_field->section(),
client.GetAppLocale());
const bool should_reauth =
is_sensitive &&
prefs::IsAutofillAiReauthBeforeFillingEnabled(client.GetPrefs());This doesn't belong here.
You're extracting this logic to reuse it for AtMemory, but AtMemory will have a different check for reauth since you cannot assume having a cached form/field.
This was mentioned before.
I think it now does belong here. Since we have an AccessMananger class it should be the one that knows how to handle and when to authenticate depending on the form/field/EntityInstance.
This check will be modified slighlty when the memory support comes along and extracted to a helper utility function in an anonymous namespace(as requested by florian below).
client.UpdateAutofillSuggestions((for discussion) It does seem a little weird that the access manager has to update the suggestion state. Is this what we want?
Yes, this is intentional and mirrors how `CreditCardAccessManager` instructs the client to display challenge selection dialogs, SMS OTP input views, and FIDO prompts.
Since the access manager controls the long-lived asynchronous operations (device re-auth and server unmasking requests), it's the most natural place to signal the client to show the throbber/loading state during this transition.
// Show a loading state during fetching or reauth.
if ((should_fetch_from_server || should_reauth) &&
base::FeatureList::IsEnabled(features::kAutofillAiWalletPrivatePasses)) {
client.UpdateAutofillSuggestions(
PrepareLoadingStateSuggestions(
base::ToVector(client.GetAutofillSuggestions()), suggestion),
FillingProduct::kAutofillAi, trigger_source,
AutofillSuggestionsIgnoreFocusLoss(true));
}This logic belongs to the delegate.
This was mentioned before.
PTAL at: https://chromium-review.git.corp.google.com/c/chromium/src/+/7786130/comment/4125aabd_b0612cc3/
It follows the patterns from `CreditCardAccessManager`
std::unique_ptr<device_reauth::DeviceAuthenticator> authenticator =Can we initialise the authenticator in the constructor and always just always have it be non-null?
DeviceAuthenticators are context-bound. Initializing them lazily on-demand when authentication ensures we are obtaining a fresh, valid, and properly parented UI authenticator, rather than holding a stale reference in a manager that lasts the life of the frame document.
Related details
- Florian Leimgruber
- Jihad Hanna
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. |
Jihad Hanna (Gerrit)
Jihad Hanna added 1 comment![]( "Open in Gerrit")
const bool should_fetch_from_server =
suggestion.GetPayload<Suggestion::AutofillAiPayload>()
.requires_server_fetch;Karol SygietThis doesn't belong here too this should be passed to the class directly so that the class doesn't care about the concept of `Suggestion`.
This was mentioned before.
Since having the `Suggestion` is needed for the UI update sake (following the CCAM patterns). there's no point in passing the bool directly (especially since (also following the CCAM) the method is named `FetchEntityInstance` following the pattern from payments access managers
Since having the Suggestion is needed for the UI update sake
I don't see it, `suggestion` is just used so that the delegate can show a loading suggestion. I don't think this should be the responsibility of the access manager, and if CCAM does something along those lines then the problem would be there and not here.
Related details
- Florian Leimgruber
- Karol Sygiet
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. |
Karol Sygiet (Gerrit)
Karol Sygiet added 1 comment![]( "Open in Gerrit")
const bool should_fetch_from_server =
suggestion.GetPayload<Suggestion::AutofillAiPayload>()
.requires_server_fetch;Karol SygietThis doesn't belong here too this should be passed to the class directly so that the class doesn't care about the concept of `Suggestion`.
This was mentioned before.
Jihad HannaSince having the `Suggestion` is needed for the UI update sake (following the CCAM patterns). there's no point in passing the bool directly (especially since (also following the CCAM) the method is named `FetchEntityInstance` following the pattern from payments access managers
Since having the Suggestion is needed for the UI update sake
I don't see it, `suggestion` is just used so that the delegate can show a loading suggestion. I don't think this should be the responsibility of the access manager, and if CCAM does something along those lines then the problem would be there and not here.
The alternative would be that the client of this manager passes another callback that will be executed when the suggestions are loading?
I'm not sure which one I like more, I'd love to keep this CL as small as possible, and IMO the current implementation does so, with the added benefit of being in line with what CCAM uses.
However if you or @fleim...@google.com feels strongly about your proposal I can do so.
Related details
- Florian Leimgruber
- Jihad Hanna
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. |
Jihad Hanna (Gerrit)
Jihad Hanna added 1 comment![]( "Open in Gerrit")
const bool should_fetch_from_server =
suggestion.GetPayload<Suggestion::AutofillAiPayload>()
.requires_server_fetch;Karol SygietThis doesn't belong here too this should be passed to the class directly so that the class doesn't care about the concept of `Suggestion`.
This was mentioned before.
Jihad HannaSince having the `Suggestion` is needed for the UI update sake (following the CCAM patterns). there's no point in passing the bool directly (especially since (also following the CCAM) the method is named `FetchEntityInstance` following the pattern from payments access managers
Karol SygietSince having the Suggestion is needed for the UI update sake
I don't see it, `suggestion` is just used so that the delegate can show a loading suggestion. I don't think this should be the responsibility of the access manager, and if CCAM does something along those lines then the problem would be there and not here.
The alternative would be that the client of this manager passes another callback that will be executed when the suggestions are loading?
I'm not sure which one I like more, I'd love to keep this CL as small as possible, and IMO the current implementation does so, with the added benefit of being in line with what CCAM uses.
However if you or @fleim...@google.com feels strongly about your proposal I can do so.
I'm not sure I understand the alternative.
Why not just compute `should_fetch` and `should_authenticate` in AED and move this whole block there as it is?
Related details
- Florian Leimgruber
- Karol Sygiet
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. |
Karol Sygiet (Gerrit)
Karol Sygiet added 1 comment![]( "Open in Gerrit")
const bool should_fetch_from_server =
suggestion.GetPayload<Suggestion::AutofillAiPayload>()
.requires_server_fetch;Karol SygietThis doesn't belong here too this should be passed to the class directly so that the class doesn't care about the concept of `Suggestion`.
This was mentioned before.
Jihad HannaSince having the `Suggestion` is needed for the UI update sake (following the CCAM patterns). there's no point in passing the bool directly (especially since (also following the CCAM) the method is named `FetchEntityInstance` following the pattern from payments access managers
Karol SygietSince having the Suggestion is needed for the UI update sake
I don't see it, `suggestion` is just used so that the delegate can show a loading suggestion. I don't think this should be the responsibility of the access manager, and if CCAM does something along those lines then the problem would be there and not here.
Jihad HannaThe alternative would be that the client of this manager passes another callback that will be executed when the suggestions are loading?
I'm not sure which one I like more, I'd love to keep this CL as small as possible, and IMO the current implementation does so, with the added benefit of being in line with what CCAM uses.
However if you or @fleim...@google.com feels strongly about your proposal I can do so.
I'm not sure I understand the alternative.
Why not just compute `should_fetch` and `should_authenticate` in AED and move this whole block there as it is?
I think it is bad because then why do you need this class at all, if the clients dictate when authentication is needed and when its not.
IMO there should be only one source of truth that determines when to fetch and when to authenticate. We probably can do that without the `Suggestion` argument in the long term, but IMO it shouldn't be done in the same CL.
Related details
- Florian Leimgruber
- Jihad Hanna
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. |
Jihad Hanna (Gerrit)
Jihad Hanna added 1 comment![]( "Open in Gerrit")
const bool should_fetch_from_server =
suggestion.GetPayload<Suggestion::AutofillAiPayload>()
.requires_server_fetch;Karol SygietThis doesn't belong here too this should be passed to the class directly so that the class doesn't care about the concept of `Suggestion`.
This was mentioned before.
Jihad HannaSince having the `Suggestion` is needed for the UI update sake (following the CCAM patterns). there's no point in passing the bool directly (especially since (also following the CCAM) the method is named `FetchEntityInstance` following the pattern from payments access managers
Karol SygietSince having the Suggestion is needed for the UI update sake
I don't see it, `suggestion` is just used so that the delegate can show a loading suggestion. I don't think this should be the responsibility of the access manager, and if CCAM does something along those lines then the problem would be there and not here.
Jihad HannaThe alternative would be that the client of this manager passes another callback that will be executed when the suggestions are loading?
I'm not sure which one I like more, I'd love to keep this CL as small as possible, and IMO the current implementation does so, with the added benefit of being in line with what CCAM uses.
However if you or @fleim...@google.com feels strongly about your proposal I can do so.
Karol SygietI'm not sure I understand the alternative.
Why not just compute `should_fetch` and `should_authenticate` in AED and move this whole block there as it is?
I think it is bad because then why do you need this class at all, if the clients dictate when authentication is needed and when its not.
IMO there should be only one source of truth that determines when to fetch and when to authenticate. We probably can do that without the `Suggestion` argument in the long term, but IMO it shouldn't be done in the same CL.
I'm open to suggestions, but the way I see it the flow can either be:
- Get a local entity directly
- Get a local entity after authentication
- Unmask an entity directly
- Unmask an entity after authentication
And this can involve up to two rounds of asynchronous operations.
The callers responsibility is to tell the AM which flow it wants and the AM handles the actual flow.
Having the AM also decide which flow to use can get more complicated: For regular suggestions, authentication is computed by looking at the cached form. This might not exist for AtMemory. Will we add needless information in the payload just to support the AM? I think passing those two booleans and disconnecting the class from any concept other than `EntityInstance` (which it definitely needs) and possibly the `AutofillClient` (and hopefully something less generic) would be best to keep this as generic and simple as possible.
Related details
- Florian Leimgruber
- Karol Sygiet
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. |
Karol Sygiet (Gerrit)
Karol Sygiet added 1 comment![]( "Open in Gerrit")
In CCAM it is exactly the credit card's AM to understand the needs of a given credit card, whether it is local or from server, needs payments authentication or chrome authentication and so on. IMO we shouldn't change this for AutofillAi's AM.
Why do you think that AccessManager needs to be separated from things other than `EntityInstance`? IMO it makes sense that it checks for various requirements depending on the website's context of what and where gets filled. The interface in CCs case is simpler since pretty much any CC fill includes some sort of SPII, so they could have generalized that out, before the call to CCAM. Memory won't complicate things that much more, since the `Suggestion` is already be there (and contains all that we need) we can just extract the `is_sensitive` to a helper and do the is_SPII computation there (to keep the main function lean).
Related details
- Florian Leimgruber
- Jihad Hanna
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. |
Jihad Hanna (Gerrit)
Jihad Hanna added 1 comment![]( "Open in Gerrit")
In CCAM it is exactly the credit card's AM to understand the needs of a given credit card.
I think that's wrong. See [the CC filling logic](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/browser_autofill_manager.cc;l=2057;drc=fff91ab83449bbfa0790f18e00e68384a3a1365d).
Why do you think that AccessManager needs to be separated from things other than `EntityInstance`?
Because the less dependencies a class has on other Autofill concepts, the easier it can be used in various settings (again, the example would be regular-autofill/at-memory/(eventually)bluedog).
IMO it makes sense that it checks for various requirements depending on the website's context of what and where gets filled...
I'll think more about this 👍
Related details
- Florian Leimgruber
- Karol Sygiet
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)
Christoph Schwering added 6 comments![]( "Open in Gerrit")
virtual void FetchEntityInstance((Why) Do we need a class? The only necessary member variable appears to be `authenticator_`, which should not survive one flow anyway.
client->ShowAutofillAiFetchFromWalletFailureNotification();Karol Sygiet(for discussion) Would it make sense to instead introduce a "FailureReason" enum and have a callback with a variant of EntityInstance and FailureReason. When, depending on whether the failure reason is re-auth failure or fetch failure, the caller can act accordingly. Showing the right error seems like a strange responsbility for the access manager.
I modeled `AutofillAiAccessManager` to closely mirror the existing payments `CreditCardAccessManager` and IbanAccessManager. In those classes, the access controller is considered the central hub that orchestrates the unmasking flow, including instructing the client to show specific error notifications if FIDO/CVC/Wallet fetching fails.
Keeping the error trigger here preserves code encapsulation for the access orchestration flow, though returning an expected/FailureReason could definitely be considered if we refactor the other access managers down the road.
Came here because Jihad asked me for an opinion.
From this and [this](https://chromium-review.git.corp.google.com/c/chromium/src/+/7786130/comment/72ca5728_9325bbc9/) and [this](https://chromium-review.git.corp.google.com/c/chromium/src/+/7786130/comment/4125aabd_b0612cc3/), it sounds like the basic question is whether we prefer
- decoupling AutofillAiAccessManager or
- consistency with CreditCardAccessManager and IbanAccessManager.
If we take the first option, I assume it's unlikely that CCAM and IAM will be refactored analogously. OTOH, I'm not sure if CCAM is a good model to follow because in the past I've found it rather difficult to understand.
Intuitively, I'd prefer cleanly decoupling AAAM. Not a massively strong opinion though because I haven't worked through what that'd actually look like.
std::unique_ptr<device_reauth::DeviceAuthenticator> authenticator =Karol SygietCan we initialise the authenticator in the constructor and always just always have it be non-null?
DeviceAuthenticators are context-bound. Initializing them lazily on-demand when authentication ensures we are obtaining a fresh, valid, and properly parented UI authenticator, rather than holding a stale reference in a manager that lasts the life of the frame document.
What does "valid" and "properly parented" mean in this context?
At first I thought the (or a?) critical scenario (in both the old and the new code) is when the user moves the tab to another window during authentication. But the `window` appears to [matter only on Android](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/device_reauth/chrome_device_authenticator_factory.cc;l=57-58;drc=91861f7ddfab07ec80ffea4b5968350a4170e8b6), where AFAIK there are no multiple windows.
// Lazily initialized: access only through GetAutofillAiAccessManager().Lazy initialization is not a good default. If there's no strong reason for it, we sohuldn't use it IMO. (AFAIK there's no reason for the other ones either.)
Do we even need a `std::unique_ptr`?
friend class AutofillAiAccessManager;Why do we need this?
Related details
- Florian Leimgruber
- Karol Sygiet
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. |
Karol Sygiet (Gerrit)
Karol Sygiet added 6 comments![]( "Open in Gerrit")
virtual void FetchEntityInstance((Why) Do we need a class? The only necessary member variable appears to be `authenticator_`, which should not survive one flow anyway.
The tab is not freezed when the authentication is in progress, user can still interact with the page and do all kinds of stuff, so the authenticator object has to be persisted somewhere. IMO a dedicated class its best for this, otherwise either `AutofillExternalDelegate` or `BAM` needs to keep it which IMO is even worse (as then it is not clear what filling product created the authenticator)
client->ShowAutofillAiFetchFromWalletFailureNotification();Karol Sygiet(for discussion) Would it make sense to instead introduce a "FailureReason" enum and have a callback with a variant of EntityInstance and FailureReason. When, depending on whether the failure reason is re-auth failure or fetch failure, the caller can act accordingly. Showing the right error seems like a strange responsbility for the access manager.
Christoph SchweringI modeled `AutofillAiAccessManager` to closely mirror the existing payments `CreditCardAccessManager` and IbanAccessManager. In those classes, the access controller is considered the central hub that orchestrates the unmasking flow, including instructing the client to show specific error notifications if FIDO/CVC/Wallet fetching fails.
Keeping the error trigger here preserves code encapsulation for the access orchestration flow, though returning an expected/FailureReason could definitely be considered if we refactor the other access managers down the road.
Came here because Jihad asked me for an opinion.
From this and [this](https://chromium-review.git.corp.google.com/c/chromium/src/+/7786130/comment/72ca5728_9325bbc9/) and [this](https://chromium-review.git.corp.google.com/c/chromium/src/+/7786130/comment/4125aabd_b0612cc3/), it sounds like the basic question is whether we prefer
- decoupling AutofillAiAccessManager or
- consistency with CreditCardAccessManager and IbanAccessManager.
If we take the first option, I assume it's unlikely that CCAM and IAM will be refactored analogously. OTOH, I'm not sure if CCAM is a good model to follow because in the past I've found it rather difficult to understand.
Intuitively, I'd prefer cleanly decoupling AAAM. Not a massively strong opinion though because I haven't worked through what that'd actually look like.
Okey, If I understood you correctly, I removed the showing suggestion from the AAiAM and a closure is passed instead.
1. Fair point, passed the bool directly and removed the suggestion argument.
std::unique_ptr<device_reauth::DeviceAuthenticator> authenticator =Karol SygietCan we initialise the authenticator in the constructor and always just always have it be non-null?
Christoph SchweringDeviceAuthenticators are context-bound. Initializing them lazily on-demand when authentication ensures we are obtaining a fresh, valid, and properly parented UI authenticator, rather than holding a stale reference in a manager that lasts the life of the frame document.
What does "valid" and "properly parented" mean in this context?
At first I thought the (or a?) critical scenario (in both the old and the new code) is when the user moves the tab to another window during authentication. But the `window` appears to [matter only on Android](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/device_reauth/chrome_device_authenticator_factory.cc;l=57-58;drc=91861f7ddfab07ec80ffea4b5968350a4170e8b6), where AFAIK there are no multiple windows.
It's exactly the same case on other platforms, but the Mac and Windows (IIUC) OSes are smart enough to know from which process/tab/chrome browser window a request was made to show the dialog, and thus which one the dialog should be bounded to.
// Lazily initialized: access only through GetAutofillAiAccessManager().Lazy initialization is not a good default. If there's no strong reason for it, we sohuldn't use it IMO. (AFAIK there's no reason for the other ones either.)
Do we even need a `std::unique_ptr`?
unique_ptr is preferred as when the navigation happens `BrowserAutofillManager::Reset()` destroyed the previous AAiAM and cancels any ongoing reauth operations (this could be implemented directly in the `BAM::Reset()`, but using unique_ptr follows the CCAM implementation).
Why do we need this?
- Christoph Schwering
- Jihad Hanna
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. |
Jihad Hanna (Gerrit)
Jihad Hanna added 24 comments![]( "Open in Gerrit")
const FormStructure& form_structure,
const AutofillField& autofill_field,These can be replaced with `will_fill_sensitive_info` or something
client->ShowAutofillAiFetchFromWalletFailureNotification();Karol Sygiet(for discussion) Would it make sense to instead introduce a "FailureReason" enum and have a callback with a variant of EntityInstance and FailureReason. When, depending on whether the failure reason is re-auth failure or fetch failure, the caller can act accordingly. Showing the right error seems like a strange responsbility for the access manager.
Christoph SchweringI modeled `AutofillAiAccessManager` to closely mirror the existing payments `CreditCardAccessManager` and IbanAccessManager. In those classes, the access controller is considered the central hub that orchestrates the unmasking flow, including instructing the client to show specific error notifications if FIDO/CVC/Wallet fetching fails.
Keeping the error trigger here preserves code encapsulation for the access orchestration flow, though returning an expected/FailureReason could definitely be considered if we refactor the other access managers down the road.
Karol SygietCame here because Jihad asked me for an opinion.
From this and [this](https://chromium-review.git.corp.google.com/c/chromium/src/+/7786130/comment/72ca5728_9325bbc9/) and [this](https://chromium-review.git.corp.google.com/c/chromium/src/+/7786130/comment/4125aabd_b0612cc3/), it sounds like the basic question is whether we prefer
- decoupling AutofillAiAccessManager or
- consistency with CreditCardAccessManager and IbanAccessManager.
If we take the first option, I assume it's unlikely that CCAM and IAM will be refactored analogously. OTOH, I'm not sure if CCAM is a good model to follow because in the past I've found it rather difficult to understand.
Intuitively, I'd prefer cleanly decoupling AAAM. Not a massively strong opinion though because I haven't worked through what that'd actually look like.
Okey, If I understood you correctly, I removed the showing suggestion from the AAiAM and a closure is passed instead.
Discussed offline: We will adopt the `FailureReason` approach
const bool should_reauth =
is_sensitive &&
prefs::IsAutofillAiReauthBeforeFillingEnabled(client_->GetPrefs());
// Show a loading state during fetching or reauth.
if ((should_fetch || should_reauth) &&
base::FeatureList::IsEnabled(features::kAutofillAiWalletPrivatePasses)) {
if (show_loading_state_callback) {
std::move(show_loading_state_callback).Run();
}
}Discussed with Bruno, and he's okay with decoupling this from `should_[fetch|reauth]` behind a killswitch.
WDYT about moving this whole block to AED
```
const bool should_show_loading = should_fetch || (is_sensitive &&
prefs::IsAutofillAiReauthBeforeFillingEnabled(client_->GetPrefs())) ||
base::IsEnabled(kFooKillswitch));
if (should_show_loading &&
base::FeatureList::IsEnabled(features::kAutofillAiWalletPrivatePasses)) {
...
}
```
This way when the killswitch is cleaned up we can remove the pref logic from there.
const bool should_fetch_from_server =
suggestion.GetPayload<Suggestion::AutofillAiPayload>()
.requires_server_fetch;Discussed offline and agreed on the next steps:
- `FormStructure` and `AutofillField` will be removed from the contract.
- The AM will take care of generic conditions like mandatory reauth.
- We will push towards moving `show_loading_state_callback` back to AED.
- The `FetchCallback` will return an optional `FailureReason` so that callers can react accordingly.
std::u16string message;
// Android is excluded here because the system biometric prompt does not
// support a custom message.
#if BUILDFLAG(IS_MAC) || BUILDFLAG(IS_WIN) || BUILDFLAG(IS_CHROMEOS) || \
BUILDFLAG(IS_IOS)
const std::u16string origin =
base::UTF8ToUTF16(autofill_field.origin().host());
message = l10n_util::GetStringFUTF16(IDS_AUTOFILL_AI_FILLING_REAUTH, origin);
#endif // BUILDFLAG(IS_MAC) || BUILDFLAG(IS_WIN) || BUILDFLAG(IS_CHROMEOS) ||
// BUILDFLAG(IS_IOS)I'd pass the origin to the `Authenticate()` method and move this logic there
// Before running `on_fetch_complete`, potentially ask for a re-auth.
const bool is_sensitive = WillFillSensitiveAttributes(
*entity, *form_structure, autofill_field->section(),
client.GetAppLocale());
const bool should_reauth =
is_sensitive &&
prefs::IsAutofillAiReauthBeforeFillingEnabled(client.GetPrefs());This doesn't belong here.
You're extracting this logic to reuse it for AtMemory, but AtMemory will have a different check for reauth since you cannot assume having a cached form/field.
Karol SygietThis was mentioned before.
I think it now does belong here. Since we have an AccessMananger class it should be the one that knows how to handle and when to authenticate depending on the form/field/EntityInstance.
This check will be modified slighlty when the memory support comes along and extracted to a helper utility function in an anonymous namespace(as requested by florian below).
Acknowledged
client.UpdateAutofillSuggestions((for discussion) It does seem a little weird that the access manager has to update the suggestion state. Is this what we want?
Yes, this is intentional and mirrors how `CreditCardAccessManager` instructs the client to display challenge selection dialogs, SMS OTP input views, and FIDO prompts.
Since the access manager controls the long-lived asynchronous operations (device re-auth and server unmasking requests), it's the most natural place to signal the client to show the throbber/loading state during this transition.
Discussed offline: We will try to move this to AED.
LogReauthToFillResultPerFieldType(ai_field_types, auth_succeeded);Let's move this to AED after introducing the `FailureReason`
// Show a loading state during fetching or reauth.
if ((should_fetch_from_server || should_reauth) &&
base::FeatureList::IsEnabled(features::kAutofillAiWalletPrivatePasses)) {
client.UpdateAutofillSuggestions(
PrepareLoadingStateSuggestions(
base::ToVector(client.GetAutofillSuggestions()), suggestion),
FillingProduct::kAutofillAi, trigger_source,
AutofillSuggestionsIgnoreFocusLoss(true));
}This logic belongs to the delegate.
Karol SygietThis was mentioned before.
PTAL at: https://chromium-review.git.corp.google.com/c/chromium/src/+/7786130/comment/4125aabd_b0612cc3/
It follows the patterns from `CreditCardAccessManager`
Jihad Hanna
Acknowledged
FetchUnmaskedEntityFromWallet(client_->GetWeakPtr(), std::move(callback),nit: I'd inline this since the above logic seems trivial
void AutofillAiAccessManager::OnReauthCompleted(nit: I'd also inline this as it's used once and the logic is trivial
AutofillTriggerSource trigger_source,Jihad HannaThis is unused, remove
Acknowledged
// Starts the filling process. It is responsible for authentication and fetching
// when needed by calling the subsequent functions.Please document the flow with bullets for clarity.
Acknowledged
MaybeAuthenticateAndFetchForFilling(Can we split this into two function, one that authenticates and one that does fetching? I feel like doing two things in one function creates quite a overloaded interface with three callback parameter.
base::OnceCallback<void(std::optional<EntityInstance>)>
fill_form_and_hide_popup);
```
+1, thanks!
Get the access manager from now and add the `|| !access_manager` case here
Acknowledged
std::unique_ptr<device_reauth::DeviceAuthenticator> StartAutofillAiReauth(Jihad HannaShould we just inline this now?
The authenticate function does not do much and so inlining might reduce the hopping factor while trying to understand this flow.
Acknowledged
if (!auth_succeeded) {Jihad Hanna`|| !client`?
Acknowledged
if (!masked_entity) {
// Re-auth failed, do not show notification.
std::move(fill_form_and_hide_popup).Run(std::nullopt);
return;
}Jihad HannaThis is not needed anymore? It is already treated in the Authenticate phase.
Acknowledged
auto unmasking_callback = base::BindOnce(
[](base::WeakPtr<AutofillClient> client,
base::OnceCallback<void(std::optional<EntityInstance>)>
fill_form_and_hide_popup,
std::optional<EntityInstance> entity) {
if (client && !entity) {
client->ShowAutofillAiFetchFromWalletFailureNotification();
}
std::move(fill_form_and_hide_popup).Run(entity);
},
client, std::move(fill_form_and_hide_popup));Karol SygietWe agreed on moving this out of the other lambda
Jihad HannaI tired moving this out of lambda but caused problems and test failures, as this requires moving the `fill_form_and_hide_popup` which the return callback may need on its own.
I've tried designing this better with Gemini but it believes this is the cleanest approach. LMK if you have some idea to move it out of here.
Jihad HannaI think with the comment above this wouldn't be a problem anymore.
Acknowledged
std::move(fill_form_and_hide_popup).Run(entity);Jihad HannaYou don't need to write stuff this way anymore, you can just define the `fetch_callback` as what you call `unmasking_callback` above and simply call `GetUnmaskedWalletEntityInstance()` here.
Acknowledged
const bool should_reauth =Jihad Hanna(just curious) Why "re"auth? wouldn't `should_authenticate` be more accurate here?
Acknowledged
const bool should_reauth =
WillFillSensitiveAttributes(entity, form, field.section(),
client.GetAppLocale()) &&
prefs::IsAutofillAiReauthBeforeFillingEnabled(client.GetPrefs());Jihad Hanna```
if (const bool should_reauth =
WillFillSensitiveAttributes(entity, form, field.section(),
client.GetAppLocale()) &&
prefs::IsAutofillAiReauthBeforeFillingEnabled(client.GetPrefs());
!should_reauth){
// No authentication is needed, simply forward the signal to the next phase.
MaybeFetchForFilling(entity, client, suggestion,
std::move(fill_form_and_hide_popup));
return nullptr;
}
```Then you could call the thing below `authentication_callback` and we wouldn't have to fake successful authentication when the boolean is false.
Acknowledged
if ((should_fetch_from_server || should_reauth) &&
base::FeatureList::IsEnabled(features::kAutofillAiWalletPrivatePasses)) {
show_loading_suggestions_callback(PrepareLoadingStateSuggestions(
base::ToVector(client.GetAutofillSuggestions()), suggestion));
}Karol SygietWDYT about calling this regardless whether `(should_fetch_from_server || should_reauth)` is true or false? I imagine that otherwise it wouldn't even show to the user because it would be very fast. We could then call it directly in AED and not pass it at all to this function, making things simpler.
Jihad HannaI'd prefer to avoid modifying the prod logic as this CL ideally should be a noop.
Acknowledged
Related details
- Christoph Schwering
- Florian Leimgruber
- Karol Sygiet
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. |
Karol Sygiet (Gerrit)
Karol Sygiet added 8 comments![]( "Open in Gerrit")
const FormStructure& form_structure,
const AutofillField& autofill_field,These can be replaced with `will_fill_sensitive_info` or something
Done
client->ShowAutofillAiFetchFromWalletFailureNotification();Karol Sygiet(for discussion) Would it make sense to instead introduce a "FailureReason" enum and have a callback with a variant of EntityInstance and FailureReason. When, depending on whether the failure reason is re-auth failure or fetch failure, the caller can act accordingly. Showing the right error seems like a strange responsbility for the access manager.
Christoph SchweringI modeled `AutofillAiAccessManager` to closely mirror the existing payments `CreditCardAccessManager` and IbanAccessManager. In those classes, the access controller is considered the central hub that orchestrates the unmasking flow, including instructing the client to show specific error notifications if FIDO/CVC/Wallet fetching fails.
Keeping the error trigger here preserves code encapsulation for the access orchestration flow, though returning an expected/FailureReason could definitely be considered if we refactor the other access managers down the road.
Karol SygietCame here because Jihad asked me for an opinion.
From this and [this](https://chromium-review.git.corp.google.com/c/chromium/src/+/7786130/comment/72ca5728_9325bbc9/) and [this](https://chromium-review.git.corp.google.com/c/chromium/src/+/7786130/comment/4125aabd_b0612cc3/), it sounds like the basic question is whether we prefer
- decoupling AutofillAiAccessManager or
- consistency with CreditCardAccessManager and IbanAccessManager.
If we take the first option, I assume it's unlikely that CCAM and IAM will be refactored analogously. OTOH, I'm not sure if CCAM is a good model to follow because in the past I've found it rather difficult to understand.
Intuitively, I'd prefer cleanly decoupling AAAM. Not a massively strong opinion though because I haven't worked through what that'd actually look like.
Jihad HannaOkey, If I understood you correctly, I removed the showing suggestion from the AAiAM and a closure is passed instead.
Discussed offline: We will adopt the `FailureReason` approach
Done
const bool should_reauth =
is_sensitive &&
prefs::IsAutofillAiReauthBeforeFillingEnabled(client_->GetPrefs());
// Show a loading state during fetching or reauth.
if ((should_fetch || should_reauth) &&
base::FeatureList::IsEnabled(features::kAutofillAiWalletPrivatePasses)) {
if (show_loading_state_callback) {
std::move(show_loading_state_callback).Run();
}
}Discussed with Bruno, and he's okay with decoupling this from `should_[fetch|reauth]` behind a killswitch.
WDYT about moving this whole block to AED
```
const bool should_show_loading = should_fetch || (is_sensitive &&
prefs::IsAutofillAiReauthBeforeFillingEnabled(client_->GetPrefs())) ||
base::IsEnabled(kFooKillswitch));
if (should_show_loading &&
base::FeatureList::IsEnabled(features::kAutofillAiWalletPrivatePasses)) {
...
}
```This way when the killswitch is cleaned up we can remove the pref logic from there.
Added a TODO, will do in a followup
std::u16string message;
// Android is excluded here because the system biometric prompt does not
// support a custom message.
#if BUILDFLAG(IS_MAC) || BUILDFLAG(IS_WIN) || BUILDFLAG(IS_CHROMEOS) || \
BUILDFLAG(IS_IOS)
const std::u16string origin =
base::UTF8ToUTF16(autofill_field.origin().host());
message = l10n_util::GetStringFUTF16(IDS_AUTOFILL_AI_FILLING_REAUTH, origin);
#endif // BUILDFLAG(IS_MAC) || BUILDFLAG(IS_WIN) || BUILDFLAG(IS_CHROMEOS) ||
// BUILDFLAG(IS_IOS)I'd pass the origin to the `Authenticate()` method and move this logic there
Done
client.UpdateAutofillSuggestions(Karol Sygiet(for discussion) It does seem a little weird that the access manager has to update the suggestion state. Is this what we want?
Jihad HannaYes, this is intentional and mirrors how `CreditCardAccessManager` instructs the client to display challenge selection dialogs, SMS OTP input views, and FIDO prompts.
Since the access manager controls the long-lived asynchronous operations (device re-auth and server unmasking requests), it's the most natural place to signal the client to show the throbber/loading state during this transition.
Discussed offline: We will try to move this to AED.
Done
LogReauthToFillResultPerFieldType(ai_field_types, auth_succeeded);Let's move this to AED after introducing the `FailureReason`
Done
FetchUnmaskedEntityFromWallet(client_->GetWeakPtr(), std::move(callback),nit: I'd inline this since the above logic seems trivial
Done
nit: I'd also inline this as it's used once and the logic is trivial
- Christoph Schwering
- Florian Leimgruber
- Jihad Hanna
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. |
Jihad Hanna (Gerrit)
Jihad Hanna added 11 comments![]( "Open in Gerrit")
// Handles optional server-side fetching of the unmasked server entity.
// Invokes `callback` with the unmasked entity, or `std::nullopt` if it fails.
void MaybeFetchForFilling(
bool should_fetch,
base::OnceCallback<void(std::optional<EntityInstance>, FailureReason)>
callback,
std::optional<EntityInstance> entity,
FailureReason failure_reason);
// Requires user authentication and runs `callback` with an authentication
// result on completion.
void Authenticate(const url::Origin& origin,
base::OnceCallback<void(bool)> callback);nitty nit: Swap order, as it makes reading the class more intuitive vis-a-vis the whole flow (Also in the source file).
base::OnceClosure show_loading_state_callback,`FunctionRef`
const url::Origin& origin,nitty nit: move above so that both booleans are next to each other
enum class FailureReason {
kNone,
kReauthFailed,
kFetchFailed,
};
using OnEntityInstanceFetchedCallback =
base::OnceCallback<void(std::optional<EntityInstance>, FailureReason)>;Should we instead remove `kNone` and make the callback take an `std::variant<EntityInstance, FailureReason>`? I think that this was Florian's suggestion and would improve type safety.
#include "components/autofill/core/browser/field_types.h"remove?
#include "components/autofill/core/browser/data_manager/autofill_ai/entity_data_manager.h"
#include "components/autofill/core/browser/filling/autofill_ai/field_filling_entity_util.h"Remove?
#include "components/autofill/core/browser/foundations/browser_autofill_manager.h"Do we still need thiS?
#include "components/autofill/core/common/form_data.h"
#include "components/autofill/core/common/form_field_data.h"Remove?
void AutofillAiAccessManager::MaybeFetchForFilling(`MaybeUnmaskServerEntity`? There's no mention of filling anymore and this way `Fetch` is not used twice (Also used in `FetchEntityInstance`).
Related details
- Christoph Schwering
- Florian Leimgruber
- Karol Sygiet
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. |