Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve quality of font rendering on Linux (and Android). #3256

Merged
merged 2 commits into from Sep 10, 2014

Conversation

@glennw
Copy link
Member

glennw commented Sep 9, 2014

The freetype hinting only works on integer pixel sizes. For this
reason, the advance width metrics for a font of size 12.99 are
the same as the advance metrics for a font of size 12.0. This
results in small fonts appearing to overlap slightly, which is
particularly noticeable on parts of Wikipedia. Round the font
size up to a pixel boundary inside the freetype system.

Also fetch the system default fonts for the generic font families
rather than hard coding them.

These two changes make the font rendering on Linux very close
to the Firefox font rendering on Wikipedia, related to metabug #2554.

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Sep 9, 2014

Critic review: https://critic.hoppipolla.co.uk/r/2538

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@highfive
Copy link

highfive commented Sep 9, 2014

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
let family_match = FcFontMatch(ptr::mut_null(), pattern, &mut result);

let family_name = if result == FcResultMatch {
let mut FC_FAMILY_C = "family".to_c_str();

This comment has been minimized.

Copy link
@pcwalton

pcwalton Sep 9, 2014

Contributor

(Aside note: Is there no way to declare this as a real Rust constant? How does the JS engine do it?)

This comment has been minimized.

Copy link
@metajack

metajack Sep 9, 2014

Contributor

Note that we have these everywhere in the current code. Perhaps a E-Easy followup bug to fix all of them if a solution is available?

This comment has been minimized.

Copy link
@jdm

jdm Sep 9, 2014

Member

static FC_FAMILY: [u8, ..7] = ['f' as u8, 'a' as u8, 'm' as u8, 'i' as u8, 'l' as u8, 'y' as u8, 0 as u8];
some_fn_taking_char_ptr(&FC_FAMILY as *const u8 as *const libc::c_char);

This comment has been minimized.

Copy link
@huonw

huonw Sep 9, 2014

Contributor
static FC_FAMILY: &'static [u8] = b"family\0";
FC_FAMILY.as_ptr() as *const libc::c_char

This comment has been minimized.

Copy link
@glennw

glennw Sep 9, 2014

Author Member

Added a new commit with that syntax + fixed other occurrences in that file.

@pcwalton
Copy link
Contributor

pcwalton commented Sep 9, 2014

r=me with that nit addressed if there's a better way to do it.

gw3583 added 2 commits Sep 9, 2014
The freetype hinting only works on integer pixel sizes. For this
reason, the advance width metrics for a font of size 12.99 are
the same as the advance metrics for a font of size 12.0. This
results in small fonts appearing to overlap slightly, which is
particularly noticeable on parts of Wikipedia. Round the font
size up to a pixel boundary inside the freetype system.

Also fetch the system default fonts for the generic font families
rather than hard coding them.

These two changes make the font rendering on Linux very close
to the Firefox font rendering on Wikipedia.
@glennw glennw force-pushed the glennw:font-fixes branch from 7d17085 to d218815 Sep 9, 2014
metajack added a commit that referenced this pull request Sep 10, 2014
Improve quality of font rendering on Linux (and Android).
@metajack metajack merged commit 6eeac02 into servo:master Sep 10, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@glennw glennw deleted the glennw:font-fixes branch Sep 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.