Bug 210543 – [GTK] Make PlatformScreen::screenDPI() GTK4-ready

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Claudio Saavedra]

[GTK] Make PlatformScreen::screenDPI() GTK4-ready

[Claudio Saavedra]

Created attachment 396521 [details]
Patch

[Adrian Perez]

Comment on attachment 396521 [details]
Patch

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

There is just one small change needed before landing; the patch looks fine otherwise.

> Source/WebCore/platform/gtk/PlatformScreenGtk.cpp:99
> +        if (dpi != -1)

It would be good better use “if (fabs(dpi - -1.0) > DBL_MIN)” here. Comparison
of floating point values is not guaranteed to work well, I guess this has been
working fine because gdk_screen_get_resolution() is doing “return -1.0” with a
literal, and doing “-1 == dpi” results in comparing two doubles with the same
bit-level representation… but feel free to leave it as-is if you would rather
leave the existing logic untouched.

> Source/WebCore/platform/gtk/PlatformScreenGtk.cpp:107
> +        g_object_get(gtkSettings, "gtk-xft-dpi", &gtkXftDpi, NULL);

NULL → nullptr

> Source/WebCore/platform/gtk/PlatformScreenGtk.cpp:120
> +    GdkMonitor* monitor = gdk_display_get_monitor(display, 0);

It's a bit sad that we cannot set the DPI per web view; it would be nicer to
find out on which monitor the widget is being displayed and use the value for
that. Anyway, this is as good as the existing code was, so that's okay 🙆‍♂️️

[Claudio Saavedra]

Comment on attachment 396521 [details]
Patch

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

>> Source/WebCore/platform/gtk/PlatformScreenGtk.cpp:99
>> +        if (dpi != -1)
> 
> It would be good better use “if (fabs(dpi - -1.0) > DBL_MIN)” here. Comparison
> of floating point values is not guaranteed to work well, I guess this has been
> working fine because gdk_screen_get_resolution() is doing “return -1.0” with a
> literal, and doing “-1 == dpi” results in comparing two doubles with the same
> bit-level representation… but feel free to leave it as-is if you would rather
> leave the existing logic untouched.

I think I'm gonna stick to the -1 comparison since we know it's a literal that's used.

>> Source/WebCore/platform/gtk/PlatformScreenGtk.cpp:120
>> +    GdkMonitor* monitor = gdk_display_get_monitor(display, 0);
> 
> It's a bit sad that we cannot set the DPI per web view; it would be nicer to
> find out on which monitor the widget is being displayed and use the value for
> that. Anyway, this is as good as the existing code was, so that's okay 🙆‍♂️️

We can check whether doing this would be feasible when we break API for GTK4, but this code will have to move somewhere else as PlatformScreen is not useful with multiple monitors.

[Claudio Saavedra]

Created attachment 396530 [details]
Patch for landing

[EWS]

Committed r260127: <https://trac.webkit.org/changeset/260127>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396530 [details].