[iOS] Add infobar message for Autofill AI prefetch failures [chromium/src : main]
Leo Zhao (Gerrit)
Leo Zhao voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Angela Novakovic
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Angela Novakovic (Gerrit)
Angela Novakovic added 3 comments![]( "Open in Gerrit")
LGTM! I've added two optional questions/suggestions.
#if BUILDFLAG(IOS_USE_BRANDED_ASSETS)
UIImage* image = MakeSymbolMulticolor(CustomSymbolWithPointSize(
kMulticolorChromeballSymbol, kAutofillAiInfobarSymbolPointSize));
#else
UIImage* image = CustomSymbolWithPointSize(kChromeProductSymbol,
kAutofillAiInfobarSymbolPointSize);
#endif // BUILDFLAG(IOS_USE_BRANDED_ASSETS)
if (image) {
icon_ = ui::ImageModel::FromImage(gfx::Image(image));optional nit: Can we move the icon setup in a method in the namespace and call it here?
TEST_F(ChromeAutofillClientIOSTest, ShowAutofillAiPreFetchFailureNotification) {Would it be useful to add a test for when an infobar already is being shown (that is not the AUTOFILL_AI_PREFETCH_FAILURE_INFOBAR_DELEGATE_IOS), and verifying that after ````client().ShowAutofillAiPreFetchFailureNotification();```` the final infobar is of type AUTOFILL_AI_PREFETCH_FAILURE_INFOBAR_DELEGATE_IOS ?
Related details
- Leo Zhao
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. |
Leo Zhao (Gerrit)
Leo Zhao added 2 comments![]( "Open in Gerrit")
#if BUILDFLAG(IOS_USE_BRANDED_ASSETS)
UIImage* image = MakeSymbolMulticolor(CustomSymbolWithPointSize(
kMulticolorChromeballSymbol, kAutofillAiInfobarSymbolPointSize));
#else
UIImage* image = CustomSymbolWithPointSize(kChromeProductSymbol,
kAutofillAiInfobarSymbolPointSize);
#endif // BUILDFLAG(IOS_USE_BRANDED_ASSETS)
if (image) {
icon_ = ui::ImageModel::FromImage(gfx::Image(image));optional nit: Can we move the icon setup in a method in the namespace and call it here?
Done.
TEST_F(ChromeAutofillClientIOSTest, ShowAutofillAiPreFetchFailureNotification) {Would it be useful to add a test for when an infobar already is being shown (that is not the AUTOFILL_AI_PREFETCH_FAILURE_INFOBAR_DELEGATE_IOS), and verifying that after ````client().ShowAutofillAiPreFetchFailureNotification();```` the final infobar is of type AUTOFILL_AI_PREFETCH_FAILURE_INFOBAR_DELEGATE_IOS ?
Infobar popups have priority. If two infobar popups with different priorities are called at the same time, infobars should contain both. Showing them on the screen is not directly tied to the array. What we can do however here is to call it twice, and see the same infobar item is only scheduled once.
Related details
- Angela Novakovic
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Angela Novakovic (Gerrit)
Angela Novakovic added 1 comment![]( "Open in Gerrit")
LGTM!
- Leo Zhao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Angela Novakovic (Gerrit)
Angela Novakovic added 3 comments![]( "Open in Gerrit")
Small nit that I forgot to mention in my last review.
constexpr CGFloat kAutofillAiInfobarSymbolPointSize = 24.0;nit: There should be a comment above this constant.
ui::ImageModel GetPrefetchFailureIcon() {nit: There should be a comment above this method.
Related details
- Leo Zhao
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. |
Leo Zhao (Gerrit)
Leo Zhao added 2 comments![]( "Open in Gerrit")
constexpr CGFloat kAutofillAiInfobarSymbolPointSize = 24.0;nit: There should be a comment above this constant.
Done.
nit: There should be a comment above this method.
- Alexis Hétu
- Angela Novakovic
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexis Hétu (Gerrit)
Alexis Hétu added 4 comments![]( "Open in Gerrit")
AUTOFILL_AI_PREFETCH_FAILURE_INFOBAR_DELEGATE_IOS = 136,Nit: the UI string uses "PRE_FETCH" instead of "PREFETCH", so this probably should use the same two word spelling.
bool AutofillAiPrefetchFailureInfoBarDelegateIOS::Accept() {
return true;
}Do we need this? (seems like the default behaviour is the same as the default behaviour if you don't have observers)
bool AutofillAiPrefetchFailureInfoBarDelegateIOS::ShouldExpire(
const NavigationDetails& details) const {
return true;
}I think this one should be removed, or the infobar will immediately expire on ANY navigation event, including same-document navigations (like hash changes) or subframe loads. The default implementation (InfoBarDelegate::ShouldExpire) should be correct.
infobars::InfoBarDelegate::InfobarPriority
AutofillAiPrefetchFailureInfoBarDelegateIOS::GetPriority() const {
return infobars::InfoBarDelegate::InfobarPriority::kDefault;
}Isn't this the default implementation too?
Related details
- Angela Novakovic
- Leo Zhao
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. |
Leo Zhao (Gerrit)
Leo Zhao added 4 comments![]( "Open in Gerrit")
AUTOFILL_AI_PREFETCH_FAILURE_INFOBAR_DELEGATE_IOS = 136,Nit: the UI string uses "PRE_FETCH" instead of "PREFETCH", so this probably should use the same two word spelling.
I have updated this to use two word spelling. I searched `./components` and found "Prefetch" is commonly used in code. So, I am keeping the file names and class names in this CL.
bool AutofillAiPrefetchFailureInfoBarDelegateIOS::Accept() {
return true;
}Do we need this? (seems like the default behaviour is the same as the default behaviour if you don't have observers)
Removed. This is the default behavior.
bool AutofillAiPrefetchFailureInfoBarDelegateIOS::ShouldExpire(
const NavigationDetails& details) const {
return true;
}I think this one should be removed, or the infobar will immediately expire on ANY navigation event, including same-document navigations (like hash changes) or subframe loads. The default implementation (InfoBarDelegate::ShouldExpire) should be correct.
Removed.
infobars::InfoBarDelegate::InfobarPriority
AutofillAiPrefetchFailureInfoBarDelegateIOS::GetPriority() const {
return infobars::InfoBarDelegate::InfobarPriority::kDefault;
}Isn't this the default implementation too?
- Alexis Hétu
- Angela Novakovic
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexis Hétu (Gerrit)
Alexis Hétu voted Code-Review+1![]( "Open in Gerrit")
- Angela Novakovic
- Leo Zhao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leo Zhao (Gerrit)
Leo Zhao added 1 comment![]( "Open in Gerrit")
Hi Kaan, please take a look at the changes to `components/infobars/core/infobar_delegate.h`.
Related details
- Angela Novakovic
- Kaan Alsan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leo Zhao (Gerrit)
Leo Zhao added 1 comment![]( "Open in Gerrit")
Hi Joe. Please take a look at the changes to `t/m/h/m/b/enums.xml`.
Related details
- Angela Novakovic
- Joe Mason
- Kaan Alsan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kaan Alsan (Gerrit)
Kaan Alsan voted Code-Review+1![]( "Open in Gerrit")
- Angela Novakovic
- Joe Mason
- Leo Zhao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Mason (Gerrit)
Joe Mason voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Angela Novakovic
- Leo Zhao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leo Zhao (Gerrit)
Leo Zhao voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Angela Novakovic
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leo Zhao (Gerrit)
Leo Zhao voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Angela Novakovic
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
5 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: ios/chrome/browser/autofill/ui_bundled/chrome_autofill_client_ios_unittest.mm
Insertions: 1, Deletions: 1.
@@ -265,7 +265,7 @@
infobars::InfoBar* infobar = infobar_manager->infobars()[0];
EXPECT_EQ(infobar->delegate()->GetIdentifier(),
infobars::InfoBarDelegate::
- AUTOFILL_AI_PREFETCH_FAILURE_INFOBAR_DELEGATE_IOS);
+ AUTOFILL_AI_PRE_FETCH_FAILURE_INFOBAR_DELEGATE_IOS);
// Calling it again should replace the existing one, so count remains 1.
client().ShowAutofillAiPreFetchFailureNotification();
```
```
The name of the file: ios/chrome/browser/autofill/ui_bundled/chrome_autofill_client_ios.mm
Insertions: 1, Deletions: 1.
@@ -690,7 +690,7 @@
const auto existing_infobar =
std::ranges::find(infobar_manager_->infobars(),
infobars::InfoBarDelegate::
- AUTOFILL_AI_PREFETCH_FAILURE_INFOBAR_DELEGATE_IOS,
+ AUTOFILL_AI_PRE_FETCH_FAILURE_INFOBAR_DELEGATE_IOS,
&infobars::InfoBar::GetIdentifier);
if (existing_infobar != infobar_manager_->infobars().cend()) {
```
```
The name of the file: ios/chrome/browser/autofill/model/autofill_ai_prefetch_failure_infobar_delegate_ios.mm
Insertions: 1, Deletions: 1.
@@ -41,7 +41,7 @@
infobars::InfoBarDelegate::InfoBarIdentifier
AutofillAiPrefetchFailureInfoBarDelegateIOS::GetIdentifier() const {
return infobars::InfoBarDelegate::
- AUTOFILL_AI_PREFETCH_FAILURE_INFOBAR_DELEGATE_IOS;
+ AUTOFILL_AI_PRE_FETCH_FAILURE_INFOBAR_DELEGATE_IOS;
}
ui::ImageModel AutofillAiPrefetchFailureInfoBarDelegateIOS::GetIcon() const {
```
Change information
[iOS] Add infobar message for Autofill AI prefetch failures
This CL adds infobar support for Autofill AI prefetch failures which can
happen for fetching Ambient Autofill entities.
Branded Asset:
https://screenshot.googleplex.com/AVRH5qEgap6Mn5T
None Branded Asset:
https://screenshot.googleplex.com/LZxPqe4gpgiiuak
- M components/infobars/core/infobar_delegate.h
- M ios/chrome/browser/autofill/model/BUILD.gn
- A ios/chrome/browser/autofill/model/autofill_ai_prefetch_failure_infobar_delegate_ios.h
- A ios/chrome/browser/autofill/model/autofill_ai_prefetch_failure_infobar_delegate_ios.mm
- M ios/chrome/browser/autofill/ui_bundled/chrome_autofill_client_ios.h
- M ios/chrome/browser/autofill/ui_bundled/chrome_autofill_client_ios.mm
- M ios/chrome/browser/autofill/ui_bundled/chrome_autofill_client_ios_unittest.mm
- M tools/metrics/histograms/metadata/browser/enums.xml
Code-Review: +1 by Joe Mason, +1 by Kaan Alsan, +1 by Alexis Hétu
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ginny Huang (Gerrit)
Ginny Huang added 1 comment![]( "Open in Gerrit")
InfobarType::kInfobarTypeConfirm, std::move(delegate)));I've came across the autofill AI infobar when reviewing another infobar CL, and I'm wondering why haven't you used `kInfobarTypeAutofillAiSaveEntity` here? If you are doing `kInfobarTypeConfirm` here, does all the switch statement with `kInfobarTypeAutofillAiSaveEntity` even work?
Related details
- Leo Zhao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leo Zhao (Gerrit)
Leo Zhao added 1 comment![]( "Open in Gerrit")
InfobarType::kInfobarTypeConfirm, std::move(delegate)));I've came across the autofill AI infobar when reviewing another infobar CL, and I'm wondering why haven't you used `kInfobarTypeAutofillAiSaveEntity` here? If you are doing `kInfobarTypeConfirm` here, does all the switch statement with `kInfobarTypeAutofillAiSaveEntity` even work?
The `ShowAutofillAiPreFetchFailureNotification` method is showing another type of infobar message that differs from `ShowEntityImportBubble` (which uses `kInfobarTypeAutofillAiSaveEntity`).
This is showing a generic confirmation infobar message. When a user taps Got It, it disappears.
`kInfobarTypeAutofillAiSaveEntity` however, shows an infobar, and pops up a dialog when a user taps "Save", and performs a series of actions.
Yes, `kInfobarTypeAutofillAiSaveEntity` is working. There are mutilple EG tests testing its behavior. Did you notice any issues that may not be covered by EG tests?
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ginny Huang (Gerrit)
Ginny Huang added 1 comment![]( "Open in Gerrit")
InfobarType::kInfobarTypeConfirm, std::move(delegate)));Leo ZhaoI've came across the autofill AI infobar when reviewing another infobar CL, and I'm wondering why haven't you used `kInfobarTypeAutofillAiSaveEntity` here? If you are doing `kInfobarTypeConfirm` here, does all the switch statement with `kInfobarTypeAutofillAiSaveEntity` even work?
The `ShowAutofillAiPreFetchFailureNotification` method is showing another type of infobar message that differs from `ShowEntityImportBubble` (which uses `kInfobarTypeAutofillAiSaveEntity`).
This is showing a generic confirmation infobar message. When a user taps Got It, it disappears.
`kInfobarTypeAutofillAiSaveEntity` however, shows an infobar, and pops up a dialog when a user taps "Save", and performs a series of actions.
Yes, `kInfobarTypeAutofillAiSaveEntity` is working. There are mutilple EG tests testing its behavior. Did you notice any issues that may not be covered by EG tests?
I didn't see `ShowEntityImportBubble`. Ok that makes sense, thanks!
One question - should the metric "Mobile.Messages.Banner.Event.InfobarTypeAutofillAiSaveEntity" fire as a result of `ShowAutofillAiPreFetchFailureNotification`? If I understand the infobar code correctly it shouldn't. If the metric is expected to fire, you might want to double check.
Related details
- Leo Zhao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ginny Huang (Gerrit)
Ginny Huang added 1 comment![]( "Open in Gerrit")
InfobarType::kInfobarTypeConfirm, std::move(delegate)));Leo ZhaoI've came across the autofill AI infobar when reviewing another infobar CL, and I'm wondering why haven't you used `kInfobarTypeAutofillAiSaveEntity` here? If you are doing `kInfobarTypeConfirm` here, does all the switch statement with `kInfobarTypeAutofillAiSaveEntity` even work?
Ginny HuangThe `ShowAutofillAiPreFetchFailureNotification` method is showing another type of infobar message that differs from `ShowEntityImportBubble` (which uses `kInfobarTypeAutofillAiSaveEntity`).
This is showing a generic confirmation infobar message. When a user taps Got It, it disappears.
`kInfobarTypeAutofillAiSaveEntity` however, shows an infobar, and pops up a dialog when a user taps "Save", and performs a series of actions.
Yes, `kInfobarTypeAutofillAiSaveEntity` is working. There are mutilple EG tests testing its behavior. Did you notice any issues that may not be covered by EG tests?
I didn't see `ShowEntityImportBubble`. Ok that makes sense, thanks!
One question - should the metric "Mobile.Messages.Banner.Event.InfobarTypeAutofillAiSaveEntity" fire as a result of `ShowAutofillAiPreFetchFailureNotification`? If I understand the infobar code correctly it shouldn't. If the metric is expected to fire, you might want to double check.
(I mean as a result of `ShowAutofillAiPreFetchFailureNotification` - I am sure that `ShowEntityImportBubble` would have triggered that histogram)
Leo Zhao (Gerrit)
Leo Zhao added 1 comment![]( "Open in Gerrit")
InfobarType::kInfobarTypeConfirm, std::move(delegate)));Leo ZhaoI've came across the autofill AI infobar when reviewing another infobar CL, and I'm wondering why haven't you used `kInfobarTypeAutofillAiSaveEntity` here? If you are doing `kInfobarTypeConfirm` here, does all the switch statement with `kInfobarTypeAutofillAiSaveEntity` even work?
Ginny HuangThe `ShowAutofillAiPreFetchFailureNotification` method is showing another type of infobar message that differs from `ShowEntityImportBubble` (which uses `kInfobarTypeAutofillAiSaveEntity`).
This is showing a generic confirmation infobar message. When a user taps Got It, it disappears.
`kInfobarTypeAutofillAiSaveEntity` however, shows an infobar, and pops up a dialog when a user taps "Save", and performs a series of actions.
Yes, `kInfobarTypeAutofillAiSaveEntity` is working. There are mutilple EG tests testing its behavior. Did you notice any issues that may not be covered by EG tests?
Ginny HuangI didn't see `ShowEntityImportBubble`. Ok that makes sense, thanks!
One question - should the metric "Mobile.Messages.Banner.Event.InfobarTypeAutofillAiSaveEntity" fire as a result of `ShowAutofillAiPreFetchFailureNotification`? If I understand the infobar code correctly it shouldn't. If the metric is expected to fire, you might want to double check.
(I mean as a result of `ShowAutofillAiPreFetchFailureNotification` - I am sure that `ShowEntityImportBubble` would have triggered that histogram)
It is nice to know that ShowAutofillAiPreFetchFailureNotification did not trigger `Mobile.Messages.Banner.Event.InfobarTypeAutofillAiSaveEntity`. The two infobar messages serve different purposes. Prefetch notification should not trigger save entity. (Some background, `InfobarTypeAutofillAiSaveEntity` is triggered by a user submitting a form that Autofill Ai recognizes. The prefetch notification is for another project called Ambient Autofill which only happens when prefetch of a form failed).
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |