// Called when biometric authentication is completed.
// Triggers the `callback` if `auth_succeeded` is true.
void OnAutofillAiReauthCompleted(base::OnceClosure callback,
bool auth_succeeded);
// Authenticates the user and runs `callback` is the authentication is
// completed.
void MaybeAuthenticateBeforeFilling(base::OnceClosure callback);Can we create a method `FillOrPreviewEntityForm()` and follow the same pattern used by [`FillOrPreviewCreditCardForm()`](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/browser_autofill_manager.cc;l=1941;drc=7666bc1983c2a5b98e5dc6fa6c28f8f53c07d06f)?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
// Called when biometric authentication is completed.
// Triggers the `callback` if `auth_succeeded` is true.
void OnAutofillAiReauthCompleted(base::OnceClosure callback,
bool auth_succeeded);
// Authenticates the user and runs `callback` is the authentication is
// completed.
void MaybeAuthenticateBeforeFilling(base::OnceClosure callback);Can we create a method `FillOrPreviewEntityForm()` and follow the same pattern used by [`FillOrPreviewCreditCardForm()`](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/browser_autofill_manager.cc;l=1941;drc=7666bc1983c2a5b98e5dc6fa6c28f8f53c07d06f)?
I am following the pattern for [passwords](https://source.chromium.org/chromium/chromium/src/+/main:components/password_manager/core/browser/password_manual_fallback_flow.cc;l=420;drc=7666bc1983c2a5b98e5dc6fa6c28f8f53c07d06f;bpv=1;bpt=1). The advantage is that any other suggestion type can easily reuse the authentication logic from the external delegate.
What is the advantage of having it on BAM?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Done
// Called when biometric authentication is completed.
// Triggers the `callback` if `auth_succeeded` is true.
void OnAutofillAiReauthCompleted(base::OnceClosure callback,
bool auth_succeeded);
// Authenticates the user and runs `callback` is the authentication is
// completed.
void MaybeAuthenticateBeforeFilling(base::OnceClosure callback);Bruno BragaCan we create a method `FillOrPreviewEntityForm()` and follow the same pattern used by [`FillOrPreviewCreditCardForm()`](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/browser_autofill_manager.cc;l=1941;drc=7666bc1983c2a5b98e5dc6fa6c28f8f53c07d06f)?
I am following the pattern for [passwords](https://source.chromium.org/chromium/chromium/src/+/main:components/password_manager/core/browser/password_manual_fallback_flow.cc;l=420;drc=7666bc1983c2a5b98e5dc6fa6c28f8f53c07d06f;bpv=1;bpt=1). The advantage is that any other suggestion type can easily reuse the authentication logic from the external delegate.
What is the advantage of having it on BAM?
Using this pattern SGTM, I just meant to move the member from the delegate to BAM and have a single function `BAM::FillOrPreviewEntityForm()` that includes lambdas to be passed to the authenticator.
The advantage is only conceptual, but usually AED should be a thin layer between BAM and the UI. Heavy logic should rather exist in BAM.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
// Called when biometric authentication is completed.
// Triggers the `callback` if `auth_succeeded` is true.
void OnAutofillAiReauthCompleted(base::OnceClosure callback,
bool auth_succeeded);
// Authenticates the user and runs `callback` is the authentication is
// completed.
void MaybeAuthenticateBeforeFilling(base::OnceClosure callback);Bruno BragaCan we create a method `FillOrPreviewEntityForm()` and follow the same pattern used by [`FillOrPreviewCreditCardForm()`](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/browser_autofill_manager.cc;l=1941;drc=7666bc1983c2a5b98e5dc6fa6c28f8f53c07d06f)?
Jihad HannaI am following the pattern for [passwords](https://source.chromium.org/chromium/chromium/src/+/main:components/password_manager/core/browser/password_manual_fallback_flow.cc;l=420;drc=7666bc1983c2a5b98e5dc6fa6c28f8f53c07d06f;bpv=1;bpt=1). The advantage is that any other suggestion type can easily reuse the authentication logic from the external delegate.
What is the advantage of having it on BAM?
Using this pattern SGTM, I just meant to move the member from the delegate to BAM and have a single function `BAM::FillOrPreviewEntityForm()` that includes lambdas to be passed to the authenticator.
The advantage is only conceptual, but usually AED should be a thin layer between BAM and the UI. Heavy logic should rather exist in BAM.
For passwords touch to fill, the authentication happens in the TTF delegate [1], but I agree that it might make sense to have this logic in BAM (credit card fetching happens in BAM as well).
authenticator_.reset();For my own education, why do we need to reset it after every filling?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
// Called when biometric authentication is completed.
// Triggers the `callback` if `auth_succeeded` is true.
void OnAutofillAiReauthCompleted(base::OnceClosure callback,
bool auth_succeeded);
// Authenticates the user and runs `callback` is the authentication is
// completed.
void MaybeAuthenticateBeforeFilling(base::OnceClosure callback);Bruno BragaCan we create a method `FillOrPreviewEntityForm()` and follow the same pattern used by [`FillOrPreviewCreditCardForm()`](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/browser_autofill_manager.cc;l=1941;drc=7666bc1983c2a5b98e5dc6fa6c28f8f53c07d06f)?
Jihad HannaI am following the pattern for [passwords](https://source.chromium.org/chromium/chromium/src/+/main:components/password_manager/core/browser/password_manual_fallback_flow.cc;l=420;drc=7666bc1983c2a5b98e5dc6fa6c28f8f53c07d06f;bpv=1;bpt=1). The advantage is that any other suggestion type can easily reuse the authentication logic from the external delegate.
What is the advantage of having it on BAM?
Using this pattern SGTM, I just meant to move the member from the delegate to BAM and have a single function `BAM::FillOrPreviewEntityForm()` that includes lambdas to be passed to the authenticator.
The advantage is only conceptual, but usually AED should be a thin layer between BAM and the UI. Heavy logic should rather exist in BAM.
These are two pretty light weight methods (in terms of simplicity of implementation) that can be easily reused. Every filling operation goes through the delegate, to me it makes sense conceptually that filling authentication resides here.
We could also get someones else's opinion.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
// Called when biometric authentication is completed.
// Triggers the `callback` if `auth_succeeded` is true.
void OnAutofillAiReauthCompleted(base::OnceClosure callback,
bool auth_succeeded);
// Authenticates the user and runs `callback` is the authentication is
// completed.
void MaybeAuthenticateBeforeFilling(base::OnceClosure callback);Bruno BragaCan we create a method `FillOrPreviewEntityForm()` and follow the same pattern used by [`FillOrPreviewCreditCardForm()`](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/browser_autofill_manager.cc;l=1941;drc=7666bc1983c2a5b98e5dc6fa6c28f8f53c07d06f)?
Jihad HannaI am following the pattern for [passwords](https://source.chromium.org/chromium/chromium/src/+/main:components/password_manager/core/browser/password_manual_fallback_flow.cc;l=420;drc=7666bc1983c2a5b98e5dc6fa6c28f8f53c07d06f;bpv=1;bpt=1). The advantage is that any other suggestion type can easily reuse the authentication logic from the external delegate.
What is the advantage of having it on BAM?
Bruno BragaUsing this pattern SGTM, I just meant to move the member from the delegate to BAM and have a single function `BAM::FillOrPreviewEntityForm()` that includes lambdas to be passed to the authenticator.
The advantage is only conceptual, but usually AED should be a thin layer between BAM and the UI. Heavy logic should rather exist in BAM.
These are two pretty light weight methods (in terms of simplicity of implementation) that can be easily reused. Every filling operation goes through the delegate, to me it makes sense conceptually that filling authentication resides here.
We could also get someones else's opinion.
Every filling operation goes through the delegate.
Same goes for the manager (no filling operation bypasses the manager)
We could also get someones else's opinion
My suggestion was a slight preference, but sure, I think @schw...@google.com was the last to refactor the CC authentication logic [here](https://chromium-review.googlesource.com/c/chromium/src/+/6242933). Chris, do you have any preference regarding where this logic should exist?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
const std::string& app_locale);nit: blank line
// Called when biometric authentication is completed.
// Triggers the `callback` if `auth_succeeded` is true.
void OnAutofillAiReauthCompleted(base::OnceClosure callback,
bool auth_succeeded);
// Authenticates the user and runs `callback` is the authentication is
// completed.
void MaybeAuthenticateBeforeFilling(base::OnceClosure callback);Bruno BragaCan we create a method `FillOrPreviewEntityForm()` and follow the same pattern used by [`FillOrPreviewCreditCardForm()`](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/foundations/browser_autofill_manager.cc;l=1941;drc=7666bc1983c2a5b98e5dc6fa6c28f8f53c07d06f)?
Jihad HannaI am following the pattern for [passwords](https://source.chromium.org/chromium/chromium/src/+/main:components/password_manager/core/browser/password_manual_fallback_flow.cc;l=420;drc=7666bc1983c2a5b98e5dc6fa6c28f8f53c07d06f;bpv=1;bpt=1). The advantage is that any other suggestion type can easily reuse the authentication logic from the external delegate.
What is the advantage of having it on BAM?
Bruno BragaUsing this pattern SGTM, I just meant to move the member from the delegate to BAM and have a single function `BAM::FillOrPreviewEntityForm()` that includes lambdas to be passed to the authenticator.
The advantage is only conceptual, but usually AED should be a thin layer between BAM and the UI. Heavy logic should rather exist in BAM.
Jihad HannaThese are two pretty light weight methods (in terms of simplicity of implementation) that can be easily reused. Every filling operation goes through the delegate, to me it makes sense conceptually that filling authentication resides here.
We could also get someones else's opinion.
Every filling operation goes through the delegate.
Same goes for the manager (no filling operation bypasses the manager)
We could also get someones else's opinion
My suggestion was a slight preference, but sure, I think @schw...@google.com was the last to refactor the CC authentication logic [here](https://chromium-review.googlesource.com/c/chromium/src/+/6242933). Chris, do you have any preference regarding where this logic should exist?
I think I agree I'd rather keep this in BAM.
My experience with these functions is that they are or were a complicated web of callbacks. Having them close to each other made it easier to reason about them and simplify them. As an example why that complexity is relevant: we had a credit card bug where a filling operation was (probably) lost somewhere in this web. We never found that bug.
(Sorry, I couldn't take a detailed look yet. I got hung up on the pointers below 😊.)
if (form_structure && autofill_field &&You've dereferenced `form_structure` in the line above 😊
base::Unretained(&manager_.get()),Why is that safe? I only took a brief look at `device_reauth::DeviceAuthenticator` but I see no (documented) guarantee that the callback isn't called after the `device_reauth::DeviceAuthenticator`'s destruction.
I think that's a case for `AM::GetWeakPtr()`. However, that returns a `WeakPtr<AM>` so you'd need a cast. So we should perhaps add a new `BAM::GetFooWeakPtr()` (no idea what a good name would be).
query_field_.global_id(), entity.as_ptr(),That's unsafe, isn't it? Assuming that the callback created by this `BindOnce()` is called asynchronously, I don't see what'd prevent `EntityDataManager` from invalidating the pointer.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
if (form_structure && autofill_field &&You've dereferenced `form_structure` in the line above 😊
If you move the logic to BAM, you can use `GetCachedFormAndField()` to get both of them at once.
query_field_.global_id(), entity.as_ptr(),That's unsafe, isn't it? Assuming that the callback created by this `BindOnce()` is called asynchronously, I don't see what'd prevent `EntityDataManager` from invalidating the pointer.
Maybe pass the GUID instead and later fetch again using EDM?
@schw...@google.com regarding the problem you mentioned. How does moving this code to BAM helps? We are going to need the same number of callbacks and calls in the stack. When debugging an issue this would boil down to the number of files we would look into.
@jihad...@google.com I still don't quite get it why it is objectively better to keep this in BAM. Can you expand?
Same goes for the manager (no filling operation bypasses the manager)
Technically data list does not, but it goes further than that. Suggestions like scan credit card, like many other, does not trigger filling. My point was mainly that the external delegate is the first entry from an user action (accepting a suggestion) to some side effect, which could be filling a form but not necessarily.
From those lens, keeping a handy authentication function at this stage makes sense to me since it can be easily reused at acceptance time.
This reason seems to trump the more subjective idea of keeping a "thin layer".
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
My slight preference towards keeping logic in BAM is because:
1) The pattern already exists in autofill (for credit cards) and is done that way, so it's more consistent with code we have in `components/autofill/` (You mentioned PWM does it differently, but then again that's a different component)
2) It keeps complex logic in BAM rather than AED. Conceptually, AED's responsibility is to route UI events to BAM and vice versa. It would be nice to keep it simple, whereas the current approach adds two methods and a state variable to it.
Of course, both the reasons aren't very concrete, which is why again it was a slight preference, maybe Chris has more reasons that me.
---
Technically data list does not
That's right, but I think that's a problem rather than a fact to build up on.
From those lens, keeping a handy authentication function at this stage makes sense to me since it can be easily reused at acceptance time.
That's a good point, but does any acceptance other than filling trigger authentication? otherwise we're in subjective land again :D
---
I think it's fine not to block the CL on this.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
const std::string& app_locale);Bruno Braganit: blank line
Done
Jihad HannaYou've dereferenced `form_structure` in the line above 😊
If you move the logic to BAM, you can use `GetCachedFormAndField()` to get both of them at once.
Done
base::Unretained(&manager_.get()),Why is that safe? I only took a brief look at `device_reauth::DeviceAuthenticator` but I see no (documented) guarantee that the callback isn't called after the `device_reauth::DeviceAuthenticator`'s destruction.
I think that's a case for `AM::GetWeakPtr()`. However, that returns a `WeakPtr<AM>` so you'd need a cast. So we should perhaps add a new `BAM::GetFooWeakPtr()` (no idea what a good name would be).
Good point, thanks a lot.
in the pinnacle of my creativity I called it `GetBrowserAutofillManagerWeakPtr`
ptal
query_field_.global_id(), entity.as_ptr(),Jihad HannaThat's unsafe, isn't it? Assuming that the callback created by this `BindOnce()` is called asynchronously, I don't see what'd prevent `EntityDataManager` from invalidating the pointer.
Maybe pass the GUID instead and later fetch again using EDM?
Yeah that is a good point. I will make this change in another CL (prior to this one)
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
LGTM % the discussion around where the authentication logic should be.
As I think it'll be easy to switch from one approach to the other, it's fine to unblock this CL for the time being until consensus is reached.
return;break (There's logic after the switch)
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
// Called when biometric authentication is completed.
// Triggers the `callback` if `auth_succeeded` is true.
void OnAutofillAiReauthCompleted(base::OnceClosure callback,
bool auth_succeeded);
// Authenticates the user and runs `callback` is the authentication is
// completed.
void MaybeAuthenticateBeforeFilling(base::OnceClosure callback);Acknowledged
break (There's logic after the switch)
Done
base::Unretained(&manager_.get()),Bruno BragaWhy is that safe? I only took a brief look at `device_reauth::DeviceAuthenticator` but I see no (documented) guarantee that the callback isn't called after the `device_reauth::DeviceAuthenticator`'s destruction.
I think that's a case for `AM::GetWeakPtr()`. However, that returns a `WeakPtr<AM>` so you'd need a cast. So we should perhaps add a new `BAM::GetFooWeakPtr()` (no idea what a good name would be).
Good point, thanks a lot.
in the pinnacle of my creativity I called it `GetBrowserAutofillManagerWeakPtr`
ptal
Done
query_field_.global_id(), entity.as_ptr(),Jihad HannaThat's unsafe, isn't it? Assuming that the callback created by this `BindOnce()` is called asynchronously, I don't see what'd prevent `EntityDataManager` from invalidating the pointer.
Bruno BragaMaybe pass the GUID instead and later fetch again using EDM?
Yeah that is a good point. I will make this change in another CL (prior to this one)
Done
authenticator_.reset();For my own education, why do we need to reset it after every filling?
If the Authentication happened there is no need to keep authenticator_ alive in memory, it can just be recreated on demand.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
authenticator_.reset();Bruno BragaFor my own education, why do we need to reset it after every filling?
If the Authentication happened there is no need to keep authenticator_ alive in memory, it can just be recreated on demand.
Done
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
13 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: components/autofill/core/browser/ui/autofill_external_delegate.cc
Insertions: 16, Deletions: 8.
@@ -36,6 +36,7 @@
#include "components/autofill/core/browser/data_manager/autofill_ai/entity_data_manager.h"
#include "components/autofill/core/browser/data_manager/payments/payments_data_manager.h"
#include "components/autofill/core/browser/data_manager/valuables/valuables_data_manager.h"
+#include "components/autofill/core/browser/data_model/autofill_ai/entity_instance.h"
#include "components/autofill/core/browser/field_type_utils.h"
#include "components/autofill/core/browser/field_types.h"
#include "components/autofill/core/browser/filling/addresses/field_filling_address_util.h"
@@ -613,10 +614,11 @@
if (EntityDataManager* edm = manager_->client().GetEntityDataManager()) {
const auto& payload =
suggestion.GetPayload<Suggestion::AutofillAiPayload>();
- if (edm->GetEntityInstance(payload.guid)) {
+ if (base::optional_ref<const EntityInstance> entity =
+ edm->GetEntityInstance(payload.guid)) {
manager_->FillOrPreviewForm(mojom::ActionPersistence::kPreview,
query_form_, query_field_.global_id(),
- payload.guid,
+ entity.as_ptr(),
AutofillTriggerSource::kAutofillAi);
}
}
@@ -807,7 +809,7 @@
const FormStructure* form_structure =
manager_->FindCachedFormById(query_form_.global_id());
if (!form_structure) {
- return;
+ break;
}
const AutofillField* autofill_field =
form_structure->GetFieldById(query_field_.global_id());
@@ -824,12 +826,18 @@
[](base::WeakPtr<BrowserAutofillManager> manager,
mojom::ActionPersistence action_persistence,
const FormData& form, const FieldGlobalId& field_id,
- const FillingPayload& filling_payload,
+ const EntityInstance::EntityId entity_id,
AutofillTriggerSource trigger_source) {
if (manager) {
- manager->FillOrPreviewForm(action_persistence, form,
- field_id, filling_payload,
- trigger_source);
+ if (EntityDataManager* edm =
+ manager->client().GetEntityDataManager()) {
+ if (base::optional_ref<const EntityInstance> entity =
+ edm->GetEntityInstance(entity_id)) {
+ manager->FillOrPreviewForm(
+ action_persistence, form, field_id,
+ entity.as_ptr(), trigger_source);
+ }
+ }
}
},
manager_->GetBrowserAutofillManagerWeakPtr(),
@@ -839,7 +847,7 @@
} else {
manager_->FillOrPreviewForm(mojom::ActionPersistence::kFill,
query_form_, query_field_.global_id(),
- payload.guid,
+ entity.as_ptr(),
AutofillTriggerSource::kAutofillAi);
}
}
```
[Autofill AI - Reauth] Reauth on filling
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |