[Webkit-unassigned] [Bug 16854] display title in tooltip onmouseover in SVG

[bugzilla-daemon at webkit.org]

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


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #53881|                            |review-
               Flag|                            |




--- Comment #22 from Nikolas Zimmermann <zimmermann at kde.org>  2010-04-21 00:08:38 PST ---
(From update of attachment 53881)
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 :-)

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