Bug 205009

Summary: Add support for scroll-behavior's css property and ScrollOptions parsing
Product: WebKit Reporter: cathiechen <cathiechen>
Component: DOMAssignee: cathiechen <cathiechen>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, beidson, cdumez, commit-queue, dbates, ddkilzer, esprehn+autocc, ews-watchlist, fred.wang, glenn, gyuyoung.kim, joepeck, kondapallykalyan, macpherson, menard, pdr, ryuan.choi, sergio, simon.fraser, 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=210634
Bug Depends on:    
Bug Blocks: 188043, 204882    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch ddkilzer: commit-queue-

Description cathiechen 2019-12-09 06:59:48 PST
Add support for scroll-behavior's css property and ScrollOptions parsing.
And add an experimental flag to guard.
Comment 1 cathiechen 2019-12-09 07:31:27 PST
Created attachment 385148 [details]
Patch
Comment 2 Frédéric Wang (:fredw) 2019-12-09 08:17:31 PST
Comment on attachment 385148 [details]
Patch

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

OK, this LGTM but I don't know if we should land this patch now + I'm the author of most of this part, so someone else should probably review.

> Source/WebCore/Sources.txt:1755
> +platform/ScrollAnimationSmooth.cpp

That change does not seem necessary for now.

> Source/WebCore/SourcesGTK.txt:-66
> -platform/ScrollAnimationSmooth.cpp

That one either.

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:178
> +    unsigned useSmoothScrolling : 1; // ScrollBehavior

I wonder if this should be placed somewhere else to optimize bitfield alignment but I'm not really sure what's the recommendation in WebKit.

> LayoutTests/platform/mac-wk1/TestExpectations:748
> +webkit.org/b/191357 imported/w3c/web-platform-tests/css/cssom-view/inheritance.html [ Skip ]

Maybe skipping that one is not necessary, since it's just css parsing.
Comment 3 cathiechen 2019-12-10 03:45:48 PST
Comment on attachment 385148 [details]
Patch

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

Hi Fred,
Thanks for the review:)

>> Source/WebCore/Sources.txt:1755
>> +platform/ScrollAnimationSmooth.cpp
> 
> That change does not seem necessary for now.

Done, move this change to web scroll patch.

>> Source/WebCore/SourcesGTK.txt:-66
>> -platform/ScrollAnimationSmooth.cpp
> 
> That one either.

Ditto

>> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:178
>> +    unsigned useSmoothScrolling : 1; // ScrollBehavior
> 
> I wonder if this should be placed somewhere else to optimize bitfield alignment but I'm not really sure what's the recommendation in WebKit.

Done, put it behind textOverflow.

>> LayoutTests/platform/mac-wk1/TestExpectations:748
>> +webkit.org/b/191357 imported/w3c/web-platform-tests/css/cssom-view/inheritance.html [ Skip ]
> 
> Maybe skipping that one is not necessary, since it's just css parsing.

inheritance.html is passed on non-mac-wk1, but not on mac-wk1. 
So I guess, maybe we can skip it first and figure this out in the follow-up bug?
Comment 4 cathiechen 2019-12-10 03:51:12 PST
Created attachment 385247 [details]
Patch
Comment 5 cathiechen 2019-12-10 04:36:51 PST
Created attachment 385250 [details]
Patch
Comment 6 Frédéric Wang (:fredw) 2019-12-10 06:27:00 PST
Comment on attachment 385148 [details]
Patch

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

>>> LayoutTests/platform/mac-wk1/TestExpectations:748
>>> +webkit.org/b/191357 imported/w3c/web-platform-tests/css/cssom-view/inheritance.html [ Skip ]
>> 
>> Maybe skipping that one is not necessary, since it's just css parsing.
> 
> inheritance.html is passed on non-mac-wk1, but not on mac-wk1. 
> So I guess, maybe we can skip it first and figure this out in the follow-up bug?

I don't understand why this is failing on WK1 but in any case, I maybe the comment should be updated to better explain why.
Comment 7 cathiechen 2019-12-10 07:31:26 PST
Comment on attachment 385148 [details]
Patch

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

>>>> LayoutTests/platform/mac-wk1/TestExpectations:748
>>>> +webkit.org/b/191357 imported/w3c/web-platform-tests/css/cssom-view/inheritance.html [ Skip ]
>>> 
>>> Maybe skipping that one is not necessary, since it's just css parsing.
>> 
>> inheritance.html is passed on non-mac-wk1, but not on mac-wk1. 
>> So I guess, maybe we can skip it first and figure this out in the follow-up bug?
> 
> I don't understand why this is failing on WK1 but in any case, I maybe the comment should be updated to better explain why.

Done.
Comment 8 cathiechen 2019-12-10 07:33:45 PST
Created attachment 385258 [details]
Patch
Comment 9 cathiechen 2019-12-10 22:42:25 PST
Created attachment 385351 [details]
Patch
Comment 10 cathiechen 2020-01-13 06:23:12 PST
Created attachment 387520 [details]
Patch
Comment 11 cathiechen 2020-01-13 06:26:02 PST
Rebase the patch.
Comment 12 cathiechen 2020-01-16 21:52:36 PST
Created attachment 388010 [details]
Patch
Comment 13 cathiechen 2020-01-17 00:42:18 PST
Created attachment 388018 [details]
Patch
Comment 14 cathiechen 2020-01-17 01:11:57 PST
Rebase the patch.

This patch has presented for a while. We plan to land it tomorrow.
Please leave a msg if there's any concern. Thanks:)
Comment 15 WebKit Commit Bot 2020-01-17 21:49:31 PST
Comment on attachment 388018 [details]
Patch

Clearing flags on attachment: 388018

Committed r254790: <https://trac.webkit.org/changeset/254790>
Comment 16 WebKit Commit Bot 2020-01-17 21:49:34 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Radar WebKit Bug Importer 2020-01-17 21:50:20 PST
<rdar://problem/58705707>
Comment 18 Brady Eidson 2020-01-18 21:19:27 PST
You missed updating the DerivedSources xcfilelists
Comment 19 Brady Eidson 2020-01-18 21:19:41 PST
Reopening to attach new patch.
Comment 20 Brady Eidson 2020-01-18 21:19:42 PST
Created attachment 388169 [details]
Patch
Comment 21 David Kilzer (:ddkilzer) 2020-01-18 22:04:12 PST
(In reply to Brady Eidson from comment #20)
> Created attachment 388169 [details]
> Patch

This commit will fail because I just committed the same build fix in r254802:

<https://trac.webkit.org/r254802>

Sorry about that.
Comment 22 Brady Eidson 2020-01-18 22:04:57 PST
(In reply to David Kilzer (:ddkilzer) from comment #21)
> (In reply to Brady Eidson from comment #20)
> > Created attachment 388169 [details]
> > Patch
> 
> This commit will fail because I just committed the same build fix in r254802:
> 
> <https://trac.webkit.org/r254802>
> 
> Sorry about that.

Way to cowboy it :)
Comment 23 David Kilzer (:ddkilzer) 2020-01-18 22:05:55 PST
(In reply to Brady Eidson from comment #22)
> (In reply to David Kilzer (:ddkilzer) from comment #21)
> > (In reply to Brady Eidson from comment #20)
> > > Created attachment 388169 [details]
> > > Patch
> > 
> > This commit will fail because I just committed the same build fix in r254802:
> > 
> > <https://trac.webkit.org/r254802>
> > 
> > Sorry about that.
> 
> Way to cowboy it :)

Honestly, I didn't think anyone else would be trying to build WebKit at this time of the night. :)
Comment 24 cathiechen 2020-01-18 23:58:38 PST
Hi,
Sorry for the troubles and thanks for fixing this:)