Faster, more compliant parseFont #944
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chearon
reviewed
Jul 24, 2017
| // TODO disabled because existing unit tests assume 100 | ||
| // font.size *= defaultHeight / 100 / 0.75 | ||
| break | ||
| case 'em': |
| break | ||
| case 'cm': | ||
| size.value *= 96.0 / 2.54 | ||
| case '%': |
There was a problem hiding this comment.
So yeah, I don't think the test is right... the units do get sent to C++, but aren't used. We probably don't need to send the units, just the normalized px value.
It looks like the current version would be doing percentages and mm wrong, I'll have to look at if the non-Pango build was ever right about that.
LinusU
reviewed
Jul 31, 2017
| ]; | ||
|
|
||
| for (var i = 0, len = tests.length; i < len; ++i) { | ||
| var str = tests[i++] | ||
| , obj = tests[i] | ||
| , expected = tests[i] |
|
Awesome! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Improves perf by ~23x. This is the fastest method that I can come up with, short of dropping into C++, and is faster than parse-css-font and css-font-parser.
Fixes caching -- should be done before parsing the string.
Supports more of the spec:
font-style,font-variant,font-weight,font-stretchmay appear in any orderfont-stretch(although it will be ignored)em,remandqwork;chandexwill parse but not work. Supportingchandexwould be fastest if the API was reworked so that the actual number of device units was calculated in the same call asfill/strokeText.Also, I think handling of
%is wrong but I didn't fix it because there's an existing assertion for it. As I read it, that should be percent of the parent (default/root) font height, and with this commit I've standardized that to 16 pt (common in browsers). Right now20%parses to20 px. (@chearon?)Fixes #920
Fixes or at least improves #566