[webkit-reviews] review granted: [Bug 177848] REGRESSION (Safari 11): custom <font-face> tag crashes a page : [Attachment 328210] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 177848: REGRESSION (Safari 11): custom <font-face> tag crashes a page
https://bugs.webkit.org/show_bug.cgi?id=177848

Attachment 328210: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 328210
  --> https://bugs.webkit.org/attachment.cgi?id=328210
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328210&action=review

> Source/WebCore/ChangeLog:11
> +	   the descriptors shouldn't accept the universal keywords ("initial",
"inhert", etc.) and our

typo: "inhert"

> Source/WebCore/svg/SVGFontFaceElement.cpp:66
>      CSSPropertyID propId = cssPropertyIdForSVGAttributeName(name);

As long as we are touching this code, can we just write out the word "property"
instead of using "prop"?

> Source/WebCore/svg/SVGFontFaceElement.cpp:69
> +	   auto& mutableProperties = m_fontFaceRule->mutableProperties();

I don’t think this local variable name needs the word "mutable" in it.

> Source/WebCore/svg/SVGFontFaceElement.cpp:70
> +	   mutableProperties.setProperty(propId, value, false);

There is no need to pass the "false" here, it’s the default.

Should take advantage of the return value of
MutableStyleProperties::setProperty. If the function does nothing, then it
returns false and we don’t have to do anything further. Could also skip the
call to rebuildFontFace in that case.

It’s really inelegant that we let the property get set and then remove it
afterward, but I suppose we have to live with that for now.

> Source/WebCore/svg/SVGFontFaceElement.cpp:72
> +	   // Because we're using a CSS property parser to parse a descriptor,
we have to manually disallow the automatic keywords which all properties
accept.

I think the comment needs to go further and emphasize that we let the property
get set, and now have to remove it.

> Source/WebCore/svg/SVGFontFaceElement.cpp:74
> +	       if (parsedValue->isInheritedValue() ||
parsedValue->isInitialValue() || parsedValue->isUnsetValue() ||
parsedValue->isRevertValue())

I worry that this code needs to be updated if we add a new feature such as
"revert" and the person working on CSSValue would have no reason to come here
and update this code. Because of that, I suggest that instead of an explicit
check for these four, you consider adding a function for this purpose in the
CSSValue class. Or maybe you could list the types that are OK instead of the
types that are forbidden.

> Source/WebCore/svg/SVGFontFaceElement.cpp:75
> +		   mutableProperties.setProperty(propId, String(), false);

No need to take advantage of the special case in setProperty, can just call
removeProperty directly:

    mutableProperties.removeProperty(propId);

> LayoutTests/svg/text/font-style-keyword.html:11
> +    <font-face font-style="initial" font-family="any">

This tests one of the four keywords. We should at least test the three others.
And still that seems like only modest test coverage given that there are
separate rules for what is expected for each of the specific attributes.

It is a really loose coupling that CSSFontFace makes assumptions based on how
the CSSParser works, but with some layers of software in between. I think it
might be even better to fix this by making CSSFontFace resilient to unexpected
CSSValue types rather than having some of those functions assume they know
exactly what the types are, while others have reason to and so carefully check
type.