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

[CanvasKit] Automatically fall back to Noto fonts #23096

Merged
merged 17 commits into from
Dec 29, 2020
Merged

[CanvasKit] Automatically fall back to Noto fonts #23096

merged 17 commits into from
Dec 29, 2020

Conversation

@harryterkelsen
Copy link
Contributor

@harryterkelsen harryterkelsen commented Dec 15, 2020 •
edited
Loading

Description

Checks if the given fonts can display all of the code units in a string, and if not, tries to download the fonts from Google Fonts which have glyphs for the missing code units.

Related Issues

Fixes flutter/flutter#53897
Fixes flutter/flutter#58496
Fixes flutter/flutter#58506

Tests

I will add tests in a follow-up PR. TODO create issue to add tests

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the contributor guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the C++, Objective-C, Java style guides for the engine.
  • I read the tree hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation.
  • All existing and new tests are passing.
  • I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change.

@harryterkelsen harryterkelsen added the Work in progress (WIP) Not ready (yet) for review! label Dec 15, 2020
@flutter-dashboard
Copy link

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.

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

@google-cla google-cla bot added the cla: yes label Dec 15, 2020
@harryterkelsen
Copy link
Contributor Author

I am uploading this WIP PR for review since it's pretty big @yjbanov. I am working on fixing the relayout issue currently.

@harryterkelsen
Copy link
Contributor Author

Fixed the issue of not updating the paragraphs when the new fonts are loaded.

yjbanov
yjbanov reviewed Dec 16, 2020
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.

Some preliminary nitpicks.

@fanchou fanchou mentioned this pull request Dec 16, 2020
Merged
13 tasks
yjbanov
yjbanov approved these changes Dec 17, 2020
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 modulo comments (which are non-blocking as long as there are no significant correctness and performance regressions). This needs tests, ideally with screenshots. Probably integration test is not necessary; a unit-test should be able to do the job. The PR adds a lot of non-trivial code paths to the paragraph construction, particular character-by-character look-ups. Let's make sure this does not regress performance. We should have some benchmarks that capture this, but perhaps we could add more (e.g. a microbenchmark that measures pure paragraph construction, without layout/rendering).

Leaving to you and @ferhatb to finish up.

@yjbanov yjbanov requested a review from ferhatb December 17, 2020 00:16
ferhatb
ferhatb suggested changes Dec 17, 2020
View reviewed changes
@harryterkelsen harryterkelsen removed the Work in progress (WIP) Not ready (yet) for review! label Dec 18, 2020
@harryterkelsen harryterkelsen merged commit eff27c7 into flutter:master Dec 29, 2020
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 29, 2020
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 29, 2020
@engine-flutter-autoroll
eff27c7 [CanvasKit] Automatically fall back to Noto fonts (flutter/en…
f93c4e5 
…gine#23096)
harryterkelsen added a commit to harryterkelsen/engine that referenced this pull request Dec 30, 2020
@harryterkelsen
Revert "[CanvasKit] Automatically fall back to Noto fonts (flutter#23096
1b6f869 
)"

This reverts commit eff27c7.

Reason for revert: Incorrect logic breaks Gallery
harryterkelsen pushed a commit that referenced this pull request Dec 31, 2020
Revert "[CanvasKit] Automatically fall back to Noto fonts (#23096)" (#…
f9f4d01 
…23357)

This reverts commit eff27c7.

Reason for revert: Incorrect logic breaks Gallery
gspencergoog pushed a commit to gspencergoog/engine that referenced this pull request Jan 5, 2021
@gspencergoog
[CanvasKit] Automatically fall back to Noto fonts (flutter#23096)
cfb5bdf
gspencergoog pushed a commit to gspencergoog/engine that referenced this pull request Jan 5, 2021
@gspencergoog
Revert "[CanvasKit] Automatically fall back to Noto fonts (flutter#23096
fe3a2dd 
)" (flutter#23357)

This reverts commit eff27c7.

Reason for revert: Incorrect logic breaks Gallery
yjbanov pushed a commit to yjbanov/engine that referenced this pull request Jan 7, 2021
@yjbanov
Revert "[CanvasKit] Automatically fall back to Noto fonts (flutter#23096
db041a4 
)" (flutter#23357)

This reverts commit eff27c7.

Reason for revert: Incorrect logic breaks Gallery
This was referenced Jan 7, 2021
Closed
Closed
Closed
harryterkelsen added a commit to harryterkelsen/engine that referenced this pull request Jan 15, 2021
@harryterkelsen
Revert "Revert "[CanvasKit] Automatically fall back to Noto fonts (fl…
680ef20 
…utter#23096)" (flutter#23357)"

This reverts commit f9f4d01.
@harryterkelsen harryterkelsen mentioned this pull request Jan 15, 2021
Merged
9 tasks
harryterkelsen pushed a commit that referenced this pull request Jan 21, 2021
Automatically download Noto fonts as backup fonts in CanvasKit mode (#…
5b9cd44 
…23728)

* Revert "Revert "[CanvasKit] Automatically fall back to Noto fonts (#23096)" (#23357)"

This reverts commit f9f4d01.

* WIP

* Use an Interval Tree to store the unicode ranges for the Noto Fonts

* Update licenses

* Remove debug print statements

* Respond to comments

* Fix analysis error

* Add tests

* Respond to comments

* Fix test

* Update goldens lock

* Skip screenshot test on Safari

* Skip CanvasKit tests on iOS Safari

* Move CanvasKit initialization so it doesn't run on iOS Safari
hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021
@hjfreyer
Automatically download Noto fonts as backup fonts in CanvasKit mode (f…
573c18a 
…lutter#23728)

* Revert "Revert "[CanvasKit] Automatically fall back to Noto fonts (flutter#23096)" (flutter#23357)"

This reverts commit f9f4d01.

* WIP

* Use an Interval Tree to store the unicode ranges for the Noto Fonts

* Update licenses

* Remove debug print statements

* Respond to comments

* Fix analysis error

* Add tests

* Respond to comments

* Fix test

* Update goldens lock

* Skip screenshot test on Safari

* Skip CanvasKit tests on iOS Safari

* Move CanvasKit initialization so it doesn't run on iOS Safari
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@yjbanov yjbanov Awaiting requested review from yjbanov

1 more reviewer

@ferhatb ferhatb ferhatb approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

3 participants

@harryterkelsen @yjbanov @ferhatb