ScrollingStateScrollingNode::ChangedProperty::NumScrollingStateNodeBits was introduced in bug 133022 so that ScrollingStateFrameScrollingNode and ScrollingStateOverflowScrollingNode know the number of bits use for properties in their parent class. In bug 135769, bug 135994, bug 145318 and bug 144482, new properties were added to ScrollingStateScrollingNode but NumScrollingStateNodeBits was not increased accordingly. This means that there are potential conflicts between these new properties and those of derived classes ScrollingStateFrameScrollingNode and ScrollingStateOverflowScrollingNode. Not sure if we have some concrete use case demonstrating the issue, though.
Created attachment 310632 [details] Patch
Comment on attachment 310632 [details] Patch Attachment 310632 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3775286 New failing tests: fast/css/sticky/sticky-margins.html fast/css/sticky/sticky-writing-mode-vertical-rl.html media/W3C/video/currentSrc/currentSrc_nonempty_after_setting_src.html tiled-drawing/scrolling/fixed-background/fixed-background-composited.html fast/css/sticky/sticky-top-margins.html fast/css/sticky/sticky-left.html fast/css/sticky/sticky-as-positioning-container.html fast/css/sticky/sticky-table-row-top.html fast/css/sticky/sticky-left-percentage.html tiled-drawing/scrolling/fixed-background/fixed-background-negative-z-index-fixed.html fast/scrolling/rtl-scrollbars-sticky-document-2.html fast/css/sticky/sticky-table-thead-top.html fast/css/sticky/sticky-top.html fast/css/sticky/sticky-side-margins.html fast/css/sticky/inflow-sticky.html fast/css/sticky/sticky-writing-mode-vertical-lr.html fast/css/sticky/sticky-writing-mode-horizontal-bt.html fast/repaint/fixed-scale.html fast/repaint/fixed.html fast/css/sticky/replaced-sticky.html
Created attachment 310638 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 310632 [details] Patch Attachment 310632 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3775307 New failing tests: scrollingcoordinator/ios/ui-scrolling-tree.html
Created attachment 310640 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 310655 [details] Patch
Comment on attachment 310655 [details] Patch Attachment 310655 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3776868 New failing tests: fast/css/sticky/sticky-margins.html fast/css/sticky/sticky-writing-mode-vertical-lr.html media/W3C/video/currentSrc/currentSrc_nonempty_after_setting_src.html tiled-drawing/scrolling/fixed-background/fixed-background-composited.html fast/css/sticky/sticky-top-margins.html fast/css/sticky/sticky-left.html fast/css/sticky/sticky-as-positioning-container.html fast/css/sticky/sticky-table-row-top.html fast/css/sticky/sticky-left-percentage.html tiled-drawing/scrolling/fixed-background/fixed-background-negative-z-index-fixed.html fast/scrolling/rtl-scrollbars-sticky-document-2.html fast/css/sticky/sticky-table-thead-top.html fast/css/sticky/sticky-top.html fast/css/sticky/inflow-sticky.html fast/css/sticky/sticky-writing-mode-vertical-rl.html fast/css/sticky/sticky-writing-mode-horizontal-bt.html fast/repaint/fixed.html fast/repaint/fixed-scale.html fast/css/sticky/sticky-side-margins.html fast/css/sticky/replaced-sticky.html
Created attachment 310660 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 310655 [details] Patch Attachment 310655 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3776989 New failing tests: scrollingcoordinator/ios/ui-scrolling-tree.html
Created attachment 310663 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 310655 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310655&action=review Nice find! > Source/WebCore/page/scrolling/ScrollingStateNode.h:208 > + typedef uint64_t ChangedProperties; There's no actual need to do this change now, right? We never have more than 32 bits.
(In reply to Simon Fraser (smfr) from comment #11) > Comment on attachment 310655 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=310655&action=review > > Nice find! > > > Source/WebCore/page/scrolling/ScrollingStateNode.h:208 > > + typedef uint64_t ChangedProperties; > > There's no actual need to do this change now, right? We never have more than > 32 bits. Mmh, I think we do (if we sum up the properties of ScrollingNode + FrameScrollingNode) and that change seemed to fix the failures in my first patch when I tested locally... Anyway, will go back to this next week.
Created attachment 310814 [details] Patch
Comment on attachment 310814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310814&action=review > Source/WebCore/page/scrolling/ScrollingStateNode.cpp:71 > + m_changedProperties |= (static_cast<ChangedProperties>(1) << propertyBit); It may be cleaner to use BitVector or FastBitVector for m_changedProperties, or, if it's not acceptable performance-wise, introduce non-resizable BitVector-like template that uses given type (e.g. uint64_t) for storage
(In reply to Konstantin Tokarev from comment #14) > It may be cleaner to use BitVector or FastBitVector for m_changedProperties, > or, if it's not acceptable performance-wise, introduce non-resizable > BitVector-like template that uses given type (e.g. uint64_t) for storage Thank you, indeed the current setup is a bit fragile, this patch was just intended to fix the obvious error without modifying too much the code. @Simon: What do you think? Is it worth following that suggestion now? Is there any performance concern?
Comment on attachment 310814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310814&action=review >> Source/WebCore/page/scrolling/ScrollingStateNode.cpp:71 >> + m_changedProperties |= (static_cast<ChangedProperties>(1) << propertyBit); > > It may be cleaner to use BitVector or FastBitVector for m_changedProperties, or, if it's not acceptable performance-wise, introduce non-resizable BitVector-like template that uses given type (e.g. uint64_t) for storage So BitVector/FastBitVector seems a bit overkill since we just use a bit more than 32bits for now and they don't have a fast helper to check ScrollingStateNode::hasChangedProperties.
Comment on attachment 310814 [details] Patch Let's land this now and move to BitVector or something in another patch.
Committed r217610: <http://trac.webkit.org/changeset/217610>
<rdar://problem/32489809>