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

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


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

Attachment 325377: Patch

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




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


More information about the webkit-reviews mailing list