[Webkit-unassigned] [Bug 142673] [GTK] Automatically adjust font size when gtk-xft-dpi changes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=142673

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

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

I'm sorry to be late here. I have only a few of comments that can be fixed in a follow up.

> Source/WebCore/platform/PlatformScreen.h:30
> +#if USE(GLIB)
> +#include <wtf/Function.h>
> +#endif

I don't normally protect common header includes even if they are only used by one implementation, but don't bother to change this now that already landed.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:164
> +    WebCore::setScreenDPIObserverHandler(nullptr);

This doesn't work, because there can be multiple instances of WebKitSettings, it's the way we make web view groups, by sharing WebKitSettings objects. In this case, once one instance is deleted, all others will stop receiving notifications. We should either change PlatformScreen to receive a pair of Function, context and keep a map, or use a single handler here but notify all WebKitSetting instances. The former should be easier.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:182
> +        auto scalingFactor = newScreenDpi / settings->priv->screenDpi;

I would set the new dpi after this to ensure it's updated when notify signals are emitted.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:190
> +        auto prevFontSize = settings->priv->preferences->defaultFontSize();
> +        auto prevMonospaceFontSize = settings->priv->preferences->defaultFixedFontSize();
> +
> +        settings->priv->preferences->setDefaultFontSize(std::round(prevFontSize * scalingFactor));
> +        g_object_notify(G_OBJECT(settings), "default-font-size");
> +
> +        settings->priv->preferences->setDefaultFixedFontSize(std::round(prevMonospaceFontSize * scalingFactor));
> +        g_object_notify(G_OBJECT(settings), "default-monospace-font-size");

This is already done by webkit_settings_set_default_font_size() and webkit_settings_set_default_monospace_font_size(), so I would use those instead. I would also use g_object_freeze_notify()/thaw_notify().

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:193
> +    });

I think we should document this behavior in the properties and/or the public setters, the user should know that the values set by her will change automatically when screen dpi changes, and that it can be monitored using the notify signal.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171114/e4686168/attachment.html>