Created attachment 384896 [details] Patch

Created attachment 384898 [details] Patch

Update: This bug will present the implement based on scroll animation in the Web process. Sorry for the confusing description.

Created attachment 384908 [details] Patch

Created attachment 385152 [details] scroll-animation-in-web-process-for-review This patch only contains the change of scroll animation in web process.

Created attachment 385153 [details] Patch

Created attachment 385154 [details] scroll-animation-in-web-process-for-review

View in context: https://bugs.webkit.org/attachment.cgi?id=385152&action=review > LayoutTests/platform/ios/TestExpectations:3468 > +webkit.org/b/204757 imported/w3c/web-platform-tests/fetch/api/request/destination/fetch-destination-no-load-event.https.html [ Pass Failure ] I'm not sure what's the diff here. > Source/WebCore/ChangeLog:164 > + (WebCore::useSmoothScrolling): These changes in the Changelog seem weird > Source/WebCore/dom/Element.cpp:1307 > + renderer->setScrollLeft(effectiveLeft, ScrollType::Programmatic, animated, ScrollClamping::Clamped); Do you need explicit ScrollClamping::Clamped here? > Source/WebCore/dom/Element.cpp:1330 > + renderer->setScrollTop(effectiveTop, ScrollType::Programmatic, animated, ScrollClamping::Clamped); Ditto. > Source/WebCore/page/DOMWindow.cpp:1636 > + if (!view->isScrollInProgress() && !scrollToOptions.left.value() && !scrollToOptions.top.value() && view->contentsScrollPosition() == IntPoint(0, 0)) I would add a C++ comment here to explain the optimization and why it is skipped, even if it's in the changelog. > Source/WebCore/platform/ScrollTypes.h:59 > + WebAnimationTimerStarted, Can we rename it WebProcessScrollAnimationTimerStarted? Or maybe NonNativeAnimationTimerStarted. Not sure what's best. > Source/WebCore/platform/ScrollableArea.h:66 > + bool isWebScrollAnimationInProgress(); WebProcess to? > Source/WebKit/ChangeLog:12 > + Add CSSOM smooth scrolling as an experimental feature. this part is weird

Comment on attachment 385154 [details] scroll-animation-in-web-process-for-review View in context: https://bugs.webkit.org/attachment.cgi?id=385154&action=review See my comments on bug 188043 > LayoutTests/platform/ios/TestExpectations:3468 > +webkit.org/b/204757 imported/w3c/web-platform-tests/fetch/api/request/destination/fetch-destination-no-load-event.https.html [ Pass Failure ] this change is weird > Source/WebCore/dom/Element.cpp:1307 > + renderer->setScrollLeft(effectiveLeft, ScrollType::Programmatic, animated, ScrollClamping::Clamped); do you need to be explicit about ScrollClamping::Clamped > Source/WebCore/dom/Element.cpp:1330 > + renderer->setScrollTop(effectiveTop, ScrollType::Programmatic, animated, ScrollClamping::Clamped); ditto > Source/WebCore/page/DOMWindow.cpp:1636 > + if (!view->isScrollInProgress() && !scrollToOptions.left.value() && !scrollToOptions.top.value() && view->contentsScrollPosition() == IntPoint(0, 0)) maybe we should explain this with a comment > Source/WebCore/page/DOMWindow.cpp:1644 > + // See https://github.com/w3c/csswg-drafts/issues/2977 maybe a separate webkit bug > Source/WebCore/platform/ScrollTypes.h:59 > + WebAnimationTimerStarted, WebProcessAnimationTimerStarted? or even NonNative? > Source/WebCore/platform/ScrollableArea.h:66 > + bool isWebScrollAnimationInProgress(); This one should be renamed too I guess.

Comment on attachment 385154 [details] scroll-animation-in-web-process-for-review View in context: https://bugs.webkit.org/attachment.cgi?id=385154&action=review Hi Fred, Thanks for the review:) >> LayoutTests/platform/ios/TestExpectations:3468 >> +webkit.org/b/204757 imported/w3c/web-platform-tests/fetch/api/request/destination/fetch-destination-no-load-event.https.html [ Pass Failure ] > > this change is weird Done >> Source/WebCore/dom/Element.cpp:1307 >> + renderer->setScrollLeft(effectiveLeft, ScrollType::Programmatic, animated, ScrollClamping::Clamped); > > do you need to be explicit about ScrollClamping::Clamped Done >> Source/WebCore/dom/Element.cpp:1330 >> + renderer->setScrollTop(effectiveTop, ScrollType::Programmatic, animated, ScrollClamping::Clamped); > > ditto Done >> Source/WebCore/page/DOMWindow.cpp:1636 >> + if (!view->isScrollInProgress() && !scrollToOptions.left.value() && !scrollToOptions.top.value() && view->contentsScrollPosition() == IntPoint(0, 0)) > > maybe we should explain this with a comment Done. >> Source/WebCore/page/DOMWindow.cpp:1644 >> + // See https://github.com/w3c/csswg-drafts/issues/2977 > > maybe a separate webkit bug Done. >> Source/WebCore/platform/ScrollTypes.h:59 >> + WebAnimationTimerStarted, > > WebProcessAnimationTimerStarted? > > or even NonNative? Change to NonNativeAnimationTimerStarted >> Source/WebCore/platform/ScrollableArea.h:66 >> + bool isWebScrollAnimationInProgress(); > > This one should be renamed too I guess. Done, change to isNonNativeAnimationInProgress

Created attachment 385255 [details] Non-native-animation-for-ews(including parsing)

Created attachment 385256 [details] Non-native-animation-for-review

Comment on attachment 385256 [details] Non-native-animation-for-review View in context: https://bugs.webkit.org/attachment.cgi?id=385256&action=review LGTM in general. But again most of this is from me, we should find another reviewer... > Source/WebCore/dom/Element.cpp:1305 > + int effectiveLeft = clampToInteger(newLeft * renderer->style().effectiveZoom()); "effective" is maybe not the good name. clampedLeft instead? > Source/WebCore/dom/Element.cpp:1328 > + int effectiveTop = clampToInteger(newTop * renderer->style().effectiveZoom()); clampedTop > Source/WebCore/page/DOMWindow.cpp:1636 > + // The previous scroll animation should be stopped by this scroll, so it shouldn't be skipped. Maybe "This is an optimization for the common case of scrolling to (0, 0) when the scroller is already at the origin. If an animated scroll is in progress, this optimization is skipped to ensure that the animated scroll is really stopped." > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:263 > + // If web scroll animation is in progress, this is set by scroll animator, otherwise, the animation should stop first. "If a non native animation" That say, isNonNativeAnimationInProgress() is a bit confusing, it already sets the scroll behavior status to NotInAnimation when returning false, so it looks like the setScrollBehaviorStatus call is not necessary? > Source/WebCore/platform/ScrollAnimator.cpp:75 > + // FIXME (TODO): This should also take into account animations in derived classes. can we fix this? or open a separate follow-up bug? > Source/WebCore/platform/ScrollableArea.cpp:124 > + I think currentBehaviorStatus() should be a private function updating the m_currentScrollBehaviorStatus(). Then you would define bool isScrollInProgress() { return currentBehaviorStatus() != ScrollBehaviorStatus::NotInAnimation; } bool isNonNativeAnimationInProgress() { return currentBehaviorStatus() != ScrollBehaviorStatus::NonNativeAnimationTimerStarted; } I think the need for an enum of scroll behavior status is not really clear without the iOS part, though. Maybe we'd want to introduce these latter and just use a boolean for now... > Source/WebCore/platform/ScrollableArea.h:66 > + bool isNonNativeAnimationInProgress(); private?

Comment on attachment 385256 [details] Non-native-animation-for-review View in context: https://bugs.webkit.org/attachment.cgi?id=385256&action=review > Source/WebCore/platform/ScrollTypes.h:57 > +enum class ScrollBehaviorStatus : uint8_t { Maybe add a FIXME bug 204936 that explains we will extend this with a native scroll status?

Comment on attachment 385256 [details] Non-native-animation-for-review View in context: https://bugs.webkit.org/attachment.cgi?id=385256&action=review Hi Fred, Thanks for the review:) >> Source/WebCore/dom/Element.cpp:1305 >> + int effectiveLeft = clampToInteger(newLeft * renderer->style().effectiveZoom()); > > "effective" is maybe not the good name. clampedLeft instead? Done >> Source/WebCore/dom/Element.cpp:1328 >> + int effectiveTop = clampToInteger(newTop * renderer->style().effectiveZoom()); > > clampedTop Done >> Source/WebCore/page/DOMWindow.cpp:1636 >> + // The previous scroll animation should be stopped by this scroll, so it shouldn't be skipped. > > Maybe "This is an optimization for the common case of scrolling to (0, 0) when the scroller is already at the origin. If an animated scroll is in progress, this optimization is skipped to ensure that the animated scroll is really stopped." Done. Thanks:) >> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:263 >> + // If web scroll animation is in progress, this is set by scroll animator, otherwise, the animation should stop first. > > "If a non native animation" > > That say, isNonNativeAnimationInProgress() is a bit confusing, it already sets the scroll behavior status to NotInAnimation when returning false, so it looks like the setScrollBehaviorStatus call is not necessary? isNonNativeAnimationInProgress() will not change status. >> Source/WebCore/platform/ScrollAnimator.cpp:75 >> + // FIXME (TODO): This should also take into account animations in derived classes. > > can we fix this? or open a separate follow-up bug? Now that we are using the ScrollBehaviorStatus, it seems this is not needed. I'll remove ScrollAnimator::isScrollInProgress and related interfaces. >> Source/WebCore/platform/ScrollTypes.h:57 >> +enum class ScrollBehaviorStatus : uint8_t { > > Maybe add a FIXME bug 204936 that explains we will extend this with a native scroll status? Done >> Source/WebCore/platform/ScrollableArea.cpp:124 >> + > > I think currentBehaviorStatus() should be a private function updating the m_currentScrollBehaviorStatus(). Then you would define > > bool isScrollInProgress() { return currentBehaviorStatus() != ScrollBehaviorStatus::NotInAnimation; } > bool isNonNativeAnimationInProgress() { return currentBehaviorStatus() != ScrollBehaviorStatus::NonNativeAnimationTimerStarted; } > > I think the need for an enum of scroll behavior status is not really clear without the iOS part, though. Maybe we'd want to introduce these latter and just use a boolean for now... Done. >> Source/WebCore/platform/ScrollableArea.h:66 >> + bool isNonNativeAnimationInProgress(); > > private? Currently, isScrollInProgress() == isNonNativeAnimationInProgress(), after add native animation status, they will be different. isScrollInProgress() will include native and non-native scroll. isNonNativeAnimationInProgress() is non-native scroll. This is need by different scenarios, so it should be public.

Created attachment 385269 [details] Non-native-animation-for-ews(including parsing)

Created attachment 385270 [details] Non-native-animation-for-review

Created attachment 385273 [details] Non-native-animation-for-ews(including parsing)

Created attachment 385274 [details] Non-native-animation-for-review

Created attachment 385278 [details] Non-native-animation-for-ews(including parsing)

Created attachment 385279 [details] Non-native-animation-for-review

Created attachment 385353 [details] Non-native-animation-for-ews(including parsing) Rebase code to fix the compile error.

Created attachment 385354 [details] Non-native-animation-for-review

Created attachment 385365 [details] Non-native-animation-for-ews(including parsing)

Created attachment 385366 [details] Non-native-animation-for-review

Created attachment 385368 [details] Non-native-animation-for-ews(including parsing)

Created attachment 385369 [details] Non-native-animation-for-review

Comment on attachment 385369 [details] Non-native-animation-for-review View in context: https://bugs.webkit.org/attachment.cgi?id=385369&action=review OK LGTM. Again, it would be good to have advice from another reviewer, as I wrote part of this patch. > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:266 > + I think that part is a no-op until we add a third scrollbehaviorstatus. Should it be moved to the other patch?

Comment on attachment 385369 [details] Non-native-animation-for-review View in context: https://bugs.webkit.org/attachment.cgi?id=385369&action=review >> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:266 >> + > > I think that part is a no-op until we add a third scrollbehaviorstatus. Should it be moved to the other patch? Yes, indeed.

Created attachment 385370 [details] Non-native-animation-for-ews(including parsing)

Created attachment 385371 [details] Non-native-animation-for-review

Created attachment 385385 [details] Non-native-animation-for-ews(including parsing)

Created attachment 385386 [details] Non-native-animation-for-review

This patch has presented for a while. We plan to land it tomorrow. Please leave a msg if there's any concern. Thanks:) I'll rebase it after bug 205009 landing.

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

Created attachment 388137 [details] Patch

Created attachment 388147 [details] Patch

Created attachment 388153 [details] Patch

Created attachment 388168 [details] Patch

Created attachment 388175 [details] Patch

Comment on attachment 388175 [details] Patch Clearing flags on attachment: 388175 Committed r254807: <https://trac.webkit.org/changeset/254807>

All reviewed patches have been landed. Closing bug.

<rdar://problem/58720725>

Comment on attachment 388175 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388175&action=review > Source/WebCore/dom/Element.cpp:923 > - scrollBy({ x, y }); > + scrollBy(ScrollToOptions(x, y)); Why these changes?

(In reply to Antti Koivisto from comment #44) > Comment on attachment 388175 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388175&action=review > > > Source/WebCore/dom/Element.cpp:923 > > - scrollBy({ x, y }); > > + scrollBy(ScrollToOptions(x, y)); > > Why these changes? ScrollToOptions inherits ScrollOptions, this changes make ScrollToOptions carries a default value `ScrollBehavior::Auto`. The changes about ScrollToOptions and ScrollOptions is from https://bugs.webkit.org/show_bug.cgi?id=205009

Filed: Bug 206494 – ScrollableArea.cpp(63): error C2338: ScrollableArea_should_stay_small

Committed r254839: <https://trac.webkit.org/changeset/254839>

Broke Apple internal builds. Reverted in r254839.

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

Created attachment 388282 [details] Patch

Comment on attachment 388282 [details] Patch Clearing flags on attachment: 388282 Committed r254849: <https://trac.webkit.org/changeset/254849>

All reviewed patches have been landed. Closing bug.

This broke scrolling with Page Up/Page Down on macOS. I'll revert for now.

It relanded in http://trac.webkit.org/changeset/254807

Rollout failed. I'll try to debug.

Re-opened since this is blocked by bug 206559

This also breaks a test: https://results.webkit.org/?suite=layout-tests&test=scrollingcoordinator%2Fmac%2Ffixed-scrolled-body.html

Sorry, I had to roll out both patches to get it to roll out cleanly. Please make sure macOS page up/down works before re-landing (and add a test!)

A big problem with the scrolling code is that it's really hard to know what the "bottleneck" functions are. There are just so many things called "scrollTo" or "setScrollPosition". That makes it hard know where to insert the "requestScrollPositionUpdate" code.

(In reply to Simon Fraser (smfr) from comment #59) > A big problem with the scrolling code is that it's really hard to know what > the "bottleneck" functions are. There are just so many things called > "scrollTo" or "setScrollPosition". > > That makes it hard know where to insert the "requestScrollPositionUpdate" > code. Yes, indeed, sorry for the mistake, I'll take a look. Here is the recursive codepath: ScrollAnimator::notifyPositionChanged ScrollableArea::setScrollOffsetFromAnimation AsyncScrollingCoordinator::requestScrollPositionUpdate AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll ScrollableArea::scrollToOffsetWithoutAnimation ScrollAnimator::scrollToOffsetWithoutAnimation ScrollAnimator::notifyPositionChanged So I moved "requestScrollPositionUpdate" out of setScrollOffsetFromAnimation. It seems I only insert it partly. I'll try to find another way to fix this.

Created attachment 389500 [details] Patch

Comment on attachment 389500 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389500&action=review The pattern X && useSmoothScrolling(options.behavior, *X) is very frequent. Do we always need to check X != nullptr ? Maybe you want to move this into useSmoothScrolling instead (i.e. make the parameter a pointer) although I think references are preferred in WebKit. Alternatively, you can maybe follow https://webkit.org/code-style-guidelines/#indentation-wrap-bool-op to avoid very long lines. > Source/WebCore/rendering/RenderLayer.cpp:2828 > + bool animated = (box->element() && useSmoothScrolling(options.behavior, *box->element())); parenthesis not needed here

Hi Fred, Thank you:) > View in context: > https://bugs.webkit.org/attachment.cgi?id=389500&action=review > > The pattern X && useSmoothScrolling(options.behavior, *X) is very frequent. > Do we always need to check X != nullptr ? Maybe you want to move this into > useSmoothScrolling instead (i.e. make the parameter a pointer) although I > think references are preferred in WebKit. Alternatively, you can maybe > follow https://webkit.org/code-style-guidelines/#indentation-wrap-bool-op to > avoid very long lines. Done > > > Source/WebCore/rendering/RenderLayer.cpp:2828 > > + bool animated = (box->element() && useSmoothScrolling(options.behavior, *box->element())); > > parenthesis not needed here Done

Created attachment 389780 [details] Patch

Created attachment 389785 [details] Patch

Hi, The patch has passed the check of EWS. I'm going to land it again:)

Hi, The patch has passed the check of EWS. I'm going to land it again:)

The commit-queue encountered the following flaky tests while processing attachment 389785 [details]: editing/spelling/spellcheck-attribute.html bug 206178 (authors: g.czajkowski@samsung.com, mark.lam@apple.com, and rniwa@webkit.org) imported/w3c/web-platform-tests/IndexedDB/fire-success-event-exception.html bug 206554 (authors: shvaikalesh@gmail.com and youennf@gmail.com) The commit-queue is continuing to process your patch.

Comment on attachment 389785 [details] Patch Clearing flags on attachment: 389785 Committed r255957: <https://trac.webkit.org/changeset/255957>

All reviewed patches have been landed. Closing bug.