Bug 179285 - [GTK] Add functionality to handle font sizes in points
Summary: [GTK] Add functionality to handle font sizes in points
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-11-04 08:37 PDT by Gabriel Ivașcu
Modified: 2017-11-12 11:51 PST (History)
7 users (show)

See Also:


Attachments
Patch (24.82 KB, patch)
2017-11-04 12:37 PDT, Gabriel Ivașcu
no flags Details | Formatted Diff | Diff
Patch (25.26 KB, patch)
2017-11-04 12:52 PDT, Gabriel Ivașcu
no flags Details | Formatted Diff | Diff
Patch (17.23 KB, patch)
2017-11-06 03:30 PST, Gabriel Ivașcu
no flags Details | Formatted Diff | Diff
Patch (12.49 KB, patch)
2017-11-07 07:27 PST, Gabriel Ivașcu
no flags Details | Formatted Diff | Diff
Patch (12.42 KB, patch)
2017-11-09 04:48 PST, Gabriel Ivașcu
no flags Details | Formatted Diff | Diff
Patch (11.69 KB, patch)
2017-11-12 08:54 PST, Gabriel Ivașcu
no flags Details | Formatted Diff | Diff
Patch (11.69 KB, patch)
2017-11-12 09:04 PST, Gabriel Ivașcu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gabriel Ivașcu 2017-11-04 08:37:13 PDT
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.
Comment 1 Gabriel Ivașcu 2017-11-04 12:37:18 PDT
Created attachment 326034 [details]
Patch
Comment 2 Build Bot 2017-11-04 12:39:05 PDT
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
Comment 3 Gabriel Ivașcu 2017-11-04 12:42:33 PDT
(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.
Comment 4 Gabriel Ivașcu 2017-11-04 12:52:46 PDT
Created attachment 326035 [details]
Patch
Comment 5 Gabriel Ivașcu 2017-11-04 12:57:44 PDT
(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.
Comment 6 Gabriel Ivașcu 2017-11-06 03:30:33 PST
Created attachment 326111 [details]
Patch
Comment 7 Gabriel Ivașcu 2017-11-06 03:42:42 PST
(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.
Comment 8 Gabriel Ivașcu 2017-11-07 07:27:20 PST
Created attachment 326210 [details]
Patch
Comment 9 Michael Catanzaro 2017-11-07 08:04:55 PST
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.
Comment 10 Michael Catanzaro 2017-11-07 09:08:20 PST
(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.
Comment 11 Carlos Garcia Campos 2017-11-08 05:32:04 PST
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().
Comment 12 Gabriel Ivașcu 2017-11-09 04:48:57 PST
Created attachment 326443 [details]
Patch
Comment 13 Michael Catanzaro 2017-11-09 09:49:20 PST
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.
Comment 14 Gabriel Ivașcu 2017-11-12 08:54:45 PST
Created attachment 326715 [details]
Patch
Comment 15 Gabriel Ivașcu 2017-11-12 09:04:59 PST
Created attachment 326716 [details]
Patch
Comment 16 WebKit Commit Bot 2017-11-12 11:51:01 PST
Comment on attachment 326716 [details]
Patch

Clearing flags on attachment: 326716

Committed r224737: <https://trac.webkit.org/changeset/224737>
Comment 17 WebKit Commit Bot 2017-11-12 11:51:03 PST
All reviewed patches have been landed.  Closing bug.