Closed Bug 1119062 (enable-unicode-range-release) Opened 9 years ago Closed 9 years ago

enable unicode-range in release builds

Categories

(Core :: Graphics: Text, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: jtd, Assigned: jtd)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 1 obsolete file)

The unicode-range implementation is currently only enabled for nightly/aurora builds, waiting for the Linux platform fontlist implementation (bug 1056479). Once that lands and appears to be stable we can enable unicode-range in release builds.
Depends on: 1180560
Alias: enable-unicode-range-release
Depends on: unicode-range
What's the plan for releasing this fix?
Flags: needinfo?(jdaggett)
Currently gated by bugs blocking the fontconfig platform fontlist use under Linux. See bug 1180560. That bug gets fixed ==> this bug gets resolved, unicode-range is enabled by default for release builds.
Flags: needinfo?(jdaggett)
Depends on: 1218123
Once bug 1180560 lands we can enable unicode-range in release builds.
Attachment #8545607 - Attachment is obsolete: true
Attachment #8689290 - Flags: review?(dbaron)
Did you send an intent to ship notice to dev-platform?  See https://wiki.mozilla.org/WebAPI/ExposureGuidelines
Flags: needinfo?(jdaggett)
Comment on attachment 8689290 [details] [diff] [review]
patch, enable unicode-range in release builds

r=dbaron


One other thought, though (since you made me look at what the test coverage is):  given that the reason for unicode-range to exist is that it allows a way of not bothering to download a font at all based on character ranges, it seems like it's probably worth adding a mochitest to actually test the "lack of download" aspect of it (with a base case that should be downloaded).  It shouldn't be too hard to do in the manner of test_bug517224.html and bug517224.sjs in layout/style/test/ (although you probably want multiple tests, like the somewhat more complicated visited_image_loading test in the same directory).
Attachment #8689290 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #7)
> Comment on attachment 8689290 [details] [diff] [review]
> patch, enable unicode-range in release builds
> 
> r=dbaron
> 
> 
> One other thought, though (since you made me look at what the test coverage
> is):  given that the reason for unicode-range to exist is that it allows a
> way of not bothering to download a font at all based on character ranges, it
> seems like it's probably worth adding a mochitest to actually test the "lack
> of download" aspect of it (with a base case that should be downloaded).  It
> shouldn't be too hard to do in the manner of test_bug517224.html and
> bug517224.sjs in layout/style/test/ (although you probably want multiple
> tests, like the somewhat more complicated visited_image_loading test in the
> same directory).

Hmmm, that's basically what the slew of tests in layout/style/test/test_unicode_range_loading.html are doing? Did you look through those? Would you consider those not sufficient? These tests use the font loading API rather than .sjs magic. But if needed I'm sure I could come up with the kind of test you're thinking about too.
Forgot to remove the "skip on Linux" of the unicode-range load test.
Attachment #8689912 - Flags: review?(dbaron)
https://hg.mozilla.org/mozilla-central/rev/ab494cc7de0d
https://hg.mozilla.org/mozilla-central/rev/55df0ae1a9f5
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment on attachment 8689290 [details] [diff] [review]
patch, enable unicode-range in release builds

Approval Request Comment
[Feature/regressing bug #]: With the new work on the Linux font backend, we can now enable unicode-range for beta/release across all platforms.
[User impact if declined]: unicode-range support can't be enabled until FF45
[Describe test coverage new/current, TreeHerder]: landed on trunk last week, no issues reported
[Risks and why]: new fontconfig font backend has been enabled for nightly/aurora for several release cycles now and all known regressions have been resolved
[String/UUID change made/needed]: none
Attachment #8689290 - Flags: approval-mozilla-aurora?
Comment on attachment 8689912 [details] [diff] [review]
patch, enable unicode-range load test under Linux

Approval Request Comment

-- same as above --
Attachment #8689912 - Flags: approval-mozilla-aurora?
John, while reviewing the patch I noticed that the pref "layout.css.unicode-range.enabled" is being removed completely. Is that intentional? I got confused because the bug talks about enabling unicode-range. Does it mean it is on and not something that can be turned off by a pref setting? Please let me know.
Flags: needinfo?(jdaggett)
(In reply to Ritu Kothari (:ritu) from comment #14)
> John, while reviewing the patch I noticed that the pref
> "layout.css.unicode-range.enabled" is being removed completely. Is that
> intentional? I got confused because the bug talks about enabling
> unicode-range. Does it mean it is on and not something that can be turned
> off by a pref setting? Please let me know.

Right, "enabling" in this case means making it always on by default and removing the pref completely.
Flags: needinfo?(jdaggett)
Comment on attachment 8689290 [details] [diff] [review]
patch, enable unicode-range in release builds

This has been on Nightly for 10 days or so. Let's uplift to Aurora44.
Attachment #8689290 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8689912 [details] [diff] [review]
patch, enable unicode-range load test under Linux

Aurora44+
Attachment #8689912 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Why MSN display Firefox 36 support?
(In reply to percyley from comment #19)
> Why MSN display Firefox 36 support?

It was supported initially behind a flag, then only in nightly/developer builds. This bug enables it by default for beta/release builds.

I'll work with the dev-docs folks to clarify this on MDN.
(In reply to John Daggett (:jtd) from comment #8)
> (In reply to David Baron [:dbaron] ⌚UTC-8 from comment #7)
> > One other thought, though (since you made me look at what the test coverage
> > is):  given that the reason for unicode-range to exist is that it allows a
> > way of not bothering to download a font at all based on character ranges, it
> > seems like it's probably worth adding a mochitest to actually test the "lack
> > of download" aspect of it (with a base case that should be downloaded).  It
> > shouldn't be too hard to do in the manner of test_bug517224.html and
> > bug517224.sjs in layout/style/test/ (although you probably want multiple
> > tests, like the somewhat more complicated visited_image_loading test in the
> > same directory).
> 
> Hmmm, that's basically what the slew of tests in
> layout/style/test/test_unicode_range_loading.html are doing? Did you look
> through those? Would you consider those not sufficient? These tests use the
> font loading API rather than .sjs magic. But if needed I'm sure I could come
> up with the kind of test you're thinking about too.

I guess I think it would still be a good idea to write such a test.
Flags: needinfo?(jdaggett)
(In reply to John Daggett (:jtd) from comment #21)
> (In reply to percyley from comment #19)
> > Why MSN display Firefox 36 support?
> 
> It was supported initially behind a flag, then only in nightly/developer
> builds. This bug enables it by default for beta/release builds.
> 
> I'll work with the dev-docs folks to clarify this on MDN.

Thanks!
In preparation for enabling unicode-range for Firefox in Google Fonts I tried out unicode-range on Firefox dev edition 44.0a2 (2015-12-02) and ran into unexpected behavior around src descriptors using local(). 

I have a test that is intended to download two font files (urls ending in ?cyrillic and ?latin-ext). 

If there are no local()'s in src descriptor Firefox downloads three font files: the ones for cyrillic and latin-ext plus one for latin. http://output.jsbin.com/vuragugoza/quiet shows this.

If there are local()'s in the src descriptor Firefox downloads the same three font files and then shortly after that downloads the same three font files again. The Dev Tools Network tab seems to show it downloads the three files, waits a bit, then downloads them again. http://output.jsbin.com/gihumugeya/quiet shows this.

If I make an example where only some src descriptors have local() then only those show as double-downloading.

I had Dev Tools open and Disable Cache (when toolbox is open) enabled when testing.
(In reply to Rod from comment #25)
> In preparation for enabling unicode-range for Firefox in Google Fonts I
> tried out unicode-range on Firefox dev edition 44.0a2 (2015-12-02) and ran
> into unexpected behavior around src descriptors using local(). 
> 
> I have a test that is intended to download two font files (urls ending in
> ?cyrillic and ?latin-ext). 

Looking at your example  http://output.jsbin.com/gihumugeya  I think it's expected that the ?latin file will also be downloaded, because neither ?cyrillic nor ?latin-ext lists the U+0020 (space) codepoint in its unicode-range.

> 
> If there are no local()'s in src descriptor Firefox downloads three font
> files: the ones for cyrillic and latin-ext plus one for latin.
> http://output.jsbin.com/vuragugoza/quiet shows this.
> 
> If there are local()'s in the src descriptor Firefox downloads the same
> three font files and then shortly after that downloads the same three font
> files again. The Dev Tools Network tab seems to show it downloads the three
> files, waits a bit, then downloads them again.
> http://output.jsbin.com/gihumugeya/quiet shows this.

I can reproduce this with Developer Edition (44.0a2), but don't see it happening in a Nightly build (45.0a1).

jdaggett, does this sound familiar? A known-and-already-fixed issue?
(In reply to percyley from comment #23)

> > It was supported initially behind a flag, then only in nightly/developer
> > builds. This bug enables it by default for beta/release builds.
> > 
> > I'll work with the dev-docs folks to clarify this on MDN.
> 
> Thanks!

Just updated the unicode-range MDN page to hopefully document current behavior more clearly.
Flags: needinfo?(jdaggett)
(In reply to Jonathan Kew (:jfkthame) from comment #26)

> > If there are local()'s in the src descriptor Firefox downloads the same
> > three font files and then shortly after that downloads the same three font
> > files again. The Dev Tools Network tab seems to show it downloads the three
> > files, waits a bit, then downloads them again.
> > http://output.jsbin.com/gihumugeya/quiet shows this.
> 
> I can reproduce this with Developer Edition (44.0a2), but don't see it
> happening in a Nightly build (45.0a1).
> 
> jdaggett, does this sound familiar? A known-and-already-fixed issue?

Let me take a look today. If it looks like there's a problem, I'll file a new bug. The repeated downloads may be something related to bug 1188802.
See Also: → 1188802
Not Firefox 45?
(In reply to percyley from comment #30)
> Not Firefox 45?

See comment 18, patches have been uplifted to aurora which is what will become Firefox 44.
Depends on: 1230437
Seems that after release 44 some font families are not matched correctly in Linux platforms.

For example "Liberation Mono" seems to be ignored even if is installed correctly in the system:

$ fc-match 'Liberation Mono'
LiberationMono-Regular.ttf: "Liberation Mono" "Regular"

This didn't happen in previous releases. Is it related to unicode-range?
(In reply to michelesr from comment #32)
> Seems that after release 44 some font families are not matched correctly in
> Linux platforms.
> 
> For example "Liberation Mono" seems to be ignored even if is installed
> correctly in the system:
> 
> $ fc-match 'Liberation Mono'
> LiberationMono-Regular.ttf: "Liberation Mono" "Regular"
> 
> This didn't happen in previous releases. Is it related to unicode-range?

Hmmm, I'm not seeing this with Liberation Mono installed under Ubuntu. What do you see for URL below?

data:text/html,<body style="font-family: Liberation Mono,serif">burp

Serif or monospace font?

If you don't see monospace could you log a new bug?
> Hmmm, I'm not seeing this with Liberation Mono installed under Ubuntu. What do you see for URL below?

DejaVu Serif system

Used as: "DejaVu Serif"
> Hmmm, I'm not seeing this with Liberation Mono installed under Ubuntu. What do you see for URL below?

Arch Linux here, Linux x86_64

fontconfig version: 2.11.1-2
fontconfig configuration: system provided/untouched 

I noticed "Liberation Mono" stopping working after upgrade from 43 to 44
(In reply to michelesr from comment #34)
> > Hmmm, I'm not seeing this with Liberation Mono installed under Ubuntu. What do you see for URL below?
> 
> DejaVu Serif system
> 
> Used as: "DejaVu Serif"

Logged as bug 1245811
Depends on: 1245811
Depends on: 1253542
Depends on: 1286011
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: