Check thread type and implement remote task update calls [chromium/src : main]
0 views
Skip to first unread message
پری کیوانلو (Gerrit)
2:07 PM (6 hours ago) 2:07 PM
to Min Qin, chromium...@chromium.org, Sophie Chang
Attention needed from Min Qin
پری کیوانلو added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention is currently required from:
- Min Qin
Submit Requirements:
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)
3:52 PM (4 hours ago) 3:52 PM
to Shakti Sahu, Tommy Nyquist, Chromium LUCI CQ, chromium...@chromium.org, Sophie Chang
Attention needed from Shakti Sahu and Tommy Nyquist
Min Qin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Min Qin
| Commit-Queue | +1 |
1 comment
Patchset-level comments
Attention is currently required from:
- Shakti Sahu
- Tommy Nyquist
Submit Requirements:
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)
4:26 PM (4 hours ago) 4:26 PM
to Min Qin, Tommy Nyquist, Chromium LUCI CQ, chromium...@chromium.org, Sophie Chang
Attention needed from Min Qin and Tommy Nyquist
Shakti Sahu voted and added 3 comments![]( "Open in Gerrit")
Votes added by Shakti Sahu
| Code-Review | +1 |
3 comments
File components/contextual_tasks/internal/contextual_tasks_service_impl.cc
Line 165, Patchset 1 (Latest):
return;Shakti Sahu . unresolved
When is this going to happen? Maybe a comment that this is unexpected edge case?
Line 170, Patchset 1 (Latest):
Thread(thread_type, server_id, "", conversation_turn_id));Shakti Sahu . unresolved
comment for this param /*param_name=*/
Line 435, Patchset 1 (Latest):
tasks_.insert_or_assign(task.GetTaskId(), task);Shakti Sahu . unresolved
This should be before notifying observers? In case they make a GetTask(..) call..
Related details
Attention is currently required from:
- Min Qin
- Tommy Nyquist
Submit Requirements:
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)
4:47 PM (3 hours ago) 4:47 PM
to Shakti Sahu, Tommy Nyquist, Chromium LUCI CQ, chromium...@chromium.org, Sophie Chang
Attention needed from Tommy Nyquist
Min Qin added 3 comments![]( "Open in Gerrit")
File components/contextual_tasks/internal/contextual_tasks_service_impl.cc
When is this going to happen? Maybe a comment that this is unexpected edge case?
Min Qin
Done
Line 170, Patchset 1:
Thread(thread_type, server_id, "", conversation_turn_id));Shakti Sahu . resolved
comment for this param /*param_name=*/
Min Qin
Done
This should be before notifying observers? In case they make a GetTask(..) call..
Min Qin
this notify is a post task, so it will always be executed later. But changed the code to put the insert_or_assign call into the if-else statement.
Related details
Attention is currently required from:
- Tommy Nyquist
Submit Requirements:
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)
5:08 PM (3 hours ago) 5:08 PM
to Min Qin, Tommy Nyquist, Chromium LUCI CQ, chromium...@chromium.org, Sophie Chang
Attention needed from Min Qin and Tommy Nyquist
Shakti Sahu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Shakti Sahu
| Code-Review | +1 |
1 comment
File components/contextual_tasks/internal/contextual_tasks_service_impl.cc
Line 431, Patchset 2 (Latest):
tasks_.insert_or_assign(task.GetTaskId(), task);Shakti Sahu . unresolved
This is common to both blocks.
Related details
Attention is currently required from:
- Min Qin
- Tommy Nyquist
Submit Requirements:
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)
5:13 PM (3 hours ago) 5:13 PM
to Shakti Sahu, Tommy Nyquist, Chromium LUCI CQ, chromium...@chromium.org, Sophie Chang
Attention needed from Tommy Nyquist
Min Qin added 1 comment![]( "Open in Gerrit")
File components/contextual_tasks/internal/contextual_tasks_service_impl.cc
Line 431, Patchset 2 (Latest):
tasks_.insert_or_assign(task.GetTaskId(), task);Shakti Sahu . resolved
This is common to both blocks.
Min Qin
Yes, but I cannot do it before the if statement since the that will make the if statement always return true. Alternatively, I can introduce a temporary variable to store tasks_.find(task.GetTaskId()) == tasks_.end(), and then use that variable later after the insert_or_assign call. But i think the current code is probably better for understanding.
Related details
Attention is currently required from:
- Tommy Nyquist
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages