[webkit-reviews] review requested: [Bug 94743] Automatically use composited scrolling : [Attachment 163925] Patch

[bugzilla-daemon at webkit.org]

vollick at chromium.org has asked	for review:
Bug 94743: Automatically use composited scrolling
https://bugs.webkit.org/show_bug.cgi?id=94743

Attachment 163925: Patch
https://bugs.webkit.org/attachment.cgi?id=163925&action=review

------- Additional Comments from vollick at chromium.org
(In reply to comment #18)
> (In reply to comment #16)
> > Created an attachment (id=163913) [details] [details]
> > Patch
> >
> > > > Source/WebCore/rendering/RenderLayer.cpp:769
> > > > +    double zMin = std::numeric_limits<double>::max();
> > > > +    double zMax = -std::numeric_limits<double>::max();
> > > > +    findMinAndMaxZIndices(this, zMin, zMax);
> > > > +
> > > > +    if (zMin >= zMax)
> > > > +	 return true;
> > > > +
> > > > +    return
!nonDescendantInThisStackingContextCouldFallBetweenDescendants(stackingContext(
), this, zMin, zMax);
> > >
> > > This code seems very hard to understand, and I can't easily tell if it's
correct.
> > >
> > > What you're really asking is whether, by making a layer a stacking
context, you affect the traversal order of the z-order tree. It seems like it
would be easier to determine that from the z-order tree, rather than crawling
around looking at zMin and zMax.
> >
> > This is true. The current patch walks the z-order tree (and I agree that
this is much easier to understand), but there's a down side: the z-order lists
are usually dirty.
> > I was expecting this to happen occasionally, and that they'd sort
themselves out eventually, but they seem to stay dirty.
>
> They are not dirty during compositing evaluation and painting. I'm not sure
what makes you think they stay dirty.

It seems that this new code gets called at a point where these lists are dirty.
I keep checking m_zOrderListsDirty, and it seems to continue to be true.

>
> > Ideally, we could cache the lists when we rebuild them and clear the dirty
bits, but ::usesCompositedScrolling
> > gets called in a deep stack of const functions, so the options I see are
> >
> >   1) Recompute the lists each time they are needed.
> >   2) Remove a bunch of const's.
> >   3) Make the lists mutable.
> >
> > None of these options seem very nice to me. Do you have a suggestion for
how to proceed?
>
> Can you build enough of a z-order subtree to make your determination? You'd
have to back up to the stacking context ancestor, build temporary z-order lists
with and without your layer being a stacking context, and compare them.
>
> Maybe there is a way to do this without actually building lists, but perhaps
it can share logic with the list building.

Yep, my patch should not build the whole z-order tree, but only those lists for
the nodes in the tree that matter.
And I have refactored the list building code so that it is reused by the new
code. Perhaps this approach is ok, then?