736010 - (downloaded-stretchy) Make downloaded fonts usable in nsMathMLChar

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Frédéric Wang (:fredw)]

Followup of 663740 comment 8.
Here is Karl's suggestion:


>The use of gfxPlatform::ResolveFontName() is preventing the use of any mathfontNAME.properties files except for mathfontUnicode.properties with downloaded fonts.

> Perhaps getting downloaded fonts working for stretchy characters is almost independent of that bug. The simplest solution there might be to simply stop using gfxPlatform::ResolveFontName() but instead have SetFontFamily in nsMathMLChar return a boolean to indicate success, returning success only when the nsFontMetrics is using the requested family or the nsGlyphTable is the Unicode table. 

> ResolvFontName doesn't know about the downloaded fonts.  I
> assume that the nsFontMetrics does but I haven't verified that.
>
> Changing the ResolveFontName call to a ResolverCallback would
> leave detecting the presence of fonts up to the ResolverCallback
> or TryVariants and TryParts.
>
> SetFontFamily callers that measure would have to check the return
> code and behave appropriately.  I expect that would be by
> returning from the caller with the appropriate code.  Callers that
> draw can probably just ignore the return code.
>
> This is just the simplest modification I see to the existing code.
> It won't be as efficient as ResolveFontName, but I expect there
> are ways of improving efficiency if necessary.
>
> With ResolvFontName gone, I guess ResolverCallback should merge
> into EnumCallback, or at least have a different name.
>

Comment 1

[Frédéric Wang (:fredw)]

This bug may also block bug 697053.

I've written an add-on that adds WOFF math fonts:
https://addons.mozilla.org/en-US/firefox/addon/mathml-fonts/

but it does not work yet with operators drawn using nsMathMLChar. Fixing this bug should make the add-on work as expected.

Comment 2

[François Wang]

Attachment: Patch V1

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 609012 [details] [diff] [review]
Patch V1

Looks good, thanks.

>+      aFont = font;

Can you change this to "aFont.name = font.name" or "aFont.name = family",
please?  That will reduce how much needs to be copied.

>+      if (!SetFontFamily(mChar->mStyleContext, mRenderingContext,
>+                         font, aGlyphTable, ch, aFamily)) return false;

Please put the "return false" on a new line (indented, and leave the following blank line).  That would make the jump statement more visible and is the more usual style for such a statement.

Comment 4

[François Wang]

Attachment: Patch V2

Comment 5

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 609342
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=f8ae80f67d54
Try run started, revision f8ae80f67d54. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=f8ae80f67d54

Comment 6

[Mozilla RelEng Bot]

Try run for f8ae80f67d54 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f8ae80f67d54
Results (out of 15 total builds):
    success: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f8ae80f67d54

Comment 7

[Frédéric Wang (:fredw)]

Another try, with the instruction to run the tests.

Comment 8

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 609342
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=5ddb4b3e9982
Try run started, revision 5ddb4b3e9982. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=5ddb4b3e9982

Comment 9

[Mozilla RelEng Bot]

Try run for 5ddb4b3e9982 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5ddb4b3e9982
Results (out of 284 total builds):
    success: 259
    warnings: 24
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5ddb4b3e9982

Comment 10

[Frédéric Wang (:fredw)]

Apparently, there are two failures on Windows:
- layout/reftests/mathml/semantics-1.xhtml (similar to bug 572899)
- layout/reftests/mathml/mfenced-10.html

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f430bb8a0049

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Yes indeed. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/031949d40753

(Feel free to take the checkin-needed flag off the bug next time you see Try failures...)

Comment 13

[Frédéric Wang (:fredw)]

(In reply to Ryan VanderMeulen from comment #12)
> (Feel free to take the checkin-needed flag off the bug next time you see Try
> failures...)

Sorry about that, I didn't notice that the checkin-needed flag was set.

(In reply to Frédéric Wang from comment #10)
> Apparently, there are two failures on Windows:
> - layout/reftests/mathml/semantics-1.xhtml (similar to bug 572899)
> - layout/reftests/mathml/mfenced-10.html

It seems that mfenced-10 passes with this patch, so we can just remove the fails-if(winWidget) flag that was added in bug 553918.

I hope that the patch from bug 572899 makes semantics-1 pass too.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

On the push I made, it was only passing on WinXP, FWIW.

Comment 15

[Frédéric Wang (:fredw)]

(In reply to Ryan VanderMeulen from comment #14)
> On the push I made, it was only passing on WinXP, FWIW.

OK, you are right. We should modify fails-if(winWidget) by a weaker condition that excludes WinXP.

Comment 16

[Frédéric Wang (:fredw)]

> OK, you are right. We should modify fails-if(winWidget) by a weaker
> condition that excludes WinXP.

I'm not sure whether there is a condition to do that. Maybe we can just mark the test random-if(winWidget)?

Comment 17

[Karl Tomlinson (:karlt)]

My suspicion re the reason for mfenced-10 failing is in bug 553918 comment 8, so in that respect the test is not really expected to pass and I'm happy for it to be marked as random on all platforms.

But I don't understand why the changes in this bug should change the result of that test, so I wonder what we are missing.

Comment 18

[Frédéric Wang (:fredw)]

In the screenshot of the test results, it seems that fences are built by parts, although the correct glyphs are not available. IIUC, we now always say that SetFontFamily succeeds when we use the Unicode table so I guess the unicode constructions are always tried, even when we don't have the basic fonts to do so.

That does not really explain the relation with bug 553918 comment 8. Actually, I'm not really sure about why semantics-1 fails with this patch either.

Comment 19

[Karl Tomlinson (:karlt)]

(In reply to Frédéric Wang from comment #18)
> In the screenshot of the test results, it seems that fences are built by
> parts, although the correct glyphs are not available.

That looks like a regression.

> IIUC, we now always
> say that SetFontFamily succeeds when we use the Unicode table so I guess the
> unicode constructions are always tried, even when we don't have the basic
> fonts to do so.

That's making some sense.  Previously the Unicode table was only tried when the fonts exist or the family was a generic family.  That meant that the Unicode table wasn't used until after the "Symbol" table (on a typical WinXP machine).

Emulating that logic with the new approach and the mTablesTried logic may be more difficult.

Comment 20

[Frédéric Wang (:fredw)]

https://tbpl.mozilla.org/?tree=Try&rev=ca8fecde634f

Comment 21

[Frédéric Wang (:fredw)]

Results look better with Karl's latest proposal:
https://tbpl.mozilla.org/?tree=Try&rev=f7e1e1b02eec

Comment 22

[Mozilla RelEng Bot]

Try run for f7e1e1b02eec is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f7e1e1b02eec
Results (out of 28 total builds):
    success: 26
    warnings: 1
    other: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f7e1e1b02eec
 Timed out after 12 hours without completing.

Comment 23

[Frédéric Wang (:fredw)]

Attachment: Patch V4

François is going to be offline in the next days, so I attach his latest patch.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3563399583c

Is it possible to create a test for this?

Comment 25

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/a3563399583c

Comment 26

[Frédéric Wang (:fredw)]

We should add a comment on dev release notes about this bug, mentioning that the MathML fonts add-on can now be used:
https://addons.mozilla.org/en-US/firefox/addon/mathml-fonts/

(In reply to Ryan VanderMeulen from comment #24)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a3563399583c
> 
> Is it possible to create a test for this?

A test is likely to be hard, because the rendering strongly depends on fonts available on the test machines. A test should also involve using @font-face rule that download a WOFF font placed somewhere on the test machine.

Comment 27

[Karl Tomlinson (:karlt)]

Fonts for reftests are typically included under layout/reftests/fonts.
layout/reftests/font-face has several examples of their use.
Note "HTTP(..)" in the reftest.list.
There are already some DejaVu variants there that have support for stretchy operators.

Perhaps a != test could check that using a different downloaded font changes the appearance of a stretchy operator.

Comment 28

[Florian Scholz (Open Web Docs)]

Frédéric has added notes here:
https://developer.mozilla.org/en/Firefox_15_for_developers#MathML
https://developer.mozilla.org/en/Mozilla_MathML_Project/Fonts#Main_fonts

I consider this as documented then.