[webkit-reviews] review denied: [Bug 37144] CSS SVG font doesn't recognize URL without element reference : [Attachment 52620] patch v0

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied MORITA Hajime <morrita at google.com>'s
request for review:
Bug 37144: CSS SVG font doesn't recognize URL without element reference
https://bugs.webkit.org/show_bug.cgi?id=37144

Attachment 52620: patch v0
https://bugs.webkit.org/attachment.cgi?id=52620&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
>      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?