Account for filter bounds for backgroundRect during PaintLayer painting

Account for filter bounds for backgroundRect during PaintLayer painting.

BUG=701654
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2781863005
Cr-Commit-Position: refs/heads/master@{#461014}
Committed: https://chromium.googlesource.com/chromium/src/+/9dde8ce672f222dc77bd460ac703a2bef6910bc7

[chrishtr, 2017-03-29 23:14:27]

Description was changed from

==========
none


none

BUG=
==========

to

==========
none


none

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-03-29 23:16:21]

Description was changed from

==========
none


none

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Account for filter bounds for backgroundRect during PaintLayer painting.

BUG=701654
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-03-29 23:17:36]

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

[chrishtr, 2017-03-29 23:17:41]

chrishtr@chromium.org changed reviewers:
+ trchen@chromium.org

[commit-bot: I haz the power, 2017-03-29 23:18:37]

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

[trchen, 2017-03-29 23:50:08]

https://codereview.chromium.org/2781863005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right):

https://codereview.chromium.org/2781863005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:206: clipRect);
Why is this line needed? The only caller of PLC::localClipRect() is
PaintLayer::boundingBoxForCompositingInternal(), and the caller intersect the
rect with physicalBoundingBox() immediately.

https://codereview.chromium.org/2781863005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:503: ClipRect& clip)
const {
The name doesn't seem to match the implementation... (In fact the implementation
looks weird.)

It seems to me that clip would be a in-out parameter, and the expected input
would be the accumulated clip that should apply to all phases of the current
layer. Then if the current object does not create its own overflow clip, returns
the accumulated clip untouched. Otherwise, return its propagating visual
overflow clipped by the accumulated clip.

https://codereview.chromium.org/2781863005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:520: 
Is it possible to make it a geometry mapper helper? This line together with line
521 basically reads
geometryMapper->mapVisualRectToAncestor(rootLayerState, layerState,
layerBoundsWithVisualOverflow)
to me.

[commit-bot: I haz the power, 2017-03-30 01:26:18]

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

[commit-bot: I haz the power, 2017-03-30 01:26:19]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2017-03-30 01:41:23]

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

[commit-bot: I haz the power, 2017-03-30 01:42:34]

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, 2017-03-30 02:48:43]

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

[commit-bot: I haz the power, 2017-03-30 02:48:44]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chrishtr, 2017-03-30 04:35:13]

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

[chrishtr, 2017-03-30 04:35:26]

https://codereview.chromium.org/2781863005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right):

https://codereview.chromium.org/2781863005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:206: clipRect);
On 2017/03/29 at 23:50:08, trchen wrote:
> Why is this line needed? The only caller of PLC::localClipRect() is
PaintLayer::boundingBoxForCompositingInternal(), and the caller intersect the
rect with physicalBoundingBox() immediately.

There is not really any point, but if I remove this line then the
GeometryMapper mode of this class will disagree with non-GeometryMapper,
which delegates to calculateRects. I don't think it does any harm.

https://codereview.chromium.org/2781863005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:503: ClipRect& clip)
const {
On 2017/03/29 at 23:50:08, trchen wrote:
> The name doesn't seem to match the implementation... (In fact the
implementation looks weird.)
> 
> It seems to me that clip would be a in-out parameter, and the expected input
would be the accumulated clip that should apply to all phases of the current
layer. Then if the current object does not create its own overflow clip, returns
the accumulated clip untouched. Otherwise, return its propagating visual
overflow clipped by the accumulated clip.

Method is now deleted.

https://codereview.chromium.org/2781863005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:520: 
On 2017/03/29 at 23:50:08, trchen wrote:
> Is it possible to make it a geometry mapper helper? This line together with
line 521 basically reads
> geometryMapper->mapVisualRectToAncestor(rootLayerState, layerState,
layerBoundsWithVisualOverflow)
> to me.

That's a great observation. In fact it led me to realize that this whole
method is not necessary, since I can call sourceToDestinationVisualRect
in cases when shouldClipOverflow() is true. This will be a significant
time savings in paint.

[commit-bot: I haz the power, 2017-03-30 04:35:37]

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, 2017-03-30 06:03:19]

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

[commit-bot: I haz the power, 2017-03-30 06:03:20]

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

[chrishtr, 2017-03-30 15:11:20]

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

[commit-bot: I haz the power, 2017-03-30 15:12:08]

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, 2017-03-30 16:29:25]

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

[commit-bot: I haz the power, 2017-03-30 16:29:25]

Dry run: This issue passed the CQ dry run.

[trchen, 2017-03-30 18:46:40]

https://codereview.chromium.org/2781863005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right):

https://codereview.chromium.org/2781863005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:410: void
PaintLayerClipper::calculateBackgroundClipRectWithGeometryMapper(
This look a lot similar to calculateForegroundClipRectWithGeometryMapper()
below.
Is it possible to extract some common logic to helper functions? Like functions
to map PaintLayer --> PaintPropertyTreeState for different stage, e.g.
pre-overflow-clip, pre-effect, post-effect states.

Doesn't have to be in this CL though. We can land then polish.

https://codereview.chromium.org/2781863005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:440: FloatRect
clipRect(localVisualRect());
This part still puzzles me. It seems to me the function is supposed to return a
clip rect that is suitable for clipping the stacking context's background phase.
Why should the visual overflow (which is supposed to be clipped by this rect)
take part in computation of this rect? A comment would help the reader to
understand.

https://codereview.chromium.org/2781863005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:476: if (&m_layer ==
context.rootLayer) {
Not related to this CL. Just taking note to remind myself this juggling looks
puzzling and I can make it cleaner.

[chrishtr, 2017-03-30 23:50:14]

https://codereview.chromium.org/2781863005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right):

https://codereview.chromium.org/2781863005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:410: void
PaintLayerClipper::calculateBackgroundClipRectWithGeometryMapper(
On 2017/03/30 at 18:46:40, trchen wrote:
> This look a lot similar to calculateForegroundClipRectWithGeometryMapper()
below.
> Is it possible to extract some common logic to helper functions? Like
functions to map PaintLayer --> PaintPropertyTreeState for different stage, e.g.
pre-overflow-clip, pre-effect, post-effect states.
> 
> Doesn't have to be in this CL though. We can land then polish.

I went ahead and factored out some common code, since it was easy enough.

https://codereview.chromium.org/2781863005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:440: FloatRect
clipRect(localVisualRect());
On 2017/03/30 at 18:46:40, trchen wrote:
> This part still puzzles me. It seems to me the function is supposed to return
a clip rect that is suitable for clipping the stacking context's background
phase. Why should the visual overflow (which is supposed to be clipped by this
rect) take part in computation of this rect? A comment would help the reader to
understand.

The background phase also paints the visual overflow, including in this CL's
example, software filters. The backgroundRect is clipped to the bounds
of all of these effects, as far as I can tell as a performance optimization.

https://codereview.chromium.org/2781863005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:476: if (&m_layer ==
context.rootLayer) {
On 2017/03/30 at 18:46:40, trchen wrote:
> Not related to this CL. Just taking note to remind myself this juggling looks
puzzling and I can make it cleaner.

ack

[chrishtr, 2017-03-30 23:50:22]

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

[commit-bot: I haz the power, 2017-03-30 23:51:31]

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, 2017-03-31 01:19:01]

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

[commit-bot: I haz the power, 2017-03-31 01:19:02]

Dry run: This issue passed the CQ dry run.

[trchen, 2017-03-31 02:13:35]

lgtm

On 2017/03/30 23:50:14, chrishtr wrote:
>
https://codereview.chromium.org/2781863005/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right):
> 
>
https://codereview.chromium.org/2781863005/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:410: void
> PaintLayerClipper::calculateBackgroundClipRectWithGeometryMapper(
> On 2017/03/30 at 18:46:40, trchen wrote:
> > This look a lot similar to calculateForegroundClipRectWithGeometryMapper()
> below.
> > Is it possible to extract some common logic to helper functions? Like
> functions to map PaintLayer --> PaintPropertyTreeState for different stage,
e.g.
> pre-overflow-clip, pre-effect, post-effect states.
> > 
> > Doesn't have to be in this CL though. We can land then polish.
> 
> I went ahead and factored out some common code, since it was easy enough.

Thanks. This looks great!

>
https://codereview.chromium.org/2781863005/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:440: FloatRect
> clipRect(localVisualRect());
> On 2017/03/30 at 18:46:40, trchen wrote:
> > This part still puzzles me. It seems to me the function is supposed to
return
> a clip rect that is suitable for clipping the stacking context's background
> phase. Why should the visual overflow (which is supposed to be clipped by this
> rect) take part in computation of this rect? A comment would help the reader
to
> understand.
> 
> The background phase also paints the visual overflow, including in this CL's
> example, software filters. The backgroundRect is clipped to the bounds
> of all of these effects, as far as I can tell as a performance optimization.

Thanks for the explanation! Feels like a weird place to do such optimization,
but oh well.

[chrishtr, 2017-03-31 03:24:22]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2017-03-31 03:24:54]

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, 2017-03-31 03:33:37]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490930662767720,
"parent_rev": "904a97549b34407f69820f1ff114c861f9c35bf6", "commit_rev":
"9dde8ce672f222dc77bd460ac703a2bef6910bc7"}

[commit-bot: I haz the power, 2017-03-31 03:34:57]

Description was changed from

==========
Account for filter bounds for backgroundRect during PaintLayer painting.

BUG=701654
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Account for filter bounds for backgroundRect during PaintLayer painting.

BUG=701654
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2781863005
Cr-Commit-Position: refs/heads/master@{#461014}
Committed:
https://chromium.googlesource.com/chromium/src/+/9dde8ce672f222dc77bd460ac703...
==========

[commit-bot: I haz the power, 2017-03-31 03:34:58]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/9dde8ce672f222dc77bd460ac703...