[webkit-reviews] review denied: [Bug 16854] display title in tooltip onmouseover in SVG : [Attachment 53881] Patch addressing Niko's comments #2

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied  review:
Bug 16854: display title in tooltip onmouseover in SVG
https://bugs.webkit.org/show_bug.cgi?id=16854

Attachment 53881: Patch addressing Niko's comments #2
https://bugs.webkit.org/attachment.cgi?id=53881&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Hi Jeff,

the patch is almost perfect! Good that you've moved the svgTag check to the
beginning, clearer now.

I still see a small issue with "titleElement", let me explain with your code:

> +    Element* titleElement = firstElementChild();
> +    for (; titleElement; titleElement = titleElement->nextElementSibling())
{
> +	   if (titleElement->hasTagName(SVGNames::titleTag) &&
titleElement->isSVGElement())
> +	       break;
> +    }
Ok, you are beginning with the firstElementChild() of a certain
SVGStyledElement. You loop over all children, take this as example:

<rect>
   <animateColor/>
</rect>

The titleElement->hasTagName(SVGNames::titleTag) check won't be true, as it's
an <animateColor> element. As there is no next sibling,
you will just exit the loop, w/o breaking early. That means "titleElement" now
stores the pointer to the <animateColor> element.

> +    // If a title child was found, return the text contents.
> +    if (titleElement)
> +	   return titleElement->innerText();

And you end up returning the "innerText" of the <animateColor> element here.
Hope you see the problem. You basically need to recheck the same condition
before returning the innerText(), aka:
if (titleELement && titleElement->hasTagName(SVGNames::titleTag) ... )
     return titleElement->innerText();

This is a bit awkward though, so I suggest to rewrite the whole loop, as
follows:

<suggestion>
bool foundTitleElement = false;
Element* titleElement = firstElementChild();
for (; titleElement; titleElement = titleElement->nextElementSibling()) {
    foundTitleElement = titleElement->hasTagName(SVGNames::titleTag) &&
titleElement->isSVGElement();
    if (foundTitleElement)
	break;
}

if (foundTitleElement) {
    ASSERT(titleElement);
    return titleElement->innerText();
}
</suggestion>

This way you save redoing the same check twice, with the drawback of an extra
bool, that is negligable though.
Do you have commit access, btw?

Hope you're not getting bored by this patch :-)