Bug 193539 – Implement asynchronous frame scrolling for iOS

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Antti Koivisto]

Make iframes scrollable on iOS

[Antti Koivisto]

Created attachment 359389 [details]
wip

[Simon Fraser (smfr)]

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.

[Frédéric Wang (:fredw)]

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.

[Antti Koivisto]

(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?

[Frédéric Wang (:fredw)]

(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 :-)

[Radar WebKit Bug Importer]

<rdar://problem/47379873>

[Antti Koivisto]

Created attachment 359465 [details]
patch

[Antti Koivisto]

Created attachment 359466 [details]
patch

[Antti Koivisto]

Created attachment 359474 [details]
patch

[Antti Koivisto]

Created attachment 359475 [details]
patch

[Antti Koivisto]

Created attachment 359476 [details]
patch

[EWS Watchlist]

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.

[Antti Koivisto]

Created attachment 359478 [details]
patch

[Simon Fraser (smfr)]

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.

[Antti Koivisto]

Created attachment 359513 [details]
patch

[Antti Koivisto]

Created attachment 359514 [details]
patch

[WebKit Commit Bot]

Comment on attachment 359514 [details]
patch

Clearing flags on attachment: 359514

Committed r240162: <https://trac.webkit.org/changeset/240162>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Frédéric Wang (:fredw)]

*** Bug 173833 has been marked as a duplicate of this bug. ***