New paint invalidation using paint property tree walk

New paint invalidation using paint property tree walk

It's under control of RuntimeEnabledFeatures::slimmingPaintInvalidation.

Now we call LayoutObject::invalidatePaintIfNeeded() from
PaintPropertyTreeBuilder with paintInvalidationContainer and paintOffset
tracked during PaintPropertyTreeBuilder tree walk.

This version still misses many pieces, like transform, clipping,
scrolling, etc. Will fix them incrementally.

Design doc: Chapter "New paint invalidation in SPv1" in https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-doA/edit?usp=sharing

BUG=591199

[Xianzhu, 2016-03-09 22:29:14]

Description was changed from

==========
New paint invalidation using paint property tree walk

BUG=591199
==========

to

==========
New paint invalidation using paint property tree walk

It's under control of RuntimeEnabledFeatures::slimmingPaintInvalidation.

Now we call LayoutObject::invalidatePaintIfNeeded() from
PaintPropertyTreeBuilder.

This version still misses many pieces, like transform, clipping,
scrolling, etc. Will fix them incrementally.

BUG=591199
==========

[Xianzhu, 2016-03-10 00:49:41]

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

[chrishtr, 2016-03-10 17:10:51]

A brief design doc on this & consensus is needed before committing code. I'd
like to make sure
pdr and trchen have seen the design and think it is ok. I can help clean up the
existing draft
if needed, let me know.

[Xianzhu, 2016-03-10 17:55:55]

Description was changed from

==========
New paint invalidation using paint property tree walk

It's under control of RuntimeEnabledFeatures::slimmingPaintInvalidation.

Now we call LayoutObject::invalidatePaintIfNeeded() from
PaintPropertyTreeBuilder.

This version still misses many pieces, like transform, clipping,
scrolling, etc. Will fix them incrementally.

BUG=591199
==========

to

==========
New paint invalidation using paint property tree walk

It's under control of RuntimeEnabledFeatures::slimmingPaintInvalidation.

Now we call LayoutObject::invalidatePaintIfNeeded() from
PaintPropertyTreeBuilder.

This version still misses many pieces, like transform, clipping,
scrolling, etc. Will fix them incrementally.

Design doc: Chapter "New paint invalidation in SPv1" in
https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d...

BUG=591199
==========

[Xianzhu, 2016-03-10 17:56:19]

On 2016/03/10 17:10:51, chrishtr wrote:
> A brief design doc on this & consensus is needed before committing code. I'd
> like to make sure
> pdr and trchen have seen the design and think it is ok. I can help clean up
the
> existing draft
> if needed, let me know.

Updated the doc and added link in CL description.

[Xianzhu, 2016-03-10 18:13:50]

Description was changed from

==========
New paint invalidation using paint property tree walk

It's under control of RuntimeEnabledFeatures::slimmingPaintInvalidation.

Now we call LayoutObject::invalidatePaintIfNeeded() from
PaintPropertyTreeBuilder.

This version still misses many pieces, like transform, clipping,
scrolling, etc. Will fix them incrementally.

Design doc: Chapter "New paint invalidation in SPv1" in
https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d...

BUG=591199
==========

to

==========
New paint invalidation using paint property tree walk

It's under control of RuntimeEnabledFeatures::slimmingPaintInvalidation.

Now we call LayoutObject::invalidatePaintIfNeeded() from
PaintPropertyTreeBuilder with paintInvalidationContainer and paintOffset
tracked during PaintPropertyTreeBuilder tree walk.

This version still misses many pieces, like transform, clipping,
scrolling, etc. Will fix them incrementally.

Design doc: Chapter "New paint invalidation in SPv1" in
https://docs.google.com/document/d/1M669yu7nsF9Wrkm7nQFi3Pp2r-QmCMqm4K7fPPo-d...

BUG=591199
==========

[Xianzhu, 2016-03-10 21:15:13]

Patchset #7 (id:120001) has been deleted

[Xianzhu, 2016-03-10 21:27:31]

Just noticed that the paintInvalidationContainer tracking method in this CL is
wrong because it still uses the containing-block/container order, not the
painting order. For example:

<div class="compositedStackingContext">
  <div class="fixed"></div>
</div>

The paintInvalidationContainer of fixed should be the compositedStackingContext.
This CL will use the LayoutView.

Fixing this.

[Xianzhu, 2016-03-10 22:37:04]

Patchset #8 (id:160001) has been deleted

[Xianzhu, 2016-03-10 22:39:08]

On 2016/03/10 21:27:31, Xianzhu wrote:
> Just noticed that the paintInvalidationContainer tracking method in this CL is
> wrong because it still uses the containing-block/container order, not the
> painting order. For example:
> 
> <div class="compositedStackingContext">
>   <div class="fixed"></div>
> </div>
> 
> The paintInvalidationContainer of fixed should be the
compositedStackingContext.
> This CL will use the LayoutView.
> 
> Fixing this.

Done. In the latest CL paintInvalidationContainer tracking should be correct.

[pdr., 2016-03-11 02:56:41]

https://codereview.chromium.org/1774193002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.h (right):

https://codereview.chromium.org/1774193002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.h:35: // For
slimmingPaintInvalidation.
Can you add the runtime enabled features part here? I think it will make it more
clear.
// For RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled.

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

https://codereview.chromium.org/1774193002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:63: const
LayoutBoxModelObject* paintInvalidationContainer;
Even when spv2 and spinvalidation are both enabled, it's convenient to keep the
property tree and paint invalidation state data separate. WDYT about introducing
a simple PaintInvalidationContext object? It might be able to hold onto the
invalidation state too.

https://codereview.chromium.org/1774193002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:337: if
(object.styleRef().isStackingContext() ||
!context.paintInvalidationContainerForOutOfFlowPositioned) {
Can you help me understand why this is needed?

We eventually want both spv2 and spinvalidation to be on so it's surprising to
see paint invalidation affecting paint property trees here.

https://codereview.chromium.org/1774193002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h (right):

https://codereview.chromium.org/1774193002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:26: const
Vector<LayoutObject*>& pendingDelayedPaintInvalidations() const { return
m_pendingDelayedPaintInvalidations; }
Does this need to be public or can it be done as the last step in
buildPropertyTrees?

We should also rename buildPropertyTrees or add a second function called
buildPropertyTreesAndInvalidatePaint

https://codereview.chromium.org/1774193002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:28: private:
This class will form the basis of all our paint work for a quarter or more so we
should be careful not to confuse folks outside the paint team. Because
PaintPropertyTreeBuilder now does one or two things, what do you think about
giving it a more ambiguous name like PrePaintTreeWalk.cpp?

@chrishtr may have thoughts too.

With or without my name suggestion, please update the comment at the top of this
file. Something like so:
// This class walks layout&frame trees in primary tree order (think DOM order),
// beginning from the root FrameView and crossing local frame boundaries. The
// walk is used to create property tree nodes for layout & frame properties such
// as clips and transforms. It should be invoked after layout, during
// InPaintPropertyUpdate.
//
// If "slimmingPaintInvalidation", we also invalidate paint along the way.
// [maybe some more detail here]

https://codereview.chromium.org/1774193002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:32:
Vector<LayoutObject*> m_pendingDelayedPaintInvalidations;
Can you add a comment above this? Something like so:
// Objects requiring invalidation after this phase of paint invalidation. See
the definition of PaintInvalidationDelayedFull for more detail.

[Xianzhu, 2016-03-11 20:03:45]

https://codereview.chromium.org/1774193002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.h (right):

https://codereview.chromium.org/1774193002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.h:35: // For
slimmingPaintInvalidation.
On 2016/03/11 02:56:41, pdr wrote:
> Can you add the runtime enabled features part here? I think it will make it
more
> clear.
> // For RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled.

Done.

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

https://codereview.chromium.org/1774193002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:63: const
LayoutBoxModelObject* paintInvalidationContainer;
On 2016/03/11 02:56:41, pdr wrote:
> Even when spv2 and spinvalidation are both enabled, it's convenient to keep
the
> property tree and paint invalidation state data separate. WDYT about
introducing
> a simple PaintInvalidationContext object? It might be able to hold onto the
> invalidation state too.

The paintInvalidationContainer fields are for spv1 only. As
PaintPropertyTreeBuilderContext is renamed to PrePaintTreeWalkContext, it seems
fine to include these for now.

https://codereview.chromium.org/1774193002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:337: if
(object.styleRef().isStackingContext() ||
!context.paintInvalidationContainerForOutOfFlowPositioned) {
On 2016/03/11 02:56:41, pdr wrote:
> Can you help me understand why this is needed?
> 
> We eventually want both spv2 and spinvalidation to be on so it's surprising to
> see paint invalidation affecting paint property trees here.

This is for SPv1 only in which we need to keep separate property trees for
GraphicsLayers. PaintInvalidationContainer will disappear for SPv2.

Added ASSERT(!RuntimeEnabledFeatures::slimmingPaintV2Enabled()) at the beginning
of the function, and also comments here.

https://codereview.chromium.org/1774193002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h (right):

https://codereview.chromium.org/1774193002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:26: const
Vector<LayoutObject*>& pendingDelayedPaintInvalidations() const { return
m_pendingDelayedPaintInvalidations; }
On 2016/03/11 02:56:41, pdr wrote:
> Does this need to be public or can it be done as the last step in
> buildPropertyTrees?
> 

We need to handle the delayed invalidations after document lifecycle is advanced
to UpdatePaintPropertiesClean to ensure we'll schedule a visual update for the
delayed paint invalidations.

> We should also rename buildPropertyTrees or add a second function called
> buildPropertyTreesAndInvalidatePaint

Done.

https://codereview.chromium.org/1774193002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:28: private:
On 2016/03/11 02:56:41, pdr wrote:
> This class will form the basis of all our paint work for a quarter or more so
we
> should be careful not to confuse folks outside the paint team. Because
> PaintPropertyTreeBuilder now does one or two things, what do you think about
> giving it a more ambiguous name like PrePaintTreeWalk.cpp?
> 
> @chrishtr may have thoughts too.
>

PrePaintTreeWalk sgtm. chrishtr wdyt?

> With or without my name suggestion, please update the comment at the top of
this
> file. Something like so:
> // This class walks layout&frame trees in primary tree order (think DOM
order),
> // beginning from the root FrameView and crossing local frame boundaries. The
> // walk is used to create property tree nodes for layout & frame properties
such
> // as clips and transforms. It should be invoked after layout, during
> // InPaintPropertyUpdate.
> //
> // If "slimmingPaintInvalidation", we also invalidate paint along the way.
> // [maybe some more detail here]

Done.

https://codereview.chromium.org/1774193002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:32:
Vector<LayoutObject*> m_pendingDelayedPaintInvalidations;
On 2016/03/11 02:56:41, pdr wrote:
> Can you add a comment above this? Something like so:
> // Objects requiring invalidation after this phase of paint invalidation. See
> the definition of PaintInvalidationDelayedFull for more detail.

Done.

[Xianzhu, 2016-03-11 20:04:47]

About renaming of PaintPropertyTreeBuilder: I included the renaming in the
latest patch set. If it looks good to you, I will create a separate patch
containing the renaming only, to make git history tracking happy.

[chrishtr, 2016-03-11 21:30:00]

https://codereview.chromium.org/1774193002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h (right):

https://codereview.chromium.org/1774193002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:28: private:
On 2016/03/11 at 20:03:45, Xianzhu wrote:
> On 2016/03/11 02:56:41, pdr wrote:
> > This class will form the basis of all our paint work for a quarter or more
so we
> > should be careful not to confuse folks outside the paint team. Because
> > PaintPropertyTreeBuilder now does one or two things, what do you think about
> > giving it a more ambiguous name like PrePaintTreeWalk.cpp?
> > 
> > @chrishtr may have thoughts too.
> >
> 
> PrePaintTreeWalk sgtm. chrishtr wdyt?

SGTM

> 
> > With or without my name suggestion, please update the comment at the top of
this
> > file. Something like so:
> > // This class walks layout&frame trees in primary tree order (think DOM
order),
> > // beginning from the root FrameView and crossing local frame boundaries.
The
> > // walk is used to create property tree nodes for layout & frame properties
such
> > // as clips and transforms. It should be invoked after layout, during
> > // InPaintPropertyUpdate.
> > //
> > // If "slimmingPaintInvalidation", we also invalidate paint along the way.
> > // [maybe some more detail here]
> 
> Done.

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/VirtualTestSuites (right):

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/VirtualTestSuites:237: "prefix":
"spinvalidation",
How many tests do these three directories add up to?

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:801: LayoutObject*
parentCrossingFrameBoundaries() const;
Make this private.

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.h (right):

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.h:36:
PaintInvalidationState(const PaintInvalidationState* parent, const
LayoutObject&, const LayoutBoxModelObject& paintInvalidationContainer, const
LayoutPoint& paintOffset);
Would it be better/possible to just fold the necessary logic into
PrePaintTreeWalkContext rather than reusing this class?

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGInline.cpp (right):

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/LayoutSVGInline.cpp:153:
ASSERT(!RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled());
Why?

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp (right):

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp:512:
ASSERT(!RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled());
Why?

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right):

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:65: const
LayoutBoxModelObject* paintInvalidationContainerForOutOfFlowPositioned;
And another one for fixed-position?

[Xianzhu, 2016-03-11 22:01:47]

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/VirtualTestSuites (right):

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/VirtualTestSuites:237: "prefix":
"spinvalidation",
On 2016/03/11 21:30:00, chrishtr wrote:
> How many tests do these three directories add up to?

548 (428 fast/repaint, 36 compositing/repaint, 84 paint).

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:801: LayoutObject*
parentCrossingFrameBoundaries() const;
On 2016/03/11 21:30:00, chrishtr wrote:
> Make this private.

Done.

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.h (right):

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.h:36:
PaintInvalidationState(const PaintInvalidationState* parent, const
LayoutObject&, const LayoutBoxModelObject& paintInvalidationContainer, const
LayoutPoint& paintOffset);
On 2016/03/11 21:30:00, chrishtr wrote:
> Would it be better/possible to just fold the necessary logic into
> PrePaintTreeWalkContext rather than reusing this class?

I believe it would be better, but it would need a big change on all paint
invalidation code. I plan the first step to reuse
LayoutObject::invalidatePaintIfNeeded() and let PrePaintTreeWalk create
PaintInvalidationStates accepted by invalidatePaintIfNeeded(). Paint
invalidation for spv2 may move all paint invalidation code into
PrePaintTreeWalk.

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGInline.cpp (right):

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/LayoutSVGInline.cpp:153:
ASSERT(!RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled());
On 2016/03/11 21:30:00, chrishtr wrote:
> Why?

Now we walk the tree in PrePaintTreeWalk in place of invalidateTreeIfNeeded and
invalidatePaintOfSubtreesIfNeeded. Next step is to extract all special logics in
all invalidateTreeIfNeeded() into PrePaintTreeWalk. Now these logics are still
missed with this patch.

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right):

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:65: const
LayoutBoxModelObject* paintInvalidationContainerForOutOfFlowPositioned;
On 2016/03/11 21:30:00, chrishtr wrote:
> And another one for fixed-position?

Previously I thought fixed-position and absolute-position are different about
their paintInvalidationContainer but then noticed they are the same: the nearest
composited stacking context ancestor.

[chrishtr, 2016-03-11 22:08:10]

On 2016/03/11 at 22:01:47, wangxianzhu wrote:
>
https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/VirtualTestSuites (right):
> 
>
https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/VirtualTestSuites:237: "prefix":
"spinvalidation",
> On 2016/03/11 21:30:00, chrishtr wrote:
> > How many tests do these three directories add up to?
> 
> 548 (428 fast/repaint, 36 compositing/repaint, 84 paint).
> 
>
https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutObject.h (right):
> 
>
https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutObject.h:801: LayoutObject*
parentCrossingFrameBoundaries() const;
> On 2016/03/11 21:30:00, chrishtr wrote:
> > Make this private.
> 
> Done.
> 
>
https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/PaintInvalidationState.h (right):
> 
>
https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/PaintInvalidationState.h:36:
PaintInvalidationState(const PaintInvalidationState* parent, const
LayoutObject&, const LayoutBoxModelObject& paintInvalidationContainer, const
LayoutPoint& paintOffset);
> On 2016/03/11 21:30:00, chrishtr wrote:
> > Would it be better/possible to just fold the necessary logic into
> > PrePaintTreeWalkContext rather than reusing this class?
> 
> I believe it would be better, but it would need a big change on all paint
invalidation code. I plan the first step to reuse
LayoutObject::invalidatePaintIfNeeded() and let PrePaintTreeWalk create
PaintInvalidationStates accepted by invalidatePaintIfNeeded(). Paint
invalidation for spv2 may move all paint invalidation code into
PrePaintTreeWalk.
> 
>
https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/svg/LayoutSVGInline.cpp (right):
> 
>
https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/svg/LayoutSVGInline.cpp:153:
ASSERT(!RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled());
> On 2016/03/11 21:30:00, chrishtr wrote:
> > Why?
> 
> Now we walk the tree in PrePaintTreeWalk in place of invalidateTreeIfNeeded
and invalidatePaintOfSubtreesIfNeeded. Next step is to extract all special
logics in all invalidateTreeIfNeeded() into PrePaintTreeWalk. Now these logics
are still missed with this patch.
> 
>
https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right):
> 
>
https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:65: const
LayoutBoxModelObject* paintInvalidationContainerForOutOfFlowPositioned;
> On 2016/03/11 21:30:00, chrishtr wrote:
> > And another one for fixed-position?
> 
> Previously I thought fixed-position and absolute-position are different about
their paintInvalidationContainer but then noticed they are the same: the nearest
composited stacking context ancestor.

I see... they differ in containing block but not in paint invalidation
container.

[chrishtr, 2016-03-11 22:08:18]

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.h (right):

https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/PaintInvalidationState.h:36:
PaintInvalidationState(const PaintInvalidationState* parent, const
LayoutObject&, const LayoutBoxModelObject& paintInvalidationContainer, const
LayoutPoint& paintOffset);
On 2016/03/11 at 22:01:47, Xianzhu wrote:
> On 2016/03/11 21:30:00, chrishtr wrote:
> > Would it be better/possible to just fold the necessary logic into
> > PrePaintTreeWalkContext rather than reusing this class?
> 
> I believe it would be better, but it would need a big change on all paint
invalidation code. I plan the first step to reuse
LayoutObject::invalidatePaintIfNeeded() and let PrePaintTreeWalk create
PaintInvalidationStates accepted by invalidatePaintIfNeeded(). Paint
invalidation for spv2 may move all paint invalidation code into
PrePaintTreeWalk.

Couldn't you do it incrementally by creating PaintInvalidationState objects
every time just before calling invalidatePaintIfNeeded?

[Xianzhu, 2016-03-11 22:14:35]

On 2016/03/11 22:08:18, chrishtr wrote:
>
https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/PaintInvalidationState.h (right):
> 
>
https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/PaintInvalidationState.h:36:
> PaintInvalidationState(const PaintInvalidationState* parent, const
> LayoutObject&, const LayoutBoxModelObject& paintInvalidationContainer, const
> LayoutPoint& paintOffset);
> On 2016/03/11 at 22:01:47, Xianzhu wrote:
> > On 2016/03/11 21:30:00, chrishtr wrote:
> > > Would it be better/possible to just fold the necessary logic into
> > > PrePaintTreeWalkContext rather than reusing this class?
> > 
> > I believe it would be better, but it would need a big change on all paint
> invalidation code. I plan the first step to reuse
> LayoutObject::invalidatePaintIfNeeded() and let PrePaintTreeWalk create
> PaintInvalidationStates accepted by invalidatePaintIfNeeded(). Paint
> invalidation for spv2 may move all paint invalidation code into
> PrePaintTreeWalk.
> 
> Couldn't you do it incrementally by creating PaintInvalidationState objects
> every time just before calling invalidatePaintIfNeeded?

This is basically what the patch does. This new PaintInvalidationState
constructor is for PrePaintTreeWalk to construct PaintInvalidationState before
calling invalidatePaintIfNeeded(). Now PaintInvalidationState is just an adapter
between PrePaintTreeWalk and invalidatePaintIfNeeded.

[chrishtr, 2016-03-11 22:39:27]

On 2016/03/11 at 22:14:35, wangxianzhu wrote:
> On 2016/03/11 22:08:18, chrishtr wrote:
> >
https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/layout/PaintInvalidationState.h (right):
> > 
> >
https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/layout/PaintInvalidationState.h:36:
> > PaintInvalidationState(const PaintInvalidationState* parent, const
> > LayoutObject&, const LayoutBoxModelObject& paintInvalidationContainer, const
> > LayoutPoint& paintOffset);
> > On 2016/03/11 at 22:01:47, Xianzhu wrote:
> > > On 2016/03/11 21:30:00, chrishtr wrote:
> > > > Would it be better/possible to just fold the necessary logic into
> > > > PrePaintTreeWalkContext rather than reusing this class?
> > > 
> > > I believe it would be better, but it would need a big change on all paint
> > invalidation code. I plan the first step to reuse
> > LayoutObject::invalidatePaintIfNeeded() and let PrePaintTreeWalk create
> > PaintInvalidationStates accepted by invalidatePaintIfNeeded(). Paint
> > invalidation for spv2 may move all paint invalidation code into
> > PrePaintTreeWalk.
> > 
> > Couldn't you do it incrementally by creating PaintInvalidationState objects
> > every time just before calling invalidatePaintIfNeeded?
> 
> This is basically what the patch does. This new PaintInvalidationState
constructor is for PrePaintTreeWalk to construct PaintInvalidationState before
calling invalidatePaintIfNeeded(). Now PaintInvalidationState is just an adapter
between PrePaintTreeWalk and invalidatePaintIfNeeded.

Except that the PaintInvalidationState is constructed for a child from the
parent, inheriting in particular the self-containing paint layer container. And
the walk() function has it in the
signature.

[Xianzhu, 2016-03-11 23:08:19]

On 2016/03/11 22:39:27, chrishtr wrote:
> On 2016/03/11 at 22:14:35, wangxianzhu wrote:
> > On 2016/03/11 22:08:18, chrishtr wrote:
> > >
>
https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/layout/PaintInvalidationState.h
(right):
> > > 
> > >
>
https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/layout/PaintInvalidationState.h:36:
> > > PaintInvalidationState(const PaintInvalidationState* parent, const
> > > LayoutObject&, const LayoutBoxModelObject& paintInvalidationContainer,
const
> > > LayoutPoint& paintOffset);
> > > On 2016/03/11 at 22:01:47, Xianzhu wrote:
> > > > On 2016/03/11 21:30:00, chrishtr wrote:
> > > > > Would it be better/possible to just fold the necessary logic into
> > > > > PrePaintTreeWalkContext rather than reusing this class?
> > > > 
> > > > I believe it would be better, but it would need a big change on all
paint
> > > invalidation code. I plan the first step to reuse
> > > LayoutObject::invalidatePaintIfNeeded() and let PrePaintTreeWalk create
> > > PaintInvalidationStates accepted by invalidatePaintIfNeeded(). Paint
> > > invalidation for spv2 may move all paint invalidation code into
> > > PrePaintTreeWalk.
> > > 
> > > Couldn't you do it incrementally by creating PaintInvalidationState
objects
> > > every time just before calling invalidatePaintIfNeeded?
> > 
> > This is basically what the patch does. This new PaintInvalidationState
> constructor is for PrePaintTreeWalk to construct PaintInvalidationState before
> calling invalidatePaintIfNeeded(). Now PaintInvalidationState is just an
adapter
> between PrePaintTreeWalk and invalidatePaintIfNeeded.
> 
> Except that the PaintInvalidationState is constructed for a child from the
> parent, inheriting in particular the self-containing paint layer container.
And
> the walk() function has it in the
> signature.

Currently PaintInvalidationState is passed through walk() just to inherit some
flags from parent PaintInvalidationState that are still needed (before we finish
the new paint invalidation, or for the whole lifecycle of paint invalidation for
spv1). Paint offset, paint invalidation container, and transform/clip (in next
steps) etc. are directly initialized based on PrePaintTreeWalkContext. With
these data using new tracking method, we can resolve the issues of the current
paint invalidation.

The flags/data that are still needed to be inherited from parent
paintInvalidationState are:
- cachedOffsetEnabled: This is for ForceHorriblySlowRectMapping. We can
gradually reduce the use of it, but I feel we have some cases (maybe related to
the fundamental compositing issue, or just some complex cases about
compositing/stacking context, etc.) that are not resolvable in spv1 and we
probably still need it.
- forcedSubtreeInvalidationWithinContainer
- forcedSubtreeInvalidationRectUpdateWithinCotainer
- enclosingSelfPaintingLayer

[trchen, 2016-03-11 23:09:39]

I'm still doing the full review, but here is one early comment.

https://codereview.chromium.org/1774193002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right):

https://codereview.chromium.org/1774193002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:25: ,
paintInvalidationContainerForOutOfFlowPositioned(nullptr)
The name of the variable isn't accurate. Paint invalidation container is a paint
order tree concept, and it shouldn't have anything to do with in-flow or
out-of-flow, which is containing block tree concept.

in-flow v.s. out-of-flow means whether a layout object's position is based on
the direct parent or some ancestor containing block.

where as normal-flow v.s. negative-flow/positive-flow means whether a layout
object paints as a part of a (pseudo) stacking context, or create paint in its
own (pseudo) stacking context.

I think what you really want to say is
paintInvalidationContainer -> paintInvalidationContainerForNormalFlow
paintInvalidationContainerForOutOfFlowPositioned ->
paintInvalidationContainerForDescendantStackingContext

And I think it worth a comment to say that normally we only need to keep one
context, but due to composited scrolling we may create paint invalidation
container for normal flow. It is the
paintInvalidationContainerForOutOfFlowPositioned that handles a special case
here.

[chrishtr, 2016-03-11 23:23:46]

On 2016/03/11 at 23:08:19, wangxianzhu wrote:
> On 2016/03/11 22:39:27, chrishtr wrote:
> > On 2016/03/11 at 22:14:35, wangxianzhu wrote:
> > > On 2016/03/11 22:08:18, chrishtr wrote:
> > > >
> >
https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
> > > > File third_party/WebKit/Source/core/layout/PaintInvalidationState.h
(right):
> > > > 
> > > >
> >
https://codereview.chromium.org/1774193002/diff/200001/third_party/WebKit/Sou...
> > > > third_party/WebKit/Source/core/layout/PaintInvalidationState.h:36:
> > > > PaintInvalidationState(const PaintInvalidationState* parent, const
> > > > LayoutObject&, const LayoutBoxModelObject& paintInvalidationContainer,
const
> > > > LayoutPoint& paintOffset);
> > > > On 2016/03/11 at 22:01:47, Xianzhu wrote:
> > > > > On 2016/03/11 21:30:00, chrishtr wrote:
> > > > > > Would it be better/possible to just fold the necessary logic into
> > > > > > PrePaintTreeWalkContext rather than reusing this class?
> > > > > 
> > > > > I believe it would be better, but it would need a big change on all
paint
> > > > invalidation code. I plan the first step to reuse
> > > > LayoutObject::invalidatePaintIfNeeded() and let PrePaintTreeWalk create
> > > > PaintInvalidationStates accepted by invalidatePaintIfNeeded(). Paint
> > > > invalidation for spv2 may move all paint invalidation code into
> > > > PrePaintTreeWalk.
> > > > 
> > > > Couldn't you do it incrementally by creating PaintInvalidationState
objects
> > > > every time just before calling invalidatePaintIfNeeded?
> > > 
> > > This is basically what the patch does. This new PaintInvalidationState
> > constructor is for PrePaintTreeWalk to construct PaintInvalidationState
before
> > calling invalidatePaintIfNeeded(). Now PaintInvalidationState is just an
adapter
> > between PrePaintTreeWalk and invalidatePaintIfNeeded.
> > 
> > Except that the PaintInvalidationState is constructed for a child from the
> > parent, inheriting in particular the self-containing paint layer container.
And
> > the walk() function has it in the
> > signature.
> 
> Currently PaintInvalidationState is passed through walk() just to inherit some
flags from parent PaintInvalidationState that are still needed (before we finish
the new paint invalidation, or for the whole lifecycle of paint invalidation for
spv1). Paint offset, paint invalidation container, and transform/clip (in next
steps) etc. are directly initialized based on PrePaintTreeWalkContext. With
these data using new tracking method, we can resolve the issues of the current
paint invalidation.
> 
> The flags/data that are still needed to be inherited from parent
paintInvalidationState are:
> - cachedOffsetEnabled: This is for ForceHorriblySlowRectMapping. We can
gradually reduce the use of it, but I feel we have some cases (maybe related to
the fundamental compositing issue, or just some complex cases about
compositing/stacking context, etc.) that are not resolvable in spv1 and we
probably still need it.
> - forcedSubtreeInvalidationWithinContainer
> - forcedSubtreeInvalidationRectUpdateWithinCotainer
> - enclosingSelfPaintingLayer


Then it seems we should expect that PaintInvalidationState will remain with
SPv1?

If so, then should all paint invalidation state go there, including ones you
currently have in the PrePaintTreeWalkContext,
and encapsulate the PaintInvalidationState as a sub-struct of the context?

If not, it seems it may be worth it to copy all of these things into the
PrePaintTreeWalkContext.

[trchen, 2016-03-11 23:36:19]

https://codereview.chromium.org/1774193002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right):

https://codereview.chromium.org/1774193002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:113:
context.paintInvalidationContainer =
context.paintInvalidationContainerForOutOfFlowPositioned;
Per the previous comment, it is not the right place to switch back invalidation
container.

https://codereview.chromium.org/1774193002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:119:
context.paintInvalidationContainer =
context.paintInvalidationContainerForOutOfFlowPositioned;
Ditto.

https://codereview.chromium.org/1774193002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:134: if
(object.isInFlowPositioned())
While this refactoring is appreciated, can we do this in another CL to reduce
noises for this CL?

https://codereview.chromium.org/1774193002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:330: if
(!object.isPaintInvalidationContainer()) {
This doesn't look right. I think it should be

if (!object.isPaintInvalidationContainer()) {
    if (object.isOrTreatedAsStackingContext())
        paintInvalidationContainerForNormalFlow =
paintInvalidationContainerForDescendantStackingContext;
    return;
}

paintInvalidationContainerForNormalFlow = object;
if (object.isStackingContext())
    paintInvalidationContainerForDescendantStackingContext = object;

https://codereview.chromium.org/1774193002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:345: // make it
correct for all cases or have to use ForceHorriblySlowRectMapping.
As pdr@ already pointed out, I don't think it makes sense to isolate property
tree per invalidation container. Actually as you handle the clip/scroll
escaping, most likely you will need the trees to produce correct paint offset.

For the classic example:
<div style="position:relative;">
  <div style="overflow:scroll;">
    <div style="opacity:0.5; will-change:opacity;">
      <div style="position:absolute;"></div>
    </div>
  </div>
</div>

The opacity div will create a paint invalidation container, and the innermost
element will paint into it, but the paint offset of the innermost element
involves converting from the viewport's transform node to the scrolling div's
transform node.

https://codereview.chromium.org/1774193002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:367: return;
The early return check is too late, and the semantic is unclear. We should move
it to the top and say

if (!RuntimeEnabledFeatures::slimmingPaintV2Enabled() &&
(!RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled() ||
!object.shouldCheckForPaintInvalidation(*paintInvalidationState)))
    return;

Then the message is clear: "quit if we have nothing to do".

[pdr., 2016-03-12 00:01:33]

On 2016/03/11 at 20:04:47, wangxianzhu wrote:
> About renaming of PaintPropertyTreeBuilder: I included the renaming in the
latest patch set. If it looks good to you, I will create a separate patch
containing the renaming only, to make git history tracking happy.

+1, I think this is a great idea.