[WebMCP] Add AbortSignal support to ExecuteTool for imperative tools [chromium/src : main]
Fr (Gerrit)
Fr voted and added 1 comment![]( "Open in Gerrit")
Votes added by Fr
| Commit-Queue | +1 |
Related details
- Anders Hartvoll Ruud
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 2 comments![]( "Open in Gerrit")
I can review this if there are no other good options available, but ideally we would try someone with the faintest clue about `AbortSignal` first.
if (options->hasSignal() && options->signal()->aborted()) {
resolver->Reject(options->signal()->reason(script_state));
return promise;
}We might want to test this insta-abort path?
Related details
- Fr
- Mason Freed
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. |
Mason Freed (Gerrit)
Mason Freed added 7 comments![]( "Open in Gerrit")
Looks ok to me, with mostly nits and small things.
return model_context->ExecuteTool(name, input_arguments, nullptr,Can you add a (big!) TODO to plumb this into the browser side? Otherwise, this patch doesn't do anything observable, other than the testing interface, right?
And in the meantime:
nit: can you add `/*signal=*/nullptr` here?
base::expected<String, WebDocument::ScriptToolError>)>>I wonder if you need couldn't keep re-using ScriptToolExecutedCallback here, by using WebString instead/somehow?
// TODO: crbug.com/479598776 - Add support for tracking execution ofPerhaps you could add another TODO here, for adding `signal` support?
"echo", "{\"text\": \"hello\"}", nullptr,Seems like it might be worth adding at least one test case to this file where a signal is provided and aborted?
if (options->hasSignal() && options->signal()->aborted()) {
resolver->Reject(options->signal()->reason(script_state));
return promise;
}We might want to test this insta-abort path?
+1, but also, should this block actually reside in `ExecuteTool()` itself? I.e. it seems like if the signal is already aborted, we shouldn't execute the tool. As it stands, that's only the case for the testing interface.
await new Promise(r => { setTimeout(r, 1000); });Might want to go a bit longer, just to avoid flakiness due to slow bots? Like `5000` maybe?
Related details
- Fr
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. |
Fr (Gerrit)
Fr added 7 comments![]( "Open in Gerrit")
return model_context->ExecuteTool(name, input_arguments, nullptr,Can you add a (big!) TODO to plumb this into the browser side? Otherwise, this patch doesn't do anything observable, other than the testing interface, right?
And in the meantime:
nit: can you add `/*signal=*/nullptr` here?
Done
base::expected<String, WebDocument::ScriptToolError>)>>I wonder if you need couldn't keep re-using ScriptToolExecutedCallback here, by using WebString instead/somehow?
How would you do that?
// TODO: crbug.com/479598776 - Add support for tracking execution ofPerhaps you could add another TODO here, for adding `signal` support?
Done
Seems like it might be worth adding at least one test case to this file where a signal is provided and aborted?
Done
if (options->hasSignal() && options->signal()->aborted()) {
resolver->Reject(options->signal()->reason(script_state));
return promise;
}Mason FreedWe might want to test this insta-abort path?
+1, but also, should this block actually reside in `ExecuteTool()` itself? I.e. it seems like if the signal is already aborted, we shouldn't execute the tool. As it stands, that's only the case for the testing interface.
I've moved this.
Might want to go a bit longer, just to avoid flakiness due to slow bots? Like `5000` maybe?
- Anders Hartvoll Ruud
- Mason Freed
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. |
Mason Freed (Gerrit)
Mason Freed added 6 comments![]( "Open in Gerrit")
// TODO(475998009): PLUMB SIGNAL TO THE BROWSER SIDE!So this is a closed bug (that I own!). Would you mind filing one with the context for this connection? I.e. a bug about finishing the implementation of AbortSignal in tools?
base::OnceCallback<void(
base::expected<String, WebDocument::ScriptToolError>)>
callback;1. undo this change.
base::expected<String, WebDocument::ScriptToolError>)>>FrI wonder if you need couldn't keep re-using ScriptToolExecutedCallback here, by using WebString instead/somehow?
How would you do that?
I'm likely missing something! But see the two *numbered* comments below...
// TODO(475998009): Add signal support for declarative tools.ditto
base::expected<String, WebDocument::ScriptToolError> result) {2. change this to `WebString`
setTimeout(() => { controller.abort('aborted after 500ms'); }, 500);nit: maybe `aborted after 500ms (this is expected)` ?
Related details
- Anders Hartvoll Ruud
- Fr
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. |
Fr (Gerrit)
Fr added 6 comments![]( "Open in Gerrit")
So this is a closed bug (that I own!). Would you mind filing one with the context for this connection? I.e. a bug about finishing the implementation of AbortSignal in tools?
Done
base::OnceCallback<void(
base::expected<String, WebDocument::ScriptToolError>)>
callback;Fr1. undo this change.
Done
base::expected<String, WebDocument::ScriptToolError>)>>FrI wonder if you need couldn't keep re-using ScriptToolExecutedCallback here, by using WebString instead/somehow?
Mason FreedHow would you do that?
I'm likely missing something! But see the two *numbered* comments below...
🙏 I get what you meant now ;)
// TODO(475998009): Add signal support for declarative tools.Frditto
Done
base::expected<String, WebDocument::ScriptToolError> result) {2. change this to `WebString`
Done
setTimeout(() => { controller.abort('aborted after 500ms'); }, 500);nit: maybe `aborted after 500ms (this is expected)` ?
- Anders Hartvoll Ruud
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
2 comments
// TODO(475998009): PLUMB SIGNAL TO THE BROWSER SIDE!FrSo this is a closed bug (that I own!). Would you mind filing one with the context for this connection? I.e. a bug about finishing the implementation of AbortSignal in tools?
Done
- Anders Hartvoll Ruud
- Fr
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fr (Gerrit)
Fr voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Anders Hartvoll Ruud
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fr (Gerrit)
Fr voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Anders Hartvoll Ruud
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
6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/script_tools/model_context_test.cc
Insertions: 48, Deletions: 7.
The diff is too large to show. Please review the diff.
```
Change information
[WebMCP] Add AbortSignal support to ExecuteTool for imperative tools
This CL adds an optional signal option to ExecuteTool so that imperative
script tools execution can be aborted. Support for declarative script
tools will come later.
As part of this CL, an uncaught promise error is not raised anymore when
a syntax error occurs when parsing JSON input arguments string. Because
of this issue, the tests were not passing.
- M third_party/blink/renderer/bindings/generated_in_core.gni
- M third_party/blink/renderer/core/exported/web_document.cc
- M third_party/blink/renderer/core/script_tools/model_context.cc
- M third_party/blink/renderer/core/script_tools/model_context.h
- M third_party/blink/renderer/core/script_tools/model_context_test.cc
- M third_party/blink/renderer/core/script_tools/model_context_testing.cc
- M third_party/blink/renderer/core/script_tools/model_context_testing.h
- M third_party/blink/renderer/core/script_tools/model_context_testing.idl
- M third_party/blink/web_tests/fast/webmcp/execute_tool-expected.txt
- M third_party/blink/web_tests/fast/webmcp/execute_tool.html
Code-Review: +1 by Mason Freed
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |