[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

--- Comment #24 from Gabriel Ivașcu <givascu at igalia.com> ---
(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.

-- 
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/0803b933/attachment.html>