[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>`.
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.
Fernando RamirezRe-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>`.
Closing this comment thread as I'm proceeding with storing `WeakPtr<AutofillManager>`.
Related details
- Christoph Schwering
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 Code-Review+1![]( "Open in Gerrit")
- 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")
if (actions::ActionItem* action_item = GetActionItem()) {
action_item->SetIsShowingBubble(false);
}Out of curiosity: What does this do, and why do we need it / why didn't we have it?
Is this redundant with or somehow related to the CloseBubble() call in OmniboxAutofillBubbleView::OnSuggestionAccepted(), which calls OnSuggestionAccepted()?
TEST_F(PaymentsUtilFillOrPreviewCardTest, NormalCreditCardFill) {Please add a brief comment above each test (also elsewhere). (We're doing this in Autofill because our tests too often become incomprehensible after a while.)
::testing::VariantWith<const CreditCard*>(::testing::Pointee(card_)),nit: add `using ::testing::VariantWith`.
Also, according to some TOTW, we should use fully qualify namespaces only for `using` statements (and maybe some other cases I forgot) but not in here, i.e., it should be `testing::VariantWith` if we don't add the `using` statement.
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 3 comments![]( "Open in Gerrit")
if (actions::ActionItem* action_item = GetActionItem()) {
action_item->SetIsShowingBubble(false);
}Out of curiosity: What does this do, and why do we need it / why didn't we have it?
Is this redundant with or somehow related to the CloseBubble() call in OmniboxAutofillBubbleView::OnSuggestionAccepted(), which calls OnSuggestionAccepted()?
What does this do
It keeps the state of the bubble with the omnibox chip in sync.
why do we need it / why didn't we have it?
I initially added this logic (in crrev.com/c/8094447) because if the bubble is currently shown and the user attempts to close the bubble by clicking the omnibox chip, it will close and immediately reopen (instead of staying closed).
Is this redundant with or somehow related to the CloseBubble() call in OmniboxAutofillBubbleView::OnSuggestionAccepted(), which calls OnSuggestionAccepted()?
Ah you're right, `SetIsShowingBubble(false)` is already called in `OmniboxAutofillBubbleController::OnBubbleClosed` so this is redundant. `OmniboxAutofillBubbleController::OnBubbleClosed` is eventually called after we call `CloseBubble()` in `OmniboxAutofillBubbleView::OnSuggestionAccepted`.
TEST_F(PaymentsUtilFillOrPreviewCardTest, NormalCreditCardFill) {Please add a brief comment above each test (also elsewhere). (We're doing this in Autofill because our tests too often become incomprehensible after a while.)
Done. Also added comments above each newly added test in `omnibox_autofill_delegate_unittest.cc`.
::testing::VariantWith<const CreditCard*>(::testing::Pointee(card_)),nit: add `using ::testing::VariantWith`.
Also, according to some TOTW, we should use fully qualify namespaces only for `using` statements (and maybe some other cases I forgot) but not in here, i.e., it should be `testing::VariantWith` if we don't add the `using` statement.
Also, according to some TOTW, we should use fully qualify namespaces only for using statements (and maybe some other cases I forgot) but not in here, i.e., it should be testing::VariantWith if we don't add the using statement.
Got it, thanks. I'll keep this in mind for future CLs. I'll add `using ::testing::VariantWith`. I also added `using ::testing::Pointee` and `using ::testing::Property`. Did the same in `omnibox_autofill_delegate_unittest.cc`.
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. |