[webkit-reviews] review granted: [Bug 210543] [GTK] Make PlatformScreen::screenDPI() GTK4-ready : [Attachment 396521] Patch

[bugzilla-daemon at webkit.org]

Adrian Perez <aperez at igalia.com> has granted Claudio Saavedra
<csaavedra at igalia.com>'s request for review:
Bug 210543: [GTK] Make PlatformScreen::screenDPI() GTK4-ready
https://bugs.webkit.org/show_bug.cgi?id=210543

Attachment 396521: Patch

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




--- Comment #2 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 396521
  --> https://bugs.webkit.org/attachment.cgi?id=396521
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 ��‍♂️️