[webkit-reviews] review granted: [Bug 57202] Allow setting composited backing stores for scrollbars and scroll corners : [Attachment 88101] only create layers for overlay scrollbars on mac

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted James Robinson
<jamesr at chromium.org>'s request for review:
Bug 57202: Allow setting composited backing stores for scrollbars and scroll
corners
https://bugs.webkit.org/show_bug.cgi?id=57202

Attachment 88101: only create layers for overlay scrollbars on mac
https://bugs.webkit.org/attachment.cgi?id=88101&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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"