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

[bugzilla-daemon at webkit.org]

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

Michael Catanzaro <mcatanzaro at igalia.com> changed:

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

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

-- 
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/5ceee4f8/attachment-0001.html>