Bug 153347 - [Font Loading] Split CSSFontSelector into a FontFaceSet implementation and the rest of the class
Summary: [Font Loading] Split CSSFontSelector into a FontFaceSet implementation and th...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords:
Depends on:
Blocks: 153346
  Show dependency treegraph
 
Reported: 2016-01-22 01:14 PST by Myles C. Maxfield
Modified: 2016-02-22 13:39 PST (History)
6 users (show)

See Also:


Attachments
WIP (47.15 KB, patch)
2016-02-18 22:08 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (46.16 KB, patch)
2016-02-19 17:03 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (63.52 KB, patch)
2016-02-20 13:38 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (81.02 KB, patch)
2016-02-21 16:49 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (81.15 KB, patch)
2016-02-21 17:04 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (92.28 KB, patch)
2016-02-21 19:44 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (93.67 KB, patch)
2016-02-21 20:58 PST, Myles C. Maxfield
koivisto: review+
Details | Formatted Diff | Diff
Patch for committing (95.29 KB, patch)
2016-02-22 12:35 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2016-01-22 01:14:28 PST
FontFaceSet concepts need to be separately implementable by other classes, but CSSFontSelector needs to implement this too.
Comment 1 Myles C. Maxfield 2016-02-18 22:08:39 PST
Created attachment 271731 [details]
WIP
Comment 2 Myles C. Maxfield 2016-02-18 22:16:03 PST
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.
Comment 3 Myles C. Maxfield 2016-02-19 17:03:47 PST
Created attachment 271826 [details]
WIP
Comment 4 Myles C. Maxfield 2016-02-20 13:30:19 PST
(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
Comment 5 Myles C. Maxfield 2016-02-20 13:38:37 PST
Created attachment 271867 [details]
WIP
Comment 6 Myles C. Maxfield 2016-02-21 16:49:49 PST
Created attachment 271894 [details]
WIP
Comment 7 Myles C. Maxfield 2016-02-21 17:04:14 PST
Created attachment 271895 [details]
WIP
Comment 8 Myles C. Maxfield 2016-02-21 19:44:44 PST
Created attachment 271898 [details]
Patch
Comment 9 Myles C. Maxfield 2016-02-21 19:50:24 PST
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.
Comment 10 Myles C. Maxfield 2016-02-21 20:58:05 PST
Created attachment 271900 [details]
Patch
Comment 11 Antti Koivisto 2016-02-22 10:00:30 PST
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.
Comment 12 Myles C. Maxfield 2016-02-22 11:03:39 PST
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.
Comment 13 Antti Koivisto 2016-02-22 12:17:18 PST
> 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.
Comment 14 Myles C. Maxfield 2016-02-22 12:35:53 PST
Created attachment 271942 [details]
Patch for committing
Comment 15 Myles C. Maxfield 2016-02-22 12:38:57 PST
(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.
Comment 16 Myles C. Maxfield 2016-02-22 13:39:58 PST
Committed r196954: <http://trac.webkit.org/changeset/196954>