Non-composited scrolling div should invalidate when it has descendant paints to its backing

Non-composited scrolling div should invalidate when it has descendant paints to its backing

This CL fixes a bug that non-composited scrolling div forgets to invalidate
because the div believed all its descandants are composited, when actually
some descendants paints to ancestor backing although composited.

BUG=401047
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180309

[trchen, 2014-08-12 01:30:50]

Hello reviewers, this CL fixes a scrolling bug when a scrollable <div> contains
both a composited <iframe> and a <div style="overflow:hidden">. PTAL, Thanks!

However personally I think this CL is not ready yet. In the pessimistic case:

<style>.scrollable { overflow: scroll }</style>
<div class="scrollable"><div class="scrollable"><div class="scrollable"><div
class="scrollable"></div></div></div></div>

the time complexity can be as bad as O(n^2). We should probably add a dirty bit
to dynamic programming this.

And of course I'll add a layout test in next upload.

[abarth-chromium, 2014-08-12 02:05:18]

Can you compute this information in one of the existing tree walks?  Trying to
compute this information from scratch looks very inefficient.

[abarth-chromium, 2014-08-12 02:05:49]

Also, can you separate out the rename into another CL?  It's adding a lot of
noise to your patch and making it hard to see what's going on.

[trchen, 2014-08-12 23:27:51]

Spent a few hours looking around the code. I realized that it is very difficult
to do this correctly.

Let me elaborate: It is not hard to write a O(n) algorithm integrated with
existing tree walk to determine whether there is non-composited RenderLayer.
However it is hard to detect whether a RenderObject actually doesn't paint
anything. If we don't detect that, this optimization is useless in many cases.
For example:

<div style="overflow:scroll; height:100px;">
    <div style="height:1000px;">
        <div style="transform:translateZ(0);">
            Lorem ipsum
        </div>
    </div>
</div>

Although a full repaint is not required in this case, our current algorithm
consider the middle <div> be a non-composited object, thus requesting a full
repaint.

Then here is the other case we have under-repaint with current algorithm:

<div style="overflow:scroll; height:100px; background-image:url('foo.png');
background-attachment:local;">
    <div style="height:1000px; transform:translateZ(0)">
        Lorem ipsum
    </div>
</div>

We fail to detect the existence of a scrolling background. There are other
potential content painted by the container. Currently we only detect selection
gaps.

I'm wondering how far are we from the universal composited scrolling? If we can
have that in the near future, then I think we can simply remove this
optimization. It is probably not worth it to find out a way to do it correctly.

[abarth-chromium, 2014-08-12 23:39:35]

On 2014/08/12 at 23:27:51, trchen wrote:
> I'm wondering how far are we from the universal composited scrolling?

We've been shipping that for a couple milestones.  It does't handle 100% of
scrolling, but it handles most of it.

> If we can have that in the near future, then I think we can simply remove this
optimization. It is probably not worth it to find out a way to do it correctly.

Do you want to try a CL that does that?  I'm not sure exactly what optimization
you're considering removing.

[trchen, 2014-08-13 00:13:54]

On 2014/08/12 at 23:39:35, abarth wrote:
> On 2014/08/12 at 23:27:51, trchen wrote:
> > I'm wondering how far are we from the universal composited scrolling?
> 
> We've been shipping that for a couple milestones.  It does't handle 100% of
scrolling, but it handles most of it.
> 
> > If we can have that in the near future, then I think we can simply remove
this optimization. It is probably not worth it to find out a way to do it
correctly.
> 
> Do you want to try a CL that does that?  I'm not sure exactly what
optimization you're considering removing.

See patchset 2. This removes the optimization to skip invalidation if a
non-composited scrollable div contains only composited children.
I'm afraid it will regress performance on too many websites unless we composite
all scrollable divs by default.

[trchen, 2014-08-13 00:23:49]

Or an alternative solution is to do the minimum effort to get known cases
work... See patchset 3.
I feel it is kind of lame but it might work surprisingly well.

[abarth-chromium, 2014-08-13 00:57:21]

This looks fine, but please add some tests.

https://codereview.chromium.org/460883002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerScrollableArea.cpp (right):

https://codereview.chromium.org/460883002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerScrollableArea.cpp:392: &&
box().style()->backgroundLayers().attachment() != LocalBackgroundAttachment;
Do we have a test that covers this case?

[trchen, 2014-08-13 00:59:35]

On 2014/08/13 at 00:57:21, abarth wrote:
> This looks fine, but please add some tests.

Sure!

>
https://codereview.chromium.org/460883002/diff/40001/Source/core/rendering/Re...
> File Source/core/rendering/RenderLayerScrollableArea.cpp (right):
> 
>
https://codereview.chromium.org/460883002/diff/40001/Source/core/rendering/Re...
> Source/core/rendering/RenderLayerScrollableArea.cpp:392: &&
box().style()->backgroundLayers().attachment() != LocalBackgroundAttachment;
> Do we have a test that covers this case?

Nope, will add a test case in addition to the original case from bug 401047.

[trchen, 2014-08-14 01:20:36]

Hello abarth, sorry for the delay. I was figuring out how to make layout tests
paint tiles at the correct timing. And also find another repaint bug when I was
making one of the tests. (See changes in CompositedLayerMapping.cpp)

The bug mechanism is that the initial values of
m_requiresOwnBackingStoreForIntrinsicReasons and
m_requiresOwnBackingStoreForAncestorReasons will cause a new layer to always
have default compositingState() == PaintsIntoOwnBacking. This tricked
GraphicsLayerUpdater::updateRecursive() into believing that the RenderObject who
created the CLM was painted into its own backing in last frame, while it
actually didn't even have a CLM in last frame.

My new test
fast/scrolling/non-composited-scrolling-repaint-to-ancestor-backing.html
incorrectly show an afterimage of "Lorem ipsum" without the fix.

Personally I believe having initial false for both values is conceptually more
correct, since it reflects the fact that the RenderObject who gained the CLM was
painted to its ancestor's backing in last frame. However I'm not confident that
the fix 100% won't break anything. WDYT?

[abarth-chromium, 2014-08-14 03:54:54]

https://codereview.chromium.org/460883002/diff/80001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/460883002/diff/80001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:165: ,
m_requiresOwnBackingStoreForAncestorReasons(false)
This CL seems fine to me, but we should get vollick's opinion about this part of
the change.

By way of context, we're hoping to eventually remove
HasOwnBackingButPaintsIntoAncestor once some work lands in CC.

[Ian Vollick, 2014-08-14 17:16:44]

https://codereview.chromium.org/460883002/diff/80001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/460883002/diff/80001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:165: ,
m_requiresOwnBackingStoreForAncestorReasons(false)
On 2014/08/14 03:54:54, abarth wrote:
> This CL seems fine to me, but we should get vollick's opinion about this part
of
> the change.
> 
> By way of context, we're hoping to eventually remove
> HasOwnBackingButPaintsIntoAncestor once some work lands in CC.

This seems fine to me. Using dirty bits here feels like overkill.

[abarth-chromium, 2014-08-14 20:45:26]

LGTM

[trchen, 2014-08-14 21:39:03]

The CQ bit was checked by trchen@chromium.org

[commit-bot: I haz the power, 2014-08-14 21:39:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/trchen@chromium.org/460883002/80001

[commit-bot: I haz the power, 2014-08-15 00:14:50]

Committed patchset #5 (80001) as 180309