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.

Sign up for GitHub

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

Jump to bottom

layout: Implement per-glyph font fallback. #5607

Merged
merged 1 commit into from
May 23, 2015
Merged

layout: Implement per-glyph font fallback. #5607

merged 1 commit into from
May 23, 2015

Conversation

pcwalton
Copy link
Contributor

@pcwalton pcwalton commented Apr 8, 2015

This improves numerous pages, for example Wikipedia and Ars Technica.

Built on #5493.

Closes #177.

Review on Reviewable

@hoppipolla-critic-bot
Copy link

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

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 Author

pcwalton commented Apr 8, 2015

r? @mbrubeck

@jdm jdm added the S-awaiting-review There is new code that needs to be reviewed. label Apr 8, 2015
@mbrubeck mbrubeck self-assigned this Apr 8, 2015
@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #5493) made this pull request unmergeable. Please resolve the merge conflicts.

@mbrubeck mbrubeck added the S-needs-rebase There are merge conflict errors. label Apr 8, 2015
@mbrubeck
Copy link
Contributor

mbrubeck commented Apr 9, 2015

Left a few comments on Critic.

@mbrubeck mbrubeck added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 9, 2015
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Apr 9, 2015
@pcwalton
Copy link
Contributor Author

pcwalton commented Apr 9, 2015

Addressed the comments. r? @mbrubeck

@mbrubeck
Copy link
Contributor

mbrubeck commented Apr 9, 2015

r+, needs squash and rebase

@mbrubeck mbrubeck added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 9, 2015
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 10, 2015
@pcwalton
Copy link
Contributor Author

@bors-servo: r=mbrubeck

@bors-servo
Copy link
Contributor

📌 Commit aafcf7a has been approved by mbrubeck

@bors-servo
Copy link
Contributor

⌛ Testing commit aafcf7a with merge 05edd87...

bors-servo pushed a commit that referenced this pull request Apr 10, 2015
Auto merge of #5607 - pcwalton:per-glyph-font-fallback, r=mbrubeck
05edd87 
This improves numerous pages, for example Wikipedia and Ars Technica.

Built on #5493.

Closes #177.
@jdm jdm added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. S-needs-squash Some (or all) of the commits in the PR should be combined. labels Apr 10, 2015
@bors-servo
Copy link
Contributor

💔 Test failed - linux1

@jdm
Copy link
Member

jdm commented Apr 10, 2015 

failures:
    line_height_float_placement_a.html == line_height_float_placement_ref.html
    text_align_complex_a.html == text_align_complex_ref.html

@jdm jdm added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Apr 10, 2015
@pcwalton
Copy link
Contributor Author

Ugh, Linux-only failures. :(

@pcwalton
Copy link
Contributor Author

@bors-servo: r=glennw

@bors-servo
Copy link
Contributor

📌 Commit 1b5a13d has been approved by glennw

@bors-servo
Copy link
Contributor

⌛ Testing commit 1b5a13d with merge d6943f2...

bors-servo pushed a commit that referenced this pull request Apr 22, 2015
Auto merge of #5607 - pcwalton:per-glyph-font-fallback, r=glennw
d6943f2 
This improves numerous pages, for example Wikipedia and Ars Technica.

Built on #5493.

Closes #177.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/5607)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux1

@jdm
Copy link
Member

jdm commented Apr 22, 2015 

    line_height_float_placement_a.html == line_height_float_placement_ref.html
    text_align_complex_a.html == text_align_complex_ref.html

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #5827) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels May 20, 2015
@mbrubeck mbrubeck added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-review There is new code that needs to be reviewed. labels May 20, 2015
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels May 22, 2015
@pcwalton
Copy link
Contributor Author

@bors-servo: r=mbrubeck

I think I found the problem: newlines aren't in Ahem so trigger inconsistent font fallback.

@bors-servo
Copy link
Contributor

📌 Commit 689a2e5 has been approved by mbrubeck

@bors-servo
Copy link
Contributor

⌛ Testing commit 689a2e5 with merge f13ab73...

bors-servo pushed a commit that referenced this pull request May 22, 2015
Auto merge of #5607 - pcwalton:per-glyph-font-fallback, r=mbrubeck
f13ab73 
This improves numerous pages, for example Wikipedia and Ars Technica.

Built on #5493.

Closes #177.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/5607)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux1

@pcwalton
layout: Implement per-glyph font fallback.
fec43b4 
This improves numerous pages, for example Wikipedia and Ars Technica.

Closes servo#177.
@pcwalton
Copy link
Contributor Author

@bors-servo: r=mbrubeck

@bors-servo
Copy link
Contributor

📌 Commit fec43b4 has been approved by mbrubeck

@bors-servo
Copy link
Contributor

⌛ Testing commit fec43b4 with merge 7561f7b...

bors-servo pushed a commit that referenced this pull request May 23, 2015
Auto merge of #5607 - pcwalton:per-glyph-font-fallback, r=mbrubeck
7561f7b 
This improves numerous pages, for example Wikipedia and Ars Technica.

Built on #5493.

Closes #177.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/5607)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2

@bors-servo bors-servo merged commit fec43b4 into servo:master May 23, 2015
@pcwalton pcwalton deleted the per-glyph-font-fallback branch May 23, 2015 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mbrubeck mbrubeck

Labels
S-awaiting-review There is new code that needs to be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TextRun creation must account for text render-ability by FontGroup fonts
7 participants
@pcwalton @hoppipolla-critic-bot @bors-servo @mbrubeck @jdm @glennw @highfive