Last Comment Bug 648548 - Provide a UI to allow users to download fonts (for web content) that are missing from their system
: Provide a UI to allow users to download fonts (for web content) that are miss...
Status: NEW
[lang=js]
: uiwanted
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All Android
: P5 normal with 3 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors: Brad Lassey [:blassey] (use needinfo?)
Depends on: 1117742 619521
Blocks: mathml-fonts 631159 788450 797662 1001570 653911
  Show dependency treegraph
 
Reported: 2011-04-08 09:42 PDT by Brad Lassey [:blassey] (use needinfo?)
Modified: 2016-01-26 08:13 PST (History)
26 users (show)
fred.wang: needinfo? (jfkthame)
mgoodwin: sec‑review? (mgoodwin)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
[PoC] - offer to download additional fonts if there are unsupported characters on the page (9.42 KB, patch)
2012-06-07 15:02 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
WIP - offer to download additional fonts, if available, for characters lacking font coverage. (11.29 KB, patch)
2013-01-09 11:32 PST, Jonathan Kew (:jfkthame)
mark.finkle: feedback+
Details | Diff | Splinter Review
WIP - offer to download additional fonts, if available, for characters lacking font coverage. (11.85 KB, patch)
2014-04-26 06:04 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
WIP - offer to download additional fonts, if available, for characters lacking font coverage. (15.19 KB, patch)
2014-04-26 14:00 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 1 - Force a reflow in nsThebesFontEnumerator::UpdateFontList (4.26 KB, patch)
2014-04-28 13:14 PDT, Frédéric Wang (:fredw)
jfkthame: review-
Details | Diff | Splinter Review
Part 2 - Offer to download additional fonts, if available, for characters lacking font coverage (11.67 KB, patch)
2014-04-28 13:18 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 2 - Offer to download additional fonts, if available, for characters lacking font coverage. , (11.83 KB, patch)
2014-04-29 09:21 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
Part 1 - Force a reflow in nsThebesFontEnumerator::UpdateFontList (6.22 KB, patch)
2014-05-06 22:24 PDT, Frédéric Wang (:fredw)
jfkthame: review+
Details | Diff | Splinter Review
Part 1 - Force a reflow in nsThebesFontEnumerator::UpdateFontList (6.24 KB, patch)
2014-10-31 05:47 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 2 - Offer to download additional fonts, if available, for characters lacking font coverage. (12.19 KB, patch)
2014-10-31 05:48 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 2 - Offer to download additional fonts, if available, for characters lacking font coverage. (12.20 KB, patch)
2014-10-31 07:41 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 2 - Offer to download additional fonts, if available, for characters lacking font coverage. (13.17 KB, patch)
2014-11-11 05:52 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 2 - Offer to download additional fonts, if available, for characters lacking font coverage. (13.10 KB, patch)
2015-02-18 08:24 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 2 - Offer to download additional fonts, if available, for characters lacking font coverage. (13.11 KB, patch)
2015-02-19 13:09 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Testcase (Egyptian_hieroglyphs) (229 bytes, text/html)
2015-03-10 14:11 PDT, Frédéric Wang (:fredw)
no flags Details
Addon to install/uninstall a font for Egyptian hieroglyphs (3.74 MB, application/x-xpinstall)
2015-03-10 14:13 PDT, Frédéric Wang (:fredw)
no flags Details
Testcase (Egyptian_hieroglyphs) (205 bytes, text/html)
2015-03-10 16:07 PDT, Frédéric Wang (:fredw)
no flags Details
Part 2 - Offer to download additional fonts, if available, for characters lacking font coverage. (13.10 KB, patch)
2015-03-14 11:26 PDT, Bill Gianopoulos [:WG9s]
no flags Details | Diff | Splinter Review

Description Brad Lassey [:blassey] (use needinfo?) 2011-04-08 09:42:52 PDT
mobile devices ship with a fairly limited set of fonts and if a user views pages that use fonts their device doesn't support they up a bit of a creek. Bug 619521 is for providing the fonts, I intend for this bug to be about the UI
Comment 1 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-05-18 03:37:50 PDT
We need at least to have an idea about what UI we want here to land strings before the pull from central to aurora because as far as I know we are not allowed to land new en-US strings in aurora
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2011-05-18 05:31:26 PDT
Until we see some movement on bug 619521, there is no reason to get too worried about this bug. As of now, this feature is at risk for Fx6.
Comment 3 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-05-18 05:34:57 PDT
(In reply to comment #2)
> Until we see some movement on bug 619521, there is no reason to get too
> worried about this bug. As of now, this feature is at risk for Fx6.

Agreed. But I have asked paul if he can take a look at bug 619521 and he is ok. Depending on the complexity it could be something or not.
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-21 07:52:55 PDT
We can reset tracking flag and assign this bug when bug 619521 has landed
Comment 5 Jonathan Kew (:jfkthame) 2012-06-07 15:02:12 PDT
Created attachment 631165 [details] [diff] [review]
[PoC] - offer to download additional fonts if there are unsupported characters on the page

I'm not at all sure what kind of UI we're really going to want here (and I'm certainly not the person to design it!), so this is just intended as a proof-of-concept, not a finished patch.

Together with the platform patches in bug 619521, this posts a doorhanger notification offering to download extra fonts if there is text on the page that will be rendered as missing-glyph boxes, and a font for the relevant script(s) is available for download. (Currently, for testing purposes, available fonts are found at people.mozilla.com/~jkew/bug-619521/, but obviously that's not the long-term solution.)
Comment 6 Jonathan Kew (:jfkthame) 2012-06-09 07:35:40 PDT
I've posted a short video showing this feature in action at http://www.youtube.com/watch?v=RDTMfCajyCU.
Comment 7 Jonathan Kew (:jfkthame) 2012-06-09 07:44:22 PDT
Comment on attachment 631165 [details] [diff] [review]
[PoC] - offer to download additional fonts if there are unsupported characters on the page

Flagging mfinkle for feedback, but anyone else is welcome to dive in. I don't know anything about javascript or how the browser front-end works, etc, so this is just the quickest-and-dirtiest proof of concept I could throw together. Maybe someone on the mobile browser team could pick up this bug and do it properly?

Also flagging for ux-review; I'm well aware the UI here isn't ready to land as it stands, but I think we need UX to provide some guidance on what the desired user experience ought to be. In particular, how much user prompting should we do, versus automatically/silently pulling down the needed fonts in the background? When multiple missing scripts are detected, do we ask the user about each one (very bad UX on Wikipedia.org, I'd expect)? Do we want UI to manage such fonts, potentially removing them from the profile to save space?
Comment 8 Jonathan Kew (:jfkthame) 2012-06-09 07:52:39 PDT
And a tryserver build with bug 619521 plus this patch is at https://tbpl.mozilla.org/?tree=Try&rev=2c8e1159e847. The R2 test seems to be persistently orange, though I'm not entirely sure yet whether that's the fault of these patches or some other issue.
Comment 9 Jonathan Kew (:jfkthame) 2013-01-09 11:32:38 PST
Created attachment 699968 [details] [diff] [review]
WIP - offer to download additional fonts, if available, for characters lacking font coverage.

Updated proof-of-concept patch; UX/front-end feedback would still be appreciated on this.
Comment 10 Brad Lassey [:blassey] (use needinfo?) 2013-01-14 12:04:03 PST
Ian, Jonathan is looking for ux feedback here.
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2013-01-16 12:16:43 PST
Comment on attachment 699968 [details] [diff] [review]
WIP - offer to download additional fonts, if available, for characters lacking font coverage.


Concept looks sound and the code looks pretty good too. I primarily focused on the browser.js part. Here is some general feedback:
* We use let over var for declaring variables
* Prefer " for all strings
* I think we'd move the new code into a FontLocator (or some name) JS class so we could lazily pull it in from a separate file and keep the code in one place.

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+    Services.obs.addObserver(this, "font-needed", false);
>+    Services.obs.addObserver(this, "available-fonts-update", false);

These would be moved to the FontLocator object and we'd call it's init() method here... that's our current pattern anyway

>+  lookForNeededFonts: function(aScriptsNeeded) {
>+    if (this._missingScripts == null) {
>+      // first time, get the list of available fonts from the server
>+      this._missingScripts = 'loading';

I don't like using _missingScripts as a string here, but as an assoc array at other times. I'm ok with using a null value, but not strings.

>+      var fontServer = "http://people.mozilla.org/~jkew/bug-619521/";
>+      var ios = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);
>+      var channel = ios.newChannel(fontServer + "fonts.json", null, null);

I wonder if we should ship a JSON file of known fonts on the server? How do we update the list of fonts though? I suppose we'll always need a network pull anyway.

>+      var listener = {
>+        onStartRequest: function(aRequest, aContext) {
>+          this._json = '';
>+        },
>+        onStopRequest: function(aRequest, aContext, aStatusCode) {
>+          try {
>+            if (JSON.parse(this._json) != undefined) {
>+              // notify that we're ready to deal with pending missing-script requests
>+              Services.obs.notifyObservers(null, "available-fonts-update", this._json);
>+            }
>+          } catch(e) { }
>+          this._json = null;
>+        },
>+        onDataAvailable: function(aRequest, aContext, aInputStream, aOffset, aCount) {
>+          var sin = Cc["@mozilla.org/scriptableinputstream;1"]
>+                    .createInstance(Ci.nsIScriptableInputStream);
>+          sin.init(aInputStream);
>+          this._json += sin.read(sin.available());
>+        },
>+      };
>+
>+      channel.asyncOpen(listener, this);

Note: We might be able to use NetUtils here and remove some of the code

>+    if (this._missingScripts == 'loading') {

Again, the string vs array issue

>+      // TODO: l10n
>+      let msg = "Download fonts for additional scripts?\n  " + wantedScripts.join("\n  ");

This could be tricky if we have a lot of scripts or the names of the scripts is long or users find it confusing. Maybe we skip the scripts names and just let the user know we can download fonts to allow them to reqd the page? (or some such wordsmithing)

>+  downloadFont: function(aFontFile) {
>+    // get the profile local fonts directory
>+    var tps = Cc["@mozilla.org/toolkit/profile-service;1"]
>+              .createInstance(Ci.nsIToolkitProfileService);
>+

nit: no wrapping needed here

>+      fontsDir.create(Components.interfaces.nsIFile.DIRECTORY_TYPE, 0x1ff);

Components.interfaces  -> Ci

>+    var fontServer = "http://people.mozilla.org/~jkew/bug-619521/"; // FIXME

Seems like it should be a preference

>+    var fontURI = Services.io.newURI(fontServer + aFontFile, null, null);
>+
>+    var persist = Cc["@mozilla.org/embedding/browser/nsWebBrowserPersist;1"]
>+                  .createInstance(Ci.nsIWebBrowserPersist);

nit: no wrapping needed

>+      case "font-needed":
>+        this.lookForNeededFonts(aData);
>+        break;
>+
>+      case "available-fonts-update":
>+        this.updateAvailableFonts(aData);
>+        break;

This would be moved to FontLocator

f+ - If you want, we can get a mobile front-end person to carry this patch forward, if we get the feature approved.
Comment 12 Ian Barlow (:ibarlow) 2013-01-16 13:09:40 PST
(In reply to Brad Lassey [:blassey] from comment #10)
> Ian, Jonathan is looking for ux feedback here.

I am assuming the UX in the patch is the same as what is represented in the youtube demo. If so, looks good to me!
Comment 13 Jonathan Kew (:jfkthame) 2013-01-17 03:56:33 PST
(In reply to Mark Finkle (:mfinkle) from comment #11)
> >+      var fontServer = "http://people.mozilla.org/~jkew/bug-619521/";
> >+      var ios = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);
> >+      var channel = ios.newChannel(fontServer + "fonts.json", null, null);
> 
> I wonder if we should ship a JSON file of known fonts on the server? How do
> we update the list of fonts though? I suppose we'll always need a network
> pull anyway.

Right, my assumption is that we shouldn't build a list into the product, as that requires a product update any time we make an additional font available. So the idea was that the browser should ask the server for a list of what's available (but this would only be done once per session, not every time we see a page with missing characters).

If we're in a world where the browser process rarely quits, though, we might need to "expire" the list after a certain length of time (daily?), so that we'll re-ping the server in case of updates.

We should also give some more thought to exactly what metadata we should include in the list of available fonts. In particular, we might want to do something about versioning.

> >+      // TODO: l10n
> >+      let msg = "Download fonts for additional scripts?\n  " + wantedScripts.join("\n  ");
> 
> This could be tricky if we have a lot of scripts or the names of the scripts
> is long or users find it confusing. Maybe we skip the scripts names and just
> let the user know we can download fonts to allow them to reqd the page? (or
> some such wordsmithing)

That's certainly a possibility, though by telling the user which script(s) are involved we give them the option to decide it's pointless to get the fonts ("I can't read Ethiopic or Syriac or Mayan Hieroglyphs anyway, don't waste my bandwidth on this!") because they don't care about them.

This is one of the main issues where I think we need UX/product-team guidance as to the appropriate user experience/interface. We need to find a balance between letting the make an informed choice, vs not overwhelming them with excessive detail. In many cases, the user is likely to only ever see this prompt a few times over the lifetime of the product (since they probably won't visit pages that use -every- possible script, and once downloaded, the fonts will persist in the profile). But there are cases such as visiting the main wikipedia homepage (with the long list of available wikipedia languages) that could trigger a prompt for all the scripts we have available, all at once.

Maybe we should use this kind of simple list if there are just a few, but at a certain threshold (around 5?), switch to a message that says something about "fonts for 16 additional writing systems", with options "ignore/download all/details..."; the "details..." option would go to a separate UI that lists the available fonts and lets the user select/ignore them individually. But maybe that's just too complicated....

> f+ - If you want, we can get a mobile front-end person to carry this patch
> forward, if we get the feature approved.

That'd be awesome - I put this together primarily as a way to test the underlying platform support from bug 619521, and to demonstrate the sort of benefit it could yield for users, but I'm not a js or front-end expert and I'd love to see someone with the proper knowledge take this side of it forward.

We'll also need to work on building an appropriate collection of fonts to make available through this mechanism; I just put a few examples onto my people.mozilla.org site for testing purposes, but we'll want a more complete collection on some permanent moz-hosted server to support this feature properly. I'm happy to help with seeking out appropriate resources, when the time comes.
Comment 14 Henri Sivonen (:hsivonen) 2014-02-27 05:13:11 PST
What happens on Wikipedia when the sidebar has names of languages written in the languages themselves?
Comment 15 Brad Lassey [:blassey] (use needinfo?) 2014-02-27 09:37:19 PST
I think the proposed UI was a door hanger offering the download. I think users would simply dismiss it in that case.
Comment 16 Brad Lassey [:blassey] (use needinfo?) 2014-02-27 09:38:01 PST
It looks like I -'d this a couple weeks ago, but I don't remember doing it, nor the reason. renoming to discuss in triage.
Comment 17 Frédéric Wang (:fredw) 2014-04-26 06:03:42 PDT
I'm not more a front-end guy than Jonathan, but I'll try to help since this is blocking bug 930504.

> + // modify a preference that will trigger reflow everywhere

This sounds like a hack. Did you have plan to find a better way to force reflow?

> - // reset font lists

Why this change?
Comment 18 Frédéric Wang (:fredw) 2014-04-26 06:04:42 PDT
Created attachment 8413180 [details] [diff] [review]
WIP - offer to download additional fonts, if available, for characters lacking font coverage.

Just unbitrotting Jonathan's patch...
Comment 19 Jonathan Kew (:jfkthame) 2014-04-26 06:14:19 PDT
(In reply to Frédéric Wang (:fredw) |away 27/04 to 06/05 from comment #17)
> I'm not more a front-end guy than Jonathan, but I'll try to help since this
> is blocking bug 930504.
> 
> > + // modify a preference that will trigger reflow everywhere
> 
> This sounds like a hack. Did you have plan to find a better way to force
> reflow?

Not really; this is a reasonable thing to do. Note that John recently enshrined it in the new gfxPlatformFontList::ForceGlobalReflow() method, for example:

http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatformFontList.cpp#1056

> 
> > - // reset font lists
> 
> Why this change?

No idea. Maybe at some point along the way, I had real changes in there, and this got left over during editing.... we can just drop it.
Comment 20 Frédéric Wang (:fredw) 2014-04-26 14:00:45 PDT
Created attachment 8413281 [details] [diff] [review]
WIP - offer to download additional fonts, if available, for characters lacking font coverage.

(In reply to Jonathan Kew (:jfkthame) from comment #19)

> Not really; this is a reasonable thing to do. Note that John recently enshrined it in the new gfxPlatformFontList::ForceGlobalReflow() method

OK, I've tried to share the function.

> No idea. Maybe at some point along the way, I had real changes in there, and this got left over during editing.... we can just drop it.

done.

(In reply to Mark Finkle (:mfinkle) from comment #11)

So I've done some code refactoring to address Mark's review comments. That should be the case for all but the "server being a preference" + "using NetUtils to simplify things".

The only functional change is in lookForNeededFont. It seems to me that when this._missingScripts == null, Jonathan's code called addToDeferredScripts twice so the second call one was doing useless work. I've added a return statement instead (but perhaps I don't understand how the JS code works).

Note that I haven't tested the patch at all (not sure how I can test the Android code) so there are probably errors in the JS code.
Comment 21 Frédéric Wang (:fredw) 2014-04-27 07:36:11 PDT
Comment on attachment 8413281 [details] [diff] [review]
WIP - offer to download additional fonts, if available, for characters lacking font coverage.

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

::: mobile/android/chrome/content/FontLocator.js
@@ +18,5 @@
> +
> +  _isLoading: false,
> +  _deferredScriptsNeeded: "",
> +  // FIXME - fontServer doesn't belong here! Make this a preference?
> +  _fontServer: "http://people.mozilla.org/~jkew/bug-619521/";

This semi-colon should really be a comma.

Just an idea for the font server... create a GitHub repository on https://github.com/mozilla to store the fonts and the json file (similar to https://github.com/mozilla-b2g/moztt/). This can be turned into a web server using a gh-pages branch (see https://pages.github.com/). That way people who want to submit specific free fonts for their scripts/languages can just send GitHub pull request and the new fonts can be published quickly.
Comment 22 Jonathan Kew (:jfkthame) 2014-04-27 07:53:04 PDT
(In reply to Frédéric Wang (:fredw) |away 27/04 to 06/05 from comment #20)
> Created attachment 8413281 [details] [diff] [review]
> WIP - offer to download additional fonts, if available, for characters
> lacking font coverage.
> 
> (In reply to Jonathan Kew (:jfkthame) from comment #19)
> 
> > Not really; this is a reasonable thing to do. Note that John recently enshrined it in the new gfxPlatformFontList::ForceGlobalReflow() method
> 
> OK, I've tried to share the function.

I see you moved it to gfxPlatform; that looks fine.

> The only functional change is in lookForNeededFont. It seems to me that when
> this._missingScripts == null, Jonathan's code called addToDeferredScripts
> twice so the second call one was doing useless work. I've added a return
> statement instead (but perhaps I don't understand how the JS code works).

Looks correct to me; I think I simply missed the return in the original patch.

I still wonder about the whole user experience here, though. Should we have some kind of UI (like the doorhanger here) that pops up when we encounter a page that needs additional fonts, and offers to provide them? Or should there be a switch somewhere else in Settings that enables or disables this feature - so that provided it's enabled, it happens silently rather than bothering the user with a prompt?

:blassey, in comment 15 it sounds like you don't think the doorhanger is much use. How else do you think we should handle this? On the one hand, it'd be nice to make it entirely automatic; but on the other hand, we shouldn't go downloading lots of data over what might be an expensive, metered connection without the user's knowledge. How do we help the user know what's happening and make an informed decision?
Comment 23 Jonathan Kew (:jfkthame) 2014-04-27 07:58:51 PDT
(In reply to Frédéric Wang (:fredw) |away 27/04 to 06/05 from comment #21)

> Just an idea for the font server... create a GitHub repository on
> https://github.com/mozilla to store the fonts and the json file (similar to
> https://github.com/mozilla-b2g/moztt/). This can be turned into a web server
> using a gh-pages branch (see https://pages.github.com/). That way people who
> want to submit specific free fonts for their scripts/languages can just send
> GitHub pull request and the new fonts can be published quickly.

That sounds pretty cool - but IMO a service that Firefox is going to rely on should really be more directly under mozilla's control, not dependent on a third party such as github. Unless we have some kind of contract with github that gives specific availability guarantees, addresses any privacy issues involved,[1] etc.... I don't think github's standard terms of service would be appropriate for something that we aim to rely on as an integral part of the Firefox product on Android.


[1] For example, the ability to identify Firefox users who end up downloading fonts to support particular languages could be considered a privacy issue.
Comment 24 Frédéric Wang (:fredw) 2014-04-28 13:14:57 PDT
Created attachment 8413950 [details] [diff] [review]
Part 1 - Force a reflow in nsThebesFontEnumerator::UpdateFontList
Comment 25 Frédéric Wang (:fredw) 2014-04-28 13:18:46 PDT
Created attachment 8413955 [details] [diff] [review]
Part 2 - Offer to download additional fonts, if available, for characters lacking font coverage

I've been able to build Fennec but I'm not able to execute it (or other Nightly builds) on the Android emulator. So I don't think I'll really be able to help on this bug...

At least, I have (tried to) addressed all the comments of the preliminary review...

@Jonathan: could you please try the patches again and check that I haven't broken too many things with this code refactoring?
Comment 26 Jonathan Kew (:jfkthame) 2014-04-29 09:21:39 PDT
Created attachment 8414548 [details] [diff] [review]
Part 2 - Offer to download additional fonts, if available, for characters lacking font coverage. ,

Minor update to :fredw's refactoring (comment 25), which almost-but-not-quite worked for me; just a couple of fixes were needed where NetUtils is now used to fetch the list of available fonts. This appears to work as expected on my Android device when visiting a page with missing characters.
Comment 27 Frédéric Wang (:fredw) 2014-04-29 09:44:29 PDT
(In reply to Jonathan Kew (:jfkthame) from comment #26)
> Created attachment 8414548 [details] [diff] [review]
> Part 2 - Offer to download additional fonts, if available, for characters
> lacking font coverage. ,
> 
> Minor update to :fredw's refactoring (comment 25), which
> almost-but-not-quite worked for me; just a couple of fixes were needed where
> NetUtils is now used to fetch the list of available fonts. This appears to
> work as expected on my Android device when visiting a page with missing
> characters.

Ah thanks! Just two remarks on the NetUtil.asyncFetch part:

- Do you think "available-fonts-update" is really needed or can we call updateAvailableFonts directly? (perhaps it's necessary to ensure sync with the "font-needed" event or something?)

- When the downloading/parsing of the json file fails (for example if the user initially does not have access to internet but reads a page offline), I think this._isLoading will stay to true indefinitely, so new "font-needed" will just continue to be accumulated without being handled...
Comment 28 Jonathan Kew (:jfkthame) 2014-04-29 10:18:21 PDT
(In reply to Frédéric Wang (:fredw) |away 27/04 to 06/05 from comment #27)

> Ah thanks! Just two remarks on the NetUtil.asyncFetch part:
> 
> - Do you think "available-fonts-update" is really needed or can we call
> updateAvailableFonts directly? (perhaps it's necessary to ensure sync with
> the "font-needed" event or something?)
> 
> - When the downloading/parsing of the json file fails (for example if the
> user initially does not have access to internet but reads a page offline), I
> think this._isLoading will stay to true indefinitely, so new "font-needed"
> will just continue to be accumulated without being handled...

Good questions - I'll take a look, and try some experiments...
Comment 29 Jonathan Kew (:jfkthame) 2014-04-29 10:18:41 PDT
Some other issues still to be considered here:

- Do we want to use a doorhanger when missing characters are found, as implemented in this patch, or some other kind of UI to enable/disable font downloads? Should this be controlled by a toggle in Settings rather than a prompt that appears when needed?

- We should probably do something to detect when the server has a newer version of a font that has previously been downloaded, and update. Include a hash of the font file in the the server's list, and compare it to the local file?

- How would we handle updates that involve replacing the font for script X, already downloaded to the user's profile, with a different font (e.g., replace a Lohit font with a Noto one that provides more coverage)?

- Should the FontLocator periodically (daily? weekly?) re-fetch the list of available fonts from the server, so that updates become available even if the user never actually quits and restarts the browser?

- Where's the font service going to be hosted, and who will be responsible for managing it (adding/updating available fonts, etc)?
Comment 30 Jonathan Kew (:jfkthame) 2014-04-30 06:00:09 PDT
Comment on attachment 8413950 [details] [diff] [review]
Part 1 - Force a reflow in nsThebesFontEnumerator::UpdateFontList

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

Oops, switching to r- as there's also a caller of ForceGlobalReflow() in gfxMacPlatformFontList.mm that needs to be updated here.
Comment 31 Frédéric Wang (:fredw) 2014-04-30 10:09:48 PDT
(In reply to Jonathan Kew (:jfkthame) from comment #22)
> :blassey, in comment 15 it sounds like you don't think the doorhanger is
> much use. How else do you think we should handle this? On the one hand, it'd
> be nice to make it entirely automatic; but on the other hand, we shouldn't
> go downloading lots of data over what might be an expensive, metered
> connection without the user's knowledge. How do we help the user know what's
> happening and make an informed decision?

I think the "users would simply dismiss" refers to Henri's question about the behavior on Wikipedia. So it rather sounds to be a support for the doorhanger...

(In reply to Jonathan Kew (:jfkthame) from comment #23)
> That sounds pretty cool - but IMO a service that Firefox is going to rely on
> should really be more directly under mozilla's control, not dependent on a
> third party such as github.

Right. However, I believe putting the fonts on a public repository to which people can easily contribute would still be useful ; even if at the end they are placed on Mozilla's servers.
Comment 32 Brad Lassey [:blassey] (use needinfo?) 2014-04-30 10:46:00 PDT
Yes, I was supporting the door hanger UX
Comment 33 Frédéric Wang (:fredw) 2014-05-06 22:24:10 PDT
Created attachment 8418533 [details] [diff] [review]
Part 1 - Force a reflow in nsThebesFontEnumerator::UpdateFontList
Comment 34 Brad Lassey [:blassey] (use needinfo?) 2014-10-20 08:29:32 PDT
filter on [mass-p5]
Comment 35 Frédéric Wang (:fredw) 2014-10-31 05:47:27 PDT
Created attachment 8514949 [details] [diff] [review]
Part 1 - Force a reflow in nsThebesFontEnumerator::UpdateFontList
Comment 36 Frédéric Wang (:fredw) 2014-10-31 05:48:40 PDT
Created attachment 8514950 [details] [diff] [review]
Part 2 - Offer to download additional fonts, if available, for characters lacking font coverage.
Comment 37 Frédéric Wang (:fredw) 2014-10-31 07:41:26 PDT
Created attachment 8515012 [details] [diff] [review]
Part 2 - Offer to download additional fonts, if available, for characters lacking font coverage.

I created a GitHub account for testing: https://github.com/fred-wang/mozilla-font-server/
Comment 38 Frédéric Wang (:fredw) 2014-11-01 01:52:44 PDT
Three quick questions:

- Could you please check again that the patches are working as expected? I see the doorhanger but the fonts don't seem to install (I can't use adb on my Android device to debug that).

- Do you think it will be worth having a list of fonts rather than a single font per script? It's possible that we don't have one font that covers the whole Unicode range (IIUC, Noto has Greek, Ancient Greek and Pontic for the Unicode script "Grek", cf bug 788450), perhaps we also want to install the style variants (italic, bold...).

- Will the method work for Desktop systems too? (in bug 467729, it is suggested to use PackageKit on Linux. We can probably easily install fonts in a local user font directory on Unix systems but I'm not sure we have permission to access Windows fontdir).
Comment 39 Frédéric Wang (:fredw) 2014-11-11 05:52:48 PST
Created attachment 8520633 [details] [diff] [review]
Part 2 - Offer to download additional fonts, if available, for characters lacking font coverage.
Comment 40 Frédéric Wang (:fredw) 2015-02-18 08:24:14 PST
Created attachment 8566033 [details] [diff] [review]
Part 2 - Offer to download additional fonts, if available, for characters lacking font coverage.

unbitrot...
Comment 41 Frédéric Wang (:fredw) 2015-02-18 08:30:02 PST
(In reply to Frédéric Wang (:fredw) from comment #38)
> - Could you please check again that the patches are working as expected? I
> see the doorhanger but the fonts don't seem to install (I can't use adb on
> my Android device to debug that).

That still seems to be the case (doorhanger appears but the tofu characters remain after "download"). I need to find an Android device on which I can debug...
Comment 42 Frédéric Wang (:fredw) 2015-02-19 13:09:20 PST
Created attachment 8566739 [details] [diff] [review]
Part 2 - Offer to download additional fonts, if available, for characters lacking font coverage.
Comment 43 Frédéric Wang (:fredw) 2015-02-19 13:24:53 PST
So with the recent Android update, it seems that many new fonts are now already installed, which is good news but prevents from testing the patches... Two pages that I could use for testing, though:

https://en.wikipedia.org/wiki/Egyptian_hieroglyphs#Block
https://mdn.mozillademos.org/en-US/docs/Mozilla/MathML_Project/MathML_Torture_Test$samples/MathML_Torture_Test

One of the problem was that a new parameter aReferrerPolicy has been added to nsIWebBrowserPersist::saveURI (bug 704320) ; I fixed that in the latest version of the patch.

Now, the front-end code seems to work as expected until the "refreshing the font list" where the missing characters / MathML operators remain displayed incorrectly. Not sure whether that's related, but I also see the "existing entry" assertions from StartupCache::PutBuffer.

@Jonathan: any idea about recent changes in the gfx code that could have broken the patches?
Comment 44 Frédéric Wang (:fredw) 2015-03-09 14:13:37 PDT
@John: Are you aware of recent changes in the font system that could have broken the patches? (Jonathan suggested bug 962440 maybe).
Comment 45 John Daggett (:jtd) 2015-03-09 22:11:19 PDT
(In reply to Frédéric Wang (:fredw) from comment #44)
> @John: Are you aware of recent changes in the font system that could have
> broken the patches? (Jonathan suggested bug 962440 maybe).

Quite possibly related to changes related to the fontlist and/or unicode-range support. Do you have a simplified testcase and a regression window?
Comment 46 Frédéric Wang (:fredw) 2015-03-10 14:11:26 PDT
Created attachment 8575564 [details]
Testcase (Egyptian_hieroglyphs)
Comment 47 Frédéric Wang (:fredw) 2015-03-10 14:13:02 PDT
Created attachment 8575566 [details]
Addon to install/uninstall a font for Egyptian hieroglyphs
Comment 48 Frédéric Wang (:fredw) 2015-03-10 14:15:37 PDT
Sorry, I can not really test on an Android device right now. But I attached a test page with Egyptian hieroglyphs and an add-on to install/uninstall a fonts for these hieroglyphs.
Comment 49 Bill Gianopoulos [:WG9s] 2015-03-10 15:31:51 PDT
(In reply to John Daggett (:jtd) from comment #45)
> (In reply to Frédéric Wang (:fredw) from comment #44)
> > @John: Are you aware of recent changes in the font system that could have
> > broken the patches? (Jonathan suggested bug 962440 maybe).
> 
> Quite possibly related to changes related to the fontlist and/or
> unicode-range support. Do you have a simplified testcase and a regression
> window?

If someone can tell me when this was last known to work, I will do a bisect and find the regressor.
Comment 50 Bill Gianopoulos [:WG9s] 2015-03-10 15:35:16 PDT
(In reply to Bill Gianopoulos [:WG9s] from comment #49)
> (In reply to John Daggett (:jtd) from comment #45)
> > (In reply to Frédéric Wang (:fredw) from comment #44)
> > > @John: Are you aware of recent changes in the font system that could have
> > > broken the patches? (Jonathan suggested bug 962440 maybe).
> > 
> > Quite possibly related to changes related to the fontlist and/or
> > unicode-range support. Do you have a simplified testcase and a regression
> > window?
> 
> If someone can tell me when this was last known to work, I will do a bisect
> and find the regressor.

And I guess terming it a regressor is a bit harsh.  It really did not regress anything since the patches here were neither reviewed nor ever landed.
Comment 51 Frédéric Wang (:fredw) 2015-03-10 15:41:29 PDT
(In reply to Bill Gianopoulos [:WG9s] from comment #50)
> And I guess terming it a regressor is a bit harsh.  It really did not
> regress anything since the patches here were neither reviewed nor ever
> landed.

Thanks. Only part 1 modifies the gfx code, and only to force a reflow after the font list update. So I believe one can try to find a regression windows using the add-on & testcase, without any of the patches applied.
Comment 52 Bill Gianopoulos [:WG9s] 2015-03-10 15:44:59 PDT
But I would still like a date when this was known to work so I have a starting place.
Comment 53 Frédéric Wang (:fredw) 2015-03-10 15:54:49 PDT
It should have been working at the date of comment 26.
Comment 54 Frédéric Wang (:fredw) 2015-03-10 16:07:19 PDT
Created attachment 8575642 [details]
Testcase (Egyptian_hieroglyphs)

Trying to use entity numbers to workaround encoding issues...
Comment 55 John Daggett (:jtd) 2015-03-11 22:15:20 PDT
(In reply to Frédéric Wang (:fredw) from comment #51)
> (In reply to Bill Gianopoulos [:WG9s] from comment #50)
> > And I guess terming it a regressor is a bit harsh.  It really did not
> > regress anything since the patches here were neither reviewed nor ever
> > landed.
> 
> Thanks. Only part 1 modifies the gfx code, and only to force a reflow after
> the font list update. So I believe one can try to find a regression windows
> using the add-on & testcase, without any of the patches applied.

I think it would be simpler just to figure out why these patches aren't working. Trunk code changes all the time and until you land something, you just need to keep updating it to trunk changes.

Are you not seeing calls to UpdateFontList such that the new fonts aren't picked up? With NSPR_LOG_MODULES=fontlist:5 what do you see the new font dumped out?
Comment 56 Bill Gianopoulos [:WG9s] 2015-03-14 08:42:59 PDT
(In reply to Frédéric Wang (:fredw) from comment #47)
> Created attachment 8575566 [details]
> Addon to install/uninstall a font for Egyptian hieroglyphs

Well the addon works with current nightly build, so I suspect the issue is with the UI patch and NOT with the gfx one.
Comment 57 Frédéric Wang (:fredw) 2015-03-14 09:17:09 PDT
(In reply to Bill Gianopoulos [:WG9s] from comment #56)
> Well the addon works with current nightly build, so I suspect the issue is
> with the UI patch and NOT with the gfx one.

Mmmh, what do you mean when you say "nightly build" (i.e. does that include the gfx patch?).
Comment 58 Bill Gianopoulos [:WG9s] 2015-03-14 11:26:01 PDT
Created attachment 8577635 [details] [diff] [review]
Part 2 - Offer to download additional fonts, if available, for characters lacking font coverage.

Fix fourth parameter for nsIWebBrowserPersist::saveURI to be an integer as specified.
Comment 59 Bill Gianopoulos [:WG9s] 2015-03-14 11:32:13 PDT
(In reply to Frédéric Wang (:fredw) from comment #57)
> (In reply to Bill Gianopoulos [:WG9s] from comment #56)
> > Well the addon works with current nightly build, so I suspect the issue is
> > with the UI patch and NOT with the gfx one.
> 
> Mmmh, what do you mean when you say "nightly build" (i.e. does that include
> the gfx patch?).

OK. I was on IRC with Fred but to get this all into the patch.  The add-on in the attachment works with a vanilla nightly build and also in a nightly build with both pending patches here as well as with either of them applied independently of each other.
Comment 60 Frédéric Wang (:fredw) 2015-04-09 11:47:25 PDT
(In reply to John Daggett (:jtd) from comment #55)
> Are you not seeing calls to UpdateFontList such that the new fonts aren't
> picked up?

So I tried again and indeed see the messages

dump("download " + fontURI.spec + " to " + localFile.path);
dump("refreshing the font list");

as well as the "existing entry" assertions from gfx mentioned in previous comments. Also, when I stop fennec and retry a second time (without clearing the cache) I still see the doorhanger but no messages, which suggests that downloadFont() exists at the first return because the font file now exists.

> With NSPR_LOG_MODULES=fontlist:5 what do you see the new font
> dumped out?

This didn't seem to provide more debug information...

(In reply to Bill Gianopoulos [:WG9s] from comment #59)
> The add-on
> in the attachment works with a vanilla nightly build and also in a nightly
> build with both pending patches here as well as with either of them applied
> independently of each other.

Same here. For some reason the font are correctly installed via the add-on but not via the UI patch. And the only difference I see is that the latter download the font from a remote server instead of copying it from the add-on installation directory.
Comment 61 Bill Gianopoulos [:WG9s] 2015-04-09 12:27:21 PDT
(In reply to Frédéric Wang (:fredw) from comment #60)

> Same here. For some reason the font are correctly installed via the add-on
> but not via the UI patch. And the only difference I see is that the latter
> download the font from a remote server instead of copying it from the add-on
> installation directory.

Perhaps the files are being saved with file permissions that don't allow the browser to read them?

Note You need to log in before you can comment on or make changes to this bug.