-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Cache system font shorthand information #1369
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
Cache system font shorthand information #1369
Conversation
5522d4e
to
ef91774
Compare
ef91774
to
f3ee595
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't check closely that the SystemFontDatabaseCoreText::xxxFontDescriptor
functions are looking up the correct fonts, or that the table mapping CTFontWeights <=> CSS weights are reasonable. And I assume the Gtk/PS/Windows port code is just being moved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to have a similar comment in CSSValueKeywords.in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not asking you to change this, and I guess it's not prevailing WebKit style to do this (only see a few instances of it), but I prefer the idiom of adding a Count
value as the last value of an enum, when that's a thing that's needed. It would remove the risk of fontShorthandCount
not being updated correctly. (I know that this means you'll get some non-exhaustive switch statement errors for that new value, but I think adding a dummy case for Count
is a reasonable price to pay.)
I wouldn't bother subtracting the Caption
value, if these enums won't have explicitly assigned values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think I disagree 😕 I guess kind of feel that A) the whole point of enums is to restrict the representable values of an int
to just the ones that are valid, but if you add new values which aren't valid, you've lost the whole philosophical purpose of enums, and B) more pragmatically, I'm more bothered by extra cases in switches (or the siren's song of just using default
instead) than I am about having some things in WebKit have to match other things - which is already true about lots of things in WebKit, including some other things in this patch itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good idea, but I think it's a bit outside the scope of this change. I'll do this in a follow-up patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition took me too long to work out. :-) It would be easier to understand as:
if (value >= before.ctWeight && value <= after.ctWeight) {
...
return ...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in general in WebKit we prefer early-return style, but I'm happy to reorganize this code to make it more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Normalize" and "denormalize" don't sound like the right terms to me. What about convertCTWeightToCSSWeight
and vice versa?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I use normalize
in other places, under the assumption that we are a web engine so of course the "normal" form will be the CSS form. I think I take your point, though - I'll rename all the places in a follow-up patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above.
Source/WebCore/SourcesWPE.txt
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
“Adwaita” refers to the default GTK theme for user interface controls, which we do reuse in the WPE port. I guess the idea is using this implementation for the WPE port, given that the GTK port gets its own version of the code. If that's the case, it would make more sense to call if SystemFontDatabaseWPE.cpp
or even SystemFontDatabaseDefault.cpp
because there is nothing specific about WPE in the code 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, no problem 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
f3ee595
to
8d161b2
Compare
8d161b2
to
c048c5d
Compare
c048c5d
to
10cdfcb
Compare
Committed r295410 (251416@main): https://commits.webkit.org/251416@main Reviewed commits have been landed. Closing PR #1369 and removing active labels. |
This broke GTK build btw... |
…ki/2.38/do_not_stop_navigation_when_fragment_navigation_starts Do not stop navigation when fragment navigation starts
10cdfcb