Cleanup: Use foreground rect in paintChildClippingMaskForFragments().

Cleanup: Use foreground rect in paintChildClippingMaskForFragments().

The clip rectangle was based on the foreground rectangle, while we
passed the background rectangle to PaintInfo.
Be consistent and use the foreground rectangle in both cases.

There was also a comment about skipping ourselves for border radius
clipping, although we did include ourselves. Removed the comment.

No behavioral changes intended, but added a test that would fail if we
had gone for the background rect instead of the foreground rect.

R=chrishtr@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184931

[mstensho (USE GERRIT), 2014-11-05 13:14:52]

Oh, and the test is very black in order to avoid anti-aliasing differences
between the test and the ref. :-P

[chrishtr, 2014-11-06 02:03:04]

https://codereview.chromium.org/706603002/diff/1/Source/core/paint/LayerPaint...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/706603002/diff/1/Source/core/paint/LayerPaint...
Source/core/paint/LayerPainter.cpp:829: PaintInfo paintInfo(context,
pixelSnappedIntRect(fragment.foregroundRect.rect()), PaintPhaseClippingMask,
PaintBehaviorNormal, paintingRootForRenderer, 0,
localPaintingInfo.rootLayer->renderer());
The rect passed here is supposed to be the bounds of the renderer. Why is the
foreground rect the right one, not the background rect?

Also, the code on line 825 may not actually execute if there is no clipping
needed.

[mstensho (USE GERRIT), 2014-11-06 14:06:41]

https://codereview.chromium.org/706603002/diff/1/Source/core/paint/LayerPaint...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/706603002/diff/1/Source/core/paint/LayerPaint...
Source/core/paint/LayerPainter.cpp:829: PaintInfo paintInfo(context,
pixelSnappedIntRect(fragment.foregroundRect.rect()), PaintPhaseClippingMask,
PaintBehaviorNormal, paintingRootForRenderer, 0,
localPaintingInfo.rootLayer->renderer());
On 2014/11/06 02:03:04, chrishtr wrote:
> The rect passed here is supposed to be the bounds of the renderer.

No, it's supposed to be the damage rectangle, isn't it?

> Why is the foreground rect the right one, not the background rect?

"Painting" a "child clipping mask" doesn't seem to really be about actual
painting, but rather about setting a clip mask for children of this layer, for
compositing. See BoxPainter::paintClippingMask(). It just fills the paint area
with black. Since this clipping mask is for children, we want to use the
foreground rect, since the background rect may include a border that children
shouldn't paint on top of.

> Also, the code on line 825 may not actually execute if there is no clipping
> needed.

That's why I think it makes sense to be consistent about which rectangle to use.

[chrishtr, 2014-11-06 18:10:02]

https://codereview.chromium.org/706603002/diff/1/Source/core/paint/LayerPaint...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/706603002/diff/1/Source/core/paint/LayerPaint...
Source/core/paint/LayerPainter.cpp:829: PaintInfo paintInfo(context,
pixelSnappedIntRect(fragment.foregroundRect.rect()), PaintPhaseClippingMask,
PaintBehaviorNormal, paintingRootForRenderer, 0,
localPaintingInfo.rootLayer->renderer());
On 2014/11/06 at 14:06:41, mstensho wrote:
> On 2014/11/06 02:03:04, chrishtr wrote:
> > The rect passed here is supposed to be the bounds of the renderer.
> 
> No, it's supposed to be the damage rectangle, isn't it?
> 
> > Why is the foreground rect the right one, not the background rect?
> 
> "Painting" a "child clipping mask" doesn't seem to really be about actual
painting, but rather about setting a clip mask for children of this layer, for
compositing. See BoxPainter::paintClippingMask(). It just fills the paint area
with black. Since this clipping mask is for children, we want to use the
foreground rect, since the background rect may include a border that children
shouldn't paint on top of.
> 
> > Also, the code on line 825 may not actually execute if there is no clipping
> > needed.
> 
> That's why I think it makes sense to be consistent about which rectangle to
use.

Bear with me, I am learning some of this code also. :)

I see. And the test you added does fail if you revert this line? How does it
fail exactly? I'm trying to see the line of code in BoxPainter that fails. Is it
that it incorrectly early outs?

Also, does Firefox render the test in this CL the same way you expected.

[mstensho (USE GERRIT), 2014-11-06 19:06:08]

https://codereview.chromium.org/706603002/diff/1/Source/core/paint/LayerPaint...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/706603002/diff/1/Source/core/paint/LayerPaint...
Source/core/paint/LayerPainter.cpp:829: PaintInfo paintInfo(context,
pixelSnappedIntRect(fragment.foregroundRect.rect()), PaintPhaseClippingMask,
PaintBehaviorNormal, paintingRootForRenderer, 0,
localPaintingInfo.rootLayer->renderer());
On 2014/11/06 18:10:01, chrishtr wrote:
> On 2014/11/06 at 14:06:41, mstensho wrote:
> > On 2014/11/06 02:03:04, chrishtr wrote:
> > > The rect passed here is supposed to be the bounds of the renderer.
> > 
> > No, it's supposed to be the damage rectangle, isn't it?
> > 
> > > Why is the foreground rect the right one, not the background rect?
> > 
> > "Painting" a "child clipping mask" doesn't seem to really be about actual
> painting, but rather about setting a clip mask for children of this layer, for
> compositing. See BoxPainter::paintClippingMask(). It just fills the paint area
> with black. Since this clipping mask is for children, we want to use the
> foreground rect, since the background rect may include a border that children
> shouldn't paint on top of.
> > 
> > > Also, the code on line 825 may not actually execute if there is no
clipping
> > > needed.
> > 
> > That's why I think it makes sense to be consistent about which rectangle to
> use.
> 
> Bear with me, I am learning some of this code also. :)
> 
> I see. And the test you added does fail if you revert this line? How does it
> fail exactly? I'm trying to see the line of code in BoxPainter that fails. Is
it
> that it incorrectly early outs?

No, it didn't fail prior to my changes. Like it says in the description, it
WOULD fail if I had changed everything (i.e. the clipping code above) to using
the background rect. I tried to change everything to using the background rect,
just to see if any existing test would fail then, but I couldn't find any, so I
thought it was best to add something that would fail.

No behavioral changes are intended with this change; it's just about being
consistent (so that I can land.https://codereview.chromium.org/704753002/ )

> Also, does Firefox render the test in this CL the same way you expected.

Firefox renders it correctly too.

[chrishtr, 2014-11-06 21:20:02]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2014-11-06 21:20:03]

lgtm

[commit-bot: I haz the power, 2014-11-06 21:20:54]

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

[commit-bot: I haz the power, 2014-11-06 21:22:49]

Committed patchset #1 (id:1) as 184931