[webkit-reviews] review denied: [Bug 93829] Text decorations specified on the containing block are not properly applied when ::first-line is present. : [Attachment 157986] Patch

[bugzilla-daemon at webkit.org]

Abhishek Arya <inferno at chromium.org> has denied Arpita Bahuguna
<arpitabahuguna at gmail.com>'s request for review:
Bug 93829: Text decorations specified on the containing block are not properly
applied when ::first-line is present.
https://bugs.webkit.org/show_bug.cgi?id=93829

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

------- Additional Comments from Abhishek Arya <inferno at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=157986&action=review


>> Source/WebCore/rendering/InlineTextBox.cpp:928
>> +	    renderer()->getTextDecorationColors(deco, underline, overline,
linethrough, true, true);
> 
> Can you explain in the changelog what was really going on here? It's not
clear what this does, or why we need to call it twice now (without and with
first line)

This looks like the only call site for getTextDecorationColors and you pass
always true for its argument. This seems wrong to me. you should probably fix
up getTextDecorationColors function itself and make it so that its style is
taken into account and not just :first-line style. I think this part of the
code is wrong since it only looks at first-line style
do {
	styleToUse = curr->style(firstlineStyle);
	int currDecs = styleToUse->textDecoration();
	if (currDecs) {


Your changelog description is just a copy of the title, so it needs more
explanation.