Last Comment Bug 736010 - (downloaded-stretchy) Make downloaded fonts usable in nsMathMLChar
(downloaded-stretchy)
: Make downloaded fonts usable in nsMathMLChar
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla15
Assigned To: François Wang
:
Mentors:
Depends on: 572899
Blocks: mathml-fonts 732830 782035
  Show dependency treegraph
 
Reported: 2012-03-15 01:56 PDT by Frédéric Wang (:fredw)
Modified: 2012-08-11 03:51 PDT (History)
7 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch V1 (8.04 KB, patch)
2012-03-24 10:01 PDT, François Wang
karlt: review+
Details | Diff | Splinter Review
Patch V2 (8.05 KB, patch)
2012-03-26 09:00 PDT, François Wang
karlt: review+
Details | Diff | Splinter Review
Patch V4 (8.38 KB, patch)
2012-04-29 23:20 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review

Description Frédéric Wang (:fredw) 2012-03-15 01:56:57 PDT
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) 2012-03-15 02:10:35 PDT
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 2012-03-24 10:01:45 PDT
Created attachment 609012 [details] [diff] [review]
Patch V1
Comment 3 Karl Tomlinson (:karlt) 2012-03-25 17:00:44 PDT
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 2012-03-26 09:00:34 PDT
Created attachment 609342 [details] [diff] [review]
Patch V2
Comment 5 Mozilla RelEng Bot 2012-03-28 13:26:17 PDT
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 2012-03-28 18:17:19 PDT
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/autolanduser@mozilla.com-f8ae80f67d54
Comment 7 Frédéric Wang (:fredw) 2012-03-29 12:27:56 PDT
Another try, with the instruction to run the tests.
Comment 8 Mozilla RelEng Bot 2012-03-29 12:31:36 PDT
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 2012-03-29 18:47:01 PDT
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/autolanduser@mozilla.com-5ddb4b3e9982
Comment 10 Frédéric Wang (:fredw) 2012-03-30 00:35:37 PDT
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] 2012-03-30 18:00:24 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f430bb8a0049
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-03-30 20:48:45 PDT
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) 2012-03-31 10:41:35 PDT
(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] 2012-03-31 10:45:11 PDT
On the push I made, it was only passing on WinXP, FWIW.
Comment 15 Frédéric Wang (:fredw) 2012-03-31 10:50:00 PDT
(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) 2012-04-01 03:02:53 PDT
> 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) 2012-04-01 21:39:14 PDT
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) 2012-04-01 23:20:19 PDT
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) 2012-04-01 23:38:53 PDT
(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) 2012-04-26 02:40:03 PDT
https://tbpl.mozilla.org/?tree=Try&rev=ca8fecde634f
Comment 21 Frédéric Wang (:fredw) 2012-04-28 06:43:16 PDT
Results look better with Karl's latest proposal:
https://tbpl.mozilla.org/?tree=Try&rev=f7e1e1b02eec
Comment 22 Mozilla RelEng Bot 2012-04-28 13:45:26 PDT
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/fred.wang@free.fr-f7e1e1b02eec
 Timed out after 12 hours without completing.
Comment 23 Frédéric Wang (:fredw) 2012-04-29 23:20:30 PDT
Created attachment 619496 [details] [diff] [review]
Patch V4

François is going to be offline in the next days, so I attach his latest patch.
Comment 24 Ryan VanderMeulen [:RyanVM] 2012-04-30 17:32:39 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3563399583c

Is it possible to create a test for this?
Comment 26 Frédéric Wang (:fredw) 2012-05-19 00:13:16 PDT
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) 2012-05-20 14:59:21 PDT
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 [:fscholz] (MDN) 2012-06-06 11:50:08 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.