Remove force-update when visual rects change.

Remove force-update when visual rects change.

This code was added in https://codereview.chromium.org/1368163002, but
subsequently https://codereview.chromium.org/2668823002 landed, which
does the same thing and in a more targeted way.

BUG=490725, 692614
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

patch from issue 2706553002 at patchset 1 (http://crrev.com/2706553002#ps1)

Review-Url: https://codereview.chromium.org/2702883002
Cr-Commit-Position: refs/heads/master@{#453808}
Committed: https://chromium.googlesource.com/chromium/src/+/ea6322df1cd9d8ce956935dd925fe64ddeda0566

[Xianzhu, 2017-02-18 07:33:24]

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

[commit-bot: I haz the power, 2017-02-18 07:33:40]

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-02-18 08:37:24]

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

[commit-bot: I haz the power, 2017-02-18 08:37:25]

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

[Xianzhu, 2017-02-18 18:29:18]

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

[commit-bot: I haz the power, 2017-02-18 18:29:28]

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-02-18 19:29:11]

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

[commit-bot: I haz the power, 2017-02-18 19:29:12]

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

[Xianzhu, 2017-02-21 17:27:08]

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

[Xianzhu, 2017-02-21 17:27:09]

chrishtr@ created the original patch
https://codereview.chromium.org/2706553002/. I reviewed it and found an issue
but he's already on vacation, so I created a new CL with the issue fixed.

[Xianzhu, 2017-02-21 17:34:46]

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

[commit-bot: I haz the power, 2017-02-21 17:35:35]

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-02-21 18:49:55]

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

[commit-bot: I haz the power, 2017-02-21 18:49:56]

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_...)

[wkorman, 2017-02-21 19:57:47]

lgtm

[Xianzhu, 2017-02-27 17:39:16]

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

[Xianzhu, 2017-02-27 17:39:17]

This needs more work.

The patch was modified to make all tests [1] pass:

- Split PaintInvalidator::updateContext() into
updatePaintInvalidationContainer() and updateVisualRect(). The former is called
whenever an object is walked, regardless of paint invalidation flags because we
may invalidate paint of an object when its clip changes without previous paint
invalidation flags, and we must ensure the context up-to-date.

- Set paint invalidation flag instead of paint property update flag when a clip
changes because a clip change doesn't affect descendant paint properties, but
affects descendant paint invalidation.

However, there seems still potential chance of failure when an ancestor clip
changed but there is no paint invalidation or paint property update flags set.

I'm considering to change the clip change detection logic into:
- check overflowClip and cssClip changes and set paint invalidation flags;
- in PrePaintTreeWalk::invalidatePaintLayerOptimizationsIfNeeded(), invalidate
layer optimizations only, without setting paint invalidation flags;
- Change PaintPropertyTreeBuilder to avoid overflowClip() if no contents will be
actually clipped (e.g. there is no overflow). This can avoid unnecessary sub
tree walk when an overflowClip changes without actual effect. This would be
helpful to cases like crbug.com/692614.

[1] The failed tests:
    All/CompositedLayerMappingTest.RootScrollerAncestorsNotClipped/0
(../../third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:76)
    All/CompositedLayerMappingTest.RootScrollerAncestorsNotClipped/1
(../../third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:76)
    RootScrollerTest.BrowserControlsResizeClippingLayer
(../../third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:938)
    RootScrollerTest.RemoveClippingOnCompositorLayers
(../../third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:756)
    RootScrollerTest.SetRootScrollerIframeUsesCorrectLayerAndCallback
(../../third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:520)
    RootScrollerTest.TestRootScrollerWithinIframe
(../../third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:465)

[chrishtr, 2017-02-27 19:18:43]

On 2017/02/27 at 17:39:17, wangxianzhu wrote:
> This needs more work.
> 
> The patch was modified to make all tests [1] pass:
> 
> - Split PaintInvalidator::updateContext() into
updatePaintInvalidationContainer() and updateVisualRect(). The former is called
whenever an object is walked, regardless of paint invalidation flags because we
may invalidate paint of an object when its clip changes without previous paint
invalidation flags, and we must ensure the context up-to-date.

Would moving the call to
PrePaintTreeWalk::invalidatePaintLayerOptimizationsIfNeeded above the call to
PaintInvalidator::invalidatPaintIfNeeded
solve that problem because it would set the force-subtree-update flag?

> 
> - Set paint invalidation flag instead of paint property update flag when a
clip changes because a clip change doesn't affect descendant paint properties,
but affects descendant paint invalidation.

This is an optimization, correct?

> 
> However, there seems still potential chance of failure when an ancestor clip
changed but there is no paint invalidation or paint property update flags set.

Would this situation you're concerned about be a bug that applies to
non-SPInvalidation SPv1 mode? I'll think some more about whether this is
possible also...

> 
> I'm considering to change the clip change detection logic into:
> - check overflowClip and cssClip changes and set paint invalidation flags;
> - in PrePaintTreeWalk::invalidatePaintLayerOptimizationsIfNeeded(), invalidate
layer optimizations only, without setting paint invalidation flags;

With this approach, how would movement of a clip relative to contained content
without the clip itself changing work? (e.g. change of transform
or paint offset of the clip).

> - Change PaintPropertyTreeBuilder to avoid overflowClip() if no contents will
be actually clipped (e.g. there is no overflow). This can avoid unnecessary sub
tree walk when an overflowClip changes without actual effect. This would be
helpful to cases like crbug.com/692614.

Does this optimization help to simplify the code in this patch?

> 
> [1] The failed tests:
>     All/CompositedLayerMappingTest.RootScrollerAncestorsNotClipped/0
(../../third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:76)
>     All/CompositedLayerMappingTest.RootScrollerAncestorsNotClipped/1
(../../third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:76)
>     RootScrollerTest.BrowserControlsResizeClippingLayer
(../../third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:938)
>     RootScrollerTest.RemoveClippingOnCompositorLayers
(../../third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:756)
>     RootScrollerTest.SetRootScrollerIframeUsesCorrectLayerAndCallback
(../../third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:520)
>     RootScrollerTest.TestRootScrollerWithinIframe
(../../third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:465)

[Xianzhu, 2017-02-27 20:32:28]

On 2017/02/27 19:18:43, chrishtr wrote:
> On 2017/02/27 at 17:39:17, wangxianzhu wrote:
> > This needs more work.
> > 
> > The patch was modified to make all tests [1] pass:
> > 
> > - Split PaintInvalidator::updateContext() into
> updatePaintInvalidationContainer() and updateVisualRect(). The former is
called
> whenever an object is walked, regardless of paint invalidation flags because
we
> may invalidate paint of an object when its clip changes without previous paint
> invalidation flags, and we must ensure the context up-to-date.
> 
> Would moving the call to
> PrePaintTreeWalk::invalidatePaintLayerOptimizationsIfNeeded above the call to
> PaintInvalidator::invalidatPaintIfNeeded
> solve that problem because it would set the force-subtree-update flag?
> 

No, because a clip cull change may be deep in a subtree of a clip change. The
clip change may not change cull rect of the object itself. We should detect clip
property change and set subtree flag.

> > 
> > - Set paint invalidation flag instead of paint property update flag when a
> clip changes because a clip change doesn't affect descendant paint properties,
> but affects descendant paint invalidation.
> 
> This is an optimization, correct?

Yes.

> 
> > 
> > However, there seems still potential chance of failure when an ancestor clip
> changed but there is no paint invalidation or paint property update flags set.
> 
> Would this situation you're concerned about be a bug that applies to
> non-SPInvalidation SPv1 mode? I'll think some more about whether this is
> possible also...
> 
> > 
> > I'm considering to change the clip change detection logic into:
> > - check overflowClip and cssClip changes and set paint invalidation flags;
> > - in PrePaintTreeWalk::invalidatePaintLayerOptimizationsIfNeeded(),
invalidate
> layer optimizations only, without setting paint invalidation flags;
> 
> With this approach, how would movement of a clip relative to contained content
> without the clip itself changing work? (e.g. change of transform
> or paint offset of the clip).

This already works because change of transform or paint offset will trigger walk
of the subtree. Otherwise invalidatePaintLayerOptimizationsIfNeeded() may not
work because we may not reach the object whose clip cull rect changes. The above
proposal ensures that we also reach the object whose clip cull rect changes on
ancestor clip change.

> 
> > - Change PaintPropertyTreeBuilder to avoid overflowClip() if no contents
will
> be actually clipped (e.g. there is no overflow). This can avoid unnecessary
sub
> tree walk when an overflowClip changes without actual effect. This would be
> helpful to cases like crbug.com/692614.
> 
> Does this optimization help to simplify the code in this patch?
> 

No, but it will improve performance by avoiding unnecessary tree walks in cases
that an ancestor clip change will cause neither visual rect change nor cull rect
change in the subtree.

> > 
> > [1] The failed tests:
> >     All/CompositedLayerMappingTest.RootScrollerAncestorsNotClipped/0
>
(../../third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:76)
> >     All/CompositedLayerMappingTest.RootScrollerAncestorsNotClipped/1
>
(../../third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:76)
> >     RootScrollerTest.BrowserControlsResizeClippingLayer
> (../../third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:938)
> >     RootScrollerTest.RemoveClippingOnCompositorLayers
> (../../third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:756)
> >     RootScrollerTest.SetRootScrollerIframeUsesCorrectLayerAndCallback
> (../../third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:520)
> >     RootScrollerTest.TestRootScrollerWithinIframe
> (../../third_party/WebKit/Source/web/tests/RootScrollerTest.cpp:465)

[Xianzhu, 2017-02-27 20:57:49]

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

[commit-bot: I haz the power, 2017-02-27 20:58:23]

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

[Xianzhu, 2017-02-27 23:00:23]

Patchset #3 (id:40001) has been deleted

[Xianzhu, 2017-02-27 23:09:07]

On 2017/02/27 20:32:28, Xianzhu wrote:
> On 2017/02/27 19:18:43, chrishtr wrote:
> > On 2017/02/27 at 17:39:17, wangxianzhu wrote:
> > > This needs more work.
> > > 
> > > The patch was modified to make all tests [1] pass:
> > > 
> > > - Split PaintInvalidator::updateContext() into
> > updatePaintInvalidationContainer() and updateVisualRect(). The former is
> called
> > whenever an object is walked, regardless of paint invalidation flags because
> we
> > may invalidate paint of an object when its clip changes without previous
paint
> > invalidation flags, and we must ensure the context up-to-date.
> > 
> > Would moving the call to
> > PrePaintTreeWalk::invalidatePaintLayerOptimizationsIfNeeded above the call
to
> > PaintInvalidator::invalidatPaintIfNeeded
> > solve that problem because it would set the force-subtree-update flag?
> > 
> 
> No, because a clip cull change may be deep in a subtree of a clip change. The
> clip change may not change cull rect of the object itself. We should detect
clip
> property change and set subtree flag.
> 

My above answer might be incorrect. I haven't found a case to support that.
PrePaintTreeWalk::invalidatePaintLayerOptimizationsIfNeeded() in the latest
patch set seems to correctly handle ancestor clip changes and set subtree paint
invalidation flag for descendants.

The unit test failures of the first patch set are all about root scroller
changes. Investigating how clip changes in the tests.

[commit-bot: I haz the power, 2017-02-28 00:54:47]

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

[commit-bot: I haz the power, 2017-02-28 00:54:48]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2017-02-28 17:22:47]

On 2017/02/27 23:09:07, Xianzhu wrote:
> 
> The unit test failures of the first patch set are all about root scroller
> changes. Investigating how clip changes in the tests.

Now I think Patch Set 2 is good for review. Ptal.

The root scroller test failures of Patch Set 1 were related to compositing
status changes, and Patch Set 2 fixes them by doing
updatePaintInvalidationContainer() (which is cheap) unconditionally. I would
like to follow-up it by investigating potential issues (e.g. where there are
missing visual rect update) and potential performance improvements (e.g. avoid
unnecessary invalidations when a compositing layer is newly created).

[chrishtr, 2017-02-28 21:25:58]

On 2017/02/27 at 23:09:07, wangxianzhu wrote:
> On 2017/02/27 20:32:28, Xianzhu wrote:
> > On 2017/02/27 19:18:43, chrishtr wrote:
> > > On 2017/02/27 at 17:39:17, wangxianzhu wrote:
> > > > This needs more work.
> > > > 
> > > > The patch was modified to make all tests [1] pass:
> > > > 
> > > > - Split PaintInvalidator::updateContext() into
> > > updatePaintInvalidationContainer() and updateVisualRect(). The former is
> > called
> > > whenever an object is walked, regardless of paint invalidation flags
because
> > we
> > > may invalidate paint of an object when its clip changes without previous
paint
> > > invalidation flags, and we must ensure the context up-to-date.
> > > 
> > > Would moving the call to
> > > PrePaintTreeWalk::invalidatePaintLayerOptimizationsIfNeeded above the call
to
> > > PaintInvalidator::invalidatPaintIfNeeded
> > > solve that problem because it would set the force-subtree-update flag?
> > > 
> > 
> > No, because a clip cull change may be deep in a subtree of a clip change.
The
> > clip change may not change cull rect of the object itself. We should detect
clip
> > property change and set subtree flag.
> > 
> 
> My above answer might be incorrect. I haven't found a case to support that.
PrePaintTreeWalk::invalidatePaintLayerOptimizationsIfNeeded() in the latest
patch set seems to correctly handle ancestor clip changes and set subtree paint
invalidation flag for descendants.


If that's the case, how about changing the patch to move the code up rather than
splitting the updateContext method?


> 
> The unit test failures of the first patch set are all about root scroller
changes. Investigating how clip changes in the tests.

[Xianzhu, 2017-02-28 22:12:55]

On 2017/02/28 21:25:58, chrishtr wrote:
> On 2017/02/27 at 23:09:07, wangxianzhu wrote:
> > On 2017/02/27 20:32:28, Xianzhu wrote:
> > > On 2017/02/27 19:18:43, chrishtr wrote:
> > > > On 2017/02/27 at 17:39:17, wangxianzhu wrote:
> > > > > This needs more work.
> > > > > 
> > > > > The patch was modified to make all tests [1] pass:
> > > > > 
> > > > > - Split PaintInvalidator::updateContext() into
> > > > updatePaintInvalidationContainer() and updateVisualRect(). The former is
> > > called
> > > > whenever an object is walked, regardless of paint invalidation flags
> because
> > > we
> > > > may invalidate paint of an object when its clip changes without previous
> paint
> > > > invalidation flags, and we must ensure the context up-to-date.
> > > > 
> > > > Would moving the call to
> > > > PrePaintTreeWalk::invalidatePaintLayerOptimizationsIfNeeded above the
call
> to
> > > > PaintInvalidator::invalidatPaintIfNeeded
> > > > solve that problem because it would set the force-subtree-update flag?
> > > > 
> > > 
> > > No, because a clip cull change may be deep in a subtree of a clip change.
> The
> > > clip change may not change cull rect of the object itself. We should
detect
> clip
> > > property change and set subtree flag.
> > > 
> > 
> > My above answer might be incorrect. I haven't found a case to support that.
> PrePaintTreeWalk::invalidatePaintLayerOptimizationsIfNeeded() in the latest
> patch set seems to correctly handle ancestor clip changes and set subtree
paint
> invalidation flag for descendants.
> 
> 
> If that's the case, how about changing the patch to move the code up rather
than
> splitting the updateContext method?
> 

This doesn't work because invalidatePaintLayerOptimizationsIfNeeded() depends on
PaintPropertyTreeBuilder::updatePropertiesForChildren().

I think splitting updateContext is a good thing because updateContext was too
long, even without the need to call updatePaintInvalidationContainer
unconditionally.

[chrishtr, 2017-02-28 22:45:28]

lgtm

https://codereview.chromium.org/2702883002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/invalidation/clip-unclip-and-change.html
(right):

https://codereview.chromium.org/2702883002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/clip-unclip-and-change.html:20:
<div style="width: 100px; height: 300px; overflow: hidden">
Add a new test rather than modifying the existing one?

[Xianzhu, 2017-03-01 00:03:33]

https://codereview.chromium.org/2702883002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/invalidation/clip-unclip-and-change.html
(right):

https://codereview.chromium.org/2702883002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/clip-unclip-and-change.html:20:
<div style="width: 100px; height: 300px; overflow: hidden">
On 2017/02/28 22:45:28, chrishtr wrote:
> Add a new test rather than modifying the existing one?
> 

The original test has been unable to test its original intent, perhaps since
some change in layout code (which seems to layout one level more than before).
This change is needed to make the test work.

[Xianzhu, 2017-03-01 00:03:39]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-03-01 00:03:40]

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

[commit-bot: I haz the power, 2017-03-01 00:04:59]

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

[chrishtr, 2017-03-01 00:51:58]

lgtm

https://codereview.chromium.org/2702883002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/invalidation/clip-unclip-and-change.html
(right):

https://codereview.chromium.org/2702883002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/clip-unclip-and-change.html:20:
<div style="width: 100px; height: 300px; overflow: hidden">
On 2017/03/01 at 00:03:33, Xianzhu wrote:
> On 2017/02/28 22:45:28, chrishtr wrote:
> > Add a new test rather than modifying the existing one?
> > 
> 
> The original test has been unable to test its original intent, perhaps since
some change in layout code (which seems to layout one level more than before).
This change is needed to make the test work.

Ok.

[commit-bot: I haz the power, 2017-03-01 03:05:48]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488326619949960,
"parent_rev": "37b0e1379938325304eeaff6afb9e7cb9f8bb618", "commit_rev":
"ea6322df1cd9d8ce956935dd925fe64ddeda0566"}

[commit-bot: I haz the power, 2017-03-01 03:06:27]

Description was changed from

==========
Remove force-update when visual rects change.

This code was added in https://codereview.chromium.org/1368163002, but
subsequently https://codereview.chromium.org/2668823002 landed, which
does the same thing and in a more targeted way.

BUG=490725,692614
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

patch from issue 2706553002 at patchset 1 (http://crrev.com/2706553002#ps1)
==========

to

==========
Remove force-update when visual rects change.

This code was added in https://codereview.chromium.org/1368163002, but
subsequently https://codereview.chromium.org/2668823002 landed, which
does the same thing and in a more targeted way.

BUG=490725,692614
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

patch from issue 2706553002 at patchset 1 (http://crrev.com/2706553002#ps1)

Review-Url: https://codereview.chromium.org/2702883002
Cr-Commit-Position: refs/heads/master@{#453808}
Committed:
https://chromium.googlesource.com/chromium/src/+/ea6322df1cd9d8ce956935dd925f...
==========

[commit-bot: I haz the power, 2017-03-01 03:06:28]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/ea6322df1cd9d8ce956935dd925f...