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. |