Bug 205009 – Add support for scroll-behavior's css property and ScrollOptions parsing

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[cathiechen]

Add support for scroll-behavior's css property and ScrollOptions parsing.
And add an experimental flag to guard.

[cathiechen]

Created attachment 385148 [details]
Patch

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

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.

[cathiechen]

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?

[cathiechen]

Created attachment 385247 [details]
Patch

[cathiechen]

Created attachment 385250 [details]
Patch

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

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.

[cathiechen]

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.

[cathiechen]

Created attachment 385258 [details]
Patch

[cathiechen]

Created attachment 385351 [details]
Patch

[cathiechen]

Created attachment 387520 [details]
Patch

[cathiechen]

Rebase the patch.

[cathiechen]

Created attachment 388010 [details]
Patch

[cathiechen]

Created attachment 388018 [details]
Patch

[cathiechen]

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

[WebKit Commit Bot]

Comment on attachment 388018 [details]
Patch

Clearing flags on attachment: 388018

Committed r254790: <https://trac.webkit.org/changeset/254790>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58705707>

[Brady Eidson]

You missed updating the DerivedSources xcfilelists

[Brady Eidson]

Reopening to attach new patch.

[Brady Eidson]

Created attachment 388169 [details]
Patch

[David Kilzer (:ddkilzer)]

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

[Brady Eidson]

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

[David Kilzer (:ddkilzer)]

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

[cathiechen]

Hi,
Sorry for the troubles and thanks for fixing this:)