Bug 69966 – Eliminate separate RenderStyle for visited link style

• Component: CSS
• Status: RESOLVED FIXED

[Antti Koivisto]

There are only a few properties that can apply to visited links and they can be part of the regular style. Many things will get simpler when the separate RenderStyle for visited links is eliminated.

[Antti Koivisto]

Created attachment 110806 [details]
patch

After this style recalc for visited link changes is unnecessary too and can be removed later.

[WebKit Review Bot]

Attachment 110806 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/css/CSSStyleApplyProperty.cpp:243:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/rendering/style/StyleInheritedData.cpp:63:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

Comment on attachment 110806 [details]
patch

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.

[Antti Koivisto]

Created attachment 110813 [details]
updated patch

- Added (currently unused) visitedLinkFill/StrokePaintUri component to SVG style for completeness
- Got rid of the macro in favor of doing the applying to the right style in SVGRenderStyle::setFill/StrokePaint()
- Cleanups.

[Antti Koivisto]

(In reply to comment #3)
> I dislike the bool usage, please replace it by an enum containing "UseVisitedLinkStye" / "UseRegularStyle" or sth. along the lines, because....

As discussed, I didn't do this. It is a private helper and usage is clear from the context.

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

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

[Dimitri Glazkov (Google)]

Comment on attachment 110813 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110813&action=review

> Source/WebCore/css/CSSStyleSelector.h:176
> +    Color getColorFromPrimitiveValue(CSSPrimitiveValue*, bool forVisitedLink = false) const;

I thought we hate bool params? :)

[Dave Hyatt]

Comment on attachment 110813 [details]
updated patch

r=me

[Antti Koivisto]

http://trac.webkit.org/changeset/97638

[Ryosuke Niwa]

It appears that this patch caused 3 printing tests to hit assertions on Snow Leopard:
http://build.webkit.org/results/SnowLeopard%20Intel%20Debug%20(Tests)/r97638%20(2608)/results.html

A stack trace sample:
0   com.apple.WebCore             	0x0000000101383c23 WebCore::Node::getFlag(WebCore::Node::NodeFlags) const + 15 (Node.h:636)
1   com.apple.WebCore             	0x000000010144c2da WebCore::Node::isLink() const + 26 (Node.h:301)
2   com.apple.WebCore             	0x0000000101464b26 void WebCore::CSSStyleSelector::applyDeclarations<true>(bool, int, int) + 52 (CSSStyleSelector.cpp:2177)
3   com.apple.WebCore             	0x0000000101446641 WebCore::CSSStyleSelector::styleForPage(int) + 363 (CSSStyleSelector.cpp:1476)
4   com.apple.WebCore             	0x00000001014bc079 WebCore::Document::styleForPage(int) + 43 (Document.cpp:1694)
5   com.apple.WebCore             	0x00000001014bc562 WebCore::Document::isPageBoxVisible(int) + 32 (Document.cpp:1714)
6   com.apple.WebCore             	0x0000000101da5853 WebCore::PrintContext::isPageBoxVisible(WebCore::Frame*, int) + 35 (PrintContext.cpp:291)
7   com.apple.WebKit              	0x0000000100c5b7ee -[WebFrame(WebKitDebug) isPageBoxVisible:] + 68 (WebCoreStatistics.mm:293)
8   DumpRenderTree                	0x000000010002d3c3 LayoutTestController::isPageBoxVisible(int) const + 43 (LayoutTestControllerMac.mm:316)
9   DumpRenderTree                	0x00000001000245aa isPageBoxVisibleCallback(OpaqueJSContext const*, OpaqueJSValue*, OpaqueJSValue*, unsigned long, OpaqueJSValue const* const*, OpaqueJSValue const**) + 116 (LayoutTestController.cpp:946)
10  com.apple.JavaScriptCore      	0x00000001002c3fad JSC::JSCallbackFunction::call(JSC::ExecState*) + 301 (JSCallbackFunction.cpp:73)
11  com.apple.JavaScriptCore      	0x00000001002ac656 cti_op_call_NotJSFunction + 425 (JITStubs.cpp:2322)
12  com.apple.JavaScriptCore      	0x00000001002a4e13 jscGeneratedNativeCode + 0 (JITStubs.cpp:945)
13  com.apple.JavaScriptCore      	0x0000000100283574 JSC::JITCode::execute(JSC::RegisterFile*, JSC::ExecState*, JSC::JSGlobalData*) + 86 (JITCode.h:103)
14  com.apple.JavaScriptCore      	0x000000010027c9eb JSC::Interpreter::execute(JSC::EvalExecutable*, JSC::ExecState*, JSC::JSValue, int, JSC::ScopeChainNode*) + 1799 (Interpreter.cpp:1295)
15  com.apple.JavaScriptCore      	0x000000010027fbdf JSC::Interpreter::callEval(JSC::ExecState*, JSC::RegisterFile*, JSC::Register*, int, int) + 887 (Interpreter.cpp:463)
16  com.apple.JavaScriptCore      	0x00000001002a80f2 cti_op_call_eval + 490 (JITStubs.cpp:3433)
17  com.apple.JavaScriptCore      	0x00000001002a4e13 jscGeneratedNativeCode + 0 (JITStubs.cpp:945)
18  com.apple.JavaScriptCore      	0x0000000100283574 JSC::JITCode::execute(JSC::RegisterFile*, JSC::ExecState*, JSC::JSGlobalData*) + 86 (JITCode.h:103)
19  com.apple.JavaScriptCore      	0x000000010027ef11 JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::ScopeChainNode*, JSC::JSObject*) + 3361 (Interpreter.cpp:897)

[Ryosuke Niwa]

These tests are also crashing on release builds:

[Ryosuke Niwa]

Lion:
http://build.webkit.org/results/Lion%20Intel%20Debug%20(Tests)/r97701%20(1009)/results.html

Qt:
http://build.webkit.org/results/Qt%20Linux%20Release/r97703%20(38636)/results.html

[Antti Koivisto]

The printing test crash was fixed in http://trac.webkit.org/changeset/97724