We shouldn't update scrollbars on the main thread, because this leads to out-of-sync issues. <rdar://problem/10710775>
Created attachment 213848 [details] Patch
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.
Comment on attachment 213848 [details] Patch Attachment 213848 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/3617003
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
Comment on attachment 213848 [details] Patch Attachment 213848 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/3494115
Created attachment 213909 [details] Patch
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?
(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.
Created attachment 213927 [details] Patch
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.
(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.
Thanks Simon! http://trac.webkit.org/changeset/157253
Windows build fix: http://trac.webkit.org/changeset/157261