Replace `IsMaskedStorageSupported` with specialized predicates [chromium/src : main]
Florian Leimgruber (Gerrit)
Florian Leimgruber added 8 comments![]( "Open in Gerrit")
Thanks! LGTM overall, except for the comment in entity_data_manager_android.cc
// TODO(crbug.com/501037715): Handle the eligibility in
// `AddOrUpdateEntityInstance` using C++ objects and drop the
// targeted_record_type parameter.I don't think that works. When we initially show the UI, we determine where we'd like to save to. But before the user confirms the save, eligiblity can change (e.g, by turning off syncing of passes in a new tab). `targeted_record_type` represents what the user accepted, while the recomputed eligibility after confirming represents the current eligibility.
Thus, the change you made below is incorrect.
// combination is asynchronous.Can you add a comment like "if saving an entity of the given (type, record_type) is not supported, the function returns false"? IIUC, this is the behavior - because kAA entities are not saveable, right?
return record_type == EntityInstance::RecordType::kServerWallet &&
!IsWalletPrivatePass(type, record_type);This would consider server orders and shipments as public passes (..which I guess is consistent with the current behavior). However, these two entity types were technically introduced after the private passes integrations and I'm not sure if they should be considered public passes. At least at the moment, we don't get any kServerWallet orders or shipments. WDYT about considering them neither private nor public passes - would that break anything?
TEST_P(AutofillEntityInstanceTest, IsWalletPrivatePassSelectTypes) {unrelated nit: Should this be "Selected"?
CHECK_EQ(entity.record_type(), EntityInstance::RecordType::kServerWallet);I think this CHECK() is still legitimate - especially if we consider order and shipments neither public nor private pass.
if (IsWalletPublicPass(*entity_type,Would an early return for `!IsWalletPrivatePass()` make more sense? Now the case where an entity type is neither public nor private is implicitly handled via the && below.
Same comment below about the `AutofillAiAction::kImportToWallet` case.
BOOL isWalletPrivatePass = autofill::IsWalletPrivatePass(Should this use `IsSavingAsynchronous()`?
Related details
- Tobiasz Ciepliński
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. |