actor: Refactor browser-side setup for script tools/webmcp [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 2 comments![]( "Open in Gerrit")
base::OnceCallback<void(Blink Style Guide: Dependencies. `core/` should not depend on `public/web`. `WebDocument` is in `public/web`. Using `WebDocument::ScriptToolError` here introduces a forbidden dependency from `core` to `public/web`.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
base::OnceCallback<Blink Style Guide: Dependencies. `core/` should not depend on `public/web`. `WebDocument` is in `public/web`. To avoid circular dependencies and layering violations, define the error type in `core` (e.g., `ModelContext::ScriptToolError`) and map it to the public API type in the `WebDocument` implementation.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
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. |
Khushal Sagar (Gerrit)
Khushal Sagar voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Ben Greenstein
- David Bokan
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. |
Dominic Farolino (Gerrit)
Dominic Farolino added 1 comment![]( "Open in Gerrit")
std::string target_document_id_;Can this be const? It is in the tool host. Actually, can we just make this the actual token itself, and convert ToString() when we need to compare it to the serialization that the DocumentIdentifierUserData provides?
Related details
- Ben Greenstein
- David Bokan
- Khushal Sagar
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 Bokan (Gerrit)
David Bokan voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Bokan
| Code-Review | +1 |
1 comment
This approach seems fine to me and I'm guessing ScriptTools diverges more from PageTool over time even if you could hack PageTool now to work.
Related details
- Ben Greenstein
- Khushal Sagar
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. |
Khushal Sagar (Gerrit)
Khushal Sagar added 2 comments![]( "Open in Gerrit")
This approach seems fine to me and I'm guessing ScriptTools diverges more from PageTool over time even if you could hack PageTool now to work.
That was exactly my thinking. There wasn't enough code to share with PageTool to justify adding script tool specific hooks to it.
Can this be const? It is in the tool host. Actually, can we just make this the actual token itself, and convert ToString() when we need to compare it to the serialization that the DocumentIdentifierUserData provides?
I can't make this const because all request sub-classes need to support a copy assignment operator.
Excellent point about using base::UnguessableToken. A lot of code uses the string directly because that's what we get from the proto [here](https://source.chromium.org/chromium/chromium/src/+/main:components/optimization_guide/proto/features/common_quality_data.proto;l=798;drc=49cae4c047d2261226d683ad63354dd31caf6dde). But the correct thing would be to deserialize, which validates that it's an unguessable token, and then use that in rest of the code. Done.
Related details
- Ben Greenstein
- Dominic Farolino
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. |
Khushal Sagar (Gerrit)
Khushal Sagar added 2 comments![]( "Open in Gerrit")
Blink Style Guide: Dependencies. `core/` should not depend on `public/web`. `WebDocument` is in `public/web`. Using `WebDocument::ScriptToolError` here introduces a forbidden dependency from `core` to `public/web`.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Acknowledged
Blink Style Guide: Dependencies. `core/` should not depend on `public/web`. `WebDocument` is in `public/web`. To avoid circular dependencies and layering violations, define the error type in `core` (e.g., `ModelContext::ScriptToolError`) and map it to the public API type in the `WebDocument` implementation.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Acknowledged
Related details
- Ben Greenstein
- Dominic Farolino
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Khushal Sagar (Gerrit)
Khushal Sagar voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ 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: chrome/browser/actor/actor_test_util.cc
Insertions: 3, Deletions: 2.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/browser/actor/tools/script_tool_host.cc
Insertions: 5, Deletions: 4.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/browser/actor/browser_action_util.cc
Insertions: 11, Deletions: 6.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/browser/actor/tools/script_tool_request.h
Insertions: 2, Deletions: 2.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/browser/actor/tools/script_tool_request.cc
Insertions: 5, Deletions: 4.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/browser/actor/tools/script_tool_host.h
Insertions: 2, Deletions: 2.
The diff is too large to show. Please review the diff.
```
Change information
actor: Refactor browser-side setup for script tools/webmcp
Use a direct sub-class of TabToolRequest and Tool for browser-side
management of script tool requests instead of using PageTool as the
base.
The behaviour between script tools and other actuation based tools is
different such that this inheritance doesn't make sense. Script tools
don't need to wait for page stabilization and observation shouldn't
be delayed.
- M chrome/browser/actor/BUILD.gn
- M chrome/browser/actor/actor_test_util.cc
- M chrome/browser/actor/browser_action_util.cc
- M chrome/browser/actor/browser_action_util.h
- M chrome/browser/actor/tools/page_tool.cc
- A chrome/browser/actor/tools/script_tool_host.cc
- A chrome/browser/actor/tools/script_tool_host.h
- M chrome/browser/actor/tools/script_tool_request.cc
- M chrome/browser/actor/tools/script_tool_request.h
Code-Review: +1 by David Bokan
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |