[Webkit-unassigned] [Bug 69966] Eliminate separate RenderStyle for visited link style

[bugzilla-daemon at webkit.org]

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


Nikolas Zimmermann <zimmermann at kde.org> changed:

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




--- Comment #3 from Nikolas Zimmermann <zimmermann at kde.org>  2011-10-13 00:56:20 PST ---
(From update of attachment 110806)
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.

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