The font support in gfxAndroidPlatform is not properly integrated with the generic (cross-platform) font management, and effectively duplicates functionality that is available in gfxPlatformFontList and supporting code. One consequence of this is that the Android code actually ends up building two copies of the list of system fonts, because it instantiates gfxFT2FontList (which scans the /system/fonts/ directory to make a list), but then it also independently builds its own font list; which of the two gets used during font lookup is not always immediately clear. The main benefit that the gfxAndroidPlatform version provides is the IPC support that allows content processes to retrieve the font list from the chrome process rather than independently re-scanning the fonts directory. However, some of the benefit is lost because the content process still instantiates its own gfxFT2FontList, which does its own scan. There's also support in gfxAndroidPlatform for caching the list of fonts in the StartupCache, to avoid having to read all the font files on subsequent startups. However, there are also shortcomings (aside from the fact that gfxAndroidPlatform ends up duplicating functionality that is maintained - for cross-platform use - elsewhere). In particular, the gfxAndroidPlatform font-scan ONLY supports .ttf files, not .otf or .ttc, which would prevent the use of such fonts if users have them on the device. Although (AFAIK) current devices only ship with .ttf fonts, there's no reason to restrict our support to this. In addition, there appear to be a couple of bugs in the StartupCache font-list support: I don't think it would handle the removal of a font file very well (the entry would remain in the cache, uselessly bloating it), and its support for files with multiple faces and for non-ASCII font names looks broken (although no current devices are probably affected by this). What I propose is to refactor the code so as to remove the "extra" font-list implementation from gfxAndroidPlatform, and instead use gfxFT2FontList as intended to provide all font list management. Mostly, this means removing the redundant code from gfxAndroidPlatform and instead forwarding requests to the font-list object. gfxFT2FontList will be enhanced by adding the IPC (chrome/content) and StartupCache support. In addition to cleaning up and unifying font-list code, and avoiding duplicate scanning of the fonts directory, the use of a standard gfxPlatformFontList subclass to manage fonts will allow us to dispense entirely with gfxFT2FontGroup.

Current work-in-progress as discussed above - this seems to be working well, but I'm still doing more testing....

A tryserver build is available at https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jkew@mozilla.com-1a8a3cef47d4/. Testing would appreciated on a variety of devices (I only have an HTC phone available locally). In particular, it'd be good to check that this does not regress bug 636042 (as it removes the code that was patched there, and replaces it with a somewhat different implementation), and that it does not regress startup performance (actually, I hope it'll show a slight improvement). cc-ing some folk from bug 636042 in the hope that you can test on your devices - thanks!

Fonts look good to me with that tryserver build on my Galaxy S 2.

Optimized and tidied up a bit further. Tryserver results look ok (modulo the usual android unit-test and talos flakiness).

Note to self: if bug 678785 gets landed before this, it will conflict - make sure that fix doesn't get lost in a merge.

What's the advantage of keeping the weight/width/style info in the startup cache?

(In reply to John Daggett (:jtd) from comment #7) > What's the advantage of keeping the weight/width/style info in the startup > cache? This lets us directly reconstruct the gfxFontFamily and its gfxFontEntry faces from the cached info, rather than an intermediate stage of "shell" families that hold a list of file paths, and have to be populated with actual face entries later. With that strategy, the result is that when we want to use a given family during font-matching, we have to instantiate Freetype faces (and hence hit the filesystem) for *all* the entries, in order to find their properties and figure out which *one* of the faces we really want. By caching these properties, we avoid this: what we retrieve from the cache is a full set of family/face records, *with* their style-matching properties, so that we can perform font-matching without needing to instantiate FT faces for all the style variants of the family - we only need to create the FT face for the individual entry that we actually choose to use. The difference is pretty marginal on typical phones, I guess, as there are only a few families that have multiple styles, and in the case of a single-style family the end result is essentially equivalent (though I think caching the info still simplifies the restore-from-cache process, compared to the two-stage version). But as we get more advanced Android devices such as larger tablets, it seems likely there will be larger and more complete collections of fonts available, whether preinstalled or as a result of user additions, and so the benefit of lazy FTFace instantiation (only creating them for the style we actually use in a given family, rather than always creating them for all styles even when we only need one of them) will be correspondingly greater.

Looks good but I need to go through and carefully diff all the blocks that have moved around, so I'll do that tomorrow and finish the review then.

Just a minor update - merged to current trunk, which now includes bug 678785.

(In reply to John Daggett (:jtd) from comment #9) > Looks good but I need to go through and carefully diff all the blocks that > have moved around, so I'll do that tomorrow and finish the review then. Ping?

Comment on attachment 560174 [details] [diff] [review] patch, refactor and fix font-list handling for Android Looks fine but if we're revamping the structure like this it seems like we should also (1) rename FontEntry to FT2FontEntry and (2) move the FontEntry and FamilyEntry classes to gfxFT2FontList.cpp. I'll leave that up to you to decide. r+ based on the assumption that actual testing of the time spent within InitFontList shows no regression from existing behavior.

(In reply to John Daggett (:jtd) from comment #12) > Looks fine but if we're revamping the structure like this it seems > like we should also (1) rename FontEntry to FT2FontEntry and (2) move > the FontEntry and FamilyEntry classes to gfxFT2FontList.cpp. I'll > leave that up to you to decide. OK, this seems like a good time for that - I'll post a separate patch for that, though. > r+ based on the assumption that actual testing of the time spent > within InitFontList shows no regression from existing behavior. I put some instrumentation into gfxPlatform::Init(), to measure the total time spent constructing the gfxAndroidPlatform and gfxFT2FontList objects (as the existing code does font-list construction in both of these). On my HTC phone, I get timings of approx 53ms on first run (i.e. before the startupcache has been populated), and 30ms on subsequent startups (when gfxAndroidPlatform gets its font list from the cache, but gfxFT2FontList still scans the filesystem). With this patch, there's a clear improvement: first-run time for this section of code drops to 27ms (yes, better than the existing "warm-start" time), and subsequent runs (when the startup cache is used) are 2ms. As this is happening on the main thread during startup, it should translate directly to a startup improvement of approx 28ms (for my particular device; YMMV). BTW, these timings all relate to the chrome process (which is what matters for initial display of the default start page). Content processes are a different story: they get the font list via IPC from the chrome process, and this seems to take widely varying times in the range of 120ms - 300ms (on my HTC device). From that, it appears that we might win substantially by scrapping the IPC font-list stuff, and just scanning the filesystem from the content process (better still, of course, if we could read the startup cache from there, but AFAIK that's not permitted). But that's a separate issue that we might want to investigate as a followup, not part of this bug.

No functional change, just reorganization of the source. This moves the FT2 font-list classes FontEntry and FontFamily over to the gfxFT2FontList.cpp/.h source files, where they more logically belong, and adds the "FT2" prefix to their names for consistency with the pattern used on other platforms. (Further clean-up we might want to do at some point would be to strip out the WinMo-specific code from these files, as we no longer support that platform. I wouldn't be surprised if the code is broken by now, but AFAIK it's not getting built or tested so we wouldn't really know.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/864c08bd7c63 https://hg.mozilla.org/integration/mozilla-inbound/rev/f4b1fa7a0f31

Backed out of inbound for Android bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/6c12bf7c3616 https://hg.mozilla.org/integration/mozilla-inbound/rev/e7895429a628 https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f4b1fa7a0f31 eg: https://tbpl.mozilla.org/php/getParsedLog.php?id=6515587&tree=Mozilla-Inbound { In file included from /builds/slave/m-in-lnx-andrd/build/dom/plugins/base/android/android_npapi.h:40, from /builds/slave/m-in-lnx-andrd/build/dom/plugins/base/android/ANPBase.h:39, from /builds/slave/m-in-lnx-andrd/build/dom/plugins/base/android/ANPTypeface.cpp:40: /tools/android-ndk-r5c/platforms/android-5/arch-arm/usr/include/jni.h:489: note: the mangling of 'va_list' has changed in GCC 4.4 /builds/slave/m-in-lnx-andrd/build/dom/plugins/base/android/ANPTypeface.cpp: In function 'ANPTypeface* anp_typeface_createFromName(const char*, ANPTypefaceStyle)': /builds/slave/m-in-lnx-andrd/build/dom/plugins/base/android/ANPTypeface.cpp:64: error: no matching function for call to 'gfxFT2Font::GetOrMakeFont(NS_ConvertASCIItoUTF16, gfxFontStyle*)' ../../../../dist/include/gfxFT2Fonts.h:71: note: candidates are: static already_AddRefed<gfxFT2Font> gfxFT2Font::GetOrMakeFont(FT2FontEntry*, const gfxFontStyle*, PRBool) /builds/slave/m-in-lnx-andrd/build/dom/plugins/base/android/ANPTypeface.cpp:63: warning: unused variable 'p' }

Re-landed on inbound, after rebasing to latest tree and fixing the build failure: https://hg.mozilla.org/integration/mozilla-inbound/rev/6081c809fcf6 https://hg.mozilla.org/integration/mozilla-inbound/rev/065af81cd739

https://hg.mozilla.org/mozilla-central/rev/6081c809fcf6 https://hg.mozilla.org/mozilla-central/rev/065af81cd739

Note that this code free()s a buffer returned from the StartupCache, which may or may not be the right thing to do. Once bug 684700 is fixed, we'll need to check whether this is being done correctly, here and elsewhere in the codebase.