Summary: | Implement asynchronous frame scrolling for iOS | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||||||||||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | bfulgham, commit-queue, ews-watchlist, fred.wang, ggaren, simon.fraser, webkit-bug-importer, zalan | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Attachments: |
|
Description
Antti Koivisto
2019-01-17 10:43:51 PST
Created attachment 359389 [details]
wip
Comment on attachment 359389 [details] wip View in context: https://bugs.webkit.org/attachment.cgi?id=359389&action=review > Source/WebCore/rendering/RenderLayerCompositor.cpp:3421 > + m_clipLayer = m_scrollLayer; Hmm, this seems like it could lead to programming mistakes. I don't think any other code considers the fact that these could be the same layer. Not sure how different this bug is, but FYI I had patches in bug 173833, bug 192303, bug 182868 with various tests that were pending review. (In reply to Frédéric Wang (:fredw) from comment #3) > Not sure how different this bug is, but FYI I had patches in bug 173833, bug > 192303, bug 182868 with various tests that were pending review. Interesting! I wasn't aware of that patch. Looks very similar. The main differences seem to be - this patch ties to asyncFrameScrollingEnabled internal setting and doesn't enable the feature by default - eliminates the unnecessary clip layer in async scrolling case (UIScrollView clips). - uses the same ScrolliongTreeFrameScrollingNode type for both main frame and subframe, switching operation based on the scroll layer type (so the setting also works) - is missing some of the bug fixes - doesn't add tests I propose we land this version as a starting point that can be experimented with and then work towards integrating your tests and bug fixes. How does that sound? (In reply to Antti Koivisto from comment #4) > (In reply to Frédéric Wang (:fredw) from comment #3) > > Not sure how different this bug is, but FYI I had patches in bug 173833, bug > > 192303, bug 182868 with various tests that were pending review. > > Interesting! I wasn't aware of that patch. Looks very similar. The main > differences seem to be > > - this patch ties to asyncFrameScrollingEnabled internal setting and doesn't > enable the feature by default > - eliminates the unnecessary clip layer in async scrolling case > (UIScrollView clips). > - uses the same ScrolliongTreeFrameScrollingNode type for both main frame > and subframe, switching operation based on the scroll layer type (so the > setting also works) > - is missing some of the bug fixes > - doesn't add tests > > I propose we land this version as a starting point that can be experimented > with and then work towards integrating your tests and bug fixes. How does > that sound? OK, that sounds good to me. I think the two last points are important to have at some point :-) Created attachment 359465 [details]
patch
Created attachment 359466 [details]
patch
Created attachment 359474 [details]
patch
Created attachment 359475 [details]
patch
Created attachment 359476 [details]
patch
Attachment 359476 [details] did not pass style-queue:
ERROR: Source/WebCore/page/scrolling/ios/ScrollingTreeFrameScrollingNodeIOS.h:64: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4]
Total errors found: 1 in 14 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 359478 [details]
patch
Comment on attachment 359478 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=359478&action=review > Source/WebCore/rendering/RenderLayerCompositor.cpp:3415 > +#if PLATFORM(IOS) if PLATFORM(IOS_FAMILY) Ideally this wouldn't be a platform #ifdef, but something else. > LayoutTests/platform/ios-wk2/compositing/tiling/tiled-drawing-async-frame-scrolling-expected.txt:46 > - (coverage rect 0.00, 0.00 300.00 x 150.00) > + (coverage rect -10.00, -10.00 800.00 x 600.00) This seems odd. Created attachment 359513 [details]
patch
Created attachment 359514 [details]
patch
Comment on attachment 359514 [details] patch Clearing flags on attachment: 359514 Committed r240162: <https://trac.webkit.org/changeset/240162> All reviewed patches have been landed. Closing bug. *** Bug 173833 has been marked as a duplicate of this bug. *** |