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

Add basic support for web fonts. Synchronous loading only #2791

Merged
merged 6 commits into from Aug 5, 2014

Conversation

@glennw
Copy link
Member

glennw commented Jul 9, 2014

for now, and only deals with TTF format fonts.

For an example, try loading http://icons.marekventur.de

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Jul 9, 2014

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

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.

@pcwalton
Copy link
Contributor

pcwalton commented Jul 9, 2014

Whoa, sweet.

@pcwalton
Copy link
Contributor

pcwalton commented Jul 9, 2014

This should eventually get a reftest. I recommend using the Ahem font, as it was explicitly designed for this: http://www.w3.org/Style/CSS/Test/Fonts/Ahem/

@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 9, 2014

We need to start running the test for missing license headers on travis

@SimonSapin
Copy link
Member

SimonSapin commented Jul 9, 2014

I’m reviewing the parsing code.

@SimonSapin
Copy link
Member

SimonSapin commented Jul 10, 2014

Partial review done on Critic.

@glennw
Copy link
Member Author

glennw commented Jul 11, 2014

Thanks for reviewing! I've pushed fixes for those and updated the critic review.

@glennw
Copy link
Member Author

glennw commented Jul 14, 2014

**** NOTE: Even when this review is completed, it can't be merged until servo/core-graphics-rs#24 has landed and had submodule ptrs updated in servo ****

** The PR above has now landed in master, and travis is now passing - so this can be merged once the review is completed. **

@glennw
Copy link
Member Author

glennw commented Jul 20, 2014

gw3583 added 5 commits Jul 8, 2014
for now, and only deals with TTF format fonts.

For an example, try loading http://icons.marekventur.de
separated format hints. Fix oversight in mac code dealing with
creating web fonts from memory.
@glennw
Copy link
Member Author

glennw commented Jul 24, 2014

Rebased and updated to support new url parser.

@SimonSapin

This comment has been minimized.

Copy link

SimonSapin commented on src/components/style/font_face.rs in a500099 Jul 25, 2014

Fix the FIXME by replacing the .unwrap() by .unwarp_or_else(|| Url::parse("about:invalid")).

This comment has been minimized.

Copy link
Owner

glennw replied Jul 28, 2014

Fixed

@glennw
Copy link
Member Author

glennw commented Aug 3, 2014

Would it be better if I split this into a few smaller PRs that are easier to review (e.g. CSS parsing separate from the platform specific font support etc)?

@SimonSapin
Copy link
Member

SimonSapin commented Aug 4, 2014

I don’t think it would really help. We’d still need to be better than this at assigning reviewers :]

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Aug 5, 2014

Is this set to merge? All issues appear addressed.

@glennw
Copy link
Member Author

glennw commented Aug 5, 2014

I kicked off a travis build this morning to make sure it is still green.

@larsbergstrom

This comment has been minimized.

Copy link

larsbergstrom commented on a37b5cb Aug 5, 2014

r+

glennw added a commit that referenced this pull request Aug 5, 2014
Add basic support for web fonts. Synchronous loading only
@glennw glennw merged commit 541f22a into servo:master Aug 5, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@glennw glennw deleted the glennw:web-fonts branch Aug 5, 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

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