[webkit-reviews] review denied: [Bug 16854] display title in tooltip onmouseover in SVG : [Attachment 53603] Patch properly walking up to find the <use> shadow parent. Update for reviewer 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 53603: Patch properly walking up to find the <use> shadow parent. 
Update for reviewer comments
https://bugs.webkit.org/attachment.cgi?id=53603&action=review

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

nice patch, though it needs to be tightend up a bit:
 
>  String SVGAElement::title() const
>  {
> -    return getAttribute(XLinkNames::titleAttr);
> +    // If the xlink:title is set (non-empty string), use it.
> +    const AtomicString& title = getAttribute(XLinkNames::titleAttr);
> +    if (title != String())
Please use "if (!title.isEmpty())" here, instead of creating temporary String
objects for comparision.


> +String SVGStyledElement::title() const
> +{
> +    // First we walk up the tree to see if we are inside a <use> shadow
tree.
> +    Node* parent = const_cast<SVGStyledElement*>(this);
> +    while (parent) {
> +	   if (parent->isShadowNode()) {
I'd suggest to use following code, in order to make the code more readable,
less nested scopes:

if (!parent->isShadowNode()) {
    parent = parent->parentNode();
    continue;
}

> +	       // Get the <use> element.
> +	       Node* shadowParent = parent->shadowParentNode();
> +	       if (shadowParent && shadowParent->nodeName() ==
SVGNames::useTag) {
I'd recommend to use a slightly more optimized check here:
if (shadowParent && shadowParent->hasTagName(SVGNames::useTag))
...

> +		   SVGUseElement* useElem =
static_cast<SVGUseElement*>(shadowParent);
> +		   if (useElem) {
Please don't use abbrevations, use "useElement", as identifier, and combine it
with the if statement.
if (SVGUseElement* useElement = ....) { ... }

> +		       // If the <use> title is empty we will keep walking up.
> +		       String useTitle(useElem->title());

> +		       if (useTitle != String())
> +			   return useTitle;
Same as above, just use "if (!useTitle.isEmpty())" instead of direct
comparisions.

> +    // If we aren't an instance in a use, then find the first <title> child
of this element.
> +    Element* child = firstElementChild();
> +    while (child && child->nodeName() != "title" && child->namespaceURI() !=
"http://www.w3.org/2000/svg")
> +	   child = child->nextElementSibling();
Why not use a simple for() loop here, like:

for (Element* child = firstElementChild(); child; child =
child->nextElementSibling()) {
    if (!child->isSVGElement())
	continue;
    if (!child->hasTagName(SVGNames::titleAttr))
	continue;
}

In general you should always try to avoid temporary strings and comparisions.

> +    // If found, return the text contents.
> +    // According to spec, the title on <svg> elements are never returned as
a tooltip.
> +    if (child && nodeName() != "svg")
> +	   return child->innerText();
if (child && !child->hasTagName(SVGNames::svgTag))
    return child->innerText();

Sorry for the lots of corrections, but I think this makes the code much nicer
to read :-)
r- for now, happy to review the next patch.