[webkit-reviews] review denied: [Bug 122585] Scrollbars are updated on the main thread rather than the scrolling thread (causing scroll bars not to appear/update quickly in some cases) : [Attachment 213909] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Beth Dakin
<bdakin at apple.com>'s request for review:
Bug 122585: Scrollbars are updated on the main thread rather than the scrolling
thread (causing scroll bars not to appear/update quickly in some cases)
https://bugs.webkit.org/show_bug.cgi?id=122585

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=213909&action=review


r- for use of ScrollbarPainter in C++, and for the badly named function

> Source/WebCore/ChangeLog:17
> +	   ScrolllableArea function called
updatesScrollLayerPositionOnMainThread()

ScrolllableArea!

> Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h:143
> +    ScrollbarPainter horizontalScrollbarPainter() const { return
m_horizontalScrollbarPainter; }

Isn't ScrollbarPainter an Objective-C type? You're exposing it in a C++ header
here.

> Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h:126
> +    void setScrollbarPaintersForNode(Scrollbar* verticalScrollbar,
Scrollbar* horizontalScrollbar, ScrollingStateScrollingNode*);

Odd that this is called setScrollbarPainters, but what you're passing in are
scrollbars.

> Source/WebCore/page/scrolling/mac/ScrollingStateScrollingNodeMac.mm:112
> +	   m_scrollingStateTree->setHasChangedProperties(true);

Is that true needed?

> Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:339
> +	   [CATransaction lock];

Do you really need to lock?

> Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:351
> +	   [CATransaction begin];
> +	   [CATransaction lock];

Can you share one transaction for both scrollbars?

> Source/WebCore/platform/ScrollableArea.h:177
> +    // Computes the so-called double value for the scrollbar's current
position and the current overhang amount.

Not sure that "so-called double value" adds any clarity.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:464
> +    return aRect;

aRect :(

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:501
> +	   macTheme->setCurrentPaintCharacteristics(_scrollbar);

No idea why setCurrentPaintCharacteristics would take a scrollbar!

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:534
> +    if (newKnobAlpha == 0)
> +	   [scrollerPainter setUsePresentationValue:NO];

This is a bit mysterious.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:1000
> +    if (layer)
> +	   [painter setLayer:layer->platformLayer()];
> +    else
> +	   [painter setLayer:nil];

[painter setLayer:layer ? layer->platformLayer() : nil] ?

> Source/WebCore/platform/mac/ScrollbarThemeMac.mm:464
> +void ScrollbarThemeMac::setCurrentPaintCharacteristics(ScrollbarThemeClient*
scrollbar)

I think this needs a better name.

> Source/WebCore/rendering/RenderLayer.cpp:1411
> +bool RenderLayer::updatesScrollLayerPositionOnMainThread() const
> +{
> +    if (Page* page = renderer().frame().page()) {
> +	   if (ScrollingCoordinator* scrollingCoordinator =
page->scrollingCoordinator())
> +	       return
scrollingCoordinator->shouldUpdateScrollLayerPositionOnMainThread();
> +    }
> +
> +    return true;
> +}

Hm, wouldn't we always update overflow scrollbars on the main thread
(currently)?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2701
> +	       m_layerForHorizontalScrollbar->setDrawsContent(false);

drawsContent is false by default.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2702
> +	       m_layerForHorizontalScrollbar->setAnchorPoint(FloatPoint3D());

Is this still necessary?