[SPv2] Paint invalidation in PreWalkTreeWalk, plumbed on PaintInvalidationState

[SPv2] Paint invalidation in PreWalkTreeWalk, plumbed on PaintInvalidationState

It's temporarily plumbed on PaintInvalidationState to let it work for spv2.
Will gradually switch to new code, using the paint property data collected
by PaintPropertyTreeBuilder.

Enable SlimmingPaintInvalidation by default when spv2 is enabled.

BUG=510908
Committed: https://crrev.com/2e9667d6597f5c1cbf25b0cc7f89a687502927b5
Cr-Commit-Position: refs/heads/master@{#398095}

[Xianzhu, 2016-06-02 19:24:57]

Description was changed from

==========
Paint invalidation in PreWalkTreeWalk, plumbed on PaintInvalidationState

BUG=510908
==========

to

==========
[SPv2] Paint invalidation in PreWalkTreeWalk, plumbed on PaintInvalidationState

BUG=510908
==========

[Xianzhu, 2016-06-03 01:10:44]

Description was changed from

==========
[SPv2] Paint invalidation in PreWalkTreeWalk, plumbed on PaintInvalidationState

BUG=510908
==========

to

==========
[SPv2] Paint invalidation in PreWalkTreeWalk, plumbed on PaintInvalidationState

It's temporarily plumbed on PaintInvalidationState to let it work for spv2.
Will gradually switch to new code, using the paint property data collected
by PaintPropertyTreeBuilder.

BUG=510908
==========

[Xianzhu, 2016-06-03 01:11:02]

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

[Xianzhu, 2016-06-03 01:11:51]

Description was changed from

==========
[SPv2] Paint invalidation in PreWalkTreeWalk, plumbed on PaintInvalidationState

It's temporarily plumbed on PaintInvalidationState to let it work for spv2.
Will gradually switch to new code, using the paint property data collected
by PaintPropertyTreeBuilder.

BUG=510908
==========

to

==========
[SPv2] Paint invalidation in PreWalkTreeWalk, plumbed on PaintInvalidationState

It's temporarily plumbed on PaintInvalidationState to let it work for spv2.
Will gradually switch to new code, using the paint property data collected
by PaintPropertyTreeBuilder.

Enable SlimmingPaintInvalidation by default when spv2 is enabled.

BUG=510908
==========

[pdr., 2016-06-03 04:43:05]

https://codereview.chromium.org/2033183002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2033183002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:39: crbug.com/510908
virtual/spv2/svg/custom/object-sizing-no-width-height-change-content-box-size.xhtml
[ Failure ]
Where does this failure come from?

https://codereview.chromium.org/2033183002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/2033183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1113: #if ENABLE(ASSERT)
This assert got dropped but it's still useful (maybe in a different place)

https://codereview.chromium.org/2033183002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2033183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1391:
PaintInvalidationReason invalidatePaintIfNeeded(const PaintInvalidationState&
paintInvalidationState) { return
m_layoutObject.invalidatePaintIfNeeded(paintInvalidationState); }
I think we accidentally backed into a bad design with MutableForPainting.
LayoutObject::invalidatePaintIfNeeded has non local side effects which make me
scared about violating const correctness. As an example, I can imagine someone
writing the following:
  const TableBorderWidth& width = toTable(object).getBorderWidth();
  PaintInvalidationReason reason =
object.getMutableForPainting().invalidatePaintIfNeeded(...);
  if (width > blah) // use-after-free because
LayoutTable::invalidatePaintIfNeeded recalcs table borders

What do you think about changing the invalidation walk to not use const?

I feel a little better about the paint property tree walk using const
LayoutObjects because it's a minor implementation detail that
ObjectPaintProperties is stored on LayoutObject.

[Xianzhu, 2016-06-03 16:46:18]

https://codereview.chromium.org/2033183002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2033183002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:39: crbug.com/510908
virtual/spv2/svg/custom/object-sizing-no-width-height-change-content-box-size.xhtml
[ Failure ]
On 2016/06/03 04:43:04, pdr. wrote:
> Where does this failure come from?

The plumbing on PaintInvalidationState is not complete yet, missing
forcedSubtreeInvalidationCheckingWithinContainer on
positionFromPaintInvalidationBacking change, etc. The missing parts will not be
plumbed on PaintInvalidationState, but will be implemented with new measures
(e.g. by checking change of paintOffset), and the failure will be fixed.

https://codereview.chromium.org/2033183002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/2033183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1113: #if ENABLE(ASSERT)
On 2016/06/03 04:43:04, pdr. wrote:
> This assert got dropped but it's still useful (maybe in a different place)

This is an accident. Restored.

https://codereview.chromium.org/2033183002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2033183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:1391:
PaintInvalidationReason invalidatePaintIfNeeded(const PaintInvalidationState&
paintInvalidationState) { return
m_layoutObject.invalidatePaintIfNeeded(paintInvalidationState); }
On 2016/06/03 04:43:04, pdr. wrote:
> I think we accidentally backed into a bad design with MutableForPainting.
> LayoutObject::invalidatePaintIfNeeded has non local side effects which make me
> scared about violating const correctness. As an example, I can imagine someone
> writing the following:
>   const TableBorderWidth& width = toTable(object).getBorderWidth();
>   PaintInvalidationReason reason =
> object.getMutableForPainting().invalidatePaintIfNeeded(...);
>   if (width > blah) // use-after-free because
> LayoutTable::invalidatePaintIfNeeded recalcs table borders
> 
> What do you think about changing the invalidation walk to not use const?
> 
> I feel a little better about the paint property tree walk using const
> LayoutObjects because it's a minor implementation detail that
> ObjectPaintProperties is stored on LayoutObject.

MutableForPainting is intended to avoid unexpected mutation of most of the
LayoutObject states (e.g. style and layout), while providing a convenient way to
store some data for painting (including paint invalidation) in LayoutObjects.
Another way is to mark the data for painting 'mutable' and the methods accessing
the data 'const', but the MutableForPainting method marks the mutable things for
painting more explicitly.

The side-effect of invalidatePaintIfNeeded() is clear: it should not change any
style/layout states, but only states for paint invalidation which include
previous paint invalidation geometries, paint invalidation flags, etc. Using
const LayoutObject in paint invalidation helps us to avoid unexpected mutation
to other states.

I think you meant collapsed border data in the example. It's safe because it's
protected by m_collapsedBordersValid flag. Extra calls to
recalcCollapsedBordersIfNeeded() will not cause use-after-free because the
function does nothing when m_collapsedBordersValid is true, and we can't access
collapsedBorderValues when m_collapsedBordersValid is false.

The plumbings on PaintInvalidationState and
LayoutObject::invalidatePaintIfNeeded() are temporary. They will be gradually
replaced by new code in PaintInvalidator. Eventually we'll remove
invalidatePaintIfNeeded() from MutableForPainting and add more direct mutation
accessors, then MutableForPainting will contain just accessor methods to
paint-specific data.

[pdr., 2016-06-03 18:00:34]

On 2016/06/03 at 16:46:18, wangxianzhu wrote:
>
https://codereview.chromium.org/2033183002/diff/80001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/2033183002/diff/80001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/TestExpectations:39: crbug.com/510908
virtual/spv2/svg/custom/object-sizing-no-width-height-change-content-box-size.xhtml
[ Failure ]
> On 2016/06/03 04:43:04, pdr. wrote:
> > Where does this failure come from?
> 
> The plumbing on PaintInvalidationState is not complete yet, missing
forcedSubtreeInvalidationCheckingWithinContainer on
positionFromPaintInvalidationBacking change, etc. The missing parts will not be
plumbed on PaintInvalidationState, but will be implemented with new measures
(e.g. by checking change of paintOffset), and the failure will be fixed.
> 
>
https://codereview.chromium.org/2033183002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/FrameView.cpp (left):
> 
>
https://codereview.chromium.org/2033183002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/FrameView.cpp:1113: #if ENABLE(ASSERT)
> On 2016/06/03 04:43:04, pdr. wrote:
> > This assert got dropped but it's still useful (maybe in a different place)
> 
> This is an accident. Restored.
> 
>
https://codereview.chromium.org/2033183002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutObject.h (right):
> 
>
https://codereview.chromium.org/2033183002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutObject.h:1391:
PaintInvalidationReason invalidatePaintIfNeeded(const PaintInvalidationState&
paintInvalidationState) { return
m_layoutObject.invalidatePaintIfNeeded(paintInvalidationState); }
> On 2016/06/03 04:43:04, pdr. wrote:
> > I think we accidentally backed into a bad design with MutableForPainting.
> > LayoutObject::invalidatePaintIfNeeded has non local side effects which make
me
> > scared about violating const correctness. As an example, I can imagine
someone
> > writing the following:
> >   const TableBorderWidth& width = toTable(object).getBorderWidth();
> >   PaintInvalidationReason reason =
> > object.getMutableForPainting().invalidatePaintIfNeeded(...);
> >   if (width > blah) // use-after-free because
> > LayoutTable::invalidatePaintIfNeeded recalcs table borders
> > 
> > What do you think about changing the invalidation walk to not use const?
> > 
> > I feel a little better about the paint property tree walk using const
> > LayoutObjects because it's a minor implementation detail that
> > ObjectPaintProperties is stored on LayoutObject.
> 
> MutableForPainting is intended to avoid unexpected mutation of most of the
LayoutObject states (e.g. style and layout), while providing a convenient way to
store some data for painting (including paint invalidation) in LayoutObjects.
Another way is to mark the data for painting 'mutable' and the methods accessing
the data 'const', but the MutableForPainting method marks the mutable things for
painting more explicitly.
> 
> The side-effect of invalidatePaintIfNeeded() is clear: it should not change
any style/layout states, but only states for paint invalidation which include
previous paint invalidation geometries, paint invalidation flags, etc. Using
const LayoutObject in paint invalidation helps us to avoid unexpected mutation
to other states.
> 
> I think you meant collapsed border data in the example. It's safe because it's
protected by m_collapsedBordersValid flag. Extra calls to
recalcCollapsedBordersIfNeeded() will not cause use-after-free because the
function does nothing when m_collapsedBordersValid is true, and we can't access
collapsedBorderValues when m_collapsedBordersValid is false.
> 
> The plumbings on PaintInvalidationState and
LayoutObject::invalidatePaintIfNeeded() are temporary. They will be gradually
replaced by new code in PaintInvalidator. Eventually we'll remove
invalidatePaintIfNeeded() from MutableForPainting and add more direct mutation
accessors, then MutableForPainting will contain just accessor methods to
paint-specific data.

LGTM. This patch has some issues but inches us closer towards the goal.

Please give chrishtr a chance to lg too.

[chrishtr, 2016-06-04 06:53:27]

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

https://codereview.chromium.org/2033183002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:362: if
(!object.isBoxModelObject() && !object.isSVG())
Why this new code?

[Xianzhu, 2016-06-06 02:16:43]

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

https://codereview.chromium.org/2033183002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:362: if
(!object.isBoxModelObject() && !object.isSVG())
On 2016/06/04 06:53:27, chrishtr wrote:
> Why this new code?

This is moved from PrePaintTreeWalker because PaintInvalidator needs to visit
LayoutTexts.

[chrishtr, 2016-06-06 15:39:43]

lgtm

[Xianzhu, 2016-06-06 17:56:10]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-06-06 17:56:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2033183002/100001

[commit-bot: I haz the power, 2016-06-06 19:39:30]

Description was changed from

==========
[SPv2] Paint invalidation in PreWalkTreeWalk, plumbed on PaintInvalidationState

It's temporarily plumbed on PaintInvalidationState to let it work for spv2.
Will gradually switch to new code, using the paint property data collected
by PaintPropertyTreeBuilder.

Enable SlimmingPaintInvalidation by default when spv2 is enabled.

BUG=510908
==========

to

==========
[SPv2] Paint invalidation in PreWalkTreeWalk, plumbed on PaintInvalidationState

It's temporarily plumbed on PaintInvalidationState to let it work for spv2.
Will gradually switch to new code, using the paint property data collected
by PaintPropertyTreeBuilder.

Enable SlimmingPaintInvalidation by default when spv2 is enabled.

BUG=510908
==========

[commit-bot: I haz the power, 2016-06-06 19:39:31]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-06 19:41:50]

Description was changed from

==========
[SPv2] Paint invalidation in PreWalkTreeWalk, plumbed on PaintInvalidationState

It's temporarily plumbed on PaintInvalidationState to let it work for spv2.
Will gradually switch to new code, using the paint property data collected
by PaintPropertyTreeBuilder.

Enable SlimmingPaintInvalidation by default when spv2 is enabled.

BUG=510908
==========

to

==========
[SPv2] Paint invalidation in PreWalkTreeWalk, plumbed on PaintInvalidationState

It's temporarily plumbed on PaintInvalidationState to let it work for spv2.
Will gradually switch to new code, using the paint property data collected
by PaintPropertyTreeBuilder.

Enable SlimmingPaintInvalidation by default when spv2 is enabled.

BUG=510908

Committed: https://crrev.com/2e9667d6597f5c1cbf25b0cc7f89a687502927b5
Cr-Commit-Position: refs/heads/master@{#398095}
==========

[commit-bot: I haz the power, 2016-06-06 19:41:52]

Patchset 6 (id:??) landed as
https://crrev.com/2e9667d6597f5c1cbf25b0cc7f89a687502927b5
Cr-Commit-Position: refs/heads/master@{#398095}