[projects] Support thread resumption via the controller [chromium/src : main]
Chase Hartsell (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Ayman Almadhoun
- David Maunder
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Maunder (Gerrit)
David Maunder added 4 comments![]( "Open in Gerrit")
std::map<const std::string, const base::Uuid> thread_server_id_to_task_id_;Are there any alternatives that avoid storing this mapping? Could this be handled at the API level or could we query the service directly when needed? This feels like additional complexity that may not be needed
auto* tab_strip_model = browser_->GetTabStripModel();I suggest we check ProjectsPanelController Weakptr first in case the browser has been closed.
existing_thread_tab_index = i;Could we just ActivateTab here and return instead of tracking the existing_thread_tab_index?
tab_strip_model->ActivateTabAt(existing_thread_tab_index.value());Does this work for multi window as well (I'm not too familiar with the TabStrip model in Desktop but if this only works for single window we should track the work for opening the URL if it exists in a separate window).
Related details
- Ayman Almadhoun
- Chase Hartsell
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. |
Chase Hartsell (Gerrit)
Chase Hartsell added 1 comment![]( "Open in Gerrit")
std::map<const std::string, const base::Uuid> thread_server_id_to_task_id_;Are there any alternatives that avoid storing this mapping? Could this be handled at the API level or could we query the service directly when needed? This feels like additional complexity that may not be needed
If we don't do this, we'd need to do one of the following:
1) Pass down `ContextualTask` to the views instead of `Thread` so they can pipe task ID up in the on click callback.
2) Call `ContextualTasksService::GetTasks` and iterate through all tasks to find the tasks with the matching ID within `OpenThread`.
3) Add a method like `GetTaskForServerId` within `ContextualTasksService` to map `server_id` to task UUID (like here) or iterate through all tasks to find the task with this ID.
4) Have our own custom struct that wraps `Thread` with the associated task UUID, and pass that down to the view instead.
Personally, 1 doesn't feel right because the views don't actually need all the context of `ContextualTask`. 2 is less time efficient and requires another callback for when `GetTasks` returns. 3 is just moving the logic we have now or doing the same thing as 2 but within the service. 4 feels like the best option if we don't want to store a mapping, but I don't know if it's worth having a new type just for this view.
Related details
- Ayman Almadhoun
- David Maunder
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. |
Chase Hartsell (Gerrit)
Chase Hartsell added 1 comment![]( "Open in Gerrit")
tab_strip_model->ActivateTabAt(existing_thread_tab_index.value());Does this work for multi window as well (I'm not too familiar with the TabStrip model in Desktop but if this only works for single window we should track the work for opening the URL if it exists in a separate window).
I assumed from the PRD this was supposed to only check within the same window, since it doesn't specify cross-window behavior. I'll check with UX to see what the intended behavior should be.
Ayman Almadhoun (Gerrit)
Ayman Almadhoun voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ayman Almadhoun
| Code-Review | +1 |
1 comment
Related details
- Chase Hartsell
- David Maunder
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. |
David Maunder (Gerrit)
David Maunder added 2 comments![]( "Open in Gerrit")
std::map<const std::string, const base::Uuid> thread_server_id_to_task_id_;Chase HartsellAre there any alternatives that avoid storing this mapping? Could this be handled at the API level or could we query the service directly when needed? This feels like additional complexity that may not be needed
If we don't do this, we'd need to do one of the following:
1) Pass down `ContextualTask` to the views instead of `Thread` so they can pipe task ID up in the on click callback.
2) Call `ContextualTasksService::GetTasks` and iterate through all tasks to find the tasks with the matching ID within `OpenThread`.
3) Add a method like `GetTaskForServerId` within `ContextualTasksService` to map `server_id` to task UUID (like here) or iterate through all tasks to find the task with this ID.
4) Have our own custom struct that wraps `Thread` with the associated task UUID, and pass that down to the view instead.Personally, 1 doesn't feel right because the views don't actually need all the context of `ContextualTask`. 2 is less time efficient and requires another callback for when `GetTasks` returns. 3 is just moving the logic we have now or doing the same thing as 2 but within the service. 4 feels like the best option if we don't want to store a mapping, but I don't know if it's worth having a new type just for this view.
Ok. My preference is 3) for a simpler API and 4) is also not bad (maybe we can call it ThreadContext). Can you create a TODO for post MVP and link it in the code? We can reconsider then. Not worth slowing down this CL for.
tab_strip_model->ActivateTabAt(existing_thread_tab_index.value());Chase HartsellDoes this work for multi window as well (I'm not too familiar with the TabStrip model in Desktop but if this only works for single window we should track the work for opening the URL if it exists in a separate window).
I assumed from the PRD this was supposed to only check within the same window, since it doesn't specify cross-window behavior. I'll check with UX to see what the intended behavior should be.
Sounds good thanks. If UX wants it and the implementation is complex we can chat to them about punting to post-MVP.
Related details
- Chase Hartsell
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. |