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

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

Applications should not need to manually handle changes in the gtk-xft-dpi setting. Currently every application that uses WebKitGTK+ must manually listen for changes in this setting and manually update the font size, or else the application will not respond properly to e.g. changes in the Large Text accessibility setting. WebKitGTK+ should handle this automatically.

If necessary, we may want to deprecate our existing font size settings and replace them with settings that expect sizes in em rather than px.

[Martin Robinson]

We decided to use pixels to maintain consistency with other WebKit ports. How likely is it that gtk-xft-dpi will change while WebKit is running?

[Michael Catanzaro]

> How likely is it that gtk-xft-dpi will change while WebKit is running?

The only reason I can think of is if people turn on the Large Text accessibility option, or Scaling Factor in GNOME Tweak Tool. The entire desktop responds immediately to that, except for a WebKitWebView. GNOME applications are getting patches to do so (see the See Also bugs; I guess maybe Endless wants this?), but it's a considerable amount of boilerplate code to share between each app.

But it's not just that: the px are also really silly difficult to use properly: https://git.gnome.org/browse/epiphany/tree/embed/ephy-embed-prefs.c#n206

[Mario Sanchez Prada]

(In reply to comment #2)
> > How likely is it that gtk-xft-dpi will change while WebKit is running?
> 
> The only reason I can think of is if people turn on the Large Text
> accessibility option, or Scaling Factor in GNOME Tweak Tool. The entire
> desktop responds immediately to that, except for a WebKitWebView.

Exactly, that's actually the use case that triggered the patches for epiphany, devhelp and yelp. Users of our system rely on quite a few WebKit based apps (including Yelp, but not only that one) to access to text-based content, and it was quite confusing for them not to get any instant feedback when the "Larger Text" a11y option was toggled.

Also, I've to say that even for me this has been an issue lately, since I normally use that a11y option myself whenever I move from working in my external 24" screen to working in my 14" 1920x1080 laptop (where I find everything a bit too small). 

> GNOME applications are getting patches to do so (see the See Also bugs; I
> guess maybe Endless wants this?), but it's a considerable amount of
> boilerplate code to share between each app.

That would definitely be interesting for us, although at the moment is not a big deal either since we have fixed the 2-3 places where this needs to be addressed (that is, is not that we had to fix trillions of apps, but Yelp + a WebKit based engine used from different apps).

Thus, I'm more open to help with this task although my understanding from our IRC conversation is that this was not going to be fixed in WebKit because of the pixels related issue, which is why I did not file the bug. But maybe I should have done it anyway... sorry about that and thanks Michael for stepping up and doing it. 

> But it's not just that: the px are also really silly difficult to use
> properly:
> https://git.gnome.org/browse/epiphany/tree/embed/ephy-embed-prefs.c#n206

True. It actually took me a while to understand what was going on there and where those "magic" numbers (25.4, 96, 72) came from.

[Michael Catanzaro]

https://bugzilla.gnome.org/show_bug.cgi?id=746166 in particular is a good example of why this needs to live in GTK+ -- there is a considerable amount of code in devhelp that tries to get the dpi right on a per-screen basis. Probably no other applications even attempt to do that right.

[Michael Catanzaro]

I mean, "why this needs to live in WebKit"

[Michael Catanzaro]

(In reply to comment #1)
> We decided to use pixels to maintain consistency with other WebKit ports.

It's tangential to this bug report, but this is something we should reconsider at the hackfest. Other ports are just wrong; the status quo is that apps using WebKit are all broken on hidpi unless the application copies a bunch of confusing boilerplate font size code out of Epiphany. We're sharing it in a bunch of GNOME apps now, but most apps don't have it and are broken. We need a simple font size setting that works independently of screen resolution, and should deprecate the current setting because it is too difficult to use properly.

[Michael Catanzaro]

I found a good summary of this issue that I wrote in a different bug, https://bugzilla.gnome.org/show_bug.cgi?id=774248#c18:

"WebKit requires applications compute text size manually based on the size of the monitor and pixel density. I wish I were kidding, but we just copy/paste the same 100 lines of code to do this among a bunch of different GNOME apps; each new app that uses WebKit doesn't know to do this and is broken until we figure it out, and it's easier to copy/paste than spend a day fixing WebKit and writing layout tests for it."

[Michael Catanzaro]

So this still needs to happen:

(In reply to Michael Catanzaro from comment #0)
> If necessary, we may want to deprecate our existing font size settings and
> replace them with settings that expect sizes in em rather than px.

The default size should be reasonable, so that most applications don't need to have any font size code at all.

Then, we need to update GNOME applications to use the new settings, and delete the 100 lines of copypaste font size setting code from each one.

[Gabriel Ivașcu]

Created attachment 325369 [details]
Patch

[Build Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Build Bot]

Attachment 325369 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:906:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:908:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:909:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:910:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:911:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:939:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:941:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:942:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:943:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:944:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
Total errors found: 10 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gabriel Ivașcu]

Hi,

Please check my proposed patch from comment 9.

I've added new font size properties in points and deprecated the old ones in pixels. Whenever the font size in points is updated, it is converted internally to pixels with respect to the screen DPI. This is similar with what WebKit1 had, only that now WebKitSettings also listens to gtk-xft-dpi changes and updates the font size internally when the DPI has changed.

I've tested this in Devhelp and I have a patch ready to use the new properties once this gets accepted.

This is my first WebKit patch, so please go easy on me :)

[Michael Catanzaro]

Thanks, I'll review it soon.

(In reply to Build Bot from comment #11)
> Attachment 325369 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:906:  When
> wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:908:  Weird
> number of spaces at line-start.  Are you using a 4-space indent? 
> [whitespace/indent] [3]
> ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:909:  Weird
> number of spaces at line-start.  Are you using a 4-space indent? 
> [whitespace/indent] [3]
> ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:910:  Weird
> number of spaces at line-start.  Are you using a 4-space indent? 
> [whitespace/indent] [3]
> ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:911:  Weird
> number of spaces at line-start.  Are you using a 4-space indent? 
> [whitespace/indent] [3]
> ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:939:  When
> wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:941:  Weird
> number of spaces at line-start.  Are you using a 4-space indent? 
> [whitespace/indent] [3]
> ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:942:  Weird
> number of spaces at line-start.  Are you using a 4-space indent? 
> [whitespace/indent] [3]
> ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:943:  Weird
> number of spaces at line-start.  Are you using a 4-space indent? 
> [whitespace/indent] [3]
> ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:944:  Weird
> number of spaces at line-start.  Are you using a 4-space indent? 
> [whitespace/indent] [3]
> WARNING: File exempt from style guide. Skipping:
> "Source/WebKit/UIProcess/API/gtk/WebKitSettings.h"
> Total errors found: 10 in 4 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

You'll have to follow WebKit code style for the property declarations too, even if it doesn't match the surrounding code:

    /**
     * WebKitSettings:default-font-size-pts:
     *
     * The default font size in points to use for content displayed if
     * no font size is specified.
     *
     * Since: 2.20
     */
    g_object_class_install_property(
        gObjectClass,
        PROP_DEFAULT_FONT_SIZE_PTS,
        g_param_spec_uint(
            "default-font-size-pts",
            _("Default font size in points"),
            _("The default font size in points used to display text."),
            5, G_MAXUINT, 12,
            readWriteConstructParamFlags));

[Michael Catanzaro]

One more pre-review comment. The EWS bots (the green and red bubbles above) try building your patch to look for problems. I see the GTK bubble is green, which is great, but the WPE bubble is red:

/home/ews/igalia-wpe-ews/WebKit/Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp: In function 'void webKitSettingsDispose(GObject*)':
/home/ews/igalia-wpe-ews/WebKit/Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:230:21: warning: unused variable 'settings' [-Wunused-variable]

You'll need to move the declaration down into the #if PLATFORM(GTK) block you added below to avoid this warning for WPE. It's C++ so the code style for declarations is different: you should limit the scope of variables by declaring them only where they're actually required, rather than at the top of functions.

/home/ews/igalia-wpe-ews/WebKit/Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp: In function 'void webKitSettingsSetProperty(GObject*, guint, const GValue*, GParamSpec*)':
/home/ews/igalia-wpe-ews/WebKit/Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:309:84: error: 'webkit_settings_set_default_font_size_pts' was not declared in this scope
/home/ews/igalia-wpe-ews/WebKit/Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:315:94: error: 'webkit_settings_set_default_monospace_font_size_pts' was not declared in this scope
/home/ews/igalia-wpe-ews/WebKit/Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp: In function 'void webKitSettingsGetProperty(GObject*, guint, GValue*, GParamSpec*)':
/home/ews/igalia-wpe-ews/WebKit/Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:489:83: error: 'webkit_settings_get_default_font_size_pts' was not declared in this scope
/home/ews/igalia-wpe-ews/WebKit/Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:495:93: error: 'webkit_settings_get_default_monospace_font_size_pts' was not declared in this scope

I forgot to tell you that there are separate WebKitSettings.h files for GTK and WPE, and they have to be updated in tandem. I know that's yucky, but the only alternative is to generate the headers from a common template, and we haven't gone down that road yet. So you'll have to take care of that too.

[Gabriel Ivașcu]

Created attachment 325377 [details]
Patch

[Gabriel Ivașcu]

(In reply to Michael Catanzaro from comment #14)
> You'll need to move the declaration down into the #if PLATFORM(GTK) block
> you added below to avoid this warning for WPE. It's C++ so the code style
> for declarations is different: you should limit the scope of variables by
> declaring them only where they're actually required, rather than at the top
> of functions.
> 
> I forgot to tell you that there are separate WebKitSettings.h files for GTK
> and WPE, and they have to be updated in tandem. I know that's yucky, but the
> only alternative is to generate the headers from a common template, and we
> haven't gone down that road yet. So you'll have to take care of that too.

OK, I fixed this and the coding style in attachment 325377 [details]

[Michael Catanzaro]

Comment on attachment 325377 [details]
Patch

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

It's pretty good for your first patch! Almost all of my comments are for minor issues, and I snuck in a couple programming quizzes (which I wouldn't ordinarily do in a patch review, but it seemed useful here). But I see two significant problems.

(1) I'm afraid I forgot to mention that API changes are dead on arrival without accompanying API tests. We're a lot stricter about tests in WebKit than we are in Epiphany; WebKit would be almost totally unmaintainable if not for the tens of thousands of tests. Most of them live under the LayoutTests directory, but API tests live under Tools/TestWebKitAPI/Tests. Normally writing a good API test is just as hard or harder than adding the new API, but in this case you'll actually have it pretty easy as with settings we normally just test the default value of the setting, and check that the value changes when you change it. You'll want to edit Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitSettings.cpp to add simple checks for the new functions: basically, just copy the existing ones and edit as appropriate. To run the API tests, normally I would tell you to use the script Tools/Scripts/run-gtk-tests, but that only really works if you build WebKit using build-webkit, whereas you're using GNOME JHBuild. So instead, the easiest way to run the tests is to cd into the build directory, WebKitBuild/(whateveryounamedit)/bin/TestWebKitAPI/WebKit2Gtk/, and then jhbuild run ./TestWebKitSettings. Use -l to list the possible tests and -p to run just one single test.

(2) Your *_get_font_size_pts() functions only return a reasonable result if you first call the corresponding *_set_font_size_pts() function. Instead, they should do the right thing even if the user has only ever called the old *_set_font_size() function. So instead of saving the size in points in the priv struct, you'll actually need to do the reverse computation to change from pixels back to points. I'm not sure, but you might need to use round(). Before you fix this, add tests for this, make sure the tests fail, and then write the implementation and make sure the tests pass.

> Source/WebKit/ChangeLog:3
> +        [GTK] Automatically adjust font size when gtk-xft-dpi changes

I've renamed this bug to have a more descriptive title. When you run prepare-ChangeLog again, it will pick up the new title. (You'll have to delete the old ChangeLog entries you already wrote, of course.)

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:80
> +    guint32 defaultFontSizePts { 12 };
> +    guint32 defaultMonospaceFontSizePts { 10 };

Wow, the existing font size settings really do use guint32 rather than guint. OK then. I was going to tell you to use guint or unsigned instead, but this is really the best type here!

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

OK, I don't normally write review comments for stuff that you've done correctly, especially twice in a row, but this line of code is an educational opportunity! Homework question: why is this initialization to 0 here needed, if in normal GObject code the priv struct is guaranteed to be zero-initialized for you? You wouldn't need the initialization here in Epiphany, since it would be guaranteed to be 0 already.

Hint: Look in WTFGType.h.

(Take a minute or two to look before you read below. Spoiler alert!)

Answer: It's constructed (in the instance init) using a placement new. Three consequences of that. (1) In WebKit, new is overridden to use WebKit's custom memory allocator bmalloc, which is faster than glibc's malloc. (2) It means the priv struct is a C++ object, so it can have constructors and destructors that will actually run. (3) C++ construction rules come into play... I'm not completely sure, but I think the state of the variable would be undefined if you didn't explicitly initialize it here.

If you poke around further in WTFGType.h, you'll notice that finalize is defined there too, to call the priv struct's destructor. So in WebKit GObject implementations, you don't define either the instance init or the finalize function like you would in normal GObject implementations. Instead you have to use constructors and destructors. (You still have dispose, though.)

P.S. In WebKit we strongly prefer inline member initialization, { 0 } style, rather than using constructor initializer lists.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:173
> +    double dpi, diagonalInPixels, diagonalInInches;
> +    GdkScreen* screen;

This is C++, so these variables should not be declared up here. Better to declare them at the point of first use.

Actually, this is good practice to do in modern C code as well. But GNOME stuff was written for C89, where variables had to be declared at the top of functions, which is why you're used to the opposite.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:175
> +    screen = gdk_screen_get_default();

GdkScreen* screen = gdk_screen_get_default();

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:180
> +    dpi = gdk_screen_get_resolution(screen);

double dpi = ...;

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:186
> +    diagonalInPixels = hypot(gdk_screen_get_width(screen), gdk_screen_get_height(screen));
> +    diagonalInInches = hypot(gdk_screen_get_width_mm(screen), gdk_screen_get_height_mm(screen)) / millimetresPerInch;

You should #include <cmath> at the top of the file to pull this in explicitly. Who knows how it's getting included right now.

Also, though I'm pretty sure it doesn't matter in this case because the return type should be double either way: we sometimes have subtle type bugs caused by mixing up C and C++ math functions. So I would explicitly call std::hypot here. No reason not to.

Also, declare them here too, not up above.

Lastly, I'll mention that using GdkScreen rather than GdkMonitor to get the screen dimensions is deprecated, but since we don't want to bump our GTK+ version requirement, I agree it's much better to use the old API unconditionally rather than add #if GTK_CHECK_VERSION() conditionals to choose between the two. So no need to change that. We can switch to GdkMonitor in the future if needed.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:193
> +static inline guint32 convertFontSizeToPixels(guint32 fontSize, double dpi)

The result of this function is being used to call internal API, not external API, so use uint32_t rather than guint32 (which is only appropriate for use in the public GObject API).

Normally we would write "unsigned" rather than uint32_t, but in this case, if you look in WebPreferences.h you'll see that the type used there is uint32_t. So best match that.

And yes, it's of course guaranteed to be the same as guint32. So this comment is somewhat pedantic. But it's a style we try to follow.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:202
> +    double dpi;
> +    int gtkXftDpi;

Declare these below, at the point of first use.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:205
> +    // gtk-xft-dpi uses -1 for default value.
> +    g_object_get(gtkSettings, "gtk-xft-dpi", &gtkXftDpi, NULL);

gint gtkXftDpi;
g_object_get(gtk_settings, "gtk-xft-dpi", &gtkXftDpi, nullptr);

Note: I used gint, rather than int, because that's the type of the gtk-xft-dpi property. Pedantic, doesn't really matter, but good to do.

Also note that I used nullptr rather than NULL. Always use nullptr in WebKit (and other modern C++ code), never NULL. It avoids weird corner case bugs.

Now, here is another homework question. If this were C code (not actually sure about C++, and doesn't matter much because C++ has nullptr), then you would technically need to use a casted (char *)NULL here rather than just plain NULL. We use this idiom all the time in GNOME C code without the cast, but that actually relies on an implementation detail of glibc and can blow up badly (and has blown up badly!) on embedded systems with alternate C libraries. Why, in C, does it need the cast? (Hint: the C library has some latitude in how it defines NULL.) (P.S. Don't actually start adding the cast in your GNOME code, because other GNOME code does not use the cast: there would be no point. But it's good to know about this stuff!)

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:234
> +        if (priv->gtkSettings && g_signal_handler_is_connected(priv->gtkSettings, priv->gtkXftDpiChangedHandlerID))

Huh. Didn't know about g_signal_handler_is_connected(). But you do not need to use it here, because priv->gtkXftDpiChangedHandlerID will be 0 if it's not connected.

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

True. You might instead write "This property does not scale the text automatically to account for screen DPI." That might be a bit more useful, because accounting for DPI *changes* is something that application developers rarely care to think about, but ensuring it works properly on hidpi screens in the first place really is important to application developers.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:900
> +     * The default font size in points to use for content displayed if

Add commas: The default font size, in points, ...

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:912
> +            5, G_MAXUINT, 12,

How do you know 5 is the right number to use for the minimum size? Are 4-point fonts not allowed?

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

Same comment here.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:935
> +     * The default font size in points to use for content displayed in

And here.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:2191
> +    g_warning("The property WebKitSettings:default-font-size is deprecated and should not be used anymore.");

This should go directly in webKitSettingsSetProperty so that the warning only prints if the GObject property is used directly. Programmers don't need runtime warnings about using deprecated functions, because the compiler will warn them, which is better. Runtime warnings are only needed for the GObject properties because there's no way to get a compile warning.

Incidentally, this will make your life easier when writing tests, too, because we only test the functions and just assume the properties are implemented properly. (Probably the wrong way around, actually, but don't worry about that now: stick to the existing style for the tests.) And if the test emits a warning, you'll have to disable warning traps to ensure the warning does not cause the test to blow up. I'm just mentioning this so you don't waste time trying to figure out how to do that in case you were to try working on the tests before you move the warning.

Also, I would add to the text of warning: "Use WebKitSettings:default-font-size-pts instead."

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:2208
> +    g_warning("The property WebKitSettings:default-font-size is deprecated and should not be used anymore.");

Ditto

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:2254
> +    g_object_notify(G_OBJECT(settings), "default-font-size-pts");

Hmm... this is a preexisting problem that you should not attempt to fix in this patch, but I would appreciate it if you file a bug describing it: because the properties in this file are not declared with G_PARAM_EXPLICIT_NOTIFY, if the property is set using g_object_set() rather than webkit_settings-set_default_font_size_pts(), I think the notify will occur twice: first here, and then again by GObject. That has caused bugs in GNOME apps in the past, and it's why G_PARAM_EXPLICIT_NOTIFY is good! (Prefix the bug title with [WPE][GTK] and select either the GTK or WPE Bugzilla component, otherwise nobody will notice you reported it.)

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:2270
> +    g_warning("The property WebKitSettings:default-monospace-font-size is deprecated and should not be used anymore.");

Move it

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:2282
> + * Deprecated: 2.20. Use webkit_settings_set_default_monospace_font_size_pts() instead.

Ditto

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:2287
> +    g_warning("The property WebKitSettings:default-monospace-font-size is deprecated and should not be used anymore.");

Ditto

[Gabriel Ivașcu]

(In reply to Michael Catanzaro from comment #17)
> Comment on attachment 325377 [details]
> Patch
>  
> (2) Your *_get_font_size_pts() functions only return a reasonable result if
> you first call the corresponding *_set_font_size_pts() function. Instead,
> they should do the right thing even if the user has only ever called the old
> *_set_font_size() function. So instead of saving the size in points in the
> priv struct, you'll actually need to do the reverse computation to change
> from pixels back to points. I'm not sure, but you might need to use round().
> Before you fix this, add tests for this, make sure the tests fail, and then
> write the implementation and make sure the tests pass.

This won't work correctly when the screen DPI changes. We need to save either the "initial" font size in points or the previous DPI to correctly calculate the new font size in pixels for the new DPI. I'll explain below.

The formulas for doing the conversions are:

(1) Pixels = Points * DPI / 72;
(2) Points = Pixels * 72 / DPI;

The 72 comes from the fact that 1 point is defined as 1/72 inches.

If we don't store Points anywhere, then that's fine only when the screen DPI remains constant. Because we can always convert back from Pixels to Points knowing the DPI. However, if the screen DPI varies, and we don't store the previous DPI either, then we can't convert back to the font size in points of the initial DPI, because we only know the current DPI which is the new one, and thus we cannot apply the scaling factor.

Here's an example:

Suppose that the initial DPI is 96 and the font size is 12 points, i.e. 16 pixels at this DPI. We only save the size in pixels. Then the DPI changes to 80, for example. Knowing only the current font size of 16 pixels and the current DPI of 80, we cannot really calculate the new font size value in pixels. Because we have no way to tell if those 16 pixels initially came from a font size of 12 points at 96 DPI. They might as well come from a font size of 8 points at 144 DPI, for example.

We can do this right only if:

A. We know that the initial font size in points was 12. Then formula (1) is used directly to calculate the new font size in pixels: 12 * 72 / 80 == 13.3 pixels. That's what my patch does now.

B. We know that the initial DPI was 96. Then both formulas (1) and (2) are used, first (2) to get the initial font size in points, and then (1) to calculate the new font size in pixels: (16 * 72 / 96) * 80 / 72 == 13.3 pixels. Here the scaling factor is basically the current DPI divided by the previous DPI, which is 80/96 in this case.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #17)
> Answer: It's constructed (in the instance init) using a placement new. Three
> consequences of that. (1) In WebKit, new is overridden to use WebKit's
> custom memory allocator bmalloc, which is faster than glibc's malloc.

Actually (1) is totally wrong... the point of placement new is that it constructs the object using previously-allocated memory. Here the memory is actually allocated (and zeroed) by GObject. I'm not sure whether or not it's undefined behavior to not explicitly initialize all the struct members.

[Michael Catanzaro]

Also, for future reference, I just rediscovered the G_PARAM_DEPRECATED flag, which is the right way to deprecate GObject properties.

[Gabriel Ivașcu]

Created attachment 325442 [details]
Patch

[Gabriel Ivașcu]

The patch in attachment 325442 [details] uses approach B. described in comment 18 to calculate the new font size values when the screen DPI changes. This simplifies things a bit.

I've also fixed the things mentioned in the previous patch review.

[Michael Catanzaro]

Comment on attachment 325442 [details]
Patch

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

The code changes are looking good. The tests need a bit more work, though.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:182
> +    double diagonalInPixels = std::hypot((double)gdk_screen_get_width(screen), (double)gdk_screen_get_height(screen));
> +    double diagonalInInches = std::hypot((double)gdk_screen_get_width_mm(screen), (double)gdk_screen_get_height_mm(screen)) / millimetresPerInch;

Is casting the parameters really required? I thought it would not be. If so, best go back to unqualified hypot, because that's a mess. And it will be even more of a mess once you fix the casting style. ;) In C++ code you should use C++ casts (static_cast, dynamic_cast, const_cast, and reinterpret_cast) rather than the old C-style casts. In WebKit you can't use dynamic_cast, because we compile with -fno-rtti, but the other three should be used in whatever combinations necessary. Here you want static_cast.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitSettings.cpp:147
> +    // Default font size in points is 12.
> +    g_assert_cmpuint(webkit_settings_get_default_font_size_pts(settings), ==, 12);
> +    webkit_settings_set_default_font_size_pts(settings, 10);
> +    g_assert_cmpuint(webkit_settings_get_default_font_size_pts(settings), ==, 10);

I'd like to see a little bit more on the tests. You should test the interaction of the new functions with the old functions. Check the value of webkit_settings_get_default_font_size_pts() before the call to webkit_settings_set_default_font_size(), so that you have a test for the default default font size. ;) Then clarify the comment, since it's confusing that the default font size in pixels was changed, but the size in points is still the same (I guess because the change was not very much?). And test to ensure a larger change in the default pixel font size causes a corresponding change in the default points font size, and also vice-versa.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitSettings.cpp:157
> +    // Default monospace font size in points is 10.
> +    g_assert_cmpuint(webkit_settings_get_default_font_size_pts(settings), ==, 10);
> +    webkit_settings_set_default_font_size_pts(settings, 8);
> +    g_assert_cmpuint(webkit_settings_get_default_font_size_pts(settings), ==, 8);

Same here. Also: whoops, this isn't testing the monospace font size!

[Gabriel Ivașcu]

(In reply to Michael Catanzaro from comment #23)
> > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:182
> > +    double diagonalInPixels = std::hypot((double)gdk_screen_get_width(screen), (double)gdk_screen_get_height(screen));
> > +    double diagonalInInches = std::hypot((double)gdk_screen_get_width_mm(screen), (double)gdk_screen_get_height_mm(screen)) / millimetresPerInch;
> 
> Is casting the parameters really required? I thought it would not be. If so,
> best go back to unqualified hypot, because that's a mess. And it will be
> even more of a mess once you fix the casting style. ;) In C++ code you
> should use C++ casts (static_cast, dynamic_cast, const_cast, and
> reinterpret_cast) rather than the old C-style casts. In WebKit you can't use
> dynamic_cast, because we compile with -fno-rtti, but the other three should
> be used in whatever combinations necessary. Here you want static_cast.

Since there's no overload of std::hypot that takes two ints, I wasn't sure which version of std::hypot C++ will call. We'd prefer the one that has doubles for parameters for reasons of precision, so that's why I added the casts.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitSettings.cpp:147
> > +    // Default font size in points is 12.
> > +    g_assert_cmpuint(webkit_settings_get_default_font_size_pts(settings), ==, 12);
> > +    webkit_settings_set_default_font_size_pts(settings, 10);
> > +    g_assert_cmpuint(webkit_settings_get_default_font_size_pts(settings), ==, 10);
> 
> I'd like to see a little bit more on the tests. You should test the
> interaction of the new functions with the old functions. Check the value of
> webkit_settings_get_default_font_size_pts() before the call to
> webkit_settings_set_default_font_size(), so that you have a test for the
> default default font size. ;) Then clarify the comment, since it's confusing
> that the default font size in pixels was changed, but the size in points is
> still the same (I guess because the change was not very much?). And test to
> ensure a larger change in the default pixel font size causes a corresponding
> change in the default points font size, and also vice-versa.

OK.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitSettings.cpp:157
> > +    // Default monospace font size in points is 10.
> > +    g_assert_cmpuint(webkit_settings_get_default_font_size_pts(settings), ==, 10);
> > +    webkit_settings_set_default_font_size_pts(settings, 8);
> > +    g_assert_cmpuint(webkit_settings_get_default_font_size_pts(settings), ==, 8);
> 
> Same here. Also: whoops, this isn't testing the monospace font size!

Right, I forgot to change the function name after I copy-pasted the line from before.

[Michael Catanzaro]

(In reply to Gabriel Ivașcu from comment #24)
> Since there's no overload of std::hypot that takes two ints, I wasn't sure
> which version of std::hypot C++ will call. We'd prefer the one that has
> doubles for parameters for reasons of precision, so that's why I added the
> casts.

cppreference is your friend:

http://en.cppreference.com/w/cpp/numeric/math/hypot

I think, without the casts, you'll get the fourth overload, since it's being called with int parameters. And that says:

"4) A set of overloads or a function template for all combinations of arguments of arithmetic type not covered by (1-3). If any argument has integral type, it is cast to double. If any other argument is long double, then the return type is long double, otherwise it is double."

so it will be double even without the casts, and the casts aren't needed. Could you check?

[Gabriel Ivașcu]

(In reply to Michael Catanzaro from comment #25)
> (In reply to Gabriel Ivașcu from comment #24)
> > Since there's no overload of std::hypot that takes two ints, I wasn't sure
> > which version of std::hypot C++ will call. We'd prefer the one that has
> > doubles for parameters for reasons of precision, so that's why I added the
> > casts.
> 
> cppreference is your friend:
> 
> http://en.cppreference.com/w/cpp/numeric/math/hypot
> 
> I think, without the casts, you'll get the fourth overload, since it's being
> called with int parameters. And that says:
> 
> "4) A set of overloads or a function template for all combinations of
> arguments of arithmetic type not covered by (1-3). If any argument has
> integral type, it is cast to double. If any other argument is long double,
> then the return type is long double, otherwise it is double."
> 
> so it will be double even without the casts, and the casts aren't needed.
> Could you check?

Yes, you're right, I was going to comment on this myself. The integral types are converted to double by default, so there's no need for explicit conversion.

[Gabriel Ivașcu]

Created attachment 325462 [details]
Patch

[Michael Catanzaro]

Comment on attachment 325462 [details]
Patch

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

OK, good, this passes my review. Only a couple minor comments remaining.

Now, because this patch adds new public API, we have a rule that it has to be approved by two GTK reviewers. I'm sure Carlos Garcia will want to be the second reviewer, so please ping him tomorrow and ask.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitSettings.cpp:47
> +    // Set a DPI value of 96 for the test. This must be set before creating
> +    // the WebKitSettings object so that the notify::gtk-xft-dpi signal handler
> +    // of WebKitSettings won't get called to modify the initial font size values.
> +    GtkSettings* gtkSettings = gtk_settings_get_default();
> +    if (gtkSettings)
> +        g_object_set(gtkSettings, "gtk-xft-dpi", 96 * 1024, nullptr);

You need #if PLATFORM(GTK) guards around this, to avoid breaking WPE.

Also: I would add a second call to this down at the end of the test, and then verify that the font size in pixels changes when you change the DPI, but the font size in points does not.

Lastly, since the font size tests are now more complicated than just setting a value and verifying that it's changed, I would add a new test case function testFontSettings and split all of this out of testWebKitSettings.

[Gabriel Ivașcu]

Created attachment 325468 [details]
Patch

[Gabriel Ivașcu]

Created attachment 325469 [details]
Patch

[Gabriel Ivașcu]

(In reply to Gabriel Ivașcu from comment #30)
> Created attachment 325469 [details]
> Patch

This is the same as attachment 325468 [details], only that it fixes a typo in a comment from the old code that I've moved to a new function in TestWebKitSettings.cpp.

[Carlos Garcia Campos]

Comment on attachment 325469 [details]
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(prevMonospaceFontSizePx * 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.

[Gabriel Ivașcu]

(In reply to Carlos Garcia Campos from comment #32)
> Comment on attachment 325469 [details]
> 
> > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:83
> > +    gulong gtkXftDpiChangedHandlerID { 0 };
> 
> This is already set to 0 on allocation.

Hmmm Michael explicitly said that the fields in the private structure are not guaranteed to be zero-initialized.
 
> > 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.

I think keeping the old properties in pixels and only adding font size setter/getter in points partially defeats the purpose of this bug. One of the goals was to make users not have to handle pixel values anymore, by getting rid of the pixel properties eventually in the future. (The other goal was to make the text scale automatically when the DPI changes.)

> > 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.

The minimum font size is not altered by the DPI change, it remains the same.

[Carlos Garcia Campos]

(In reply to Gabriel Ivașcu from comment #33)
> (In reply to Carlos Garcia Campos from comment #32)
> > Comment on attachment 325469 [details]
> > 
> > > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:83
> > > +    gulong gtkXftDpiChangedHandlerID { 0 };
> > 
> > This is already set to 0 on allocation.
> 
> Hmmm Michael explicitly said that the fields in the private structure are
> not guaranteed to be zero-initialized.

The private struct is allocated by glib as part of the object instance struct initialization using g_malloc0().
 
> > > 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.
> 
> I think keeping the old properties in pixels and only adding font size
> setter/getter in points partially defeats the purpose of this bug. One of
> the goals was to make users not have to handle pixel values anymore, by
> getting rid of the pixel properties eventually in the future. (The other
> goal was to make the text scale automatically when the DPI changes.)

Two goals, two bugs and patches then :-)

> > > 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.
> 
> The minimum font size is not altered by the DPI change, it remains the same.

I meant the whole block, not just that particular line.

[Michael Catanzaro]

(In reply to Gabriel Ivașcu from comment #33)
> Hmmm Michael explicitly said that the fields in the private structure are
> not guaranteed to be zero-initialized.

But see my next comment #19 where I corrected that to "I'm not sure". I think it's technically default-constructed, which is effectively the same. But certainly the fact that the struct is initially zero-initialized has no effect on its value after construction.

> I think keeping the old properties in pixels and only adding font size
> setter/getter in points partially defeats the purpose of this bug. One of
> the goals was to make users not have to handle pixel values anymore, by
> getting rid of the pixel properties eventually in the future. (The other
> goal was to make the text scale automatically when the DPI changes.)

But you don't have to handle pixels anymore if you use the functions. There's not really any compelling reason that the properties have to be used directly; it's just a bit more convenient when initializing the WebKitSettings object.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #35)
> (In reply to Gabriel Ivașcu from comment #33)
> > Hmmm Michael explicitly said that the fields in the private structure are
> > not guaranteed to be zero-initialized.
> 
> But see my next comment #19 where I corrected that to "I'm not sure". I
> think it's technically default-constructed, which is effectively the same.

No, that's wrong too. The internet says the members are just uninitialized.

> But certainly the fact that the struct is initially zero-initialized has no
> effect on its value after construction.

I might be wrong about that too... I guess the values are undefined, and it just always works on all compilers because compilers don't touch the memory that's already been zeroed. But I'm not sure; maybe it is safe to do.

[Gabriel Ivașcu]

OK, as Carlos pointed out, I'll split this into two separate bugs and patches:

(1) Add functionality to handle font sizes in points.
(2) Handle DPI changes and scale the text accordingly.

I suggest keeping this bug for (2), as that was its original purpose, but for that I need to ask Michael to change the bug's name back to the original, since I don't have enough permissions.

And I'll open a separate bug for (1) with a smaller patch for it. But for this I'm not sure if we should fall back to the alternative design, where we keep the old font size properties in pixels, and only add setter/getter methods for the font size in points. I believe this isn't really the best approach, for reasons of usability and consistency.

I'll try to highlight below why I favor adding points properties and deprecating the pixels properties (which should be eventually removed for good):

A. It's a plus to consistency. Currently, "minimum-font-size" property is in points, whereas "default-font-size" and "default-monospace-font-size" are in pixels, which is already not too consistent. Splitting the default font size properties in both pixels and points, will break the consistency even more, whereas replacing the pixels properties with points properties will bring unity.

B. I think users should have to deal with raw pixels. And it's easier for them to understand that they are supposed to use only the points properties/methods from now on if they see that the pixels properties/methods are deprecated and will be removed in a future version. This is in contrast with keeping the pixels properties/methods and only adding points methods, which can be misleading and confusing: changing the pixels value will change the points value too and vice versa. (Of course, this would happen too if they would use both the new properties and the deprecated ones, but that would be their fault.)

C. It's easier for apps to handle font sizes in points rather than pixels, i.e. they can pass to WebKitSettings the font size value obtained directly from the user preferences. Furthermore, what apps currently do, is to set multiple font properties at once with g_object_set(). See https://git.gnome.org/browse/epiphany/tree/embed/ephy-embed-prefs.c?h=gnome-3-26#n354 and https://git.gnome.org/browse/devhelp/tree/src/dh-util.c?h=gnome-3-24#n168. Switching from pixels properties to points properties will only require changing the property names, whereas keeping the pixels properties and only adding points methods, will require calling the setter for points in addition to g_object_set().

If I'm missing something and you strongly believe that the font size properties in pixels should not be deprecated and/or removed, please let me know, so I would know what patch to upload to (1).

[Gabriel Ivașcu]

(In reply to Gabriel Ivașcu from comment #37) 
> B. I think users should have to deal with raw pixels. 

*should NOT have

[Carlos Garcia Campos]

(In reply to Gabriel Ivașcu from comment #37)
> OK, as Carlos pointed out, I'll split this into two separate bugs and
> patches:
> 
> (1) Add functionality to handle font sizes in points.
> (2) Handle DPI changes and scale the text accordingly.

Perfect, thanks!

> I suggest keeping this bug for (2), as that was its original purpose, but
> for that I need to ask Michael to change the bug's name back to the
> original, since I don't have enough permissions.
> 
> And I'll open a separate bug for (1) with a smaller patch for it. But for
> this I'm not sure if we should fall back to the alternative design, where we
> keep the old font size properties in pixels, and only add setter/getter
> methods for the font size in points. I believe this isn't really the best
> approach, for reasons of usability and consistency.
> 
> I'll try to highlight below why I favor adding points properties and
> deprecating the pixels properties (which should be eventually removed for
> good):
> 
> A. It's a plus to consistency. Currently, "minimum-font-size" property is in
> points, whereas "default-font-size" and "default-monospace-font-size" are in
> pixels, which is already not too consistent.

Yes, I didn't even know minimum-font-size was in points.

> Splitting the default font size
> properties in both pixels and points, will break the consistency even more,
> whereas replacing the pixels properties with points properties will bring
> unity.

That would still be inconsistent because new properties in points will have the -pts suffix, but minimum-font-size will be in point without the -pts suffix. What I don't like about adding new properties is that changing one property will affect two properties, because of both are changing the same internal value. They are not two properties but two ways of expressing the same property. So, adding methods to set the same property in other units makes it clear that we are changing the same thing.

> B. I think users should have to deal with raw pixels. And it's easier for
> them to understand that they are supposed to use only the points
> properties/methods from now on if they see that the pixels
> properties/methods are deprecated and will be removed in a future version.

I'm not sure that's always the case. In a GNOME desktop environment with gnome-settings-daemon, and GTK+ providing accurate information about the DPI, that's for sure the case. But in a embedded device, with not desktop and not using any toolkit at all (WPE), that targets a specific screen where you know the DPI, you might want to move the conversions to the app side instead of using the default one provided by WebCore.

> This is in contrast with keeping the pixels properties/methods and only
> adding points methods, which can be misleading and confusing: changing the
> pixels value will change the points value too and vice versa. (Of course,
> this would happen too if they would use both the new properties and the
> deprecated ones, but that would be their fault.)

I'm not sure I get what you mean here, the fact that changing one property makes another one change too is what I find confusing here.

> C. It's easier for apps to handle font sizes in points rather than pixels,
> i.e. they can pass to WebKitSettings the font size value obtained directly
> from the user preferences.

I agree, and again that's the case of desktop applications in a desktop environment with system preferences.

> Furthermore, what apps currently do, is to set
> multiple font properties at once with g_object_set(). See
> https://git.gnome.org/browse/epiphany/tree/embed/ephy-embed-prefs.c?h=gnome-
> 3-26#n354 and
> https://git.gnome.org/browse/devhelp/tree/src/dh-util.c?h=gnome-3-24#n168.
> Switching from pixels properties to points properties will only require
> changing the property names, whereas keeping the pixels properties and only
> adding points methods, will require calling the setter for points in
> addition to g_object_set().

Yes, I agree it's more convenient to use g_object_set() in that case, but I don't think it's enough reason. We could add global functions to convert between pixels/points instead of making them part of WebKitSettings, that way you could keep using g_object_set().

> If I'm missing something and you strongly believe that the font size
> properties in pixels should not be deprecated and/or removed, please let me
> know, so I would know what patch to upload to (1).

I still think it's better to not duplicate the properties for the reasons I've just exposed, but that doesn't mean the decision is made, so don't change the patch yet.

[Gabriel Ivașcu]

(In reply to Carlos Garcia Campos from comment #39) 
> That would still be inconsistent because new properties in points will have
> the -pts suffix, but minimum-font-size will be in point without the -pts
> suffix. What I don't like about adding new properties is that changing one
> property will affect two properties, because of both are changing the same
> internal value. They are not two properties but two ways of expressing the
> same property. So, adding methods to set the same property in other units
> makes it clear that we are changing the same thing.

I understand what you're saying. My reasoning was that:

1) Users would not use both set of properties at the same time, since the first set would be deprecated.

2) Eventually the pixels properties will be completely removed in a future version, and then the points properties will take their place and so there will be only one set of properties.
 
> I'm not sure that's always the case. In a GNOME desktop environment with
> gnome-settings-daemon, and GTK+ providing accurate information about the
> DPI, that's for sure the case. But in a embedded device, with not desktop
> and not using any toolkit at all (WPE), that targets a specific screen where
> you know the DPI, you might want to move the conversions to the app side
> instead of using the default one provided by WebCore.

I admit, I only took into consideration the case of a desktop environment. You definitely have more knowledge about this stuff.

> I agree, and again that's the case of desktop applications in a desktop
> environment with system preferences.

Ditto.
 
> Yes, I agree it's more convenient to use g_object_set() in that case, but I
> don't think it's enough reason. We could add global functions to convert
> between pixels/points instead of making them part of WebKitSettings, that
> way you could keep using g_object_set().

In either design we choose, apps would not really need their own functions to convert between points and pixels anymore. That implies calculating the screen DPI, and WebCore would already be doing that in PlatformScreen. Apps would use the setter method for points and the conversion will be made internally by WebKitSettings with the DPI from WebCore.
 
> I still think it's better to not duplicate the properties for the reasons
> I've just exposed, but that doesn't mean the decision is made, so don't
> change the patch yet.

OK, since I didn't really consider the case of embedded devices where apps might want to do their own conversions, I think we can keep the properties in pixels and only add methods for font sizes in points that the desktop environment apps could use.

Maybe Michael can have the final say on this?

[Michael Catanzaro]

> Maybe Michael can have the final say on this?

Wrong way around: I'll defer to Carlos. But let's discuss a bit more.

I was almost going to ask you to implement Carlos's suggestion of having global functions to convert between pixels and points and vice-versa (also for GTK only). We don't currently have any global functions in our API, but we have another use-case for one in bug #174816, so there's no harm in adding them. This will make it easier to set the font size using g_object_set() while keeping the original points properties. But I eventually decided that's a bad idea, because of your argument regarding the disparity between having the existing minimum-font-size in points and the other two font sizes in pixels. That's confusing, and really annoying me. (Does minimum-font-size even work properly? How could it, if WebKit was not able to convert between points and pixels?)

I think our solution should fix that. And the only way to do that is to deprecate the existing properties. So my preference is still to stick with the original plan, deprecate all the existing pixels stuff and add the points versions. I don't agree with Carlos that it's bad to have multiple properties that change at the same time; it makes sense in certain situations, such as GTlsCertificate where we provide separate DER and PEM properties to get the certificate in different formats. I don't think that's confusing. And in this case, we'll actually only have one non-deprecated way to get the settings, which is relevant because the deprecated ones will eventually go away. So I think that's fine. In the future, when we add the GTK+ 4 API, we can remove the original pixels versions (from the new API) and then drop the -pts from the names (in the new API), addressing the naming disparity. So this is right for the long term. It'll require some #if conditions, but I think it's the best approach.

Now, for WPE, I agree with Carlos that it probably makes the most sense to stick with pixels instead of points, at least for now, because (to my knowledge) WPE does not expose any monitor DPI information, so the points settings cannot ever really work properly. I propose that we just accept that WPE and GTK need different API here. So let's guard the new API with #if PLATFORM(GTK), guard the G_PARAM_DEPRECATED use as well, remove the new points API from the WPE header file, and not deprecate the pixels API for WPE. It'll require even more conditions, which will be a bit messy, but it's the right thing to do IMO.

Carlos, does that sound OK?

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #41)
> > Maybe Michael can have the final say on this?
> 
> Wrong way around: I'll defer to Carlos. But let's discuss a bit more.
> 
> I was almost going to ask you to implement Carlos's suggestion of having
> global functions to convert between pixels and points and vice-versa (also
> for GTK only). We don't currently have any global functions in our API, but
> we have another use-case for one in bug #174816, so there's no harm in
> adding them. This will make it easier to set the font size using
> g_object_set() while keeping the original points properties. But I
> eventually decided that's a bad idea, because of your argument regarding the
> disparity between having the existing minimum-font-size in points and the
> other two font sizes in pixels. That's confusing, and really annoying me.
> (Does minimum-font-size even work properly? How could it, if WebKit was not
> able to convert between points and pixels?)

It turns out that it's a bug in the documentation, minimum font size is no different to other font size settings and it's in pixels, which is what WebCore expects. There's also minimum logical size that we used to expose in WebKit1, but never exposed in WebKit2, I guess because I didn't find any application using it.

> I think our solution should fix that. And the only way to do that is to
> deprecate the existing properties.

The actual fix is removing the points thing from the documentation.

> So my preference is still to stick with
> the original plan, deprecate all the existing pixels stuff and add the
> points versions. I don't agree with Carlos that it's bad to have multiple
> properties that change at the same time; it makes sense in certain
> situations, such as GTlsCertificate where we provide separate DER and PEM
> properties to get the certificate in different formats. I don't think that's
> confusing. And in this case, we'll actually only have one non-deprecated way
> to get the settings, which is relevant because the deprecated ones will
> eventually go away.

Thing is that I was not convinced of deprecated the pixel ones yet, so I was considering having both at the same time.

> So I think that's fine. In the future, when we add the
> GTK+ 4 API, we can remove the original pixels versions (from the new API)
> and then drop the -pts from the names (in the new API), addressing the
> naming disparity. So this is right for the long term. It'll require some #if
> conditions, but I think it's the best approach.

I think using pixels will work in any case, so having convenient methods to get/set in points for other cases like desktop environment would solve the issue without deprecating and making more difficult for apps to port to the next API version.

> Now, for WPE, I agree with Carlos that it probably makes the most sense to
> stick with pixels instead of points, at least for now, because (to my
> knowledge) WPE does not expose any monitor DPI information, so the points
> settings cannot ever really work properly. I propose that we just accept
> that WPE and GTK need different API here. So let's guard the new API with
> #if PLATFORM(GTK), guard the G_PARAM_DEPRECATED use as well, remove the new
> points API from the WPE header file, and not deprecate the pixels API for
> WPE. It'll require even more conditions, which will be a bit messy, but it's
> the right thing to do IMO.
> 
> Carlos, does that sound OK?

I don't think it's a good idea to introduce more differences in the API between GTK and WPE, and I don't think it's needed in this case.

[Gabriel Ivașcu]

OK, I assume we've reached a conclusion? I'll update the patch in bug 179285.

[Michael Catanzaro]

(In reply to Gabriel Ivașcu from comment #43)
> OK, I assume we've reached a conclusion? I'll update the patch in bug 179285.

Almost. Instead of get_default_font_size_pts() and set_default_font_size_pts(), let's instead go with Carlos's suggestion to expose functions for converting from points to pixels and vice-versa. Then those global functions can be used in combination with the existing pixels properties. And we can minimize the difference between the GTK and WPE APIs, since it would just be two functions that WPE doesn't have, rather than having a separate set of font size properties that work a bit differently.

And, of course, please fix the documentation of minimum-font-size.

[Gabriel Ivașcu]

OK. What would be the best place for the new global functions to convert between pixels and points?

[Michael Catanzaro]

I'm going to suggest adding a new API header "webkit-util.h". But let's see if Carlos has a different recommendation.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #46)
> I'm going to suggest adding a new API header "webkit-util.h". But let's see
> if Carlos has a different recommendation.

Why are we still discussing this in this bug? Anyway, maybe generic functions to convert between points and pixels sounds out of scope of WebKit, it should be in GTK+ instead (or GdkScreen). If we add generic methods it might not be obvious that those are thought to be used to set/get fonts sizes in WebKitSettings in points. Adding them to WebKitSettings makes it obvious. If we still want to have global functions, I would add them to WebKitSettings as "static" methods (static as in C++, methods of WebKitSettings that don't receive an object instance).

[Gabriel Ivașcu]

(In reply to Carlos Garcia Campos from comment #47)
> Why are we still discussing this in this bug?

OK, let's switch this conversation to bug 179285.

> Adding them to WebKitSettings makes it obvious. If we still want to have
> global functions, I would add them to WebKitSettings as "static" methods
> (static as in C++, methods of WebKitSettings that don't receive an object
> instance).

I'm OK with this. I'll submit a patch to bug 179285.

[Gabriel Ivașcu]

Created attachment 326753 [details]
Patch

[Gabriel Ivașcu]

Created attachment 326755 [details]
Patch

[Michael Catanzaro]

Comment on attachment 326755 [details]
Patch

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

Good!

> Source/WebCore/platform/gtk/PlatformScreenGtk.cpp:43
> +#include <wtf/Optional.h>
>  
>  #include <cmath>
>  #include <gtk/gtk.h>

This header should go down below with the other <> includes; it's treated as a "system" header even though it's really part of WebKit, because it's not in the WebCore subproject.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:180
> +        double newScreenDpi = WebCore::screenDPI();
> +
> +        if (newScreenDpi == settings->priv->screenDpi)

Style nit: I'd use auto here, and remove the blank line.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:185
> +        double scalingFactor = newScreenDpi / settings->priv->screenDpi;
> +        uint32_t prevFontSize = settings->priv->preferences->defaultFontSize();
> +        uint32_t prevMonospaceFontSize = settings->priv->preferences->defaultFixedFontSize();

Probably best to use auto for all of these declarations, instead of writing out the type and hoping it never changes.

[Gabriel Ivașcu]

Created attachment 326773 [details]
Patch

[Michael Catanzaro]

Comment on attachment 326773 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by Michael Catanzaro.

Don't set r? again when the reviewer is already set. Use 'webkit-patch --no-review'

[WebKit Commit Bot]

Comment on attachment 326773 [details]
Patch

Clearing flags on attachment: 326773

Committed r224776: <https://trac.webkit.org/changeset/224776>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Gabriel Ivașcu]

(In reply to Michael Catanzaro from comment #53)
> > Source/WebCore/ChangeLog:6
> > +        Reviewed by Michael Catanzaro.
> 
> Don't set r? again when the reviewer is already set. Use 'webkit-patch
> --no-review'

Sorry, I thought I need to wait for cgarcia's review too.

[Michael Catanzaro]

Well he already reviewed this one, and I'm fairly confident you implemented his suggestion correctly. At least, it looks good to me. Of course, if he does have follow-up comments, you should implement them.

(Two GTK/WPE reviewers are required for patches that add API, but this patch doesn't.)

[Carlos Garcia Campos]

Comment on attachment 326773 [details]
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.

[Gabriel Ivașcu]

(In reply to Carlos Garcia Campos from comment #58)
> > 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.

What do you mean by context? The WebKitSettings object itself?

> > 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().

Why would we need to freeze/thaw the notify signals if we're going to replace the manual set and notify with the setter function? Do we want to emit the signals only after both font sizes were set?

[Carlos Garcia Campos]

(In reply to Gabriel Ivașcu from comment #59)
> (In reply to Carlos Garcia Campos from comment #58)
> > > 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.
> 
> What do you mean by context? The WebKitSettings object itself?

Yes, in this case is the WebKitSettings object itself, but from the WebCore point of view it's void*.

> > > 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().
> 
> Why would we need to freeze/thaw the notify signals if we're going to
> replace the manual set and notify with the setter function? Do we want to
> emit the signals only after both font sizes were set?

We don't need it, but it's desirable when we know that notify could be called multiple times in a row.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #58)
> > 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.

Ow :/

[Gabriel Ivașcu]

Created attachment 326881 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 326881 [details]
Patch

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

> Source/WebCore/platform/gtk/PlatformScreenGtk.cpp:43
> -#include <wtf/Optional.h>
> +#include <unordered_map>

Use HashMap from WTF, please.

> Source/WebCore/platform/gtk/PlatformScreenGtk.cpp:116
> -static std::optional<Function<void()>> screenDPIObserverHandler;
> +static std::unordered_map<void*, Function<void()>> m_screenDPIObserverHandlersMap;

For global maps we normally use a function with a NeverDestroyed map that returns a reference of the map.

> Source/WebCore/platform/gtk/PlatformScreenGtk.cpp:128
>  

Instead of null-checking gtkSettings everywhere  we can just return early since it doesn't make sense to save the handler that we are not going to call ever.

> Source/WebCore/platform/gtk/PlatformScreenGtk.cpp:134
> +    if (m_screenDPIObserverHandlersMap.size() > 0) {

Do not compare to 0, use either size() or when using HashMap !isEmpty().

[Gabriel Ivașcu]

Created attachment 326975 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 326975 [details]
Patch

Clearing flags on attachment: 326975

Committed r224872: <https://trac.webkit.org/changeset/224872>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.