Bug 193539 - Implement asynchronous frame scrolling for iOS
Summary: Implement asynchronous frame scrolling for iOS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 173833 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-01-17 10:43 PST by Antti Koivisto
Modified: 2019-01-22 00:52 PST (History)
8 users (show)

See Also:


Attachments
wip (21.67 KB, patch)
2019-01-17 10:44 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (28.43 KB, patch)
2019-01-18 02:28 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (28.92 KB, patch)
2019-01-18 03:10 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (39.79 KB, patch)
2019-01-18 05:01 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (40.47 KB, patch)
2019-01-18 05:03 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (40.42 KB, patch)
2019-01-18 05:05 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (40.66 KB, patch)
2019-01-18 05:11 PST, Antti Koivisto
simon.fraser: review+
Details | Formatted Diff | Diff
patch (40.67 KB, patch)
2019-01-18 11:11 PST, Antti Koivisto
koivisto: commit-queue+
Details | Formatted Diff | Diff
patch (39.77 KB, patch)
2019-01-18 11:23 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2019-01-17 10:43:51 PST
Make iframes scrollable on iOS
Comment 1 Antti Koivisto 2019-01-17 10:44:23 PST
Created attachment 359389 [details]
wip
Comment 2 Simon Fraser (smfr) 2019-01-17 10:51:53 PST
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.
Comment 3 Frédéric Wang (:fredw) 2019-01-17 11:12:23 PST
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.
Comment 4 Antti Koivisto 2019-01-18 01:27:51 PST
(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?
Comment 5 Frédéric Wang (:fredw) 2019-01-18 01:33:46 PST
(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 :-)
Comment 6 Radar WebKit Bug Importer 2019-01-18 02:03:46 PST
<rdar://problem/47379873>
Comment 7 Antti Koivisto 2019-01-18 02:28:16 PST
Created attachment 359465 [details]
patch
Comment 8 Antti Koivisto 2019-01-18 03:10:27 PST
Created attachment 359466 [details]
patch
Comment 9 Antti Koivisto 2019-01-18 05:01:31 PST
Created attachment 359474 [details]
patch
Comment 10 Antti Koivisto 2019-01-18 05:03:22 PST
Created attachment 359475 [details]
patch
Comment 11 Antti Koivisto 2019-01-18 05:05:28 PST
Created attachment 359476 [details]
patch
Comment 12 Build Bot 2019-01-18 05:07:44 PST
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.
Comment 13 Antti Koivisto 2019-01-18 05:11:17 PST
Created attachment 359478 [details]
patch
Comment 14 Simon Fraser (smfr) 2019-01-18 10:59:33 PST
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.
Comment 15 Antti Koivisto 2019-01-18 11:11:17 PST
Created attachment 359513 [details]
patch
Comment 16 Antti Koivisto 2019-01-18 11:23:48 PST
Created attachment 359514 [details]
patch
Comment 17 WebKit Commit Bot 2019-01-18 12:07:50 PST
Comment on attachment 359514 [details]
patch

Clearing flags on attachment: 359514

Committed r240162: <https://trac.webkit.org/changeset/240162>
Comment 18 WebKit Commit Bot 2019-01-18 12:07:53 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Frédéric Wang (:fredw) 2019-01-22 00:52:19 PST
*** Bug 173833 has been marked as a duplicate of this bug. ***