[WebMCP] Add navigator.modelContext.ontoolactivated [chromium/src : main]
Fr (Gerrit)
Fr added 1 comment![]( "Open in Gerrit")
Is this what you had in mind with https://github.com/webmachinelearning/webmcp/issues/126#issuecomment-4001724830?
Related details
- Dominic Farolino
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 added 2 comments![]( "Open in Gerrit")
Yep, it looks great. Do we have any way to trigger tools in web test that let us test this event there? Or can we at least add other tests?
Member<Element> element_;Can these members be `const`?
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 2 comments![]( "Open in Gerrit")
Yep, it looks great. Do we have any way to trigger tools in web test that let us test this event there? Or can we at least add other tests?
I've just added web tests.
Can these members be `const`?
Nope. See error messages when I add `const`:
```
../../third_party/blink/renderer/core/events/tool_activated_event.cc:24:16: error: no viable overloaded '='
24 | tool_name_ = initializer->toolName();
| ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~
../../third_party/blink/renderer/platform/wtf/text/wtf_string.h:172:11: note: candidate function not viable: 'this' argument has type 'const String', but method is not marked const
172 | String& operator=(const String&) = default;
| ^
../../third_party/blink/renderer/platform/wtf/text/wtf_string.h:175:11: note: candidate function not viable: 'this' argument has type 'const String', but method is not marked const
175 | String& operator=(String&&) = default;
| ^
../../third_party/blink/renderer/core/events/tool_activated_event.cc:27:14: error: no viable overloaded '='
27 | element_ = initializer->element();
```
Related details
- Dominic Farolino
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 added 3 comments![]( "Open in Gerrit")
Can you make the internal WPT test an external, tentative, WPT? That should be possible since I don't think we're relying on any internal APIs (of course modelContextTesting will be known by another name soon...)
Member<Element> element_;FrCan these members be `const`?
Nope. See error messages when I add `const`:
```
../../third_party/blink/renderer/core/events/tool_activated_event.cc:24:16: error: no viable overloaded '='
24 | tool_name_ = initializer->toolName();
| ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~
../../third_party/blink/renderer/platform/wtf/text/wtf_string.h:172:11: note: candidate function not viable: 'this' argument has type 'const String', but method is not marked const
172 | String& operator=(const String&) = default;
| ^
../../third_party/blink/renderer/platform/wtf/text/wtf_string.h:175:11: note: candidate function not viable: 'this' argument has type 'const String', but method is not marked const
175 | String& operator=(String&&) = default;
| ^
../../third_party/blink/renderer/core/events/tool_activated_event.cc:27:14: error: no viable overloaded '='
27 | element_ = initializer->element();
```
Well you'd have to modify the first constructor's initialization of the two members, to happen in the initialization list as opposed to the ctor body. Maybe it's not worth it. I just like const :)
<input type="submit" />Void elements don't need a trailing close slash, and it's generally recommended to omit it to keep the right mental model of these elements.
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 3 comments![]( "Open in Gerrit")
Can you make the internal WPT test an external, tentative, WPT? That should be possible since I don't think we're relying on any internal APIs (of course modelContextTesting will be known by another name soon...)
How about we do that for all web platform tests that are in third_party/blink/web_tests/fast/webmcp/ except use
Member<Element> element_;FrCan these members be `const`?
Dominic FarolinoNope. See error messages when I add `const`:
```
../../third_party/blink/renderer/core/events/tool_activated_event.cc:24:16: error: no viable overloaded '='
24 | tool_name_ = initializer->toolName();
| ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~
../../third_party/blink/renderer/platform/wtf/text/wtf_string.h:172:11: note: candidate function not viable: 'this' argument has type 'const String', but method is not marked const
172 | String& operator=(const String&) = default;
| ^
../../third_party/blink/renderer/platform/wtf/text/wtf_string.h:175:11: note: candidate function not viable: 'this' argument has type 'const String', but method is not marked const
175 | String& operator=(String&&) = default;
| ^
../../third_party/blink/renderer/core/events/tool_activated_event.cc:27:14: error: no viable overloaded '='
27 | element_ = initializer->element();
```
Well you'd have to modify the first constructor's initialization of the two members, to happen in the initialization list as opposed to the ctor body. Maybe it's not worth it. I just like const :)
Done
Void elements don't need a trailing close slash, and it's generally recommended to omit it to keep the right mental model of these elements.
- Dominic Farolino
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 added 2 comments![]( "Open in Gerrit")
This LGTM. Is there any follow-up we should mention in the CL description about:
- Removing `WebMCPEvent` which seems obsoleted by this CL
- Doing the same with the cancelled event
}I suppose we'll remove this in a separate CL, after some evangelization to folks in the dev trial?
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 2 comments![]( "Open in Gerrit")
This LGTM. Is there any follow-up we should mention in the CL description about:
- Removing `WebMCPEvent` which seems obsoleted by this CL
- Doing the same with the cancelled event
https://chromium-review.googlesource.com/c/chromium/src/+/7653555/ is next indeed. Not sure it's worth mentioning it in the CL description as we'll probably land both.
We can add WebMCPEvent expected removal though.
I suppose we'll remove this in a separate CL, after some evangelization to folks in the dev trial?
Exactly. I wanted to have both toolactivated and toolcancel working in Canary, warn folks, and only then, start to remove WebMCP events.
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. |
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 2 comments![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Code-Review | +1 |
2 comments
FrThis LGTM. Is there any follow-up we should mention in the CL description about:
- Removing `WebMCPEvent` which seems obsoleted by this CL
- Doing the same with the cancelled event
https://chromium-review.googlesource.com/c/chromium/src/+/7653555/ is next indeed. Not sure it's worth mentioning it in the CL description as we'll probably land both.
We can add WebMCPEvent expected removal though.
I think it could be worth a quick mention just for historical completeness!
FrI suppose we'll remove this in a separate CL, after some evangelization to folks in the dev trial?
Exactly. I wanted to have both toolactivated and toolcancel working in Canary, warn folks, and only then, start to remove WebMCP events.
- 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. |