The default bug view has changed. See this FAQ.
Bug 736010 (downloaded-stretchy)

Make downloaded fonts usable in nsMathMLChar

RESOLVED FIXED in mozilla15

Status

()

Core
MathML
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: fredw, Assigned: François Wang)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla15
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
>
(Reporter)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
Created attachment 609012 [details] [diff] [review]
Patch V1
Attachment #609012 - Flags: review?(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.
Attachment #609012 - Flags: review?(karlt) → review+
Keywords: helpwanted
(Assignee)

Comment 4

5 years ago
Created attachment 609342 [details] [diff] [review]
Patch V2
Attachment #609012 - Attachment is obsolete: true
Attachment #609342 - Flags: review?(karlt)
Attachment #609342 - Flags: review?(karlt) → review+
(Assignee)

Updated

5 years ago
Whiteboard: [autoland-try]

Updated

5 years ago
Whiteboard: [autoland-try] → [autoland-in-queue]

Comment 5

5 years ago
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

5 years ago
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

Updated

5 years ago
Whiteboard: [autoland-in-queue]
(Reporter)

Comment 7

5 years ago
Another try, with the instruction to run the tests.
Whiteboard: [autoland-try:609342:-b do -p all -u all -t all]

Updated

5 years ago
Whiteboard: [autoland-try:609342:-b do -p all -u all -t all] → [autoland-in-queue]

Comment 8

5 years ago
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

5 years ago
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

Updated

5 years ago
Whiteboard: [autoland-in-queue]
Keywords: checkin-needed
Whiteboard: [need b=736010 and r=karlt added to commit message]
(Reporter)

Comment 10

5 years ago
Apparently, there are two failures on Windows:
- layout/reftests/mathml/semantics-1.xhtml (similar to bug 572899)
- layout/reftests/mathml/mfenced-10.html
https://hg.mozilla.org/integration/mozilla-inbound/rev/f430bb8a0049
Flags: in-testsuite?
Keywords: checkin-needed
Whiteboard: [need b=736010 and r=karlt added to commit message]
Target Milestone: --- → mozilla14
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...)
(Reporter)

Updated

5 years ago
Depends on: 572899
(Reporter)

Comment 13

5 years ago
(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.
On the push I made, it was only passing on WinXP, FWIW.
(Reporter)

Comment 15

5 years ago
(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.
(Reporter)

Comment 16

5 years ago
> 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)?
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.
(Reporter)

Comment 18

5 years ago
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.
(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.
(Reporter)

Comment 20

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=ca8fecde634f
(Reporter)

Comment 21

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

Comment 22

5 years ago
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.
(Reporter)

Comment 23

5 years ago
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.
Attachment #619496 - Flags: review?(karlt)
Attachment #619496 - Flags: review?(karlt) → review+
(Reporter)

Updated

5 years ago
Attachment #609342 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3563399583c

Is it possible to create a test for this?
Keywords: checkin-needed
Target Milestone: mozilla14 → mozilla15
https://hg.mozilla.org/mozilla-central/rev/a3563399583c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 26

5 years ago
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.
Keywords: dev-doc-needed
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.
No longer blocks: 295193, 732830
No longer depends on: 572899
Blocks: 295193, 732830
Depends on: 572899
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.
Keywords: dev-doc-needed → dev-doc-complete
(Reporter)

Updated

5 years ago
Depends on: 468568
(Reporter)

Updated

5 years ago
No longer depends on: 468568
(Reporter)

Updated

5 years ago
Blocks: 782035
You need to log in before you can comment on or make changes to this bug.