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

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Jeff Schiller
<codedread at gmail.com>'s request for review:
Bug 16854: display title in tooltip onmouseover in SVG
https://bugs.webkit.org/show_bug.cgi?id=16854

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

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

almost there, some small comments:

>  
> +String SVGStyledElement::title() const
> +{
> +    // Else, we walk up the tree to see if we are inside a <use> shadow
tree.
Slightly adjust the comment to sth. like:
"Walk up the tree, to find out whether we're inside a <use> shadow tree, to
find the right title"

> +    Node* parent = const_cast<SVGStyledElement*>(this);
> +    while (parent) {
> +	   if (!parent->isShadowNode()) {
> +	       parent = parent->parentNode();
> +	       continue;
> +	   }
> +	   
> +	   // Get the <use> element.
> +	   Node* shadowParent = parent->shadowParentNode();
> +	   if (shadowParent && shadowParent->hasTagName(SVGNames::useTag)) {
> +	       if (SVGUseElement* useElement =
static_cast<SVGUseElement*>(shadowParent)) {
No need to do another if() here, just use
SVGUseElement* useElement = ..
the static_cast can not fail.

> +		   // If the <use> title is empty we will keep walking up.
> +		   String useTitle(useElement->title());
> +		   if (!useTitle.isEmpty())
> +		       return useTitle;
Why do you want to keep walking up if the title is empty?
You're not using the result of 'parent' anywhere, and there are no nested
shadow trees. So you can just stop here:
if (useTitle.isEmpty())
    break;
return useTitle;

> +    // If we aren't an instance in a use, then find the first <title> child
of this element.
> +    Element* titleElement = firstElementChild();
> +    for ( ; titleElement; titleElement = titleElement->nextElementSibling())
{
You should remove the first space: for(; titleElement; ...)

> +	   if (!titleElement->hasTagName(SVGNames::titleTag))
> +	       continue;
> +	   if (!titleElement->isSVGElement())
> +	       continue;
I'd do the checks the other way round, because isSVGElement() is cheaper.

> +    // If a title child was found, return the text contents.
> +    // According to spec, we should not return titles when hovering over
<svg> elements (those 
> +    // <title> elements are the title of the document, not a tooltip).
> +    if (titleElement && !this->hasTagName(SVGNames::svgTag))
> +	   return titleElement->innerText();
You can omit the "this->" here.
Oh and you need to be careful, and recheck wheter titleElement is actually a
SVGNames::titleTag element.
Say you're hovering over a node with a single child, which is not a <title>
element, then "titleElement" will store a pointer to it, and you're just
returning it's innerText below, no matter whether it's a <title> or not.
To summarize, I'd rewrite as follows:
if (titleElement && titleElement->hasTagName(SVGNames::titleTag))
    return titleElement->innerText();

This saves the svgTag check completly :-)

Sorry that I missed some parts during the last review! Do you have commit
access btw? Or do you want to me to cq+ after you've uploaded the next version?