Calculate correct clip foregroundBound and backgroundBound

Calculate correct clip foregroundBound and backgroundBound

This is a followup CL of https://codereview.chromium.org/976543002/. In
that CL, sub-pixel accumulation is considered to calculate layerBounds,
foregroundBounds and backgroundBounds if renderLayer doesn't paint into
its own backing layer. But in that CL, sub-pixel accumulation didn't
apply to foregroundBounds and backgroundBounds for unpaginated layers.
This CL fixes above issue.

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

[qiankun, 2015-08-07 14:49:56]

qiankun.miao@intel.com changed reviewers:
+ chrishtr@chromium.org, leviw@chromium.org, pdr@chromium.org

[qiankun, 2015-08-07 14:49:57]

PTAL.

[pdr., 2015-08-07 23:20:31]

https://codereview.chromium.org/1278113003/diff/20001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerClipper.cpp (right):

https://codereview.chromium.org/1278113003/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerClipper.cpp:232:
foregroundRect.move(context.subPixelAccumulation);
Why don't we need to move backgroundRect too?

[chrishtr, 2015-08-08 02:51:40]

Please add a test.

[eae, 2015-08-12 20:26:21]

eae@chromium.org changed reviewers:
+ eae@chromium.org
- leviw@chromium.org

[qiankun, 2015-08-13 10:36:03]

https://codereview.chromium.org/1278113003/diff/20001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerClipper.cpp (right):

https://codereview.chromium.org/1278113003/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerClipper.cpp:232:
foregroundRect.move(context.subPixelAccumulation);
On 2015/08/07 23:20:31, pdr wrote:
> Why don't we need to move backgroundRect too?

Thanks for review. backgroundRect also needs to move. I updated the CL. Layout
test was added.

[qiankun, 2015-08-17 09:03:10]

hi reviewers, please help to review this new patch. Thanks!

[pdr., 2015-08-17 21:17:54]

Chris is out on vacation so I'll give this a shot. May need some
handholding/patience as I don't know this code very well.

https://codereview.chromium.org/1278113003/diff/60001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerClipper.cpp (left):

https://codereview.chromium.org/1278113003/diff/60001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerClipper.cpp:220:
backgroundRect.move(roundedIntSize(context.subPixelAccumulation));
We used roundedIntSize before, but don't now. Was that intentional or tested?

https://codereview.chromium.org/1278113003/diff/60001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerClipper.cpp (right):

https://codereview.chromium.org/1278113003/diff/60001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerClipper.cpp:220:
backgroundRect.intersect(paintDirtyRect);
This seems wrong... we're intersecting with the paintDirtyRect and then moving
by the subPixelAccumulation below. I think we need to do dirty rect intersection
before moving.

[qiankun, 2015-08-19 09:14:19]

https://codereview.chromium.org/1278113003/diff/60001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerClipper.cpp (left):

https://codereview.chromium.org/1278113003/diff/60001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerClipper.cpp:220:
backgroundRect.move(roundedIntSize(context.subPixelAccumulation));
On 2015/08/17 21:17:53, pdr wrote:
> We used roundedIntSize before, but don't now. Was that intentional or tested?

It's intentional. The layout test which was added for this line passed.

https://codereview.chromium.org/1278113003/diff/60001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerClipper.cpp (right):

https://codereview.chromium.org/1278113003/diff/60001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerClipper.cpp:220:
backgroundRect.intersect(paintDirtyRect);
On 2015/08/17 21:17:53, pdr wrote:
> This seems wrong... we're intersecting with the paintDirtyRect and then moving
> by the subPixelAccumulation below. I think we need to do dirty rect
intersection
> before moving.

Both backgroundRect and paintDirtyRect should move by subPixelAccumulation here.
But backgroundRect and paintDirtyRect didn't consider subPixelAccumulation, so
moving subPixelAccumulation after intersection.

[pdr., 2015-08-20 05:55:13]

https://codereview.chromium.org/1278113003/diff/60001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerClipper.cpp (right):

https://codereview.chromium.org/1278113003/diff/60001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerClipper.cpp:220:
backgroundRect.intersect(paintDirtyRect);
On 2015/08/19 at 09:14:19, qiankun wrote:
> On 2015/08/17 21:17:53, pdr wrote:
> > This seems wrong... we're intersecting with the paintDirtyRect and then
moving
> > by the subPixelAccumulation below. I think we need to do dirty rect
intersection
> > before moving.
> 
> Both backgroundRect and paintDirtyRect should move by subPixelAccumulation
here. But backgroundRect and paintDirtyRect didn't consider
subPixelAccumulation, so moving subPixelAccumulation after intersection.

I'm just worried about the order these are being done.

For example (x, y, w, h):
paintDirtyRect: (0, 0, 100, 100)
backgroundRect: (-1, 0, 1, 100)
subpixelAccumulation: (0.5, 0)

Without your patch:
backgroundRect.move(subpixelAccumulation) = (-0.5, 0, 1, 100)
backgroundRect.intersect(paintDirtyRect) = (0, 0, 0.5, 100)
This will paint a line at (0, 0) with width 0.5px and height 100px.

With your patch:
backgroundRect.intersect(paintDirtyRect) = (0, 0, 0, 0)
backgroundRect.move(subpixelAccumulation) = (0.5, 0, 0, 0)
This will paint nothing because it's empty.

[qiankun, 2015-08-20 06:08:43]

https://codereview.chromium.org/1278113003/diff/60001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerClipper.cpp (right):

https://codereview.chromium.org/1278113003/diff/60001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerClipper.cpp:220:
backgroundRect.intersect(paintDirtyRect);
On 2015/08/20 05:55:13, pdr wrote:
> On 2015/08/19 at 09:14:19, qiankun wrote:
> > On 2015/08/17 21:17:53, pdr wrote:
> > > This seems wrong... we're intersecting with the paintDirtyRect and then
> moving
> > > by the subPixelAccumulation below. I think we need to do dirty rect
> intersection
> > > before moving.
> > 
> > Both backgroundRect and paintDirtyRect should move by subPixelAccumulation
> here. But backgroundRect and paintDirtyRect didn't consider
> subPixelAccumulation, so moving subPixelAccumulation after intersection.
> 
> I'm just worried about the order these are being done.
> 
> For example (x, y, w, h):
> paintDirtyRect: (0, 0, 100, 100)
> backgroundRect: (-1, 0, 1, 100)
> subpixelAccumulation: (0.5, 0)
> 
> Without your patch:
> backgroundRect.move(subpixelAccumulation) = (-0.5, 0, 1, 100)
Current implementation is wrong here. I think we should also do
paintDirtyRect.move(subpixelAccumulation) before intersection. 

> backgroundRect.intersect(paintDirtyRect) = (0, 0, 0.5, 100)
> This will paint a line at (0, 0) with width 0.5px and height 100px.
> 
> With your patch:
> backgroundRect.intersect(paintDirtyRect) = (0, 0, 0, 0)
> backgroundRect.move(subpixelAccumulation) = (0.5, 0, 0, 0)
> This will paint nothing because it's empty.

[pdr., 2015-08-20 06:15:56]

On 2015/08/20 at 06:08:43, qiankun.miao wrote:
>
https://codereview.chromium.org/1278113003/diff/60001/Source/core/paint/Depre...
> File Source/core/paint/DeprecatedPaintLayerClipper.cpp (right):
> 
>
https://codereview.chromium.org/1278113003/diff/60001/Source/core/paint/Depre...
> Source/core/paint/DeprecatedPaintLayerClipper.cpp:220:
backgroundRect.intersect(paintDirtyRect);
> On 2015/08/20 05:55:13, pdr wrote:
> > On 2015/08/19 at 09:14:19, qiankun wrote:
> > > On 2015/08/17 21:17:53, pdr wrote:
> > > > This seems wrong... we're intersecting with the paintDirtyRect and then
> > moving
> > > > by the subPixelAccumulation below. I think we need to do dirty rect
> > intersection
> > > > before moving.
> > > 
> > > Both backgroundRect and paintDirtyRect should move by subPixelAccumulation
> > here. But backgroundRect and paintDirtyRect didn't consider
> > subPixelAccumulation, so moving subPixelAccumulation after intersection.
> > 
> > I'm just worried about the order these are being done.
> > 
> > For example (x, y, w, h):
> > paintDirtyRect: (0, 0, 100, 100)
> > backgroundRect: (-1, 0, 1, 100)
> > subpixelAccumulation: (0.5, 0)
> > 
> > Without your patch:
> > backgroundRect.move(subpixelAccumulation) = (-0.5, 0, 1, 100)
> Current implementation is wrong here. I think we should also do
paintDirtyRect.move(subpixelAccumulation) before intersection.

I don't think so, as the paint dirty rect is pixel aligned and shouldn't have
subpixel issues.

If this is an existing bug, can you file a new bug with an example html file
showing the bug?

All hope is not lost though. It's pretty easy to refactor your patch to avoid
this intersect/move ordering problem (or at least keep the current bug if there
is one).

> 
> > backgroundRect.intersect(paintDirtyRect) = (0, 0, 0.5, 100)
> > This will paint a line at (0, 0) with width 0.5px and height 100px.
> > 
> > With your patch:
> > backgroundRect.intersect(paintDirtyRect) = (0, 0, 0, 0)
> > backgroundRect.move(subpixelAccumulation) = (0.5, 0, 0, 0)
> > This will paint nothing because it's empty.

[qiankun, 2015-08-21 09:48:29]

On 2015/08/20 06:15:56, pdr wrote:
> On 2015/08/20 at 06:08:43, qiankun.miao wrote:
> >
>
https://codereview.chromium.org/1278113003/diff/60001/Source/core/paint/Depre...
> > File Source/core/paint/DeprecatedPaintLayerClipper.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1278113003/diff/60001/Source/core/paint/Depre...
> > Source/core/paint/DeprecatedPaintLayerClipper.cpp:220:
> backgroundRect.intersect(paintDirtyRect);
> > On 2015/08/20 05:55:13, pdr wrote:
> > > On 2015/08/19 at 09:14:19, qiankun wrote:
> > > > On 2015/08/17 21:17:53, pdr wrote:
> > > > > This seems wrong... we're intersecting with the paintDirtyRect and
then
> > > moving
> > > > > by the subPixelAccumulation below. I think we need to do dirty rect
> > > intersection
> > > > > before moving.
> > > > 
> > > > Both backgroundRect and paintDirtyRect should move by
subPixelAccumulation
> > > here. But backgroundRect and paintDirtyRect didn't consider
> > > subPixelAccumulation, so moving subPixelAccumulation after intersection.
> > > 
> > > I'm just worried about the order these are being done.
> > > 
> > > For example (x, y, w, h):
> > > paintDirtyRect: (0, 0, 100, 100)
> > > backgroundRect: (-1, 0, 1, 100)
> > > subpixelAccumulation: (0.5, 0)
> > > 
> > > Without your patch:
> > > backgroundRect.move(subpixelAccumulation) = (-0.5, 0, 1, 100)
> > Current implementation is wrong here. I think we should also do
> paintDirtyRect.move(subpixelAccumulation) before intersection.
> 
> I don't think so, as the paint dirty rect is pixel aligned and shouldn't have
> subpixel issues.
> 
> If this is an existing bug, can you file a new bug with an example html file
> showing the bug?
> 
> All hope is not lost though. It's pretty easy to refactor your patch to avoid
> this intersect/move ordering problem (or at least keep the current bug if
there
> is one).
> 

Thanks you pdr@. I am wrong. dirty rect is pixel aligned as you said. I updated
the patch to move backgroundRect before intersection. PTAL.

[pdr., 2015-08-21 21:40:48]

On 2015/08/21 at 09:48:29, qiankun.miao wrote:
> On 2015/08/20 06:15:56, pdr wrote:
> > On 2015/08/20 at 06:08:43, qiankun.miao wrote:
> > >
> >
https://codereview.chromium.org/1278113003/diff/60001/Source/core/paint/Depre...
> > > File Source/core/paint/DeprecatedPaintLayerClipper.cpp (right):
> > > 
> > >
> >
https://codereview.chromium.org/1278113003/diff/60001/Source/core/paint/Depre...
> > > Source/core/paint/DeprecatedPaintLayerClipper.cpp:220:
> > backgroundRect.intersect(paintDirtyRect);
> > > On 2015/08/20 05:55:13, pdr wrote:
> > > > On 2015/08/19 at 09:14:19, qiankun wrote:
> > > > > On 2015/08/17 21:17:53, pdr wrote:
> > > > > > This seems wrong... we're intersecting with the paintDirtyRect and
then
> > > > moving
> > > > > > by the subPixelAccumulation below. I think we need to do dirty rect
> > > > intersection
> > > > > > before moving.
> > > > > 
> > > > > Both backgroundRect and paintDirtyRect should move by
subPixelAccumulation
> > > > here. But backgroundRect and paintDirtyRect didn't consider
> > > > subPixelAccumulation, so moving subPixelAccumulation after intersection.
> > > > 
> > > > I'm just worried about the order these are being done.
> > > > 
> > > > For example (x, y, w, h):
> > > > paintDirtyRect: (0, 0, 100, 100)
> > > > backgroundRect: (-1, 0, 1, 100)
> > > > subpixelAccumulation: (0.5, 0)
> > > > 
> > > > Without your patch:
> > > > backgroundRect.move(subpixelAccumulation) = (-0.5, 0, 1, 100)
> > > Current implementation is wrong here. I think we should also do
> > paintDirtyRect.move(subpixelAccumulation) before intersection.
> > 
> > I don't think so, as the paint dirty rect is pixel aligned and shouldn't
have
> > subpixel issues.
> > 
> > If this is an existing bug, can you file a new bug with an example html file
> > showing the bug?
> > 
> > All hope is not lost though. It's pretty easy to refactor your patch to
avoid
> > this intersect/move ordering problem (or at least keep the current bug if
there
> > is one).
> > 
> 
> Thanks you pdr@. I am wrong. dirty rect is pixel aligned as you said. I
updated the patch to move backgroundRect before intersection. PTAL.

LGTM

[pdr., 2015-08-21 21:40:52]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2015-08-21 21:41:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1278113003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1278113003/80001

[commit-bot: I haz the power, 2015-08-22 00:38:09]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201030