Bug 37144 – CSS SVG font doesn't recognize URL without element reference

• Component: CSS
• Status: RESOLVED FIXED

[Hajime Morrita]

SVG font face with element reference like following works well:

        @font-face {
            font-family: 'SVGraffiti';
            src: url("resources/graffiti.svg#SVGraffiti") format(svg)
        }

But one without element reference doen't work:

        @font-face {
            font-family: 'SVGraffiti';
            src: url("resources/graffiti.svg") format(svg) /* Note that URL has no fragment (like #xxxx) */
        }

CSS Font spec http://www.w3.org/TR/css3-fonts/ says " If the element reference is omitted, a reference to the first defined font is implied." (4.3)

[Hajime Morrita]

Created attachment 52620 [details]
patch v0

[Darin Adler]

Comment on attachment 52620 [details]
patch v0

>      unsigned fonts = list->length();

Not your fault, since this is existing code, but "fonts" is a terrible name for a variable that holds a count. A variable named fonts should hold fonts. The variable should be named listLength or numFonts or fontCount.

> +    if (!fontName.isEmpty() && 0 < fonts)

"0 < fonts" is an unusual way to write this condition, and not in keeping with the WebKit coding style. In the WebKit project we would write this as:

    if (!fontName.isEmpty() && listLength)

Or something along those lines.

I think the logic you wrote is backwards. Your new code runs if fontName is non-empty, but your comment makes it sound like you should handle the case where the font name is empty. We need a test case that uses a font name to choose a font other than the first one in the list, otherwise the code is not sufficiently tested.

The code is too oblique. Instead I suggest you write something more like this:

    if (fontName.isEmpty()) {
        // If there is no font name, just use the first font in the list.
        if (!listLength)
            return 0;
        ASSERT(list->item(0)->hasTagName(SVGNames::fontTag));
        return static_cast<SVGFontElement*>(list->item(0));
    }

Is it right to use the first font in the list and ignore the others? The test case doesn't test that behavior, and I think it should. What do other browsers do? What does the SVG standard say about this?

[Hajime Morrita]

Hi darin, thank you for reviewing!

During taking the feedback, I noticed that the last patch is broken and Bug 37187 blocks this change.
So I'll fix Bug 37187 first, then come back here.


(In reply to comment #2)
> (From update of attachment 52620 [details])
> >      unsigned fonts = list->length();
> 
> Not your fault, since this is existing code, but "fonts" is a terrible name for
> a variable that holds a count. A variable named fonts should hold fonts. The
> variable should be named listLength or numFonts or fontCount.
> 
> > +    if (!fontName.isEmpty() && 0 < fonts)
> 
> "0 < fonts" is an unusual way to write this condition, and not in keeping with
> the WebKit coding style. In the WebKit project we would write this as:
> 
>     if (!fontName.isEmpty() && listLength)
> 
> Or something along those lines.
> 
> I think the logic you wrote is backwards. Your new code runs if fontName is
> non-empty, but your comment makes it sound like you should handle the case
> where the font name is empty. We need a test case that uses a font name to
> choose a font other than the first one in the list, otherwise the code is not
> sufficiently tested.
> 
> The code is too oblique. Instead I suggest you write something more like this:
> 
>     if (fontName.isEmpty()) {
>         // If there is no font name, just use the first font in the list.
>         if (!listLength)
>             return 0;
>         ASSERT(list->item(0)->hasTagName(SVGNames::fontTag));
>         return static_cast<SVGFontElement*>(list->item(0));
>     }
> 
> Is it right to use the first font in the list and ignore the others? The test
> case doesn't test that behavior, and I think it should. What do other browsers
> do? What does the SVG standard say about this?

[Hajime Morrita]

Created attachment 55212 [details]
patch v1

[Hajime Morrita]

Now Bug 37187 was fixed and I go back here.
Updated patch got advices on #2 - added reasonable tests, did cleanup ugly code.

[Dirk Schulze]

Comment on attachment 55212 [details]
patch v1

WebCore/loader/CachedFont.cpp:170
 +          ASSERT(node && node->hasTagName(SVGNames::fontTag));
Can you put this assert before the if and get rid of the assert in the loop? Normaly it's better to take an assert per test.

We already have SVGFreeSans.svg in svg/custom/resources. Can't you use this font?

[Hajime Morrita]

Created attachment 55679 [details]
patch v2

[Hajime Morrita]

Krit, thank you for reviewing!
I updated the patch.

(In reply to comment #6)
> (From update of attachment 55212 [details])
> WebCore/loader/CachedFont.cpp:170
>  +          ASSERT(node && node->hasTagName(SVGNames::fontTag));
> Can you put this assert before the if and get rid of the assert in the loop? Normaly it's better to take an assert per test.
Fixed.

> We already have SVGFreeSans.svg in svg/custom/resources. Can't you use this font?
Thank you for pointing this out.
But the test need the svg file that contains 2 <font> elements, 
because the test ensures that the code can skip first <font> and pick next one.

Could you review again please?

[Dirk Schulze]

Comment on attachment 55679 [details]
patch v2

LGTM. r=me.

[Hajime Morrita]

Comment on attachment 55679 [details]
patch v2

Thanks! I'll land this manually.

[Hajime Morrita]

Committed r59194: <http://trac.webkit.org/changeset/59194>