Force subtree paint property updates on local border box changes

Force subtree paint property updates on local border box changes

If the local border box changes, the paint properties of descendants may
also need updating. This patch updates the local border box code to
work like all other paint property updates where changes force a
subtree update. This DCHECK was missed previously because paint offset
changes would invalidate these cases.

TEST=virtual/spinvalidation/paint/invalidation/scroll-descendant-with-cached-cliprects.html
BUG=674623
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

[pdr., 2016-12-15 19:34:16]

Description was changed from

==========
Force subtree paint property updates on local border box changes

If the local border box changes, the paint properties of descendants may
also need updating. This patch updates the local border box code to
work like all other paint property updates where changes force a
subtree update. This DCHECK was missed previously because paint offset
changes would invalidate these cases.

TEST=virtual/spinvalidation/paint/invalidation/scroll-descendant-with-cached-cliprects.html
BUG=674623
==========

to

==========
Force subtree paint property updates on local border box changes

If the local border box changes, the paint properties of descendants may
also need updating. This patch updates the local border box code to
work like all other paint property updates where changes force a
subtree update. This DCHECK was missed previously because paint offset
changes would invalidate these cases.

TEST=virtual/spinvalidation/paint/invalidation/scroll-descendant-with-cached-cliprects.html
BUG=674623
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2016-12-15 19:34:37]

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

[pdr., 2016-12-15 19:34:44]

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

[commit-bot: I haz the power, 2016-12-15 19:35:22]

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

[chrishtr, 2016-12-15 19:42:52]

https://codereview.chromium.org/2584653002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):

https://codereview.chromium.org/2584653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:3234:
TEST_P(PaintPropertyTreeBuilderTest, LocalBorderBoxChanges) {
For completeness, please also add a test for position: fixed.

https://codereview.chromium.org/2584653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:3260:
EXPECT_EQ(FloatRect(0, 0, 800, 600),
Also check its transform node and scroll node to see that it's the root and not
the scroller.

And an example with effects?

[Xianzhu, 2016-12-15 19:46:28]

https://codereview.chromium.org/2584653002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right):

https://codereview.chromium.org/2584653002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:141:
m_localBorderBoxProperties->paintOffset = paintOffset;
What's the relationship between this check of paint offset change and the check
in PrePaintTreeWalk of paint offset change? Why doesn't the latter need paint
property update of subtree?

[pdr., 2016-12-15 20:04:54]

On 2016/12/15 at 19:46:28, wangxianzhu wrote:
>
https://codereview.chromium.org/2584653002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right):
> 
>
https://codereview.chromium.org/2584653002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:141:
m_localBorderBoxProperties->paintOffset = paintOffset;
> What's the relationship between this check of paint offset change and the
check in PrePaintTreeWalk of paint offset change? Why doesn't the latter need
paint property update of subtree?

That's a good question. I think we need to force a subtree update if paint
offset changes too, but that will hurt perf. I think we're getting away with
this currently because paint invalidation prevents the subtree walk from
stopping.

[commit-bot: I haz the power, 2016-12-15 20:49:03]

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

[commit-bot: I haz the power, 2016-12-15 20:49:03]

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

[pdr., 2016-12-15 21:09:39]

On 2016/12/15 at 20:04:54, pdr. wrote:
> On 2016/12/15 at 19:46:28, wangxianzhu wrote:
> >
https://codereview.chromium.org/2584653002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (right):
> > 
> >
https://codereview.chromium.org/2584653002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:141:
m_localBorderBoxProperties->paintOffset = paintOffset;
> > What's the relationship between this check of paint offset change and the
check in PrePaintTreeWalk of paint offset change? Why doesn't the latter need
paint property update of subtree?
> 
> That's a good question. I think we need to force a subtree update if paint
offset changes too, but that will hurt perf. I think we're getting away with
this currently because paint invalidation prevents the subtree walk from
stopping.

I talked with Chris. I think we should change the prepaint tree walk to force a
subtree update. I will do that as a followup.

[Xianzhu, 2016-12-15 21:29:30]

lgtm

[pdr., 2016-12-15 23:13:21]

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

[commit-bot: I haz the power, 2016-12-15 23:14:37]

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

[Xianzhu, 2016-12-16 00:45:39]

When thinking of solution of subtree movement, I thought of a question about
this CL: can checking of localBorderBoxProperties change catch all cases of
crbug.com/674623? For example, can this CL catch the case that an ancestor
object changes 'position' which doesn't affect its own localBorderBoxProperties
but causes some descendant needs property update?

[pdr., 2016-12-16 03:04:56]

On 2016/12/16 at 00:45:39, wangxianzhu wrote:
> When thinking of solution of subtree movement, I thought of a question about
this CL: can checking of localBorderBoxProperties change catch all cases of
crbug.com/674623? For example, can this CL catch the case that an ancestor
object changes 'position' which doesn't affect its own localBorderBoxProperties
but causes some descendant needs property update?

This is a good question. We would need a case where properties after
updateLocalBorderBoxContext are affected by the position change. There are some
paint offset cases that would be different, but we should be able to catch
those. The absolute or fixed contexts could also change though. I made a few
testcases of abs/fixed context changes that ended up being safe in different
ways, which makes me a bit worried.

Lets not land this patch and work through the remaining test failures. If no
more position changing bugs come up, lets land this. Otherwise, we may need to
force a subtree update on positioning changes.

[commit-bot: I haz the power, 2016-12-16 03:06:59]

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

[commit-bot: I haz the power, 2016-12-16 03:07:00]

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

[chrishtr, 2016-12-16 19:28:44]

On 2016/12/16 at 03:04:56, pdr wrote:
> On 2016/12/16 at 00:45:39, wangxianzhu wrote:
> > When thinking of solution of subtree movement, I thought of a question about
this CL: can checking of localBorderBoxProperties change catch all cases of
crbug.com/674623? For example, can this CL catch the case that an ancestor
object changes 'position' which doesn't affect its own localBorderBoxProperties
but causes some descendant needs property update?
> 
> This is a good question. We would need a case where properties after
updateLocalBorderBoxContext are affected by the position change. There are some
paint offset cases that would be different, but we should be able to catch
those. The absolute or fixed contexts could also change though. I made a few
testcases of abs/fixed context changes that ended up being safe in different
ways, which makes me a bit worried.
> 
> Lets not land this patch and work through the remaining test failures. If no
more position changing bugs come up, lets land this. Otherwise, we may need to
force a subtree update on positioning changes.

<style>
* { margin: 0; }
</style>
<div id=ancestor style="position: absolute; overflow: hidden">
  <div id=descendant style="position: absolute">
  </div>
</div>
<script>
ancestor.style.position = "relative";
</script>

When executing the line of script above, |ancestor| will not change its local
border box properties, because its containing block does not change.
Also, it has the same local property tree objects, because its overflow clipping
does not change. Same goes for paintOffset.

However, |descendant|'s clipping container changes because its containing block
is no longer "ancestor".

Therefore this CL needs to change to force subtree on positioning change.