-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Use Skia's CSS3 style matcher for dynamically loaded font sets #15468
Use Skia's CSS3 style matcher for dynamically loaded font sets #15468
Conversation
This will improve font matching for SkParagraph, which relies on the FontStyleSet's matchStyle implementation to find the closest match for a FontStyle.
e95251f
to
0696098
Compare
@@ -39,7 +39,7 @@ class TypefaceFontStyleSet : public SkFontStyleSet { | |||
int count() override; | |||
|
|||
// |SkFontStyleSet| | |||
void getStyle(int index, SkFontStyle*, SkString* style) override; | |||
void getStyle(int index, SkFontStyle* style, SkString* name) override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you document what this method does with the name
parameter. It isn't clear why name is passed into a method called getStyle
especially since the name isn't used to determine the style produced.
The name parameter is not in SkFontStyleSet
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an implementation of an method defined by SkFontStyleSet
:
https://github.com/google/skia/blob/master/include/core/SkFontMgr.h#L25
The parameter names came from:
https://github.com/google/skia/blob/master/src/ports/SkFontMgr_custom.h#L107
AFAICT this is an optional parameter that font style set implementations can use to provide a name for a style for debugging purposes. I don't know of any official docs from Skia about this parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh ok, well in any case, may be useful to document that to reduce need to sleuth in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
flutter/engine#15468) (#48784) flutter/engine@0235a50...fd269f6 git log 0235a50..fd269f6 --first-parent --oneline 2020-01-14 jason-simmons@users.noreply.github.com Use Skia's CSS3 style matcher for dynamically loaded font sets (flutter/engine#15468) 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 garyq@google.com on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
…er#15468) This will improve font matching for SkParagraph, which relies on the FontStyleSet's matchStyle implementation to find the closest match for a FontStyle.
flutter#15468)" This reverts commit 4d391b5.
…er#15468) This will improve font matching for SkParagraph, which relies on the FontStyleSet's matchStyle implementation to find the closest match for a FontStyle.
This will improve font matching for SkParagraph, which relies on the
FontStyleSet's matchStyle implementation to find the closest match for a
FontStyle.