WebMCP: Queue declarative tool registration and schema calculation [chromium/src : main]
Dominic Farolino (Gerrit)
Dominic Farolino voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Commit-Queue | +1 |
1 comment
Found more things I could clean up. As a result of moving to a "queue a task to register/unregister declarative tool" model, I found I could remove all of the `toolchange` task queuing infra in model_context.cc, and move the input schema diffing from there, to the method in HTMLFormElement that performs tool registration.
Related details
- Mason Freed
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 added 2 comments![]( "Open in Gerrit")
tool_data->RefreshDeclarativeInputSchema();Is this still needed?
navigator.modelContextTesting.ontoolchange = () => {Shall we use addEventListener instead there in case the test also uses navigator.modelContextTesting.ontoolchange which would be overridden IIUC?
Related details
- Dominic Farolino
- 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. |
Philip Pfaffe (Gerrit)
Philip Pfaffe added 1 comment![]( "Open in Gerrit")
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 2 comments![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Commit-Queue | +1 |
2 comments
tool_data->RefreshDeclarativeInputSchema();Dominic FarolinoIs this still needed?
I tried to remove it but some other listTools() tests still rely on it for some subtle reason, and since I'm not focusing on that API as much in this CL, my goal is remove this after without piling onto this CL.
model_context_->NotifyToolChange();OK this crap is ugly! But necessary to get these unit tests to work since there is no browser process to fire `NotifyToolChange()` in unit tests, so we have to "simulate" one here. It's just a hack to get around that until we move all of these unit tests over to WPTs, which I'll do later (I don't want to block merging this PR back to M149 on this).
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. |
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Commit-Queue | +1 |
1 comment
Shall we use addEventListener instead there in case the test also uses navigator.modelContextTesting.ontoolchange which would be overridden IIUC?
Sounds reasonable. Done.
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. |
Dominic Farolino (Gerrit)
Dominic Farolino voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
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. |
Dominic Farolino (Gerrit)
Dominic Farolino voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
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 12 comments![]( "Open in Gerrit")
I think this looks great. Lots of comments, but most are very small.
to make them wait on `toolchange` being fired before accessing the tool.Kudos on the 75 line commit description. 😊
TaskHandle mcp_registration_task_handle_;nit: maybe just `mcp_registration_task_`?
Side note, I kind of hate that we mix and match "webmcp" and "mcp" everywhere. But let's not solve that now I guess.
// regard to `this`, or when a control's attributes are changed. It will only
// queue a task to register a new tool if `this` already has an existingI wonder if it would be possible to CHECK that it's only called if `this` has a tool?
if (!mcp_registration_task_handle_.IsActive()) {This makes me wonder - should we re-schedule it instead of leaving it where it is in the task queue? I can't think of why, but perhaps it's better that you push it out each time we have a new update? (If you can't think of a good reason either, then this is ok as-is.)
if (name == html_names::kToolnameAttr ||
name == html_names::kTooldescriptionAttr) {
if (params.old_value != params.new_value) {
ScheduleDeclarativeWebMCPToolRegistration();
}
}nit:
```
if ((name == html_names::kToolnameAttr ||
name == html_names::kTooldescriptionAttr) &&
(params.old_value != params.new_value)) {
ScheduleDeclarativeWebMCPToolRegistration();
}
```
void HTMLFormElement::ScheduleWebMCPSchemaUpdate() {
if (!RuntimeEnabledFeatures::WebMCPEnabled(GetExecutionContext())) {
return;
}
if (!IsValidWebMCPForm()) {
return;
}
ScheduleDeclarativeWebMCPToolRegistration();
}This is a pretty thin wrapper - perhaps we just remove it and update the few remaining callers to call `ScheduleDeclarativeWebMCPToolRegistration` directly?
model_context_->NotifyToolChange();OK this crap is ugly! But necessary to get these unit tests to work since there is no browser process to fire `NotifyToolChange()` in unit tests, so we have to "simulate" one here. It's just a hack to get around that until we move all of these unit tests over to WPTs, which I'll do later (I don't want to block merging this PR back to M149 on this).
Ack. 😊
async function waitForFormToolSchemaToMatch(expected_schema) {Maybe move this/these to the utility `.js` files?
navigator.modelContextTesting.ontoolchange = check;How come we can't switch these tests to `modelContext` and just use `waitForTool()`?
nit: extra blank lines. I'd get rid of both of them. This comment applies to most tests above.
Also, why not move `assert_true(!!navigator.modelContext);` into `waitForTool()`? Then these are all just one line additions.
Related details
- Dominic Farolino
- 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. |
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 10 comments![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Commit-Queue | +1 |
10 comments
nit: maybe just `mcp_registration_task_`?
Side note, I kind of hate that we mix and match "webmcp" and "mcp" everywhere. But let's not solve that now I guess.
Agreed. I have a personal goal to make everything consistently named in the future.
nit: maybe remove the extra blank line
Done, but I also just removed the last two text lines here anyways, since I think they were superfluous.
// regard to `this`, or when a control's attributes are changed. It will only
// queue a task to register a new tool if `this` already has an existingI wonder if it would be possible to CHECK that it's only called if `this` has a tool?
In the current state, no. That's what [this check protects against](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/html_form_element.cc;l=1632-1634;drc=c065efc537674efbf5ed6292606039bcc6a22f6d). A form control can become associated with a form that doesn't have `toolname`/`tooldescription` attributes, and therefore impact the form's input schema, since there is no tool. Does that make sense?
This makes me wonder - should we re-schedule it instead of leaving it where it is in the task queue? I can't think of why, but perhaps it's better that you push it out each time we have a new update? (If you can't think of a good reason either, then this is ok as-is.)
I can't think of a good reason either where it would *actually* matter, and while I kind of like the intuition of "pushing [the task] out" each time, I implemented and it looked a little weird:
- Either we pull the `.Cancel()` out unconditionally above the `if (!is_valid_mcp_form)` block, or;
- We duplicate it by keeping it right where it is in there, and ALSO adding it on the line that you made this comment, just before we re-queue.
I have thoughts on how to simplify and restructure this whole method, so I'll save those for another CL I think, and continue with the current not-pushing-the-task-out approach we have here. Thanks for the comment.
if (name == html_names::kToolnameAttr ||
name == html_names::kTooldescriptionAttr) {
if (params.old_value != params.new_value) {
ScheduleDeclarativeWebMCPToolRegistration();
}
}nit:
```
if ((name == html_names::kToolnameAttr ||
name == html_names::kTooldescriptionAttr) &&
(params.old_value != params.new_value)) {
ScheduleDeclarativeWebMCPToolRegistration();
}
```
Done
void HTMLFormElement::ScheduleWebMCPSchemaUpdate() {
if (!RuntimeEnabledFeatures::WebMCPEnabled(GetExecutionContext())) {
return;
}
if (!IsValidWebMCPForm()) {
return;
}
ScheduleDeclarativeWebMCPToolRegistration();
}This is a pretty thin wrapper - perhaps we just remove it and update the few remaining callers to call `ScheduleDeclarativeWebMCPToolRegistration` directly?
Hmm, it is a pretty thing wrapper. However, it does do less work, than `ScheduleDeclarativeWebMCPToolRegistration()`, which is why I assume it exists. For associating of every single for control element, regardless of WebMCP usage, this method runs, so it's best to have it stop after a simple pointer-is-null check (which is what `if (!IsValidWebMCPForm()) {` does), rather than entering `ScheduleDeclarativeWebMCPToolRegistration()` and grabbing attributes and running `isConnected()` all to do nothing in 99.9% of forms, where `is_valid_mcp_tool` is null.
But maybe the performance implications of entering that method aren't actually bad (the attribute getter does have "fast" in the name). What do you think? Basically I think this indirection exists for performance reasons, but I'm not sure how serious it is.
model_context_->NotifyToolChange();Mason FreedOK this crap is ugly! But necessary to get these unit tests to work since there is no browser process to fire `NotifyToolChange()` in unit tests, so we have to "simulate" one here. It's just a hack to get around that until we move all of these unit tests over to WPTs, which I'll do later (I don't want to block merging this PR back to M149 on this).
Ack. 😊
Done
async function waitForFormToolSchemaToMatch(expected_schema) {Maybe move this/these to the utility `.js` files?
Created a new `helpers.js` file in `external/wpt/webmcp/resources`, and referenced it from this file. But I think the wpt_internal/ and other tests cannot reference this file, so this is the only test that uses the helper. But once we remove the `modelContextTesting` API and move all the tests here, then others will be able to make use of it.
How come we can't switch these tests to `modelContext` and just use `waitForTool()`?
The only reason is because `modelContext` is gated on secure context, and this file is not HTTPs. But yeah we can rename it and use the new API accordingly. Done.
nit: extra blank lines. I'd get rid of both of them. This comment applies to most tests above.
Also, why not move `assert_true(!!navigator.modelContext);` into `waitForTool()`? Then these are all just one line additions.
Done, and I made it:
```
assert_implements(navigator.modelContextTesting, "modelContext is available");
```
which I think is slightly better.
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 voted and added 6 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
6 comments
// regard to `this`, or when a control's attributes are changed. It will only
// queue a task to register a new tool if `this` already has an existingDominic FarolinoI wonder if it would be possible to CHECK that it's only called if `this` has a tool?
In the current state, no. That's what [this check protects against](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/html_form_element.cc;l=1632-1634;drc=c065efc537674efbf5ed6292606039bcc6a22f6d). A form control can become associated with a form that doesn't have `toolname`/`tooldescription` attributes, and therefore impact the form's input schema, since there is no tool. Does that make sense?
Yep, makes sense. Thanks.
if (!mcp_registration_task_handle_.IsActive()) {Dominic FarolinoThis makes me wonder - should we re-schedule it instead of leaving it where it is in the task queue? I can't think of why, but perhaps it's better that you push it out each time we have a new update? (If you can't think of a good reason either, then this is ok as-is.)
I can't think of a good reason either where it would *actually* matter, and while I kind of like the intuition of "pushing [the task] out" each time, I implemented and it looked a little weird:
- Either we pull the `.Cancel()` out unconditionally above the `if (!is_valid_mcp_form)` block, or;
- We duplicate it by keeping it right where it is in there, and ALSO adding it on the line that you made this comment, just before we re-queue.I have thoughts on how to simplify and restructure this whole method, so I'll save those for another CL I think, and continue with the current not-pushing-the-task-out approach we have here. Thanks for the comment.
Ok, this is good as-is. Thanks for thinking about it.
void HTMLFormElement::ScheduleWebMCPSchemaUpdate() {
if (!RuntimeEnabledFeatures::WebMCPEnabled(GetExecutionContext())) {
return;
}
if (!IsValidWebMCPForm()) {
return;
}
ScheduleDeclarativeWebMCPToolRegistration();
}Dominic FarolinoThis is a pretty thin wrapper - perhaps we just remove it and update the few remaining callers to call `ScheduleDeclarativeWebMCPToolRegistration` directly?
Hmm, it is a pretty thing wrapper. However, it does do less work, than `ScheduleDeclarativeWebMCPToolRegistration()`, which is why I assume it exists. For associating of every single for control element, regardless of WebMCP usage, this method runs, so it's best to have it stop after a simple pointer-is-null check (which is what `if (!IsValidWebMCPForm()) {` does), rather than entering `ScheduleDeclarativeWebMCPToolRegistration()` and grabbing attributes and running `isConnected()` all to do nothing in 99.9% of forms, where `is_valid_mcp_tool` is null.
But maybe the performance implications of entering that method aren't actually bad (the attribute getter does have "fast" in the name). What do you think? Basically I think this indirection exists for performance reasons, but I'm not sure how serious it is.
Right, I guess I was assuming you'd change the existing (4?) callers of ScheduleWebMCPSchemaUpdate() to something like
```
if (RuntimeEnabledFeatures::WebMCPEnabled(GetExecutionContext()) &&
IsValidWebMCPForm() {
ScheduleDeclarativeWebMCPToolRegistration();
}
```
The feature check is already present in a few cases. Perhaps that's too verbose. I guess at minimum, if you want to leave this, rename it to something like `ScheduleDeclarativeWebMCPToolRegistrationIfActive()` or something? Otherwise, it's a bit confusing to figure out which of the "Schedule*" functions to call at a given location.
async function waitForFormToolSchemaToMatch(expected_schema) {Dominic FarolinoMaybe move this/these to the utility `.js` files?
Created a new `helpers.js` file in `external/wpt/webmcp/resources`, and referenced it from this file. But I think the wpt_internal/ and other tests cannot reference this file, so this is the only test that uses the helper. But once we remove the `modelContextTesting` API and move all the tests here, then others will be able to make use of it.
Lovely
navigator.modelContextTesting.ontoolchange = check;Dominic FarolinoHow come we can't switch these tests to `modelContext` and just use `waitForTool()`?
The only reason is because `modelContext` is gated on secure context, and this file is not HTTPs. But yeah we can rename it and use the new API accordingly. Done.
Nice, thanks. I didn't realize WebMCP required secure context! TIL.
Related details
- Dominic Farolino
- 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 1 comment![]( "Open in Gerrit")
navigator.modelContextTesting.ontoolchange = check;Dominic FarolinoHow come we can't switch these tests to `modelContext` and just use `waitForTool()`?
Mason FreedThe only reason is because `modelContext` is gated on secure context, and this file is not HTTPs. But yeah we can rename it and use the new API accordingly. Done.
Nice, thanks. I didn't realize WebMCP required secure context! TIL.
FWIW, I've filed https://b.corp.google.com/issues/503588985 last month when I've noticed this.
Related details
- 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. |