Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Move extension to TypefaceFontProvider instead of SkFontMgr #35381

Merged
auto-submit merged 1 commit into from
Aug 23, 2022
Merged

Move extension to TypefaceFontProvider instead of SkFontMgr #35381

auto-submit merged 1 commit into from
Aug 23, 2022

Conversation

@srujzs
Copy link
Contributor

@srujzs srujzs commented Aug 12, 2022 •
edited
Loading

This extension should belong on TypefaceFontProvider instead of the parent class. The only usage is in lib/web_ui/lib/src/engine/canvaskit/fonts.dart, and both usages are on a variable typed as TypefaceFontProvider. In the future, multiple extensions on the same @staticInterop type will be an error (dart-lang/sdk#49350), so this must be migrated.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • [NO - need test exemption/test is unneeded because this should not build if the extension is on the wrong class] I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Aug 12, 2022
@srujzs srujzs marked this pull request as ready for review August 12, 2022 19:09
@srujzs srujzs marked this pull request as draft August 12, 2022 19:11
@srujzs srujzs marked this pull request as ready for review August 12, 2022 19:11
@srujzs
Copy link
Contributor Author

srujzs commented Aug 12, 2022

cc @yjbanov PTAL

@flutter-dashboard
Copy link

flutter-dashboard bot commented Aug 17, 2022

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@Hixie
Copy link
Contributor

Hixie commented Aug 17, 2022

test-exempt: this effectively removes (broken) API surface, so there's nothing to test really.

yjbanov
yjbanov approved these changes Aug 23, 2022
View reviewed changes
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

lgtm

@yjbanov yjbanov added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 23, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Aug 23, 2022

  • Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@auto-submit
Copy link
Contributor

auto-submit bot commented Aug 23, 2022

Validations Fail.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 23, 2022
eyebrowsoffire
eyebrowsoffire approved these changes Aug 23, 2022
View reviewed changes
Copy link
Contributor

@eyebrowsoffire eyebrowsoffire left a comment

Choose a reason for hiding this comment

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

LGTM!

@yjbanov yjbanov added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 23, 2022
@auto-submit auto-submit bot merged commit d1ac8d8 into flutter:main Aug 23, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 24, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 24, 2022
@engine-flutter-autoroll
d1ac8d8 Move extension to TypefaceFontProvider instead of SkFontMgr (f…
a994021 
…lutter/engine#35381)
zanderso pushed a commit to flutter/flutter that referenced this pull request Aug 24, 2022
@engine-flutter-autoroll
Roll Flutter Engine from 2ecccf1 to de4ef8c (3 revisions) (#110132)
b5c2ef8 
* d1ac8d8 Move extension to TypefaceFontProvider instead of SkFontMgr (flutter/engine#35381)

* 55b46a7 Roll Skia from fad2781bd328 to 6cec033662c7 (3 revisions) (flutter/engine#35649)

* de4ef8c Roll Dart SDK from 3042d6196824 to 9be467274ae4 (1 revision) (flutter/engine#35650)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@yjbanov yjbanov yjbanov approved these changes

@eyebrowsoffire eyebrowsoffire eyebrowsoffire approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App needs tests platform-web Code specifically for the web engine

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@srujzs @Hixie @yjbanov @eyebrowsoffire