Bug 157694 – Support ArrayBufferViews in the CSS Font Loading API

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Support ArrayBufferViews in the CSS Font Loading API

[Myles C. Maxfield]

Created attachment 278894 [details]
WIP

[Myles C. Maxfield]

Created attachment 278902 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/25554267>

[Myles C. Maxfield]

Created attachment 278909 [details]
Patch

[Darin Adler]

Comment on attachment 278909 [details]
Patch

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

> Source/WebCore/css/CSSFontFaceSource.cpp:71
> +CSSFontFaceSource::CSSFontFaceSource(CSSFontFace& owner, const String& familyNameOrURI, CachedFont* font, SVGFontFaceElement* fontFace, RefPtr<JSC::ArrayBufferView> arrayBufferView)

This should be RefPtr&&; that’s the correct idiom for something that a function takes ownership of with WTFMove.

> Source/WebCore/css/CSSFontFaceSource.cpp:144
> +                RefPtr<SharedBuffer> buffer = SharedBuffer::create(static_cast<const char*>(m_immediateSource->baseAddress()), m_immediateSource->byteLength());
> +                ASSERT(buffer);

This construction is wasteful. This SharedBuffer::create function returns a Ref, not a RefPtr, and we should use auto to accommodate that. And Ref explicitly states that the value is never null; it would obviate the need for an assertion.

> Source/WebCore/css/FontFace.cpp:44
> +    bool immediateData = false;

What does the term "immediate data" mean? I guess it means local data that does not require loading. Is there a more precise term for this? Maybe dataRequiresAsynchronousLoading, initialized to true?

> Source/WebCore/css/FontFace.cpp:59
> +        std::unique_ptr<CSSFontFaceSource> source = std::make_unique<CSSFontFaceSource>(result->backing(), String(), nullptr, nullptr, arrayBufferView);

This line is super-long and would be shorter and less repetitive with auto and { }:

    auto source = std::make_unique<CSSFontFaceSource>(result->backing(), { }, nullptr, nullptr, arrayBufferView);

> Source/WebCore/css/FontFace.cpp:63
> +    } else if (auto arrayBuffer = toArrayBuffer(source)) {
> +        auto arrayBufferView = JSC::Uint8Array::create(arrayBuffer, 0, arrayBuffer->byteLength());

I suggest making a helper function that combines the toArrayBufferView and toArrayBuffer cases and returns a Ref<ArrayBufferView> so don’t need to repeat code.

Another possibility would be a helper function that makes the CSSFontFaceSource, which could help us avoid the local variable and the WTFMove.

> Source/WebCore/css/FontFace.cpp:64
> +        std::unique_ptr<CSSFontFaceSource> source = std::make_unique<CSSFontFaceSource>(result->backing(), String(), nullptr, nullptr, arrayBufferView);

This line is super-long and would be shorter and less repetitive with auto and { }:

    auto source = std::make_unique<CSSFontFaceSource>(result->backing(), { }, nullptr, nullptr, arrayBufferView);

> Source/WebCore/loader/cache/CachedFont.cpp:118
> +#if !PLATFORM(COCOA)
> +    if (isWOFF(buffer.get())) {
> +        Vector<char> convertedFont;
> +        if (!convertWOFFToSfnt(buffer.get(), convertedFont))
> +            buffer = nullptr;
> +        else
> +            buffer = SharedBuffer::adoptVector(convertedFont);
> +        wrapping = false;
> +    } else
> +#endif
> +        wrapping = true;

This construction with an else that goes across #endif is not great. I think we should just set wrapping to true before the #if, and then set it to false inside the if statement body and not worry about the dead store.

It’s strange that isWOFF takes a SharedBuffer* instead of SharedBuffer&.

[Darin Adler]

Comment on attachment 278909 [details]
Patch

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

>> Source/WebCore/css/FontFace.cpp:64
>> +        std::unique_ptr<CSSFontFaceSource> source = std::make_unique<CSSFontFaceSource>(result->backing(), String(), nullptr, nullptr, arrayBufferView);
> 
> This line is super-long and would be shorter and less repetitive with auto and { }:
> 
>     auto source = std::make_unique<CSSFontFaceSource>(result->backing(), { }, nullptr, nullptr, arrayBufferView);

I also think this should be WTFMove(arrayBufferView) to avoid one round of reference count churn.

[Myles C. Maxfield]

Comment on attachment 278909 [details]
Patch

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

>> Source/WebCore/css/CSSFontFaceSource.cpp:144
>> +                ASSERT(buffer);
> 
> This construction is wasteful. This SharedBuffer::create function returns a Ref, not a RefPtr, and we should use auto to accommodate that. And Ref explicitly states that the value is never null; it would obviate the need for an assertion.

Unfortunately, it currently returns a PassRefPtr :(

I'd like to fix that, but think that's outside the scope of this patch.

>> Source/WebCore/css/FontFace.cpp:44
>> +    bool immediateData = false;
> 
> What does the term "immediate data" mean? I guess it means local data that does not require loading. Is there a more precise term for this? Maybe dataRequiresAsynchronousLoading, initialized to true?

I was going for the sense of "immediate" that is used in assembly instructions, meaning that the data is right here and you don't have to do anything to get to it. I'm happy with dataRequiresAsynchronousLoading too.

>> Source/WebCore/css/FontFace.cpp:59
>> +        std::unique_ptr<CSSFontFaceSource> source = std::make_unique<CSSFontFaceSource>(result->backing(), String(), nullptr, nullptr, arrayBufferView);
> 
> This line is super-long and would be shorter and less repetitive with auto and { }:
> 
>     auto source = std::make_unique<CSSFontFaceSource>(result->backing(), { }, nullptr, nullptr, arrayBufferView);

It looks like the { } causes a compilation failure.

[Myles C. Maxfield]

Created attachment 278935 [details]
Patch for committing

[WebKit Commit Bot]

Attachment 278935 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/cache/CachedFont.cpp:107:  'buffer' is incorrectly named. It should be named 'protector' or 'protectedBytes'.  [readability/naming/protected] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 278935 [details]
Patch for committing

Clearing flags on attachment: 278935

Committed r200921: <http://trac.webkit.org/changeset/200921>

[Brady Eidson]

(In reply to comment #9)
> Attachment 278935 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebCore/loader/cache/CachedFont.cpp:107:  'buffer' is
> incorrectly named. It should be named 'protector' or 'protectedBytes'. 
> [readability/naming/protected] [4]
> Total errors found: 1 in 11 files


While the intention of this RefPtr was not to be a protector, and therefore the style checker incorrectly flagged it...  the RefPtr is actually a misuse of RefPtr because it is entirely unnecessary.

I'm considering ways to detect RefPtr misuse, though as long as we style check line-by-line that may be difficult.

[Myles C. Maxfield]

*** Bug 154624 has been marked as a duplicate of this bug. ***