ctx.font = "" asserts in CSS parser
<rdar://problem/56391016>
Created attachment 381252 [details] Patch
Comment on attachment 381252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381252&action=review r=me > Source/WebCore/ChangeLog:13 > + will assert. This was the only case I could find where we sidestepped > + most of the parsing infrastructure and injected a raw string. For the record (which I think is what you were referring to), there are a number of other places where we call `CSSParser::parseValue`, and most of them already do a check for an empty string before calling it, but not all of them. The only other one I could is `CSSFontFaceSet::matchingFacesExcludingPreinstalledFonts` (which looks to be reachable from JavaScript via `FontFaceSet.prototype.check`), so perhaps we should add a similar check there as well. > LayoutTests/http/wpt/2dcontext/text-styles/2d.text.font.parse.invalid.html:21 > +ctx.font = '20px serif'; I realize this is a WPT, but should we have a test to check that setting a valid value after an invalid one does in fact change the `font`, just to make sure that an invalid value doesn't "permanently" mess everything up? ``` ctx.font = '20px serif'; ctx.font = 'invalid'; ctx.font = '10px sans-serif'; _assertSame(ctx.font, '10px sans-serif', "ctx.font", "'10px sans-serif'"); ```
Comment on attachment 381252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381252&action=review >> LayoutTests/http/wpt/2dcontext/text-styles/2d.text.font.parse.invalid.html:21 >> +ctx.font = '20px serif'; > > I realize this is a WPT, but should we have a test to check that setting a valid value after an invalid one does in fact change the `font`, just to make sure that an invalid value doesn't "permanently" mess everything up? > ``` > ctx.font = '20px serif'; > ctx.font = 'invalid'; > ctx.font = '10px sans-serif'; > _assertSame(ctx.font, '10px sans-serif', "ctx.font", "'10px sans-serif'"); > ``` We probably should. Feel free to make a WPT request :)
Will do FontFaceSet separately.
Committed r251270: <https://trac.webkit.org/changeset/251270>