Bug 122585 – Scrollbars are updated on the main thread rather than the scrolling thread (causing scrollbars not to appear/update quickly in some cases)

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Beth Dakin]

We shouldn't update scrollbars on the main thread, because this leads to out-of-sync issues.

<rdar://problem/10710775>

[Beth Dakin]

Created attachment 213848 [details]
Patch

[WebKit Commit Bot]

Attachment 213848 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/loader/icon/IconDatabase.cpp', u'Source/WebCore/page/FrameView.cpp', u'Source/WebCore/page/FrameView.h', u'Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp', u'Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h', u'Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h', u'Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm', u'Source/WebCore/page/scrolling/mac/ScrollingStateScrollingNodeMac.mm', u'Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.h', u'Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm', u'Source/WebCore/platform/ScrollAnimator.h', u'Source/WebCore/platform/ScrollableArea.cpp', u'Source/WebCore/platform/ScrollableArea.h', u'Source/WebCore/platform/Scrollbar.cpp', u'Source/WebCore/platform/Scrollbar.h', u'Source/WebCore/platform/ScrollbarThemeClient.h', u'Source/WebCore/platform/mac/NSScrollerImpDetails.h', u'Source/WebCore/platform/mac/ScrollAnimatorMac.h', u'Source/WebCore/platform/mac/ScrollAnimatorMac.mm', u'Source/WebCore/platform/mac/ScrollbarThemeMac.h', u'Source/WebCore/platform/mac/ScrollbarThemeMac.mm', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayer.h', u'Source/WebCore/rendering/RenderLayerCompositor.cpp', u'Source/WebCore/rendering/RenderListBox.h', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h']" exit_code: 1
Source/WebCore/loader/icon/IconDatabase.cpp:266:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EFL EWS Bot]

Comment on attachment 213848 [details]
Patch

Attachment 213848 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3617003

[EFL EWS Bot]

Comment on attachment 213848 [details]
Patch

Attachment 213848 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/3619005

[Build Bot]

Comment on attachment 213848 [details]
Patch

Attachment 213848 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/3494115

[Beth Dakin]

Created attachment 213909 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 213909 [details]
Patch

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?

[Beth Dakin]

(In reply to comment #7)
> (From update of attachment 213909 [details])
> 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!
> 

Hehe, fixed!

> > 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.
> 

Oops. Added a void* typedef for other platforms.

> > 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.
> 

Changed name to setScrollbarPaintersFromScrollbarsForNode()

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

Yes :-(

> > Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:339
> > +        [CATransaction lock];
> 
> Do you really need to lock?
> 

So I've been told.

> > Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:351
> > +        [CATransaction begin];
> > +        [CATransaction lock];
> 
> Can you share one transaction for both scrollbars?
> 

Yes. Fixed.

> > 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.
> 

I removed it.

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

Changed to rect.

> > Source/WebCore/platform/mac/ScrollAnimatorMac.mm:501
> > +        macTheme->setCurrentPaintCharacteristics(_scrollbar);
> 
> No idea why setCurrentPaintCharacteristics would take a scrollbar!
> 

I changed the name to setCurrentPaintCharacteristicsForScrollbar()

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

I added a comment.

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

Good call.

> > Source/WebCore/platform/mac/ScrollbarThemeMac.mm:464
> > +void ScrollbarThemeMac::setCurrentPaintCharacteristics(ScrollbarThemeClient* scrollbar)
> 
> I think this needs a better name.
> 

As noted above, I changed it to setCurrentPaintCharacteristicsForScrollbar()

> > 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)?
> 

Heh, good point. I removed this implementation and just added one in the header that returns true.

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

Removed.

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:2702
> > +            m_layerForHorizontalScrollbar->setAnchorPoint(FloatPoint3D());
> 
> Is this still necessary?

Apparently not!

New patch on the way.

[Beth Dakin]

Created attachment 213927 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 213927 [details]
Patch

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

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

Not sure that the "Current" adds anything.

[Beth Dakin]

(In reply to comment #10)
> (From update of attachment 213927 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213927&action=review
> 
> > Source/WebCore/platform/mac/ScrollbarThemeMac.mm:464
> > +void ScrollbarThemeMac::setCurrentPaintCharacteristicsForScrollbar(ScrollbarThemeClient* scrollbar)
> 
> Not sure that the "Current" adds anything.

Will remove.

[Beth Dakin]

Thanks Simon! http://trac.webkit.org/changeset/157253

[Beth Dakin]

Windows build fix: http://trac.webkit.org/changeset/157261