[Autofill AI - Reauth] Reauth on filling [chromium/src : main]

0 views
Skip to first unread message

Jihad Hanna (Gerrit)

unread,
Dec 17, 2025, 6:23:34 AM (6 days ago) Dec 17
to Bruno Braga, AyeAye, Timofey Chudakov, chromium...@chromium.org, blink-...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, osaul+aut...@google.com, shgar+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
Attention needed from Bruno Braga and Timofey Chudakov

Jihad Hanna added 2 comments

File components/autofill/core/browser/ui/autofill_external_delegate.h
Line 234, Patchset 7 (Latest): // 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);
Jihad Hanna . unresolved

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)?

File components/autofill/core/browser/ui/autofill_external_delegate.cc
Open in Gerrit

Related details

Attention is currently required from:
  • Bruno Braga
  • Timofey Chudakov
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2bfa781e3517dfcde08231b52d932d85dbab472e
Gerrit-Change-Number: 7253475
Gerrit-PatchSet: 7
Gerrit-Owner: Bruno Braga <bruno...@google.com>
Gerrit-Reviewer: Jihad Hanna <jihad...@google.com>
Gerrit-Reviewer: Timofey Chudakov <tchu...@google.com>
Gerrit-Attention: Bruno Braga <bruno...@google.com>
Gerrit-Attention: Timofey Chudakov <tchu...@google.com>
Gerrit-Comment-Date: Wed, 17 Dec 2025 11:23:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Bruno Braga (Gerrit)

unread,
Dec 17, 2025, 8:03:33 AM (6 days ago) Dec 17
to AyeAye, Jihad Hanna, Timofey Chudakov, chromium...@chromium.org, blink-...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, osaul+aut...@google.com, shgar+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
Attention needed from Jihad Hanna and Timofey Chudakov

Bruno Braga added 1 comment

File components/autofill/core/browser/ui/autofill_external_delegate.h
Line 234, Patchset 7: // 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);
Jihad Hanna . unresolved

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)?

Bruno Braga

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?

Open in Gerrit

Related details

Attention is currently required from:
  • Jihad Hanna
  • Timofey Chudakov
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2bfa781e3517dfcde08231b52d932d85dbab472e
Gerrit-Change-Number: 7253475
Gerrit-PatchSet: 8
Gerrit-Owner: Bruno Braga <bruno...@google.com>
Gerrit-Reviewer: Jihad Hanna <jihad...@google.com>
Gerrit-Reviewer: Timofey Chudakov <tchu...@google.com>
Gerrit-Attention: Jihad Hanna <jihad...@google.com>
Gerrit-Attention: Timofey Chudakov <tchu...@google.com>
Gerrit-Comment-Date: Wed, 17 Dec 2025 13:03:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Jihad Hanna <jihad...@google.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Bruno Braga (Gerrit)

unread,
Dec 17, 2025, 11:12:54 AM (6 days ago) Dec 17
to AyeAye, Jihad Hanna, Timofey Chudakov, chromium...@chromium.org, blink-...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, osaul+aut...@google.com, shgar+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
Attention needed from Jihad Hanna and Timofey Chudakov

Bruno Braga added 1 comment

File components/autofill/core/browser/ui/autofill_external_delegate.cc
Line 823, Patchset 7: query_field_.global_id(), &*entity,
Jihad Hanna . resolved
Bruno Braga

Done

Gerrit-Comment-Date: Wed, 17 Dec 2025 16:12:40 +0000
satisfied_requirement
unsatisfied_requirement
open
diffy

Jihad Hanna (Gerrit)

unread,
Dec 17, 2025, 11:16:26 AM (6 days ago) Dec 17
to Bruno Braga, AyeAye, Timofey Chudakov, chromium...@chromium.org, blink-...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, osaul+aut...@google.com, shgar+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
Attention needed from Bruno Braga and Timofey Chudakov

Jihad Hanna added 1 comment

File components/autofill/core/browser/ui/autofill_external_delegate.h
Line 234, Patchset 7: // 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);
Jihad Hanna . unresolved

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)?

Bruno Braga

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?

Jihad Hanna

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.

Open in Gerrit

Related details

Attention is currently required from:
  • Bruno Braga
  • Timofey Chudakov
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I2bfa781e3517dfcde08231b52d932d85dbab472e
Gerrit-Change-Number: 7253475
Gerrit-PatchSet: 8
Gerrit-Owner: Bruno Braga <bruno...@google.com>
Gerrit-Reviewer: Jihad Hanna <jihad...@google.com>
Gerrit-Reviewer: Timofey Chudakov <tchu...@google.com>
Gerrit-Attention: Bruno Braga <bruno...@google.com>
Gerrit-Attention: Timofey Chudakov <tchu...@google.com>
Gerrit-Comment-Date: Wed, 17 Dec 2025 16:16:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Jihad Hanna <jihad...@google.com>
Comment-In-Reply-To: Bruno Braga <bruno...@google.com>
satisfied_requirement
unsatisfied_requirement
open
diffy

Timofey Chudakov (Gerrit)

unread,
Dec 18, 2025, 7:17:19 AM (5 days ago) Dec 18
to Bruno Braga, AyeAye, Jihad Hanna, chromium...@chromium.org, blink-...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, osaul+aut...@google.com, shgar+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
Attention needed from Bruno Braga

Timofey Chudakov voted and added 3 comments

Votes added by Timofey Chudakov

Code-Review+1

3 comments

Patchset-level comments
File-level comment, Patchset 9 (Latest):
Timofey Chudakov . resolved

LGTM % Jihad's comment.

File components/autofill/core/browser/ui/autofill_external_delegate.h
Line 234, Patchset 7: // 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);
Jihad Hanna . unresolved

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)?

Bruno Braga

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?

Jihad Hanna

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.

Timofey Chudakov

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).

[1] https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/touch_to_fill/password_manager/touch_to_fill_controller_autofill_delegate.cc;l=145-148;drc=7666bc1983c2a5b98e5dc6fa6c28f8f53c07d06f

File components/autofill/core/browser/ui/autofill_external_delegate.cc
Line 1435, Patchset 9 (Latest): authenticator_.reset();
Timofey Chudakov . unresolved

For my own education, why do we need to reset it after every filling?

Open in Gerrit

Related details

Attention is currently required from:
  • Bruno Braga
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Owners
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I2bfa781e3517dfcde08231b52d932d85dbab472e
    Gerrit-Change-Number: 7253475
    Gerrit-PatchSet: 9
    Gerrit-Owner: Bruno Braga <bruno...@google.com>
    Gerrit-Reviewer: Jihad Hanna <jihad...@google.com>
    Gerrit-Reviewer: Timofey Chudakov <tchu...@google.com>
    Gerrit-Attention: Bruno Braga <bruno...@google.com>
    Gerrit-Comment-Date: Thu, 18 Dec 2025 12:16:58 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Bruno Braga (Gerrit)

    unread,
    Dec 18, 2025, 7:35:14 AM (5 days ago) Dec 18
    to Timofey Chudakov, AyeAye, Jihad Hanna, chromium...@chromium.org, blink-...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, osaul+aut...@google.com, shgar+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
    Attention needed from Jihad Hanna

    Bruno Braga added 1 comment

    File components/autofill/core/browser/ui/autofill_external_delegate.h
    Line 234, Patchset 7: // 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);
    Jihad Hanna . unresolved

    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)?

    Bruno Braga

    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?

    Jihad Hanna

    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.

    Bruno Braga

    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.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Jihad Hanna
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Owners
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I2bfa781e3517dfcde08231b52d932d85dbab472e
    Gerrit-Change-Number: 7253475
    Gerrit-PatchSet: 9
    Gerrit-Owner: Bruno Braga <bruno...@google.com>
    Gerrit-Reviewer: Jihad Hanna <jihad...@google.com>
    Gerrit-Reviewer: Timofey Chudakov <tchu...@google.com>
    Gerrit-Attention: Jihad Hanna <jihad...@google.com>
    Gerrit-Comment-Date: Thu, 18 Dec 2025 12:34:52 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Jihad Hanna (Gerrit)

    unread,
    Dec 18, 2025, 7:46:50 AM (5 days ago) Dec 18
    to Bruno Braga, Timofey Chudakov, AyeAye, chromium...@chromium.org, blink-...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, osaul+aut...@google.com, shgar+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
    Attention needed from Bruno Braga and Christoph Schwering

    Jihad Hanna added 1 comment

    File components/autofill/core/browser/ui/autofill_external_delegate.h
    Line 234, Patchset 7: // 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);
    Jihad Hanna . unresolved

    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)?

    Bruno Braga

    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?

    Jihad Hanna

    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.

    Bruno Braga

    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.

    Jihad Hanna

    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?

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Bruno Braga
    • Christoph Schwering
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Owners
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I2bfa781e3517dfcde08231b52d932d85dbab472e
    Gerrit-Change-Number: 7253475
    Gerrit-PatchSet: 9
    Gerrit-Owner: Bruno Braga <bruno...@google.com>
    Gerrit-Reviewer: Jihad Hanna <jihad...@google.com>
    Gerrit-Reviewer: Timofey Chudakov <tchu...@google.com>
    Gerrit-CC: Christoph Schwering <schw...@google.com>
    Gerrit-Attention: Bruno Braga <bruno...@google.com>
    Gerrit-Attention: Christoph Schwering <schw...@google.com>
    Gerrit-Comment-Date: Thu, 18 Dec 2025 12:46:34 +0000
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Christoph Schwering (Gerrit)

    unread,
    Dec 18, 2025, 11:33:24 AM (5 days ago) Dec 18
    to Bruno Braga, Timofey Chudakov, AyeAye, Jihad Hanna, chromium...@chromium.org, blink-...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, osaul+aut...@google.com, shgar+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
    Attention needed from Bruno Braga

    Christoph Schwering added 5 comments

    File components/autofill/core/browser/filling/autofill_ai/field_filling_entity_util.h
    Line 59, Patchset 9 (Latest): const std::string& app_locale);
    Christoph Schwering . unresolved

    nit: blank line

    File components/autofill/core/browser/ui/autofill_external_delegate.h
    Line 234, Patchset 7: // 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);
    Jihad Hanna . unresolved

    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)?

    Bruno Braga

    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?

    Jihad Hanna

    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.

    Bruno Braga

    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.

    Jihad Hanna

    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?

    Christoph Schwering

    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 😊.)

    File components/autofill/core/browser/ui/autofill_external_delegate.cc
    Line 812, Patchset 9 (Latest): if (form_structure && autofill_field &&
    Christoph Schwering . unresolved

    You've dereferenced `form_structure` in the line above 😊

    Line 822, Patchset 9 (Latest): base::Unretained(&manager_.get()),
    Christoph Schwering . unresolved

    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).

    Line 824, Patchset 9 (Latest): query_field_.global_id(), entity.as_ptr(),
    Christoph Schwering . unresolved

    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.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Bruno Braga
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Owners
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I2bfa781e3517dfcde08231b52d932d85dbab472e
    Gerrit-Change-Number: 7253475
    Gerrit-PatchSet: 9
    Gerrit-Owner: Bruno Braga <bruno...@google.com>
    Gerrit-Reviewer: Jihad Hanna <jihad...@google.com>
    Gerrit-Reviewer: Timofey Chudakov <tchu...@google.com>
    Gerrit-CC: Christoph Schwering <schw...@google.com>
    Gerrit-Attention: Bruno Braga <bruno...@google.com>
    Gerrit-Comment-Date: Thu, 18 Dec 2025 16:33:07 +0000
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Jihad Hanna (Gerrit)

    unread,
    Dec 18, 2025, 12:55:04 PM (5 days ago) Dec 18
    to Bruno Braga, Timofey Chudakov, AyeAye, chromium...@chromium.org, blink-...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, osaul+aut...@google.com, shgar+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
    Attention needed from Bruno Braga

    Jihad Hanna added 2 comments

    File components/autofill/core/browser/ui/autofill_external_delegate.cc
    Line 812, Patchset 9 (Latest): if (form_structure && autofill_field &&
    Christoph Schwering . unresolved

    You've dereferenced `form_structure` in the line above 😊

    Jihad Hanna

    If you move the logic to BAM, you can use `GetCachedFormAndField()` to get both of them at once.

    Line 824, Patchset 9 (Latest): query_field_.global_id(), entity.as_ptr(),
    Christoph Schwering . unresolved

    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.

    Jihad Hanna

    Maybe pass the GUID instead and later fetch again using EDM?

    Gerrit-Comment-Date: Thu, 18 Dec 2025 17:54:42 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Christoph Schwering <schw...@google.com>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Bruno Braga (Gerrit)

    unread,
    Dec 19, 2025, 3:42:18 AM (4 days ago) Dec 19
    to Timofey Chudakov, AyeAye, Jihad Hanna, chromium...@chromium.org, blink-...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, osaul+aut...@google.com, shgar+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
    Attention needed from Christoph Schwering, Jihad Hanna and Timofey Chudakov

    Bruno Braga added 1 comment

    File components/autofill/core/browser/ui/autofill_external_delegate.h
    Bruno Braga

    @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".

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Christoph Schwering
    • Jihad Hanna
    • Timofey Chudakov
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Owners
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I2bfa781e3517dfcde08231b52d932d85dbab472e
    Gerrit-Change-Number: 7253475
    Gerrit-PatchSet: 9
    Gerrit-Owner: Bruno Braga <bruno...@google.com>
    Gerrit-Reviewer: Jihad Hanna <jihad...@google.com>
    Gerrit-Reviewer: Timofey Chudakov <tchu...@google.com>
    Gerrit-CC: Christoph Schwering <schw...@google.com>
    Gerrit-Attention: Jihad Hanna <jihad...@google.com>
    Gerrit-Attention: Timofey Chudakov <tchu...@google.com>
    Gerrit-Attention: Christoph Schwering <schw...@google.com>
    Gerrit-Comment-Date: Fri, 19 Dec 2025 08:41:57 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Jihad Hanna <jihad...@google.com>
    Comment-In-Reply-To: Bruno Braga <bruno...@google.com>
    Comment-In-Reply-To: Christoph Schwering <schw...@google.com>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Jihad Hanna (Gerrit)

    unread,
    Dec 19, 2025, 4:50:11 AM (4 days ago) Dec 19
    to Bruno Braga, Timofey Chudakov, AyeAye, chromium...@chromium.org, blink-...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, osaul+aut...@google.com, shgar+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
    Attention needed from Bruno Braga, Christoph Schwering and Timofey Chudakov

    Jihad Hanna added 1 comment

    File components/autofill/core/browser/ui/autofill_external_delegate.h
    Jihad Hanna

    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.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Bruno Braga
    • Christoph Schwering
    • Timofey Chudakov
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Owners
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I2bfa781e3517dfcde08231b52d932d85dbab472e
    Gerrit-Change-Number: 7253475
    Gerrit-PatchSet: 9
    Gerrit-Owner: Bruno Braga <bruno...@google.com>
    Gerrit-Reviewer: Jihad Hanna <jihad...@google.com>
    Gerrit-Reviewer: Timofey Chudakov <tchu...@google.com>
    Gerrit-CC: Christoph Schwering <schw...@google.com>
    Gerrit-Attention: Bruno Braga <bruno...@google.com>
    Gerrit-Attention: Timofey Chudakov <tchu...@google.com>
    Gerrit-Attention: Christoph Schwering <schw...@google.com>
    Gerrit-Comment-Date: Fri, 19 Dec 2025 09:49:53 +0000
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Bruno Braga (Gerrit)

    unread,
    Dec 19, 2025, 11:34:19 AM (4 days ago) Dec 19
    to Chromium LUCI CQ, Timofey Chudakov, AyeAye, Jihad Hanna, chromium...@chromium.org, blink-...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, osaul+aut...@google.com, shgar+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
    Attention needed from Christoph Schwering, Jihad Hanna and Timofey Chudakov

    Bruno Braga added 4 comments

    File components/autofill/core/browser/filling/autofill_ai/field_filling_entity_util.h
    Line 59, Patchset 9: const std::string& app_locale);
    Christoph Schwering . resolved

    nit: blank line

    Bruno Braga

    Done

    File components/autofill/core/browser/ui/autofill_external_delegate.cc
    Line 812, Patchset 9: if (form_structure && autofill_field &&
    Christoph Schwering . resolved

    You've dereferenced `form_structure` in the line above 😊

    Jihad Hanna

    If you move the logic to BAM, you can use `GetCachedFormAndField()` to get both of them at once.

    Bruno Braga

    Done

    Line 822, Patchset 9: base::Unretained(&manager_.get()),
    Christoph Schwering . unresolved

    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).

    Bruno Braga

    Good point, thanks a lot.

    in the pinnacle of my creativity I called it `GetBrowserAutofillManagerWeakPtr`

    ptal

    Line 824, Patchset 9: query_field_.global_id(), entity.as_ptr(),
    Christoph Schwering . unresolved

    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.

    Jihad Hanna

    Maybe pass the GUID instead and later fetch again using EDM?

    Bruno Braga

    Yeah that is a good point. I will make this change in another CL (prior to this one)

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Christoph Schwering
    • Jihad Hanna
    • Timofey Chudakov
    Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Owners
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      • requirement is not satisfiedReview-Enforcement
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I2bfa781e3517dfcde08231b52d932d85dbab472e
      Gerrit-Change-Number: 7253475
      Gerrit-PatchSet: 13
      Gerrit-Owner: Bruno Braga <bruno...@google.com>
      Gerrit-Reviewer: Bruno Braga <bruno...@google.com>
      Gerrit-Reviewer: Jihad Hanna <jihad...@google.com>
      Gerrit-Reviewer: Timofey Chudakov <tchu...@google.com>
      Gerrit-CC: Christoph Schwering <schw...@google.com>
      Gerrit-Attention: Jihad Hanna <jihad...@google.com>
      Gerrit-Attention: Timofey Chudakov <tchu...@google.com>
      Gerrit-Attention: Christoph Schwering <schw...@google.com>
      Gerrit-Comment-Date: Fri, 19 Dec 2025 16:34:02 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      Comment-In-Reply-To: Jihad Hanna <jihad...@google.com>
      Comment-In-Reply-To: Christoph Schwering <schw...@google.com>
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Jihad Hanna (Gerrit)

      unread,
      5:39 AM (2 hours ago) 5:39 AM
      to Bruno Braga, Chromium LUCI CQ, Timofey Chudakov, AyeAye, chromium...@chromium.org, blink-...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, osaul+aut...@google.com, shgar+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
      Attention needed from Bruno Braga, Christoph Schwering and Timofey Chudakov

      Jihad Hanna voted and added 2 comments

      Votes added by Jihad Hanna

      Code-Review+1

      2 comments

      Patchset-level comments
      File-level comment, Patchset 13 (Latest):
      Jihad Hanna . resolved

      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.

      File components/autofill/core/browser/ui/autofill_external_delegate.cc
      Line 810, Patchset 13 (Latest): return;
      Jihad Hanna . unresolved

      break (There's logic after the switch)

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Bruno Braga
      • Christoph Schwering
      • Timofey Chudakov
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement satisfiedCode-Owners
      • requirement satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      • requirement satisfiedReview-Enforcement
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I2bfa781e3517dfcde08231b52d932d85dbab472e
      Gerrit-Change-Number: 7253475
      Gerrit-PatchSet: 13
      Gerrit-Owner: Bruno Braga <bruno...@google.com>
      Gerrit-Reviewer: Bruno Braga <bruno...@google.com>
      Gerrit-Reviewer: Jihad Hanna <jihad...@google.com>
      Gerrit-Reviewer: Timofey Chudakov <tchu...@google.com>
      Gerrit-CC: Christoph Schwering <schw...@google.com>
      Gerrit-Attention: Bruno Braga <bruno...@google.com>
      Gerrit-Attention: Timofey Chudakov <tchu...@google.com>
      Gerrit-Attention: Christoph Schwering <schw...@google.com>
      Gerrit-Comment-Date: Tue, 23 Dec 2025 10:39:27 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: Yes
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Bruno Braga (Gerrit)

      unread,
      6:20 AM (2 hours ago) 6:20 AM
      to Jihad Hanna, Chromium LUCI CQ, Timofey Chudakov, AyeAye, chromium...@chromium.org, blink-...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, osaul+aut...@google.com, shgar+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
      Attention needed from Christoph Schwering and Timofey Chudakov

      Bruno Braga added 5 comments

      File components/autofill/core/browser/ui/autofill_external_delegate.h
      Line 234, Patchset 7: // 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);
      Jihad Hanna . resolved
      Bruno Braga

      Acknowledged

      File components/autofill/core/browser/ui/autofill_external_delegate.cc
      Line 810, Patchset 13: return;
      Jihad Hanna . resolved

      break (There's logic after the switch)

      Bruno Braga

      Done

      Line 822, Patchset 9: base::Unretained(&manager_.get()),
      Christoph Schwering . resolved

      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).

      Bruno Braga

      Good point, thanks a lot.

      in the pinnacle of my creativity I called it `GetBrowserAutofillManagerWeakPtr`

      ptal

      Bruno Braga

      Done

      Line 824, Patchset 9: query_field_.global_id(), entity.as_ptr(),
      Christoph Schwering . resolved

      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.

      Jihad Hanna

      Maybe pass the GUID instead and later fetch again using EDM?

      Bruno Braga

      Yeah that is a good point. I will make this change in another CL (prior to this one)

      Bruno Braga

      Done

      Line 1435, Patchset 9: authenticator_.reset();
      Timofey Chudakov . unresolved

      For my own education, why do we need to reset it after every filling?

      Bruno Braga

      If the Authentication happened there is no need to keep authenticator_ alive in memory, it can just be recreated on demand.

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Christoph Schwering
      • Timofey Chudakov
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement satisfiedCode-Owners
      • requirement satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      • requirement satisfiedReview-Enforcement
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I2bfa781e3517dfcde08231b52d932d85dbab472e
      Gerrit-Change-Number: 7253475
      Gerrit-PatchSet: 14
      Gerrit-Owner: Bruno Braga <bruno...@google.com>
      Gerrit-Reviewer: Bruno Braga <bruno...@google.com>
      Gerrit-Reviewer: Jihad Hanna <jihad...@google.com>
      Gerrit-Reviewer: Timofey Chudakov <tchu...@google.com>
      Gerrit-CC: Christoph Schwering <schw...@google.com>
      Gerrit-Attention: Timofey Chudakov <tchu...@google.com>
      Gerrit-Attention: Christoph Schwering <schw...@google.com>
      Gerrit-Comment-Date: Tue, 23 Dec 2025 11:19:44 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      Comment-In-Reply-To: Jihad Hanna <jihad...@google.com>
      Comment-In-Reply-To: Bruno Braga <bruno...@google.com>
      Comment-In-Reply-To: Timofey Chudakov <tchu...@google.com>
      Comment-In-Reply-To: Christoph Schwering <schw...@google.com>
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Bruno Braga (Gerrit)

      unread,
      6:20 AM (2 hours ago) 6:20 AM
      to Jihad Hanna, Chromium LUCI CQ, Timofey Chudakov, AyeAye, chromium...@chromium.org, blink-...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, osaul+aut...@google.com, shgar+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
      Attention needed from Christoph Schwering and Timofey Chudakov

      Bruno Braga added 1 comment

      File components/autofill/core/browser/ui/autofill_external_delegate.cc
      Line 1435, Patchset 9: authenticator_.reset();
      Timofey Chudakov . resolved

      For my own education, why do we need to reset it after every filling?

      Bruno Braga

      If the Authentication happened there is no need to keep authenticator_ alive in memory, it can just be recreated on demand.

      Bruno Braga

      Done

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Christoph Schwering
      • Timofey Chudakov
      Submit Requirements:
        • requirement satisfiedCode-Coverage
        • requirement satisfiedCode-Owners
        • requirement satisfiedCode-Review
        • requirement satisfiedReview-Enforcement
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: comment
        Gerrit-Project: chromium/src
        Gerrit-Branch: main
        Gerrit-Change-Id: I2bfa781e3517dfcde08231b52d932d85dbab472e
        Gerrit-Change-Number: 7253475
        Gerrit-PatchSet: 14
        Gerrit-Owner: Bruno Braga <bruno...@google.com>
        Gerrit-Reviewer: Bruno Braga <bruno...@google.com>
        Gerrit-Reviewer: Jihad Hanna <jihad...@google.com>
        Gerrit-Reviewer: Timofey Chudakov <tchu...@google.com>
        Gerrit-CC: Christoph Schwering <schw...@google.com>
        Gerrit-Attention: Timofey Chudakov <tchu...@google.com>
        Gerrit-Attention: Christoph Schwering <schw...@google.com>
        Gerrit-Comment-Date: Tue, 23 Dec 2025 11:19:52 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: No
        satisfied_requirement
        open
        diffy

        Bruno Braga (Gerrit)

        unread,
        6:22 AM (2 hours ago) 6:22 AM
        to Jihad Hanna, Chromium LUCI CQ, Timofey Chudakov, AyeAye, chromium...@chromium.org, blink-...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, osaul+aut...@google.com, shgar+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com
        Attention needed from Christoph Schwering and Timofey Chudakov

        Bruno Braga voted Commit-Queue+2

        Commit-Queue+2
        Gerrit-Comment-Date: Tue, 23 Dec 2025 11:22:29 +0000
        Gerrit-HasComments: No
        Gerrit-Has-Labels: Yes
        satisfied_requirement
        open
        diffy

        Chromium LUCI CQ (Gerrit)

        unread,
        7:29 AM (1 hour ago) 7:29 AM
        to Bruno Braga, Jihad Hanna, Timofey Chudakov, AyeAye, chromium...@chromium.org, blink-...@chromium.org, armalhotra+a...@google.com, browser-comp...@chromium.org, osaul+aut...@google.com, shgar+aut...@google.com, siashah+au...@chromium.org, siyua+aut...@chromium.org, vinnypersky+...@google.com

        Chromium LUCI CQ submitted the change with unreviewed changes

        Unreviewed changes

        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);
        }
        }
        ```

        Change information

        Commit message:
        [Autofill AI - Reauth] Reauth on filling
        Bug: 468236777
        Change-Id: I2bfa781e3517dfcde08231b52d932d85dbab472e
        Reviewed-by: Jihad Hanna <jihad...@google.com>
        Commit-Queue: Bruno Braga <bruno...@google.com>
        Cr-Commit-Position: refs/heads/main@{#1562141}
        Files:
        • M components/autofill/core/browser/filling/autofill_ai/field_filling_entity_util.cc
        • M components/autofill/core/browser/filling/autofill_ai/field_filling_entity_util.h
        • M components/autofill/core/browser/foundations/browser_autofill_manager.cc
        • M components/autofill/core/browser/foundations/browser_autofill_manager.h
        • M components/autofill/core/browser/ui/autofill_external_delegate.cc
        • M components/autofill/core/browser/ui/autofill_external_delegate.h
        • M components/autofill/core/browser/ui/autofill_external_delegate_unittest.cc
        • M components/autofill_strings.grdp
        Change size: M
        Delta: 8 files changed, 223 insertions(+), 11 deletions(-)
        Branch: refs/heads/main
        Submit Requirements:
        • requirement satisfiedCode-Review: +1 by Jihad Hanna
        Open in Gerrit
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: merged
        Gerrit-Project: chromium/src
        Gerrit-Branch: main
        Gerrit-Change-Id: I2bfa781e3517dfcde08231b52d932d85dbab472e
        Gerrit-Change-Number: 7253475
        Gerrit-PatchSet: 15
        Gerrit-Owner: Bruno Braga <bruno...@google.com>
        Gerrit-Reviewer: Bruno Braga <bruno...@google.com>
        Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
        Gerrit-Reviewer: Jihad Hanna <jihad...@google.com>
        Gerrit-Reviewer: Timofey Chudakov <tchu...@google.com>
        Gerrit-CC: Christoph Schwering <schw...@google.com>
        open
        diffy
        satisfied_requirement
        Reply all
        Reply to author
        Forward
        0 new messages