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

fix: match Chrome's font fallback behavior #15486

Merged
merged 6 commits into from Nov 8, 2018

Conversation

Projects
None yet
4 participants
@nornagon
Contributor

nornagon commented Oct 31, 2018

This PR sets up blink's default fonts (serif, sans-serif, etc.) to match Chrome's behavior, which includes per-script fallbacks.

Fixes #15481.

TODO:

  • tests. There's one test for japanese script, but it needs a couple more to make sure the mechanism is working correctly.
  • Chromium uses a cache and claims that iterating the whole list every time adds significantly to renderer startup time. That doesn't intuitively seem true to me, but I haven't measured. It needs to be measured, and possibly we should build a cache.
  • Evaluate the possibility of directly using font_family_cache.cc from Chrome. Its use of Profile doesn't match the needs of Electron.
  • Evaluate the possibility of splitting out kFontDefaults in upstream Chromium (defined here) so we can include them without patching. This would be difficult currently, these defaults are embedded pretty deep into Chromium's preference system and it'd be a challenge to refactor them to a common place in a way that would make sense to upstream to Chromium. The lists change quite infrequently though: the last change was in 2016, so I think it's OK to copy/paste them.

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes
  • tests are changed or added
  • relevant documentation is changed or added
  • PR title follows semantic commit guidelines

Release Notes

Notes: fixed default font fallback for non-latin scripts

@nornagon nornagon requested a review from electron/reviewers as a code owner Oct 31, 2018

@nornagon

This comment has been minimized.

Contributor

nornagon commented Nov 2, 2018

I measured the performance of the SetFontDefaults function and found it took about 500µs on my mac pro. I think that's slow enough that it's worth adding a cache.

@nornagon nornagon requested a review from ckerr Nov 6, 2018

@nornagon nornagon changed the title from [wip] fix: match Chrome's font fallback behavior to fix: match Chrome's font fallback behavior Nov 6, 2018

@jkleinsc

LGTM

Show resolved Hide resolved atom/browser/font_defaults.cc

nornagon added some commits Nov 6, 2018

@jkleinsc jkleinsc merged commit 7e0e12b into master Nov 8, 2018

21 checks passed

Absolute Zero
Semantic Pull Request ready to be squashed
Details
WIP ready for review
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-checkout Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing-tests Your tests passed on CircleCI!
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

release-clerk bot commented Nov 8, 2018

Release Notes Persisted

fixed default font fallback for non-latin scripts

@jkleinsc jkleinsc deleted the font-defaults branch Nov 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment