[AtMemory] Add ToPersonalContextEntity conversion utility [chromium/src : main]
Karol Sygiet (Gerrit)
Karol Sygiet added 1 comment![]( "Open in Gerrit")
case MemoryDataType::kShipmentDeliveryAddress:
case MemoryDataType::kRedressNumberFull:
case MemoryDataType::kRedressNumberName:
case MemoryDataType::kRedressNumberNumber:
case MemoryDataType::kKnownTravelerNumberExpirationDate:
case MemoryDataType::kOrderGrandTotal:Although they are AutofillAi types, the proto doesn't use them: https://source.chromium.org/chromium/chromium/src/+/main:components/personal_context/proto/features/common_data.proto;l=157;drc=458a490c2544e4ea08bb9457d8fe8f5c65af07c4
cc: @akv...@google.com
Related details
- Andrew Vuong
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. |
Karol Sygiet (Gerrit)
Karol Sygiet added 1 comment![]( "Open in Gerrit")
case MemoryDataType::kShipmentDeliveryAddress:
case MemoryDataType::kRedressNumberFull:
case MemoryDataType::kRedressNumberName:
case MemoryDataType::kRedressNumberNumber:
case MemoryDataType::kKnownTravelerNumberExpirationDate:
case MemoryDataType::kOrderGrandTotal:Moving the comment here as the file location changed:
Although they are AutofillAi types, the proto doesn't use them: https://source.chromium.org/chromium/chromium/src/+/main:components/personal_context/proto/features/common_data.proto;l=157;drc=458a490c2544e4ea08bb9457d8fe8f5c65af07c4
cc: @akv...@google.com
Related details
- Andrew Vuong
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. |
Karol Sygiet (Gerrit)
Karol Sygiet added 1 comment![]( "Open in Gerrit")
case MemoryDataType::kShipmentDeliveryAddress:
case MemoryDataType::kRedressNumberFull:
case MemoryDataType::kRedressNumberName:
case MemoryDataType::kRedressNumberNumber:
case MemoryDataType::kKnownTravelerNumberExpirationDate:
case MemoryDataType::kOrderGrandTotal:Although they are AutofillAi types, the proto doesn't use them: https://source.chromium.org/chromium/chromium/src/+/main:components/personal_context/proto/features/common_data.proto;l=157;drc=458a490c2544e4ea08bb9457d8fe8f5c65af07c4
cc: @akv...@google.com
Acknowledged
Karol Sygiet (Gerrit)
Karol Sygiet added 1 comment![]( "Open in Gerrit")
- Andrew Vuong
- 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. |
Jan Keitel (Gerrit)
Jan Keitel added 5 comments![]( "Open in Gerrit")
const std::u16string& value,
MemoryDataType memory_data_type,
const std::vector<EntryMetadata>& metadata_list);Please document what the parameters mean. In particular the `value` is obscure to me. I assume it's the value we would fill?
std::optional<personal_context::proto::Date> ParseDate(std::string_view str) {Please document.
Also, for my understanding: What guarantees the format? Are all these strings optioned from the `SerializeData` function?
const std::u16string& value,
MemoryDataType memory_data_type,
const std::vector<EntryMetadata>& metadata_list) {Nit: Prefer `std::u16string_view` and `base::span<const ...>`.
*entity.mutable_passport()->mutable_issue_date() = *date;Here and elsewhere: `std::move`.
for (std::string_view name : base::SplitStringPiece(Here and below: Can you help me understand what guarantees that the individual product names do not have "," contained in them?
Related details
- Andrew Vuong
- Karol Sygiet
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. |
Andrew Vuong (Gerrit)
Andrew Vuong added 1 comment![]( "Open in Gerrit")
case MemoryDataType::kShipmentDeliveryAddress:
case MemoryDataType::kRedressNumberFull:
case MemoryDataType::kRedressNumberName:
case MemoryDataType::kRedressNumberNumber:
case MemoryDataType::kKnownTravelerNumberExpirationDate:
case MemoryDataType::kOrderGrandTotal:Moving the comment here as the file location changed:
Andrew VuongAlthough they are AutofillAi types, the proto doesn't use them: https://source.chromium.org/chromium/chromium/src/+/main:components/personal_context/proto/features/common_data.proto;l=157;drc=458a490c2544e4ea08bb9457d8fe8f5c65af07c4
cc: @akv...@google.com
More context: `MemoryDataType::kShipmentDeliveryAddress` -> Is being deprecated for now in favor of zip_code: b/523387151 for Autofill-usecase. It will still be populated, but the quality is not guaranteed (maybe fine for AtMem tbh but needs evals).
The other 5 are not supported as structured entities server-side. We may receive them as schemaless in the future (or schemaful is we would like to push for that).
Related details
- Karol Sygiet
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. |
Karol Sygiet (Gerrit)
Karol Sygiet added 5 comments![]( "Open in Gerrit")
const std::u16string& value,
MemoryDataType memory_data_type,
const std::vector<EntryMetadata>& metadata_list);Please document what the parameters mean. In particular the `value` is obscure to me. I assume it's the value we would fill?
Does it look better now? I know that you've suggested differently in the past but IMO having the `AtMemoryPayload` as an argument here would make it easier to follow.
std::optional<personal_context::proto::Date> ParseDate(std::string_view str) {Please document.
Also, for my understanding: What guarantees the format? Are all these strings optioned from the `SerializeData` function?
Added a comment.
yes it parses whatever `SerializeAttributeValue` and thus `SerializeData` serialized.
const std::u16string& value,
MemoryDataType memory_data_type,
const std::vector<EntryMetadata>& metadata_list) {Nit: Prefer `std::u16string_view` and `base::span<const ...>`.
Done
*entity.mutable_passport()->mutable_issue_date() = *date;Here and elsewhere: `std::move`.
Done
for (std::string_view name : base::SplitStringPiece(Here and below: Can you help me understand what guarantees that the individual product names do not have "," contained in them?
There's no guarantee at the moment, but that's how they were serialized in the `SerializeAttributeValue`.
And yes the values can in theory have commas in them etc. But at the same time I'm not sure that splitting it too much will cause much trouble since the `Entity` will only be used to help PersonalContextService understand which SPII it should return to us.
Related details
- 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. |
Jan Keitel (Gerrit)
Jan Keitel voted and added 3 comments![]( "Open in Gerrit")
Votes added by Jan Keitel
| Code-Review | +1 |
3 comments
const std::u16string& value,
MemoryDataType memory_data_type,
const std::vector<EntryMetadata>& metadata_list);Karol SygietPlease document what the parameters mean. In particular the `value` is obscure to me. I assume it's the value we would fill?
Does it look better now? I know that you've suggested differently in the past but IMO having the `AtMemoryPayload` as an argument here would make it easier to follow.
Yes, I find this helpful - thank you!
std::optional<personal_context::proto::Date> ParseDate(std::string_view str) {Karol SygietPlease document.
Also, for my understanding: What guarantees the format? Are all these strings optioned from the `SerializeData` function?
Added a comment.
yes it parses whatever `SerializeAttributeValue` and thus `SerializeData` serialized.
Acknowledged
for (std::string_view name : base::SplitStringPiece(Karol SygietHere and below: Can you help me understand what guarantees that the individual product names do not have "," contained in them?
There's no guarantee at the moment, but that's how they were serialized in the `SerializeAttributeValue`.
And yes the values can in theory have commas in them etc. But at the same time I'm not sure that splitting it too much will cause much trouble since the `Entity` will only be used to help PersonalContextService understand which SPII it should return to us.
Okay, could you just leave a comment for now that explains why you are okay with the trade-off? Without knowing how the server system works, it's not clear to me whether this would cause problems or not.
Related details
- Karol Sygiet
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. |
Jan Keitel (Gerrit)
Jan Keitel added 2 comments![]( "Open in Gerrit")
"//components/personal_context/proto:personal_context_proto",Please fix this WARNING reported by autoreview issue finding: Since `memory_data_type_util.h` includes `components/personal_context/proto/features/common_data.pb.h` and uses `personal_context::proto::Entity` in its public interface, `//components/personal_context/proto:personal_context_proto` should be added to `public_deps` instead of `deps`.
This prevents GN `check` errors for targets that depend on `annotation_reducer` and include `memory_data_type_util.h` (as they will need transitive access to the proto headers).
const std::string& val) {Please fix this WARNING reported by autoreview issue finding: Optional optimization: Since protobuf 3 generates `std::string&&` overloads for string setters, you can take `val` by value (`std::string val`) and `std::move` it into the proto setters (e.g. `entity.mutable_passport()->set_number(std::move(val));`).
This would avoid copying the string out of the temporary created by `base::UTF16ToUTF8` at the call site.
Karol Sygiet (Gerrit)
Karol Sygiet added 4 comments![]( "Open in Gerrit")
"//components/personal_context/proto:personal_context_proto",Please fix this WARNING reported by autoreview issue finding: Since `memory_data_type_util.h` includes `components/personal_context/proto/features/common_data.pb.h` and uses `personal_context::proto::Entity` in its public interface, `//components/personal_context/proto:personal_context_proto` should be added to `public_deps` instead of `deps`.
This prevents GN `check` errors for targets that depend on `annotation_reducer` and include `memory_data_type_util.h` (as they will need transitive access to the proto headers).
Done
Please fix this WARNING reported by autoreview issue finding: Optional optimization: Since protobuf 3 generates `std::string&&` overloads for string setters, you can take `val` by value (`std::string val`) and `std::move` it into the proto setters (e.g. `entity.mutable_passport()->set_number(std::move(val));`).
This would avoid copying the string out of the temporary created by `base::UTF16ToUTF8` at the call site.
Done
for (std::string_view name : base::SplitStringPiece(Karol SygietHere and below: Can you help me understand what guarantees that the individual product names do not have "," contained in them?
Jan KeitelThere's no guarantee at the moment, but that's how they were serialized in the `SerializeAttributeValue`.
And yes the values can in theory have commas in them etc. But at the same time I'm not sure that splitting it too much will cause much trouble since the `Entity` will only be used to help PersonalContextService understand which SPII it should return to us.
Okay, could you just leave a comment for now that explains why you are okay with the trade-off? Without knowing how the server system works, it's not clear to me whether this would cause problems or not.
Added comments.
case MemoryDataType::kShipmentDeliveryAddress:
case MemoryDataType::kRedressNumberFull:
case MemoryDataType::kRedressNumberName:
case MemoryDataType::kRedressNumberNumber:
case MemoryDataType::kKnownTravelerNumberExpirationDate:
case MemoryDataType::kOrderGrandTotal:Andrew VuongMoving the comment here as the file location changed:
Although they are AutofillAi types, the proto doesn't use them: https://source.chromium.org/chromium/chromium/src/+/main:components/personal_context/proto/features/common_data.proto;l=157;drc=458a490c2544e4ea08bb9457d8fe8f5c65af07c4
cc: @akv...@google.com
More context: `MemoryDataType::kShipmentDeliveryAddress` -> Is being deprecated for now in favor of zip_code: b/523387151 for Autofill-usecase. It will still be populated, but the quality is not guaranteed (maybe fine for AtMem tbh but needs evals).
The other 5 are not supported as structured entities server-side. We may receive them as schemaless in the future (or schemaful is we would like to push for that).
- Andrew Vuong
- 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. |