1197717 - Load fonts from writable location

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

This is an intermediate step for bug 1194338.

Currently we package fonts into omni.ja in the assets folder of the APK. In order to support downloadable fonts we need to read the fonts from a location that is writeable by the downloader.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Richard, can you help me to understand how fonts are loaded? I've been reading the patches of bug 648548 to start. There the fonts are saved to the profile but I assume we want to use a profile-agnostic location here (that can be read by guest profiles, reftests, .. too)? Do you have any pointers to code or bugs for me? :)

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Just found bug 878674 where stopped copying fonts from the APK to the filesystem (to use them from there) and started to load them directly from the APK. This bug might have some useful hints too.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Also helpful: Bug 785291 - Add support to load fonts from an APK.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Font loading from profile directories has been implemented in bug 619521. This bug should contain information to extend font loading to more directories. It's a big fennec font loading history lesson for me. :)

Comment 5

[Richard Newman [:rnewman]]

Yup, it's basically adding another directory search path to the work done in that bug.

Although on Android there's almost a 1:1 correspondence between profiles and users, I want to make sure we don't download fonts each time we enter guest mode, or have guest mode fail to find fonts.

That means we'll be needing an additional non-profile-linked directory here, into which we'll copy the downloaded (eventually; packaged for testing) fonts. Naturally we'll be using per-app shared prefs for tracking.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1197717-font-loading.patch

In addition to the system's font folder and the profile folder this patch will try to load fonts from the profile independent NS_APP_USER_PROFILES_ROOT_DIR/fonts folder.

I've never written production C++ code so I'm not sure if this is the best way. This patch has been created by mostly imitating the surrounding code. ;)

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Oh and I tested the patch by writing some Java helper code that copies the fonts from the external storage to this internal storage location (in a build with MOZ_ANDROID_EXCLUDE_FONTS=1) - roughly like the download service will do later. The copied fonts are picked up and used to render the test site[1]. I've also been running a subset of the reftests (font-serif.html) and they pass.

[1]: http://www.zipcon.net/~swhite/docs/computers/browsers/fonttest.html

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8652804 [details] [diff] [review]
1197717-font-loading.patch

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

LGTM, but I'm not a gfx peer.

Jeff, could you review this or 302 to someone who can? We're building font downloading into Fennec, and step one is loading from a profile-agnostic location.

Comment 9

[Richard Newman [:rnewman]]

Also, I suspect that the profiles dir is not the right place to put this. I think we want the entry that corresponds to GetUserLocalDataDirectory -- the parent of NS_APP_USER_PROFILES_ROOT_DIR -- but I don't see an nsAppDirectoryServiceDefs define for it.

Comment 10

[John Daggett (:jtd)]

Comment on attachment 8652804 [details] [diff] [review]
1197717-font-loading.patch

Patch looks fine but it seems like we should be doing something like gfxFcPlatformFontList::ActivateBundledFonts which is what we do on other platforms. Not sure what there isn't this method for Android code. Switching the review to Jonathan, as he wrote that code.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #9)
> Also, I suspect that the profiles dir is not the right place to put this. I
> think we want the entry that corresponds to GetUserLocalDataDirectory -- the
> parent of NS_APP_USER_PROFILES_ROOT_DIR -- but I don't see an
> nsAppDirectoryServiceDefs define for it.

Throwing in my 2 cents: I think the hyphenation code does a good job using the directory service to look for dict files in /data/data/org.mozilla.fennec/hyphenation, among other locations, including inside the omnijar.

http://mxr.mozilla.org/mozilla-central/source/intl/hyphenation/nsHyphenationManager.cpp#138

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8652804 [details] [diff] [review]
1197717-font-loading.patch

Alright, I'm trying to rewrite the patch to replicate what we are doing with hyphenation dictionaries.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1197717-font-loading-v2.patch

This patch now loads fonts from (in my case, on my device):
/data/user/0/org.mozilla.fennec_sebastian/fonts

.. just like hyphenation dictionaries that are loaded from (among other locations):
/data/user/0/org.mozilla.fennec_sebastian/hyphenation

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 8656640 [details] [diff] [review]
1197717-font-loading-v2.patch

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

I haven't been closely following the discussions of where downloaded resources like this should be stored; but assuming this finds the right folder, the code looks OK. A couple of minor points before landing:

1. The commit message needs updating, as it's not starting from NS_APP_USER_PROFILES_ROOT_DIR any more (that was an earlier version, IIUC).

2. Please move this chunk *before* the existing code that loads fonts from the user's current profile directory. Such fonts have presumably been added/downloaded by the individual user, so we should load them last, after all "standard" product fonts (i.e. from the /system/fonts directory, omnijar, and now these new downloaded resources).

r=me with those fixups.

Comment 15

[Jonathan Kew [:jfkthame]]

Wait a minute... the patch here is looking at a directory under NS_XPCOM_CURRENT_PROCESS_DIR. Is that necessarily going to be a writable location?

Comment 16

[Richard Newman [:rnewman]]

Comment on attachment 8656640 [details] [diff] [review]
1197717-font-loading-v2.patch

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

Update the long commit description; it's outdated.

::: gfx/thebes/gfxFT2FontList.cpp
@@ +1219,5 @@
>              FindFontsInDir(localPath, &fnc, FT2FontFamily::kVisible);
>          }
>      }
>  
> +    // look for downloaded fonts in a profile-agnostic "fonts" directory

Nit: proper sentence. Capitalization and punctuation.

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Jonathan Kew (:jfkthame) from comment #15)
> Wait a minute... the patch here is looking at a directory under
> NS_XPCOM_CURRENT_PROCESS_DIR. Is that necessarily going to be a writable
> location?

Yes, I have been able to write to this location from the Java world on Android. Does it make sense to move this behind some Android flag (which one?)?

Comment 18

[Jonathan Kew [:jfkthame]]

(In reply to Sebastian Kaspari (:sebastian) from comment #17)
> (In reply to Jonathan Kew (:jfkthame) from comment #15)
> > Wait a minute... the patch here is looking at a directory under
> > NS_XPCOM_CURRENT_PROCESS_DIR. Is that necessarily going to be a writable
> > location?
> 
> Yes, I have been able to write to this location from the Java world on
> Android.

Will that necessarily always be true? What happens if the app is installed on a filesystem that is then mounted as read-only, for instance?

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Jonathan Kew (:jfkthame) from comment #18)
> Will that necessarily always be true? What happens if the app is installed
> on a filesystem that is then mounted as read-only, for instance?

If a (rooted) user goes ahead an mounts /data/user/* as read-only then we probably are going to face more problems. In this case here only on the writing side and not here while reading the directory.

However it would be interesting to know what happens if Firefox is installed as system app in /systems/app. What folder does NS_XPCOM_CURRENT_PROCESS_DIR return?

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

On Android I'm writing to the "fonts" subfolder of Context.getApplicationInfo().dataDir. By definition that is the directory for persistent data[1], so it /should/ always be writable.

In my tests this is the same directory that NS_XPCOM_CURRENT_PROCESS_DIR points to. However I'm not experienced enough in C++ to understand how NS_XPCOM_CURRENT_PROCESS_DIR is resolved and whether it will point to different directories in some cases. Can you help me to understand how this constant is translated to an actual directory?

[1]: http://developer.android.com/reference/android/content/pm/ApplicationInfo.html#dataDir

Comment 21

[Jonathan Kew [:jfkthame]]

I don't know much about the directory service, but AFAICS from comments in the source (e.g. around http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsDirectoryServiceDefs.h#43), it looks like in theory this may be the directory where the executable is found. That's why I was concerned it might not always be writable; in general I'd think that writing into the location where executables are found ought to be a privileged operation, to reduce the risk of malicious code modifying it.

You probably need to check with someone who knows about the directory service and about permissions, security, etc.; sorry, that "someone" isn't me!

Comment 22

[Sebastian Kaspari (:sebastian; :pocmo)]

Okay, let's gain some experience. The Fx44 cycle has just started, so we have time to test this thoroughly in Nightly. I'm going to adapt the patch as mentioned above (outdated commit message, moving the code before the existing code that loads fonts from the user's current profile), push to try and land this. Then file a follow-up bug to investigate what happens if the app is in /systems/app, how NS_XPCOM_CURRENT_PROCESS_DIR is resolved on Android, can this folder ever be readable-only, .. etc.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f45c7ca272a0

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f45c7ca272a0