[webkit-reviews] review denied: [Bug 219305] Add support for overscroll-behavior parsing : [Attachment 414947] Patch

Sat Nov 28 11:29:26 PST 2020

Sam Weinig <sam at webkit.org> has denied cathiechen <cathiechen at igalia.com>'s
request for review:
Bug 219305: Add support for overscroll-behavior parsing
https://bugs.webkit.org/show_bug.cgi?id=219305

Attachment 414947: Patch

https://bugs.webkit.org/attachment.cgi?id=414947&action=review

--- Comment #4 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 414947
  --> https://bugs.webkit.org/attachment.cgi?id=414947
Patch

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

> Source/WebKitLegacy/mac/WebView/WebPreferenceKeysPrivate.h:174
> +#define WebKitOverscrollBehaviorEnabledPreferenceKey
@"WebKitOverscrollBehaviorEnabled"

There is no need to add this. Tests can access preference state without it.

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:3119
> +- (BOOL)overscrollBehaviorEnabled
> +{
> +    return [self
_boolValueForKey:WebKitOverscrollBehaviorEnabledPreferenceKey];
> +}
> +
> +- (void)setOverscrollBehaviorEnabled:(BOOL)flag
> +{
> +    [self _setBoolValue:flag
forKey:WebKitOverscrollBehaviorEnabledPreferenceKey];
> +}

There is no need to add this. Tests can access preference state without it.

> Source/WebKitLegacy/mac/WebView/WebPreferencesPrivate.h:325
> + at property (nonatomic) BOOL overscrollBehaviorEnabled;

There is no need to add this. Tests can access preference state without it.

> Tools/DumpRenderTree/TestOptions.cpp:109
> +	       { "OverscrollBehaviorEnabled", false },

We have mostly been enabling experimental features by defaults in tests (which
happens automatically). What is your intention with this change?

>
LayoutTests/imported/w3c/web-platform-tests/css/css-overscroll-behavior/inherit
ance.html:1
> +<!DOCTYPE html><!-- webkit-test-runner [ OverscrollBehaviorEnabled=true ]
-->

In general, we try not to edit the imported WPT tests unless we intend to
upstream those changes at the same time. If you instead follow the pattern of
having experimental features on by default (in the tests) this change should
not be necessary.