[iOS][Forms AI] Enable the new add menu for Addresses and more [chromium/src : main]
Alexis Hétu (Gerrit)
Alexis Hétu added 1 comment![]( "Open in Gerrit")
autofill::features::kAutofillAiWithDataSchema) &&I think this feature isn't required here. Having `kAutofillAiCreateEntityDataManager` should be sufficient to view or modify entities. `kAutofillAiCreateEntityDataManager` is required to have an `_entityDataManager`, so checking `_entityDataManager` should be enough.
Related details
- Leo Zhao
- Tommy Martino
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 1 comment![]( "Open in Gerrit")
autofill::features::kAutofillAiWithDataSchema) &&I think this feature isn't required here. Having `kAutofillAiCreateEntityDataManager` should be sufficient to view or modify entities. `kAutofillAiCreateEntityDataManager` is required to have an `_entityDataManager`, so checking `_entityDataManager` should be enough.
I just checked Android code:
https://chromium.googlesource.com/chromium/src/+/main/chrome/android/java/src/org/chromium/chrome/browser/autofill/settings/AutofillProfilesFragment.java#448.
It appears that Android uses a stricter check.
The change was made yesterday at here:
https://chromium.googlesource.com/chromium/src/+/bbcd32a640f713f57bff96c4a6d45b211b8c1eef
Related details
- Alexis Hétu
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 1 comment![]( "Open in Gerrit")
Leo ZhaoI think this feature isn't required here. Having `kAutofillAiCreateEntityDataManager` should be sufficient to view or modify entities. `kAutofillAiCreateEntityDataManager` is required to have an `_entityDataManager`, so checking `_entityDataManager` should be enough.
I just checked Android code:
https://chromium.googlesource.com/chromium/src/+/main/chrome/android/java/src/org/chromium/chrome/browser/autofill/settings/AutofillProfilesFragment.java#448.
It appears that Android uses a stricter check.
The change was made yesterday at here:
https://chromium.googlesource.com/chromium/src/+/bbcd32a640f713f57bff96c4a6d45b211b8c1eef
I have updated this function using Android's check as reference. Since on iOS, the "Add" button is for adding addresses, we are not disabling it. When adding entities are not allowed, it reverts back to simply add a new address without the additional pop-up menu. So for users who are not eligible to Autofill AI, it should behave the same as before Autofill AI was added. A recording is added. What do you think?
Related details
- Alexis Hétu
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 Bruno. Can you please review `canAddEntities` in `ios/chrome/browser/settings/ui_bundled/autofill/autofill_profile_table_view_controller.mm`. It is about showing the entities in the Add menu. It has been discussed before that the user can view, delete entities as long as there are entities stored locally. What about adding new entities? The latest patchset uses a logic from what Android is using. Please take a look to see if that is the intended logic we want to go with.
Related details
- Bruno Braga
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bruno Braga (Gerrit)
Bruno Braga added 1 comment![]( "Open in Gerrit")
https://drive.google.com/file/d/1vBT3t9-S6Tdh1_XDo8lx6F0PWCANjB47/view?usp=sharingIs this really the UI we want? In other platforms we simply disable the add buttons for each entity. With this UX how is the user going to know the feature exists and that it is disabled?
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 1 comment![]( "Open in Gerrit")
Is this really the UI we want? In other platforms we simply disable the add buttons for each entity. With this UX how is the user going to know the feature exists and that it is disabled?
Are we going to disallow users adding entities based on the conditions added in the code here? If those condition checks are confirmed, then I can work with designers to see what the UI should look like for iOS.
Related details
- Bruno Braga
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bruno Braga (Gerrit)
Bruno Braga added 1 comment![]( "Open in Gerrit")
Leo ZhaoIs this really the UI we want? In other platforms we simply disable the add buttons for each entity. With this UX how is the user going to know the feature exists and that it is disabled?
Are we going to disallow users adding entities based on the conditions added in the code here? If those condition checks are confirmed, then I can work with designers to see what the UI should look like for iOS.
if an user is part of the experiment but is not eligible for some reason (address pref off, enterprise policy off, opted-out etc), we should still show them what entities they would be able to add if they correctly enrolled.
This would mean still displaying the add buttons but with a disabled style (non clickable)
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")
Leo ZhaoIs this really the UI we want? In other platforms we simply disable the add buttons for each entity. With this UX how is the user going to know the feature exists and that it is disabled?
Bruno BragaAre we going to disallow users adding entities based on the conditions added in the code here? If those condition checks are confirmed, then I can work with designers to see what the UI should look like for iOS.
if an user is part of the experiment but is not eligible for some reason (address pref off, enterprise policy off, opted-out etc), we should still show them what entities they would be able to add if they correctly enrolled.
This would mean still displaying the add buttons but with a disabled style (non clickable)
I talked with Liang. iOS has the popup style menu, it is not ideal to have 6 of the 7 button disabled. When the feature is rolled out, whether or not `kEnableOrDisable` is an allowed action is the flag to disable or hide the button, which usually means enterprise policy off, or other less frequent cases. The conclusion is that we can go with this for now, and devise a solution later if needed.
Related details
- Bruno Braga
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")
Leo ZhaoIs this really the UI we want? In other platforms we simply disable the add buttons for each entity. With this UX how is the user going to know the feature exists and that it is disabled?
Bruno BragaAre we going to disallow users adding entities based on the conditions added in the code here? If those condition checks are confirmed, then I can work with designers to see what the UI should look like for iOS.
Leo Zhaoif an user is part of the experiment but is not eligible for some reason (address pref off, enterprise policy off, opted-out etc), we should still show them what entities they would be able to add if they correctly enrolled.
This would mean still displaying the add buttons but with a disabled style (non clickable)
I talked with Liang. iOS has the popup style menu, it is not ideal to have 6 of the 7 button disabled. When the feature is rolled out, whether or not `kEnableOrDisable` is an allowed action is the flag to disable or hide the button, which usually means enterprise policy off, or other less frequent cases. The conclusion is that we can go with this for now, and devise a solution later if needed.
More discussions, this is the plan:
- when entity data manager is not available, then we skip the Add menu. Add behaves as it is today.
- when entity data manager is available, but based on the conditions adding entities is disabled, then we show Add menu, with entities items disabled.
- I will push a change later today.
Bruno Braga (Gerrit)
Bruno Braga voted and added 2 comments![]( "Open in Gerrit")
Votes added by Bruno Braga
| Code-Review | +1 |
2 comments
Hi Bruno. Can you please review `canAddEntities` in `ios/chrome/browser/settings/ui_bundled/autofill/autofill_profile_table_view_controller.mm`. It is about showing the entities in the Add menu. It has been discussed before that the user can view, delete entities as long as there are entities stored locally. What about adding new entities? The latest patchset uses a logic from what Android is using. Please take a look to see if that is the intended logic we want to go with.
Yes, the user should be able to edit or delete existing entities. Your logic looks correct.
Also double check that if the main feature flag kAutofillAiWithDataSchema is off but kAutofillAiCreateEntityDataManager is on, the user should still be able to see, edit and delete existing entities. All other feature should be unavailable.
This covers the case of a possible rollback
if (!base::FeatureList::IsEnabled(
autofill::features::kAutofillAiWithDataSchema) ||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 1 comment![]( "Open in Gerrit")
if (!base::FeatureList::IsEnabled(
autofill::features::kAutofillAiWithDataSchema) ||I have removed this additional feature check in patchset 12.
I noticed that when Autofill (Save and fill addresses) is toggled off, `autofill::GetAutofillAiOptInStatus` returns false, therefore, makes entities menu disabled, like shown in the video under "Only Entities enabled (features flags enabled + autofill disabled)". Just an observation.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Martino (Gerrit)
Tommy Martino voted and added 2 comments![]( "Open in Gerrit")
Votes added by Tommy Martino
| Code-Review | +1 |
2 comments
entitiesEnabled:(BOOL)entitiesEnabled {nit: since the only value you ever pass for this param is `[self canAddEntities]`, you can remove the param and just call `[self canAddEntities]` directly.
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 1 comment![]( "Open in Gerrit")
nit: since the only value you ever pass for this param is `[self canAddEntities]`, you can remove the param and just call `[self canAddEntities]` directly.
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
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Martino (Gerrit)
Tommy Martino voted and added 2 comments![]( "Open in Gerrit")
Votes added by Tommy Martino
| Code-Review | +1 |
2 comments
config.features_disabled.push_back(In general, this is not a great way to get tests to pass:
1. If the tests are failing because your feature breaks some behavior in the app (i.e., if we can no longer add address manually) then obviously we want to know that ASAP, and this change masks those failures.
2. If the app is fine and only the tests need to be updated, then we'll still have a ton of extra work to do when we come back to clean up the feature flag.
Since we have pretty high confidence we want to eventually launch Autofill AI, we would be better off force-*enabling* the feature and updating the test to reflect the desired final behavior. (In some cases we'll even create tests for both states, enabled and disabled, but I don't think we need to go that far in this case.)
I'm OK with it if we need to land this as-is to get other work unblocked, and then correct the tests in a follow-up. But let's try not to leave this for the cleanup phase, either.
config.features_disabled.push_back(Same here
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 voted Commit-Queue+2![]( "Open in Gerrit")
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 2 comments![]( "Open in Gerrit")
config.features_disabled.push_back(In general, this is not a great way to get tests to pass:
1. If the tests are failing because your feature breaks some behavior in the app (i.e., if we can no longer add address manually) then obviously we want to know that ASAP, and this change masks those failures.
2. If the app is fine and only the tests need to be updated, then we'll still have a ton of extra work to do when we come back to clean up the feature flag.
Since we have pretty high confidence we want to eventually launch Autofill AI, we would be better off force-*enabling* the feature and updating the test to reflect the desired final behavior. (In some cases we'll even create tests for both states, enabled and disabled, but I don't think we need to go that far in this case.)
I'm OK with it if we need to land this as-is to get other work unblocked, and then correct the tests in a follow-up. But let's try not to leave this for the cleanup phase, either.
I will create another CL to fix the tests.
config.features_disabled.push_back(Leo ZhaoSame here
I will create another CL to fix the tests.