Summary: | ScrollingStateScrollingNode::ChangedProperty::NumScrollingStateNodeBits is wrongly set | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Frédéric Wang (:fredw) <fred.wang> | ||||||||||||||||
Component: | WebCore Misc. | Assignee: | Frédéric Wang (:fredw) <fred.wang> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | annulen, buildbot, cmarcelo, jamesr, luiz, rniwa, simon.fraser, tonikitoo, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | Other | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 133022, 135769, 135994, 144482, 145318 | ||||||||||||||||||
Bug Blocks: | 171667 | ||||||||||||||||||
Attachments: |
|
Description
Frédéric Wang (:fredw)
2017-05-19 01:04:53 PDT
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> |