[webkit-reviews] review requested: [Bug 94743] Automatically use composited scrolling : [Attachment 176053] 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 176053: Patch
https://bugs.webkit.org/attachment.cgi?id=176053&action=review

------- Additional Comments from vollick at chromium.org
(In reply to comment #42)
> > Source/WebCore/rendering/RenderLayer.cpp:1727
> > +	 bool automaticallyOptIn =
renderer()->frame()->page()->settings()->acceleratedCompositingForOverflowScrol
lEnabled()
>
> At least frame() and page() can be null here, so better check for that.
I've addded RenderLayer::acceleratedCompositingForOverflowScrollEnabled() since
we need to do this check in several places. I do the required nullity checks
there.
>
> > Source/WebCore/rendering/RenderLayer.cpp:1731
> > +	 return renderer()->style()->useTouchOverflowScrolling() ||
automaticallyOptIn;
>
> Nit: this could be the other way around so there's no need to consult the
style when we've already opted in.
Done.
>
> > Source/WebCore/rendering/RenderLayer.cpp:1901
> > +	 if (compositor()->inCompositingMode() && usesCompositedScrolling())
>
> usesCompositedScrolling() is also called in a few other places to avoid
repaints. Do we need this same inCompositingMode() check there too?
We do, yes. Rather than checking inCompositingMode() everywhere, I've changed
usesCompositedScrolling() to return true if and only iff the layer is actually
using composited scrolling, and I've added needsCompositedScrolling() to
indicate that the layer would prefer to use composited scrolling.
>
> > Source/WebCore/rendering/RenderLayerBacking.cpp:740
> > +	     FloatSize clipSize = paddingBox.size();
>
> Perhaps this should be a separate patch to keep things simpler? Could use a
test too.
Done. wkb.ug/103156

(In reply to comment #43)
> (From update of attachment 175811 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=175811&action=review
>
> As far as I can see, you're only calling canSafelyEstablishAStackingContext()
in usesCompositedScrolling() to determine whether it's ok to do so. What I
don't see is the actually affecting the computed z-order lists. Am I missing
that?
I think this comment may no longer be applicable -- I now compute
m_canSafelyEstablishAStackingContext right after we update our layer lists.
>
> There are other cases where we'd like to be able to treat something like a
stacking context when possible (e.g. something with overflow and composited
descendants), so it would be good if there same code could be reused there. I
think it would also be better if you could run this 'can be a stacking context'
logic at z-order-list building time, which would I think allow the
isDescendant() logic to avoid running up to the root on failure. Then the
z-order lists for the layers would just reflect that this was made a stacking
context.
Done. I think that this approach is also cleaner and more understandable.
Although the code is short, I've included a lengthy comment explaining how/why
it works. It's also efficient. It's linear the number of nodes in the stacking
context -- that is the nodes we visit in collectLayers. We also do this work no
more frequently than we call collectLayers.
>
> Without pixel or ref tests I don't see how your tests show that the 'can be a
stacking context' logic is correct.
I used to have an internals call that I used to ask the layer if it was really
using composited scrolling. I compared this with an array of expected values I
made by stepping through the loop and manually checking if it was safe to
become a stacking context at that iteration. Sami suggested that rather than
using the booleans, I dump the layer trees. So we now either produce no layer
tree (if we didn't become a stacking context and promote the layer), or we get
the resulting layer tree. This lines up with the boolean test I was doing
earlier, but also had the benefit of ensuring that our trees both get generated
and look the way we wanted. There are lots of permutations in this test (30)
and it seemed overkill to put each one in its own test with a visual
expectation.
>
> >> Source/WebCore/ChangeLog:12
> >> +	      antialiasing, so it is important that automatically opting in is
only
> >
> > Did you mean paint order instead of antialiasing?
>
> I presume by antialiasing he really means font smoothing.
Yes, that's right. I was referring to sub-pixel aa.
>
> > Source/WebCore/rendering/RenderLayer.cpp:492
> > +	 layer->compositor()->requestReevaluateCompositingAfterLayout();
>
> I don't like it that things outside of RLC can tell it to do
compositing-related stuff; this is the first instance of it.
>
> Can you move this logic to inside of RLC somehow?
I couldn't see a way, but perhaps I'm missing something. It's now the case that
changing the z-order or normal flow lists can affect our decision to switch in
or out of compositing mode, so I've made a request to reevaluate when these
lists get dirtied.
>
> Rename automaticallyOptIn to alwaysUseCompositedScrolling or something.
I've gone with forceUseCompositedScrolling. Is that ok?
>
> >> Source/WebCore/rendering/RenderLayerBacking.cpp:740
> >> +	      FloatSize clipSize = paddingBox.size();
> >
> > Perhaps this should be a separate patch to keep things simpler? Could use a
test too.
>
> Agreed, please separate this out.
Done.
>
> > Source/WebCore/rendering/RenderLayerCompositor.h:234
> > +	 void requestReevaluateCompositingAfterLayout() {
m_reevaluateCompositingAfterLayout = true; }
>
> I don't like this name. I think it should be
setShouldReevaluateCompositingAfterLayout().
Done.