Issue 694753004: Correctly ignore "self" for border radius clipping when painting the background phase or fragments.

Issue 694753004: Correctly ignore "self" for border radius clipping when painting the background phase or fragments. (Closed)

Created:
6 years, 1 month ago by chrishtr

Modified:
6 years, 1 month ago

Reviewers:
mstensho (USE GERRIT), pdr.

CC:
blink-reviews, mstensho (USE GERRIT)

Base URL:
svn://svn.chromium.org/blink/trunk

Project:
blink

Visibility:
Public.

More Reviews

Description

Correctly ignore "self" for border radius clipping when painting the background phase or fragments. This was broken in crrev.com/653303003. There is no test for the latter, but that code is not live yet. Punting to mstensho@opera.com. BUG=425030 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184831

Patch Set 1 #

Patch Set 2 : Added test #

Patch Set 3 : Fixed test #

Patch Set 4 : Removed fprintf. #

Total comments: 8

Patch Set 5 : Fixed #

Patch Set 6 : Merged. #

Total comments: 2

Patch Set 7 : Fixed. #

Patch Set 8 : Made test a reftest. #

Patch Set 9 : Fixed #

Created: 6 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+13 lines, -3 lines)			Patch
M	LayoutTests/TestExpectations	View	1 2 3 4 5 6 7	1 chunk	+0 lines, -1 line	0 comments	Download
A	LayoutTests/fast/clip/nested-rounded-rect.html	View	1 2 3 4	1 chunk	+5 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/clip/nested-rounded-rect-expected.html	View	1 2 3 4 5 6 7	1 chunk	+5 lines, -0 lines	0 comments	Download
M	Source/core/paint/LayerPainter.cpp	View	1 2 3 4 5 6 7	2 chunks	+3 lines, -2 lines	0 comments	Download

Messages

Total messages: 15 (5 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

mstensho (USE GERRIT)

https://codereview.chromium.org/694753004/diff/60001/LayoutTests/TestExpectations File LayoutTests/TestExpectations (right): https://codereview.chromium.org/694753004/diff/60001/LayoutTests/TestExpectations#newcode1 LayoutTests/TestExpectations:1: crbug.com/425030 fast/clip/nested-rounded-rect.html [ NeedsRebaseline ] Did you consider writing ...

6 years, 1 month ago (2014-10-31 22:35:34 UTC) #4

chrishtr

I tried comparing with this in the expected file: <!DOCTYPE html> <div style="position: relative; overflow: ...

6 years, 1 month ago (2014-10-31 23:53:16 UTC) #5

I tried comparing with this in the expected file:

<!DOCTYPE html>
<div style="position: relative; overflow: hidden; border: 1px solid black;
border-radius: 4px; background: lightgray; width: 20px; height: 20px">
</div>

and can't seem to get rid of (invisible to me) pixel differences in the top left
and bottom left corner, presumably to some sort of antialiasing difference.

https://codereview.chromium.org/694753004/diff/60001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/694753004/diff/60001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:1: crbug.com/425030
fast/clip/nested-rounded-rect.html [ NeedsRebaseline ]
On 2014/10/31 22:35:34, mstensho wrote:
> Did you consider writing a reftest instead? Then there'd be no need for this.

I did. After significant fiddling, I was unable to get it to be pixel-wise the
same. between the
two versions I was writing. Will try some more..

https://codereview.chromium.org/694753004/diff/60001/LayoutTests/fast/clip/ne...
File LayoutTests/fast/clip/nested-rounded-rect.html (right):

https://codereview.chromium.org/694753004/diff/60001/LayoutTests/fast/clip/ne...
LayoutTests/fast/clip/nested-rounded-rect.html:1: <div  style="position:
relative; overflow: hidden; border-radius: 4px;">
On 2014/10/31 at 22:35:34, mstensho wrote:
> Can you add a <!DOCTYPE html>?

Done.

https://codereview.chromium.org/694753004/diff/60001/LayoutTests/fast/clip/ne...
LayoutTests/fast/clip/nested-rounded-rect.html:2: <div style="display:
inline-block; position: relative; overflow: hidden; border: 1px solid black;
border-radius: 4px 0px 0px 4px; background: lightgray; width: 20px; height:
20px">
On 2014/10/31 at 22:35:34, mstensho wrote:
> Any reason why it's inline-block?

Removed.

https://codereview.chromium.org/694753004/diff/60001/Source/core/paint/LayerP...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/694753004/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:876:
LayerPainter(*m_renderLayer.parent()).applyRoundedRectClips(paintingInfo,
context, clipRect, paintFlags, *clipRecorder);
On 2014/10/31 at 22:35:34, mstensho wrote:
> I'm curious as to why we need to play with rounded corners here at all. We'll
end up in paintBackgroundForFragments() & co anyway. All we should have to worry
about here in paintTransformedLayerIntoFragments() is to clip to the column
boundaries. Could you add a FIXME, and I'll get back to it? We should at least
have a test for it. I'd also like to engage in a staring contest with this code
and see what survives.

Added FIXME.

mstensho (USE GERRIT)

I've looked enough at this now to give you a non-owner lgtm. :) Regarding a ...

6 years, 1 month ago (2014-11-03 13:28:09 UTC) #6

chrishtr

https://codereview.chromium.org/694753004/diff/100001/Source/core/paint/LayerPainter.cpp File Source/core/paint/LayerPainter.cpp (right): https://codereview.chromium.org/694753004/diff/100001/Source/core/paint/LayerPainter.cpp#newcode691 Source/core/paint/LayerPainter.cpp:691: applyRoundedRectClips(localPaintingInfo, context, fragment.backgroundRect, paintFlags, *clipRecorder, DoNotIncludeSelfForBorderRadius); On 2014/11/03 at ...

6 years, 1 month ago (2014-11-03 17:36:21 UTC) #7

chrishtr

On 2014/11/03 at 13:28:09, mstensho wrote: > I've looked enough at this now to give ...

6 years, 1 month ago (2014-11-04 06:25:45 UTC) #8

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/694753004/140001

6 years, 1 month ago (2014-11-04 06:27:49 UTC) #10

commit-bot: I haz the power

Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/32152)

6 years, 1 month ago (2014-11-04 07:23:19 UTC) #12

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/694753004/160001

6 years, 1 month ago (2014-11-04 16:57:53 UTC) #14

commit-bot: I haz the power

6 years, 1 month ago (2014-11-04 17:10:47 UTC) #15

Message was sent while issue was closed.

Committed patchset #9 (id:160001) as 184831

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages