WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
69966
Eliminate separate RenderStyle for visited link style
https://bugs.webkit.org/show_bug.cgi?id=69966
Summary
Eliminate separate RenderStyle for visited link style
Antti Koivisto
Reported
2011-10-12 15:12:52 PDT
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.
Attachments
patch
(52.96 KB, patch)
2011-10-13 00:21 PDT
,
Antti Koivisto
zimmermann
: review-
Details
Formatted Diff
Diff
updated patch
(55.55 KB, patch)
2011-10-13 01:51 PDT
,
Antti Koivisto
hyatt
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Antti Koivisto
Comment 1
2011-10-13 00:21:34 PDT
Created
attachment 110806
[details]
patch After this style recalc for visited link changes is unnecessary too and can be removed later.
WebKit Review Bot
Comment 2
2011-10-13 00:25:13 PDT
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 3
2011-10-13 00:56:20 PDT
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
Comment 4
2011-10-13 01:51:46 PDT
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
Comment 5
2011-10-13 02:06:50 PDT
(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 6
2011-10-17 09:21:55 PDT
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 7
2011-10-17 09:24:21 PDT
Comment on
attachment 110813
[details]
updated patch r=me
Antti Koivisto
Comment 8
2011-10-17 12:48:23 PDT
http://trac.webkit.org/changeset/97638
Ryosuke Niwa
Comment 9
2011-10-17 18:38:41 PDT
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
Comment 10
2011-10-17 22:02:17 PDT
These tests are also crashing on release builds:
Ryosuke Niwa
Comment 11
2011-10-17 22:05:08 PDT
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
Comment 12
2011-10-17 23:42:09 PDT
The printing test crash was fixed in
http://trac.webkit.org/changeset/97724
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug