Bug 204882

Summary: Add support for scroll behavior relies on ScrollAnimation of the Web process
Product: WebKit Reporter: cathiechen <cathiechen>
Component: DOMAssignee: cathiechen <cathiechen>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, cdumez, cmarcelo, commit-queue, dbates, esprehn+autocc, ews-watchlist, fred.wang, glenn, gyuyoung.kim, Hironori.Fujii, jamesr, jbedard, joepeck, kangil.han, koivisto, kondapallykalyan, luiz, macpherson, menard, mifenton, mmaxfield, pdr, ryuan.choi, sergio, simon.fraser, tonikitoo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=206553
Bug Depends on: 205009, 206559, 206884, 208566    
Bug Blocks: 188043, 204936, 210171    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
scroll-animation-in-web-process-for-review
none
Patch
none
scroll-animation-in-web-process-for-review
none
Non-native-animation-for-ews(including parsing)
none
Non-native-animation-for-review
none
Non-native-animation-for-ews(including parsing)
none
Non-native-animation-for-review
none
Non-native-animation-for-ews(including parsing)
none
Non-native-animation-for-review
none
Non-native-animation-for-ews(including parsing)
none
Non-native-animation-for-review
none
Non-native-animation-for-ews(including parsing)
none
Non-native-animation-for-review
none
Non-native-animation-for-ews(including parsing)
none
Non-native-animation-for-review
none
Non-native-animation-for-ews(including parsing)
none
Non-native-animation-for-review
fred.wang: review+
Non-native-animation-for-ews(including parsing)
none
Non-native-animation-for-review
none
Non-native-animation-for-ews(including parsing)
none
Non-native-animation-for-review
none
native-animation-for-ews(including parsing + non_native_animation)
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description cathiechen 2019-12-05 02:44:31 PST
This is the first support part of bug 188043.
Deal with scroll-behavior CSS property and ScrollOptions parsing and add a run time flag for scroll-behavior.
This will be followed by two other support parts:
  - Support smooth scrolling by animation timer in the Web process.
  - Support iOS smooth scrolling by using native interfaces in the UI process.
Comment 1 cathiechen 2019-12-05 04:00:38 PST
Created attachment 384896 [details]
Patch
Comment 2 cathiechen 2019-12-05 05:36:06 PST
Created attachment 384898 [details]
Patch
Comment 3 cathiechen 2019-12-05 08:49:01 PST
Update:
This bug will present the implement based on scroll animation in the Web process.

Sorry for the confusing description.
Comment 4 cathiechen 2019-12-05 08:51:18 PST
Created attachment 384908 [details]
Patch
Comment 5 cathiechen 2019-12-09 08:35:27 PST
Created attachment 385152 [details]
scroll-animation-in-web-process-for-review

This patch only contains the change of scroll animation in web process.
Comment 6 cathiechen 2019-12-09 08:45:18 PST
Created attachment 385153 [details]
Patch
Comment 7 cathiechen 2019-12-09 08:47:10 PST
Created attachment 385154 [details]
scroll-animation-in-web-process-for-review
Comment 8 Frédéric Wang (:fredw) 2019-12-09 08:51:35 PST
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 9 Frédéric Wang (:fredw) 2019-12-09 08:55:22 PST
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 10 cathiechen 2019-12-10 06:02:14 PST
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
Comment 11 cathiechen 2019-12-10 06:08:14 PST
Created attachment 385255 [details]
Non-native-animation-for-ews(including parsing)
Comment 12 cathiechen 2019-12-10 06:09:07 PST
Created attachment 385256 [details]
Non-native-animation-for-review
Comment 13 Frédéric Wang (:fredw) 2019-12-10 07:12:42 PST
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 14 Frédéric Wang (:fredw) 2019-12-10 07:18:34 PST
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 15 cathiechen 2019-12-10 09:14:30 PST
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.
Comment 16 cathiechen 2019-12-10 09:25:15 PST
Created attachment 385269 [details]
Non-native-animation-for-ews(including parsing)
Comment 17 cathiechen 2019-12-10 09:25:36 PST
Created attachment 385270 [details]
Non-native-animation-for-review
Comment 18 cathiechen 2019-12-10 09:55:50 PST
Created attachment 385273 [details]
Non-native-animation-for-ews(including parsing)
Comment 19 cathiechen 2019-12-10 09:56:20 PST
Created attachment 385274 [details]
Non-native-animation-for-review
Comment 20 cathiechen 2019-12-10 10:07:54 PST
Created attachment 385278 [details]
Non-native-animation-for-ews(including parsing)
Comment 21 cathiechen 2019-12-10 10:08:19 PST
Created attachment 385279 [details]
Non-native-animation-for-review
Comment 22 cathiechen 2019-12-10 22:52:59 PST
Created attachment 385353 [details]
Non-native-animation-for-ews(including parsing)

Rebase code to fix the compile error.
Comment 23 cathiechen 2019-12-10 22:53:27 PST
Created attachment 385354 [details]
Non-native-animation-for-review
Comment 24 cathiechen 2019-12-11 01:36:58 PST
Created attachment 385365 [details]
Non-native-animation-for-ews(including parsing)
Comment 25 cathiechen 2019-12-11 01:37:19 PST
Created attachment 385366 [details]
Non-native-animation-for-review
Comment 26 cathiechen 2019-12-11 02:42:05 PST
Created attachment 385368 [details]
Non-native-animation-for-ews(including parsing)
Comment 27 cathiechen 2019-12-11 02:42:27 PST
Created attachment 385369 [details]
Non-native-animation-for-review
Comment 28 Frédéric Wang (:fredw) 2019-12-11 02:54:53 PST
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 29 cathiechen 2019-12-11 03:09:56 PST
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.
Comment 30 cathiechen 2019-12-11 03:12:33 PST
Created attachment 385370 [details]
Non-native-animation-for-ews(including parsing)
Comment 31 cathiechen 2019-12-11 03:12:51 PST
Created attachment 385371 [details]
Non-native-animation-for-review
Comment 32 cathiechen 2019-12-11 07:01:48 PST
Created attachment 385385 [details]
Non-native-animation-for-ews(including parsing)
Comment 33 cathiechen 2019-12-11 07:02:04 PST
Created attachment 385386 [details]
Non-native-animation-for-review
Comment 34 cathiechen 2020-01-17 01:15:58 PST
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.
Comment 35 cathiechen 2020-01-18 00:24:10 PST
Created attachment 388135 [details]
native-animation-for-ews(including parsing + non_native_animation)
Comment 36 cathiechen 2020-01-18 00:33:07 PST
Created attachment 388137 [details]
Patch
Comment 37 cathiechen 2020-01-18 04:04:16 PST
Created attachment 388147 [details]
Patch
Comment 38 cathiechen 2020-01-18 09:58:14 PST
Created attachment 388153 [details]
Patch
Comment 39 cathiechen 2020-01-18 21:14:08 PST
Created attachment 388168 [details]
Patch
Comment 40 cathiechen 2020-01-19 01:33:20 PST
Created attachment 388175 [details]
Patch
Comment 41 WebKit Commit Bot 2020-01-19 08:43:49 PST
Comment on attachment 388175 [details]
Patch

Clearing flags on attachment: 388175

Committed r254807: <https://trac.webkit.org/changeset/254807>
Comment 42 WebKit Commit Bot 2020-01-19 08:43:52 PST
All reviewed patches have been landed.  Closing bug.
Comment 43 Radar WebKit Bug Importer 2020-01-19 08:44:30 PST
<rdar://problem/58720725>
Comment 44 Antti Koivisto 2020-01-19 09:03:51 PST
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?
Comment 45 cathiechen 2020-01-19 09:15:44 PST
(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
Comment 46 Fujii Hironori 2020-01-20 04:02:03 PST
Filed: Bug 206494 – ScrollableArea.cpp(63): error C2338: ScrollableArea_should_stay_small
Comment 47 Fujii Hironori 2020-01-20 17:37:30 PST
Committed r254839: <https://trac.webkit.org/changeset/254839>
Comment 48 Fujii Hironori 2020-01-20 17:39:02 PST
Broke Apple internal builds. Reverted in r254839.
Comment 49 Fujii Hironori 2020-01-20 17:42:14 PST
*** Bug 206494 has been marked as a duplicate of this bug. ***
Comment 50 cathiechen 2020-01-21 00:03:35 PST
Created attachment 388282 [details]
Patch
Comment 51 WebKit Commit Bot 2020-01-21 04:25:19 PST
Comment on attachment 388282 [details]
Patch

Clearing flags on attachment: 388282

Committed r254849: <https://trac.webkit.org/changeset/254849>
Comment 52 WebKit Commit Bot 2020-01-21 04:25:22 PST
All reviewed patches have been landed.  Closing bug.
Comment 53 Simon Fraser (smfr) 2020-01-21 13:57:51 PST
This broke scrolling with Page Up/Page Down on macOS. I'll revert for now.
Comment 54 Simon Fraser (smfr) 2020-01-21 13:58:47 PST
It relanded in http://trac.webkit.org/changeset/254807
Comment 55 Simon Fraser (smfr) 2020-01-21 14:01:34 PST
Rollout failed. I'll try to debug.
Comment 56 WebKit Commit Bot 2020-01-21 15:36:22 PST
Re-opened since this is blocked by bug 206559
Comment 58 Simon Fraser (smfr) 2020-01-21 15:37:48 PST
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!)
Comment 59 Simon Fraser (smfr) 2020-01-21 15:41:06 PST
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.
Comment 60 cathiechen 2020-01-22 00:38:45 PST
(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.
Comment 61 cathiechen 2020-02-03 05:12:43 PST
Created attachment 389500 [details]
Patch
Comment 62 Frédéric Wang (:fredw) 2020-02-04 09:37:24 PST
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
Comment 63 cathiechen 2020-02-05 00:26:25 PST
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
Comment 64 cathiechen 2020-02-05 00:32:37 PST
Created attachment 389780 [details]
Patch
Comment 65 cathiechen 2020-02-05 01:48:25 PST
Created attachment 389785 [details]
Patch
Comment 66 cathiechen 2020-02-06 07:49:12 PST
Hi,

The patch has passed the check of EWS. I'm going to land it again:)
Comment 67 cathiechen 2020-02-06 07:49:12 PST
Hi,

The patch has passed the check of EWS. I'm going to land it again:)
Comment 68 WebKit Commit Bot 2020-02-06 08:48:21 PST
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 69 WebKit Commit Bot 2020-02-06 08:49:10 PST
Comment on attachment 389785 [details]
Patch

Clearing flags on attachment: 389785

Committed r255957: <https://trac.webkit.org/changeset/255957>
Comment 70 WebKit Commit Bot 2020-02-06 08:49:13 PST
All reviewed patches have been landed.  Closing bug.