Bug 139627 – Stop returning GlyphPage from various Font functions

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Antti Koivisto]

It is only used for an obscure SVG font fallback case.

[Antti Koivisto]

Created attachment 243270 [details]
patch

[Antti Koivisto]

Created attachment 243271 [details]
patch 2

[Myles C. Maxfield]

Comment on attachment 243271 [details]
patch 2

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

Changing behavior for the sake of code cleanup is probably not okay. Can't you separate the call into two halves, and only call the second half from the SVG code? If you do change this behavior, you should probably add a test to make the intended behavior more explicit (rather than relying on updating a W3C test)

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:336
> +    // It is not clear what should happen but we never try to resolve font fallbacks context-sensitively.

What was wrong with the old behavior?

[Antti Koivisto]

> Changing behavior for the sake of code cleanup is probably not okay.

WebKit does not have such rule. 

> Can't
> you separate the call into two halves, and only call the second half from
> the SVG code? If you do change this behavior, you should probably add a test
> to make the intended behavior more explicit (rather than relying on updating
> a W3C test)

The test changes are progressions, the patch also fixes a bug.

There are currently no tests for non-trivial fallback behavior for probably because it is not specified. The whole thing is a super obscure corner of an obsolete spec.

I understand that the plan is to eventually start converting SVG fonts to regular fonts. I think we'll need to remove all context sensitive features to make make that happen. Seems unlikely that anyone uses them in real world.
 
> What was wrong with the old behavior?

It requires a pile of ugly code to support a case that probably never happens in real world.

[Darin Adler]

Comment on attachment 243271 [details]
patch 2

FontGlyphs.cpp:212:18: error: 'WebCore::GlyphData WebCore::glyphDataForCJKCharacterWithoutSyntheticItalic(UChar32, WebCore::GlyphData&, unsigned int)' defined but not used

[Antti Koivisto]

Created attachment 243305 [details]
try to fix efl build

[Darin Adler]

Comment on attachment 243305 [details]
try to fix efl build

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

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:336
> +    // It is not clear what should happen but we never try to resolve font fallbacks context-sensitively.

I don’t completely understand this comment. Is there a way to write it more directly? I can’t tell if “we never try” is describing correct behavior or current behavior.

[Antti Koivisto]

https://trac.webkit.org/r177490