Remove AutofillAiClient::GetManager() [chromium/src : main]
Jihad Hanna (Gerrit)
Jihad Hanna voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Florian Leimgruber
Code-Coverage
Code-Review
| 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")
tab->GetTabFeatures()->chrome_autofill_ai_client(),Is this safe? The existing code checks if `chrome_autofill_ai_client()`. And there is a [CHECK](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill_ai/core/browser/autofill_ai_manager.cc;l=196;drc=c57ed3486a328eeb84d0c8872931f20649a5743d) in `AutofillAiManager`'s constructor.
Related details
- Jihad Hanna
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jihad Hanna (Gerrit)
Jihad Hanna voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jihad Hanna
| Commit-Queue | +1 |
1 comment
tab->GetTabFeatures()->chrome_autofill_ai_client(),Is this safe? The existing code checks if `chrome_autofill_ai_client()`. And there is a [CHECK](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill_ai/core/browser/autofill_ai_manager.cc;l=196;drc=c57ed3486a328eeb84d0c8872931f20649a5743d) in `AutofillAiManager`'s constructor.
- Florian Leimgruber
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Florian Leimgruber (Gerrit)
Florian Leimgruber voted and added 2 comments![]( "Open in Gerrit")
Votes added by Florian Leimgruber
| Code-Review | +1 |
2 comments
if (tabs::TabInterface* tab = tabs::TabInterface::MaybeGetFromContents((general question) When do we prefer to initialize these things lazily in the getter vs in the constructor of the client? E.g.:
- The crowdsourcing manager is initialized lazily [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/autofill/chrome_autofill_client.cc;l=397-402;drc=997022c9de8c1e4ed9081b8789c1057d0fce0e28) because of virtual function calls.
- The form data import is initialized lazily [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/autofill/chrome_autofill_client.cc;l=619-625;drc=997022c9de8c1e4ed9081b8789c1057d0fce0e28) for no apparent reason.
- A few lines below, the `SaveUpdateAddressProfileFlowManager` is initialized eagerly.
Initializing the AiManager eagerly is closer to the current behavior. But I wonder why this is the right choice.
tab->GetTabFeatures()->chrome_autofill_ai_client(),Jihad HannaIs this safe? The existing code checks if `chrome_autofill_ai_client()`. And there is a [CHECK](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill_ai/core/browser/autofill_ai_manager.cc;l=196;drc=c57ed3486a328eeb84d0c8872931f20649a5743d) in `AutofillAiManager`'s constructor.
Nested inside a null check for the client.
- Jihad Hanna
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jihad Hanna (Gerrit)
Jihad Hanna added 1 comment![]( "Open in Gerrit")
if (tabs::TabInterface* tab = tabs::TabInterface::MaybeGetFromContents((general question) When do we prefer to initialize these things lazily in the getter vs in the constructor of the client? E.g.:
- The crowdsourcing manager is initialized lazily [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/autofill/chrome_autofill_client.cc;l=397-402;drc=997022c9de8c1e4ed9081b8789c1057d0fce0e28) because of virtual function calls.
- The form data import is initialized lazily [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/autofill/chrome_autofill_client.cc;l=619-625;drc=997022c9de8c1e4ed9081b8789c1057d0fce0e28) for no apparent reason.
- A few lines below, the `SaveUpdateAddressProfileFlowManager` is initialized eagerly.
Initializing the AiManager eagerly is closer to the current behavior. But I wonder why this is the right choice.
My **guess** is that we try to use lazy initialization when the member has a large size and otherwise we initialize in the constructor because it makes the lifetime of the object tied to that of the client which makes reasoning simpler.
Related details
- Florian Leimgruber
Code-Coverage
Code-Review
No-Unresolved-Comments
| 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")
if (tabs::TabInterface* tab = tabs::TabInterface::MaybeGetFromContents(Jihad Hanna(general question) When do we prefer to initialize these things lazily in the getter vs in the constructor of the client? E.g.:
- The crowdsourcing manager is initialized lazily [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/autofill/chrome_autofill_client.cc;l=397-402;drc=997022c9de8c1e4ed9081b8789c1057d0fce0e28) because of virtual function calls.
- The form data import is initialized lazily [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/autofill/chrome_autofill_client.cc;l=619-625;drc=997022c9de8c1e4ed9081b8789c1057d0fce0e28) for no apparent reason.
- A few lines below, the `SaveUpdateAddressProfileFlowManager` is initialized eagerly.
Initializing the AiManager eagerly is closer to the current behavior. But I wonder why this is the right choice.
My **guess** is that we try to use lazy initialization when the member has a large size and otherwise we initialize in the constructor because it makes the lifetime of the object tied to that of the client which makes reasoning simpler.
- Jihad Hanna
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jan Keitel (Gerrit)
Jan Keitel voted and added 3 comments![]( "Open in Gerrit")
Votes added by Jan Keitel
| Code-Review | +1 |
3 comments
#include "components/autofill_ai/core/browser/autofill_ai_manager.h"Please forward declare instead.
if (tabs::TabInterface* tab = tabs::TabInterface::MaybeGetFromContents(Jihad Hanna(general question) When do we prefer to initialize these things lazily in the getter vs in the constructor of the client? E.g.:
- The crowdsourcing manager is initialized lazily [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/autofill/chrome_autofill_client.cc;l=397-402;drc=997022c9de8c1e4ed9081b8789c1057d0fce0e28) because of virtual function calls.
- The form data import is initialized lazily [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/autofill/chrome_autofill_client.cc;l=619-625;drc=997022c9de8c1e4ed9081b8789c1057d0fce0e28) for no apparent reason.
- A few lines below, the `SaveUpdateAddressProfileFlowManager` is initialized eagerly.
Initializing the AiManager eagerly is closer to the current behavior. But I wonder why this is the right choice.
Florian LeimgruberMy **guess** is that we try to use lazy initialization when the member has a large size and otherwise we initialize in the constructor because it makes the lifetime of the object tied to that of the client which makes reasoning simpler.
Acknowledged
There's [guidance](https://source.chromium.org/chromium/chromium/src/+/main:docs/chrome_browser_design_principles.md) to avoid lazy instantiation. In general, I think that's probably right, but we're not really sticking to it in Autofill at the moment.
Related details
- Jihad Hanna
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jihad Hanna (Gerrit)
Jihad Hanna voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jihad Hanna
| Commit-Queue | +1 |
1 comment
#include "components/autofill_ai/core/browser/autofill_ai_manager.h"Jihad HannaPlease forward declare instead.
Done
Related details
- Florian Leimgruber
- Jan Keitel
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jihad Hanna (Gerrit)
Jihad Hanna voted Commit-Queue+1![]( "Open in Gerrit")
- Jan Keitel
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jan Keitel (Gerrit)
Jan Keitel voted Code-Review+1![]( "Open in Gerrit")
- Jihad Hanna
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jan Keitel (Gerrit)
Jan Keitel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jan Keitel
| Code-Review | +0 |
1 comment
Okay, now you have a separate issue - the destruction order of `WebContentsUserData` is not well-defined and you have one `WebContentsUserData` object that depends on data from another. This leads to a dangling pointer.
How about the following: You change the order of CLs.
- You first move `GetMqlsUploadService` to `AutofillClient`.
- You then move the AutofillAI UKM logger from the `AutofillAi` manager to `AutofillClient`.
- Next, you move and/or get rid of the AutofillAi client/manager classes.
Would that work?
Related details
- Jihad Hanna
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |