Bug 57202 – Allow setting composited backing stores for scrollbars and scroll corners

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

Allow setting composited backing stores for scrollbars and scroll corners

[James Robinson]

Created attachment 87097 [details]
Patch

[James Robinson]

This provides the WebCore/platform/ infrastructure for compositing scrollbars/scroll corners and sets the compositing layers for the root ScrollView in the WebKit2 compositor  (which seems like the cleanest compositor backend currently).  I've implemented support for this in the Chromium compositor as well, but that patch is a bit messier so I'll post it separately.  I'm not sure exactly how to do this for the WebKit1/mac compositor, but I think it should be fairly straightforward for someone familiar with the design of that.

I've tested this manually using MiniBrowser and CA_PRINT_TREE=1 and the Chromium equivalent.  I think the best way to make layout tests would be to extend layerTreeAsText() to dump the scrollbar/scroll corner layers and to add pixel tests for scrollbar updates.  It looks like I'd have to extend each platform-specific version of layerTreeAsText() in order to do that so I'd appreciate feedback on the design before starting on that to make sure I'm on the right track.

Side notes: the corner resize widget seems broken (it's just a white square) in MiniBrowser on my Snow Leopard box on all pages that trigger the compositor with or without this patch.  Also I haven't tested this patch with overlay scrollbars but think it should work out OK.

[Build Bot]

Attachment 87097 [details] did not build on win:
Build output: http://queues.webkit.org/results/8272022

[James Robinson]

Created attachment 87098 [details]
add new files to WebCore.vcproj

[Build Bot]

Attachment 87098 [details] did not build on win:
Build output: http://queues.webkit.org/results/8272094

[Simon Fraser (smfr)]

Comment on attachment 87098 [details]
add new files to WebCore.vcproj

View in context: https://bugs.webkit.org/attachment.cgi?id=87098&action=review

This patch feels a bit too heavy on the RefPtrs and classes for everything. It also doesn't do anything for overflow:scroll scrollbars. Did you consider having ScrollableArea be the one to manage the GraphicsLayers for scrollbars, and itself be the GraphicsLayerClient?

> Source/WebCore/platform/ScrollCornerBacking.cpp:63
> +    m_graphicsLayer->setContentsRect(cornerRect);

You shouldn't have to call setContentsRect I think.

> Source/WebCore/platform/ScrollCornerBacking.cpp:64
> +    m_graphicsLayer->setNeedsDisplay();

Is it correct to repaint it every time? I'd expect the contents to not change much.

> Source/WebCore/platform/ScrollCornerBacking.cpp:74
> +    if (m_scrollView) {

An early return would be cleaner here.

> Source/WebCore/platform/ScrollCornerBacking.h:43
> +class ScrollCornerBacking : public GraphicsLayerClient, public RefCounted<ScrollCornerBacking> {

Does it really need to be RefCounted?

> Source/WebCore/platform/ScrollView.h:336
> +    RefPtr<ScrollbarBacking> m_horizontalScrollbarBacking;
> +    RefPtr<ScrollbarBacking> m_verticalScrollbarBacking;
> +    RefPtr<ScrollCornerBacking> m_scrollCornerBacking;

Why do both the ScrollBar and ScrollView need refs to the backing?

> Source/WebCore/platform/ScrollbarBacking.cpp:67
> +        m_graphicsLayer->setPosition(scrollbarRect.location());
> +        m_graphicsLayer->setSize(scrollbarRect.size());
> +        m_graphicsLayer->setContentsRect(scrollbarRect);
> +        m_graphicsLayer->setNeedsDisplay();

Why does invalidate require all this to happen?

[James Robinson]

Comment on attachment 87098 [details]
add new files to WebCore.vcproj

(clearing review flag, this patch isn't ready)

[James Robinson]

(In reply to comment #6)
> (From update of attachment 87098 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87098&action=review
> 
> This patch feels a bit too heavy on the RefPtrs and classes for everything. It also doesn't do anything for overflow:scroll scrollbars. Did you consider having ScrollableArea be the one to manage the GraphicsLayers for scrollbars, and itself be the GraphicsLayerClient?
> 

Thanks for the feedback.  I think that could work as well, although there are a lot more impls of ScrollableArea that would be affected.  I'll also have to make ScrollableArea fully aware of scroll corners but that doesn't seem too bad.

> > Source/WebCore/platform/ScrollCornerBacking.h:43
> > +class ScrollCornerBacking : public GraphicsLayerClient, public RefCounted<ScrollCornerBacking> {
> 
> Does it really need to be RefCounted?
> 
> > Source/WebCore/platform/ScrollView.h:336
> > +    RefPtr<ScrollbarBacking> m_horizontalScrollbarBacking;
> > +    RefPtr<ScrollbarBacking> m_verticalScrollbarBacking;
> > +    RefPtr<ScrollCornerBacking> m_scrollCornerBacking;
> 
> Why do both the ScrollBar and ScrollView need refs to the backing?

I think the ScrollView (or ScrollableArea) should be the logical owner of the backing, but there are a few classes that are not ScrollableAreas but that do hold RefPtr<Scrollbar>s (such as EventHandler and HitTestResult), and it didn't seem obvious that these objects had a shorter lifetime than the ScrollView, so I thought it would be safer if both held a ref to the backing to ensure that the backing always outlived the scrollbar itself.  I'll see if I can simplify this.


I'll move the backing logic up to ScrollableArea, remove the unnecessary invalidations, and post a new patch.

[James Robinson]

New approach:

allow setting GraphicLayers for the 2 scrollbars and corners on ScrollableArea.  ScrollableArea won't own these layers but it will redirect invalidations and manage the size of these layers if they exist

expand RenderLayerCompositor and RenderLayerBacking how to create+own a trio of GraphicsLayers and set them on the ScrollView/RenderLayer when that view/layer becomes composited.  For RenderLayerCompositor the layers will be siblings m_clipLayer and held by a new non-rendering GraphicsLayer.  For RenderLayerBacking the layers will be siblings of the clipping layer(s) if they exist.

This means I don't have to patch any specific compositor impl, since it's all GraphicsLayers.  The main downside is that ScrollableArea will grow by 3 pointers, which means every RenderLayer gets a bit bigger.  If this is a major concern I could add a bitfield to ScrollableArea and do a hashtable lookup, which would cut the memory growth from 3 words down to 1 at the cost of slower lookups.  I think the memory use is not too bad since RenderLayers are already pretty fat (18 pointers, lots of rects + points + ints).

[James Robinson]

Created attachment 87661 [details]
Patch

[James Robinson]

Patch added that works for the main frame and iframes on platforms that don't use platform widgets for FrameViews.  I believe this patch is pretty easy to extend to all RenderLayerBackings that have scrollbars, and plan to start on that ASAP, but this patch seems big enough to be reviewed+landed by itself first.  I've tested this with WebKit1 on mac (where it doesn't do much, since platform widgets are used), WebKit2 on Snow Leopard, and chromium.  I don't have good access to a platform that uses overlay scrollbars but _believe_ everything should work fine for that case as well.

[Build Bot]

Attachment 87661 [details] did not build on win:
Build output: http://queues.webkit.org/results/8311133

[James Robinson]

Created attachment 87677 [details]
fix compile breaks on win, avoid software painting composited scrollbars

[Simon Fraser (smfr)]

Comment on attachment 87677 [details]
fix compile breaks on win, avoid software painting composited scrollbars

View in context: https://bugs.webkit.org/attachment.cgi?id=87677&action=review

> Source/WebCore/ChangeLog:17
> +        FrameView and RenderLayerComposited updated to provide layers for frames that don't
> +        use platform widgets for scrollbars.  The scrollbar/scroll corner layers exist as siblings
> +        of the frame's clip layer as children of a new synthetic (non-rendering) scroll layer.  The
> +        scrollbar/scroll corner layers always exist in the hierarchy, but if there is no
> +        scrollbar/corner then the layers do not render.

I'm concerned about adding new GraphicsLayers for scrollbars on SnowLeopard, where we don't need them for Safari. I think you should add a switch somewhere for platforms to opt in to composited scrollbars.

Otherwise this all looks good.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1445
> +            m_horizontalScrollbarLayer = GraphicsLayer::create(view);

This is a little weird, that you create a GraphicsLayer whose client is another object. It could lead to lifetime issues.

[James Robinson]

(In reply to comment #14)
> (From update of attachment 87677 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87677&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +        FrameView and RenderLayerComposited updated to provide layers for frames that don't
> > +        use platform widgets for scrollbars.  The scrollbar/scroll corner layers exist as siblings
> > +        of the frame's clip layer as children of a new synthetic (non-rendering) scroll layer.  The
> > +        scrollbar/scroll corner layers always exist in the hierarchy, but if there is no
> > +        scrollbar/corner then the layers do not render.
> 
> I'm concerned about adding new GraphicsLayers for scrollbars on SnowLeopard, where we don't need them for Safari. I think you should add a switch somewhere for platforms to opt in to composited scrollbars.

I can check if the FrameView has a platformWidget() before creating the scrollbar+scroll corner layers.  That means that there will still be one extra layer created in Safari on non-WK2 (the structural scroll owner layer).  Does that sound OK?  It'd be possible to get rid of that as well but I'm a little concerned about making the tree structure even more variable.

I don't know if we need an explicit switch yet - we can always add one if some platform doesn't want composited scrollbars.

[James Robinson]

 > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1445
> > +            m_horizontalScrollbarLayer = GraphicsLayer::create(view);
> 
> This is a little weird, that you create a GraphicsLayer whose client is another object. It could lead to lifetime issues.

These GraphicsLayers are owned (in the OwnPtr<> sense) by the RenderLayerCompositor, which is OwnPtr<> owned by the RenderView.  The GraphicsLayerClient is the ScrollableArea, in this case a FrameView.  I believe a FrameView will always outlive its RenderView, so the GraphicsLayerClient should always outlive the GraphicsLayer and thus I think I'm safe here.

A similar situation exists for overflow divs - there I think the GraphicsLayerClient will be the RenderLayer (which implements ScrollableArea) and the GraphicsLayers will live on the RenderLayerBacking.  There the ownership model is more direct - the RenderLayer has an OwnPtr<> to its RenderLayerBacking, so when the GraphicsLayerClient is destroyed the associated GraphicsLayers will be as well.

[Simon Fraser (smfr)]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 87677 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=87677&action=review
> > 
> > > Source/WebCore/ChangeLog:17
> > > +        FrameView and RenderLayerComposited updated to provide layers for frames that don't
> > > +        use platform widgets for scrollbars.  The scrollbar/scroll corner layers exist as siblings
> > > +        of the frame's clip layer as children of a new synthetic (non-rendering) scroll layer.  The
> > > +        scrollbar/scroll corner layers always exist in the hierarchy, but if there is no
> > > +        scrollbar/corner then the layers do not render.
> > 
> > I'm concerned about adding new GraphicsLayers for scrollbars on SnowLeopard, where we don't need them for Safari. I think you should add a switch somewhere for platforms to opt in to composited scrollbars.
> 
> I can check if the FrameView has a platformWidget() before creating the scrollbar+scroll corner layers.  That means that there will still be one extra layer created in Safari on non-WK2 (the structural scroll owner layer).  Does that sound OK?  It'd be possible to get rid of that as well but I'm a little concerned about making the tree structure even more variable.

We already have a pretty variable tree structure with clipping layers and such, so I think you should avoid useless layers on platforms that don't need them.

[James Robinson]

(In reply to comment #17)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > (From update of attachment 87677 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=87677&action=review
> > > 
> > > > Source/WebCore/ChangeLog:17
> > > > +        FrameView and RenderLayerComposited updated to provide layers for frames that don't
> > > > +        use platform widgets for scrollbars.  The scrollbar/scroll corner layers exist as siblings
> > > > +        of the frame's clip layer as children of a new synthetic (non-rendering) scroll layer.  The
> > > > +        scrollbar/scroll corner layers always exist in the hierarchy, but if there is no
> > > > +        scrollbar/corner then the layers do not render.
> > > 
> > > I'm concerned about adding new GraphicsLayers for scrollbars on SnowLeopard, where we don't need them for Safari. I think you should add a switch somewhere for platforms to opt in to composited scrollbars.
> > 
> > I can check if the FrameView has a platformWidget() before creating the scrollbar+scroll corner layers.  That means that there will still be one extra layer created in Safari on non-WK2 (the structural scroll owner layer).  Does that sound OK?  It'd be possible to get rid of that as well but I'm a little concerned about making the tree structure even more variable.
> 
> We already have a pretty variable tree structure with clipping layers and such, so I think you should avoid useless layers on platforms that don't need them.

You make me work so hard :).  This is achievable, I'll upload a new patch shortly.

[James Robinson]

Created attachment 87824 [details]
Patch

[James Robinson]

Changes since the last patch:

- updated RLC::frameViewDidChangeSize() to update either the scroll layer owner or the clip layer, depending on which is there
- updated RLC::rootPlatformLayer() to return either the scroll layer owner, clip layer, or root platform layer
- updated RLC::ensureRootPlatformLayer() to only create the scroll layer owner + scrollbar + scroll corner layers when the view is not using a platform widget
- updated lots of layerTreeAsText test expectations
- merged up to ToT

Mac non-WK2 produces the same graphics layer tree as it does without this patch now.

I'd also like to state for the record that mac-wk2 test expectations are a real pain to deal with.

Would you mind taking a look at the layer management changes in RenderLayerCompositor, Simon?  I imagine I'll be doing a similar dance in RenderLayerBacking to make sure the scroll layers are siblings of the interior clipping layer.

[James Robinson]

Created attachment 88101 [details]
only create layers for overlay scrollbars on mac

[Simon Fraser (smfr)]

Comment on attachment 88101 [details]
only create layers for overlay scrollbars on mac

View in context: https://bugs.webkit.org/attachment.cgi?id=88101&action=review

> LayoutTests/compositing/iframes/overlapped-nested-iframes-expected.txt:17
>                  (GraphicsLayer

Why does this test gain scrollbar layers? Or is the plan to add a Mac-specific result which doesn't have them?

> Source/WebCore/ChangeLog:9
> +        scroll corner. ScrollableArea can position the layers and route invalidations+paint calls to

Just say "invalidate and paint"

> Source/WebCore/ChangeLog:14
> +        FrameView and RenderLayerComposited updated to provide layers for frames that don't
> +        use platform widgets for scrollbars.  The scrollbar/scroll corner layers exist as siblings

I think this text is out of date now.

> Source/WebCore/ChangeLog:23
> +        This code should be pretty easy to extend to all compositing layers that have scrollbars by
> +        extending RenderLayerBacking in a similar way to RenderLayerCompositors, taking care to put
> +        the scrollbar layers in the correct place in the hierarchy and updating all the clips
> +        appropriately.  To keep this patch simple and cut down on the amount of layout test result
> +        churn I'd rather do that bit as a separate patch.

I don't think you need to say this in the changelog.

> Source/WebCore/page/FrameView.h:322
> +    virtual GraphicsLayer* horizontalScrollbarLayer() const;
> +    virtual GraphicsLayer* verticalScrollbarLayer() const;
> +    virtual GraphicsLayer* scrollCornerLayer() const;

Maybe slightly more readable as "layerFor..."

> Source/WebCore/platform/ScrollView.h:300
> +    virtual bool scrollCornerPresent() const;

Normally we use names like isScrollCornerPresent(), or isScrollCornerVisible()?

> Source/WebCore/platform/ScrollableArea.cpp:182
> +    graphicsLayer->setNeedsDisplay();

It seems bad to cause the layer to paint every time this method is called, even if the scrollbarRect doesn't change.

> Source/WebCore/platform/ScrollableArea.cpp:192
> +    graphicsLayer->setNeedsDisplay();

Ditto

> Source/WebCore/platform/ScrollableArea.cpp:276
> +static void paintScrollbar(Scrollbar* scrollbar, GraphicsContext& context, const IntRect& inClip)

inClip is old-skool

> Source/WebCore/platform/ScrollableArea.h:141
> +    virtual bool showDebugBorders() const { return false; }
> +    virtual bool showRepaintCounter() const { return false; }

It would be nice to wire these up so that we can see layer borders for scrollbars, which will tell us visually whether scrollbars are in layers.

> Source/WebCore/rendering/RenderLayer.cpp:1833
> +}

This is worth a comment saying that overflow:scroll areas never have a scroll corner.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1464
> +                m_horizontalScrollbarLayer->setName("iframe horizontal scrollbar");

Just for iframes? I'd say "horizontal scrollbar layer"

[James Robinson]

(In reply to comment #22)
> (From update of attachment 88101 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88101&action=review
> 
> > LayoutTests/compositing/iframes/overlapped-nested-iframes-expected.txt:17
> >                  (GraphicsLayer
> 
> Why does this test gain scrollbar layers? Or is the plan to add a Mac-specific result which doesn't have them?

Oops, the test is Skipped on mac-snowleopard which confused my rebaseline merge script.  I'll just remove this expectation.  The graphics layer tree doesn't change on Mac and won't until overlay scrollbars are flipped on by default in DRT on some mac port.

[James Robinson]

I hate to keep changing this patch after you've reviewed it but I think I need to move the positioning code down to ScrollView - I can't reuse it for other ScrollableArea subclasses.  The issue is that in order to position the layer we need to set the offset for the (for example) layerForHorizontalScrollbar relative to its parent layer.  The scrollbar's frameRect is its position relative to its containing Widget.  For ScrollViews, which themselves are Widgets, the layerForHorizontalScrollbar's parent is a GraphicsLayer at offset 0,0 from the Widget so the frameRect's offset is the same as the layerForHorizontalScrollbar's offset should be.  For RenderLayers, the layer itself is offset as some position relative to its containing Widget so the positioning code will have to take that offset into account.  I think the cleanest way to handle this is to move the positioning logic down into ScrollableArea subclasses so each can do the right thing.

[Simon Fraser (smfr)]

Can you just not fix RenderLayer scrolling right now? I may have some changes coming in that area.

[James Robinson]

Created attachment 89146 [details]
wip, reflections off

[James Robinson]

Updated patch to include RenderLayers as well.  This patch isn't quite ready (hence no review?) but hopefully it's useful to look at for whatever work you are planning on doing.

Changes:
- ScrollableArea is no longer a GraphicsLayerClient. RenderLayerCompositor and RenderLayerBacking are the clients for overflow control layers for FrameViews and RenderLayers, respectively.  I couldn't share much logic between the two cases and this makes the object lifetime cleaner
- RenderLayers that have backings now have layers for overflow controls

Unresolved issues:
- compositing/reflections/nested-reflection-transformed.html and compositing/reflections/nested-reflection-transformed2.html fail in pixel mode only.  The reflection is at slightly the wrong offset - I've fudged something up in RenderLayerBacking::updateGraphicsLayerGeometry.  If I comment out line 389 (resetting graphicsLayerParentLocation in the case where there's a scroll layer) then these tests render correctly but a bunch of other tests fail.  I'm not quite sure what the issue is here yet.

- platform/chromium/compositing/backface-visibility-transformed.html fails - it just draws a red rectangle instead of a green skewed rectangle.  Still investigating.

Any feedback on the new RenderLayer / RenderLayerBacking stuff would be greatly appreciated.  Handling scrollbars associated with RenderLayers that do not have their own backings will be more difficult since it'll be hard to get the scrollbar in the correct place in the z-ordering of the page.  It should be possible eventually.

[James Robinson]

Oh, and I haven't updated the ChangeLogs either.

[James Robinson]

Created attachment 89331 [details]
Patch

[James Robinson]

Vangelis helped me simplify this a good amount, which resolved the other issues I was having.

This patch creates layers for FrameViews and RenderLayers that have backing only when the overflow controls exist and doesn't add any extra structural layers in any case.  The comments in RenderLayerCompositor and RenderLayerBacking + the ChangeLog explain the layer tree changes.

One significant change to RenderLayerCompositor is that its m_clipLayer's size is now set to the FrameView's visibleContentRect including scrollbars since the overflow controls are children of the m_clipLayer.  For non-overlay scrollbars this is fine since the overlay controls display on top of the frame's content.  For overlay scrollbars this is not a change since (as I understand it) the layout width == the visible content rect.  I can't verify the overlay scrollbar changes manually or construct test cases that run in that mode, so verification of my assumptions would be appreciated.

Overall I feel that this is a bit cleaner.  I also took this opportunity to clean up some of the redundant compositing/ test expectations.

[Simon Fraser (smfr)]

Comment on attachment 89331 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=89331&action=review

r=me but please fix the clipLayer geometry issue. You could add a testcase or two where composited iframe contents potentially overlap the iframe scrollbars.

> Source/WebCore/rendering/RenderLayer.cpp:3631
> +}
> +GraphicsLayer* RenderLayer::layerForScrollCorner() const

Missing blank line.

> Source/WebCore/rendering/RenderLayerBacking.h:159
> +    bool requiresHorizontalScrollbarLayer();
> +    bool requiresVerticalScrollbarLayer();
> +    bool requiresScrollCornerLayer();

These should be const.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1144
> -        m_clipLayer->setSize(FloatSize(frameView->layoutWidth(), frameView->layoutHeight()));
> +        m_clipLayer->setSize(frameView->visibleContentRect(true /* include scrollbars */).size());

This is wrong for non-overlay scrollbars. The clipping layer has to exclude the scrollbar, otherwise composited content inside the iframe can overlap those scrollbars.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1469
> +    if (view->platformWidget())
> +        return false;
> +#if PLATFORM(MAC)
> +    if (!view->hasOverlayScrollbars())
> +        return false;
> +#endif

Please wrap this up in a helper function, rather than repeating it 3 times.

[James Robinson]

Thanks Simon.  I'll create some more test cases for iframes+scrollbars.  Can you describe a case that you think would break with the clip I set? All non-overlay scroll bars are opaque and the overflow controls layers render on top so the simpler cases I tried render correctly.

[Simon Fraser (smfr)]

Take LayoutTests/compositing/iframes/composited-parent-iframe.html, and edit resources/composited-subframe.html to give the blue box a -webkit-transform: translate3d(60px, 0, 0).

[James Robinson]

Created attachment 89538 [details]
Patch

[James Robinson]

Feedback addressed and a set of new tests added.  I've changed the clip rect for RenderLayerCompositor back to what it was before (the rectangle excluding scrollbars) and in the process discovered bug 58515, and added a new layer to be a parent of both the clip and overflow controls layers.  In the process I noticed a position bug with FrameViews and broke up the frameViewDidChangeSize(offset) notification into frameViewDidChangeSize() and frameViewDidChangeLocation(offset).

Tests added to verify that the graphics layer tree is correct when scrollbars are dynamically added/removed from the page and that the proper repaints happen when scrollbars are removed.  We have decent pixel coverage for the normal rendering of scrollbars with, for example, http://trac.webkit.org/browser/trunk/LayoutTests/compositing/overflow/scrollbar-painting.html.

[James Robinson]

Landed http://trac.webkit.org/changeset/83820

[Andrey Kosyakov]

Comment on attachment 89538 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=89538&action=review

> Source/WebCore/platform/ScrollableArea.h:85
> +    virtual void paintScrollCorner(GraphicsContext*, const IntRect&) { }

This broke chromium clang build, as this method is shadowed by RenderLayer::paintScrollCorner() with a different signature.
It does nothing and I failed to find any calls of it, so I'm removing it to fix the build.