Fix paint invalidation for composited vertical-rl having overflow

Fix paint invalidation for composited vertical-rl having overflow

Previously we special cased in slowMapToVisualRectInAncestorSpace()
in PaintInvalidationState.cpp for LayoutView background invalidation
which was because LayoutView::localOverflowRectForPaintInvalidation()
was in physical coordinates while other
localOverflowRectForPaintInvalidation() implementations are in flipped
physical coordinates.

This CL removes the special case, but let
LayoutView::localOverflowRectForPaintInvalidation() returns a rect in
flipped physical coordinates.

BUG=650417
TEST=paint/invalidation/composited-vertical-rl-overflow.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/049d91617706a0fd7f96e861364e4340c966d855
Cr-Commit-Position: refs/heads/master@{#421362}

[Xianzhu, 2016-09-26 22:30:53]

Description was changed from

==========
Fix paint invalidation for composited vertical-rl having overflow

Previously we special cased in slowMapToVisualRectInAncestorSpace()
in PaintInvalidationState.cpp for LayoutView background invalidation
which was because LayoutView::localOverflowRectForPaintInvalidation()
was in physical coordinates while other
localOverflowRectForPaintInvalidation() implementations are in flipped
physical coordinates.

This CL removes the special case, but let
LayoutView::localOverflowRectForPaintInvalidation() returns a rect in
flipped physical coordinates.

BUG=650417
TEST=paint/invalidation/composited-vertical-rl-overflow.html
==========

to

==========
Fix paint invalidation for composited vertical-rl having overflow

Previously we special cased in slowMapToVisualRectInAncestorSpace()
in PaintInvalidationState.cpp for LayoutView background invalidation
which was because LayoutView::localOverflowRectForPaintInvalidation()
was in physical coordinates while other
localOverflowRectForPaintInvalidation() implementations are in flipped
physical coordinates.

This CL removes the special case, but let
LayoutView::localOverflowRectForPaintInvalidation() returns a rect in
flipped physical coordinates.

BUG=650417
TEST=paint/invalidation/composited-vertical-rl-overflow.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2016-09-26 22:31:03]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 22:32:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2016-09-26 22:35:31]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 22:36:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 23:39:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 23:39:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xianzhu, 2016-09-26 23:44:23]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org, wkorman@chromium.org

[chrishtr, 2016-09-27 00:02:44]

https://codereview.chromium.org/2371933003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (left):

https://codereview.chromium.org/2371933003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutView.cpp:297: return
LayoutRect(documentRect());
Why not just return layoutOverflowRect() here? This avoids double-flipping and
the pixel snapping in documentRect().
Also the pixel snapping in documentRect() looks incorrect. The call sites to it
should I think be adjusted to flow
subpixel positioning through and snap at the end. e.g.:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co...

is not adding up the subpixelAccumulation before snapping.

https://codereview.chromium.org/2371933003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp (left):

https://codereview.chromium.org/2371933003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:425: if
(object.isBox() && object != ancestor)
Is there an existing test that this is not needed for boxes that are not
LayoutViews? I agree with your 
logical argument, but we did debug for a while and concluded this was correct...
:(

[Xianzhu, 2016-09-27 00:26:45]

https://codereview.chromium.org/2371933003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (left):

https://codereview.chromium.org/2371933003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutView.cpp:297: return
LayoutRect(documentRect());
On 2016/09/27 00:02:43, chrishtr wrote:
> Why not just return layoutOverflowRect() here? This avoids double-flipping and
> the pixel snapping in documentRect().
> Also the pixel snapping in documentRect() looks incorrect. The call sites to
it
> should I think be adjusted to flow
> subpixel positioning through and snap at the end. e.g.:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co...
> 
> is not adding up the subpixelAccumulation before snapping.

Good idea. Should have looked into documentRect().

Done.

https://codereview.chromium.org/2371933003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp (left):

https://codereview.chromium.org/2371933003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:425: if
(object.isBox() && object != ancestor)
On 2016/09/27 00:02:43, chrishtr wrote:
> Is there an existing test that this is not needed for boxes that are not
> LayoutViews? I agree with your 
> logical argument, but we did debug for a while and concluded this was
correct...
> :(

There seems no existing test. The added test
paint/invalidation/composited-vertical-rl-overflow.html can reproduce the bug.
The key is unsymmetrical left and right overflows.

I also didn't notice the issue when the the above code was added. Just noticed
it when I was trying to fix the flipping issue for slimmingPaintInvalidation on
spv1.

[Xianzhu, 2016-09-27 00:27:29]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 00:28:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 03:14:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 03:14:59]

Dry run: This issue passed the CQ dry run.

[wkorman, 2016-09-27 18:22:23]

lgtm

[chrishtr, 2016-09-27 18:24:34]

https://codereview.chromium.org/2371933003/diff/10010/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (left):

https://codereview.chromium.org/2371933003/diff/10010/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutView.cpp:297: return
LayoutRect(documentRect());
Not your fault, but could you add a TODO to resolve the pixel-snapping issue
with documentRect()?

[Xianzhu, 2016-09-27 18:36:23]

https://codereview.chromium.org/2371933003/diff/10010/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (left):

https://codereview.chromium.org/2371933003/diff/10010/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutView.cpp:297: return
LayoutRect(documentRect());
On 2016/09/27 18:24:34, chrishtr wrote:
> Not your fault, but could you add a TODO to resolve the pixel-snapping issue
> with documentRect()?

Done.

[chrishtr, 2016-09-27 18:55:20]

lgtm

https://codereview.chromium.org/2371933003/diff/50001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/2371933003/diff/50001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutView.cpp:825: //
TODO(crbug.com/650768): The pixel snapping looks incorect.
nit: "incorrect"

[Xianzhu, 2016-09-27 20:48:39]

https://codereview.chromium.org/2371933003/diff/50001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/2371933003/diff/50001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutView.cpp:825: //
TODO(crbug.com/650768): The pixel snapping looks incorect.
On 2016/09/27 18:55:20, chrishtr wrote:
> nit: "incorrect"

Done.

[Xianzhu, 2016-09-27 20:48:44]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-09-27 20:48:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
wkorman@chromium.org, chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2371933003/#ps70001
(title: "-")

[commit-bot: I haz the power, 2016-09-27 20:49:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 22:28:50]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2016-09-27 22:31:07]

Description was changed from

==========
Fix paint invalidation for composited vertical-rl having overflow

Previously we special cased in slowMapToVisualRectInAncestorSpace()
in PaintInvalidationState.cpp for LayoutView background invalidation
which was because LayoutView::localOverflowRectForPaintInvalidation()
was in physical coordinates while other
localOverflowRectForPaintInvalidation() implementations are in flipped
physical coordinates.

This CL removes the special case, but let
LayoutView::localOverflowRectForPaintInvalidation() returns a rect in
flipped physical coordinates.

BUG=650417
TEST=paint/invalidation/composited-vertical-rl-overflow.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix paint invalidation for composited vertical-rl having overflow

Previously we special cased in slowMapToVisualRectInAncestorSpace()
in PaintInvalidationState.cpp for LayoutView background invalidation
which was because LayoutView::localOverflowRectForPaintInvalidation()
was in physical coordinates while other
localOverflowRectForPaintInvalidation() implementations are in flipped
physical coordinates.

This CL removes the special case, but let
LayoutView::localOverflowRectForPaintInvalidation() returns a rect in
flipped physical coordinates.

BUG=650417
TEST=paint/invalidation/composited-vertical-rl-overflow.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/049d91617706a0fd7f96e861364e4340c966d855
Cr-Commit-Position: refs/heads/master@{#421362}
==========

[commit-bot: I haz the power, 2016-09-27 22:31:08]

Patchset 5 (id:??) landed as
https://crrev.com/049d91617706a0fd7f96e861364e4340c966d855
Cr-Commit-Position: refs/heads/master@{#421362}