[webkit-reviews] review denied: [Bug 142673] [GTK] Add settings to set font size in points, and deprecate the pixel size settings : [Attachment 325469] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Gabriel Ivașcu
<givascu at igalia.com>'s request for review:
Bug 142673: [GTK] Add settings to set font size in points, and deprecate the
pixel size settings
https://bugs.webkit.org/show_bug.cgi?id=142673

Attachment 325469: Patch

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




--- Comment #32 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 325469
  --> https://bugs.webkit.org/attachment.cgi?id=325469
Patch

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

I would split this patch in two, one to add the new API, either as new
properties or as new convenient methods to set/get the font size in points, and
then another patch to automatically update the font size when dpi changes.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:83
> +    gulong gtkXftDpiChangedHandlerID { 0 };

This is already set to 0 on allocation.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:188
> +static double getScreenDPI()
> +{
> +    static const double defaultDPI = 96;
> +
> +#if PLATFORM(GTK)
> +    static const double millimetresPerInch = 25.4;
> +
> +    GdkScreen* screen = gdk_screen_get_default();
> +    if (!screen)
> +	   return defaultDPI;
> +
> +    double dpi = gdk_screen_get_resolution(screen);
> +    if (dpi != -1)
> +	   return dpi;
> +
> +    double diagonalInPixels = std::hypot(gdk_screen_get_width(screen),
gdk_screen_get_height(screen));
> +    double diagonalInInches = std::hypot(gdk_screen_get_width_mm(screen),
gdk_screen_get_height_mm(screen)) / millimetresPerInch;
> +
> +    return diagonalInPixels / diagonalInInches;
> +#else
> +    return defaultDPI;
> +#endif
> +}

This should probably be moved to PlatformScreenGtk.cpp adding a new method to
get the screen DPI.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:209
> +   
settings->priv->preferences->setDefaultFontSize(std::round(prevFontSizePx *
scalingFactor));
> +   
settings->priv->preferences->setDefaultFixedFontSize(std::round(prevMonospaceFo
ntSizePx * scalingFactor));

So, in the end we are changing the font settings in pixels, but we are not
sending the notify signal. We should check it actually changed and emit the
signal.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:227
> +#if PLATFORM(GTK)
> +    priv->gtkSettings = gtk_settings_get_default();
> +    if (priv->gtkSettings)
> +	   priv->gtkXftDpiChangedHandlerID =
g_signal_connect(priv->gtkSettings, "notify::gtk-xft-dpi",
G_CALLBACK(webKitSettingsGtkXftDpiChanged), WEBKIT_SETTINGS(object));
> +#endif

This could also be factored out in platform layer to avoid more ifdefs here,
maybe adding a PlatformScreenObserver or similar. Or even simpler, adding a
function to PlatformScreen to set a dpi observer
setScreenDPIObserverHandler(Function<void()>&&); that you can pass a lambda
here in the constructor and nullptr in dispose.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:886
> +	* Deprecated: 2.20. This property does not scale the text automatically
> +	* to account for screen DPI. Use default-font-size-pts instead.

This is not true after this patch. We are connecting to the dpi changed signal
in the constructor and updating this setting.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:2234
> +guint32 webkit_settings_get_default_font_size_pts(WebKitSettings* settings)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_SETTINGS(settings), 0);
> +
> +    return
convertPixelsToPoints(settings->priv->preferences->defaultFontSize(),
settings->priv->screenDPI);

Since we are updating the same thing underneath, I wonder why we need new
settings and deprecate the old ones. We could simply add these new functions to
set/get the same setting but using points instead of pixels.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitSettings.cpp:-152
> -    g_assert_cmpuint(webkit_settings_get_minimum_font_size(settings), ==,
7);

I think you can leave this here and add another specific test case for checking
the updates after the dpi change.