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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=69966

Attachment 110806: patch
https://bugs.webkit.org/attachment.cgi?id=110806&action=review

------- 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,
(selector->parentStyle()->*m_default)());
> +	   } 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++)	   

Ditto.

> Source/WebCore/css/SVGCSSStyleSelector.cpp:235
> -		   svgstyle->setFillPaint(svgParentStyle->fillPaintType(),
svgParentStyle->fillPaintColor(), svgParentStyle->fillPaintUri());
> +		   APPLY_VISITED_LINK_VALID_PAINT(setFillPaint,
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
macro.

> Source/WebCore/css/SVGCSSStyleSelector.cpp:239
> -		  
svgstyle->setFillPaint(SVGRenderStyle::initialFillPaintType(),
SVGRenderStyle::initialFillPaintColor(),
SVGRenderStyle::initialFillPaintUri());
> +		   APPLY_VISITED_LINK_VALID_PAINT(setFillPaint,
setVisitedLinkFillPaint, SVGRenderStyle::initialFillPaintType(),
SVGRenderStyle::initialFillPaintColor(),
SVGRenderStyle::initialFillPaintUri());

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
SVG.