Skip to content
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
Merged

fix: match Chrome's font fallback behavior #15486

merged 6 commits into from Nov 8, 2018

Conversation

nornagon
Copy link
Member

@nornagon 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
Copy link
Member Author

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 November 6, 2018 01:56
@nornagon nornagon changed the title [wip] fix: match Chrome's font fallback behavior fix: match Chrome's font fallback behavior Nov 6, 2018
Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

LGTM

atom/browser/font_defaults.cc Show resolved Hide resolved
@jkleinsc jkleinsc merged commit 7e0e12b into master Nov 8, 2018
@release-clerk
Copy link

release-clerk bot commented Nov 8, 2018

Release Notes Persisted

fixed default font fallback for non-latin scripts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default fonts should match Chrome
4 participants