[webmcp] Only include supported types in FormModelContextParameterMissingName [chromium/src : main]
Philip Pfaffe (Gerrit)
Philip Pfaffe added 1 comment![]( "Open in Gerrit")
bool FormMCPSchema::IsSupportedParameter(ListedElement& control) const {How likely is this list to change? I.e., how likely is it that this introduces false positives or false negatives down the line, whenever a new type gets supported or unsupported?
Related details
- Anders Hartvoll Ruud
- Lusa Zhan
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 3 comments![]( "Open in Gerrit")
We should not emit issues for unsupported types.Not a strong opinion, but ... are you sure about that? You could look at this way:
* A named form control group isn't supported -> "ISSUE: unsupported combination of form controls".
* A form control group is missing a name -> "ISSUE: you forgot the name".
These seem fair to warn about independently? (Or should one mistake cancel the other?)
if (!IsSupportedParameter(*element)) {
continue;
}This is not correct. We need to check the type against `it`, not `element`.
bool FormMCPSchema::IsSupportedParameter(ListedElement& control) const {How likely is this list to change? I.e., how likely is it that this introduces false positives or false negatives down the line, whenever a new type gets supported or unsupported?
Yeah, I think we should drop this and instead just do:
```
bool required_unused;
if (!ComputeParameterSchema("", required_unused)) {
// Don't emit an issue if the parameter is unsupported anyway.
return;
}
```
Related details
- Lusa Zhan
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. |
Lusa Zhan (Gerrit)
Lusa Zhan added 3 comments![]( "Open in Gerrit")
We should not emit issues for unsupported types.Not a strong opinion, but ... are you sure about that? You could look at this way:
* A named form control group isn't supported -> "ISSUE: unsupported combination of form controls".
* A form control group is missing a name -> "ISSUE: you forgot the name".These seem fair to warn about independently? (Or should one mistake cancel the other?)
The description is a little poorly worded, actually.
"We should not emit issues for unsupported types." should be "we should not emit FormModelContextParameterMissingName for unsupported types".
1. A form control group is missing a name -> FormModelContextParameterMissingName
2. A named form control group isn't supported -> "ISSUE: unsupported combination of form controls". (we should consider adding this)
3. An unsupported from control group is missing a name -> no issue
I think having 3 be an issue would just introduce unnecessary noise. If a form control is not supported, why have an issue for it? If the dev adds a name, it still wouldn't be picked up by WebMCP right?
if (!IsSupportedParameter(*element)) {
continue;
}This is not correct. We need to check the type against `it`, not `element`.
Could you explain why?
I think for the empty name `""`, we do want the type check to be by element, since `name_to_controls_[""]` may include different types. All controls without a name get dumped into `name_to_controls_[""]` even if they're not really in a control group.
bool FormMCPSchema::IsSupportedParameter(ListedElement& control) const {Anders Hartvoll RuudHow likely is this list to change? I.e., how likely is it that this introduces false positives or false negatives down the line, whenever a new type gets supported or unsupported?
Yeah, I think we should drop this and instead just do:
```
bool required_unused;
if (!ComputeParameterSchema("", required_unused)) {
// Don't emit an issue if the parameter is unsupported anyway.
return;
}
```
- Anders Hartvoll Ruud
- 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. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 2 comments![]( "Open in Gerrit")
We should not emit issues for unsupported types.Lusa ZhanNot a strong opinion, but ... are you sure about that? You could look at this way:
* A named form control group isn't supported -> "ISSUE: unsupported combination of form controls".
* A form control group is missing a name -> "ISSUE: you forgot the name".These seem fair to warn about independently? (Or should one mistake cancel the other?)
The description is a little poorly worded, actually.
"We should not emit issues for unsupported types." should be "we should not emit FormModelContextParameterMissingName for unsupported types".1. A form control group is missing a name -> FormModelContextParameterMissingName
2. A named form control group isn't supported -> "ISSUE: unsupported combination of form controls". (we should consider adding this)
3. An unsupported from control group is missing a name -> no issueI think having 3 be an issue would just introduce unnecessary noise. If a form control is not supported, why have an issue for it? If the dev adds a name, it still wouldn't be picked up by WebMCP right?
Marked as resolved.
if (!IsSupportedParameter(*element)) {
continue;
}Lusa ZhanThis is not correct. We need to check the type against `it`, not `element`.
Could you explain why?
I think for the empty name `""`, we do want the type check to be by element, since `name_to_controls_[""]` may include different types. All controls without a name get dumped into `name_to_controls_[""]` even if they're not really in a control group.
Ah, OK, you want to treat each element as if it was intended as standing alone in a group.
We cannot generally answer the question "does this element constitute a valid parameter", because the answer depends on the other elements in the same group.
So `IsSupportedParameter(ListedElement&)` needs to be `IsSupportedParameter(const ControlVector&)` to stay consistent. I would just create the temporary vector at the call site, for now. (We can spanify this separately afterwards to make it nicer.)
Related details
- Lusa Zhan
- 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. |
Lusa Zhan (Gerrit)
Lusa Zhan added 1 comment![]( "Open in Gerrit")
if (!IsSupportedParameter(*element)) {
continue;
}Lusa ZhanThis is not correct. We need to check the type against `it`, not `element`.
Anders Hartvoll RuudCould you explain why?
I think for the empty name `""`, we do want the type check to be by element, since `name_to_controls_[""]` may include different types. All controls without a name get dumped into `name_to_controls_[""]` even if they're not really in a control group.
Ah, OK, you want to treat each element as if it was intended as standing alone in a group.
We cannot generally answer the question "does this element constitute a valid parameter", because the answer depends on the other elements in the same group.
So `IsSupportedParameter(ListedElement&)` needs to be `IsSupportedParameter(const ControlVector&)` to stay consistent. I would just create the temporary vector at the call site, for now. (We can spanify this separately afterwards to make it nicer.)
- 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 voted Code-Review+1![]( "Open in Gerrit")
- Lusa Zhan
- Philip Pfaffe
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |