Skip to content

Merge upstream changes from Warp's core-foundation-rs fork - #517

Closed
alokedesai wants to merge 8 commits into
servo:mainfrom
warpdotdev:master
Closed

Merge upstream changes from Warp's core-foundation-rs fork#517
alokedesai wants to merge 8 commits into
servo:mainfrom
warpdotdev:master

Conversation

@alokedesai

Copy link
Copy Markdown
Contributor

Hey servo folks,
Sending a PR to merge Warp's changes to core-foundation-rs upstream. Note, these changes are purely additive, and shouldn't have an affect on any existing core-foundation-rs APIs:

Changes

  1. Adds get_matrix to CTFont, matching CTFontGetMatrix.
  2. Adds get_typographic_bounds to CTRun, matching CTRunGetTypographicBounds
  3. Adds a patch for a panic in extract_number_for_key that occurs in Ventura. The underlying issue is an issue within the Core Framework library on Ventura--but this patch allows this library to not panic on Ventura if someone tries to access any of the font traits.

@alokedesai

alokedesai commented Jun 27, 2022

Copy link
Copy Markdown
Contributor Author

cc @madsmtm--tagging you in case you'd be interested in reviewing. I think the changes here (notably the change to fix a crash on Ventura) would be pretty valuable to be merged upstream. Thanks!

@jrmuizel

Copy link
Copy Markdown
Collaborator

Can you clean up the commit history and avoid the formatting changes?

@jrmuizel

Copy link
Copy Markdown
Collaborator

Can you also give some example code or add a test case for reproducing the Ventura failure?

@madsmtm madsmtm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know much about CoreText, but probably looks fine to me

@bors-servo

Copy link
Copy Markdown
Contributor

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

@vorporeal

Copy link
Copy Markdown
Contributor

In the name of finally getting (at least some of) these changes merged upstream, I've sent out #674 and #675 for the bits adding new functions.

In terms of the Ventura fix, I can't think of an easy way to write a test for it. FWIW, this has been live in Warp (used by ~100k macOS users) for a year now with no indication of it causing any issues. I can send out a separate PR for that as well, or we can keep it in our own fork; either works for us.

@nicoburns

Copy link
Copy Markdown

I can send out a separate PR for that as well, or we can keep it in our own fork; either works for us.

I definitely think we will want this fix upstream. A separate PR might make sense at this point seeing as it has been done for the other changes.

In terms of the Ventura fix, I can't think of an easy way to write a test for it.

I think the best test for this would be a usage of the API that triggers the bug without the fix if you have a standalone reproduction (perhaps using a particular system font). But unless anyone else strongly objects, I think it would be better to land this without a test than not at all if you are unable to provide that. A link to the original bug in warp that lead to this fix would also potentially be helpful.

I'm sorry that progress on reviewing/merging this has been so slow. The Servo project is still getting back up to speed after a long hiatus and is still quite behind on maintenance of libraries. Hopefully this will improve over the next few months.

@vorporeal

vorporeal commented May 1, 2024

Copy link
Copy Markdown
Contributor

Yeah we (@alokedesai or I) can put together a separate PR for the extract_number_for_key fix.

No need to apologize for latency here - there was an explicit request to clean up the commit history and provide some example/test code for the extract_number_for_key issue, and nobody from our side followed up. 😅

@vorporeal

Copy link
Copy Markdown
Contributor

Hey all, finally got around to sending out one last PR for the Ventura crash fix portion of this PR: #735

As mentioned in the PR description, we attempted ~6 months ago to switch to the published version of this crate (in case Apple had fixed the underlying issue in a newer release) but immediately had to roll back that release of Warp due to it still being an issue in the wild.

@jdm jdm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with these changes but it would be nice to clean up the commit history as previously requested.

@vorporeal

Copy link
Copy Markdown
Contributor

With the PR you just approved, we'll be able to close this out - I've already merged separate PRs for the other bits.

(Feel free to close this one out now and we can focus on getting #735 merged.)

@jdm

jdm commented Jul 12, 2025

Copy link
Copy Markdown
Member

Cool, let's do that!

@jdm jdm closed this Jul 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants