Bug 148355 – @font-face related cleanup

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

@font-face related cleanup

[Myles C. Maxfield]

Created attachment 259711 [details]
Patch

[Myles C. Maxfield]

Created attachment 259717 [details]
Patch

[Myles C. Maxfield]

Created attachment 259719 [details]
Patch

[Darin Adler]

Comment on attachment 259719 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259719&action=review

> Source/WebCore/css/CSSFontFace.cpp:101
> +    for (CSSSegmentedFontFace* face : m_segmentedFontFaces)

When I write a like like this, I write auto* because I don’t feel the need to recite the class name to make the one line of code below readable and auto is much shorter and can never accidentally widen a type to a base class pointer.

> Source/WebCore/css/CSSFontSelector.cpp:93
> +            return Optional<FontTraitsMask>();

Instead I think this can and should be:

    return Nullopt;

Could also possibly use:

    return { };

> Source/WebCore/css/CSSFontSelector.cpp:187
> +    if (RuntimeEnabledFeatures::sharedFeatures().fontLoadEventsEnabled())
> +        rule = static_pointer_cast<CSSFontFaceRule>(fontFaceRule->createCSSOMWrapper());

I would not expect this to compile. There is no local variable with the type fontFaceRule in scope here.

I know you just moved this, but seems really awkward to have to cast to CSSFontFaceRule. Can we override createCSSOMWrapper in StyleRuleFontFace to narrow the type there rather than here at the call site?

> Source/WebCore/css/CSSFontSelector.cpp:236
> +        return String();

I like to use { } instead of String() here in new code. What do you think?

> Source/WebCore/css/CSSFontSelector.cpp:263
> +    RefPtr<CSSValue> fontFamily = style.getPropertyCSSValue(CSSPropertyFontFamily);
> +    RefPtr<CSSValue> src = style.getPropertyCSSValue(CSSPropertySrc);
> +    RefPtr<CSSValue> unicodeRange = style.getPropertyCSSValue(CSSPropertyUnicodeRange);

I’m not sure why getPropertyCSSValue returns a PassRefPtr; it’s returning internal state, not creating a new object. Using smart pointers here results in unnecessary reference count churn and extra "get" calls and such.

> Source/WebCore/css/CSSFontSelector.cpp:281
> +    FontTraitsMask traitsMask;
> +    if (Optional<FontTraitsMask> computedTraitsMask = computeTraitsMask(style))
> +        traitsMask = computedTraitsMask.value();
> +    else
> +        return;

I’d suggest using auto here.

I also think the early return is easier to read, even though it doesn’t scope the computed traits mask local as tightly.

auto computedTraitsMask = computeTraitsMask(style);
if (!computedTraitsMask)
    return;
auto traitsMask = computedTraitsMask.value();

> Source/WebCore/css/CSSFontSelector.cpp:300
> +        auto& familyFontFaces = m_fontFaces.add(familyName, nullptr).iterator->value;

Why do we use unique_ptr<Vector> instead of just Vector in this map? Is it really more efficient to do that?

> Source/WebCore/css/CSSFontSelector.cpp:304
>              ASSERT(!m_locallyInstalledFontFaces.contains(familyName));

Can all the code in scope in this if statement go into a function? It really seems to distract from the main purpose of the addFontFaceRule function, and I don’t think it even uses anything except "this" and "familyName".

> Source/WebCore/css/CSSFontSelector.cpp:306
> +            Vector<FontTraitsMask> locallyInstalledFontsTraitsMasks = FontCache::singleton().getTraitsInFamily(familyName);

Given its narrow scope, I suggest just calling this “traitsMasks”.

> Source/WebCore/css/CSSFontSelector.cpp:308
> +                auto familyLocallyInstalledFaces = std::make_unique<Vector<Ref<CSSFontFace>>>();

I don’t think we need the word family in this local variable. Frankly, given its narrow scope, I might just suggest calling it “faces”.

> Source/WebCore/css/CSSFontSelector.cpp:309
> +                for (FontTraitsMask mask : locallyInstalledFontsTraitsMasks) {

auto here?

> Source/WebCore/css/CSSFontSelector.cpp:310
> +                    Ref<CSSFontFace> locallyInstalledFontFace = CSSFontFace::create(mask, nullptr, true);

auto here?

Given its narrow scope, I suggest just calling this local variable “face”.

> Source/WebCore/css/CSSFontSelector.cpp:315
>                  m_locallyInstalledFontFaces.set(familyName, WTF::move(familyLocallyInstalledFaces));

Should use add here rather than set.

> Source/WebCore/css/CSSFontSelector.cpp:481
> +    Vector<Ref<CSSFontFace>>* familyFontFaces = m_fontFaces.get(family);

I suggest using auto* here.

> Source/WebCore/css/CSSFontSelector.cpp:512
> +        if (Vector<Ref<CSSFontFace>>* familyLocallyInstalledFontFaces = m_locallyInstalledFontFaces.get(family)) {

I suggest auto* here. Also, given the narrow scope, I would just call this “faces”.

> Source/WebCore/css/CSSFontSelector.cpp:525
> +        for (auto& candidateFontFace : candidateFontFaces)

Given the narrow scope, I suggest just calling this “face”.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:248
> +        return Vector<FontTraitsMask>();

I like to use { } here instead of restating the type.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:252
> +        return Vector<FontTraitsMask>();

Ditto.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:254
> +    Vector<FontTraitsMask> traitsMasks;

This will be slightly more efficient if we use reserveInitialCapacity and uncheckedAppend.

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:142
> +    return Vector<FontTraitsMask>();

return { };

> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:561
> +    Vector<FontTraitsMask> result;
> +    for (unsigned mask : procData.m_traitsMasks)
> +        result.append(static_cast<FontTraitsMask>(mask));
> +    return result;

This would be slightly more efficient if we used reserveInitialCapacity and uncheckedAppend. That’s one benefit of the copyToVector template function and makes me wish we had one that like for creating a new vector.

[Myles C. Maxfield]

Comment on attachment 259719 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259719&action=review

>> Source/WebCore/css/CSSFontSelector.cpp:187
>> +        rule = static_pointer_cast<CSSFontFaceRule>(fontFaceRule->createCSSOMWrapper());
> 
> I would not expect this to compile. There is no local variable with the type fontFaceRule in scope here.
> 
> I know you just moved this, but seems really awkward to have to cast to CSSFontFaceRule. Can we override createCSSOMWrapper in StyleRuleFontFace to narrow the type there rather than here at the call site?

FONT_LOAD_EVENTS are off :|

>> Source/WebCore/css/CSSFontSelector.cpp:236
>> +        return String();
> 
> I like to use { } instead of String() here in new code. What do you think?

I think in some cases it is valuable to restate the return type (if the code is sufficiently complicated). In this case, I don't think it is, so I prefer { }.

>> Source/WebCore/css/CSSFontSelector.cpp:263
>> +    RefPtr<CSSValue> unicodeRange = style.getPropertyCSSValue(CSSPropertyUnicodeRange);
> 
> I’m not sure why getPropertyCSSValue returns a PassRefPtr; it’s returning internal state, not creating a new object. Using smart pointers here results in unnecessary reference count churn and extra "get" calls and such.

This is a good idea for a follow up patch.

[Myles C. Maxfield]

Created attachment 259750 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 259719 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259719&action=review

>> Source/WebCore/css/CSSFontSelector.cpp:300
>> +        auto& familyFontFaces = m_fontFaces.add(familyName, nullptr).iterator->value;
> 
> Why do we use unique_ptr<Vector> instead of just Vector in this map? Is it really more efficient to do that?

I suspect it's because the author wanted to use HashMap::get() instead of HashMap::find(). Instead, we want to use find().

[Myles C. Maxfield]

Created attachment 259751 [details]
Patch for committing

[Myles C. Maxfield]

Committed r188853: <http://trac.webkit.org/changeset/188853>

[Myles C. Maxfield]

Comment on attachment 259719 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259719&action=review

>>> Source/WebCore/css/CSSFontSelector.cpp:263
>>> +    RefPtr<CSSValue> unicodeRange = style.getPropertyCSSValue(CSSPropertyUnicodeRange);
>> 
>> I’m not sure why getPropertyCSSValue returns a PassRefPtr; it’s returning internal state, not creating a new object. Using smart pointers here results in unnecessary reference count churn and extra "get" calls and such.
> 
> This is a good idea for a follow up patch.

https://bugs.webkit.org/show_bug.cgi?id=148374