Brad Lassey [:blassey] (use needinfo?) Reporter
	Description • 13 years ago

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

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 1 • 13 years ago

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

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 2 • 13 years ago

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.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 3 • 13 years ago

(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.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 4 • 13 years ago

We can reset tracking flag and assign this bug when bug 619521 has landed

	Jonathan Kew [:jfkthame]
	Comment 5 • 12 years ago

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.)

	Jonathan Kew [:jfkthame]
	Comment 6 • 12 years ago

I've posted a short video showing this feature in action at http://www.youtube.com/watch?v=RDTMfCajyCU.

	Jonathan Kew [:jfkthame]
	Comment 7 • 12 years ago

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?

	Jonathan Kew [:jfkthame]
	Comment 8 • 12 years ago

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.

	Jonathan Kew [:jfkthame]
	Comment 9 • 11 years ago

Updated proof-of-concept patch; UX/front-end feedback would still be appreciated on this.

	Brad Lassey [:blassey] (use needinfo?) Reporter
	Comment 10 • 11 years ago

Ian, Jonathan is looking for ux feedback here.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 11 • 11 years ago

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.

	Ian Barlow (:ibarlow)
	Comment 12 • 11 years ago

(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!

	Jonathan Kew [:jfkthame]
	Comment 13 • 11 years ago

(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.

	Henri Sivonen (:hsivonen)
	Comment 14 • 10 years ago

What happens on Wikipedia when the sidebar has names of languages written in the languages themselves?

	Brad Lassey [:blassey] (use needinfo?) Reporter
	Comment 15 • 10 years ago

I think the proposed UI was a door hanger offering the download. I think users would simply dismiss it in that case.

	Brad Lassey [:blassey] (use needinfo?) Reporter
	Comment 16 • 10 years ago

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.

	Frédéric Wang (:fredw)
	Comment 17 • 10 years ago

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?

	Frédéric Wang (:fredw)
	Comment 18 • 10 years ago

Just unbitrotting Jonathan's patch...

	Jonathan Kew [:jfkthame]
	Comment 19 • 10 years ago

(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.

	Frédéric Wang (:fredw)
	Comment 20 • 10 years ago

(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.

	Frédéric Wang (:fredw)
	Comment 21 • 10 years ago

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.

	Jonathan Kew [:jfkthame]
	Comment 22 • 10 years ago

(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?

	Jonathan Kew [:jfkthame]
	Comment 23 • 10 years ago

(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.

	Frédéric Wang (:fredw)
	Comment 25 • 10 years ago

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?

	Jonathan Kew [:jfkthame]
	Comment 26 • 10 years ago

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.

	Frédéric Wang (:fredw)
	Comment 27 • 10 years ago

(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...

	Jonathan Kew [:jfkthame]
	Comment 28 • 10 years ago

(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...

	Jonathan Kew [:jfkthame]
	Comment 29 • 10 years ago

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)?

	Jonathan Kew [:jfkthame]
	Comment 30 • 10 years ago

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.

	Frédéric Wang (:fredw)
	Comment 31 • 10 years ago

(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.

	Brad Lassey [:blassey] (use needinfo?) Reporter
	Comment 32 • 10 years ago

Yes, I was supporting the door hanger UX

	Brad Lassey [:blassey] (use needinfo?) Reporter
	Comment 34 • 10 years ago

filter on [mass-p5]

	Frédéric Wang (:fredw)
	Comment 37 • 10 years ago

I created a GitHub account for testing: https://github.com/fred-wang/mozilla-font-server/

	Frédéric Wang (:fredw)
	Comment 38 • 10 years ago

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).

	Frédéric Wang (:fredw)
	Comment 40 • 9 years ago

unbitrot...

	Frédéric Wang (:fredw)
	Comment 41 • 9 years ago

(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...

	Frédéric Wang (:fredw)
	Comment 43 • 9 years ago

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?

	Frédéric Wang (:fredw)
	Comment 44 • 9 years ago

@John: Are you aware of recent changes in the font system that could have broken the patches? (Jonathan suggested bug 962440 maybe).

	John Daggett (:jtd)
	Comment 45 • 9 years ago

(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?

	Frédéric Wang (:fredw)
	Comment 48 • 9 years ago

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.

	Bill Gianopoulos [:WG9s]
	Comment 49 • 9 years ago

(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.

	Bill Gianopoulos [:WG9s]
	Comment 50 • 9 years ago

(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.

	Frédéric Wang (:fredw)
	Comment 51 • 9 years ago

(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.

	Bill Gianopoulos [:WG9s]
	Comment 52 • 9 years ago

But I would still like a date when this was known to work so I have a starting place.

	Frédéric Wang (:fredw)
	Comment 53 • 9 years ago

It should have been working at the date of comment 26.

	Frédéric Wang (:fredw)
	Comment 54 • 9 years ago

Trying to use entity numbers to workaround encoding issues...

	John Daggett (:jtd)
	Comment 55 • 9 years ago

(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?

	Bill Gianopoulos [:WG9s]
	Comment 56 • 9 years ago

(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.

	Frédéric Wang (:fredw)
	Comment 57 • 9 years ago

(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?).

	Bill Gianopoulos [:WG9s]
	Comment 58 • 9 years ago

Fix fourth parameter for nsIWebBrowserPersist::saveURI to be an integer as specified.

	Bill Gianopoulos [:WG9s]
	Comment 59 • 9 years ago

(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.

	Frédéric Wang (:fredw)
	Comment 60 • 9 years ago

(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.

	Bill Gianopoulos [:WG9s]
	Comment 61 • 9 years ago

(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?

	BMO Automation
	Comment 62 • 3 years ago

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.