Bug 210543 - [GTK] Make PlatformScreen::screenDPI() GTK4-ready
Summary: [GTK] Make PlatformScreen::screenDPI() GTK4-ready
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Claudio Saavedra
URL:
Keywords:
Depends on:
Blocks: GTK4
  Show dependency treegraph
 
Reported: 2020-04-15 05:44 PDT by Claudio Saavedra
Modified: 2020-04-15 07:54 PDT (History)
2 users (show)

See Also:


Attachments
Patch (3.30 KB, patch)
2020-04-15 05:46 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch for landing (3.31 KB, patch)
2020-04-15 07:15 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Claudio Saavedra 2020-04-15 05:44:13 PDT
[GTK] Make PlatformScreen::screenDPI() GTK4-ready
Comment 1 Claudio Saavedra 2020-04-15 05:46:28 PDT
Created attachment 396521 [details]
Patch
Comment 2 Adrian Perez 2020-04-15 06:37:13 PDT
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 🙆‍♂️️
Comment 3 Claudio Saavedra 2020-04-15 07:13:45 PDT
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.
Comment 4 Claudio Saavedra 2020-04-15 07:15:25 PDT
Created attachment 396530 [details]
Patch for landing
Comment 5 EWS 2020-04-15 07:54:57 PDT
Committed r260127: <https://trac.webkit.org/changeset/260127>

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