Bug 179285 – [GTK] Add functionality to handle font sizes in points

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Gabriel Ivașcu]

It can be in form of getter/setter functions and/or new font size properties in points.

This is not particularly useful on its own, but it's in conjunction with bug 142673 (the main discussion is in that thread).

When both bugs are fixed, apps will not have to handle font sizes in pixel values anymore, which is more convenient for them.

[Gabriel Ivașcu]

Created attachment 326034 [details]
Patch

[Build Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Gabriel Ivașcu]

(In reply to Gabriel Ivașcu from comment #1)
> Created attachment 326034 [details]
> Patch

As I mentioned in bug 142673, comment 37, this patch add new font size properties in points and deprecated the current font size properties in pixels.

However, if you eventually decide to keep the current font size properties in pixels, please let me know so I can update the patch.

[Gabriel Ivașcu]

Created attachment 326035 [details]
Patch

[Gabriel Ivașcu]

(In reply to Gabriel Ivașcu from comment #4)
> Created attachment 326035 [details]
> Patch

The patch in attachment 326034 [details] had some build errors on WPE.

[Gabriel Ivașcu]

Created attachment 326111 [details]
Patch

[Gabriel Ivașcu]

(In reply to Gabriel Ivașcu from comment #6)
> Created attachment 326111 [details]
> Patch

This is an updated patch based on the resolution of the discussions in bug 142673.

[Gabriel Ivașcu]

Created attachment 326210 [details]
Patch

[Michael Catanzaro]

Comment on attachment 326210 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326210&action=review

I like it. Nice and simple, easy to use, and documented in a logical place. First r=me. Carlos will give final approval.

I know it's sad to do a bunch of work, redo it again and again, and wind up committing a much simpler result. But that's how we wind up with excellent APIs.

> Source/WebCore/platform/wpe/PlatformScreenWPE.cpp:59
> +    notImplemented();

Please remove this line, it's badly-named and doesn't do what you think... I've never quite understood its intended use, but I guess it's intended to be a TODO for stuff that you intend to implement in the near future, or for stuff that should never be called due to lack of implementation on some port. But in this case, it's going to print spam when applications change their font settings, and we don't want that.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #9)
> Please remove this line, it's badly-named and doesn't do what you think...

Looks like I was wrong about notImplemented()... I guess it is fine here.

[Carlos Garcia Campos]

Comment on attachment 326210 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326210&action=review

> Source/WebCore/platform/PlatformScreen.h:62
> +double screenGetDPI();

screenDPI()

> Source/WebCore/platform/gtk/PlatformScreenGtk.cpp:96
> +    static const double millimetresPerInch = 25.4;
> +    static double cachedDPI = 0;

These should be moved below, right before they are used.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3290
> +guint32 webkit_settings_convert_pixels_to_points(guint32 pixels)

This is still a generic function, even if it's prefix with webkit_settings, what I meant was to use something like webkit_settings_font_size_to_points(), or similar, to make ti clear that this is expected to be used with font size settings.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3308
> +guint32 webkit_settings_convert_points_to_pixels(guint32 points)

And webkit_settings_font_size_to_pixels().

[Gabriel Ivașcu]

Created attachment 326443 [details]
Patch

[Michael Catanzaro]

Comment on attachment 326443 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326443&action=review

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Make sure to either use 'webkit-patch apply-from-bug' once you have r+ to get  your reviewer's name in the changelog, or else manually write your reviewer's name here... you already had r+ from Carlos, but now you've deleted it.

> Source/WebKit/UIProcess/API/wpe/WebKitSettings.h:435
> +WEBKIT_API guint32
> +webkit_settings_font_size_to_points                            (guint32 pixels);
> +
> +WEBKIT_API guint32
> +webkit_settings_font_size_to_pixels                            (guint32 points);

I think these should not be exposed at all on WPE, since they won't work properly. So remove them from this header, and guard the implementation of the functions swith #if PLATFORM(GTK) to ensure they're not exported.

[Gabriel Ivașcu]

Created attachment 326715 [details]
Patch

[Gabriel Ivașcu]

Created attachment 326716 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 326716 [details]
Patch

Clearing flags on attachment: 326716

Committed r224737: <https://trac.webkit.org/changeset/224737>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.