Bug 151650 – [SVG -> OTF Converter] Force UnitsPerEm to 1000

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[SVG -> OTF Converter] Force UnitsPerEm to 1000

[Myles C. Maxfield]

Created attachment 266235 [details]
Patch

[Build Bot]

Comment on attachment 266235 [details]
Patch

Attachment 266235 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/495667

New failing tests:
svg/W3C-SVG-1.1/fonts-elem-02-t.svg
svg/custom/glyph-setting-d-attribute.svg
svg/W3C-SVG-1.1/fonts-elem-06-t.svg
svg/text/svg-font-word-rounding-hacks-spaces.html
svg/W3C-SVG-1.1/text-intro-02-b.svg
svg/W3C-SVG-1.1/fonts-elem-07-b.svg
svg/custom/glyph-selection-non-bmp.svg
svg/custom/glyph-selection-lang-attribute.svg
svg/batik/text/textEffect3.svg
fast/ruby/ruby-expansion-cjk-5.html
svg/W3C-SVG-1.1/fonts-elem-04-b.svg
svg/W3C-SVG-1.1/text-intro-01-t.svg
fast/ruby/ruby-expansion-cjk-3.html
fast/ruby/ruby-expansion-cjk.html
svg/W3C-SVG-1.1/text-intro-03-b.svg
svg/W3C-SVG-1.1/fonts-elem-01-t.svg
svg/wicd/test-rightsizing-b.xhtml
svg/custom/glyph-selection-arabic-forms.svg
svg/batik/text/textPosition2.svg
svg/custom/glyph-selection-bidi-mirror.svg
svg/W3C-SVG-1.1/fonts-elem-03-b.svg
svg/custom/acid3-test-77.html
svg/W3C-SVG-1.1/text-align-08-b.svg
platform/mac/svg/fonts/svg-font-general.html
svg/W3C-SVG-1.1/text-intro-04-t.svg
fast/ruby/ruby-expansion-cjk-4.html
svg/W3C-SVG-1.1/fonts-glyph-03-t.svg

[Build Bot]

Created attachment 266237 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 266235 [details]
Patch

Attachment 266235 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/495671

New failing tests:
svg/W3C-SVG-1.1/fonts-elem-02-t.svg
svg/custom/glyph-setting-d-attribute.svg
svg/W3C-SVG-1.1/fonts-elem-06-t.svg
svg/text/svg-font-word-rounding-hacks-spaces.html
svg/W3C-SVG-1.1/text-intro-02-b.svg
svg/custom/glyph-selection-bidi-mirror.svg
svg/custom/glyph-selection-non-bmp.svg
svg/W3C-SVG-1.1/fonts-elem-07-b.svg
svg/batik/text/textEffect3.svg
fast/ruby/ruby-expansion-cjk-5.html
svg/custom/glyph-selection-lang-attribute.svg
svg/batik/text/textPosition2.svg
fast/ruby/ruby-expansion-cjk-3.html
fast/ruby/ruby-expansion-cjk.html
svg/W3C-SVG-1.1/text-intro-03-b.svg
fast/ruby/ruby-expansion-cjk-2.html
svg/W3C-SVG-1.1/fonts-elem-01-t.svg
svg/wicd/test-rightsizing-b.xhtml
svg/custom/glyph-selection-arabic-forms.svg
svg/W3C-SVG-1.1/text-intro-01-t.svg
svg/W3C-SVG-1.1/fonts-elem-03-b.svg
svg/custom/acid3-test-77.html
svg/W3C-SVG-1.1/text-align-08-b.svg
platform/mac/svg/fonts/svg-font-general.html
svg/W3C-SVG-1.1/text-intro-04-t.svg
fast/ruby/ruby-expansion-cjk-4.html
svg/W3C-SVG-1.1/fonts-glyph-03-t.svg

[Build Bot]

Created attachment 266238 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 266235 [details]
Patch

Attachment 266235 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/495670

New failing tests:
svg/W3C-SVG-1.1/fonts-elem-02-t.svg
svg/custom/glyph-setting-d-attribute.svg
svg/W3C-SVG-1.1/fonts-elem-06-t.svg
svg/text/svg-font-word-rounding-hacks-spaces.html
svg/W3C-SVG-1.1/text-intro-02-b.svg
svg/custom/glyph-selection-lang-attribute.svg
svg/custom/glyph-selection-non-bmp.svg
svg/custom/glyph-selection-bidi-mirror.svg
svg/batik/text/textEffect3.svg
fast/ruby/ruby-expansion-cjk-5.html
svg/W3C-SVG-1.1/fonts-elem-04-b.svg
svg/W3C-SVG-1.1/text-intro-01-t.svg
fast/ruby/ruby-expansion-cjk-3.html
fast/ruby/ruby-expansion-cjk.html
svg/W3C-SVG-1.1/fonts-elem-07-b.svg
svg/W3C-SVG-1.1/text-intro-03-b.svg
svg/W3C-SVG-1.1/fonts-elem-01-t.svg
svg/wicd/test-rightsizing-b.xhtml
svg/custom/glyph-selection-arabic-forms.svg
svg/batik/text/textPosition2.svg
svg/W3C-SVG-1.1/fonts-elem-03-b.svg
svg/custom/acid3-test-77.html
svg/W3C-SVG-1.1/text-align-08-b.svg
platform/mac/svg/fonts/svg-font-general.html
svg/W3C-SVG-1.1/text-intro-04-t.svg
fast/ruby/ruby-expansion-cjk-4.html
svg/W3C-SVG-1.1/fonts-glyph-03-t.svg

[Build Bot]

Created attachment 266240 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Myles C. Maxfield]

It looks like there are some real failures:

platform/mac/svg/fonts/svg-font-general.html
svg/W3C-SVG-1.1/fonts-elem-06-t.svg
svg/W3C-SVG-1.1/fonts-glyph-03-t.svg
svg/W3C-SVG-1.1/text-align-08-b.svg
svg/custom/glyph-selection-bidi-mirror.svg
svg/custom/glyph-selection-lang-attribute.svg
svg/custom/glyph-setting-d-attribute.svg
svg/text/svg-font-word-rounding-hacks-spaces.html	
svg/custom/glyph-selection-arabic-forms.svg

[Darin Adler]

Comment on attachment 266235 [details]
Patch

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

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:235
> +    template <typename T>
> +    T scaleUnitsPerEm(T value) const
> +    {
> +        return value / static_cast<double>(m_inputUnitsPerEm) * s_outputUnitsPerEm;
> +    }

Strange to cast m_inputUnitsPerEm to double. Seems like an arbitrary choice. Why not cast "value" to double instead? Or do the entire thing with integers by multiplying first and then dividing?

The algorithm involves conversion to floating point, scaling, and then truncation. I’d think we’d want to round instead of truncating.

Do we have a guarantee that m_inputUnitsPerEm is not zero? Do we have a guarantee that the scaling won’t result in overflow? What’s the desired behavior if it does?

In new code I’ve been using this format:

    template<typename T> T scaleUnitsPerEm(T value) const

Putting the template on the same line seems to make sense in a case like this, making it easier to read rather than harder. And the lack of space after "template" is a style that both Anders and I prefer.a

[Myles C. Maxfield]

Created attachment 266279 [details]
Patch

[Myles C. Maxfield]

svg/text/svg-font-word-rounding-hacks-spaces.html may still be a real failure.

[Myles C. Maxfield]

Created attachment 266307 [details]
Needs rebaselines

[Build Bot]

Comment on attachment 266307 [details]
Needs rebaselines

Attachment 266307 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/499219

New failing tests:
fast/ruby/ruby-expansion-cjk-3.html
fast/ruby/ruby-expansion-cjk-5.html
svg/batik/text/textEffect3.svg
svg/W3C-SVG-1.1/fonts-elem-02-t.svg
svg/custom/acid3-test-77.html
svg/W3C-SVG-1.1/text-intro-03-b.svg
svg/batik/text/textPosition2.svg
svg/W3C-SVG-1.1/text-intro-04-t.svg
fast/ruby/ruby-expansion-cjk.html
fast/ruby/ruby-expansion-cjk-4.html
svg/W3C-SVG-1.1/fonts-elem-01-t.svg
svg/W3C-SVG-1.1/text-intro-02-b.svg
svg/wicd/test-rightsizing-b.xhtml
svg/W3C-SVG-1.1/fonts-elem-07-b.svg
svg/W3C-SVG-1.1/fonts-elem-04-b.svg
svg/W3C-SVG-1.1/text-intro-01-t.svg
svg/W3C-SVG-1.1/fonts-elem-03-b.svg

[Build Bot]

Created attachment 266312 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 266307 [details]
Needs rebaselines

Attachment 266307 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/499235

New failing tests:
fast/ruby/ruby-expansion-cjk-3.html
fast/ruby/ruby-expansion-cjk-5.html
svg/batik/text/textEffect3.svg
svg/W3C-SVG-1.1/fonts-elem-02-t.svg
svg/custom/acid3-test-77.html
svg/W3C-SVG-1.1/text-intro-03-b.svg
svg/W3C-SVG-1.1/text-intro-04-t.svg
svg/batik/text/textPosition2.svg
fast/ruby/ruby-expansion-cjk-2.html
fast/ruby/ruby-expansion-cjk.html
fast/ruby/ruby-expansion-cjk-4.html
svg/W3C-SVG-1.1/fonts-elem-01-t.svg
svg/W3C-SVG-1.1/text-intro-02-b.svg
svg/wicd/test-rightsizing-b.xhtml
svg/W3C-SVG-1.1/fonts-elem-07-b.svg
svg/W3C-SVG-1.1/text-intro-01-t.svg
svg/W3C-SVG-1.1/fonts-elem-03-b.svg

[Build Bot]

Created attachment 266313 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 266307 [details]
Needs rebaselines

Attachment 266307 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/499223

New failing tests:
fast/ruby/ruby-expansion-cjk-3.html
fast/ruby/ruby-expansion-cjk-5.html
svg/batik/text/textEffect3.svg
svg/W3C-SVG-1.1/fonts-elem-02-t.svg
svg/custom/acid3-test-77.html
svg/W3C-SVG-1.1/text-intro-03-b.svg
svg/batik/text/textPosition2.svg
svg/W3C-SVG-1.1/text-intro-04-t.svg
fast/ruby/ruby-expansion-cjk.html
fast/ruby/ruby-expansion-cjk-4.html
svg/W3C-SVG-1.1/fonts-elem-01-t.svg
svg/W3C-SVG-1.1/text-intro-02-b.svg
svg/wicd/test-rightsizing-b.xhtml
svg/W3C-SVG-1.1/fonts-elem-07-b.svg
svg/W3C-SVG-1.1/fonts-elem-04-b.svg
svg/W3C-SVG-1.1/text-intro-01-t.svg
svg/W3C-SVG-1.1/fonts-elem-03-b.svg

[Build Bot]

Created attachment 266314 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Myles C. Maxfield]

Created attachment 266318 [details]
Patch

[Build Bot]

Comment on attachment 266318 [details]
Patch

Attachment 266318 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/499615

New failing tests:
svg/W3C-SVG-1.1/fonts-elem-04-b.svg

[Build Bot]

Created attachment 266327 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 266318 [details]
Patch

Attachment 266318 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/499612

New failing tests:
svg/W3C-SVG-1.1/fonts-elem-04-b.svg

[Build Bot]

Created attachment 266328 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Antti Koivisto]

Comment on attachment 266318 [details]
Patch

r=me

[Darin Adler]

Did you see my comments and my questions?

[Myles C. Maxfield]

(In reply to comment #25)
> Did you see my comments and my questions?

Yes; this latest patch wasn't intended to be a real proposal; it only fixes some of the tests which were failing. I will address your comments in the next patch.

[Myles C. Maxfield]

Comment on attachment 266235 [details]
Patch

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

>> Source/WebCore/svg/SVGToOTFFontConversion.cpp:235
>> +    }
> 
> Strange to cast m_inputUnitsPerEm to double. Seems like an arbitrary choice. Why not cast "value" to double instead? Or do the entire thing with integers by multiplying first and then dividing?
> 
> The algorithm involves conversion to floating point, scaling, and then truncation. I’d think we’d want to round instead of truncating.
> 
> Do we have a guarantee that m_inputUnitsPerEm is not zero? Do we have a guarantee that the scaling won’t result in overflow? What’s the desired behavior if it does?
> 
> In new code I’ve been using this format:
> 
>     template<typename T> T scaleUnitsPerEm(T value) const
> 
> Putting the template on the same line seems to make sense in a case like this, making it easier to read rather than harder. And the lack of space after "template" is a style that both Anders and I prefer.a

Doing this all with integers is probably the most elegant solution.

We know m_inputUnitsPerEm isn't zero, because we check for this condition (and set it to 1). The result will only overflow if the font author uses values that are wildly outside of the em square; in this case, they will have bigger problems than this one.

[Myles C. Maxfield]

Created attachment 266390 [details]
Patch for committing

[WebKit Commit Bot]

Comment on attachment 266390 [details]
Patch for committing

Clearing flags on attachment: 266390

Committed r192930: <http://trac.webkit.org/changeset/192930>

[Myles C. Maxfield]

Created attachment 270812 [details]
Enable on Windows