[webkit-reviews] review granted: [Bug 222735] Allow creation of a CSSFontSelector with a non-Document ScriptExecutionContext : [Attachment 422381] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Mar 5 12:20:36 PST 2021
Darin Adler <darin at apple.com> has granted Chris Lord <clord at igalia.com>'s
request for review:
Bug 222735: Allow creation of a CSSFontSelector with a non-Document
ScriptExecutionContext
https://bugs.webkit.org/show_bug.cgi?id=222735
Attachment 422381: Patch
https://bugs.webkit.org/attachment.cgi?id=422381&action=review
--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 422381
--> https://bugs.webkit.org/attachment.cgi?id=422381
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=422381&action=review
Looks fine; not sure why it’s not building on all platforms yet.
> Source/WebCore/css/CSSFontSelector.cpp:306
> + bool resolveGenericFamilyFirst = familyName ==
m_fontFamilyNames[static_cast<long int>(FamilyNamesIndex::StandardFamily)];
The static_cast should be to "unsigned" or "int". We should never need to use
"long int" in our code. Also wondering why the cast is needed at all, but maybe
it’s converting from an enum to an integer?
> Source/WebCore/platform/graphics/FontGenericFamilies.cpp:202
> + case FamilyNamesIndex::SystemUiFamily:
> + default:
> + return WTF::nullopt;
If you leave out default then we’ll get a warning if we forget to handle any of
the enumeration values; that’s a welcome tiny bit of future-proofing. Instead
you can put the return WTF::nullopt after the switch statement. Unless that
runs into warning problems on Windows compilers or something?
> Source/WebCore/platform/graphics/FontGenericFamilies.h:27
> #ifndef FontGenericFamilies_h
> #define FontGenericFamilies_h
Would be nice to move to #pragma once since we’re touching this file.
> Source/WebCore/platform/graphics/FontGenericFamilies.h:63
> + Optional<const String&>
fontFamily(WebKitFontFamilyNames::FamilyNamesIndex, UScriptCode =
USCRIPT_COMMON) const;
Not sure the return type here is optimal. I’d suggest just const String& and
returning a reference to a null or empty string if there’s no failure, or const
String* and using nullptr.
Optional is really clear semantically, but one of those others might be nice
since String already has a built in null and empty value, and references have a
natural optional form (pointers) without using the Optional template.
More information about the webkit-reviews
mailing list