Bug 158112 – [Font Loading] Allow empty strings in FontFace constructor

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[Font Loading] Allow empty strings in FontFace constructor

[Myles C. Maxfield]

Created attachment 279871 [details]
Patch

[Darin Adler]

Comment on attachment 279871 [details]
Patch

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

Seems like half of this patch is additional code that is not needed to implement the desired behavior.

> Source/WebCore/ChangeLog:8
> +        Other browsers accept empty strings and parse them as if they are omitted.

Does the standard mandate this behavior or is it just an unwritten rule?

> Source/WebCore/css/FontFace.cpp:55
> +    if (family.isEmpty())
> +        return nullptr;

This is not needed. The setFamily function will set the exception code to SYNTAX_ERR and the code below will notice that and return nullptr. I don’t think we need to optimize the performance of this failure case, so I suggest omitting this check.

> Source/WebCore/css/FontFace.cpp:76
> +    result->setStyle(descriptors.style.isEmpty() ? ASCIILiteral("normal") : descriptors.style, ec);

Super-irritating that all these default values are replicated here and in FontFace.idl. I suppose we could instead make all the IDL file defaults be empty string instead; it wouldn’t affect behavior and the code generated would be slightly more efficient. But then I suppose the IDL file wouldn’t match the one from the specification.

> Source/WebCore/css/FontFace.cpp:145
> +    if (family.isEmpty()) {
> +        ec = SYNTAX_ERR;
> +        return;
> +    }

I am pretty sure this is not needed. Although I haven’t tested it, I’m almost certain that if you call parseString with an empty string it will return a nullptr. I don’t think we need to optimize the performance of this failure case, so I suggest omitting this check.

> Source/WebCore/css/FontFace.cpp:159
> +    if (style.isEmpty()) {
> +        ec = SYNTAX_ERR;
> +        return;
> +    }

Ditto.

> Source/WebCore/css/FontFace.cpp:173
> +    if (weight.isEmpty()) {
> +        ec = SYNTAX_ERR;
> +        return;
> +    }

Ditto.

> Source/WebCore/css/FontFace.cpp:192
> +    if (unicodeRange.isEmpty()) {
> +        ec = SYNTAX_ERR;
> +        return;
> +    }

Ditto.

> Source/WebCore/css/FontFace.cpp:206
> +    if (variant.isEmpty()) {
> +        ec = SYNTAX_ERR;
> +        return;
> +    }

Ditto.

> Source/WebCore/css/FontFace.cpp:263
> +    if (featureSettings.isEmpty()) {
> +        ec = SYNTAX_ERR;
> +        return;
> +    }

Ditto.

[Myles C. Maxfield]

Comment on attachment 279871 [details]
Patch

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

>> Source/WebCore/css/FontFace.cpp:55
>> +        return nullptr;
> 
> This is not needed. The setFamily function will set the exception code to SYNTAX_ERR and the code below will notice that and return nullptr. I don’t think we need to optimize the performance of this failure case, so I suggest omitting this check.

Good catch.

>> Source/WebCore/css/FontFace.cpp:76
>> +    result->setStyle(descriptors.style.isEmpty() ? ASCIILiteral("normal") : descriptors.style, ec);
> 
> Super-irritating that all these default values are replicated here and in FontFace.idl. I suppose we could instead make all the IDL file defaults be empty string instead; it wouldn’t affect behavior and the code generated would be slightly more efficient. But then I suppose the IDL file wouldn’t match the one from the specification.

It's difficult to decide either way. I definitely don't like duplication, but I also believe that IDL files describe our intent of what we implement. Developers will look at our IDL file to figure stuff like this out.

Maybe a comment...

>> Source/WebCore/css/FontFace.cpp:145
>> +    }
> 
> I am pretty sure this is not needed. Although I haven’t tested it, I’m almost certain that if you call parseString with an empty string it will return a nullptr. I don’t think we need to optimize the performance of this failure case, so I suggest omitting this check.

parseString unconditionally calls CSSParser::parseValue(), whose first line is ASSERT(!string.isEmpty());

I think it's valuable for the constructor to simply call set{Family,Style,Weight,...}(), which means that the null check should be done inside the constructor, not inside (a function called by) those setter functions. (Otherwise, if the check was in parseString(), the set*() functions would set ec and the constructor wouldn't know about it.)

[Myles C. Maxfield]

Committed r201421: <http://trac.webkit.org/changeset/201421>

[Myles C. Maxfield]

Comment on attachment 279871 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        Other browsers accept empty strings and parse them as if they are omitted.
> 
> Does the standard mandate this behavior or is it just an unwritten rule?

The spec says "Parse the family argument, and the members of the descriptors argument, according to the grammars of the corresponding descriptors of the CSS @font-face rule." which seems to me it would dictate that empty strings cause errors. I don't see anything about replacing empty strings with their default value.

So this appears to be an unwritten rule. I found out about it from a web developer saying that their site works in other browsers but not WebKit.

Do you think this is worth updating the spec?

[Darin Adler]

(In reply to comment #5)
> Do you think this is worth updating the spec?

Yes.

[Myles C. Maxfield]

(In reply to comment #6)
> (In reply to comment #5)
> > Do you think this is worth updating the spec?
> 
> Yes.

https://github.com/w3c/csswg-drafts/issues/151