[AutofillAi - M4] Implement get/update entity on the c++ side [chromium/src : main]
Timofey Chudakov (Gerrit)
Timofey Chudakov added 5 comments![]( "Open in Gerrit")
EntityDataManagerAndroid::GetEntityInstance(const std::string& guid) {Bruno BragaPlease write Jni type converter for `EntityInstance` and `AttributeInstance`, [example](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/ui/addresses/android/autofill_address_ui_component_android.h;drc=6abb42116c6059a2c2fe94860ea3488e08644f42).
Timofey ChudakovWe do not have the equivalent for an Autofill Profile, do you know why? IIUC the advantage of what you are suggesting is that creating an Entity Instance will be easier to read since we will avoid all the boilerplate jni code and could use regular c++ instead?
We do not have the equivalent for an Autofill Profile, do you know why
It's a tech debt, the address editor was written before JNI converters were added to chrome.
The advantage is that the conversion is a implemented separately from the logic inside the `EntityDataManagerAndroid` (improves readability significantly in my opinion)
entity_data_manager().AddOrUpdateEntityInstance(This will work fine for adding a new entity instance % we call `FinalizeInfo` when the Java -> C++ conversion happens. This will reset the name structure after every conversion.
I would pull somebody familiar with the name data model in Autofill and Autofill AI specifically to validate this like @vizcay
base::android::ConvertUTF16ToJavaString(
env, !is_date ? attribute.GetCompleteInfo(app_locale) : u"");Timofey Chudakov@tchu...@google.com: Is there a more ergonomic way of doing this? Can we just pass null values instead of empty strings?
Bruno BragaYes, we should just pass strings themself and use `@JniType("std::u16string")` on the Java side. C++ doesn't have `nullptr` strings, so there won't be any `null` strings on the Java side.
Timofey ChudakovPlease take a look, this is obsolete now.
Acknowledged
EntityInstance::EntityId(Could you please annotate parameters here with `/*parameter=*/`
"", base::Time::Now(), 0, base::Time(), record_type,This will overwrite the use count and use date after every conversion.
This will work only if you retrieve an existing entity and then set all attributes there.
[for discussion] I would still pass these values to Java for consistency.
Related details
- Bruno Braga
- Jan Keitel
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. |
Bruno Braga (Gerrit)
Bruno Braga added 5 comments![]( "Open in Gerrit")
EntityDataManagerAndroid::GetEntityInstance(const std::string& guid) {Bruno BragaPlease write Jni type converter for `EntityInstance` and `AttributeInstance`, [example](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/ui/addresses/android/autofill_address_ui_component_android.h;drc=6abb42116c6059a2c2fe94860ea3488e08644f42).
We do not have the equivalent for an Autofill Profile, do you know why? IIUC the advantage of what you are suggesting is that creating an Entity Instance will be easier to read since we will avoid all the boilerplate jni code and could use regular c++ instead?
Done
entity_data_manager().AddOrUpdateEntityInstance(This will work fine for adding a new entity instance % we call `FinalizeInfo` when the Java -> C++ conversion happens. This will reset the name structure after every conversion.
I would pull somebody familiar with the name data model in Autofill and Autofill AI specifically to validate this like @vizcay
I believe we just need to call `FinalizeInfo()` for each attribute. @jihad...@google.com
base::android::ConvertUTF16ToJavaString(
env, !is_date ? attribute.GetCompleteInfo(app_locale) : u"");Timofey Chudakov@tchu...@google.com: Is there a more ergonomic way of doing this? Can we just pass null values instead of empty strings?
Bruno BragaYes, we should just pass strings themself and use `@JniType("std::u16string")` on the Java side. C++ doesn't have `nullptr` strings, so there won't be any `null` strings on the Java side.
Please take a look, this is obsolete now.
Done
Could you please annotate parameters here with `/*parameter=*/`
Done
"", base::Time::Now(), 0, base::Time(), record_type,This will overwrite the use count and use date after every conversion.
This will work only if you retrieve an existing entity and then set all attributes there.
[for discussion] I would still pass these values to Java for consistency.
This is what we want to do and is also the behaviour for desktop. If the user just edited an entity it means they are showing interest in it.
We have a frecency logic that pushes recently used entities to the top, same would happen if the user just manually corrected and entity.
Related details
- Jihad Hanna
- Timofey Chudakov
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. |
Jihad Hanna (Gerrit)
Jihad Hanna added 1 comment![]( "Open in Gerrit")
entity_data_manager().AddOrUpdateEntityInstance(Bruno BragaThis will work fine for adding a new entity instance % we call `FinalizeInfo` when the Java -> C++ conversion happens. This will reset the name structure after every conversion.
I would pull somebody familiar with the name data model in Autofill and Autofill AI specifically to validate this like @vizcay
I believe we just need to call `FinalizeInfo()` for each attribute. @jihad...@google.com
We usually call `FinalizeInfo()` when creating an entity.
When does the Java conversion happen?
Related details
- Bruno Braga
- Timofey Chudakov
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. |
Bruno Braga (Gerrit)
Bruno Braga added 1 comment![]( "Open in Gerrit")
entity_data_manager().AddOrUpdateEntityInstance(Bruno BragaThis will work fine for adding a new entity instance % we call `FinalizeInfo` when the Java -> C++ conversion happens. This will reset the name structure after every conversion.
I would pull somebody familiar with the name data model in Autofill and Autofill AI specifically to validate this like @vizcay
Jihad HannaI believe we just need to call `FinalizeInfo()` for each attribute. @jihad...@google.com
We usually call `FinalizeInfo()` when creating an entity.
When does the Java conversion happen?
We also call it when updating an entity from the settings page. Is this a problem?
The conversion from java to c++ happens in the line above:
```
EntityInstanceAndroid entity_android =
EntityInstanceAndroid::FromJavaEntityInstance(env, jEntity);
```
Related details
- Jihad Hanna
- Timofey Chudakov
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. |
Jihad Hanna (Gerrit)
Jihad Hanna added 1 comment![]( "Open in Gerrit")
entity_data_manager().AddOrUpdateEntityInstance(Bruno BragaThis will work fine for adding a new entity instance % we call `FinalizeInfo` when the Java -> C++ conversion happens. This will reset the name structure after every conversion.
I would pull somebody familiar with the name data model in Autofill and Autofill AI specifically to validate this like @vizcay
Jihad HannaI believe we just need to call `FinalizeInfo()` for each attribute. @jihad...@google.com
Bruno BragaWe usually call `FinalizeInfo()` when creating an entity.
When does the Java conversion happen?
We also call it when updating an entity from the settings page. Is this a problem?
The conversion from java to c++ happens in the line above:
```
EntityInstanceAndroid entity_android =
EntityInstanceAndroid::FromJavaEntityInstance(env, jEntity);
```
We also call it when updating an entity from the settings page. Is this a problem?
I think it's a small problem, but since [addresses do the same](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/extensions/api/autofill_private/autofill_private_api.cc;l=285;drc=7666bc1983c2a5b98e5dc6fa6c28f8f53c07d06f) I think it's something we so far accept.
I think the function you're currently implementing is supposed to also be used for settings (but correct me if I'm wrong), and in that case I agree with Bruno that we should call `FinalizeInfo()` there.
Related details
- Bruno Braga
- Timofey Chudakov
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. |
Bruno Braga (Gerrit)
Bruno Braga added 1 comment![]( "Open in Gerrit")
entity_data_manager().AddOrUpdateEntityInstance(Bruno BragaThis will work fine for adding a new entity instance % we call `FinalizeInfo` when the Java -> C++ conversion happens. This will reset the name structure after every conversion.
I would pull somebody familiar with the name data model in Autofill and Autofill AI specifically to validate this like @vizcay
Jihad HannaI believe we just need to call `FinalizeInfo()` for each attribute. @jihad...@google.com
Bruno BragaWe usually call `FinalizeInfo()` when creating an entity.
When does the Java conversion happen?
Jihad HannaWe also call it when updating an entity from the settings page. Is this a problem?
The conversion from java to c++ happens in the line above:
```
EntityInstanceAndroid entity_android =
EntityInstanceAndroid::FromJavaEntityInstance(env, jEntity);
```
We also call it when updating an entity from the settings page. Is this a problem?
I think it's a small problem, but since [addresses do the same](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/extensions/api/autofill_private/autofill_private_api.cc;l=285;drc=7666bc1983c2a5b98e5dc6fa6c28f8f53c07d06f) I think it's something we so far accept.
I think the function you're currently implementing is supposed to also be used for settings (but correct me if I'm wrong), and in that case I agree with Bruno that we should call `FinalizeInfo()` there.
- Jihad Hanna
- Timofey Chudakov
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. |
Jihad Hanna (Gerrit)
Jihad Hanna added 1 comment![]( "Open in Gerrit")
if (std::holds_alternative<AttributeInstanceAndroidDateType>(value)) {
const std::string& app_locale = g_browser_process->GetApplicationLocale();
autofill::FieldType field_type =
attribute_type.ToAttributeType().field_type();
const AttributeInstanceAndroidDateType& date_value =
std::get<AttributeInstanceAndroidDateType>(value);
instance.SetInfo(
field_type, date_value.day, app_locale,
AutofillFormatString(u"D", autofill::FormatString_Type_DATE),
VerificationStatus::kUserVerified);
instance.SetInfo(
field_type, date_value.month, app_locale,
AutofillFormatString(u"M", autofill::FormatString_Type_DATE),
VerificationStatus::kUserVerified);
instance.SetInfo(
field_type, date_value.year, app_locale,
AutofillFormatString(u"YYYY", autofill::FormatString_Type_DATE),
VerificationStatus::kUserVerified);
} else {
instance.SetRawInfo(attribute_type.ToAttributeType().field_type(),
std::get<std::u16string>(value),
VerificationStatus::kUserVerified);
}
I think this is incomplete, you should iterate over `AttributeType::field_subtypes()` and set each of them, like [in the database logic](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/webdata/autofill_ai/entity_table.cc;l=685;drc=89424e3fec685efa12c7a6f598dbf3e9ebfbcf4d)
Related details
- Bruno Braga
- Timofey Chudakov
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. |
Timofey Chudakov (Gerrit)
Timofey Chudakov added 3 comments![]( "Open in Gerrit")
entity_data_manager().AddOrUpdateEntityInstance(Bruno BragaThis will work fine for adding a new entity instance % we call `FinalizeInfo` when the Java -> C++ conversion happens. This will reset the name structure after every conversion.
I would pull somebody familiar with the name data model in Autofill and Autofill AI specifically to validate this like @vizcay
Jihad HannaI believe we just need to call `FinalizeInfo()` for each attribute. @jihad...@google.com
Bruno BragaWe usually call `FinalizeInfo()` when creating an entity.
When does the Java conversion happen?
Jihad HannaWe also call it when updating an entity from the settings page. Is this a problem?
The conversion from java to c++ happens in the line above:
```
EntityInstanceAndroid entity_android =
EntityInstanceAndroid::FromJavaEntityInstance(env, jEntity);
```
Bruno BragaWe also call it when updating an entity from the settings page. Is this a problem?
I think it's a small problem, but since [addresses do the same](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/extensions/api/autofill_private/autofill_private_api.cc;l=285;drc=7666bc1983c2a5b98e5dc6fa6c28f8f53c07d06f) I think it's something we so far accept.
I think the function you're currently implementing is supposed to also be used for settings (but correct me if I'm wrong), and in that case I agree with Bruno that we should call `FinalizeInfo()` there.
I added a TODO to handle this in a follow up.
Acknowledged
"", base::Time::Now(), 0, base::Time(), record_type,This will overwrite the use count and use date after every conversion.
This will work only if you retrieve an existing entity and then set all attributes there.
[for discussion] I would still pass these values to Java for consistency.
This is what we want to do and is also the behaviour for desktop. If the user just edited an entity it means they are showing interest in it.
We have a frecency logic that pushes recently used entities to the top, same would happen if the user just manually corrected and entity.
Okay, so we want to reset the `date_modified` and `use_date`. What about the `use_count`?
/*nickname=*/"", /*use_date=*/base::Time::Now(), /*use_count=*/0,Please fix this WARNING reported by ClangTidy: check: bugprone-argument-comment
argument name 'use_date' in comment does not m...
check: bugprone-argument-comment
argument name 'use_date' in comment does not match parameter name 'date_modified' (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html)
(Note: You can add `Skip-Clang-Tidy-Checks: bugprone-argument-comment` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel` or `mac-clang-tidy-rel`)
Related details
- Bruno Braga
- Jihad Hanna
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. |
Bruno Braga (Gerrit)
Bruno Braga added 1 comment![]( "Open in Gerrit")
if (std::holds_alternative<AttributeInstanceAndroidDateType>(value)) {
const std::string& app_locale = g_browser_process->GetApplicationLocale();
autofill::FieldType field_type =
attribute_type.ToAttributeType().field_type();
const AttributeInstanceAndroidDateType& date_value =
std::get<AttributeInstanceAndroidDateType>(value);
instance.SetInfo(
field_type, date_value.day, app_locale,
AutofillFormatString(u"D", autofill::FormatString_Type_DATE),
VerificationStatus::kUserVerified);
instance.SetInfo(
field_type, date_value.month, app_locale,
AutofillFormatString(u"M", autofill::FormatString_Type_DATE),
VerificationStatus::kUserVerified);
instance.SetInfo(
field_type, date_value.year, app_locale,
AutofillFormatString(u"YYYY", autofill::FormatString_Type_DATE),
VerificationStatus::kUserVerified);
} else {
instance.SetRawInfo(attribute_type.ToAttributeType().field_type(),
std::get<std::u16string>(value),
VerificationStatus::kUserVerified);
}
I think this is incomplete, you should iterate over `AttributeType::field_subtypes()` and set each of them, like [in the database logic](https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/webdata/autofill_ai/entity_table.cc;l=685;drc=89424e3fec685efa12c7a6f598dbf3e9ebfbcf4d)
PTAL, now I just copy the whole attribute instance if its new or was not modified
Related details
- Jihad Hanna
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. |
Jihad Hanna (Gerrit)
Jihad Hanna added 1 comment![]( "Open in Gerrit")
- Bruno Braga
- Timofey Chudakov
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. |