[SPInvalidation] Update paint properties when layer or paintsWithTransform status change

[SPInvalidation] Update paint properties when layer or paintsWithTransform status change

BUG=645667
TEST=virtual/spinvalidation/fast/dynamic/static-to-relative-with-absolute-child.html etc.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/d4d7aff504d23a7fd6462d2ecf51b2722545edb5
Cr-Commit-Position: refs/heads/master@{#438704}

[Xianzhu, 2016-12-14 17:55:15]

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

[Xianzhu, 2016-12-14 17:56:01]

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

[commit-bot: I haz the power, 2016-12-14 17:56:01]

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

[pdr., 2016-12-14 18:05:53]

On 2016/12/14 at 17:56:01, wangxianzhu wrote:
> 

LGTM

[Xianzhu, 2016-12-14 18:37:17]

Description was changed from

==========
[SPInvalidation] Update paint properties when a BoxModelObject gets/loses layer

BUG=645667
TEST=virtual/spinvalidation/fast/dynamic/static-to-relative-with-absolute-child.html
==========

to

==========
[SPInvalidation] Update paint properties when a BoxModelObject gets/loses layer

BUG=645667
TEST=virtual/spinvalidation/fast/dynamic/static-to-relative-with-absolute-child.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2016-12-14 18:38:15]

Description was changed from

==========
[SPInvalidation] Update paint properties when a BoxModelObject gets/loses layer

BUG=645667
TEST=virtual/spinvalidation/fast/dynamic/static-to-relative-with-absolute-child.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[SPInvalidation] Update paint properties when layer or paintsWithTransform
status change

BUG=645667
TEST=virtual/spinvalidation/fast/dynamic/static-to-relative-with-absolute-child.html
etc.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2016-12-14 18:38:22]

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

[commit-bot: I haz the power, 2016-12-14 18:39:21]

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-14 18:39:35]

When working on paintOffsetTranslation issues, I found that it is related to
this CL, so created a patch fixing both layer status change and
paintsWithTransform change.

[Xianzhu, 2016-12-14 18:39:57]

Description was changed from

==========
[SPInvalidation] Update paint properties when layer or paintsWithTransform
status change

BUG=645667
TEST=virtual/spinvalidation/fast/dynamic/static-to-relative-with-absolute-child.html
etc.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[SPInvalidation] Update paint properties when layer or paintsWithTransform
status change

BUG=645667
TEST=virtual/spinvalidation/fast/dynamic/static-to-relative-with-absolute-child.html
etc.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2016-12-14 18:41:00]

Ptal.

https://codereview.chromium.org/2574293003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2574293003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:223:
layer->paintsWithTransform(GlobalPaintFlattenCompositingLayers))
This avoids paintsWithTransform from depending on compositing state, making it
possible to use the same condition in LayoutBoxModelObject::styleDidChange() for
setting needsPaintPropertyUpdate.

[commit-bot: I haz the power, 2016-12-14 20:22:46]

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

[commit-bot: I haz the power, 2016-12-14 20:22:47]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-12-14 21:52:53]

https://codereview.chromium.org/2574293003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2574293003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:223:
layer->paintsWithTransform(GlobalPaintFlattenCompositingLayers))
On 2016/12/14 at 18:41:00, Xianzhu wrote:
> This avoids paintsWithTransform from depending on compositing state, making it
possible to use the same condition in LayoutBoxModelObject::styleDidChange() for
setting needsPaintPropertyUpdate.

"compositingState() != PaintsIntoOwnBacking" should always be false in spv2, so
I think we should track this differently. It feels like a layering violation to
use values on the paint property tree for detecting changes. I think we should
be able to catch these changes just based on style. Would it be sufficient to
check for cases where a transform style or fixed style is added or removed?

[Xianzhu, 2016-12-14 21:53:12]

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

[commit-bot: I haz the power, 2016-12-14 21:53:44]

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-14 22:02:31]

https://codereview.chromium.org/2574293003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2574293003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:223:
layer->paintsWithTransform(GlobalPaintFlattenCompositingLayers))
On 2016/12/14 21:52:53, pdr. wrote:
> On 2016/12/14 at 18:41:00, Xianzhu wrote:
> > This avoids paintsWithTransform from depending on compositing state, making
it
> possible to use the same condition in LayoutBoxModelObject::styleDidChange()
for
> setting needsPaintPropertyUpdate.
> 
> "compositingState() != PaintsIntoOwnBacking" should always be false in spv2,
so
> I think we should track this differently. It feels like a layering violation
to
> use values on the paint property tree for detecting changes. I think we should
> be able to catch these changes just based on style. Would it be sufficient to
> check for cases where a transform style or fixed style is added or removed?

When you posted this comment, I just uploaded a patch-set adding TODO(chrishtr)
here and in LayoutBoxModelObject::styleDidChange() because Chris seems about to
make further change about this (https://codereview.chromium.org/2570423003), and
I'd like to leave the follow-up of the above code to Chris.

I changed the parameter of paintsWithTransform in this CL to ensure correctness
for SlimmingPaintInvalidation.

[pdr., 2016-12-14 22:31:20]

On 2016/12/14 at 22:02:31, wangxianzhu wrote:
>
https://codereview.chromium.org/2574293003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/2574293003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:223:
layer->paintsWithTransform(GlobalPaintFlattenCompositingLayers))
> On 2016/12/14 21:52:53, pdr. wrote:
> > On 2016/12/14 at 18:41:00, Xianzhu wrote:
> > > This avoids paintsWithTransform from depending on compositing state,
making it
> > possible to use the same condition in LayoutBoxModelObject::styleDidChange()
for
> > setting needsPaintPropertyUpdate.
> > 
> > "compositingState() != PaintsIntoOwnBacking" should always be false in spv2,
so
> > I think we should track this differently. It feels like a layering violation
to
> > use values on the paint property tree for detecting changes. I think we
should
> > be able to catch these changes just based on style. Would it be sufficient
to
> > check for cases where a transform style or fixed style is added or removed?
> 
> When you posted this comment, I just uploaded a patch-set adding
TODO(chrishtr) here and in LayoutBoxModelObject::styleDidChange() because Chris
seems about to make further change about this
(https://codereview.chromium.org/2570423003), and I'd like to leave the
follow-up of the above code to Chris.
> 
> I changed the parameter of paintsWithTransform in this CL to ensure
correctness for SlimmingPaintInvalidation.

I still think we should avoid relying on the current property tree state when
making decisions on whether to invalidate. For this specific case, calculating
whether a transform is needed is not expensive, but for other paint properties
it is, so this pattern of reading the existing paint property state is not
general. In addition, this approach leads to duplicating logic in two places.
Instead, I'd like to invalidate when the paint property tree inputs change if at
all possible.

I'm unsure if Chris's patch will be landable so I'd like to avoid relying on it
just yet (Tien-Ren hasn't responded yet).

Why are the existing invalidations based on transform and position style changes
insufficient for catching this case?

[Xianzhu, 2016-12-14 23:17:15]

On 2016/12/14 22:31:20, pdr. wrote:
> On 2016/12/14 at 22:02:31, wangxianzhu wrote:
> >
>
https://codereview.chromium.org/2574293003/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/2574293003/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:223:
> layer->paintsWithTransform(GlobalPaintFlattenCompositingLayers))
> > On 2016/12/14 21:52:53, pdr. wrote:
> > > On 2016/12/14 at 18:41:00, Xianzhu wrote:
> > > > This avoids paintsWithTransform from depending on compositing state,
> making it
> > > possible to use the same condition in
LayoutBoxModelObject::styleDidChange()
> for
> > > setting needsPaintPropertyUpdate.
> > > 
> > > "compositingState() != PaintsIntoOwnBacking" should always be false in
spv2,
> so
> > > I think we should track this differently. It feels like a layering
violation
> to
> > > use values on the paint property tree for detecting changes. I think we
> should
> > > be able to catch these changes just based on style. Would it be sufficient
> to
> > > check for cases where a transform style or fixed style is added or
removed?
> > 
> > When you posted this comment, I just uploaded a patch-set adding
> TODO(chrishtr) here and in LayoutBoxModelObject::styleDidChange() because
Chris
> seems about to make further change about this
> (https://codereview.chromium.org/2570423003), and I'd like to leave the
> follow-up of the above code to Chris.
> > 
> > I changed the parameter of paintsWithTransform in this CL to ensure
> correctness for SlimmingPaintInvalidation.
> 
> I still think we should avoid relying on the current property tree state when
> making decisions on whether to invalidate. For this specific case, calculating
> whether a transform is needed is not expensive, but for other paint properties
> it is, so this pattern of reading the existing paint property state is not
> general. In addition, this approach leads to duplicating logic in two places.
> Instead, I'd like to invalidate when the paint property tree inputs change if
at
> all possible.
> 
> I'm unsure if Chris's patch will be landable so I'd like to avoid relying on
it
> just yet (Tien-Ren hasn't responded yet).
> 
> Why are the existing invalidations based on transform and position style
changes
> insufficient for catching this case?

Changed to check FixedPosition. Still need the TODOs to remind us to keep the
conditions consistent.
Ptal.

I think we always need duplicating logic (conditions) in two places in
invalidation-and-update:

Invalidation:
  if (<condition> changed)
    setNeedsUpdate();

Update:
  if (needsUpdate())
    do something depending on <condition>;

[pdr., 2016-12-14 23:23:19]

On 2016/12/14 at 23:17:15, wangxianzhu wrote:
> On 2016/12/14 22:31:20, pdr. wrote:
> > On 2016/12/14 at 22:02:31, wangxianzhu wrote:
> > >
> >
https://codereview.chromium.org/2574293003/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
> > (right):
> > > 
> > >
> >
https://codereview.chromium.org/2574293003/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:223:
> > layer->paintsWithTransform(GlobalPaintFlattenCompositingLayers))
> > > On 2016/12/14 21:52:53, pdr. wrote:
> > > > On 2016/12/14 at 18:41:00, Xianzhu wrote:
> > > > > This avoids paintsWithTransform from depending on compositing state,
> > making it
> > > > possible to use the same condition in
LayoutBoxModelObject::styleDidChange()
> > for
> > > > setting needsPaintPropertyUpdate.
> > > > 
> > > > "compositingState() != PaintsIntoOwnBacking" should always be false in
spv2,
> > so
> > > > I think we should track this differently. It feels like a layering
violation
> > to
> > > > use values on the paint property tree for detecting changes. I think we
> > should
> > > > be able to catch these changes just based on style. Would it be
sufficient
> > to
> > > > check for cases where a transform style or fixed style is added or
removed?
> > > 
> > > When you posted this comment, I just uploaded a patch-set adding
> > TODO(chrishtr) here and in LayoutBoxModelObject::styleDidChange() because
Chris
> > seems about to make further change about this
> > (https://codereview.chromium.org/2570423003), and I'd like to leave the
> > follow-up of the above code to Chris.
> > > 
> > > I changed the parameter of paintsWithTransform in this CL to ensure
> > correctness for SlimmingPaintInvalidation.
> > 
> > I still think we should avoid relying on the current property tree state
when
> > making decisions on whether to invalidate. For this specific case,
calculating
> > whether a transform is needed is not expensive, but for other paint
properties
> > it is, so this pattern of reading the existing paint property state is not
> > general. In addition, this approach leads to duplicating logic in two
places.
> > Instead, I'd like to invalidate when the paint property tree inputs change
if at
> > all possible.
> > 
> > I'm unsure if Chris's patch will be landable so I'd like to avoid relying on
it
> > just yet (Tien-Ren hasn't responded yet).
> > 
> > Why are the existing invalidations based on transform and position style
changes
> > insufficient for catching this case?
> 
> Changed to check FixedPosition. Still need the TODOs to remind us to keep the
conditions consistent.
> Ptal.

LGTM, thanks.

> 
> I think we always need duplicating logic (conditions) in two places in
invalidation-and-update:
> 
> Invalidation:
>   if (<condition> changed)
>     setNeedsUpdate();
> 
> Update:
>   if (needsUpdate())
>     do something depending on <condition>;

A difference is that we only need to duplicate the inputs, but not the
computation itself, but you do raise a good point that we still need to
duplicate code.

[Xianzhu, 2016-12-14 23:52:14]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-12-14 23:52:15]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2574293003/#ps80001
(title: "Rebase on origin/master")

[commit-bot: I haz the power, 2016-12-14 23:52:40]

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-12-15 01:29:37]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481759534764090,
"parent_rev": "c0b150ee00d60e35096a6705e4d2f48d37549e20", "commit_rev":
"f1307c6bd089952af7ff1abf2eb67a4ae03a24af"}

[commit-bot: I haz the power, 2016-12-15 01:30:29]

Description was changed from

==========
[SPInvalidation] Update paint properties when layer or paintsWithTransform
status change

BUG=645667
TEST=virtual/spinvalidation/fast/dynamic/static-to-relative-with-absolute-child.html
etc.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[SPInvalidation] Update paint properties when layer or paintsWithTransform
status change

BUG=645667
TEST=virtual/spinvalidation/fast/dynamic/static-to-relative-with-absolute-child.html
etc.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2574293003
==========

[commit-bot: I haz the power, 2016-12-15 01:30:30]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-15 01:33:08]

Description was changed from

==========
[SPInvalidation] Update paint properties when layer or paintsWithTransform
status change

BUG=645667
TEST=virtual/spinvalidation/fast/dynamic/static-to-relative-with-absolute-child.html
etc.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2574293003
==========

to

==========
[SPInvalidation] Update paint properties when layer or paintsWithTransform
status change

BUG=645667
TEST=virtual/spinvalidation/fast/dynamic/static-to-relative-with-absolute-child.html
etc.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/d4d7aff504d23a7fd6462d2ecf51b2722545edb5
Cr-Commit-Position: refs/heads/master@{#438704}
==========

[commit-bot: I haz the power, 2016-12-15 01:33:10]

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