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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Apr 18 02:21:17 PDT 2010


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


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #53603|review?                     |review-
               Flag|                            |




--- Comment #14 from Nikolas Zimmermann <zimmermann at kde.org>  2010-04-18 02:21:17 PST ---
(From update of attachment 53603)
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.

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



More information about the webkit-unassigned mailing list