Bug 149036

Summary: Split FontDescription into lower and higher level types
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: TextAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, mmaxfield
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
none
patch
darin: review+
patch
none
patch none

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.