Closed Bug 831354 Opened 7 years ago Closed 7 years ago

Ship fonts for content in Firefox for Android

Categories

(Firefox for Android :: Theme and Visual Design, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 21
Tracking Status
relnote-firefox --- 21+

People

(Reporter: ibarlow, Assigned: jfkthame)

References

Details

Attachments

(12 files, 3 obsolete files)

3.34 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
3.67 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
6.33 KB, patch
jwir3
: review+
Details | Diff | Splinter Review
1.12 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
927 bytes, patch
jfkthame
: review+
Details | Diff | Splinter Review
4.11 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.58 MB, patch
blassey
: review+
Details | Diff | Splinter Review
3.63 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.22 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.32 KB, patch
blassey
: review+
Details | Diff | Splinter Review
1.54 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
5.58 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
According to Wikipedia Google commissioned Open Sans and Charis by SIL. They point to different download sources. 

http://www.google.com/webfonts#UsePlace:use/Collection:Open+Sans

http://scripts.sil.org/cms/scripts/page.php?site_id=nrsi&item_id=CharisSILfont
Whichever source you prefer. :)
Attached patch patch (obsolete) — Splinter Review
Attachment #702920 - Flags: review?(mark.finkle)
Comment on attachment 702920 [details] [diff] [review]
patch

err...wrong font files. New patch coming
Attachment #702920 - Attachment is obsolete: true
Attachment #702920 - Flags: review?(mark.finkle)
patch with font files is too big for bugzilla's brain, so I posted it here http://dump.lassey.us/ship_open_fonts.patch
Attachment #702936 - Flags: review?(mark.finkle)
Attachment #702936 - Flags: review?(mark.finkle) → review+
Regression: Mozilla-Inbound - Robocop Pan Benchmark - Android 2.2 (Native) - 38.6% increase
-------------------------------------------------------------------------------------------
    Previous: avg 8663.460 stddev 956.659 of 30 runs up to revision c4b6301bee26
    New     : avg 12010.640 stddev 626.874 of 5 runs since revision 780c48afccc1
    Change  : +3347.180 (38.6% / z=3.499)
    Graph   : http://mzl.la/U21700
We should be using the Charis SIL Compact release of Charis, not the "default" release, otherwise we'll get excessively wide default line spacing in blocks of serif text.

  http://scripts.sil.org/ttw/fonts2go.cgi?family=CharisSIL&pkg=Compact&ver=4.110

The default font prefs should be changed (Roboto/Droid Sans -> Open Sans, Droid Serif -> Charis SIL Compact) in all the language prefs that reference them, not just for x-western.

With those fixes, let's have a tryserver run and see what residue there is.
Assignee: blassey.bugs → dbaron
Comment on attachment 703349 [details] [diff] [review]
, test fix 1:  Make continuous-inline-1{cd} tests no longer assume that the height of an inline in the default font is less than 20px.

r=me
Attachment #703349 - Flags: review?(bzbarsky) → review+
Attachment #703367 - Flags: review?(dholbert) → review+
Attachment #703370 - Flags: review?(jfkthame) → review+
Attachment #703385 - Flags: review?(dbaron) → review+
I've pushed a tryserver job with the Charis SIL Compact fonts instead of Charis SIL and with updated per-language font prefs, along with these various fixes. Let's see how things look after that...

https://tbpl.mozilla.org/?tree=Try&rev=7599a0d2f417
Attachment #703359 - Flags: review?(sjohnson) → review+
Blocks: 831883
flexbox-align-self-vert-rtl-1.xhtml wasn't among the failures in comment 16's try run, so I think that means "test fix 5" does what it's supposed to do.  Pushed it:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/64d9b755c25e
Flagging as [leave open] so that the test patches won't end up inadvertantly auto closing this bug when they're merged to m-c.
Whiteboard: [leave open]
In comment 16's try run, font-inflation/select-combobox-1.html is among the failures; dbaron, does this mean that "test fix 2" didn't work out quite as well as hoped?
Updated version of the fonts patch. Still looking into a few test failures that this causes (see tryserver run above).
(In reply to Jonathan Kew (:jfkthame) from comment #20)
> In comment 16's try run, font-inflation/select-combobox-1.html is among the
> failures; dbaron, does this mean that "test fix 2" didn't work out quite as
> well as hoped?

No, it means that in test fix 2, I was really aiming to fix select-combobox-2, which was what was broken the last time around, but I applied the same fix to select-combobox-1, which was largely similar.  However, when doing that, I failed to notice that select-combobox-1.html (but not its reference) had line-height: 1.0 in it -- which should be removed, I think.

I'd push a fix, except inbound is closed.
With the current Open Sans font prefs (which -don't- include specifying it for Arabic, in particular), we can expect the canvas and svg lang-font tests to pass on Android, and so the existing fails() annotations can be removed, along with a minor fix in the svg version of the test.
Attachment #703623 - Flags: review?(dbaron)
Attachment #703623 - Flags: review?(dbaron) → review+
Followup to test fix 2 landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03fb2a777aec

Also, I seem to have accidentally click the "assign to me" button earlier.  Fixing that.
Assignee: dbaron → blassey.bugs
OS: Mac OS X → Android
Hardware: x86 → All
The greek-uppercase-1 test fails because with the new font prefs, we pick Charis for the English-tagged element (note that the language tagging there is a necessary part of the test - the point is to test that uppercasing behavior is *different* depending on the language tag), but Charis doesn't have most of the Greek characters, so fallback kicks in and we get a mixture of Charis and Droid Serif; and then the combining diacritic has little chance of behaving properly across the font-run boundary. Just specifying Droid Serif explicitly in the font-family list ought to fix this.
Attachment #703843 - Flags: review?(dbaron)
Attachment #703843 - Flags: review?(dbaron) → review+
With the various fixes here, I think we're down to basically two issues remaining:

(1) layout/reftests/text/subpixel-lineheight-1a.html fails, because the default line height does not get snapped to a whole number of pixels, AFAICS. I think this is a genuine code bug, and the test just "got lucky" up till now - the same seems to be true on OS X, too. As such, I think we should mark the test as failing, file a new bug on this, and move on - it needn't block this bug.

(2) There are several failures (some of them intermittent) in layout/reftests/scrolling/* testcases. I think these also represent a real bug; in particular, see the persistent failure in scrolling/transformed-1.html at https://tbpl.mozilla.org/?tree=Try&rev=7599a0d2f417. It looks like one row of pixels within the last full line of text is getting duplicated, resulting in glyphs that look slightly "stretched". This seems like it must be a bug somewhere in layers/repainting/scrolling/compositing.
Latest tryserver job, showing just the two issues from comment 27:
https://tbpl.mozilla.org/?tree=Try&rev=1e1972abacb0
FTR, filed bug 832313 regarding the fragility of subpixel-lineheight on OS X (it doesn't snap default line-height to pixels). I'm not yet sure why it's proving troublesome on Android, as we do seem to be rounding to pixels there.
Attachment #703575 - Flags: review?(blassey.bugs)
Comment on attachment 703575 [details] [diff] [review]
Ship fonts for content in Firefox for Android

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

::: modules/libpref/src/init/all.js
@@ +3184,4 @@
>  pref("font.name.monospace.zh-HK", "Droid Sans Mono");
>  
> +pref("font.name.serif.zh-TW", "Charis SIL Compact");
> +pref("font.name.sans-serif.zh-TW", "Open Sans");

shouldn't we keep the Droid Sans fallback? so:

pref("font.name.sans-serif.zh-TW", "Open Sans, Droid Sans");

And the same comment for all of these.
Given that we're shipping Open Sans, so we -know- it's present, I don't think there's any need to add a fallback to the font.name.* prefs (which aren't really intended to take a list in the first place, the fact that it works is an accident!)
Attachment #703575 - Flags: review?(blassey.bugs) → review+
This marks the remaining problem reftests as fuzzy (actually, subpixel-lineheight was already fuzzy, we're just making it more so). Of the scrolling tests, only transformed-1 seems to be permanently orange, the others are intermittent, but from all the fuzzy and random annotations in the manifest there, it looks like lots of these tests are unreliable already. I think with this, we can land the fonts; waiting on tryserver reftest results to confirm this. https://tbpl.mozilla.org/?tree=Try&rev=ef80755dc784.
Attachment #704060 - Flags: review?(blassey.bugs)
Attachment #704060 - Flags: review?(blassey.bugs) → review+
Hmm - so it looks like this may have turned B2G R10 orange with an "unexpected pass" (reftests/text/fallback-01.xhtml), probably due to the changed prefs. If that persists, I think we should just update the manifest accordingly; it doesn't look like a reason to back out. But I'll look at it more carefully tomorrow, if nobody does anything with it in the meantime.
Unfortunately, I did something about it by backing it out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d670603dc3c2 before I read that comment, though (if we've actually run every b2g reftest chunk by now, which is doubtful) your total score was:

R10 pass: https://tbpl.mozilla.org/php/getParsedLog.php?id=18942023&tree=Mozilla-Inbound
R1 2 fails: https://tbpl.mozilla.org/php/getParsedLog.php?id=18943080&tree=Mozilla-Inbound
R7 fail: https://tbpl.mozilla.org/php/getParsedLog.php?id=18945446&tree=Mozilla-Inbound

which is a little more than I would just adjust for on my own tick.
OK, so it looks like we use the "Android" prefs on B2G as well - that caught me out, I assumed it would have its own font prefs. So B2G is currently relying on the Droid Sans and Droid Serif setting of the font.name.* prefs; changing those to Open Sans and Charis SIL Compact respectively but not including those fonts in the B2G build causes a few tests to break because it no longer finds a serif font, everything falls back to Droid Sans.

What we should do here, then, is to provide parallel font.name-list.* prefs that include the Droid fonts, so that B2G still finds those.

I think we should then file a followup bug for B2G to have its own separate definition of the default font prefs, instead of piggy-backing on the Android prefs, as it is -expected- that it'll have a different collection of fonts available. "Solving" this by providing lists everywhere that include both the Android and B2G font names is ugly, and less performant than specifying the correct name as the primary font.name.* setting.
Pushed fixes 7 and 8 back into the tree, as those should be harmless (fix 6 cannot land until the fonts themselves re-land).
https://hg.mozilla.org/integration/mozilla-inbound/rev/47b719f58603
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1d4081f0582
Whiteboard: [leave open]
(In reply to Jonathan Kew (:jfkthame) from comment #36)
> I think we should then file a followup bug for B2G to have its own separate
> definition of the default font prefs, instead of piggy-backing on the
> Android prefs, as it is -expected- that it'll have a different collection of
> fonts available. "Solving" this by providing lists everywhere that include
> both the Android and B2G font names is ugly, and less performant than
> specifying the correct name as the primary font.name.* setting.

Might it be easier to just give B2G its own separate chunk of font
prefs now?
And re-landed the actual fonts (and Makefile changes to include them in the build), but -without- the changes to default font prefs:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b784ce7fd90f

This should get them safely into the tree, so we don't have to keep backing out and re-landing all these big files in the event that we have another bounce.

Keeping the [leave open] whiteboard annotation, as this isn't finished until we actually do the prefs change (and land "fix 6"). New prefs patch upcoming...
(In reply to David Baron [:dbaron] from comment #38)
> (In reply to Jonathan Kew (:jfkthame) from comment #36)
> > I think we should then file a followup bug for B2G to have its own separate
> > definition of the default font prefs, instead of piggy-backing on the
> > Android prefs, as it is -expected- that it'll have a different collection of
> > fonts available. "Solving" this by providing lists everywhere that include
> > both the Android and B2G font names is ugly, and less performant than
> > specifying the correct name as the primary font.name.* setting.
> 
> Might it be easier to just give B2G its own separate chunk of font
> prefs now?

Perhaps; I'm about to dig back into the prefs, I'll see how it looks.
OK, this seems like the simplest solution. Here's a patch that preserves the existing ANDROID font prefs within an #ifdef MOZ_B2G block, so that we don't affect their configuration at all, and then introduces the updated prefs using the bundled fonts for the non-B2G case. I've pushed this to tryserver (https://tbpl.mozilla.org/?tree=Try&rev=ebf209797dc8) in hopes of getting confirmation that it's green on both android -and- b2g platforms, though I can't see why it wouldn't be. As noted in a comment in the patch, I suspect we could do some cleanup of the prefs for B2G, given that we control what fonts are actually present, but that sounds like a followup bug.
Attachment #704206 - Flags: review?(dbaron)
Drat - even just landing the fonts without changing prefs causes a reftest failure, because the test involving Combining Grapheme Joiner is finding one of the new fonts during fallback (as the default font doesn't include that character), and this affects vertical metrics. This patch should fix that, by giving that testcase an explicit, large line-height so that the precise metrics of the fallback it happens to hit will not matter.
Comment on attachment 704215 [details] [diff] [review]
test fix 9 - give the CGJ reftest a large line-height, so it's less sensitive to the metrics of any fallback font that happens to be found.

Pushed this test-fix to inbound prior to review, in order to clear the R4 orange and avoid the churn of backing out the fonts again. Hope nothing bad happens as a result!

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec597237c8fe
Attachment #704215 - Flags: review?(dbaron)
Comment on attachment 704206 [details] [diff] [review]
update default font prefs to use the bundled Open Sans and Charis SIL Compact on Android.

(In reply to Jonathan Kew (:jfkthame) from comment #41)
> Created attachment 704206 [details] [diff] [review]
> update default font prefs to use the bundled Open Sans and Charis SIL
> Compact on Android.

> I've pushed this to tryserver
> (https://tbpl.mozilla.org/?tree=Try&rev=ebf209797dc8) in hopes of getting
> confirmation that it's green on both android -and- b2g platforms, though I
> can't see why it wouldn't be.

Well, that didn't go as planned - the reftest failures still show up on "B2G Arm opt". Which seems to imply that MOZ_B2G didn't have the expected effect in all.js. Time to go dig a bit further...
Attachment #704206 - Flags: review?(dbaron)
Checking inside the omni.ja file included in http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds/jkew@mozilla.com-ebf209797dc8/try-ics_armv7a_gecko/b2g-21.0a1.en-US.android-arm.tar.gz confirms that it most definitely has the ANDROID (but not MOZ_B2G) version of the font-name prefs.
According to glandium, it should work to use MOZ_WIDGET_GONK for the B2G-platform-specific prefs; trying this to see how it goes. https://tbpl.mozilla.org/?tree=Try&rev=ad484f9cc60d.
Attachment #704206 - Attachment is obsolete: true
That still failed to use the right prefs on b2g (see TBPL results). Using MOZ_WIDGET_GONK in my local build seems to behave as expected, so this is distinctly puzzling.
Are you sure the preprocessor being used for all.js understands // comments?
Turns out the problem was apparently the fact that I included // comments on the preprocessor lines. (Sigh.) So here's a version without those. Seems to work OK in my local build, at least; tryserver push is at https://tbpl.mozilla.org/?tree=Try&rev=c36e04633fb3.
Attachment #704260 - Flags: review?(dbaron)
Attachment #704225 - Attachment is obsolete: true
Comment on attachment 704215 [details] [diff] [review]
test fix 9 - give the CGJ reftest a large line-height, so it's less sensitive to the metrics of any fallback font that happens to be found.

I actually don't follow how this helps -- or do metrics of fallback fonts influence what we do for 'line-height: normal' somehow?

Anyway, r=dbaron, though I'm sort of curious how it's helping.
Attachment #704215 - Flags: review?(dbaron) → review+
Comment on attachment 704260 [details] [diff] [review]
update default font prefs to use the bundled Open Sans and Charis SIL Compact on Android.

I'm going to trust that the deltas on the actually-Android section have been previously reviewed; r=dbaron.
Attachment #704260 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] from comment #52)
> Comment on attachment 704260 [details] [diff] [review]
> update default font prefs to use the bundled Open Sans and Charis SIL
> Compact on Android.
> 
> I'm going to trust that the deltas on the actually-Android section have been
> previously reviewed; r=dbaron.

Yes, they're the same as in the earlier (fonts + prefs all at once) patch that blassey reviewed.
(In reply to David Baron [:dbaron] from comment #51)
> Comment on attachment 704215 [details] [diff] [review]
> test fix 9 - give the CGJ reftest a large line-height, so it's less
> sensitive to the metrics of any fallback font that happens to be found.
> 
> I actually don't follow how this helps -- or do metrics of fallback fonts
> influence what we do for 'line-height: normal' somehow?
>

Yes, line-height:normal will be affected if the line includes characters from fonts with larger vertical metrics, so if the CGJ falls back to a taller font, then it'll affect line height even though it's invisible.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f849e0aa18d6 (font prefs)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d4ce26f2c28 (fix 6)

And provided it sticks this time, that concludes this bug. :)
Whiteboard: [leave open]
wouldn't this increase the size of the already large (~40MB) apk?
It increased the size of inbound builds from ~23MB to ~26MB. Where are you seeing ~40MB APKs?
Sorry I looked at total size on my Android.  anyway a 3MB increase is already 12%.
Yeah, it's pretty big. Bug 831883 reduces this by ~200K which helps a bit. You can definitely the see the memory usage regression at http://people.mozilla.com/~kgupta/awsy/index.html#which=Start,when=40 (from the first landing+backout). Once that graph catches up to the re-landing it should show up as a more permanent regression.
This may have caused a regression in the Talos trobopan test: see bug 833000.
This bug represents more of a feature, hence no need to track for release.(Spoke to :elancaster offline & it was accidentally flagged)
Depends on: 841855
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #60)
> Yeah, it's pretty big. Bug 831883 reduces this by ~200K which helps a bit.
> You can definitely the see the memory usage regression at
> http://people.mozilla.com/~kgupta/awsy/index.html#which=Start,when=40 (from
> the first landing+backout). Once that graph catches up to the re-landing it
> should show up as a more permanent regression.

AWSY shows this increased the "freetype" entry in about:memory at start-up from 0.60 to 2.18 MiB, i.e. 1.58 MiB.  Was that really just for two additional fonts?
How many fonts did we have before that?  Are these new fonts especially large?  Can they be shrunk at all?  Do we know how the freetype memory is divided up?
Jonathan can answer those questions better than I can. I'd tag him for needinfo but there's no needinfo option on this bug (maybe because it's resolved?).
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #66)
> Jonathan can answer those questions better than I can. I'd tag him for
> needinfo but there's no needinfo option on this bug (maybe because it's
> resolved?).

Kats, can you file another bug to investigate the memory impact of this and mark it as needing info from johnathan?
Depends on: 844669
> Kats, can you file another bug to investigate the memory impact of this and
> mark it as needing info from johnathan?

I just filed bug 844669.
Was it intended that these ship in B2G?  I'm seeing these fonts in out/target/otoro/product/system/fonts in my B2G otoro build.
You need to log in before you can comment on or make changes to this bug.