Bug 139627 - Stop returning GlyphPage from various Font functions
Summary: Stop returning GlyphPage from various Font functions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-14 07:26 PST by Antti Koivisto
Modified: 2014-12-18 04:17 PST (History)
13 users (show)

See Also:


Attachments
patch (63.68 KB, patch)
2014-12-14 07:43 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch 2 (63.67 KB, patch)
2014-12-14 07:58 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
try to fix efl build (63.62 KB, patch)
2014-12-15 12:53 PST, Antti Koivisto
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2014-12-14 07:26:38 PST
It is only used for an obscure SVG font fallback case.
Comment 1 Antti Koivisto 2014-12-14 07:43:33 PST
Created attachment 243270 [details]
patch
Comment 2 Antti Koivisto 2014-12-14 07:58:44 PST
Created attachment 243271 [details]
patch 2
Comment 3 Myles C. Maxfield 2014-12-14 10:24:22 PST
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?
Comment 4 Antti Koivisto 2014-12-14 11:50:43 PST
> 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.
Comment 5 Darin Adler 2014-12-15 09:23:17 PST
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
Comment 6 Antti Koivisto 2014-12-15 12:53:40 PST
Created attachment 243305 [details]
try to fix efl build
Comment 7 Darin Adler 2014-12-16 23:11:16 PST
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.
Comment 8 Antti Koivisto 2014-12-18 04:17:21 PST
https://trac.webkit.org/r177490