Skip to content

[Skia] Implement Font::applyTransforms with HarfBuzz #50802

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
Closed
wants to merge 1 commit into from
Closed

[Skia] Implement Font::applyTransforms with HarfBuzz #50802

wants to merge 1 commit into from

Conversation

@carlosgcampos
Copy link
Contributor

@carlosgcampos carlosgcampos commented Sep 16, 2025 •
edited by webkit-early-warning-system
Loading

4de8bbf

[Skia] Implement Font::applyTransforms with HarfBuzz
https://bugs.webkit.org/show_bug.cgi?id=297088

Reviewed by NOBODY (OOPS!).

Revert 299354@main and implement Font::applyTransforms instead, trying
to skip the harfbuzz shapping when possible.

4de8bbf

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ❌ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ❌ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@carlosgcampos carlosgcampos self-assigned this Sep 16, 2025
@carlosgcampos carlosgcampos added the Platform Portability improvements and other general platform improvements not driven directly by site bugs. label Sep 16, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Sep 16, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 3498ad6)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ❌ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ❌ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ❌ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@carlosgcampos carlosgcampos marked this pull request as draft September 16, 2025 12:38
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Sep 16, 2025
aperezdc
aperezdc reviewed Sep 16, 2025
View reviewed changes
Source/WebCore/platform/graphics/skia/FontSkia.cpp Outdated
Vector<hb_feature_t> featuresWithKerning;
if (!enableKerning) {
featuresWithKerning.reserveInitialCapacity(featuresSize + 1);
static hb_feature_t kernFeature { HB_TAG('k', 'e', 'r', 'n'), 0, 0, static_cast<unsigned>(-1) };
Copy link
Contributor

@aperezdc aperezdc Sep 16, 2025

Choose a reason for hiding this comment

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

This could be const, or even constexpr:

Suggested change
static hb_feature_t kernFeature { HB_TAG('k', 'e', 'r', 'n'), 0, 0, static_cast<unsigned>(-1) };
static constexpr hb_feature_t kernFeature { HB_TAG('k', 'e', 'r', 'n'), 0, 0, static_cast<unsigned>(-1) };

@carlosgcampos carlosgcampos removed the merging-blocked Applied to prevent a change from being merged label Sep 17, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Sep 17, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 8bc064d)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe   🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ⏳ 🧪 wpe-wk2   🧪 win-tests
✅ 🧪 webkitperl   🧪 ios-wk2 ⏳ 🧪 api-mac ⏳ 🧪 api-wpe
  🧪 ios-wk2-wpt   🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc   🧪 api-ios   🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision   🧪 mac-AS-debug-wk2   🧪 gtk-wk2
✅ 🛠 vision-sim   🧪 mac-wk2-stress   🧪 api-gtk
  🧪 vision-wk2   🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim   🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@carlosgcampos
[Skia] Implement Font::applyTransforms with HarfBuzz
4de8bbf 
https://bugs.webkit.org/show_bug.cgi?id=297088

Reviewed by NOBODY (OOPS!).

Revert 299354@main and implement Font::applyTransforms instead, trying
to skip the harfbuzz shapping when possible.
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Sep 17, 2025 •
edited
Loading

EWS run on current version of this PR (hash 4de8bbf)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ❌ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ❌ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Sep 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aperezdc aperezdc aperezdc left review comments

@alexgcastro alexgcastro Awaiting requested review from alexgcastro alexgcastro is a code owner

@nikolaszimmermann nikolaszimmermann Awaiting requested review from nikolaszimmermann nikolaszimmermann is a code owner

Assignees

@carlosgcampos carlosgcampos

Labels

merging-blocked Applied to prevent a change from being merged Platform Portability improvements and other general platform improvements not driven directly by site bugs.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@carlosgcampos @webkit-early-warning-system @aperezdc @webkit-ews-buildbot