[AF] Fill or preview cards from the omnibox delegate [chromium/src : main]
Yishui Liu (Gerrit)
Yishui Liu added 1 comment![]( "Open in Gerrit")
Hello Fernando and Vinny,
Could you please take a look?
Thanks
Related details
- Fernando Ramirez
- Vinny Persky
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fernando Ramirez (Gerrit)
Fernando Ramirez voted and added 3 comments![]( "Open in Gerrit")
Votes added by Fernando Ramirez
| Code-Review | +1 |
3 comments
This looks great, thank you! Just left a clarifying question, I believe my understanding is correct, especially after looking at the new tests.
BrowserAutofillManager* OmniboxAutofillDelegate::GetManagerWithForm(
FormGlobalId form_id) {
for (AutofillDriver* driver :
client_->GetAutofillDriverFactory().GetExistingDrivers()) {
auto* manager =
static_cast<BrowserAutofillManager*>(&driver->GetAutofillManager());
if (manager->FindCachedFormById(form_id)) {
return manager;
}
}
return nullptr;
}So the previous check:
```
auto* manager = static_cast<BrowserAutofillManager*>(
client_->GetAutofillManagerForPrimaryMainFrame());
```
this would only work if the CC field was in the main frame, but would early return if it was in an iframe, for example?
If we use `GetManagerWithForm`, do we still need this in `OnGetIntersectionObserverInfo`:
```
const FormStructure* form =
manager->FindCachedFormById(trigger_form_global_id_);
if (!form) {
return;
}
```
We're already verifying that the manager has the form, so the `if (!form)` check isn't needed. Instead can we add `CHECK(manager->FindCachedFormById(trigger_form_global_id_))` here too?
if (suggestion.type != SuggestionType::kCreditCardEntry &&
suggestion.type != SuggestionType::kVirtualCreditCardEntry) {
return;
}There shouldn't be any other suggestion types here since we make sure to only display credit card and virtual card suggestions on the omnibox autofill bubble: https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/ui/payments/omnibox_autofill_delegate.cc;l=368-370;drc=5650b3485eaa8443ee812a44056a1b88b4ee0ce1
Can we add `CHECK()` here to assert that?
Related details
- Vinny Persky
- Yishui Liu
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. |
Vinny Persky (Gerrit)
Vinny Persky added 2 comments![]( "Open in Gerrit")
Adding Chris as this is an area he has a lot of expertise in
const CreditCard* card_to_fill = credit_card;
std::optional<CreditCard> virtual_card;
if (action_persistence == mojom::ActionPersistence::kFill &&
suggestion.type == SuggestionType::kVirtualCreditCardEntry) {
virtual_card = CreditCard::CreateVirtualCard(*credit_card);
card_to_fill = &*virtual_card;
}
manager->FillOrPreviewForm(action_persistence, trigger_form_global_id_,
trigger_field_global_id_, card_to_fill,
AutofillTriggerSource::kOmniboxAutofill,
/*blocked_fields=*/{});It looks like a lot of this is handled in AutofillExternalDelegate::AutofillForm(). Is there a way we can re-use that?
Related details
- Christoph Schwering
- Yishui Liu
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 2 comments![]( "Open in Gerrit")
BrowserAutofillManager* OmniboxAutofillDelegate::GetManagerWithForm(
FormGlobalId form_id) {
for (AutofillDriver* driver :
client_->GetAutofillDriverFactory().GetExistingDrivers()) {
auto* manager =
static_cast<BrowserAutofillManager*>(&driver->GetAutofillManager());
if (manager->FindCachedFormById(form_id)) {
return manager;
}
}
return nullptr;
}So the previous check:
```
auto* manager = static_cast<BrowserAutofillManager*>(
client_->GetAutofillManagerForPrimaryMainFrame());
```
this would only work if the CC field was in the main frame, but would early return if it was in an iframe, for example?If we use `GetManagerWithForm`, do we still need this in `OnGetIntersectionObserverInfo`:
```
const FormStructure* form =
manager->FindCachedFormById(trigger_form_global_id_);
if (!form) {
return;
}
```
We're already verifying that the manager has the form, so the `if (!form)` check isn't needed. Instead can we add `CHECK(manager->FindCachedFormById(trigger_form_global_id_))` here too?
IIRC the whole omnibox mechanism was intended to only consider forms of the primary main frame's AutofillManager. Why do we need this change? I don't know what the consequences are.
AutofillDriverRouter* g_active_router = nullptr;I don't know yet what the motivation is but accessing the router in here is probably wrong, and most likely there should be no global variables.
Related details
- Yishui Liu
Fernando Ramirez (Gerrit)
Fernando Ramirez added 1 comment![]( "Open in Gerrit")
Bug: 490213796I believe this should be the last CL needed to complete this task. Can this instead be "Fixed"?
Related details
- Yishui Liu
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. |
Fernando Ramirez (Gerrit)
Fernando Ramirez added 5 comments![]( "Open in Gerrit")
I believe this should be the last CL needed to complete this task. Can this instead be "Fixed"?
Done
BrowserAutofillManager* OmniboxAutofillDelegate::GetManagerWithForm(
FormGlobalId form_id) {
for (AutofillDriver* driver :
client_->GetAutofillDriverFactory().GetExistingDrivers()) {
auto* manager =
static_cast<BrowserAutofillManager*>(&driver->GetAutofillManager());
if (manager->FindCachedFormById(form_id)) {
return manager;
}
}
return nullptr;
}Christoph SchweringSo the previous check:
```
auto* manager = static_cast<BrowserAutofillManager*>(
client_->GetAutofillManagerForPrimaryMainFrame());
```
this would only work if the CC field was in the main frame, but would early return if it was in an iframe, for example?If we use `GetManagerWithForm`, do we still need this in `OnGetIntersectionObserverInfo`:
```
const FormStructure* form =
manager->FindCachedFormById(trigger_form_global_id_);
if (!form) {
return;
}
```
We're already verifying that the manager has the form, so the `if (!form)` check isn't needed. Instead can we add `CHECK(manager->FindCachedFormById(trigger_form_global_id_))` here too?
IIRC the whole omnibox mechanism was intended to only consider forms of the primary main frame's AutofillManager. Why do we need this change? I don't know what the consequences are.
You're right, we can just use the primary main frame autofill manager to find the form.
if (suggestion.type != SuggestionType::kCreditCardEntry &&
suggestion.type != SuggestionType::kVirtualCreditCardEntry) {
return;
}There shouldn't be any other suggestion types here since we make sure to only display credit card and virtual card suggestions on the omnibox autofill bubble: https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/ui/payments/omnibox_autofill_delegate.cc;l=368-370;drc=5650b3485eaa8443ee812a44056a1b88b4ee0ce1
Can we add `CHECK()` here to assert that?
Done
const CreditCard* card_to_fill = credit_card;
std::optional<CreditCard> virtual_card;
if (action_persistence == mojom::ActionPersistence::kFill &&
suggestion.type == SuggestionType::kVirtualCreditCardEntry) {
virtual_card = CreditCard::CreateVirtualCard(*credit_card);
card_to_fill = &*virtual_card;
}
manager->FillOrPreviewForm(action_persistence, trigger_form_global_id_,
trigger_field_global_id_, card_to_fill,
AutofillTriggerSource::kOmniboxAutofill,
/*blocked_fields=*/{});It looks like a lot of this is handled in AutofillExternalDelegate::AutofillForm(). Is there a way we can re-use that?
Good idea, Yishui created a helper function `FillOrPreviewCard` in `payments_util.h`.
I don't know yet what the motivation is but accessing the router in here is probably wrong, and most likely there should be no global variables.
- Christoph Schwering
- Vinny Persky
- Yishui Liu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 3 comments![]( "Open in Gerrit")
Could only take a partial look so far
std::optional<CreditCard> virtual_card;You don't a need an `optional` because `CreditCard` has a default ctor, right?
auto* manager = static_cast<BrowserAutofillManager*>(
client_->GetAutofillManagerForPrimaryMainFrame());This isn't necessarily the right one, correct?
It probably most often is the right one, so in practice it may work just fine.
But a safer option could be to store the `manager.GetWeakPtr()` where you also remember the `trigger_{form,field}_global_id_`.
Related details
- Fernando Ramirez
- Vinny Persky
- Yishui Liu
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. |
Fernando Ramirez (Gerrit)
Fernando Ramirez added 2 comments![]( "Open in Gerrit")
You don't a need an `optional` because `CreditCard` has a default ctor, right?
Technically yes, `CreditCard` has a default ctor but using `std::optional` allows us to avoid allocating a virtual `CreditCard` when not needed.
auto* manager = static_cast<BrowserAutofillManager*>(
client_->GetAutofillManagerForPrimaryMainFrame());This isn't necessarily the right one, correct?
It probably most often is the right one, so in practice it may work just fine.
But a safer option could be to store the `manager.GetWeakPtr()` where you also remember the `trigger_{form,field}_global_id_`.
Good idea, this is a better approach. I've gone ahead and stored a `trigger_autofill_manager_` and replaced all calls to `GetAutofillManagerForPrimaryMainFrame` with it. I tested the build locally and everything worked as expected.
Related details
- Christoph Schwering
- Vinny Persky
- Yishui Liu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vinny Persky (Gerrit)
Vinny Persky voted Code-Review+1![]( "Open in Gerrit")
- Christoph Schwering
- Fernando Ramirez
- Yishui Liu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fernando Ramirez (Gerrit)
Fernando Ramirez added 1 comment![]( "Open in Gerrit")
auto* manager = static_cast<BrowserAutofillManager*>(
client_->GetAutofillManagerForPrimaryMainFrame());Fernando RamirezThis isn't necessarily the right one, correct?
It probably most often is the right one, so in practice it may work just fine.
But a safer option could be to store the `manager.GetWeakPtr()` where you also remember the `trigger_{form,field}_global_id_`.
Good idea, this is a better approach. I've gone ahead and stored a `trigger_autofill_manager_` and replaced all calls to `GetAutofillManagerForPrimaryMainFrame` with it. I tested the build locally and everything worked as expected.
Re-opening this comment thread based on your last comment here: https://chromium-review.git.corp.google.com/c/chromium/src/+/8069095/comment/e8d19c9d_2456c4fd/
Related details
- Christoph Schwering
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. |
Fernando Ramirez (Gerrit)
Fernando Ramirez added 1 comment![]( "Open in Gerrit")
auto* manager = static_cast<BrowserAutofillManager*>(
client_->GetAutofillManagerForPrimaryMainFrame());Fernando RamirezThis isn't necessarily the right one, correct?
It probably most often is the right one, so in practice it may work just fine.
But a safer option could be to store the `manager.GetWeakPtr()` where you also remember the `trigger_{form,field}_global_id_`.
Fernando RamirezGood idea, this is a better approach. I've gone ahead and stored a `trigger_autofill_manager_` and replaced all calls to `GetAutofillManagerForPrimaryMainFrame` with it. I tested the build locally and everything worked as expected.
Re-opening this comment thread based on your last comment here: https://chromium-review.git.corp.google.com/c/chromium/src/+/8069095/comment/e8d19c9d_2456c4fd/
Update: based on your other comment in https://chromium-review.git.corp.google.com/c/chromium/src/+/8109166/comment/63bc7454_3419a383/
It looks like it's better to proceed with storing the `WeakPtr<AutofillManager>`.