[webkit-reviews] review denied: [Bug 209729] [WPE] Add axis-locking to kinetic scrolling : [Attachment 395562] Patch

[bugzilla-daemon at webkit.org]

Adrian Perez <aperez at igalia.com> has denied Chris Lord <clord at igalia.com>'s
request for review:
Bug 209729: [WPE] Add axis-locking to kinetic scrolling
https://bugs.webkit.org/show_bug.cgi?id=209729

Attachment 395562: Patch

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




--- Comment #3 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 395562
  --> https://bugs.webkit.org/attachment.cgi?id=395562
Patch

The logic for the axis locking looks good but I think it would be
better to avoid adding new API if possible. Also the patch needs
a small rebase to be able to be applied to trunk right now.

Thanks a lot for working on this!

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

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:184
> +#endif

Do we really want to allow customization of these values? If there are
some reasonable fixed values which provide a better experience overall,
I would go for those and remove the API bits that allow changing them.
For example I see a fixed “#define SCROLL_CAPTURE_THRESHOLD_MS 150” in
the GTK code (that's in “gtkeventcontrollerscroll.c”).

We can always add the new API later if we find out that people routinely
need to tweak those for different touch{screens,panels}.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1594
> +	       0, G_MAXUINT, 200,

If we end up keeping the properties (instead of using constants) it
seems a bit ridiculous to allow “G_MAXUINT” as the maximum value
here. Anything more than a few hundreds of milliseconds will feel
sluggish to the user, so we can cap this at e.g. 1 second (1000ms).

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1611
> +	       0, G_MAXUINT, 8,

Same here, being able to specify a count of pixels bigger than a few hundreds
is kind of pointless as well :D

Related issue: how is this value treated for an output device which uses a
scaling factor different than 1x? Will this be scaled as well (meaning that
the value is in logical pixels) or not (the value is physical pixels)? It
should be at least documented if we keep the new public API.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1629
> +	       0, G_MAXUINT, 15,

Ditto.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1647
> +	       0, G_MAXUINT, 30,

Ditto.

> Source/WebKit/UIProcess/API/wpe/WPEView.h:69
> +    static View* create(struct wpe_view_backend* backend, const
API::PageConfiguration& configuration, WebKitSettings* settings)

Using constant values for the scroll axis locking would prevent needing
to pass the WebKitSettings instance around.