Skip to content

/ webrender

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

Implement synthetic italics on Linux. #1721

Merged
merged 1 commit into from Sep 21, 2017
Merged

Implement synthetic italics on Linux. #1721

merged 1 commit into from Sep 21, 2017
+64 −12

Conversation

@glennw
Copy link
Member

glennw commented Sep 19, 2017 •
edited by larsbergstrom

Mac and Windows implementations still need to be implemented.

This change is Reviewable
@glennw
Copy link
Member Author

glennw commented Sep 19, 2017

italics
@glennw
Copy link
Member Author

glennw commented Sep 19, 2017

r? @nical or @kvark

cc @gankro - this is just the Linux implementation, for now.
@glennw glennw force-pushed the glennw:synthetic branch from eee3708 to 0f35f46 Sep 19, 2017
@nical
Copy link
Collaborator

nical commented Sep 19, 2017

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Sep 19, 2017

📌 Commit 0f35f46 has been approved by nical
@bors-servo
Copy link
Contributor

bors-servo commented Sep 19, 2017

⌛ Testing commit 0f35f46 with merge 377811d...
bors-servo added a commit that referenced this pull request Sep 19, 2017
@bors-servo
Auto merge of #1721 - glennw:synthetic, r=nical
Loading status checks…
377811d 
Implement synthetic italics on Linux.

Mac and Windows implementations still need to be implemented.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1721)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 19, 2017

💔 Test failed - status-travis
@glennw
Copy link
Member Author

glennw commented Sep 19, 2017

@bors-servo retry
@glennw glennw force-pushed the glennw:synthetic branch from 0f35f46 to da943ca Sep 19, 2017
@glennw
Copy link
Member Author

glennw commented Sep 19, 2017

@bors-servo r=nical
@bors-servo
Copy link
Contributor

bors-servo commented Sep 19, 2017

📌 Commit da943ca has been approved by nical
@glennw glennw force-pushed the glennw:synthetic branch from da943ca to cf49b84 Sep 19, 2017
@glennw
Copy link
Member Author

glennw commented Sep 19, 2017

@bors-servo r=nical
@bors-servo
Copy link
Contributor

bors-servo commented Sep 19, 2017

📌 Commit cf49b84 has been approved by nical
@bors-servo
Copy link
Contributor

bors-servo commented Sep 20, 2017

🔒 Merge conflict
@bors-servo
Copy link
Contributor

bors-servo commented Sep 20, 2017

☔ The latest upstream changes (presumably #1714) made this pull request unmergeable. Please resolve the merge conflicts.
@gw3583
Implement synthetic italics on Linux.
Loading status checks…
4dbc5d8 
Mac and Windows implementations still need to be implemented.
@glennw glennw force-pushed the glennw:synthetic branch from cf49b84 to 4dbc5d8 Sep 21, 2017
@glennw
Copy link
Member Author

glennw commented Sep 21, 2017

Rebased.

@bors-servo r=nical
@bors-servo
Copy link
Contributor

bors-servo commented Sep 21, 2017

📌 Commit 4dbc5d8 has been approved by nical
@bors-servo
Copy link
Contributor

bors-servo commented Sep 21, 2017

⌛ Testing commit 4dbc5d8 with merge 7328cb4...
bors-servo added a commit that referenced this pull request Sep 21, 2017
@bors-servo
Auto merge of #1721 - glennw:synthetic, r=nical
Loading status checks…
7328cb4 
Implement synthetic italics on Linux.

Mac and Windows implementations still need to be implemented.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1721)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 21, 2017

☀️ Test successful - status-travis
Approved by: nical
Pushing 7328cb4 to master...
@bors-servo bors-servo merged commit 4dbc5d8 into servo:master Sep 21, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@glennw glennw deleted the glennw:synthetic branch Sep 21, 2017
@staktrace
Copy link
Contributor

staktrace commented Sep 21, 2017

@glennw @nical This broke the gecko build, see https://bugzilla.mozilla.org/show_bug.cgi?id=1401244#c3

The problem appears to be that Option inside FontInstanceOptions is not supported by cbindgen; it generates a dummy "struct Option_FontRenderMode;" in the generated bindings but that's not sufficient.

@eqrion Thoughts? Can we make cbindgen support this? ^
@eqrion
Copy link
Contributor

eqrion commented Sep 21, 2017 •
edited

An Option of an enum is tricky/unsafe to use in FFI. We'd need something like the work @gankro was doing in rustc
@staktrace
Copy link
Contributor

staktrace commented Sep 21, 2017

@nical Should we back this change out then? It's going to block updating gecko to anything past WR 2a005f1
@nical
Copy link
Collaborator

nical commented Sep 21, 2017

I think that we can work around this in bindings.rs, I'd like to try that before backing out if you are not in a hurry.
@staktrace
Copy link
Contributor

staktrace commented Sep 21, 2017

Sure, if you have something in mind to work around this I'm happy to wait a bit.
@glennw
Copy link
Member Author

glennw commented Sep 21, 2017

Oh, it didn't even occur to me that cbindgen would have trouble with this. One easy way is I could add a "Default" variant to FontRenderMode, and then we use FontRenderMode::Default instead of None, which would mean it doesn't need to be an option. Want me to make that change?
@staktrace
Copy link
Contributor

staktrace commented Sep 21, 2017

The patch nical posted to bugzilla seems to solve the problem. I don't have a strong opinion as to which solution is preferable.
@glennw
Copy link
Member Author

glennw commented Sep 21, 2017

OK, I'll wait to hear @nical's opinion before making any change.
@nical
Copy link
Collaborator

nical commented Sep 25, 2017

The WebRender update landed last Friday, no need to make the FontRenderMode change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@glennw @nical @bors-servo @staktrace @eqrion @gw3583
You can’t perform that action at this time.