[webkit-reviews] review denied: [Bug 69966] Eliminate separate RenderStyle for visited link style : [Attachment 110806] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 13 00:56:19 PDT 2011

Nikolas Zimmermann <zimmermann at kde.org> has denied Antti Koivisto
<koivisto at iki.fi>'s request for review:
Bug 69966: Eliminate separate RenderStyle for visited link style

Attachment 110806: patch

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=110806&action=review

Great patch, some comments though leading to r-:

> Source/WebCore/css/CSSStyleApplyProperty.cpp:243
> +	   if (m_default && !color.isValid()) {
> +	       applyColorValue(selector,
> +	   } else

Unnecessary braces.

> Source/WebCore/css/CSSStyleSelector.cpp:2178
> +	   for (int i = startIndex; i <= endIndex; i++) {

Micro optimization: ++i.

> Source/WebCore/css/CSSStyleSelector.cpp:2191
> +    for (int i = startIndex; i <= endIndex; i++)	   


> Source/WebCore/css/SVGCSSStyleSelector.cpp:235
> -		   svgstyle->setFillPaint(svgParentStyle->fillPaintType(),
svgParentStyle->fillPaintColor(), svgParentStyle->fillPaintUri());
setVisitedLinkFillPaint, svgParentStyle->fillPaintType(),
svgParentStyle->fillPaintColor(), svgParentStyle->fillPaintUri());

Ok, as this APPLY_VISITED_LINK_VALID_PAINT macro is actually only used for two
properties: fill & stroke, I'd propose to create two
setFillPaint/setStrokePaint methods that replace this macro alltogether.
I'd be fine with the macro usage, if it would help to replace dozens of this
kind of functions, but as it's only about two, I'd rather avoid creating a new

> Source/WebCore/css/SVGCSSStyleSelector.cpp:239
> -		  
setVisitedLinkFillPaint, SVGRenderStyle::initialFillPaintType(),

Another general concern: For SVGs fill/stroke properties you also need an URI,
beside the color & paint type. The rest of the patch indicates you're not
passing on a URI to the SVGRenderStyle, so this would only work for simple
colors, not for other paint servers like gradients/patterns.

>From IRC discussion the result is that this is already broken in trunk. It
makes sense to still pass around the uris here, as only the rendering code then
needs an adjustment to take into account complex paint uris for SVG.

> Source/WebCore/rendering/style/RenderStyle.cpp:1121
> +Color RenderStyle::colorIncludingFallback(int colorProperty, bool
visitedLink) const

I dislike the bool usage, please replace it by an enum containing
"UseVisitedLinkStye" / "UseRegularStyle" or sth. along the lines, because....

> Source/WebCore/rendering/style/RenderStyle.cpp:1178
> +    Color unvisitedColor = colorIncludingFallback(colorProperty, false);

... this is awkward to read on the call site! Passing sth. like
"DontUseVisitedStyle" or "UseRegularStyleOnly" (whatever :-) is much easier to
read and follow.

> Source/WebCore/rendering/style/SVGRenderStyleDefs.cpp:66
> +	   && visitedLinkPaintColor == other.visitedLinkPaintColor;

An URI is missing for the visitedLinkPaintUri.

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:84
> +	   SVGPaint::SVGPaintType visitedPaintType = applyToFill ?
svgStyle->visitedLinkFillPaintType() : svgStyle->visitedLinkStrokePaintType();

Could you file a bug report that SVGs visited style handling ignores complex
paint servers and add that bug url with a FIXME here.
Then we can fix the rendering seperated and fully fix visited link styles for

More information about the webkit-reviews mailing list