Bug 149036 - Split FontDescription into lower and higher level types
Summary: Split FontDescription into lower and higher level types
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-09-10 06:47 PDT by Antti Koivisto
Modified: 2015-09-15 17:17 PDT (History)
2 users (show)

See Also:


Attachments
patch (91.29 KB, patch)
2015-09-10 10:50 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (92.77 KB, patch)
2015-09-11 02:53 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (104.11 KB, patch)
2015-09-11 03:55 PDT, Antti Koivisto
darin: review+
Details | Formatted Diff | Diff
patch (105.21 KB, patch)
2015-09-15 14:39 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (105.73 KB, patch)
2015-09-15 15:25 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2015-09-10 06:47:58 PDT
Currently FontDescription is used through the text subsystem. However much of the data it carries is only needed by FontCascade and text layout but not by the lower Font/FontCache layer. This make code confusing. For example families specified in FontDescription are ignored at lower levels.
Comment 1 Antti Koivisto 2015-09-10 10:50:30 PDT
Created attachment 260939 [details]
patch
Comment 2 Myles C. Maxfield 2015-09-10 14:32:29 PDT
Too much red to review.
Comment 3 Antti Koivisto 2015-09-11 02:53:50 PDT
Created attachment 260998 [details]
patch
Comment 4 Antti Koivisto 2015-09-11 03:55:49 PDT
Created attachment 261002 [details]
patch
Comment 5 Darin Adler 2015-09-11 09:14:50 PDT
Comment on attachment 261002 [details]
patch

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

A bit of a shame that the more commonly used class is the one that has a longer name now.

> Source/WebCore/platform/graphics/FontDescription.h:41
>  class FontDescription {

Seems like this class is almost a struct.

> Source/WebCore/platform/graphics/FontDescription.h:50
> +    int computedPixelSize() const { return int(m_computedSize + 0.5f); }

Not new: This is a strange way to write rounding code.

> Source/WebCore/platform/graphics/FontDescription.h:66
> +    void setComputedSize(float s) { m_computedSize = clampToFloat(s); }

Maybe not new: I don’t understand why it’s valuable to call clampToFloat on a float.

> Source/WebCore/platform/graphics/FontDescription.h:82
> +    FontTraitsMask traitsMask() const;
> +private:

Normally we leave a blank line before "private".

> Source/WebCore/rendering/RenderFullScreen.cpp:88
> +    fullscreenStyle.get().setFontDescription(FontCascadeDescription());

Might want to use { } here so we don’t have to state the type explicitly.
Comment 6 Antti Koivisto 2015-09-15 14:39:50 PDT
Created attachment 261239 [details]
patch
Comment 7 Antti Koivisto 2015-09-15 15:25:02 PDT
Created attachment 261249 [details]
patch
Comment 8 WebKit Commit Bot 2015-09-15 17:17:14 PDT
Comment on attachment 261249 [details]
patch

Clearing flags on attachment: 261249

Committed r189830: <http://trac.webkit.org/changeset/189830>
Comment 9 WebKit Commit Bot 2015-09-15 17:17:19 PDT
All reviewed patches have been landed.  Closing bug.