[Webkit-unassigned] [Bug 142673] [GTK] Add settings to set font size in points, and deprecate the pixel size settings

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 30 19:22:43 PDT 2017 

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

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #325377|review?, commit-queue?      |review-, commit-queue-
              Flags|                            |

--- Comment #17 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 325377
  --> https://bugs.webkit.org/attachment.cgi?id=325377
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

-- 
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/20171031/852736ee/attachment-0001.html>

More information about the webkit-unassigned mailing list