[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





--- Comment #3 from MORITA Hajime <morrita at google.com>  2010-04-06 23:00:33 PST ---
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?

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