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

layout: Load Web fonts asynchronously. #7596

Merged
merged 2 commits into from Sep 28, 2015

Conversation

@pcwalton
Copy link
Contributor

pcwalton commented Sep 10, 2015

Improves page load times significantly.

Closes #7343.

Review on Reviewable

@highfive
Copy link

highfive commented Sep 10, 2015

warning Warning warning

  • These commits modify layout code, but no reftests are modified. Please consider adding a reftest!
@pcwalton
Copy link
Contributor Author

pcwalton commented Sep 10, 2015

@jdm
Copy link
Member

jdm commented Sep 10, 2015

I think I'd prefer this use the official async network request API, rather than the ad-hoc version it currently does.

@pcwalton pcwalton force-pushed the pcwalton:async-web-font-loading branch from b22e4a4 to 424cb42 Sep 10, 2015
@pcwalton
Copy link
Contributor Author

pcwalton commented Sep 10, 2015

Updated to use the API suggested by @jdm.

@bors-servo
Copy link
Contributor

bors-servo commented Sep 13, 2015

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

@pcwalton
Copy link
Contributor Author

pcwalton commented Sep 15, 2015

@jdm Did you have any more comments on this? I don't recall if we discussed anything more on IRC.

@pcwalton pcwalton force-pushed the pcwalton:async-web-font-loading branch from 424cb42 to e195a0a Sep 15, 2015
@jdm
Copy link
Member

jdm commented Sep 16, 2015

My preference would be to integrate the network requests with some existing thread's event loop, rather than spawning two new threads per requested font.

@pcwalton pcwalton force-pushed the pcwalton:async-web-font-loading branch from e195a0a to 7d77a5e Sep 17, 2015
@pcwalton
Copy link
Contributor Author

pcwalton commented Sep 17, 2015

I eliminated one thread by using the router and the other one by integrating into the layout task event loop. r? @mbrubeck

@jdm
Copy link
Member

jdm commented Sep 17, 2015

Thanks!

@mbrubeck
Copy link
Contributor

mbrubeck commented Sep 18, 2015

-S-awaiting-review +S-needs-code-changes


Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.


components/gfx/font_cache_task.rs, line 158 [r1] (raw file):
This should check whether the ResponseComplete message contains an Err status.


components/gfx/font_cache_task.rs, line 358 [r1] (raw file):
This result isn't read anywhere. Should we remove it and just send and empty message?


components/layout/layout_task.rs, line 481 [r1] (raw file):
Could we switch these to use descriptive names?


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Sep 19, 2015

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

@pcwalton pcwalton force-pushed the pcwalton:async-web-font-loading branch from 7d77a5e to 5133ffa Sep 25, 2015
@pcwalton
Copy link
Contributor Author

pcwalton commented Sep 25, 2015

Review status: 1 of 9 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.


components/layout/layout_task.rs, line 0 [r1] (raw file):
Hmm, Reviewable didn't seem to save the context here. Could you specify which names you're referring to?


Comments from the review on Reviewable.io

@pcwalton pcwalton force-pushed the pcwalton:async-web-font-loading branch from 5133ffa to af8c3f1 Sep 25, 2015
@pcwalton
Copy link
Contributor Author

pcwalton commented Sep 25, 2015

Rebased and comments addressed except for the naming one. r? @mbrubeck

@jdm
Copy link
Member

jdm commented Sep 25, 2015

If you click on the link in the comment on github you'll see the right context.

@mbrubeck
Copy link
Contributor

mbrubeck commented Sep 25, 2015

-S-awaiting-review +S-needs-code-changes


Reviewed 9 of 9 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


components/layout/layout_task.rs, line 481 [r1] (raw file):
port1, port2, port3, port4


Comments from the review on Reviewable.io

@pcwalton pcwalton force-pushed the pcwalton:async-web-font-loading branch from af8c3f1 to 3b72686 Sep 27, 2015
@pcwalton pcwalton force-pushed the pcwalton:async-web-font-loading branch from 3b72686 to 1892b27 Sep 27, 2015
@pcwalton
Copy link
Contributor Author

pcwalton commented Sep 27, 2015

@mbrubeck Addressed the naming comment. r?

pcwalton added 2 commits Sep 10, 2015
Improves page load times significantly.

Closes #7343.
@mbrubeck
Copy link
Contributor

mbrubeck commented Sep 28, 2015

Reviewed 4 of 4 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


Comments from the review on Reviewable.io

@mbrubeck
Copy link
Contributor

mbrubeck commented Sep 28, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Sep 28, 2015

📌 Commit 1892b27 has been approved by mbrubeck

@bors-servo
Copy link
Contributor

bors-servo commented Sep 28, 2015

Testing commit 1892b27 with merge 7933b48...

bors-servo pushed a commit that referenced this pull request Sep 28, 2015
layout: Load Web fonts asynchronously.

Improves page load times significantly.

Closes #7343.

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

bors-servo commented Sep 28, 2015

@bors-servo bors-servo merged commit 1892b27 into servo:master Sep 28, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Sep 28, 2015
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

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