Skip to content

Fix SkOrderedFontMgr::onMatchFamily #193

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
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix SkOrderedFontMgr::onMatchFamily #193

wants to merge 3 commits into from
+11 −5

Conversation

samizdatco
Copy link
Contributor

SkOrderedFontMgr's matchFamily currently has a bug where it will only look in the first FontMgr in its list when searching for fonts. The problem is that its search loop is expecting a null response to signal "no matches", but the actual behavior of FontMgr is to never return null. Instead it will return a zero-length SkFontStyleSet.

This PR adds a count() > 0 check to the return value rather than just checking whether it is non-null.

@samizdatco
check for empty SkFontStyleSet rather than null
50f05e8 
- matchFamily never returns null; a zero-length set is its return value on match failure
@google-cla Google CLA
Copy link

google-cla bot commented Dec 4, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@skia-codereview-bot
Copy link

This PR (HEAD: 50f05e8) has been imported to Gerrit for code review.

Please visit review.skia.org/926716 to see it. Please CC yourself to the Gerrit change.

Note:

  • Skia uses only Gerrit for reviews and submitting code (doc).
  • All comments are handled within Gerrit. Any comments on the GitHub PR will be ignored.
  • The PR author can continue to upload commits to the branch used by the PR in order to address feedback from Gerrit.
  • Once the code is ready to be merged, a maintainer will submit the change on Gerrit and skia-codereview-bot will close this PR.
  • Similarly, if a change is abandoned on Gerrit, the corresponding PR will be closed with a note.

@pragmatrix pragmatrix mentioned this pull request Dec 4, 2024
Closed
@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

2 similar comments
@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

hubot pushed a commit that referenced this pull request Aug 21, 2025
@samizdatco
Fix SkOrderedFontMgr::onMatchFamily
41b1c0f 
SkOrderedFontMgr's `matchFamily` currently has [a bug](https://issues.skia.org/issues/382016481) where it will only look in the first FontMgr in its list when searching for fonts. The problem is that its search loop is expecting a `null` response to signal "no matches", but the actual behavior of FontMgr is to never return null. Instead it will return a zero-length SkFontStyleSet.

This PR adds a `count() > 0` check to the return value rather than just checking whether it is non-null.

This is an imported pull request from
#193

Added the author to the list of Skia AUTHORS

GitOrigin-RevId: 736a0e3
Change-Id: I6e0f9a6fe5a5d7e6a45567bc0f83ed27024cb584
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/926716
Commit-Queue: Julia Lavrova <[email protected]>
Reviewed-by: Ben Wagner <[email protected]>
Reviewed-by: Julia Lavrova <[email protected]>
@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

19 similar comments
@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

29 similar comments
@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

@skia-codereview-bot
Copy link

This PR is being closed because review.skia.org/926716 has been merged.

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
Development

Successfully merging this pull request may close these issues.
2 participants
@samizdatco @skia-codereview-bot