Implement the invokeTool CDP command in blink [chromium/src : main]
Philip Pfaffe (Gerrit)
Philip Pfaffe added 1 comment![]( "Open in Gerrit")
Related details
- Alex Rudenko
- 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. |
Alex Rudenko (Gerrit)
Alex Rudenko voted Code-Review+1![]( "Open in Gerrit")
- Anders Hartvoll Ruud
- Philip Pfaffe
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 1 comment![]( "Open in Gerrit")
std::optional<base::UnguessableToken> execution_id = std::nullopt);This does not make sense. We're already returning an UnguessableToken that can be used to identify the execution.
Related details
- Philip Pfaffe
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. |
Philip Pfaffe (Gerrit)
Philip Pfaffe added 1 comment![]( "Open in Gerrit")
std::optional<base::UnguessableToken> execution_id = std::nullopt);This does not make sense. We're already returning an UnguessableToken that can be used to identify the execution.
If execution_id is given here and the tool is invoked successfully, the execution_id is returned again. The agent can't use the return value since it needs to return the execution_id to the CDP client _before_ the tool is actually called.
Related details
- Anders Hartvoll Ruud
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. |
Philip Pfaffe (Gerrit)
Philip Pfaffe added 1 comment![]( "Open in Gerrit")
std::optional<base::UnguessableToken> execution_id = std::nullopt);Philip PfaffeThis does not make sense. We're already returning an UnguessableToken that can be used to identify the execution.
If execution_id is given here and the tool is invoked successfully, the execution_id is returned again. The agent can't use the return value since it needs to return the execution_id to the CDP client _before_ the tool is actually called.
Also see crrev.com/c/7722812/19/third_party/blink/renderer/core/script_tools/model_context.cc#351 which will do the same once that lands.
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 1 comment![]( "Open in Gerrit")
std::optional<base::UnguessableToken> execution_id = std::nullopt);Philip PfaffeThis does not make sense. We're already returning an UnguessableToken that can be used to identify the execution.
Philip PfaffeIf execution_id is given here and the tool is invoked successfully, the execution_id is returned again. The agent can't use the return value since it needs to return the execution_id to the CDP client _before_ the tool is actually called.
Also see crrev.com/c/7722812/19/third_party/blink/renderer/core/script_tools/model_context.cc#351 which will do the same once that lands.
I lack the full context, but the API in that link _also_ doesn't make sense to me. Returning the token seems like the best way. If that is not possible, then a caller-provided token seems fine (though strictly worse, because you can pass the same token to multiple executions). However, `ExecuteTool` both accepting a token _and_ returning one seems like the API just failed to make up its mind.
The agent can't use the return value since it needs to return the execution_id to the CDP client before the tool is actually called.
Under that limitation, I would keep track of the executions myself. ("Myself" being `InspectorWebMCPAgent`.)
Though I understand how that's less tempting if we're anyway moving towards a caller-provided token.
Related details
- Philip Pfaffe
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. |
Philip Pfaffe (Gerrit)
Philip Pfaffe added 1 comment![]( "Open in Gerrit")
std::optional<base::UnguessableToken> execution_id = std::nullopt);Philip PfaffeThis does not make sense. We're already returning an UnguessableToken that can be used to identify the execution.
Philip PfaffeIf execution_id is given here and the tool is invoked successfully, the execution_id is returned again. The agent can't use the return value since it needs to return the execution_id to the CDP client _before_ the tool is actually called.
Anders Hartvoll RuudAlso see crrev.com/c/7722812/19/third_party/blink/renderer/core/script_tools/model_context.cc#351 which will do the same once that lands.
I lack the full context, but the API in that link _also_ doesn't make sense to me. Returning the token seems like the best way. If that is not possible, then a caller-provided token seems fine (though strictly worse, because you can pass the same token to multiple executions). However, `ExecuteTool` both accepting a token _and_ returning one seems like the API just failed to make up its mind.
The agent can't use the return value since it needs to return the execution_id to the CDP client before the tool is actually called.
Under that limitation, I would keep track of the executions myself. ("Myself" being `InspectorWebMCPAgent`.)
Though I understand how that's less tempting if we're anyway moving towards a caller-provided token.
I don't disagree, but crbug.com/501190526 will clean that up.
Related details
- Anders Hartvoll Ruud
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. |
Philip Pfaffe (Gerrit)
Philip Pfaffe added 1 comment![]( "Open in Gerrit")
std::optional<base::UnguessableToken> execution_id = std::nullopt);Philip PfaffeThis does not make sense. We're already returning an UnguessableToken that can be used to identify the execution.
Philip PfaffeIf execution_id is given here and the tool is invoked successfully, the execution_id is returned again. The agent can't use the return value since it needs to return the execution_id to the CDP client _before_ the tool is actually called.
Anders Hartvoll RuudAlso see crrev.com/c/7722812/19/third_party/blink/renderer/core/script_tools/model_context.cc#351 which will do the same once that lands.
Philip PfaffeI lack the full context, but the API in that link _also_ doesn't make sense to me. Returning the token seems like the best way. If that is not possible, then a caller-provided token seems fine (though strictly worse, because you can pass the same token to multiple executions). However, `ExecuteTool` both accepting a token _and_ returning one seems like the API just failed to make up its mind.
The agent can't use the return value since it needs to return the execution_id to the CDP client before the tool is actually called.
Under that limitation, I would keep track of the executions myself. ("Myself" being `InspectorWebMCPAgent`.)
Though I understand how that's less tempting if we're anyway moving towards a caller-provided token.
I don't disagree, but crbug.com/501190526 will clean that up.
@mas...@chromium.org WDYT? Should we land this change or should I wait for yours plus the followup?
Related details
- 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. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 1 comment![]( "Open in Gerrit")
std::optional<base::UnguessableToken> execution_id = std::nullopt);Philip PfaffeThis does not make sense. We're already returning an UnguessableToken that can be used to identify the execution.
Philip PfaffeIf execution_id is given here and the tool is invoked successfully, the execution_id is returned again. The agent can't use the return value since it needs to return the execution_id to the CDP client _before_ the tool is actually called.
Anders Hartvoll RuudAlso see crrev.com/c/7722812/19/third_party/blink/renderer/core/script_tools/model_context.cc#351 which will do the same once that lands.
Philip PfaffeI lack the full context, but the API in that link _also_ doesn't make sense to me. Returning the token seems like the best way. If that is not possible, then a caller-provided token seems fine (though strictly worse, because you can pass the same token to multiple executions). However, `ExecuteTool` both accepting a token _and_ returning one seems like the API just failed to make up its mind.
The agent can't use the return value since it needs to return the execution_id to the CDP client before the tool is actually called.
Under that limitation, I would keep track of the executions myself. ("Myself" being `InspectorWebMCPAgent`.)
Though I understand how that's less tempting if we're anyway moving towards a caller-provided token.
Philip PfaffeI don't disagree, but crbug.com/501190526 will clean that up.
@mas...@chromium.org WDYT? Should we land this change or should I wait for yours plus the followup?
(Since @mas...@chromium.org is OOO, and we need to maintain velocity:)
I can live with this if we tweak the overloads such that param/return values "agree", e.g.:
```
bool ModelContext::ExecuteTool(const base::UnguessableToken& execution_id, ...) {
/* Main logic */
/* Return true on success */
}
std::optional<base::UnguessableToken> ModelContext::ExecuteTool(...) {
base::UnguessableToken execution_id = base::UnguessableToken::Create();
if (ExecuteTool(execution_id, ...)) {
return execution_id;
}
return std::nullopt;
}
```---
The inspector will then use the top one (directly), and everything else will use the bottom one until Issue 501190526 unifies things.
Related details
- Mason Freed
- Philip Pfaffe
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. |
Mark Foltz (Gerrit)
Mark Foltz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mark Foltz
| Code-Review | -1 |
1 comment
This CL would introduce merge conflicts with Mason's CL, which is higher priority for the project as a whole and riskier to land. I would prefer to block this on his CL and clean up later.
Related details
- Mason Freed
- Philip Pfaffe
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. |
Philip Pfaffe (Gerrit)
Philip Pfaffe added 1 comment![]( "Open in Gerrit")
This CL would introduce merge conflicts with Mason's CL, which is higher priority for the project as a whole and riskier to land. I would prefer to block this on his CL and clean up later.
Only a mechanical one (renaming the execution_id local to token), I've asked Mason to review, I don't think there'll be a real conflict here.
Related details
- Mark Foltz
- 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. |
Mark Foltz (Gerrit)
Mark Foltz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mark Foltz
| Code-Review | +0 |
1 comment
- Mason Freed
- Philip Pfaffe
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. |
Mark Foltz (Gerrit)
Mark Foltz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mark Foltz
| Code-Review | +1 |
1 comment
The CL mentioned above will provide the token that can be used here. I don't think it's entirely wrong to return it as a value as well as passing in the callback, but if there only one calling pattern that consumes the token (sync or async) we should pick one or the other.
Related details
- Mason Freed
- Philip Pfaffe
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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
2 comments
LGTM, and ok to land first. I'll rebase on top of this.
I agree this is kind of weird, but I can try to clean it up with crbug.com/501190526.
Related details
- Philip Pfaffe
Philip Pfaffe (Gerrit)
Philip Pfaffe added 1 comment![]( "Open in Gerrit")
std::optional<base::UnguessableToken> execution_id = std::nullopt);I've renamed this to "override", maybe that makes the intent clearer. I've also renamed the token back to execution_id, that minimizes the merge conflict!
Related details
- Anders Hartvoll Ruud
- Mark Foltz
- 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. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud voted and added 1 comment![]( "Open in Gerrit")
Votes added by Anders Hartvoll Ruud
| Code-Review | +1 |
1 comment
std::optional<base::UnguessableToken> execution_id = std::nullopt);- Mark Foltz
- Mason Freed
- Philip Pfaffe
Philip Pfaffe (Gerrit)
Philip Pfaffe voted and added 1 comment![]( "Open in Gerrit")
Votes added by Philip Pfaffe
| Commit-Queue | +2 |
Philip PfaffeThis CL would introduce merge conflicts with Mason's CL, which is higher priority for the project as a whole and riskier to land. I would prefer to block this on his CL and clean up later.
Only a mechanical one (renaming the execution_id local to token), I've asked Mason to review, I don't think there'll be a real conflict here.
Done
Related details
- Mark Foltz
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Pfaffe (Gerrit)
Philip Pfaffe voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Mark Foltz
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromium-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
chromiu...@luci-project-accounts.iam.gserviceaccount.com submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
3 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/web_tests/virtual/webmcp/inspector-protocol/webmcp/invoke-tool-expected.txt
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
Change information
Implement the invokeTool CDP command in blink
This is a stop gap solution while we are figuring out the correct
browser-side implementation in crrev.com/c/7696374. The blink-only
version will not work for cross-document results.
- M third_party/blink/public/devtools_protocol/domains/WebMCP.pdl
- M third_party/blink/renderer/core/inspector/inspector_web_mcp_agent.cc
- M third_party/blink/renderer/core/inspector/inspector_web_mcp_agent.h
- M third_party/blink/renderer/core/script_tools/model_context.cc
- M third_party/blink/renderer/core/script_tools/model_context.h
- A third_party/blink/web_tests/inspector-protocol/webmcp/invoke-tool.js
- A third_party/blink/web_tests/virtual/webmcp/inspector-protocol/webmcp/invoke-tool-expected.txt
Code-Review: +1 by Alex Rudenko, +1 by Anders Hartvoll Ruud, +1 by Mason Freed, +1 by Mark Foltz
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |