Reject SMIL clock values with out-of-range minutes or seconds [chromium/src : main]
Alison Maher (Gerrit)
Alison Maher added 3 comments![]( "Open in Gerrit")
Reject SMIL clock values with out-of-range minutes or secondsIs this work behind a larger feature flag, or is this behavior shipped and could potentially cause potential compat issues once changed?
The SMIL timing spec requires that minutes be in the range 0-59nit: this looks like it can be re-formatted to remove the extra space at the beginning of each line (if you edit in Gerrit, it should have the option to reformat for you)
if (!parsed_hour || !parsed_min || !parsed_sec || *parsed_min >= 60 ||
*parsed_sec >= 60) {nit: might be worth a comment above here, and the if-statement modified below with a link to the spec indicating why we cap at 60
Related details
- Brandon Maslen
- Giovanni Del Valle
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. |
Giovanni Del Valle (Gerrit)
Giovanni Del Valle added 2 comments![]( "Open in Gerrit")
The SMIL timing spec requires that minutes be in the range 0-59nit: this looks like it can be re-formatted to remove the extra space at the beginning of each line (if you edit in Gerrit, it should have the option to reformat for you)
Done
if (!parsed_hour || !parsed_min || !parsed_sec || *parsed_min >= 60 ||
*parsed_sec >= 60) {nit: might be worth a comment above here, and the if-statement modified below with a link to the spec indicating why we cap at 60
- Alison Maher
- Brandon Maslen
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. |
Giovanni Del Valle (Gerrit)
Giovanni Del Valle added 1 comment![]( "Open in Gerrit")
Reject SMIL clock values with out-of-range minutes or secondsIs this work behind a larger feature flag, or is this behavior shipped and could potentially cause potential compat issues once changed?
I looked into the compat question offline. This appears to be a low-risk spec-alignment fix rather than a new behavior behind a feature flag. The SMIL timing spec has long required minutes/seconds to be in [0, 59], and Firefox/WebKit are making the same change without guards. And if there are authors relying on non-spec behavior regarding the invalid clock values, they would need to update to follow the valid clock values in the spec.
Related details
- Alison Maher
- Brandon Maslen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alison Maher (Gerrit)
Alison Maher voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alison Maher
| Code-Review | +1 |
1 comment
Reject SMIL clock values with out-of-range minutes or secondsGiovanni Del ValleIs this work behind a larger feature flag, or is this behavior shipped and could potentially cause potential compat issues once changed?
I looked into the compat question offline. This appears to be a low-risk spec-alignment fix rather than a new behavior behind a feature flag. The SMIL timing spec has long required minutes/seconds to be in [0, 59], and Firefox/WebKit are making the same change without guards. And if there are authors relying on non-spec behavior regarding the invalid clock values, they would need to update to follow the valid clock values in the spec.
Thanks, this seems reasonable to me. +fs@ since it looks like you've touched this method a bit in the past to confirm treating this as a normal bug fix should be safe enough
Related details
- Brandon Maslen
- Fredrik Söderquist
- Giovanni Del Valle
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 2 comments![]( "Open in Gerrit")
Reject SMIL clock values with out-of-range minutes or secondsGiovanni Del ValleIs this work behind a larger feature flag, or is this behavior shipped and could potentially cause potential compat issues once changed?
Alison MaherI looked into the compat question offline. This appears to be a low-risk spec-alignment fix rather than a new behavior behind a feature flag. The SMIL timing spec has long required minutes/seconds to be in [0, 59], and Firefox/WebKit are making the same change without guards. And if there are authors relying on non-spec behavior regarding the invalid clock values, they would need to update to follow the valid clock values in the spec.
Thanks, this seems reasonable to me. +fs@ since it looks like you've touched this method a bit in the past to confirm treating this as a normal bug fix should be safe enough
I think this change would've been ok to count as a low-risk bug fix. However, since the bug was assigned to me I've implemented a slightly more substantial fix, that this CL will conflict with (a preparatory refactor is in the process of landing as I write this). Please indicate interest/assign the bug to you earlier (but this CL seems to have been created after I assigned the bug to myself).
and seconds be in the range 0-59 (plus optional fractional part)It looks weird with leading spaces. (Also, the flowing of the paragraphs is a bit odd when we're at it.)
Related details
- Alison Maher
- Brandon Maslen
- Giovanni Del Valle
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. |