[Webkit-unassigned] [Bug 37144] CSS SVG font doesn't recognize URL without element reference

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=37144


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #52620|review?                     |review-
               Flag|                            |




--- Comment #2 from Darin Adler <darin at apple.com>  2010-04-06 09:05:45 PST ---
(From update of attachment 52620)
>      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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.