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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Oct 31 08:54:03 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 325442: Patch

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




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


More information about the webkit-reviews mailing list