Bug 37144 - CSS SVG font doesn't recognize URL without element reference
Summary: CSS SVG font doesn't recognize URL without element reference
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-06 04:09 PDT by Hajime Morrita
Modified: 2010-05-11 19:16 PDT (History)
1 user (show)

See Also:


Attachments
patch v0 (122.59 KB, patch)
2010-04-06 04:17 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v1 (136.51 KB, patch)
2010-05-06 01:30 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v2 (136.56 KB, patch)
2010-05-11 02:12 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2010-04-06 04:09:46 PDT
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)
Comment 1 Hajime Morrita 2010-04-06 04:17:16 PDT
Created attachment 52620 [details]
patch v0
Comment 2 Darin Adler 2010-04-06 09:05:45 PDT
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?
Comment 3 Hajime Morrita 2010-04-06 23:00:33 PDT
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?
Comment 4 Hajime Morrita 2010-05-06 01:30:05 PDT
Created attachment 55212 [details]
patch v1
Comment 5 Hajime Morrita 2010-05-06 01:33:44 PDT
Now Bug 37187 was fixed and I go back here.
Updated patch got advices on #2 - added reasonable tests, did cleanup ugly code.
Comment 6 Dirk Schulze 2010-05-11 01:28:02 PDT
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?
Comment 7 Hajime Morrita 2010-05-11 02:12:16 PDT
Created attachment 55679 [details]
patch v2
Comment 8 Hajime Morrita 2010-05-11 02:16:48 PDT
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?
Comment 9 Dirk Schulze 2010-05-11 04:10:11 PDT
Comment on attachment 55679 [details]
patch v2

LGTM. r=me.
Comment 10 Hajime Morrita 2010-05-11 17:35:50 PDT
Comment on attachment 55679 [details]
patch v2

Thanks! I'll land this manually.
Comment 11 Hajime Morrita 2010-05-11 19:15:38 PDT
Committed r59194: <http://trac.webkit.org/changeset/59194>