[Font Loading] Implement FontFaceSet

• Component: Text
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Much of this implementation can perhaps be ported from the existing "FontLoader" class that's already in the tree.

[Myles C. Maxfield]

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

[Myles C. Maxfield]

Created attachment 270184 [details]
Applied on top of attachment 270144 [details]

[Myles C. Maxfield]

Created attachment 270290 [details]
Applied on top of attachment 270144 [details]

[Myles C. Maxfield]

Created attachment 270293 [details]
Applied on top of attachment 270144 [details]

[Myles C. Maxfield]

Created attachment 270415 [details]
Applied on top of attachment 270144 [details]

[Myles C. Maxfield]

We need to support iteration. Iteration must iterate in document-order, followed by non-CSS-connected fonts in insertion order.

[Myles C. Maxfield]

Adding a font that is CSS-Connected needs to throw a InvalidModificationError

[Myles C. Maxfield]

We need our loading/loaded status to match our constituent FontFaces. add() and remove() need to be updated to maintain this invariant.

[Myles C. Maxfield]

We need to implement event triggering.

[Myles C. Maxfield]

Created attachment 270455 [details]
Applied on top of attachment 270144 [details]

[Myles C. Maxfield]

Created attachment 270477 [details]
Applied on top of attachment 270478 [details]

[Myles C. Maxfield]

Created attachment 271512 [details]
WIP

[Myles C. Maxfield]

Created attachment 271522 [details]
WIP

[Myles C. Maxfield]

Created attachment 271529 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 271529 [details]
Patch

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

> LayoutTests/fast/text/font-face-set-javascript.html:24
> +var item = iterator.next();

I should test that fonts are iterated in insertion order.

[Myles C. Maxfield]

Comment on attachment 271529 [details]
Patch

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

> Source/WebCore/css/FontFaceSet.cpp:135
> +        if (face.get().status() == CSSFontFace::Status::Failure) {

This failure check should occur before we add anything to m_pendingPromises.

[Simon Fraser (smfr)]

Comment on attachment 271529 [details]
Patch

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

> Source/WebCore/css/CSSFontFace.h:2
> - * Copyright (C) 2007, 2008 Apple Inc. All rights reserved.
> + * Copyright (C) 2016 Apple Inc. All rights reserved.

You should preserve the 2007,2008 dates.

> Source/WebCore/css/CSSFontFace.h:52
> +    static Ref<CSSFontFace> create(CSSFontSelector& fontSelector, FontFace* wrapper = nullptr, bool isLocalFallback = false) { return adoptRef(*new CSSFontFace(fontSelector, wrapper, isLocalFallback)); }

Should not all be on one line.

> Source/WebCore/css/CSSFontFace.h:96
> +        virtual void kick(CSSFontFace&) { };

Kick is not a very descriptive name. Please choose a new one.

> Source/WebCore/css/CSSFontFaceSet.cpp:88
> +    for (size_t i = 0; i < m_faces.size(); ++i) {

Use a modern loop?

> Source/WebCore/css/CSSFontFaceSet.cpp:97
> +    ASSERT_NOT_REACHED();

Is it really impossible to hit this, even with bad JS?

> Source/WebCore/css/CSSFontFaceSet.cpp:112
> +static bool familiesIntersect(CSSFontFace& face, const CSSValueList& request)

const CSSFontFace&

> Source/WebCore/css/CSSFontFaceSet.h:49
> +    bool has(const CSSFontFace&) const;

I would prefer hasFace()

> Source/WebCore/css/CSSFontFaceSet.h:50
> +    size_t size() const { return m_faces.size(); }

Never really happy with size() as a count, since it's ambiguous with byte size.

> Source/WebCore/css/CSSFontFaceSet.h:73
> +    Status m_status { Status::Loaded };

Seems odd that the initial state is Loaded?

> Source/WebCore/css/CSSSegmentedFontFace.cpp:58
> +void CSSSegmentedFontFace::kick(CSSFontFace&)

Donut know what "kick" means.

> Source/WebCore/css/FontFaceSet.h:57
> +    bool has(FontFace*) const;
> +    size_t size() const;

Ditto.

> Source/WebCore/css/FontFaceSet.h:88
> +    using RefCounted<FontFaceSet>::ref;
> +    using RefCounted<FontFaceSet>::deref;

Why do you need these?

[Chris Dumez]

Comment on attachment 271529 [details]
Patch

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

> Source/WebCore/css/CSSFontFaceSet.cpp:119
> +    for (auto &family1 : faceFamilies) {

auto&

> Source/WebCore/css/CSSFontFaceSet.cpp:165
> +    for (auto face : matchingFaces)

auto&?

> Source/WebCore/css/CSSFontFaceSet.cpp:175
> +    for (auto face : matchingFaces) {

auto&?

> Source/WebCore/css/CSSFontFaceSet.h:44
> +class CSSFontFaceSet : public CSSFontFace::Client {

final?

> Source/WebCore/css/FontFace.h:2
> + * Copyright (C) 2016 Apple Inc. All rights reserved.

should keep the old dates.

> Source/WebCore/css/FontFaceSet.cpp:123
> +    if (!matchingFaces.size()) {

matchingFaces.isEmpty() is a tad more readable.

> Source/WebCore/css/FontFaceSet.cpp:128
> +    for (auto face : matchingFaces)

auto&

> Source/WebCore/css/FontFaceSet.cpp:133
> +    for (auto face : matchingFaces) {

auto&

> Source/WebCore/css/FontFaceSet.cpp:142
> +        auto& vec = m_pendingPromises.add(RefPtr<FontFace>(face.get().wrapper()), Vector<Ref<PendingPromise>>()).iterator->value;

vector

> Source/WebCore/css/FontFaceSet.h:46
> +class FontFaceSet : public RefCounted<FontFaceSet>, public CSSFontFaceSetClient, public EventTargetWithInlineData, public ActiveDOMObject {

final?

[Myles C. Maxfield]

Comment on attachment 271529 [details]
Patch

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

>> Source/WebCore/css/CSSFontFaceSet.cpp:88
>> +    for (size_t i = 0; i < m_faces.size(); ++i) {
> 
> Use a modern loop?

I use "i" below. m_faces.remove(i);

>> Source/WebCore/css/CSSFontFaceSet.cpp:97
>> +    ASSERT_NOT_REACHED();
> 
> Is it really impossible to hit this, even with bad JS?

Yes, FontFaceSet is an intermediary between JavaScript and this function, and it will only call this function if CSSFontFaceSet::has().

Like Darin says: "No aspirational ASSERT()s!"

>> Source/WebCore/css/CSSFontFaceSet.cpp:165
>> +    for (auto face : matchingFaces)
> 
> auto&?

Wow, my testing didn't catch this because "face" is a std::reference_wrapper :|

>> Source/WebCore/css/CSSFontFaceSet.h:50
>> +    size_t size() const { return m_faces.size(); }
> 
> Never really happy with size() as a count, since it's ambiguous with byte size.

I think it's more valuable to be consistent with C++ conventions rather than (weirdly) use Obj-C conventions.

>> Source/WebCore/css/CSSFontFaceSet.h:73
>> +    Status m_status { Status::Loaded };
> 
> Seems odd that the initial state is Loaded?

The state is "loaded" iff there are 0 loads active. Otherwise, the state is "loading."

>> Source/WebCore/css/FontFaceSet.h:88
>> +    using RefCounted<FontFaceSet>::deref;
> 
> Why do you need these?

RefCounted has a ref() and deref() function, and EventTarget has a ref() and deref() function.

[Myles C. Maxfield]

Comment on attachment 271529 [details]
Patch

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

>>> Source/WebCore/css/CSSFontFaceSet.h:50
>>> +    size_t size() const { return m_faces.size(); }
>> 
>> Never really happy with size() as a count, since it's ambiguous with byte size.
> 
> I think it's more valuable to be consistent with C++ conventions rather than (weirdly) use Obj-C conventions.

What do you think I should name it?

[Simon Fraser (smfr)]

(In reply to comment #20)
> Comment on attachment 271529 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271529&action=review
> 
> >>> Source/WebCore/css/CSSFontFaceSet.h:50
> >>> +    size_t size() const { return m_faces.size(); }
> >> 
> >> Never really happy with size() as a count, since it's ambiguous with byte size.
> > 
> > I think it's more valuable to be consistent with C++ conventions rather than (weirdly) use Obj-C conventions.
> 
> What do you think I should name it?

faceCount? numberOfFaces()?

[Myles C. Maxfield]

Committed r196747: <http://trac.webkit.org/changeset/196747>