[webkit-reviews] review denied: [Bug 71944] CSS 2.1 failure: outline-color-applies-to* tests fail : [Attachment 120883] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Robert Hogan
<robert at webkit.org>'s request for review:
Bug 71944: CSS 2.1 failure: outline-color-applies-to* tests fail
https://bugs.webkit.org/show_bug.cgi?id=71944

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=120883&action=review


Part of the approach needs to be re-thought to avoid painting twice.

> Source/WebCore/rendering/RenderTableRow.cpp:232
>      if (!layer())
>	   return;

This check seems useless: the ASSERT was never reached and guarantees that we
do have a layer.

Also it looks like your code would like to run even if we have a layer which
seem wrong.

> Source/WebCore/rendering/RenderTableSection.cpp:962
> +	       if (child->isTableRow() && child->hasOutline() &&
child->style()->visibility() == VISIBLE)
> +		   child->paint(paintInfo, paintOffset);

This looks wrong.

You can paint your table rows and cells through 2 code paths: either through a
self-painting layer or directly through RenderTableSection::paintObject (which
actually paints cells directly ie bypassing some of the row's painting code).
If you look closely at the painting code in RenderTableSection and
RenderTableRow, there are checks to avoid calling one code path from the other
one. Here you are calling RenderTableRow::paint() without caring which code
path you are on so you will either trigger the ASSERT or paint twice. I don't
know if the ASSERT is covered but please add a test with an outline on a row
with a self-painting layer if it is not!

By patching RenderTableRow::paint(), you have fixed part of the self-painting
layer code path (the one involving a row's self painting layer). However the
other code path is already calling RenderTableCell::paint and thus would need
to be patched to account for the row's outline.