[webkit-reviews] review granted: [Bug 203127] ctx.font = "" asserts in CSS parser : [Attachment 381252] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Dean Jackson <dino at apple.com>'s
request for review:
Bug 203127: ctx.font = "" asserts in CSS parser
https://bugs.webkit.org/show_bug.cgi?id=203127

Attachment 381252: Patch

https://bugs.webkit.org/attachment.cgi?id=381252&action=review




--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 381252
  --> https://bugs.webkit.org/attachment.cgi?id=381252
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'");
```