[IOS][AIM]Do not show typed suggestions when composebox has attachments [chromium/src : main]
Ameur Hosni (Gerrit)
Ameur Hosni voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ameur Hosni
| Commit-Queue | +1 |
1 comment
#if BUILDFLAG(IS_IOS)`kLensOverlayContextualSearchbox` seems to have too many parameters that are very specific to desktop. In addition to that I think for ios we also need to ensure that we clear results.
For convenience adding this platform check to ensure this code path is executed for iOS.
I suspect this is the same for android as well. FYI @en...@google.com
Related details
- Christian Xu
- Nihar Majmudar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nihar Majmudar (Gerrit)
Nihar Majmudar added 3 comments![]( "Open in Gerrit")
case OmniboxEventProto::CONTEXTUAL_SEARCHBOX:Why not keep this NTP_COMPOSEBOX, comments in `composebox_omnibox_client.mm` still apply, regarding creating a new page classification for iOS
StopSuggest();
ClearAllResults();
return;Why is this needed? Won't other results not show already if we're going to be returning the verbatim match only?
return metrics::OmniboxEventProto::NTP_COMPOSEBOX;should iOS eventually have its own page classifications? Also why do we need to return the contextual searchbox page classification if there are attachments?
This gets handled later on for composebox's: https://source.chromium.org/chromium/chromium/src/+/main:components/omnibox/browser/remote_suggestions_service.cc;l=319-323?q=chrome-contextual
Related details
- Ameur Hosni
- Christian Xu
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. |
Ameur Hosni (Gerrit)
Ameur Hosni added 3 comments![]( "Open in Gerrit")
case OmniboxEventProto::CONTEXTUAL_SEARCHBOX:Why not keep this NTP_COMPOSEBOX, comments in `composebox_omnibox_client.mm` still apply, regarding creating a new page classification for iOS
We still want to identify if composebox has attachments (see my reply in composebox_omnibox_client.mm).
StopSuggest();
ClearAllResults();
return;Why is this needed? Won't other results not show already if we're going to be returning the verbatim match only?
I thought this is needed to prevent this bug : https://g-issues.chromium.org/issues/463999790
return metrics::OmniboxEventProto::NTP_COMPOSEBOX;should iOS eventually have its own page classifications? Also why do we need to return the contextual searchbox page classification if there are attachments?
This gets handled later on for composebox's: https://source.chromium.org/chromium/chromium/src/+/main:components/omnibox/browser/remote_suggestions_service.cc;l=319-323?q=chrome-contextual
We are planning to use `NTP_OMNIBOX_COMPOSEBOX` `SRP_OMNIBOX_COMPOSEBOX`.. Why do you think we should use our own classification? I thought those were intended to be share across platforms.
Regarding why we return contextual searchbox page classification when attachments exist: the code you linked updates the client, sure, but the input page classification doesn't reflect on the contextual query unless we explicitly set it. And setting it is necessary because we use that page_classification to decide if we should return suggestions for contextual queries in the search_provider.
Related details
- Christian Xu
- Nihar Majmudar
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. |
Nihar Majmudar (Gerrit)
Nihar Majmudar added 2 comments![]( "Open in Gerrit")
StopSuggest();
ClearAllResults();
return;Ameur HosniWhy is this needed? Won't other results not show already if we're going to be returning the verbatim match only?
I thought this is needed to prevent this bug : https://g-issues.chromium.org/issues/463999790
Oh, I see, but if an image is added to a typed query, won't this just return the verbatim match? Wouldn't this new verbatim match update clear the previous matches?
return metrics::OmniboxEventProto::NTP_COMPOSEBOX;Ameur Hosnishould iOS eventually have its own page classifications? Also why do we need to return the contextual searchbox page classification if there are attachments?
This gets handled later on for composebox's: https://source.chromium.org/chromium/chromium/src/+/main:components/omnibox/browser/remote_suggestions_service.cc;l=319-323?q=chrome-contextual
We are planning to use `NTP_OMNIBOX_COMPOSEBOX` `SRP_OMNIBOX_COMPOSEBOX`.. Why do you think we should use our own classification? I thought those were intended to be share across platforms.
Regarding why we return contextual searchbox page classification when attachments exist: the code you linked updates the client, sure, but the input page classification doesn't reflect on the contextual query unless we explicitly set it. And setting it is necessary because we use that page_classification to decide if we should return suggestions for contextual queries in the search_provider.
oops you are correct, don't know why i thought each platform had their own.
Regarding the check in SearchProvider, why don't we check if its a composebox and if there are lens signals available on the input? We don't want to change the page classification because these should all still be getting logged as the composebox.
Related details
- Ameur Hosni
- Christian Xu
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. |
Ameur Hosni (Gerrit)
Ameur Hosni voted and added 3 comments![]( "Open in Gerrit")
Votes added by Ameur Hosni
| Auto-Submit | +1 |
| Commit-Queue | +1 |
3 comments
Ameur HosniWhy not keep this NTP_COMPOSEBOX, comments in `composebox_omnibox_client.mm` still apply, regarding creating a new page classification for iOS
We still want to identify if composebox has attachments (see my reply in composebox_omnibox_client.mm).
Done
Ameur HosniWhy is this needed? Won't other results not show already if we're going to be returning the verbatim match only?
Nihar MajmudarI thought this is needed to prevent this bug : https://g-issues.chromium.org/issues/463999790
Oh, I see, but if an image is added to a typed query, won't this just return the verbatim match? Wouldn't this new verbatim match update clear the previous matches?
Confirming this is not related, I can see that we don't run history query for Composebox classification.
I think it's probably related to the fact that contextual zps for an image request is failing with 500 which probably mess up the suggestions somewhere.
Ameur Hosnishould iOS eventually have its own page classifications? Also why do we need to return the contextual searchbox page classification if there are attachments?
This gets handled later on for composebox's: https://source.chromium.org/chromium/chromium/src/+/main:components/omnibox/browser/remote_suggestions_service.cc;l=319-323?q=chrome-contextual
Nihar MajmudarWe are planning to use `NTP_OMNIBOX_COMPOSEBOX` `SRP_OMNIBOX_COMPOSEBOX`.. Why do you think we should use our own classification? I thought those were intended to be share across platforms.
Regarding why we return contextual searchbox page classification when attachments exist: the code you linked updates the client, sure, but the input page classification doesn't reflect on the contextual query unless we explicitly set it. And setting it is necessary because we use that page_classification to decide if we should return suggestions for contextual queries in the search_provider.
oops you are correct, don't know why i thought each platform had their own.
Regarding the check in SearchProvider, why don't we check if its a composebox and if there are lens signals available on the input? We don't want to change the page classification because these should all still be getting logged as the composebox.
- Christian Xu
- Nihar Majmudar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nihar Majmudar (Gerrit)
Nihar Majmudar voted and added 2 comments![]( "Open in Gerrit")
Votes added by Nihar Majmudar
| Code-Review | +1 |
2 comments
if (omnibox::IsComposebox(input_.current_page_classification()) &&
input_.lens_overlay_suggest_inputs().has_value() &&
!base::FeatureList::IsEnabled(
omnibox::kComposeboxAttachmentsTypedState)) {
return;
}
qq: This won't affect the desktop implementation since we hide the dropdown in this case for now right? But desktop will need to enable this flag if we ever want context typed suggestions?
Related details
- Ameur Hosni
- Christian Xu
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. |
Ameur Hosni (Gerrit)
Ameur Hosni voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ameur Hosni
| Auto-Submit | +1 |
1 comment
if (omnibox::IsComposebox(input_.current_page_classification()) &&
input_.lens_overlay_suggest_inputs().has_value() &&
!base::FeatureList::IsEnabled(
omnibox::kComposeboxAttachmentsTypedState)) {
return;
}
qq: This won't affect the desktop implementation since we hide the dropdown in this case for now right? But desktop will need to enable this flag if we ever want context typed suggestions?
Yes That's correct. I can guard this with a platform check if you think you want to handle this differently.
Related details
- Christian Xu
- Nihar Majmudar
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. |
Nihar Majmudar (Gerrit)
Nihar Majmudar added 1 comment![]( "Open in Gerrit")
if (omnibox::IsComposebox(input_.current_page_classification()) &&
input_.lens_overlay_suggest_inputs().has_value() &&
!base::FeatureList::IsEnabled(
omnibox::kComposeboxAttachmentsTypedState)) {
return;
}
Ameur Hosniqq: This won't affect the desktop implementation since we hide the dropdown in this case for now right? But desktop will need to enable this flag if we ever want context typed suggestions?
Yes That's correct. I can guard this with a platform check if you think you want to handle this differently.
No, I think this is fine, I'll move our code to using this flag later. Thanks!
Related details
- Ameur Hosni
- Christian Xu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ameur Hosni (Gerrit)
Ameur Hosni voted Commit-Queue+2![]( "Open in Gerrit")
- Christian Xu
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![]( "Open in Gerrit")
Change information
[IOS][AIM]Do not show typed suggestions when composebox has attachments
This CL introduces a new feature to control whether or not to show
suggestions when attachments are present in the composebox.
- M components/omnibox/browser/search_provider.cc
- M components/omnibox/common/omnibox_features.cc
- M components/omnibox/common/omnibox_features.h
Code-Review: +1 by Nihar Majmudar
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Duncan Mercer (Gerrit)
Duncan Mercer added 1 comment![]( "Open in Gerrit")
if (omnibox::IsComposebox(input_.current_page_classification()) &&
input_.lens_overlay_suggest_inputs().has_value() &&
!base::FeatureList::IsEnabled(
omnibox::kComposeboxAttachmentsTypedState)) {This broke the Lens side panel suggestions on Chrome desktop. Why are we adding a universal flag instead of letting each feature control this behavior?
Related details
- Ameur Hosni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ameur Hosni (Gerrit)
Ameur Hosni added 1 comment![]( "Open in Gerrit")
if (omnibox::IsComposebox(input_.current_page_classification()) &&
input_.lens_overlay_suggest_inputs().has_value() &&
!base::FeatureList::IsEnabled(
omnibox::kComposeboxAttachmentsTypedState)) {This broke the Lens side panel suggestions on Chrome desktop. Why are we adding a universal flag instead of letting each feature control this behavior?
Thanks for flagging Duncan, sent a fix here : crrev.com/c/7234492
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |