Autofill: Update MemorySearchResult to have metadata list, errors and types [chromium/src : main]
Ireneusz Szulc (Gerrit)
Ireneusz Szulc added 1 comment![]( "Open in Gerrit")
std::u16string GetEntryTypeLabel(accessibility_annotator::EntryType type) {Tanmay PatilI think this function shouldn't live here. It's too related to @memory. It could be moved to `components/autofill/core/browser/at_memory/at_memory_data_type.cc`
Ireneusz SzulcSince you suggested below, that the labels do not need to show type strings removing this completely.
Hang on, this function is needed but in other place. Right now, nothing sets value for `.type_name` and it remains empty. I'll reimplement it back.
Related details
- Christoph Schwering
- Kevin Puthusseri
- Tanmay Patil
- Xufan Xiong
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xufan Xiong (Gerrit)
Xufan Xiong added 1 comment![]( "Open in Gerrit")
virtual MemorySearchResult Query(const std::u16string& query);I see the parameter type for `Query` was changed from `std::u16string_view` to `const std::u16string&`. Was there a specific reason for this?
Related details
- Christoph Schwering
- Ireneusz Szulc
- Kevin Puthusseri
- Tanmay Patil
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. |
Kevin Puthusseri (Gerrit)
Kevin Puthusseri added 1 comment![]( "Open in Gerrit")
virtual MemorySearchResult Query(const std::u16string& query);I see the parameter type for `Query` was changed from `std::u16string_view` to `const std::u16string&`. Was there a specific reason for this?
The original query string may be deallocated while the service does async work. See https://chromium-review.git.corp.google.com/c/chromium/src/+/7658343/comment/91f3c73d_53b44280/ for more context.
Related details
- Christoph Schwering
- Ireneusz Szulc
- Tanmay Patil
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. |
Xufan Xiong (Gerrit)
Xufan Xiong added 1 comment![]( "Open in Gerrit")
virtual MemorySearchResult Query(const std::u16string& query);Kevin PuthusseriI see the parameter type for `Query` was changed from `std::u16string_view` to `const std::u16string&`. Was there a specific reason for this?
The original query string may be deallocated while the service does async work. See https://chromium-review.git.corp.google.com/c/chromium/src/+/7658343/comment/91f3c73d_53b44280/ for more context.
- Christoph Schwering
- Ireneusz Szulc
- Kevin Puthusseri
- Tanmay Patil
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Puthusseri (Gerrit)
Kevin Puthusseri added 3 comments![]( "Open in Gerrit")
kGeminiFailure, // Call to gemini via. MES failed.[nit]: remove keyword `MES`.
```suggestion
kGeminiFailure, // Call to gemini failed.
```
std::u16string type_name;@ireneu...@google.com, should this be optional? As we discussed offline, this field should only be set if entry type is unknown. Otherwise, it should be empty and we'll have some shared map that does translation between type <> type_name.
// Number"). It has to be filled with free-form text even for unknown types.Clarifying usage of this filled to specify that it should only be used if type is unknown.
```suggestion
// Number"). It has to be filled with free-form text only for unknown types.
```
Related details
- Christoph Schwering
- Ireneusz Szulc
- Tanmay Patil
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. |
Xufan Xiong (Gerrit)
Xufan Xiong added 1 comment![]( "Open in Gerrit")
LGTM. I don't have access to +1, so left a comment here.
Xufan Xiong (Gerrit)
Xufan Xiong added 1 comment![]( "Open in Gerrit")
Ireneusz Szulc (Gerrit)
Ireneusz Szulc added 3 comments![]( "Open in Gerrit")
kGeminiFailure, // Call to gemini via. MES failed.[nit]: remove keyword `MES`.
```suggestion
kGeminiFailure, // Call to gemini failed.
```
Done
std::u16string type_name;@ireneu...@google.com, should this be optional? As we discussed offline, this field should only be set if entry type is unknown. Otherwise, it should be empty and we'll have some shared map that does translation between type <> type_name.
The same point here: Can't we make this required since we always need to have the type name?
// Number"). It has to be filled with free-form text even for unknown types.Clarifying usage of this filled to specify that it should only be used if type is unknown.
```suggestion
// Number"). It has to be filled with free-form text only for unknown types.
```
Actually, my intention is the opposite. The type name is always required, while the enum type might be unknown. That's why I'd prefer the name to be pre-filled before it reaches the UI layer. I know that the `type -> name` mapping exists somewhere anyways, so it's just a matter of where we perform this evaluation. The rationale is to keep the UI decoupled from the implementation details of the underlying data sources. Whether the data originates from Autofill, Gemini, or anything else, the name can be evaluated directly at the source, potentially making use of additional context.
Related details
- Christoph Schwering
- Kevin Puthusseri
- Tanmay Patil
Kevin Puthusseri (Gerrit)
Kevin Puthusseri added 2 comments![]( "Open in Gerrit")
std::u16string type_name;Ireneusz Szulc@ireneu...@google.com, should this be optional? As we discussed offline, this field should only be set if entry type is unknown. Otherwise, it should be empty and we'll have some shared map that does translation between type <> type_name.
The same point here: Can't we make this required since we always need to have the type name?
Same point from the thread earlier.
// Number"). It has to be filled with free-form text even for unknown types.Ireneusz SzulcClarifying usage of this filled to specify that it should only be used if type is unknown.
```suggestion
// Number"). It has to be filled with free-form text only for unknown types.
```
Actually, my intention is the opposite. The type name is always required, while the enum type might be unknown. That's why I'd prefer the name to be pre-filled before it reaches the UI layer. I know that the `type -> name` mapping exists somewhere anyways, so it's just a matter of where we perform this evaluation. The rationale is to keep the UI decoupled from the implementation details of the underlying data sources. Whether the data originates from Autofill, Gemini, or anything else, the name can be evaluated directly at the source, potentially making use of additional context.
I think we agree on making things easier wherever possible. The point here was to _not_ have the memory service dictate the locale friendly type names unless absolutely required. This is something the UI should own since it should be easy to change and adapt per UX/product requirements without impacting the logic that returns the type themselves. Does that make sense? Moreover, I expect other things to be tied to the type name (such as icons).
The API here is to let consumers of the memory service know that we don't have a known type which is why, using an LLM for resolution, we're generating the name of the new type. If we always returned a known type, this string field would not be a part of the API.
Related details
- Christoph Schwering
- Ireneusz Szulc
- Tanmay Patil
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. |
Christoph Schwering (Gerrit)
Christoph Schwering added 10 comments![]( "Open in Gerrit")
for (const auto& metadata : entry.metadata_list) {nit: spell out the type
bool operator==(const MemoryEntry& other) const = default;nit: move up?
struct MemoryEntry {We have way too many structs with default constructors in //compontents/aa. So it's completely unclear what members should be and can be assumed to be set. I think this needs to be fixed asap because it's becoming a problem in code review already.
enum class EntrySource { kAutofill = 0, kGmail, kAmbient, kLiveTabs };nit: remove
// Source of the Entry (eg: kGmail).
std::vector<EntrySource> source;Singular or plural -- can there be multiple sources?
// Translates Autofill attribute names to entry types.
accessibility_annotator::QueryIntentType AttributeTypeToEntryType(
AttributeType type);
// Returns the localized name of the entry type.
std::u16string GetEntryTypeName(accessibility_annotator::QueryIntentType type);Should this live in `from_accessibility_annotator.{cc,h}`?
// Returns the localized name of the entry type.Add `ForI18n` to the function name in analogy to `EntityType::GetNameForI18n()`?
if (data_type && std::holds_alternative<EntityType>(*data_type)) {nit: `get_if`
MemoryEntry entry;Please avoid default-constructed structs.
Related details
- Ireneusz Szulc
- Tanmay Patil
Ireneusz Szulc (Gerrit)
Ireneusz Szulc added 6 comments![]( "Open in Gerrit")
for (const auto& metadata : entry.metadata_list) {nit: spell out the type
Done
std::u16string type_name;Ireneusz Szulc@ireneu...@google.com, should this be optional? As we discussed offline, this field should only be set if entry type is unknown. Otherwise, it should be empty and we'll have some shared map that does translation between type <> type_name.
Kevin PuthusseriThe same point here: Can't we make this required since we always need to have the type name?
Same point from the thread earlier.
Done
// Number"). It has to be filled with free-form text even for unknown types.Ireneusz SzulcClarifying usage of this filled to specify that it should only be used if type is unknown.
```suggestion
// Number"). It has to be filled with free-form text only for unknown types.
```
Kevin PuthusseriActually, my intention is the opposite. The type name is always required, while the enum type might be unknown. That's why I'd prefer the name to be pre-filled before it reaches the UI layer. I know that the `type -> name` mapping exists somewhere anyways, so it's just a matter of where we perform this evaluation. The rationale is to keep the UI decoupled from the implementation details of the underlying data sources. Whether the data originates from Autofill, Gemini, or anything else, the name can be evaluated directly at the source, potentially making use of additional context.
I think we agree on making things easier wherever possible. The point here was to _not_ have the memory service dictate the locale friendly type names unless absolutely required. This is something the UI should own since it should be easy to change and adapt per UX/product requirements without impacting the logic that returns the type themselves. Does that make sense? Moreover, I expect other things to be tied to the type name (such as icons).
The API here is to let consumers of the memory service know that we don't have a known type which is why, using an LLM for resolution, we're generating the name of the new type. If we always returned a known type, this string field would not be a part of the API.
As discussed offline, I'll change it to allow empty strings in the type name (though it's encouraged to fill it to improve filtering, etc.). In such case Autofill would evaluate it on UI if it's missing.
bool operator==(const MemoryEntry& other) const = default;Ireneusz Szulcnit: move up?
Done
enum class EntrySource { kAutofill = 0, kGmail, kAmbient, kLiveTabs };Ireneusz Szulcnit: remove
Done
// Source of the Entry (eg: kGmail).
std::vector<EntrySource> source;Singular or plural -- can there be multiple sources?
- Christoph Schwering
- Kevin Puthusseri
- Tanmay Patil
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. |
Ireneusz Szulc (Gerrit)
Ireneusz Szulc added 7 comments![]( "Open in Gerrit")
@schw...@google.com Friendly ping, can we get the +1 for this CL?
Acknowledged
struct MemoryEntry {We have way too many structs with default constructors in //compontents/aa. So it's completely unclear what members should be and can be assumed to be set. I think this needs to be fixed asap because it's becoming a problem in code review already.
Done
// Translates Autofill attribute names to entry types.
accessibility_annotator::QueryIntentType AttributeTypeToEntryType(
AttributeType type);
// Returns the localized name of the entry type.
std::u16string GetEntryTypeName(accessibility_annotator::QueryIntentType type);Should this live in `from_accessibility_annotator.{cc,h}`?
Okay, I can move them to components/autofill/core/browser/data_model/autofill_ai/from_accessibility_annotator.cc
// Returns the localized name of the entry type.Add `ForI18n` to the function name in analogy to `EntityType::GetNameForI18n()`?
Done
if (data_type && std::holds_alternative<EntityType>(*data_type)) {Ireneusz Szulcnit: `get_if`
Done
MemoryEntry entry;Ireneusz SzulcPlease avoid default-constructed structs.
Done
EntryMetadata metadata;- Christoph Schwering
- Tanmay Patil
- Xufan Xiong
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 11 comments![]( "Open in Gerrit")
for (const auto& metadata : entry.metadata_list) {nit: spell out the type
kFinalResponseSuccess = 0, // Final response with all data-sources.nit: remove?
Enums start with `0` by default. We usually either specify all numbers (if they matter) or none.
// Creates a data entry for a specific attribute of an Autofill AI entitynit: punctuation
MemoryEntry entry(nit: `Memory entry = MemotryEntry(...` (go/totw/88)
Same elsewhere
MemoryEntry entry(nit: add `[D]CHECK_EQ(entity.type(), attr.entity_type())`?
if (other_attr.type() == attr.type()) {Shouldn't this be `other_attr.entity_type() == entity.type()`?
(The unit tests passes `_` for the `metadata_list` of `EntityInstance`s, I think.)
entry.metadata_list = all_metadata;nit: std::move()?
entries.push_back(std::move(attr_entry));nitty nit: inline?
std::optional<AtMemoryDataType> autofill_type =Since `AutofillType` is a an existing Autofill class, maybe better call this `atmemory_type`.
nit: spell out the type
- Ireneusz Szulc
- Tanmay Patil
- Xufan Xiong
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. |
Ireneusz Szulc (Gerrit)
Ireneusz Szulc added 11 comments![]( "Open in Gerrit")
Please add a CL description
Done
for (const auto& metadata : entry.metadata_list) {nit: spell out the type
Done
kFinalResponseSuccess = 0, // Final response with all data-sources.nit: remove?
Enums start with `0` by default. We usually either specify all numbers (if they matter) or none.
Done
// Creates a data entry for a specific attribute of an Autofill AI entityIreneusz Szulcnit: punctuation
Done
nit: add `[D]CHECK_EQ(entity.type(), attr.entity_type())`?
Done
nit: `Memory entry = MemotryEntry(...` (go/totw/88)
Same elsewhere
Done
if (other_attr.type() == attr.type()) {Shouldn't this be `other_attr.entity_type() == entity.type()`?
(The unit tests passes `_` for the `metadata_list` of `EntityInstance`s, I think.)
Not really, here we want to get the other attributes from the same entity, so we're excluding the attributes with the same type as the "primary" attribute. In other words, we're looking for sibling attributes.
Anyway, that's fair point that unit tests didn't verify metadata list, I fixed this now.
entry.metadata_list = all_metadata;Ireneusz Szulcnit: std::move()?
Done
entries.push_back(std::move(attr_entry));Ireneusz Szulcnitty nit: inline?
Done
std::optional<AtMemoryDataType> autofill_type =Since `AutofillType` is a an existing Autofill class, maybe better call this `atmemory_type`.
Done
nit: spell out the type
- Christoph Schwering
- Tanmay Patil
- Xufan Xiong
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. |
Xufan Xiong (Gerrit)
Xufan Xiong added 1 comment![]( "Open in Gerrit")
LGTM. I don't have access to +1, waiting for Chris's.
Related details
- Christoph Schwering
- Ireneusz Szulc
- Tanmay Patil
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. |
Christoph Schwering (Gerrit)
Christoph Schwering voted and added 2 comments![]( "Open in Gerrit")
Votes added by Christoph Schwering
| Code-Review | +1 |
2 comments
AtMemoryDataType autofill_type);nit: `at_memory_type`
if (other_attr.type() == attr.type()) {Ireneusz SzulcShouldn't this be `other_attr.entity_type() == entity.type()`?
(The unit tests passes `_` for the `metadata_list` of `EntityInstance`s, I think.)
Not really, here we want to get the other attributes from the same entity, so we're excluding the attributes with the same type as the "primary" attribute. In other words, we're looking for sibling attributes.
Anyway, that's fair point that unit tests didn't verify metadata list, I fixed this now.
- Ireneusz Szulc
- Tanmay Patil
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. |
Dominic Battre (Gerrit)
Dominic Battre added 1 comment![]( "Open in Gerrit")
AtMemoryDataType autofill_type);nit: `at_memory_type`
Let's close this comment, land the CL and address it in a followup CL to unlock the rest of the CL chain.
Ireneusz Szulc (Gerrit)
Ireneusz Szulc voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ireneusz Szulc
| Code-Review | +1 |
1 comment
Dominic Battrenit: `at_memory_type`
Let's close this comment, land the CL and address it in a followup CL to unlock the rest of the CL chain.
- Dominic Battre
- Tanmay Patil
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ireneusz Szulc (Gerrit)
Ireneusz Szulc voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Autofill: Update MemorySearchResult to have metadata list, errors and types
This change refactors the data structures used for search results
to support structured metadata and improved status reporting.
It allows for returning partial results while the search is still
running.
The new API schema adds attribution to a data source for each individual
search result.
Flat description of a search result has transitioned into a structured
metadata list, which would allow for displaying nested results in
future.
Need for API changes:
https://docs.google.com/document/d/17tEOy0milfIAkSq043020wZC8Yx5l9qsHdJ2eG_Ao3U/edit?pli=1&tab=t.w7qpqpq7vv8v
BYPASS_LARGE_CHANGE_WARNING: CL adds boiler plate conversions
Low-Coverage-Reason: LARGE_SCALE_REFACTOR
- M chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
- M chrome/browser/ui/autofill/autofill_popup_controller_impl.h
- M chrome/browser/ui/autofill/autofill_popup_controller_impl_unittest.cc
- M chrome/browser/ui/autofill/mock_accessibility_query_service.h
- M components/accessibility_annotator/core/accessibility_query_service.cc
- M components/accessibility_annotator/core/accessibility_query_service.h
- M components/accessibility_annotator/core/accessibility_query_service_unittest.cc
- M components/accessibility_annotator/core/annotation_reducer/memory_search_result.cc
- M components/accessibility_annotator/core/annotation_reducer/memory_search_result.h
- M components/accessibility_annotator/core/annotation_reducer/query_intent_type.h
- M components/autofill/core/browser/at_memory/at_memory_data_type.cc
- M components/autofill/core/browser/at_memory/at_memory_data_type.h
- M components/autofill/core/browser/at_memory/at_memory_data_type_unittest.cc
- M components/autofill/core/browser/at_memory/autofill_data_provider_impl.cc
- M components/autofill/core/browser/at_memory/autofill_data_provider_impl.h
- M components/autofill/core/browser/at_memory/autofill_data_provider_impl_unittest.cc
- M components/autofill/core/browser/data_model/autofill_ai/from_accessibility_annotator.cc
- M components/autofill/core/browser/data_model/autofill_ai/from_accessibility_annotator.h
Code-Review: +1 by Christoph Schwering, +1 by Ireneusz Szulc
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
luci-bisection@appspot.gserviceaccount.com (Gerrit)
Message from luci-bi...@appspot.gserviceaccount.com![]( "Open in Gerrit")
LUCI Bisection has identified this change as the culprit of a build failure. See the analysis: https://ci.chromium.org/ui/p/chromium/bisection/compile-analysis/b/8686465712814222401
A revert for this change was not created because the builder that this CL broke is not watched by gardeners, therefore less important. You can consider revert this CL, fix forward or let builder owners resolve it themselves.
Sample failed build: https://ci.chromium.org/b/8686465712814222401
If this is a false positive, please report it at http://b.corp.google.com/createIssue?component=1199205&description=Analysis%3A+https%3A%2F%2Fchromium-review.googlesource.com%2Fc%2Fchromium%2Fsrc%2F%2B%2F7654073&format=PLAIN&priority=P3&title=Wrongly+blamed+https%3A%2F%2Fci.chromium.org%2Fui%2Fp%2Fchromium%2Fbisection%2Fcompile-analysis%2Fb%2F8686465712814222401&type=BUG
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
luci-bisection@appspot.gserviceaccount.com (Gerrit)
luci-bi...@appspot.gserviceaccount.com has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |