Bug 204936 – Smooth-scroll animations should run in the UI process on iOS

• Status: RESOLVED
• Resolution: FIXED
• Component: Scrolling

[cathiechen]

This is the second implement part of scroll behavior in bug 188043.
This patch contains the implement of scroll behavior that relies on native scroll interfaces on iOS platform.
On other platforms, the behavior isn't changed. I.g. The smooth scroll will be performed by an instant scroll instead.

[cathiechen]

Created attachment 384994 [details]
Patch

[cathiechen]

Created attachment 385146 [details]
Patch

[cathiechen]

Created attachment 385155 [details]
UI-scroll-animation-for-review

This patch only contains change for UI scroll.

[cathiechen]

Created attachment 385361 [details]
native-animation-for-ews(including parsing + non_native_animation)

[cathiechen]

Created attachment 385362 [details]
native-animation-for-review

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

Comment on attachment 385362 [details]
native-animation-for-review

View in context: https://bugs.webkit.org/attachment.cgi?id=385362&action=review

> LayoutTests/ChangeLog:12
> +        copy scroll-behavior test to fast/scrolling/ios/

why not move them into fast/scrolling/ios/cssom-view?

> LayoutTests/ChangeLog:14
> +        * fast/scrolling/ios/resources/scroll-behavior.js: Added.

I think this is copied from  LayoutTests/imported/w3c/web-platform-tests/css/cssom-view/support/scroll-behavior.js ; any reason why you decided to move it to the shared ios/resources/ ?

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:255
> +    // The scroll position of animated scrolling will be set from the the UI process. So it shouldn't be set now.

the the => by the?

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:-266
> -

unneeded whitespace change

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:226
> +    }

This is a bit weird, why not

#if PLATFORM(IOS_FAMILY)
    if (requestScrollWithAnimation())
       return;
#endif

?

> Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp:215
> +#endif

Do we really need to add IOS conditionals for this and other APIs?

I think we can add the instantScrollDidStopAnimation parameter on all platforms with a default value and ignore it on non-iOS ones.

[cathiechen]

Comment on attachment 385362 [details]
native-animation-for-review

View in context: https://bugs.webkit.org/attachment.cgi?id=385362&action=review

Hi Fred,
Thanks for the review:)

>> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:255
>> +    // The scroll position of animated scrolling will be set from the the UI process. So it shouldn't be set now.
> 
> the the => by the?

Done

>> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:-266
>> -
> 
> unneeded whitespace change

done

>> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:226
>> +    }
> 
> This is a bit weird, why not
> 
> #if PLATFORM(IOS_FAMILY)
>     if (requestScrollWithAnimation())
>        return;
> #endif
> 
> ?

Done

>> Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp:215
>> +#endif
> 
> Do we really need to add IOS conditionals for this and other APIs?
> 
> I think we can add the instantScrollDidStopAnimation parameter on all platforms with a default value and ignore it on non-iOS ones.

done

[cathiechen]

Created attachment 385374 [details]
native-animation-for-ews(including parsing + non_native_animation)

[cathiechen]

Created attachment 385375 [details]
native-animation-for-review

[cathiechen]

Created attachment 385388 [details]
native-animation-for-ews(including parsing + non_native_animation)

[cathiechen]

Created attachment 385389 [details]
native-animation-for-review

[cathiechen]

Created attachment 385399 [details]
native-animation-for-ews(including parsing + non_native_animation)

[cathiechen]

Created attachment 385400 [details]
native-animation-for-review

[cathiechen]

Created attachment 385402 [details]
native-animation-for-ews(including parsing + non_native_animation)

[cathiechen]

Created attachment 385403 [details]
native-animation-for-review

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

Comment on attachment 385403 [details]
native-animation-for-review

View in context: https://bugs.webkit.org/attachment.cgi?id=385403&action=review

> LayoutTests/ChangeLog:12
> +        copy scroll-behavior test to fast/scrolling/ios/

I guess this is only needed because AsyncOverflowScrollingEnabled is not fully supported on non-iOS platforms? Maybe explain this?

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:277
> +#endif

Maybe you can do only one call stateNode->setRequestedScrollPosition (see below).

bool animated = false
#if PLATFORM(IOS_FAMILY)
...
#endif
stateNode->setRequestedScrollPosition(scrollPosition, inProgrammaticScroll, animated);

> Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp:240
> +#endif

I think this #if #else can be merged in one function definition using the UNUSED_PARAM(requestedWithAnimation) on non-iOS platform?

> Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h:119
> +#endif

setRequestedScrollPosition can be have a single declaration outside the #if ; maybe you can remove the default value for requestedWithAnimation you end up always being explicit.

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:168
> +#endif

You don't need PLATFORM(IOS_FAMILY) here, just use UNUSED_PARAM(needSyncScrollPosition) on non-iOS platforms.

> Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp:273
> +#endif

Again, you can probably do only one node.setRequestedScrollPosition call ouside the #if

> Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp:231
> +#endif

Ditto?

> Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingTree.cpp:111
> +#endif

Can probably merge this in one m_scrollingCoordinatorProxy.scrollingTreeNodeDidScroll call and do the #if to calculate the last parameter.

[cathiechen]

Comment on attachment 385403 [details]
native-animation-for-review

View in context: https://bugs.webkit.org/attachment.cgi?id=385403&action=review

Hi Fred,
Thanks for the review:)

>> LayoutTests/ChangeLog:12
>> +        copy scroll-behavior test to fast/scrolling/ios/
> 
> I guess this is only needed because AsyncOverflowScrollingEnabled is not fully supported on non-iOS platforms? Maybe explain this?

done

>> Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp:240
>> +#endif
> 
> I think this #if #else can be merged in one function definition using the UNUSED_PARAM(requestedWithAnimation) on non-iOS platform?

Done, thanks:)

>> Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h:119
>> +#endif
> 
> setRequestedScrollPosition can be have a single declaration outside the #if ; maybe you can remove the default value for requestedWithAnimation you end up always being explicit.

Done

>> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:168
>> +#endif
> 
> You don't need PLATFORM(IOS_FAMILY) here, just use UNUSED_PARAM(needSyncScrollPosition) on non-iOS platforms.

Done

>> Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp:273
>> +#endif
> 
> Again, you can probably do only one node.setRequestedScrollPosition call ouside the #if

Done

>> Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp:231
>> +#endif
> 
> Ditto?

done

>> Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingTree.cpp:111
>> +#endif
> 
> Can probably merge this in one m_scrollingCoordinatorProxy.scrollingTreeNodeDidScroll call and do the #if to calculate the last parameter.

done

[cathiechen]

Created attachment 385591 [details]
native-animation-for-ews(including parsing + non_native_animation)

[cathiechen]

Created attachment 385592 [details]
native-animation-for-review

[cathiechen]

Created attachment 385596 [details]
native-animation-for-ews(including parsing + non_native_animation)

[cathiechen]

Created attachment 385597 [details]
native-animation-for-ews(including parsing + non_native_animation)

[cathiechen]

Created attachment 385600 [details]
native-animation-for-ews(including parsing + non_native_animation)

[cathiechen]

Created attachment 385602 [details]
native-animation-for-ews(including parsing + non_native_animation)

[cathiechen]

Created attachment 385716 [details]
native-animation-for-ews(including parsing + non_native_animation)

Rebase

[cathiechen]

Created attachment 385717 [details]
native-animation-for-review

[cathiechen]

Created attachment 385722 [details]
native-animation-for-ews(including parsing + non_native_animation)

[cathiechen]

Created attachment 385723 [details]
native-animation-for-review

[cathiechen]

Created attachment 388201 [details]
Patch

[cathiechen]

Created attachment 390497 [details]
Patch

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

Comment on attachment 390497 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390497&action=review

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:307
> +void AsyncScrollingCoordinator::setScrollAnimationInProgress(ScrollingNodeID nodeID, bool isScrollAnimationInProgress)

This function does nothing if isScrollAnimationInProgress is true. Should it be called interruptScrollAnimation()?

> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.mm:110
> +    setScrollAnimationInProgress(nodeID, isScrollAnimationInProgress);

This could be
if (!isScrollAnimationInProgress)
  interruptScrollAnimation(nodeID);

[cathiechen]

Comment on attachment 390497 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390497&action=review

>> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:307
>> +void AsyncScrollingCoordinator::setScrollAnimationInProgress(ScrollingNodeID nodeID, bool isScrollAnimationInProgress)
> 
> This function does nothing if isScrollAnimationInProgress is true. Should it be called interruptScrollAnimation()?

isScrollAnimationInProgress is always false currently. This function is used to indicate that native animated scroll is end.
I changed the name to nativeScrollAnimationStopped, WDYT?

[cathiechen]

Created attachment 391029 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/71331742>

[Simon Fraser (smfr)]

I'll take this over, if that's OK.

[cathiechen]

(In reply to Simon Fraser (smfr) from comment #34)
> I'll take this over, if that's OK.

Sure, no problem, thanks!

[Simon Fraser (smfr)]

Created attachment 440729 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 440730 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Simon Fraser (smfr)]

Created attachment 440731 [details]
Patch

[EWS]

Committed r283911 (242782@main): <https://commits.webkit.org/242782@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440731 [details].