This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Make SkUnicode explicitly instead of relying on SkParagraph to make it for us#52086
Merged
kjlubick merged 4 commits intoflutter:mainfrom Apr 29, 2024
Merged
Make SkUnicode explicitly instead of relying on SkParagraph to make it for us#52086kjlubick merged 4 commits intoflutter:mainfrom
kjlubick merged 4 commits intoflutter:mainfrom
Conversation
hubot
pushed a commit
to google/skia
that referenced
this pull request
Apr 15, 2024
Flutter uses the public interface and also need access to the getters. See flutter/engine#52086 Change-Id: I84e2a09a65c63328ba20fee42a80cf851744cb14 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/840000 Reviewed-by: Ben Wagner <[email protected]> Commit-Queue: Ben Wagner <[email protected]> Auto-Submit: Kevin Lubick <[email protected]>
Contributor
|
I was expecting a similar change to our CanvasKit side too: https://github.com/flutter/engine/blob/main/lib/web_ui/lib/src/engine/canvaskit/text.dart#L1133 Is there a reason CanvasKit didn't require a change? Also, I think @jason-simmons should take a look too. |
jason-simmons
approved these changes
Apr 29, 2024
Contributor
Author
|
canvaskit did need changes, e.g. https://github.com/google/skia/blob/f7bfa8eef5b576b0d024216db7ee43353efc6ee3/modules/canvaskit/paragraph_bindings.cpp#L655-L660 However, the dart code you link shells out to CanvasKit, which didn't have any API changes (we hid those away). |
mdebbar
approved these changes
Apr 29, 2024
Contributor
mdebbar
left a comment
mdebbar
left a comment
There was a problem hiding this comment.
However, the dart code you link shells out to CanvasKit, which didn't have any API changes (we hid those away).
Nice! Thanks for clarifying.
LGTM!
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Apr 29, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Apr 29, 2024
auto-submit bot
pushed a commit
to flutter/flutter
that referenced
this pull request
Apr 29, 2024
flutter/engine@399837d...5165c71 2024-04-29 [email protected] Remove references to goma (flutter/engine#52411) 2024-04-29 [email protected] Make SkUnicode explicitly instead of relying on SkParagraph to make it for us (flutter/engine#52086) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In https://skia-review.googlesource.com/c/skia/+/838417 Skia staged the removal of automatically creating an SkUnicode as part of the effort to make Skia more modular. Clients will now have to construct and SkUnicode and pass it to SkParagraph to provide the appropriate data. Flutter sometimes uses ICU and sometimes uses a "client" SkUnicode which makes use of the browser APIs to get the data.
We should come back to the skwasm code especially, because right now skwasm gets the data necessary to make an SkUnicode::Client, but stores it in the ParagraphBuilder, just to take it out again later to make the SkUnicode::Client and pass that back into the ParagraphBuilder. skwasm should just create the SkUnicode::Client directly and pass it to the ParagraphBuilder::make().
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.