[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