[BnplTab] Add TabIndex to Suggestion and SuggestionFilter [chromium/src : main]
Yishui Liu (Gerrit)
Yishui Liu added 1 comment![]( "Open in Gerrit")
Hi Wilson and Vinny,
Can you take a look on this CL?
Thanks
Related details
- Vinny Persky
- Wilson Low
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yishui Liu (Gerrit)
Yishui Liu added 1 comment![]( "Open in Gerrit")
Merged recent popup_view_views change. Tests should be fixed now.
Related details
- Vinny Persky
- Wilson Low
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wilson Low (Gerrit)
Wilson Low added 5 comments![]( "Open in Gerrit")
Thanks Yishui!
[BnplTab] Add TabIndex to Suggestion and SuggestionFilternit: use "[BNPL][PNPL]" instead to be consistent with other CLs (example: crrev.com/c/7604719)
// The index of the tab that shows all Pay Later suggestions.
inline constexpr int kPayLaterTabIndex = 1;Since this isn't used in this CL yet, it might be better to remove this for now and add this constant later in the CL where we use it to set the tab index for the pay later suggestions.
inline constexpr int kDefaultTabIndex = 0;Should we define this in `suggestion.h` instead? My thought process is that this `constants.h` file specific to payments, but this default tab index will be used for all suggestions including passwords and addresses (even if it doesn't do anything for those suggestions yet)
// The index of the default suggestion tab which shows all suggestions except
// Pay Later ones.Remove for now? (after moving to suggestion.h)
Related details
- Vinny Persky
- Yishui Liu
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. |
Vinny Persky (Gerrit)
Vinny Persky added 2 comments![]( "Open in Gerrit")
using SuggestionFilter = std::variant<StringFilter, Suggestion::TabIndex>;Lets get rid of StringFilter and just do u16string here? So that we don't need to do the double dereference. I don't think we gain much readability from "StringFilter" vs "u16string"
// The index of the tab that the suggestion is shown in.Can we please add more description here? I.e. what "tab" is, mention why this is needed (in the tabbed pane case), common use cases for the tabbed pane, and that it is not always shown?
Related details
- Yishui Liu
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. |
Yishui Liu (Gerrit)
Yishui Liu added 6 comments![]( "Open in Gerrit")
nit: use "[BNPL][PNPL]" instead to be consistent with other CLs (example: crrev.com/c/7604719)
title updated
using SuggestionFilter = std::variant<StringFilter, Suggestion::TabIndex>;Lets get rid of StringFilter and just do u16string here? So that we don't need to do the double dereference. I don't think we gain much readability from "StringFilter" vs "u16string"
Updated to use u16string directly.
// The index of the tab that shows all Pay Later suggestions.
inline constexpr int kPayLaterTabIndex = 1;Since this isn't used in this CL yet, it might be better to remove this for now and add this constant later in the CL where we use it to set the tab index for the pay later suggestions.
removed constant
Should we define this in `suggestion.h` instead? My thought process is that this `constants.h` file specific to payments, but this default tab index will be used for all suggestions including passwords and addresses (even if it doesn't do anything for those suggestions yet)
moved to suggestion.h
// The index of the default suggestion tab which shows all suggestions except
// Pay Later ones.Remove for now? (after moving to suggestion.h)
Updated the description and not mentioning pay now pay later tab.
// The index of the tab that the suggestion is shown in.Can we please add more description here? I.e. what "tab" is, mention why this is needed (in the tabbed pane case), common use cases for the tabbed pane, and that it is not always shown?
- Vinny Persky
- Wilson Low
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. |
Vinny Persky (Gerrit)
Vinny Persky voted and added 2 comments![]( "Open in Gerrit")
Votes added by Vinny Persky
| Code-Review | +1 |
2 comments
using SuggestionFilter = std::variant<std::u16string, Suggestion::TabIndex>;Sorry for the back and forth.. I think actually having the alias here would be better. Can we please go back to that?
// The index of the tab that the suggestion is shown in.Yishui LiuCan we please add more description here? I.e. what "tab" is, mention why this is needed (in the tabbed pane case), common use cases for the tabbed pane, and that it is not always shown?
member description updated to include more details.
- Wilson Low
- Yishui Liu
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. |
Wilson Low (Gerrit)
Wilson Low voted and added 2 comments![]( "Open in Gerrit")
Votes added by Wilson Low
| Code-Review | +1 |
2 comments
lgtm + adding Bruno and Slobo to the review, PTAL thanks!
// The index of the default suggestion tab which shows all suggestions except
// Pay Later ones.Yishui LiuRemove for now? (after moving to suggestion.h)
Updated the description and not mentioning pay now pay later tab.
- Bruno Braga
- Slobodan Pejic
- Yishui Liu
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. |
Yishui Liu (Gerrit)
Yishui Liu added 1 comment![]( "Open in Gerrit")
using SuggestionFilter = std::variant<std::u16string, Suggestion::TabIndex>;Sorry for the back and forth.. I think actually having the alias here would be better. Can we please go back to that?
- Bruno Braga
- Slobodan Pejic
- Vinny Persky
- Wilson Low
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slobodan Pejic (Gerrit)
Slobodan Pejic added 6 comments![]( "Open in Gerrit")
using SuggestionFilter = std::variant<std::u16string, Suggestion::TabIndex>;Sorry for the back and forth.. I think actually having the alias here would be better. Can we please go back to that?
I would lean toward keeping the alias as well.
for (const Suggestion& suggestion : suggestions) {
if (suggestion.filtration_policy ==
Suggestion::FiltrationPolicy::kPresentOnlyWithoutFilter) {
continue;
} else if (suggestion.filtration_policy ==
Suggestion::FiltrationPolicy::kStatic) {
result.first.push_back(suggestion);
result.second.emplace_back();
} else if (std::holds_alternative<std::u16string>(filter)) {
std::u16string string_filter = std::get<std::u16string>(filter);
size_t pos = base::i18n::ToLower(suggestion.main_text.value)
.find(base::i18n::ToLower(string_filter));
if (pos != std::u16string::npos) {
result.first.push_back(suggestion);
result.second.push_back(AutofillPopupController::SuggestionFilterMatch{
.main_text_match = {pos, pos + string_filter.size()}});
}
}
}nit: Avoid lowercasing the string_filter inside the loop. Could this store the lowered filter into an optional<std::string> before the loop?
Something like:
```
std::optional<std::u16string> lower_string_filter =
std::holds_alternative<std::u16string>(filter)
? base::i18n::ToLower(std::get<std::u16string>(filter))
: std::nullopt;
```
// for example, the "Pay Now Pay Later" tab.This is two tabs, right?
```suggestion
// for example, the "Pay Now"/"Pay Later" tabs.
```
using TabIndex = base::StrongAlias<struct TabIndexTag, int>;nit: Consider if we want this to be unsigned to prevent us needing to consider negative tab indicies.
inline constexpr int kDefaultSuggestionTabIndex = 0;The [StrongAlias(T) constructor](https://source.chromium.org/chromium/chromium/src/+/main:base/types/strong_alias.h;l=83;drc=7c96e75b1ecd4d673eefbb4bdd11286d462aa7ee) is constexpr: Can we just have this be a TabIndex?
```suggestion
inline constexpr TabIndex kDefaultSuggestionTabIndex(0);
```
Then below:
```
-- TabIndex tab_index = TabIndex(kDefaultSuggestionTabIndex);
++ TabIndex tab_index = kDefaultSuggestionTabIndex;
```
// displayed in if not specified.```suggestion
// displayed in unless specified otherwise in Suggestion::tab_index.
```
Related details
- Bruno Braga
- Vinny Persky
- Wilson Low
- Yishui Liu
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. |
Yishui Liu (Gerrit)
Yishui Liu added 6 comments![]( "Open in Gerrit")
using SuggestionFilter = std::variant<std::u16string, Suggestion::TabIndex>;Slobodan PejicSorry for the back and forth.. I think actually having the alias here would be better. Can we please go back to that?
I would lean toward keeping the alias as well.
Already changed back to strong alias.
for (const Suggestion& suggestion : suggestions) {
if (suggestion.filtration_policy ==
Suggestion::FiltrationPolicy::kPresentOnlyWithoutFilter) {
continue;
} else if (suggestion.filtration_policy ==
Suggestion::FiltrationPolicy::kStatic) {
result.first.push_back(suggestion);
result.second.emplace_back();
} else if (std::holds_alternative<std::u16string>(filter)) {
std::u16string string_filter = std::get<std::u16string>(filter);
size_t pos = base::i18n::ToLower(suggestion.main_text.value)
.find(base::i18n::ToLower(string_filter));
if (pos != std::u16string::npos) {
result.first.push_back(suggestion);
result.second.push_back(AutofillPopupController::SuggestionFilterMatch{
.main_text_match = {pos, pos + string_filter.size()}});
}
}
}nit: Avoid lowercasing the string_filter inside the loop. Could this store the lowered filter into an optional<std::string> before the loop?
Something like:
```
std::optional<std::u16string> lower_string_filter =
std::holds_alternative<std::u16string>(filter)
? base::i18n::ToLower(std::get<std::u16string>(filter))
: std::nullopt;
```
moved the string filter conversion out from the loop.
// for example, the "Pay Now Pay Later" tab.This is two tabs, right?
```suggestion
// for example, the "Pay Now"/"Pay Later" tabs.
```
That's correct. Comments updated.
using TabIndex = base::StrongAlias<struct TabIndexTag, int>;nit: Consider if we want this to be unsigned to prevent us needing to consider negative tab indicies.
This index filter will be called by [TabbedPaneListener::TabSelectedAt](https://source.chromium.org/chromium/chromium/src/+/main:ui/views/controls/tabbed_pane/tabbed_pane_listener.h;bpv=1;bpt=1;l=17?q=tabbedpaneli&ss=chromium%2Fchromium%2Fsrc&gsn=TabSelectedAt&gs=KYTHE%3A%2F%2Fkythe%3A%2F%2Fchromium.googlesource.com%2Fcodesearch%2Fchromium%2Fsrc%2F%2Fmain%3Flang%3Dc%252B%252B%3Fpath%3Dui%2Fviews%2Fcontrols%2Ftabbed_pane%2Ftabbed_pane_listener.h%23EjnUb3Kt1vNmnvBD93jcf0kCOlTn5GAirvz1F1EnVkc) implementation which takes an int index value.
We will need to cast the value if we want to use unsigned int here, which may be unnecessary.
Besides, a negative index filter will just lead to empty suggestion list if we do not add any suggestion with negative tab index.
inline constexpr int kDefaultSuggestionTabIndex = 0;The [StrongAlias(T) constructor](https://source.chromium.org/chromium/chromium/src/+/main:base/types/strong_alias.h;l=83;drc=7c96e75b1ecd4d673eefbb4bdd11286d462aa7ee) is constexpr: Can we just have this be a TabIndex?
```suggestion
inline constexpr TabIndex kDefaultSuggestionTabIndex(0);
```
Then below:
```
-- TabIndex tab_index = TabIndex(kDefaultSuggestionTabIndex);
++ TabIndex tab_index = kDefaultSuggestionTabIndex;
```
Renamed `TabIndex` to `SuggestionTabIndex` since it needs to moved to autofill scope for this and defined the constant as `SuggestionTabIndex` directly.
// displayed in if not specified.```suggestion
// displayed in unless specified otherwise in Suggestion::tab_index.
```
- Bruno Braga
- Slobodan Pejic
- Vinny Persky
- Wilson Low
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. |
Slobodan Pejic (Gerrit)
Slobodan Pejic voted and added 2 comments![]( "Open in Gerrit")
Votes added by Slobodan Pejic
| Code-Review | +1 |
2 comments
using TabIndex = base::StrongAlias<struct TabIndexTag, int>;Yishui Liunit: Consider if we want this to be unsigned to prevent us needing to consider negative tab indicies.
This index filter will be called by [TabbedPaneListener::TabSelectedAt](https://source.chromium.org/chromium/chromium/src/+/main:ui/views/controls/tabbed_pane/tabbed_pane_listener.h;bpv=1;bpt=1;l=17?q=tabbedpaneli&ss=chromium%2Fchromium%2Fsrc&gsn=TabSelectedAt&gs=KYTHE%3A%2F%2Fkythe%3A%2F%2Fchromium.googlesource.com%2Fcodesearch%2Fchromium%2Fsrc%2F%2Fmain%3Flang%3Dc%252B%252B%3Fpath%3Dui%2Fviews%2Fcontrols%2Ftabbed_pane%2Ftabbed_pane_listener.h%23EjnUb3Kt1vNmnvBD93jcf0kCOlTn5GAirvz1F1EnVkc) implementation which takes an int index value.
We will need to cast the value if we want to use unsigned int here, which may be unnecessary.
Besides, a negative index filter will just lead to empty suggestion list if we do not add any suggestion with negative tab index.
Ack. Let's keep it consistent with TabSelectedAt.
inline constexpr int kDefaultSuggestionTabIndex = 0;Yishui LiuThe [StrongAlias(T) constructor](https://source.chromium.org/chromium/chromium/src/+/main:base/types/strong_alias.h;l=83;drc=7c96e75b1ecd4d673eefbb4bdd11286d462aa7ee) is constexpr: Can we just have this be a TabIndex?
```suggestion
inline constexpr TabIndex kDefaultSuggestionTabIndex(0);
```
Then below:
```
-- TabIndex tab_index = TabIndex(kDefaultSuggestionTabIndex);
++ TabIndex tab_index = kDefaultSuggestionTabIndex;
```
Renamed `TabIndex` to `SuggestionTabIndex` since it needs to moved to autofill scope for this and defined the constant as `SuggestionTabIndex` directly.
- Bruno Braga
- Vinny Persky
- Wilson Low
- Yishui Liu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vinny Persky (Gerrit)
Vinny Persky voted Code-Review+1![]( "Open in Gerrit")
- Bruno Braga
- Wilson Low
- Yishui Liu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wilson Low (Gerrit)
Wilson Low voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Bruno Braga
- Yishui Liu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wilson Low (Gerrit)
Wilson Low added 1 comment![]( "Open in Gerrit")
Adding Stephen to the review, PTAL thanks!
Related details
- Bruno Braga
- Stephen McGruer
- Yishui Liu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen McGruer (Gerrit)
Stephen McGruer added 9 comments![]( "Open in Gerrit")
While I am generally a proponent of small CLs, I actually think this CL does too little. Without the context of *how* this new integer tab index concept is going to be used in practice, it is hard to consider whether this "works" in terms of good architectural shape for Autofill or not. In any case, I do have some concepts, in comments below.
This CL add a int member to `Suggestion` to filter suggestion forSuper-nit: 'an', albeit I'm not sure you really need 'int' here and therefore could keep it as 'adds a member to...'
This CL add a int member to `Suggestion` to filter suggestion forSuper-nit: 'suggestions' (plural)
This CL add a int member to `Suggestion` to filter suggestion forSuper-nit: 'adds'
different tab.It is not clear to a reader what a 'tab' is. Immediately I thought of a browser tab, because that is the most common 'tab' concept in our world. Please update this to be clear that it refers to a tab on the autofill pop-up dialog.
Filtering logic for tab index will be added in the next CL.Super-nit: 'for the tab index'
using SuggestionFilter = std::variant<StringFilter, SuggestionTabIndex>;Architecturally this feels like it is combining disparate use-cases to me. One case is actually *filtering* the set of suggestions based on the user input. The other instead is a UI question of indicating/*selecting which UI surface* a particular suggestion should be displayed on. It seems entirely reasonable for both to be present at the same time, thus we shouldn't have an either/or relationship?
I'm actually surprised that the existing filtering happens at the AutofillPopupControllerImpl level and not in the original Suggestion generation. I assume this is to allow it to react as more characters are typed without requiring re-generation of Suggestions? If that is the reason, then it feels like even more reason that the SuggestionTabIndex concept is not a filter (or at least not the same either/or filter as the string one), and instead is a static UI concept of where a Suggestion should be visually displayed.
I think these should be different concepts at the code level, albeit I am interested to know what brunobraga@'s opinion is.
// Note: Suggestions typically appear in a single popup bubble.This sentence isn't true, Suggestions appear in many different ways of which one is a popup bubble.
// The index of the tab that the suggestion will be shown in.Again I am hit with this concept of "we are saying tab without a reader necessarily having any clue what that is". I think we need to find the right "central" place to define this (maybe here, maybe its at the tab_index member point) and explain what a tab is in this context.
Related details
- Bruno Braga
- Yishui Liu
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. |
Vinny Persky (Gerrit)
Vinny Persky added 1 comment![]( "Open in Gerrit")
using SuggestionFilter = std::variant<StringFilter, SuggestionTabIndex>;Architecturally this feels like it is combining disparate use-cases to me. One case is actually *filtering* the set of suggestions based on the user input. The other instead is a UI question of indicating/*selecting which UI surface* a particular suggestion should be displayed on. It seems entirely reasonable for both to be present at the same time, thus we shouldn't have an either/or relationship?
I'm actually surprised that the existing filtering happens at the AutofillPopupControllerImpl level and not in the original Suggestion generation. I assume this is to allow it to react as more characters are typed without requiring re-generation of Suggestions? If that is the reason, then it feels like even more reason that the SuggestionTabIndex concept is not a filter (or at least not the same either/or filter as the string one), and instead is a static UI concept of where a Suggestion should be visually displayed.
I think these should be different concepts at the code level, albeit I am interested to know what brunobraga@'s opinion is.
I can also see the case for them being similar - based on a user input/action, change which suggestions we are showing to the user. Clicking a tab or typing in a string can both be lumped together as user actions.
Agreed, it's not highly unreasonable for them to be together in the future. But for practical purposes, we had an infrastructure where, based on X user action, we can filter which suggestions to show the user, which lined up nicely with what we were looking to do for this project and lets us build on an existing infra rather than building something new. I don't think lumping these together since they're both user actions is unreasonable either.
In the future, if we wanted them both to coexist, it would take a bit of extending this infrastructure but I don't think it would be a huge amount of work. I'd rather not build something new to handle a case that might never exist, especially when we can re-use.
>I'm actually surprised that the existing filtering happens at the AutofillPopupControllerImpl level and not in the original Suggestion generation. I assume this is to allow it to react as more characters are typed without requiring re-generation of Suggestions? If that is the reason, then it feels like even more reason that the SuggestionTabIndex concept is not a filter (or at least not the same either/or filter as the string one), and instead is a static UI concept of where a Suggestion should be visually displayed.
Yes but we still need to do this at the popup controller level because it's a UI click that the user makes to change which suggestions to display.
If people want to go in a different direction, I'm not opposed, but since this approach has been aligned (between Slobo, Bruno and I, as well as Jan when I ran this approach by him before he went OOO), and is very close to being implemented (and isn't a huge lift in the codebase), can we go ahead with this as we re-align and figure out what is preferred? This CL is blocking a lot of the future CLs for this project
Vinny Persky (Gerrit)
Vinny Persky added 1 comment![]( "Open in Gerrit")
using SuggestionFilter = std::variant<StringFilter, SuggestionTabIndex>;Vinny PerskyArchitecturally this feels like it is combining disparate use-cases to me. One case is actually *filtering* the set of suggestions based on the user input. The other instead is a UI question of indicating/*selecting which UI surface* a particular suggestion should be displayed on. It seems entirely reasonable for both to be present at the same time, thus we shouldn't have an either/or relationship?
I'm actually surprised that the existing filtering happens at the AutofillPopupControllerImpl level and not in the original Suggestion generation. I assume this is to allow it to react as more characters are typed without requiring re-generation of Suggestions? If that is the reason, then it feels like even more reason that the SuggestionTabIndex concept is not a filter (or at least not the same either/or filter as the string one), and instead is a static UI concept of where a Suggestion should be visually displayed.
I think these should be different concepts at the code level, albeit I am interested to know what brunobraga@'s opinion is.
I can also see the case for them being similar - based on a user input/action, change which suggestions we are showing to the user. Clicking a tab or typing in a string can both be lumped together as user actions.
Agreed, it's not highly unreasonable for them to be together in the future. But for practical purposes, we had an infrastructure where, based on X user action, we can filter which suggestions to show the user, which lined up nicely with what we were looking to do for this project and lets us build on an existing infra rather than building something new. I don't think lumping these together since they're both user actions is unreasonable either.
In the future, if we wanted them both to coexist, it would take a bit of extending this infrastructure but I don't think it would be a huge amount of work. I'd rather not build something new to handle a case that might never exist, especially when we can re-use.
>I'm actually surprised that the existing filtering happens at the AutofillPopupControllerImpl level and not in the original Suggestion generation. I assume this is to allow it to react as more characters are typed without requiring re-generation of Suggestions? If that is the reason, then it feels like even more reason that the SuggestionTabIndex concept is not a filter (or at least not the same either/or filter as the string one), and instead is a static UI concept of where a Suggestion should be visually displayed.
Yes but we still need to do this at the popup controller level because it's a UI click that the user makes to change which suggestions to display.
If people want to go in a different direction, I'm not opposed, but since this approach has been aligned (between Slobo, Bruno and I, as well as Jan when I ran this approach by him before he went OOO), and is very close to being implemented (and isn't a huge lift in the codebase), can we go ahead with this as we re-align and figure out what is preferred? This CL is blocking a lot of the future CLs for this project
Though FWIW, I have a preference for this approach so we don't end up with two different ways of changing which suggestions are shown to users.
Yishui Liu (Gerrit)
Yishui Liu added 8 comments![]( "Open in Gerrit")
While I am generally a proponent of small CLs, I actually think this CL does too little. Without the context of *how* this new integer tab index concept is going to be used in practice, it is hard to consider whether this "works" in terms of good architectural shape for Autofill or not. In any case, I do have some concepts, in comments below.
I meant to have this CL more like a refactoring to only adds the support for taking int value as suggestion filter without changing the current flow, and the next CL adding the logic for handling the new filter.
This CL add a int member to `Suggestion` to filter suggestion forYishui LiuSuper-nit: 'adds'
Description updated.
This CL add a int member to `Suggestion` to filter suggestion forYishui LiuSuper-nit: 'suggestions' (plural)
Description updated.
This CL add a int member to `Suggestion` to filter suggestion forSuper-nit: 'an', albeit I'm not sure you really need 'int' here and therefore could keep it as 'adds a member to...'
Description updated.
It is not clear to a reader what a 'tab' is. Immediately I thought of a browser tab, because that is the most common 'tab' concept in our world. Please update this to be clear that it refers to a tab on the autofill pop-up dialog.
Added more detail about where the tabs locate
Filtering logic for tab index will be added in the next CL.Super-nit: 'for the tab index'
description updated.
// Note: Suggestions typically appear in a single popup bubble.This sentence isn't true, Suggestions appear in many different ways of which one is a popup bubble.
Updated the comment to only mention suggestions are normally shown in the same list.
// The index of the tab that the suggestion will be shown in.Again I am hit with this concept of "we are saying tab without a reader necessarily having any clue what that is". I think we need to find the right "central" place to define this (maybe here, maybe its at the tab_index member point) and explain what a tab is in this context.
I move this line here so that we can define the default index constant directly in the strong alias type. We can move this line down with the `tab_index` member, but then the constant will need to be defined as int, and we will need to convert it to the alias type when we use it.
Added more detail about the index type. Please let me know if you still think moving this line with the `tab_index` member is more readable.
Related details
- Bruno Braga
- Stephen McGruer
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Bruno Braga
| Code-Review | +1 |
1 comment
using SuggestionFilter = std::variant<StringFilter, SuggestionTabIndex>;Vinny PerskyArchitecturally this feels like it is combining disparate use-cases to me. One case is actually *filtering* the set of suggestions based on the user input. The other instead is a UI question of indicating/*selecting which UI surface* a particular suggestion should be displayed on. It seems entirely reasonable for both to be present at the same time, thus we shouldn't have an either/or relationship?
I'm actually surprised that the existing filtering happens at the AutofillPopupControllerImpl level and not in the original Suggestion generation. I assume this is to allow it to react as more characters are typed without requiring re-generation of Suggestions? If that is the reason, then it feels like even more reason that the SuggestionTabIndex concept is not a filter (or at least not the same either/or filter as the string one), and instead is a static UI concept of where a Suggestion should be visually displayed.
I think these should be different concepts at the code level, albeit I am interested to know what brunobraga@'s opinion is.
Vinny PerskyI can also see the case for them being similar - based on a user input/action, change which suggestions we are showing to the user. Clicking a tab or typing in a string can both be lumped together as user actions.
Agreed, it's not highly unreasonable for them to be together in the future. But for practical purposes, we had an infrastructure where, based on X user action, we can filter which suggestions to show the user, which lined up nicely with what we were looking to do for this project and lets us build on an existing infra rather than building something new. I don't think lumping these together since they're both user actions is unreasonable either.
In the future, if we wanted them both to coexist, it would take a bit of extending this infrastructure but I don't think it would be a huge amount of work. I'd rather not build something new to handle a case that might never exist, especially when we can re-use.
>I'm actually surprised that the existing filtering happens at the AutofillPopupControllerImpl level and not in the original Suggestion generation. I assume this is to allow it to react as more characters are typed without requiring re-generation of Suggestions? If that is the reason, then it feels like even more reason that the SuggestionTabIndex concept is not a filter (or at least not the same either/or filter as the string one), and instead is a static UI concept of where a Suggestion should be visually displayed.
Yes but we still need to do this at the popup controller level because it's a UI click that the user makes to change which suggestions to display.
If people want to go in a different direction, I'm not opposed, but since this approach has been aligned (between Slobo, Bruno and I, as well as Jan when I ran this approach by him before he went OOO), and is very close to being implemented (and isn't a huge lift in the codebase), can we go ahead with this as we re-align and figure out what is preferred? This CL is blocking a lot of the future CLs for this project
Though FWIW, I have a preference for this approach so we don't end up with two different ways of changing which suggestions are shown to users.
I prefer not coding with the expectation that both filtering options will coexist, instead only making this change if/when the feature requirements exists.
To me the current approach is a good solution that builds upon what we have and does not add much complexity to the code, being also pretty self contained.
The SuggestionTabIndex being a filter or not is more of a semantics choice I think, concretely thought we will be displaying a set of suggestions based on a user's decision (or filtering choice).
Related details
- Stephen McGruer
- Yishui Liu
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. |
Stephen McGruer (Gerrit)
Stephen McGruer voted and added 1 comment![]( "Open in Gerrit")
Votes added by Stephen McGruer
| Code-Review | +1 |
1 comment
using SuggestionFilter = std::variant<StringFilter, SuggestionTabIndex>;Vinny PerskyArchitecturally this feels like it is combining disparate use-cases to me. One case is actually *filtering* the set of suggestions based on the user input. The other instead is a UI question of indicating/*selecting which UI surface* a particular suggestion should be displayed on. It seems entirely reasonable for both to be present at the same time, thus we shouldn't have an either/or relationship?
I'm actually surprised that the existing filtering happens at the AutofillPopupControllerImpl level and not in the original Suggestion generation. I assume this is to allow it to react as more characters are typed without requiring re-generation of Suggestions? If that is the reason, then it feels like even more reason that the SuggestionTabIndex concept is not a filter (or at least not the same either/or filter as the string one), and instead is a static UI concept of where a Suggestion should be visually displayed.
I think these should be different concepts at the code level, albeit I am interested to know what brunobraga@'s opinion is.
Vinny PerskyI can also see the case for them being similar - based on a user input/action, change which suggestions we are showing to the user. Clicking a tab or typing in a string can both be lumped together as user actions.
Agreed, it's not highly unreasonable for them to be together in the future. But for practical purposes, we had an infrastructure where, based on X user action, we can filter which suggestions to show the user, which lined up nicely with what we were looking to do for this project and lets us build on an existing infra rather than building something new. I don't think lumping these together since they're both user actions is unreasonable either.
In the future, if we wanted them both to coexist, it would take a bit of extending this infrastructure but I don't think it would be a huge amount of work. I'd rather not build something new to handle a case that might never exist, especially when we can re-use.
>I'm actually surprised that the existing filtering happens at the AutofillPopupControllerImpl level and not in the original Suggestion generation. I assume this is to allow it to react as more characters are typed without requiring re-generation of Suggestions? If that is the reason, then it feels like even more reason that the SuggestionTabIndex concept is not a filter (or at least not the same either/or filter as the string one), and instead is a static UI concept of where a Suggestion should be visually displayed.
Yes but we still need to do this at the popup controller level because it's a UI click that the user makes to change which suggestions to display.
If people want to go in a different direction, I'm not opposed, but since this approach has been aligned (between Slobo, Bruno and I, as well as Jan when I ran this approach by him before he went OOO), and is very close to being implemented (and isn't a huge lift in the codebase), can we go ahead with this as we re-align and figure out what is preferred? This CL is blocking a lot of the future CLs for this project
Bruno BragaThough FWIW, I have a preference for this approach so we don't end up with two different ways of changing which suggestions are shown to users.
I prefer not coding with the expectation that both filtering options will coexist, instead only making this change if/when the feature requirements exists.
To me the current approach is a good solution that builds upon what we have and does not add much complexity to the code, being also pretty self contained.
The SuggestionTabIndex being a filter or not is more of a semantics choice I think, concretely thought we will be displaying a set of suggestions based on a user's decision (or filtering choice).
Ack, sounds good to me. Thanks both for your thoughts 😊.
Related details
- Yishui Liu
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. |
Yishui Liu (Gerrit)
Yishui Liu added 1 comment![]( "Open in Gerrit")
// The index of the tab that the suggestion will be shown in.Yishui LiuAgain I am hit with this concept of "we are saying tab without a reader necessarily having any clue what that is". I think we need to find the right "central" place to define this (maybe here, maybe its at the tab_index member point) and explain what a tab is in this context.
I move this line here so that we can define the default index constant directly in the strong alias type. We can move this line down with the `tab_index` member, but then the constant will need to be defined as int, and we will need to convert it to the alias type when we use it.
Added more detail about the index type. Please let me know if you still think moving this line with the `tab_index` member is more readable.
Resolving this comment as the change is reviewed. Please let us know if you have any further questions.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yishui Liu (Gerrit)
Yishui Liu added 1 comment![]( "Open in Gerrit")
Only resolved merge conflict since latest approved change.
Yishui Liu (Gerrit)
Yishui Liu 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. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
28 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/ui/autofill/autofill_popup_controller_impl_unittest.cc
Insertions: 30, Deletions: 0.
The diff is too large to show. Please review the diff.
```
Change information
[BNPL][PNPL] Add TabIndex to Suggestion and SuggestionFilter
This CL adds an int member to `Suggestion` to filter suggestions in the
autofill popup for showing them in tabbed panes instead of all in the
same list.
`AutofillPopupController` is also updated to take string or int as
suggestion filter.
Filtering logic for the tab index will be added in the next CL.
Design doc: go/chrome_bnpl_pay_now_pay_later_tabs
- M chrome/browser/ui/autofill/autofill_popup_controller.h
- M chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
- M chrome/browser/ui/autofill/autofill_popup_controller_impl_unittest.cc
- M chrome/browser/ui/views/autofill/popup/popup_view_views.cc
- M chrome/browser/ui/views/autofill/popup/popup_view_views_unittest.cc
- M components/autofill/core/browser/suggestions/suggestion.h
Code-Review: +1 by Wilson Low, +1 by Stephen McGruer, +1 by Vinny Persky, +1 by Bruno Braga, +1 by Slobodan Pejic
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |