Bug 45318 – Complex Path FontCache does not handle SmallCaps properly

• Status: RESOLVED
• Resolution: WORKSFORME
• Component: Text

[Gary Simmons]

Created attachment 66754 [details]
Patch to add small caps to font cache

Overview: 

When rendering with FAST_PATH turned off, the complex path for small caps data directly requests a FontPlatformData with SmallCapsVariant. Currently the SmallCapsVariant and the PlainVariant will have the same hash and occupy same space in the font cache resulting in incorrect rendering (always Small Caps or always plain)

Steps to Reproduce:

1) Build with fast path disabled
2) Load a webpage using small caps of a particular font
3) Load another page with exactly the same font but without small caps

Expected Results:

The small caps page will have small caps, the plain page will have plain.

Actual Results:

Both pages have Small Caps (likewise if steps 2 and 3 are reversed both pages will have plain).

[Andreas Kling]

Comment on attachment 66754 [details]
Patch to add small caps to font cache

> +        css2.1 and canvas LayoutTests in sequence cover this by using small-caps fonts.

So you're saying this is only covered when the layout tests are run in a specific order?
That's not good enough IMO, could you make a test that covers this explicitly?

[Gary Simmons]

(In reply to comment #1)
> (From update of attachment 66754 [details])
> > +        css2.1 and canvas LayoutTests in sequence cover this by using small-caps fonts.
> 
> So you're saying this is only covered when the layout tests are run in a specific order?
> That's not good enough IMO, could you make a test that covers this explicitly?

Will do. I'll add it to this bug as soon as I have a chance to access some machines to generate baselines.

[Andreas Kling]

Comment on attachment 66754 [details]
Patch to add small caps to font cache

r- due to lack of test.

I don’t understand this change. As far as I can tell, the font cache has no notion of small caps. Why should it care?

[Gary Simmons]

Created attachment 66916 [details]
Updated patch with LayoutTests

I've updated the patch and attached a relevant layout test. This bug only manifests itself in the Qt port as all other ports are now using the fast path for fonts. When using the non-fast path code, Small Caps is requested as a variant attribute in the FontDescription. With the current code, the generated FontPlatformData object will be indistinguishable (in terms of hash keys) from the plain variant causing whichever one is used first to become always used.

If the test is manually loaded in the current Qt browser, you'll see all the text as small caps (which is incorrect as described by the test and demonstrated by other browsers).

Any ports using the fast path are exempt from this issue as they request the smallCapsFontData from the SimpleFontData object rather than directly asking the cache. 

I have generated expected results for Qt but I have not had access to the resources to generate results for other ports as of yet.

[Kenneth Rohde Christiansen]

If you use Qt >= 4.7, Qt is using the fast font path.

I am not sure I fully understand how this will impact ports that make full use of WebKit’s font system. Will they have extra entries in the cache now? That sounds bad.

[Gary Simmons]

(In reply to comment #7)
> I am not sure I fully understand how this will impact ports that make full use of WebKit’s font system. Will they have extra entries in the cache now? That sounds bad.

Any port going through the fast path requests the smallCapsFontData directly from the font object. The small caps font data is sort of implicitly cached in the SimpleFontData. The "cache" in this case effectively is within the SimpleFontData itself rather than the FontCache.

If we're not going through the fast path however, the font cache key must distinguish between different FontPlatformData objects, and as small caps is an attribute of the FontDescription used to retrieve the FontPlatformData. Virtually every other attribute of the FontDescription is used in the FontCacheKey.

So yes in the non fast path there would be 2 entries in the FontCache (the small caps and the non-small caps versions) in the fast path the object for small caps object would lie within the SimpleFontData object for the non small caps version as a member.

[George Staikos]

It seems to me like this is the right thing to do.

[Robert Hogan]

This seems overdue an r+ to me. Could you fix up the misformatting in the changelog, rebase and resubmit?

[Eric Seidel (no email)]

Comment on attachment 66916 [details]
Updated patch with LayoutTests

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

> LayoutTests/ChangeLog:5
> +        Load the same font twice once as small caps and the second time as plain to ensure that small caps

Your changelog should mention the bug.  prepare-ChangeLog -b NUMBER will fill it in for you.  As would webkit-patch upload.

> WebCore/ChangeLog:5
> +        Add smallCaps to the Font Cache key to allow differentiating between plain and small caps variants o            fonts when running text through the non-fast path code such as in the Qt port.

misformating.  Also missing bug here.

> WebCore/platform/graphics/FontCache.cpp:59
> +                             bool smallCaps = false, bool isPrinterFont = false, FontRenderingMode renderingMode = NormalRenderingMode)

bools are strictly worse than enums.  Enums make callsites much clearer.

[Ahmad Saleem]

Created attachment 460508 [details]
Safari 15.5 matches other browsers

I am not sure on expected output but I took the test case from the attached patch and changed it into following JSFiddle:

Link (Missing generic font family - san-serif) - https://jsfiddle.net/z28rogtb/1/show
Link (font family added) - https://jsfiddle.net/fs2xkajw/

Further, I test it across all browsers and they match with each (refer to attached screenshot). (Only attached of test case without san-serif but it matches when I add font family).

Is this fixed along the way or something is needed? If it is former case, can this be marked as "RESOLVED CONFIGURATION CHANGED" or Duplicate of XYZ? Thanks!

[Alexey Proskuryakov]

Does this test still trigger the complex text path? Or does the issue persist, but not visible here because we can now display this test using simple text path?

(In reply to Alexey Proskuryakov from comment #13)
> Does this test still trigger the complex text path? Or does the issue
> persist, but not visible here because we can now display this test using
> simple text path?

Note that this has only ever been an issue on some non-Apple port(s).