Closed
Bug 700023
Opened 13 years ago
Closed 12 years ago
enable Graphite font shaping by default
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: jfkthame, Assigned: jfkthame)
References
Details
Attachments
(2 files, 1 obsolete file)
1.11 KB,
patch
|
jtd
:
review+
|
Details | Diff | Splinter Review |
1.69 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
In bug 631479, graphite support will be landed preffed-off; once we have sufficient confidence in the implementation, and test coverage, we should flip the pref to enable it by default so that sites depending on Graphite-enabled fonts will work out-of-the-box.
Assignee | ||
Comment 1•12 years ago
|
||
We've had the Graphite text-shaping library in Firefox (preffed-off by default) for a year now, since it landed for FF11 in bug 631479. It's had considerable fuzz-testing, both as part of the Firefox build (see bug 631479 comment 147, and bug 681976 dependencies) and upstream within the SIL graphite project (see the /tests/ directory in the graphite repository). Issues found have been promptly addressed by the team, and by this time the code appears to be both stable and robust. While Graphite is always likely to be a niche technology, primarily of interest to those with specialized script needs that are not covered by OpenType, there are projects that are actively using it and relying on Gecko's graphite support - e.g. the sign-language example in bug 836225, or the Javanese font at https://sites.google.com/site/jawaunicode/main-page. So I think it's time to flip the setting of the graphite pref, so that this functionality is available by default - in particular, so that users visiting a site that deploys a Graphite-enabled font will see the correct rendering without having to manually change a setting in about:config first.
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #717191 -
Flags: review?(jdaggett)
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → jfkthame
Assignee | ||
Comment 3•12 years ago
|
||
Tryserver run in progress: https://tbpl.mozilla.org/?tree=Try&rev=cceeb004fe1d (includes the graphite reftests from bug 700022, which are currently on mozilla-inbound).
Updated•12 years ago
|
Attachment #717191 -
Flags: review?(jdaggett) → review+
Comment 4•12 years ago
|
||
Might be a good idea to wait a couple days to see what's going on with B2G R6, just in case there's something there related to Graphite (e.g. memory stomping). That's failing in your tryserver build. Seems like it should be unrelated (WebGL error).
Assignee | ||
Comment 5•12 years ago
|
||
That was bug 844439 (for details, see the sad story of b2g-R6 on inbound over the weekend).
Assignee | ||
Comment 6•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdc3588c8ce2
Target Milestone: --- → mozilla22
Comment 7•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cdc3588c8ce2
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 8•12 years ago
|
||
Given the 22MB regression on Android (bug 846832) I think we should back this out for now until we can diagnose that. There's no pressing need for Graphite to be on by default.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 9•12 years ago
|
||
(In reply to John Daggett (:jtd) from comment #8) > Given the 22MB regression on Android (bug 846832) I think we should back > this out for now until we can diagnose that. There's no pressing need for > Graphite to be on by default. I agree that this is a serious regression, though we should let :jfkthame have a look first. I'm also curious why this is a fennec-only regression. Perhaps querying for Graphite tables in Android fonts keeps the font in RAM. That's just a guess though, and Jonathan can have a better look (and prepare a backout patch if needed.)
Assignee | ||
Comment 10•12 years ago
|
||
The reason this impacts fennec (only) is that the Charis fonts we're now bundling as the default serif font for FF on Android include graphite layout tables in addition to OpenType (and AAT, actually). The graphite tables are around 300K (per face) in the font files, but it looks like the RAM footprint for actually using the graphite shaper must be substantially higher than for harfbuzz/opentype. So it's not surprising there'd be an impact here, but I am surprised at how large it looks. So I think for now, at least, we should flip the pref back to false on Android, pending further investigation.
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #720579 -
Flags: review?(jdaggett)
Comment 12•12 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #11) > Created attachment 720579 [details] [diff] [review] > followup - flip the graphite pref back to false on android, due to memory > usage regression (see bug 846832) Juts to be clear, you don't believe this affects B2G? We're doing very little testing on m-c these days, so if it did, it's unlikely anyone would notice for a while.
Assignee | ||
Comment 13•12 years ago
|
||
It only affects android because that's the only place (AFAIK) we have a set of default font faces that include graphite tables. Though if B2G decides to use the same fonts as we're shipping for android, it would become an issue there as well. I notice there are a couple of open bugs about adding/changing default fonts on B2G, so this is something to watch. Also note that bug 847344 (about optimizing how we use the API) should address the memory impact here.
Comment 14•12 years ago
|
||
Comment on attachment 720579 [details] [diff] [review] followup - flip the graphite pref back to false on android, due to memory usage regression (see bug 846832) Review of attachment 720579 [details] [diff] [review]: ----------------------------------------------------------------- ::: modules/libpref/src/init/all.js @@ +238,2 @@ > pref("gfx.font_rendering.graphite.enabled", true); > +#endif Note you can override this pref for android by putting it in mobile/android/app/mobile.js instead. i.e. leave it "true" in all.js and then put the same pref with value "false" in mobile.js. I think that would be preferable to ifdef'ing.
Assignee | ||
Comment 15•12 years ago
|
||
OK, here's an updated patch that disables it in mobile.js.
Attachment #720757 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Updated•12 years ago
|
Attachment #720579 -
Attachment is obsolete: true
Attachment #720579 -
Flags: review?(jdaggett)
Updated•12 years ago
|
Attachment #720757 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 16•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a0bcaa622f0
Comment 17•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6a0bcaa622f0
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•