[SPv2] Use pre-computed paint property context for layer painter and disable fragmentation

[SPv2] Use pre-computed paint property context for layer painter and disable fragmentation

This is part #2 of the CLs to associate overflow clip/scroll nodes to
display items.

In part #1 we added the pre-computed paint property context on PaintLayer.
This CL actually associate them with the display items. It also disables
visual (PaintLayer) fragmentation because we haven't implemented property node
generation for them.

Committed: https://crrev.com/0b6ccf5a8df850b5ada971820f1376cb3c268023
Cr-Commit-Position: refs/heads/master@{#373982}

[trchen, 2016-02-01 23:58:34]

trchen@chromium.org changed reviewers:
+ jbroman@chromium.org, pdr@chromium.org

[trchen, 2016-02-01 23:58:34]

[2/5] Split out of my previous CL.

[chrishtr, 2016-02-02 19:41:10]

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

[chrishtr, 2016-02-02 19:41:10]

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

https://codereview.chromium.org/1657993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:331: // Related
thread
https://groups.google.com/a/chromium.org/forum/#!topic/graphics-dev/81XuWFf-mxM
Following up on this now in the google group, we need to come to a resolution.

[chrishtr, 2016-02-02 19:43:31]

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

https://codereview.chromium.org/1657993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:352:
scopedPaintChunkProperties.emplace(context.paintController(),
m_paintLayer.cachedSPv2PaintParameters().properties);
Is it true that this is a pure cache? i.e. the old code that you're deleting is
still
correct?

If so, please factor this into a helper method and add an ASSERT that the cached
values
match the non-cached.

[trchen, 2016-02-02 21:54:52]

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

https://codereview.chromium.org/1657993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:331: // Related
thread
https://groups.google.com/a/chromium.org/forum/#!topic/graphics-dev/81XuWFf-mxM
On 2016/02/02 19:41:10, chrishtr wrote:
> Following up on this now in the google group, we need to come to a resolution.

This CL will unblock many other things, and I don't think the decision on
multicol will affect this CL too much. (i.e. I will continue discussion on the
group but IMO that shouldn't block this CL.)

https://codereview.chromium.org/1657993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:352:
scopedPaintChunkProperties.emplace(context.paintController(),
m_paintLayer.cachedSPv2PaintParameters().properties);
On 2016/02/02 19:43:31, chrishtr wrote:
> Is it true that this is a pure cache? i.e. the old code that you're deleting
is
> still
> correct?
> 
> If so, please factor this into a helper method and add an ASSERT that the
cached
> values
> match the non-cached.

Nope, the old code won't work for non-trivial cases such as:

<div style="overflow:scroll">
    <div style="position:relative; z-index:0;"></div>
(extra clip not in the painting ancestor)

and

<div style="opacity:0.5; overflow:scroll">
    <div style="position:absolute;">
(clip in painting ancestor doesn't apply)

[chrishtr, 2016-02-02 23:31:59]

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

https://codereview.chromium.org/1657993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:331: // Related
thread
https://groups.google.com/a/chromium.org/forum/#!topic/graphics-dev/81XuWFf-mxM
On 2016/02/02 at 21:54:51, trchen wrote:
> On 2016/02/02 19:41:10, chrishtr wrote:
> > Following up on this now in the google group, we need to come to a
resolution.
> 
> This CL will unblock many other things, and I don't think the decision on
multicol will affect this CL too much. (i.e. I will continue discussion on the
group but IMO that shouldn't block this CL.)

Sure, of course.

https://codereview.chromium.org/1657993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:352:
scopedPaintChunkProperties.emplace(context.paintController(),
m_paintLayer.cachedSPv2PaintParameters().properties);
On 2016/02/02 at 21:54:51, trchen wrote:
> On 2016/02/02 19:43:31, chrishtr wrote:
> > Is it true that this is a pure cache? i.e. the old code that you're deleting
is
> > still
> > correct?
> > 
> > If so, please factor this into a helper method and add an ASSERT that the
cached
> > values
> > match the non-cached.
> 
> Nope, the old code won't work for non-trivial cases such as:
> 
> <div style="overflow:scroll">
>     <div style="position:relative; z-index:0;"></div>
> (extra clip not in the painting ancestor)
> 
> and
> 
> <div style="opacity:0.5; overflow:scroll">
>     <div style="position:absolute;">
> (clip in painting ancestor doesn't apply)

It's still not clear what you are trying to achieve.

Why does the ObjectPaintProperties of this layer not have the correct clip node?
What is the difference between that and the cached struct you are adding here?

[trchen, 2016-02-02 23:48:26]

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

https://codereview.chromium.org/1657993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:352:
scopedPaintChunkProperties.emplace(context.paintController(),
m_paintLayer.cachedSPv2PaintParameters().properties);
On 2016/02/02 23:31:59, chrishtr wrote:
> On 2016/02/02 at 21:54:51, trchen wrote:
> > On 2016/02/02 19:43:31, chrishtr wrote:
> > > Is it true that this is a pure cache? i.e. the old code that you're
deleting
> is
> > > still
> > > correct?
> > > 
> > > If so, please factor this into a helper method and add an ASSERT that the
> cached
> > > values
> > > match the non-cached.
> > 
> > Nope, the old code won't work for non-trivial cases such as:
> > 
> > <div style="overflow:scroll">
> >     <div style="position:relative; z-index:0;"></div>
> > (extra clip not in the painting ancestor)
> > 
> > and
> > 
> > <div style="opacity:0.5; overflow:scroll">
> >     <div style="position:absolute;">
> > (clip in painting ancestor doesn't apply)
> 
> It's still not clear what you are trying to achieve.
> 
> Why does the ObjectPaintProperties of this layer not have the correct clip
node?
> What is the difference between that and the cached struct you are adding here?

ObjectPaintProperties hold references to the property nodes created by the
object, but not the context applied on the object. For example, a plain boring
<div> creates no property nodes can be still affected by a transform created by
its ancestor.

The cached struct saves the complete paint property context that should be used
as a starting point to paint this stacking context. The context can't inherit
from paint traversal directly, for the two examples above, and will need a
containing block walk to compute. Having a cache saved the walk.

The old code I'm replacing is incorrect because it inherits context from paint
traversal, without walking containing block.

[trchen, 2016-02-05 00:21:00]

rebased on top of latest CL #1.

[pdr., 2016-02-05 07:34:43]

LGTM

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

https://codereview.chromium.org/1657993002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:629: LayoutPoint
paintOffset;
WDYT about just starting with -m_paintLayer.layoutBoxLocation()?

[trchen, 2016-02-05 21:51:36]

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

[commit-bot: I haz the power, 2016-02-05 21:53:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1657993002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1657993002/40001

[commit-bot: I haz the power, 2016-02-05 22:27:28]

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

[commit-bot: I haz the power, 2016-02-05 22:27:29]

Dry run: Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[pdr., 2016-02-05 22:27:45]

https://codereview.chromium.org/1657993002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/ObjectPaintProperties.h (left):

https://codereview.chromium.org/1657993002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ObjectPaintProperties.h:56:
TransformPaintPropertyNode* transformForLayerContents() const;
Oops, removing this needs an update to PaintPropertyTreePrinter.cpp

[trchen, 2016-02-05 22:36:30]

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

[commit-bot: I haz the power, 2016-02-05 22:39:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1657993002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1657993002/60001

[commit-bot: I haz the power, 2016-02-06 01:05:26]

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

[commit-bot: I haz the power, 2016-02-06 01:05:27]

Dry run: This issue passed the CQ dry run.

[trchen, 2016-02-06 01:17:48]

The CQ bit was checked by trchen@chromium.org

[trchen, 2016-02-06 01:17:48]

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/1657993002/#ps60001
(title: "update tree printer")

[commit-bot: I haz the power, 2016-02-06 01:20:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1657993002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1657993002/60001

[commit-bot: I haz the power, 2016-02-06 01:27:16]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-06 01:28:31]

Description was changed from

==========
[SPv2] Use pre-computed paint property context for layer painter and disable
fragmentation

This is part #2 of the CLs to associate overflow clip/scroll nodes to
display items.

In part #1 we added the pre-computed paint property context on PaintLayer.
This CL actually associate them with the display items. It also disables
visual (PaintLayer) fragmentation because we haven't implemented property node
generation for them.
==========

to

==========
[SPv2] Use pre-computed paint property context for layer painter and disable
fragmentation

This is part #2 of the CLs to associate overflow clip/scroll nodes to
display items.

In part #1 we added the pre-computed paint property context on PaintLayer.
This CL actually associate them with the display items. It also disables
visual (PaintLayer) fragmentation because we haven't implemented property node
generation for them.

Committed: https://crrev.com/0b6ccf5a8df850b5ada971820f1376cb3c268023
Cr-Commit-Position: refs/heads/master@{#373982}
==========

[commit-bot: I haz the power, 2016-02-06 01:28:32]

Patchset 4 (id:??) landed as
https://crrev.com/0b6ccf5a8df850b5ada971820f1376cb3c268023
Cr-Commit-Position: refs/heads/master@{#373982}