Bug 153347 – [Font Loading] Split CSSFontSelector into a FontFaceSet implementation and the rest of the class

• Component: Text
• Status: RESOLVED FIXED

[Myles C. Maxfield]

FontFaceSet concepts need to be separately implementable by other classes, but CSSFontSelector needs to implement this too.

[Myles C. Maxfield]

Created attachment 271731 [details]
WIP

[Myles C. Maxfield]

There are some tasks which need to be done:

1. Any changes to the Document's CSSFontFaceSet (adding/removing a FontFace, or changing a property of a constituent CSSFontFace) should cause a relayout
2. Modifying a CSS-Connected FontFace must be visible in the CSSOM and vice-versa
3. The invariants inside CSSFontFaceSet need to continue to hold, even when properties of constituent CSSFontFaces change.
4. Removing a CSS-Connect FontFace from the Document's FontFaceSet must make the FontFace un-CSS-Connected
5. CSSFontFaceSet::m_faces and m_cssConnectedFaces can be unified
6. CSS-Connected CSSFontFaceSets and CSSFontFaces currently have a null wrapper. These wrappers need to be lazily created.
7. There may be a state tracking problem because the Document's CSSFontFaceSet gets created before its wrapper.
8. CSSFontSelector's versioning needs to still work
9. CSSFontSelector and its CSSFontFaceSet need to live for the entire lifetime of the Document.

[Myles C. Maxfield]

Created attachment 271826 [details]
WIP

[Myles C. Maxfield]

(In reply to comment #2)
> There are some tasks which need to be done:
> 
> 1. Any changes to the Document's CSSFontFaceSet (adding/removing a FontFace,
> or changing a property of a constituent CSSFontFace) should cause a relayout
> 2. Modifying a CSS-Connected FontFace must be visible in the CSSOM and
> vice-versa
> 3. The invariants inside CSSFontFaceSet need to continue to hold, even when
> properties of constituent CSSFontFaces change.
> 4. Removing a CSS-Connect FontFace from the Document's FontFaceSet must make
> the FontFace un-CSS-Connected
> 5. CSSFontFaceSet::m_faces and m_cssConnectedFaces can be unified
> 6. CSS-Connected CSSFontFaceSets and CSSFontFaces currently have a null
> wrapper. These wrappers need to be lazily created.
> 7. There may be a state tracking problem because the Document's
> CSSFontFaceSet gets created before its wrapper.
> 8. CSSFontSelector's versioning needs to still work
> 9. CSSFontSelector and its CSSFontFaceSet need to live for the entire
> lifetime of the Document.

In this patch I have tackled:
> 5. CSSFontFaceSet::m_faces and m_cssConnectedFaces can be unified
> 6. CSS-Connected CSSFontFaceSets and CSSFontFaces currently have a null
> wrapper. These wrappers need to be lazily created.
> 7. There may be a state tracking problem because the Document's
> CSSFontFaceSet gets created before its wrapper.

I still need to tackle in this patch:
> 1. Any changes to the Document's CSSFontFaceSet (adding/removing a FontFace,
> or changing a property of a constituent CSSFontFace) should cause a relayout
> 3. The invariants inside CSSFontFaceSet need to continue to hold, even when
> properties of constituent CSSFontFaces change.
> 4. Removing a CSS-Connect FontFace from the Document's FontFaceSet must make
> the FontFace un-CSS-Connected
> 8. CSSFontSelector's versioning needs to still work

Out of scope for this patch:
> 9. CSSFontSelector and its CSSFontFaceSet need to live for the entire
> lifetime of the Document.
> 2. Modifying a CSS-Connected FontFace must be visible in the CSSOM and
> vice-versa

[Myles C. Maxfield]

Created attachment 271867 [details]
WIP

[Myles C. Maxfield]

Created attachment 271894 [details]
WIP

[Myles C. Maxfield]

Created attachment 271895 [details]
WIP

[Myles C. Maxfield]

Created attachment 271898 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 271898 [details]
Patch

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

> Source/WebCore/ChangeLog:29
> +

I should also mention the problem of the CSSFontSelector being destroyed.

[Myles C. Maxfield]

Created attachment 271900 [details]
Patch

[Antti Koivisto]

Comment on attachment 271900 [details]
Patch

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

> Source/WebCore/css/CSSFontFace.cpp:57
> +        SVGFontFaceElement* fontFaceElement = nullptr;

Shouldn't this be inside ENABLE(SVG_FONTS)?

Why is this stuff around? :)

> Source/WebCore/css/CSSFontFace.h:90
> +    static Optional<FontTraitsMask> calculateStyle(CSSValue& style);
> +    static Optional<FontTraitsMask> calculateWeight(CSSValue& weight);

calculateStyle/WeightMask?

> Source/WebCore/css/CSSFontFace.h:149
> +    // We don't guarantee that the FontFace wrapper will be the same every time you ask for it.
> +    Ref<FontFace> wrapper(JSC::ExecState&);

With many APIs we go to some lengths to ensure that it is the same wrapper. Wonder what other implementations do here.

> Source/WebCore/css/CSSFontFaceSet.cpp:106
> +    Vector<Ref<CSSFontFace>> faces = { };

No need to to initialize with { }

> Source/WebCore/css/CSSFontFaceSet.cpp:110
> +        RefPtr<CSSValueList> familyList = CSSValueList::createCommaSeparated();

Ref<> (or auto)

> Source/WebCore/css/CSSFontFaceSet.cpp:151
> +void CSSFontFaceSet::addToFacesLookupTable(CSSFontFace& face)
> +{
> +    if (face.families()) {
> +        for (auto& item : *face.families()) {

Please use early return instead of building a tower.

> Source/WebCore/css/CSSFontFaceSet.cpp:335
> +class FontFaceComparator {
> +public:

I realize you are just moving this but a lambda would be more modern.

[Myles C. Maxfield]

Comment on attachment 271900 [details]
Patch

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

>> Source/WebCore/css/CSSFontFace.cpp:57
>> +        SVGFontFaceElement* fontFaceElement = nullptr;
> 
> Shouldn't this be inside ENABLE(SVG_FONTS)?
> 
> Why is this stuff around? :)

It only ever gets set to non-null inside ENABLE(SVG_FONTS)

Yes, I am working on getting rid of it. Apparently getting a windows build which even vaguely matches what our Windows bots see is a 1week+ task

>> Source/WebCore/css/CSSFontFace.h:149
>> +    Ref<FontFace> wrapper(JSC::ExecState&);
> 
> With many APIs we go to some lengths to ensure that it is the same wrapper. Wonder what other implementations do here.

Yeah, this is due to the ownership model between CSSFontFace / FontFace / CSSFontFaceSet / FontFaceSet. In particular, if a site never touches this new API, I'd like to never create a FontFace / FontFaceSet (and only use the lower-level CSSFontFace / CSSFontFaceSet objects). However, if the site does use this new API, we want to lazily create these higher-level objects and associate them with the pre-existing CSSFontFace objects. However, this means that the ownership model is cyclic: CSSFontFace must retain its wrapping FontFace, but FontFace must retain its backing CSSFontFace.

This patch circumvents the problem by making the upwards-pointing reference a WeakPtr, and recreating the upper-level object if it got destroyed, but this has the problem of potentially making the identity of the FontFace change. However, this change is only visible if no references to the old object exist, which means simply running a === in Javascript won't show the problem. Instead, the only way to observe the problem would be to compare the FontFace's promise for equality (because recreating the FontFace means recreating its promise). I don't think this is a deal-breaker for this patch because the CSS Font Loading spec already has the concept of the identity of promises changing out from under you (FontFaceSet's "ready" promise is spec'ed to be reset to a new object every time loading starts, which could occur at arbitrary times).

There are two ways I can think of to solve this: 1) Use a registry to associate the higher-level & lower-level objects, so the references don't reside in the objects themselves, and 2) Manually convert between strong references and weak reference when detecting the above condition.

I think solving this problem is outside of the scope of this patch, but I definitely want to solve it.

>> Source/WebCore/css/CSSFontFaceSet.cpp:335
>> +public:
> 
> I realize you are just moving this but a lambda would be more modern.

I disagree in this case. operator() is 57 lines long, and it is only used in getFontFace() which is 39 lines long. Putting the comparator inside getFontFace() would make getFontFace() unnecessarily complicated and long.

[Antti Koivisto]

> I disagree in this case. operator() is 57 lines long, and it is only used in
> getFontFace() which is 39 lines long. Putting the comparator inside
> getFontFace() would make getFontFace() unnecessarily complicated and long.

You misunderstand. The idea would be to have a free-standing function (instead of a class) with all the parameters. You would invoke that function from a tiny lambda.

[Myles C. Maxfield]

Created attachment 271942 [details]
Patch for committing

[Myles C. Maxfield]

(In reply to comment #13)
> > I disagree in this case. operator() is 57 lines long, and it is only used in
> > getFontFace() which is 39 lines long. Putting the comparator inside
> > getFontFace() would make getFontFace() unnecessarily complicated and long.
> 
> You misunderstand. The idea would be to have a free-standing function
> (instead of a class) with all the parameters. You would invoke that function
> from a tiny lambda.

Oh, that's fine. I'll do that.

[Myles C. Maxfield]

Committed r196954: <http://trac.webkit.org/changeset/196954>