Skip PaintPhaseDescendantOutlinesOnly if no descendent outlines in the layer

Skip PaintPhaseDescendantOutlinesOnly if no descendant outlines in the layer

BUG=574938
TEST=PaintLayerPainterTest.PaintPhaseOutline
TEST=fast/multicol/span/outline.html

Committed: https://crrev.com/42c48698112091f8067d13d8e8d84a799b658e4b
Cr-Commit-Position: refs/heads/master@{#371652}

[Xianzhu, 2016-01-12 18:52:48]

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

[chrishtr, 2016-01-12 23:52:24]

https://codereview.chromium.org/1584493002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1584493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1389: PaintLayer* layer =
enclosingLayer()->enclosingSelfPaintingLayer();
This is a tree walk for every LayoutObject. PaintInvalidationState goes to great
lengths to avoid this for paint invalidation container, I don't think we can
afford this cost...

https://codereview.chromium.org/1584493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1390: if
(layer->layoutObject() != this)
Why this conditional?

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

https://codereview.chromium.org/1584493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:951: for (PaintLayer* layer
= const_cast<PaintLayer*>(this); layer; layer = layer->parent()) {
should be compositingContainer() instead of parent() right?

[Xianzhu, 2016-01-13 00:11:03]

Also found that some painters (e.g. TablePainter) handles PaintPhaseSelfOutline
and PaintPhaseChildOutlines incorrectly. They paint child outlines during
PaintPhaseSelfOutline phase, but ignore PaintPhaseChildOutlines. This was not an
issue before this change because they two phases were interchangeably if the
layer is not scrollable.

Also working on fixing this.

https://codereview.chromium.org/1584493002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1584493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1389: PaintLayer* layer =
enclosingLayer()->enclosingSelfPaintingLayer();
On 2016/01/12 23:52:24, chrishtr wrote:
> This is a tree walk for every LayoutObject. PaintInvalidationState goes to
great
> lengths to avoid this for paint invalidation container, I don't think we can
> afford this cost...

Not every LayoutObject, but only LayoutObject having outline which is relatively
rare.

Just noticed that we already have such tree walk in
LayoutObject::invalidateDisplayItemClients(), but there haven't been performance
regressions reported, so we should definitely fix this. Will land the fix before
this CL.

https://codereview.chromium.org/1584493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1390: if
(layer->layoutObject() != this)
On 2016/01/12 23:52:24, chrishtr wrote:
> Why this conditional?

We paint self outline and child outlines in different phases. Here we need to
exclude self outline.

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

https://codereview.chromium.org/1584493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:951: for (PaintLayer* layer
= const_cast<PaintLayer*>(this); layer; layer = layer->parent()) {
On 2016/01/12 23:52:24, chrishtr wrote:
> should be compositingContainer() instead of parent() right?

No. We just want to find the self painting layer which may be between this layer
and compositingContainer(). This won't violet paint order because if this layer
is self painting, the function returns this; otherwise the self painting layer
should definitely be the ancestor of this layer.

[chrishtr, 2016-01-13 05:01:17]

https://codereview.chromium.org/1584493002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1584493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1389: PaintLayer* layer =
enclosingLayer()->enclosingSelfPaintingLayer();
On 2016/01/13 at 00:11:03, Xianzhu wrote:
> On 2016/01/12 23:52:24, chrishtr wrote:
> > This is a tree walk for every LayoutObject. PaintInvalidationState goes to
great
> > lengths to avoid this for paint invalidation container, I don't think we can
> > afford this cost...
> 
> Not every LayoutObject, but only LayoutObject having outline which is
relatively rare.
> 
> Just noticed that we already have such tree walk in
LayoutObject::invalidateDisplayItemClients(), but there haven't been performance
regressions reported, so we should definitely fix this. Will land the fix before
this CL.

I'll wait for that patch first then.

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

https://codereview.chromium.org/1584493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:951: for (PaintLayer* layer
= const_cast<PaintLayer*>(this); layer; layer = layer->parent()) {
On 2016/01/13 at 00:11:03, Xianzhu wrote:
> On 2016/01/12 23:52:24, chrishtr wrote:
> > should be compositingContainer() instead of parent() right?
> 
> No. We just want to find the self painting layer which may be between this
layer and compositingContainer(). This won't violet paint order because if this
layer is self painting, the function returns this; otherwise the self painting
layer should definitely be the ancestor of this layer.

I see, ok.

[chrishtr, 2016-01-13 05:04:01]

Also, could you performance test this? It should be straightforward to use
rasterize-and-record to demonstrate how much faster this is on a page without
outlines.

[Xianzhu, 2016-01-14 18:08:02]

On 2016/01/13 05:04:01, chrishtr wrote:
> Also, could you performance test this? It should be straightforward to use
> rasterize-and-record to demonstrate how much faster this is on a page without
> outlines.

Tested combination of several CLs (https://codereview.chromium.org/1584903002/,
https://codereview.chromium.org/1585823002/, this CL and
https://codereview.chromium.org/1581593003/) on cluster telemetry. Results are:
- On Android
  record_time_cache_disabled: -4.5%
  record_time_subsequence_cache_disabled: -32.5%
- On Linux
  record_time_cache_disabled: -4.5%
  record_time_subsequence_cache_disabled: -45.2%

See runs 420 and 421 in https://ct.skia.org/chromium_perf_runs/ for details.

[Xianzhu, 2016-01-20 18:43:09]

Depended CLs landed. This CL is ready. Ptal.

[Xianzhu, 2016-01-20 20:02:48]

Just found a bug about outline in multi-column span (not covered by existing
layout tests). Investigating.

[Xianzhu, 2016-01-20 21:44:39]

On 2016/01/20 20:02:48, Xianzhu wrote:
> Just found a bug about outline in multi-column span (not covered by existing
> layout tests). Investigating.

The latest CL changed PaintInvalidationState::enclosingLayer() to
PaintInvalidationState::enclosingSelfPaintingLayer() (with the special handling
of multicol spanner removed), and changed how LayoutObject asserts the enclosing
layer has setNeedsRepaint.

FYI, some details about multicol spanner:

For DOM:
<div style="multicol">
  <div style="-webkit-column-span: all">
    ...
  </div>
</div>

the layer/layout tree is like:
    layer 1
      LayoutBlockFlow DIV multicol
        LayoutMultiColumnSpannerPlaceholder (anonymous)
        LayoutMultiColumnSet (anonymous)
     normal flow list(1)
      layer 2
        LayoutMultiColumnFlowThread (anonymous)
          LayoutBlockFlow (column spanner) DIV column-span:all
            ...
          LayoutBlockFlow (anonymous)
            ... multi-column contents

The spanner has a special layer ownership: it's enclosingLayer is layer 2, but
it's painted by layer 1.

PaintInvalidationContainer::enclosingSelfPaintingLayer() automatically returns
layer 1 for the spanner because m_enclosingSelfPaintingLayer is set according to
the paint invalidation order for the spanner. The order is same as painting
order for the spanner: the spanner's paint invalidation and paint are invoked
from the anonymous LayoutMultiColumnSpannerPlaceholder, not the enclosing layer.

[Xianzhu, 2016-01-22 00:48:50]

Description was changed from

==========
Skip PaintPhaseChildOutlines if no child outlines in the layer

BUG=574938
TEST=PaintLayerPainterTest.PaintPhaseOutline
==========

to

==========
Skip PaintPhaseDescendantOutlinesOnly if no descendant outlines in the layer

BUG=574938
TEST=PaintLayerPainterTest.PaintPhaseOutline
==========

[Xianzhu, 2016-01-22 00:50:44]

Patchset #9 (id:160001) has been deleted

[Xianzhu, 2016-01-22 00:50:51]

Patchset #8 (id:140001) has been deleted

[Xianzhu, 2016-01-22 00:51:24]

Description was changed from

==========
Skip PaintPhaseDescendantOutlinesOnly if no descendant outlines in the layer

BUG=574938
TEST=PaintLayerPainterTest.PaintPhaseOutline
==========

to

==========
Skip PaintPhaseDescendantOutlinesOnly if no descendant outlines in the layer

BUG=574938
TEST=PaintLayerPainterTest.PaintPhaseOutline
TEST=fast/multicol/span/outline.html
==========

[Xianzhu, 2016-01-22 17:30:26]

Ptal. The PaintPhaseFloat CL depends on this.

[chrishtr, 2016-01-22 18:21:34]

chrishtr@chromium.org changed reviewers:
+ mstensho@opera.com

[chrishtr, 2016-01-22 18:21:35]

+Morten for second review of the multi-column part.

[Xianzhu, 2016-01-22 18:24:57]

On 2016/01/22 18:21:35, chrishtr wrote:
> +Morten for second review of the multi-column part.

Morten, please see #10 for what is multi-column related.

[mstensho (USE GERRIT), 2016-01-22 21:24:26]

Not too familiar with this code, so I'll just write something about spanners for
now. :)

The thing with spanners is that their containing block is the multicol
container. Anything else in the ancestry between the spanner and the multicol
container should not affect how the spanner is displayed. See
LayoutObject::containingBlock().

There's something for spanners in accumulateOffsetTowardsAncestor() in
PaintLayer.cpp. Does your CL agree with the code there? Essentially, the
enclosing layer of a spanner is always the multicol container [1].

[1] But note that we don't allow spanners to be spanners if they are put inside
things like out-of-flow objects. So it's not completely crazy. See
LayoutMultiColumnFlowThread::descendantIsValidColumnSpanner().

https://codereview.chromium.org/1584493002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/multicol/span/outline.html (right):

https://codereview.chromium.org/1584493002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/multicol/span/outline.html:2: <p>Test that
an outlined element inside a spanner. Passes if there is a complete blue
rectangle below.</p>
-"that"?

https://codereview.chromium.org/1584493002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/multicol/span/outline.html (right):

https://codereview.chromium.org/1584493002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/multicol/span/outline.html:4: <div
style="-webkit-column-span:all">
Is this still going to work if you make this one position:relative? Also, will
it keep working if you wrap a position:relative DIV around it?

[Xianzhu, 2016-01-22 22:07:22]

On 2016/01/22 21:24:26, mstensho wrote:
> Not too familiar with this code, so I'll just write something about spanners
for
> now. :)
> 
> The thing with spanners is that their containing block is the multicol
> container. Anything else in the ancestry between the spanner and the multicol
> container should not affect how the spanner is displayed. See
> LayoutObject::containingBlock().
> 
> There's something for spanners in accumulateOffsetTowardsAncestor() in
> PaintLayer.cpp. Does your CL agree with the code there? Essentially, the
> enclosing layer of a spanner is always the multicol container [1].

Yes the CL agrees it. However I modified the CL to make it more like the code
there.

> 
> [1] But note that we don't allow spanners to be spanners if they are put
inside
> things like out-of-flow objects. So it's not completely crazy. See
> LayoutMultiColumnFlowThread::descendantIsValidColumnSpanner().
>

[Xianzhu, 2016-01-22 22:07:34]

https://codereview.chromium.org/1584493002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/multicol/span/outline.html (right):

https://codereview.chromium.org/1584493002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/multicol/span/outline.html:2: <p>Test that
an outlined element inside a spanner. Passes if there is a complete blue
rectangle below.</p>
On 2016/01/22 21:24:25, mstensho wrote:
> -"that"?

Removed.

https://codereview.chromium.org/1584493002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/multicol/span/outline.html (right):

https://codereview.chromium.org/1584493002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/multicol/span/outline.html:4: <div
style="-webkit-column-span:all">
On 2016/01/22 21:24:26, mstensho wrote:
> Is this still going to work if you make this one position:relative? Also, will
> it keep working if you wrap a position:relative DIV around it?

Yes. Added a position:relative DIV wrapper.

[chrishtr, 2016-01-23 00:52:37]

Please split out the fix for multicol that is not in the current tests.
Otherwise looks generally
good.

[Xianzhu, 2016-01-23 01:17:58]

On 2016/01/23 00:52:37, chrishtr wrote:
> Please split out the fix for multicol that is not in the current tests.
> Otherwise looks generally
> good.

The multicol issue is not a standalone issue, but only with this CL when we mark
the layer needs outline phase. We need to consider multicol spanner when
checking if the correct layer is marked.

[chrishtr, 2016-01-23 01:38:02]

On 2016/01/23 at 01:17:58, wangxianzhu wrote:
> On 2016/01/23 00:52:37, chrishtr wrote:
> > Please split out the fix for multicol that is not in the current tests.
> > Otherwise looks generally
> > good.
> 
> The multicol issue is not a standalone issue, but only with this CL when we
mark the layer needs outline phase. We need to consider multicol spanner when
checking if the correct layer is marked.

Sure, but this patch has an effect on paint invalidation code even without the
outline phase code. Therefore it should be a
fix to a code structure issue which does not expose itself currently but should
be compatible with the current code.

[Xianzhu, 2016-01-23 02:08:29]

On 2016/01/23 01:17:58, Xianzhu wrote:
> On 2016/01/23 00:52:37, chrishtr wrote:
> > Please split out the fix for multicol that is not in the current tests.
> > Otherwise looks generally
> > good.
> 
> The multicol issue is not a standalone issue, but only with this CL when we
mark
> the layer needs outline phase. We need to consider multicol spanner when
> checking if the correct layer is marked.

Agreed. Done.

[chrishtr, 2016-01-23 04:58:22]

lgtm

[Xianzhu, 2016-01-26 22:40:07]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-01-26 22:40:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1584493002/#ps260001
(title: "Rebase")

[commit-bot: I haz the power, 2016-01-26 22:42:29]

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

[commit-bot: I haz the power, 2016-01-27 00:16:22]

Description was changed from

==========
Skip PaintPhaseDescendantOutlinesOnly if no descendant outlines in the layer

BUG=574938
TEST=PaintLayerPainterTest.PaintPhaseOutline
TEST=fast/multicol/span/outline.html
==========

to

==========
Skip PaintPhaseDescendantOutlinesOnly if no descendant outlines in the layer

BUG=574938
TEST=PaintLayerPainterTest.PaintPhaseOutline
TEST=fast/multicol/span/outline.html
==========

[commit-bot: I haz the power, 2016-01-27 00:16:23]

Committed patchset #12 (id:260001)

[commit-bot: I haz the power, 2016-01-27 00:17:07]

Description was changed from

==========
Skip PaintPhaseDescendantOutlinesOnly if no descendant outlines in the layer

BUG=574938
TEST=PaintLayerPainterTest.PaintPhaseOutline
TEST=fast/multicol/span/outline.html
==========

to

==========
Skip PaintPhaseDescendantOutlinesOnly if no descendant outlines in the layer

BUG=574938
TEST=PaintLayerPainterTest.PaintPhaseOutline
TEST=fast/multicol/span/outline.html

Committed: https://crrev.com/42c48698112091f8067d13d8e8d84a799b658e4b
Cr-Commit-Position: refs/heads/master@{#371652}
==========

[commit-bot: I haz the power, 2016-01-27 00:17:08]

Patchset 12 (id:??) landed as
https://crrev.com/42c48698112091f8067d13d8e8d84a799b658e4b
Cr-Commit-Position: refs/heads/master@{#371652}