[contextual_tasks] Auto-suggested chip dismissal per thread+URL [chromium/src : main]
Shakti Sahu (Gerrit)
Shakti Sahu added 4 comments![]( "Open in Gerrit")
Tested manually. Haven't updated tests but need a review first.
void ContextualTasksComposeboxHandler::UpdateSuggestedTabContext(FYI. This was moved from `ComposeboxHandler` to this subclass. The logic seems more convenient here. `ContextualTasksComposeboxHandler` will do the dismissed URL tracking and filtering of suggested URLs (which is the active tab URL) when requested by outside world.
!web_ui_interface->IsActiveTabContextSuggestionShowing()) {This makes the earlier check for `auto_tab_context_suggestion_enabled` earlier redundant.
void ContextualTasksUI::DisableActiveTabContextSuggestion() {This is now directly done at `ContextualTasksComposeboxHandler`.
Related details
- Min Qin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shakti Sahu (Gerrit)
Shakti Sahu added 1 comment![]( "Open in Gerrit")
// TODO(shaktisahu): Pass the URL of the chip from the UI. This requires URLThis one is a bit weird. When I tried to plumb the URL through JS, a whole lot of plumbing across various callsites was needed and was making things messier there. That's why I decided to take this alternative. Is this correct enough?
Related details
- Min Qin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Min Qin (Gerrit)
Min Qin added 2 comments![]( "Open in Gerrit")
blocklisted_suggestions_.erase(tab->GetContents()->GetLastCommittedURL());this can be moved to outside the if block
if (from_automatic_chip) {should this matter? if user delete another URL context, we should also stop suggesting it right?
Related details
- Shakti Sahu
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. |
Shakti Sahu (Gerrit)
Shakti Sahu added 2 comments![]( "Open in Gerrit")
blocklisted_suggestions_.erase(tab->GetContents()->GetLastCommittedURL());this can be moved to outside the if block
Removed the `blocklisted_suggestions_.erase` from the other block around line 814. We want to remove it from blocklist only if it was explicitly added and not auto-added.
if (from_automatic_chip) {should this matter? if user delete another URL context, we should also stop suggesting it right?
Yes, but there is a caveat below. I am assuming that the current URL in active tab is the one that the user has dismissed via auto chip. I couldn't get it plumbed all the way through.
So, I am just adding to blocklist when auto chip is dismissed.
Related details
- Min Qin
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. |
Min Qin (Gerrit)
Min Qin added 1 comment![]( "Open in Gerrit")
if (from_automatic_chip) {Shakti Sahushould this matter? if user delete another URL context, we should also stop suggesting it right?
Yes, but there is a caveat below. I am assuming that the current URL in active tab is the one that the user has dismissed via auto chip. I couldn't get it plumbed all the way through.
So, I am just adding to blocklist when auto chip is dismissed.
so if you check the current code, I guess you can get the URL from the file info even it it is not auto chip?
std::optional<SessionID> associated_tab_id;
auto* contextual_session_handle = GetContextualSessionHandle();
if (contextual_session_handle) {
const contextual_search::FileInfo* file_info =
contextual_session_handle->GetController()->GetFileInfo(file_token);
if (file_info && file_info->tab_session_id.has_value()) {
associated_tab_id = file_info->tab_session_id.value();
}
}
Related details
- Shakti Sahu
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. |
Shakti Sahu (Gerrit)
Shakti Sahu added 1 comment![]( "Open in Gerrit")
if (from_automatic_chip) {Shakti Sahushould this matter? if user delete another URL context, we should also stop suggesting it right?
Min QinYes, but there is a caveat below. I am assuming that the current URL in active tab is the one that the user has dismissed via auto chip. I couldn't get it plumbed all the way through.
So, I am just adding to blocklist when auto chip is dismissed.
so if you check the current code, I guess you can get the URL from the file info even it it is not auto chip?
std::optional<SessionID> associated_tab_id;
auto* contextual_session_handle = GetContextualSessionHandle();
if (contextual_session_handle) {
const contextual_search::FileInfo* file_info =
contextual_session_handle->GetController()->GetFileInfo(file_token);
if (file_info && file_info->tab_session_id.has_value()) {
associated_tab_id = file_info->tab_session_id.value();
}
}
I also thought so, but I think the auto chip aren't even added to the `contextual_session_handle` because they have `delay_upload=true`. So there wont be a `file_info` I think.
Related details
- Min Qin
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. |
Min Qin (Gerrit)
Min Qin added 1 comment![]( "Open in Gerrit")
if (from_automatic_chip) {Shakti Sahushould this matter? if user delete another URL context, we should also stop suggesting it right?
Min QinYes, but there is a caveat below. I am assuming that the current URL in active tab is the one that the user has dismissed via auto chip. I couldn't get it plumbed all the way through.
So, I am just adding to blocklist when auto chip is dismissed.
Shakti Sahuso if you check the current code, I guess you can get the URL from the file info even it it is not auto chip?
std::optional<SessionID> associated_tab_id;
auto* contextual_session_handle = GetContextualSessionHandle();
if (contextual_session_handle) {
const contextual_search::FileInfo* file_info =
contextual_session_handle->GetController()->GetFileInfo(file_token);
if (file_info && file_info->tab_session_id.has_value()) {
associated_tab_id = file_info->tab_session_id.value();
}
}
I also thought so, but I think the auto chip aren't even added to the `contextual_session_handle` because they have `delay_upload=true`. So there wont be a `file_info` I think.
Yes, for autochip, we can just use active tab to get the URL, but for non-auto chip, we get it from file_info and also add it to blacklist here?
Related details
- Shakti Sahu
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. |
Shakti Sahu (Gerrit)
Shakti Sahu added 1 comment![]( "Open in Gerrit")
if (from_automatic_chip) {Shakti Sahushould this matter? if user delete another URL context, we should also stop suggesting it right?
Min QinYes, but there is a caveat below. I am assuming that the current URL in active tab is the one that the user has dismissed via auto chip. I couldn't get it plumbed all the way through.
So, I am just adding to blocklist when auto chip is dismissed.
Shakti Sahuso if you check the current code, I guess you can get the URL from the file info even it it is not auto chip?
std::optional<SessionID> associated_tab_id;
auto* contextual_session_handle = GetContextualSessionHandle();
if (contextual_session_handle) {
const contextual_search::FileInfo* file_info =
contextual_session_handle->GetController()->GetFileInfo(file_token);
if (file_info && file_info->tab_session_id.has_value()) {
associated_tab_id = file_info->tab_session_id.value();
}
}
Min QinI also thought so, but I think the auto chip aren't even added to the `contextual_session_handle` because they have `delay_upload=true`. So there wont be a `file_info` I think.
Yes, for autochip, we can just use active tab to get the URL, but for non-auto chip, we get it from file_info and also add it to blacklist here?
But the URL may have changed in that tab though (since it was an explictly added tab). Is that still okay?
For the auto-chip case, it's very unlikely to have changed since it constantly observes navigations on the active tab.
Related details
- Min Qin
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. |
Min Qin (Gerrit)
Min Qin added 1 comment![]( "Open in Gerrit")
if (from_automatic_chip) {Shakti Sahushould this matter? if user delete another URL context, we should also stop suggesting it right?
Min QinYes, but there is a caveat below. I am assuming that the current URL in active tab is the one that the user has dismissed via auto chip. I couldn't get it plumbed all the way through.
So, I am just adding to blocklist when auto chip is dismissed.
Shakti Sahuso if you check the current code, I guess you can get the URL from the file info even it it is not auto chip?
std::optional<SessionID> associated_tab_id;
auto* contextual_session_handle = GetContextualSessionHandle();
if (contextual_session_handle) {
const contextual_search::FileInfo* file_info =
contextual_session_handle->GetController()->GetFileInfo(file_token);
if (file_info && file_info->tab_session_id.has_value()) {
associated_tab_id = file_info->tab_session_id.value();
}
}
Min QinI also thought so, but I think the auto chip aren't even added to the `contextual_session_handle` because they have `delay_upload=true`. So there wont be a `file_info` I think.
Shakti SahuYes, for autochip, we can just use active tab to get the URL, but for non-auto chip, we get it from file_info and also add it to blacklist here?
But the URL may have changed in that tab though (since it was an explictly added tab). Is that still okay?
For the auto-chip case, it's very unlikely to have changed since it constantly observes navigations on the active tab.
the fileinfo.tab_url should contain the tab URL uploaded? https://source.chromium.org/chromium/chromium/src/+/main:components/contextual_search/contextual_search_types.h;l=101
Related details
- Shakti Sahu
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. |
Shakti Sahu (Gerrit)
Shakti Sahu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Shakti Sahu
| Commit-Queue | +1 |
1 comment
- Min Qin
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. |
Min Qin (Gerrit)
Min Qin voted Code-Review+1![]( "Open in Gerrit")
- Shakti Sahu
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. |
Shakti Sahu (Gerrit)
Shakti Sahu voted and added 2 comments![]( "Open in Gerrit")
Votes added by Shakti Sahu
| Commit-Queue | +2 |
2 comments
blocklisted_suggestions_.erase(tab->GetContents()->GetLastCommittedURL());Shakti Sahuthis can be moved to outside the if block
Removed the `blocklisted_suggestions_.erase` from the other block around line 814. We want to remove it from blocklist only if it was explicitly added and not auto-added.
Done
if (from_automatic_chip) {Shakti Sahushould this matter? if user delete another URL context, we should also stop suggesting it right?
Min QinYes, but there is a caveat below. I am assuming that the current URL in active tab is the one that the user has dismissed via auto chip. I couldn't get it plumbed all the way through.
So, I am just adding to blocklist when auto chip is dismissed.
Shakti Sahuso if you check the current code, I guess you can get the URL from the file info even it it is not auto chip?
std::optional<SessionID> associated_tab_id;
auto* contextual_session_handle = GetContextualSessionHandle();
if (contextual_session_handle) {
const contextual_search::FileInfo* file_info =
contextual_session_handle->GetController()->GetFileInfo(file_token);
if (file_info && file_info->tab_session_id.has_value()) {
associated_tab_id = file_info->tab_session_id.value();
}
}
Min QinI also thought so, but I think the auto chip aren't even added to the `contextual_session_handle` because they have `delay_upload=true`. So there wont be a `file_info` I think.
Shakti SahuYes, for autochip, we can just use active tab to get the URL, but for non-auto chip, we get it from file_info and also add it to blacklist here?
Min QinBut the URL may have changed in that tab though (since it was an explictly added tab). Is that still okay?
For the auto-chip case, it's very unlikely to have changed since it constantly observes navigations on the active tab.
Shakti Sahuthe fileinfo.tab_url should contain the tab URL uploaded? https://source.chromium.org/chromium/chromium/src/+/main:components/contextual_search/contextual_search_types.h;l=101
Oh, I see. Thanks for catching this. Updated the CL.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shakti Sahu (Gerrit)
Shakti Sahu added 1 comment![]( "Open in Gerrit")
sophiechang@ - For overall lgtm on chrome/browser/ui/webui/cr_components/composebox/composebox_handler.*
Related details
- Sophie Chang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |