1197716 - Package fonts separately for reftests

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

This is an intermediate step for bug 1194338.

In bug 1194338 we'll download fonts at runtime. However we can't do this when running reftests. We need to package them separately and install them along with the fontless build.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

There are multiple options here:

1) Package fonts into a separate APK. If this APK is sharing the same uid (Manifest) then it can copy the files into the other app's data directory.

2) Package fonts into an add-on and install at test time. Jonalmeida looked into this for hyphenation dictionaries.

3) Use some (test) component in the app that accepts fonts from the host and copies them to the destination. Maybe we already have something like that? Do we push test profiles to the app?


I'd like to avoid 1) and 2) because they have the overhead of maintaining a separate app/add-on/code base. 3) would be nice - if it already exists.

@gbrown: Do you have some insights here? :)

Comment 2

[Geoff Brown [:gbrown]]

We do push test profiles to the app: We create a profile on the host and then push that to device before starting Firefox to run reftests. Can we install fonts by simply copying files to the profile? If so, I think that's the best solution here.

See http://hg.mozilla.org/mozilla-central/annotate/d56c816b35c3/layout/tools/reftest/remotereftest.py#l212.

I'd be happy to write the reftest harness changes if you can tell me what needs to go where.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Geoff Brown [:gbrown] from comment #2)
> We do push test profiles to the app: We create a profile on the host and
> then push that to device before starting Firefox to run reftests. Can we
> install fonts by simply copying files to the profile? If so, I think that's
> the best solution here.

This sounds interesting! But the fonts will live outside of the profile directory (so that all profiles can share them) in "app data"/fonts. Can we still use this mechanism for that?

> See
> http://hg.mozilla.org/mozilla-central/annotate/d56c816b35c3/layout/tools/
> reftest/remotereftest.py#l212.

This looks like it only handles profile data currently, right? How do we accept and handle all this data on the device side?

> I'd be happy to write the reftest harness changes if you can tell me what
> needs to go where.

In a nutshell: The upcoming DownloadContentService (Bug 1197720) is downloading the fonts that are now in mobile/android/fonts [1] and copies them to new File(context.getApplicationInfo().dataDir, "fonts"). After that it notifies Gecko to reload fonts.

Depending on what's the best way forward here we might be able to reuse some of the code on the device side. It's not landed yet though.

[1]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/fonts

Comment 4

[Geoff Brown [:gbrown]]

(In reply to Sebastian Kaspari (:sebastian) from comment #3)
> (In reply to Geoff Brown [:gbrown] from comment #2)
> > We do push test profiles to the app: We create a profile on the host and
> > then push that to device before starting Firefox to run reftests. Can we
> > install fonts by simply copying files to the profile? If so, I think that's
> > the best solution here.
> 
> This sounds interesting! But the fonts will live outside of the profile
> directory (so that all profiles can share them) in "app data"/fonts. Can we
> still use this mechanism for that?
> 
> > See
> > http://hg.mozilla.org/mozilla-central/annotate/d56c816b35c3/layout/tools/
> > reftest/remotereftest.py#l212.
> 
> This looks like it only handles profile data currently, right? How do we
> accept and handle all this data on the device side?

Yes, it only handles profile data. We build a profile on the host, push it to device, then start Firefox and simply point it to the newly constructed profile. I guess that's not helpful here.

I did not realize that the fonts would be in the app data directory. The test harness probably could still push the fonts to that directory, but permissions are a concern. I don't think there is any other operation in the reftest harness that requires root. Your options from comment 1 seem right: new apk, add-on, new app component to communicate with host (maybe pull from the host webserver?).

> > I'd be happy to write the reftest harness changes if you can tell me what
> > needs to go where.
> 
> In a nutshell: The upcoming DownloadContentService (Bug 1197720) is
> downloading the fonts that are now in mobile/android/fonts [1] and copies
> them to new File(context.getApplicationInfo().dataDir, "fonts"). After that
> it notifies Gecko to reload fonts.
> 
> Depending on what's the best way forward here we might be able to reuse some
> of the code on the device side. It's not landed yet though.
> 
> [1]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/fonts

Thanks. I'll keep an eye on bug 1197720.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Geoff Brown [:gbrown] from comment #4)
> (maybe pull from the host webserver?).

That's an interesting idea. We would just need to switch the catalog of downloadable content to point to the host instead of some live servers. But executing the tests would have to be delayed until everything's copied over.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

I just realized that I have been over-complicating things here. The download service stores the fonts in the app data directory, but Gecko loads fonts from the profile directory too ("fonts" subdirectory). So for the reftests we could just put them there - if this is easy and already supported.

@gbrown: How can we do this? I looked through remotereftest.py and copyExtraFilesToProfile() looks interesting but I am not sure where those files need to be added in order to get copied over.

Comment 7

[Geoff Brown [:gbrown]]

Attachment: add fonts to profile when run from 'make reftest-remote'

Thanks Sebastian. That's good news, I think. 

We can use the "extra profile files" feature by simply setting a command line option. In this patch, I add <objdir>/dist/bin/res/fonts to the profile, when run from make. That dir has all the files in <srcdir>/mobile/android/fonts plus a few more; I used it here because it is a little easier to reference the objdir. With this patch, "make reftest-remote" creates a profile with /mnt/sdcard/tests/reftest/profile/fonts populated with all those font files.

This patch is insufficient for tests run on treeherder. On treeherder, we need to get files from the "test package", which does not currently contain any font files. I'll look into adding fonts to the test package.

Are the fonts only required for reftests? Why are they not required for mochitests and robocop?

Comment 8

[Richard Newman [:rnewman]]

(In reply to Geoff Brown [:gbrown] from comment #7)

> Are the fonts only required for reftests? Why are they not required for
> mochitests and robocop?

If the fonts are missing, we fall back to the system fonts. They're only needed for reftests because reftests do pixel-perfect comparisons, and using the wrong fonts breaks those.

Comment 9

[Geoff Brown [:gbrown]]

Attachment: on android, include fonts in reftest profile

This patch includes fonts in the remote reftest profile by:
 - adding the fonts to the reftest test package, at reftest/fonts
 - updating mozharness configs to use the --extra-profile-file option to include the fonts from the test package in the profile, for android 2.3, android 4.3, and, for good measure, android 4.0 plain reftest
 - updating the 'remote-reftest' make target to also use --extra-profile-file
(I have done the same for my work-in-progress mach reftest command in bug 1087791).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=df078a65fe5d verifies that this does no harm. Extra logging shows that the fonts directory is indeed pushed to the test profile.

:sebastian -- Is there anything I can do/check to verify that the fonts will actually be used?

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Geoff Brown [:gbrown] from comment #9)
> :sebastian -- Is there anything I can do/check to verify that the fonts will
> actually be used?

Yes, building with MOZ_ANDROID_EXCLUDE_FONTS=1 should result in an APK without fonts. I saw a bunch of reftests failing with that.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8671966 [details] [diff] [review]
on android, include fonts in reftest profile

Review of attachment 8671966 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome! This patch is so much smaller than I thought it would be. :)

Comment 12

[Geoff Brown [:gbrown]]

(In reply to Sebastian Kaspari (:sebastian) from comment #10)
> Yes, building with MOZ_ANDROID_EXCLUDE_FONTS=1 should result in an APK
> without fonts. I saw a bunch of reftests failing with that.

Thanks. I see that too: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ed1368a0ef8

Unfortunately, with my patch, all the failures persist: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b81f12899e59

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8671966 [details] [diff] [review]
on android, include fonts in reftest profile

Review of attachment 8671966 [details] [diff] [review]:
-----------------------------------------------------------------

assuming a good answer to the question below r=me

::: testing/mozharness/configs/android/android_panda_releng.py
@@ +167,5 @@
>                  "--bootstrap",
>                  "--http-port=%(http_port)s",
>                  "--ssl-port=%(ssl_port)s",
>                  "--symbols-path=%(symbols_path)s",
> +                "--extra-profile-file=reftest/fonts",

why is this "reftest/fonts" and the others "fonts" ?

Comment 14

[Geoff Brown [:gbrown]]

(In reply to Joel Maher (:jmaher) from comment #13)
> why is this "reftest/fonts" and the others "fonts" ?

On Android 4.0, reftests are run from <test-package-directory>; on Android 2.3 and Android 4.3, reftests are run from <test-package-directory>/reftest.

Comment 15

[Geoff Brown [:gbrown]]

As noted in comment 12, with MOZ_ANDROID_EXCLUDE_FONTS=1, various reftests fail, even with my patch to push fonts to <profile>/fonts. Debugging locally, I see http://hg.mozilla.org/mozilla-central/annotate/d01dd42e654b/gfx/thebes/gfxFT2FontList.cpp#l934 called for each .ttf file installed in <profile>/fonts.

:sebastian -- Any idea why the tests are still failing?

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

I built locally with MOZ_ANDROID_EXCLUDE_FONTS=1, modified the DownloadContentService to copy the fonts to the profile instead of appDir and after that I used the following test site (The serif fonts are good to see whether the fonts are being used or not):
http://www.zipcon.net/~swhite/docs/computers/browsers/fonttest.html

This seems to work as it should. In my case the fonts are in:
* /data/data/org.mozilla.fennec_sebastian/files/mozilla/z90pui70.default/fonts/

@gbrown: I assume reftests use a different location for the profile because appDir can't be written from the outside? I'd like to move my profile there and test again. How do you point fennec to the new profile in tests?

Next I'll try to run the failing tests locally to see if they pass with my current configuration.

Comment 17

[Geoff Brown [:gbrown]]

Yes, I think that's the main (only) reason we use /sdcard for the profile -- less permission issues. 

Reftests start fennec with "-profile /mnt/sdcard/tests/reftest/profile/" on the command line.

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

I did another test with profile at /sdcard/profiles/testing and Fennec started with:
> adb shell 'am start -a android.activity.MAIN -n org.mozilla.fennec_sebastian/.App --es args "--profile /sdcard/profiles/testing"'

The modified DownloadContentService downloaded the fonts to /sdcard/profiles/testing/fonts/ and they have been used on the test site.

Then I moved the fonts to a new folder and started Fennec with the profile set to the new folder (to more simulate what's happening in reftests). This worked too.

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: justfonts.list

I applied the patch locally and used the attached, limited list of reftests (reftest-sanity) to test locally. I see those tests fail too.

Looking at the external storage during test I only see the following files in the fonts sub directory:
> ./reftest/profile/fonts:
> mathfont.properties
> mathfontSTIXGeneral.properties
> mathfontUnicode.properties

After pushing the fonts to this folder myself (adb) the tests pass. I wonder if the fonts never end up in the profile directory or if they are replaced by this mathfont files.

Comment 20

[Geoff Brown [:gbrown]]

:sebastian originally suggested taking the fonts from <srcdir>/mobile/android/fonts, but I changed that to <objdir>/dist/bin/res/fonts. At the time, I was sure that the objdir directory had all the same font files as the srcdir + the mathfont properties; now I find that <objdir>/dist/bin/res/fonts only contains the mathfont properties.

I am testing a new version of my patch that copies fonts from <srcdir>/mobile/android/fonts.

Comment 21

[Geoff Brown [:gbrown]]

I repeated my earlier try experiment. First, I just set MOZ_ANDROID_EXCLUDE_FONTS=1, in https://treeherder.mozilla.org/#/jobs?repo=try&revision=06c608df8d8d; many reftests failed. eg, R13:

REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/svg/text/simple-anchor-end.svg | image comparison (==), max difference: 200, number of differing pixels: 39
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/svg/text/simple-multiple-dx-anchor-end.svg | image comparison (==), max difference: 200, number of differing pixels: 39
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/svg/text/multiple-x-multiple-dx-anchor-end.svg | image comparison (==), max difference: 200, number of differing pixels: 39 

On top of that, I pushed my patch but with a modification to source fonts from <srcdir>/mobile/android/fonts instead of <objdir>/dist/bin/res/fonts: https://treeherder.mozilla.org/#/jobs?repo=try&revision=13348188a8ec; this resolved all reftest failures except for several mathml failures:

REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/mirror-op-1.html | image comparison (==), max difference: 255, number of differing pixels: 343
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/mirror-op-2.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/mirror-op-3.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/mirror-op-4.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/mfenced-10.html | image comparison (==), max difference: 255, number of differing pixels: 612
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/mfenced-11.html | image comparison (==), max difference: 255, number of differing pixels: 175
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/mfenced-12.html | image comparison (==), max difference: 255, number of differing pixels: 5174
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/stretchy-munderover-1a.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/stretchy-munderover-2a.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/stretchy-munderover-3a.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/stretchy-munderover-3d.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/stretchy-mfenced-1a.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/stretchy-mfenced-3a.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/stretchy-mfenced-4a.html | image comparison (==), max difference: 254, number of differing pixels: 845
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/stretchy-mfenced-4b.html | image comparison (==), max difference: 254, number of differing pixels: 845
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/mathml/stretchy-mover-2a.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/stretchy-mover-2b.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/stretchy-largeop-1.html | image comparison (==), max difference: 255, number of differing pixels: 162
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/stretchy-largeop-2.html | image comparison (==), max difference: 255, number of differing pixels: 1650
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/embellished-op-1-1.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/embellished-op-1-2.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/embellished-op-1-3.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/embellished-op-1-4.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/embellished-op-1-5.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/embellished-op-2-1.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/embellished-op-2-2.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/embellished-op-2-3.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/embellished-op-2-4.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/embellished-op-3-1.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/embellished-op-3-2.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/embellished-op-3-3.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/embellished-op-3-4.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/embellished-op-3-5.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/scale-stretchy-2.xhtml | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/scale-stretchy-4.xhtml | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/scale-stretchy-5.xhtml | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/stretchy-1.html | image comparison (!=) 


REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/scriptlevel-movablelimits-1.html | image comparison (==), max difference: 255, number of differing pixels: 597
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/mo-lspace-rspace.html | image comparison (==), max difference: 255, number of differing pixels: 128
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/mo-lspace-rspace-2.html | image comparison (==), max difference: 255, number of differing pixels: 15987
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/mo-lspace-rspace-3.html | image comparison (==), max difference: 255, number of differing pixels: 693
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/mo-invisibleoperators.html | image comparison (==), max difference: 255, number of differing pixels: 377
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/opentype-stretchy.html | image comparison (==), max difference: 255, number of differing pixels: 368
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/op-dict-1.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/op-dict-3.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/op-dict-5.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/op-dict-7.html | image comparison (!=)
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/op-dict-8.html | image comparison (==), max difference: 255, number of differing pixels: 210
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/op-dict-10.html | image comparison (==), max difference: 255, number of differing pixels: 250
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/mathml/op-dict-12.html | image comparison (==), max difference: 236, number of differing pixels: 62 

I suspect the mathml failures are related to the mathfont properties files:

-rw-rw-r-- 1 gbrown gbrown   90317 Jan  1  2010 mathfont.properties
-rw-rw-r-- 1 gbrown gbrown    6211 Jan  1  2010 mathfontSTIXGeneral.properties
-rw-rw-r-- 1 gbrown gbrown    3096 Jan  1  2010 mathfontUnicode.properties

These are normally found in the omni.jar, in assets/res/fonts, but they are not present if MOZ_ANDROID_EXCLUDE_FONTS=1. Including these files in the test profile does not seem to help. Consider http://hg.mozilla.org/mozilla-central/annotate/59a6ad6a921f/layout/mathml/nsMathMLChar.cpp#l162 -- it looks like there is no provision for reading from <profile>/fonts.

Comment 22

[Geoff Brown [:gbrown]]

There is one other complication: 2 mathml mochitests fail with MOZ_ANDROID_EXCLUDE_FONTS=1:

http://archive.mozilla.org/pub/mobile/try-builds/[e-mail]-13348188a8ec9dec7389bb42e295dc1f26a8620b/try-android-api-11/try_ubuntu64_vm_armv7_large_test-mochitest-15-bm123-tests1-linux64-build211.txt.gz

165 INFO TEST-UNEXPECTED-FAIL | layout/mathml/tests/test_opentype-limits.html | LowerLimitBaselineDropMin
166 INFO TEST-UNEXPECTED-FAIL | layout/mathml/tests/test_opentype-limits.html | Bad AccentBaseHeight
173 INFO TEST-UNEXPECTED-FAIL | layout/mathml/tests/test_opentype-radical.html | Bad RadicalDegreeBottomRaisePercent 


I think we need to either include the mathfont properties in the omni.jar, or update the mathml code to recognize <profile>/fonts; if the latter, I'll need to also provide <profile>/fonts for mochitests.

Comment 23

[Geoff Brown [:gbrown]]

Attachment: on android, include fonts in reftest profile, v2

This is like the previous version, but makes some small adjustments:
 - Include the .ttf files in <objdir>/dist/bin/res/fonts regardless of MOZ_ANDROID_EXCLUDE_FONTS. All font files are still excluded from the apk with MOZ_ANDROID_EXCLUDE_FONTS; this just gives us one place to find all the font files (ttf + mathfont properties) required for tests.
 - Copy fonts to the mochitest profile too, to allow for tests like layout/mathml/tests/test_opentype-limits.html.

Comment 24

[Geoff Brown [:gbrown]]

Attachment: search for mathfont properties in <profile>/fonts too

We will need to do something like this if we want to exclude the mathfont properties files from the apk.

:sebastian -- Are you familiar with the mathfont properties files? Should we have them in the apk, or make a change like this so that the associated tests will pick them up from <profile>/fonts?

This works fine:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=25cb1163d6be

has MOZ_ANDROID_EXCLUDE_FONTS=1, no ttf or mathfont properties in the apk, and all tests pass (ttf and mathfont properties files are copied to <profile>/fonts).

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

I am surprised that we exclude these mathml files with MOZ_ANDROID_EXCLUDE_FONTS. They are tiny:

> lrwxr-xr-x   1 sebastian  staff    75B Nov  5 09:51 mathfont.properties
> lrwxr-xr-x   1 sebastian  staff    86B Nov  5 09:51 mathfontSTIXGeneral.properties
> lrwxr-xr-x   1 sebastian  staff    82B Nov  5 09:51 mathfontUnicode.properties

The flag has been introduced in bug 1063868. (Changeset: https://hg.mozilla.org/integration/fx-team/rev/b1821478bc41)

I assume that this removes the .properties files from the omnija:
> +#ifndef MOZ_ANDROID_EXCLUDE_FONTS
> +@BINPATH@/res/fonts/*
> +#endif

Richard: Given that we did this to reduce the APK size (I assume) - do we even need to exclude the .properties files. Afaik they are not related to the .ttf files? And I guess we do not want to download those tiny files. So should we just continue to ship them in the APK (ignoring MOZ_ANDROID_EXCLUDE_FONTS)?

Comment 26

[Richard Newman [:rnewman]]

I would expect that it's harmless to include them, if you're willing to futz with the build process to only exclude .ttf!

Comment 27

[Geoff Brown [:gbrown]]

(In reply to Sebastian Kaspari (:sebastian) from comment #25)
> I am surprised that we exclude these mathml files with
> MOZ_ANDROID_EXCLUDE_FONTS. They are tiny:
> 
> > lrwxr-xr-x   1 sebastian  staff    75B Nov  5 09:51 mathfont.properties
> > lrwxr-xr-x   1 sebastian  staff    86B Nov  5 09:51 mathfontSTIXGeneral.properties
> > lrwxr-xr-x   1 sebastian  staff    82B Nov  5 09:51 mathfontUnicode.properties

That might be the size of the links. The files are a little bigger:

-rw-rw-r-- 1 gbrown gbrown 92229 Sep  2 08:22 layout/mathml/mathfont.properties
-rw-r--r-- 1 gbrown gbrown  7328 May  9  2014 layout/mathml/mathfontSTIXGeneral.properties
-rw-r--r-- 1 gbrown gbrown  5017 Feb  7  2014 layout/mathml/mathfontUnicode.properties

Comment 28

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8682816 [details] [diff] [review]
on android, include fonts in reftest profile, v2

Review of attachment 8682816 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/fonts/moz.build
@@ +3,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +RESOURCE_FILES.fonts += [

Is this needed so that the fonts show up in dist/bin/res/fonts? But they are not packed into the omni.ja/APK because of package-manifest.in?

I assume we can't reference them from the source directory but need them somewhere in the obj-* directory for the tests to pick them up?

Comment 29

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8682822 [details] [diff] [review]
search for mathfont properties in <profile>/fonts too

Review of attachment 8682822 [details] [diff] [review]:
-----------------------------------------------------------------

With the changes in bug 1222124 we won't need this anymore, right?

If those files every get big enough for us to care more, we can follow this approach and download them like everything else. For downloaded fonts we notify Gecko to rebuild the font list. I wonder what would be needed to notify an already running Gecko that those files are now available. But this is something we don't need to worry about for now.

Comment 30

[Geoff Brown [:gbrown]]

(In reply to Sebastian Kaspari (:sebastian) from comment #29)
> With the changes in bug 1222124 we won't need this anymore, right?

Correct.

Comment 31

[Geoff Brown [:gbrown]]

(In reply to Sebastian Kaspari (:sebastian) from comment #28)
> > +RESOURCE_FILES.fonts += [
> 
> Is this needed so that the fonts show up in dist/bin/res/fonts? But they are
> not packed into the omni.ja/APK because of package-manifest.in?

The issue here was that the tests needed to pick up both the ttf files and the properties from a single directory. I was using the objdir just as a convenient location to copy the files from.

Now that mathfont properties will be included in the apk, I can just copy the ttf files from the source directory, so I don't need this change.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6acba962a5d

Comment 32

[Geoff Brown [:gbrown]]

Attachment: on android, include fonts in test profiles

Minor updates to the original patch. r=jmaher carried.

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23589b27e751

Comment 34

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Geoff Brown [:gbrown] from comment #31)
> The issue here was that the tests needed to pick up both the ttf files and
> the properties from a single directory. I was using the objdir just as a
> convenient location to copy the files from.

Awesome! :)

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/23589b27e751