Bug 153348 - [Font Loading] Implement FontFaceSet
Summary: [Font Loading] Implement FontFaceSet
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:
: 153642 (view as bug list)
Depends on:
Blocks: 153346 153918 158884
  Show dependency treegraph
 
Reported: 2016-01-22 01:15 PST by Myles C. Maxfield
Modified: 2016-06-18 02:07 PDT (History)
8 users (show)

See Also:


Attachments
Applied on top of attachment 270144 (20.45 KB, patch)
2016-01-28 21:59 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Applied on top of attachment 270144 (43.98 KB, patch)
2016-01-29 21:29 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Applied on top of attachment 270144 (50.93 KB, patch)
2016-01-29 21:41 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Applied on top of attachment 270144 (51.00 KB, patch)
2016-02-01 13:04 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Applied on top of attachment 270144 (55.12 KB, patch)
2016-02-01 17:14 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Applied on top of attachment 270478 (55.62 KB, patch)
2016-02-02 01:08 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (71.90 KB, patch)
2016-02-16 17:32 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (81.62 KB, patch)
2016-02-16 19:57 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (91.67 KB, patch)
2016-02-16 21:10 PST, Myles C. Maxfield
simon.fraser: review+
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:15:40 PST
Much of this implementation can perhaps be ported from the existing "FontLoader" class that's already in the tree.
Comment 1 Myles C. Maxfield 2016-01-28 21:57:35 PST
*** Bug 153642 has been marked as a duplicate of this bug. ***
Comment 2 Myles C. Maxfield 2016-01-28 21:59:55 PST
Created attachment 270184 [details]
Applied on top of attachment 270144 [details]
Comment 3 Myles C. Maxfield 2016-01-29 21:29:34 PST
Created attachment 270290 [details]
Applied on top of attachment 270144 [details]
Comment 4 Myles C. Maxfield 2016-01-29 21:41:43 PST
Created attachment 270293 [details]
Applied on top of attachment 270144 [details]
Comment 5 Myles C. Maxfield 2016-02-01 13:04:59 PST
Created attachment 270415 [details]
Applied on top of attachment 270144 [details]
Comment 6 Myles C. Maxfield 2016-02-01 15:43:05 PST
We need to support iteration. Iteration must iterate in document-order, followed by non-CSS-connected fonts in insertion order.
Comment 7 Myles C. Maxfield 2016-02-01 15:44:00 PST
Adding a font that is CSS-Connected needs to throw a InvalidModificationError
Comment 8 Myles C. Maxfield 2016-02-01 15:46:04 PST
We need our loading/loaded status to match our constituent FontFaces. add() and remove() need to be updated to maintain this invariant.
Comment 9 Myles C. Maxfield 2016-02-01 15:47:55 PST
We need to implement event triggering.
Comment 10 Myles C. Maxfield 2016-02-01 17:14:22 PST
Created attachment 270455 [details]
Applied on top of attachment 270144 [details]
Comment 11 Myles C. Maxfield 2016-02-02 01:08:02 PST
Created attachment 270477 [details]
Applied on top of attachment 270478 [details]
Comment 12 Myles C. Maxfield 2016-02-16 17:32:30 PST
Created attachment 271512 [details]
WIP
Comment 13 Myles C. Maxfield 2016-02-16 19:57:30 PST
Created attachment 271522 [details]
WIP
Comment 14 Myles C. Maxfield 2016-02-16 21:10:05 PST
Created attachment 271529 [details]
Patch
Comment 15 Myles C. Maxfield 2016-02-16 21:13:26 PST
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.
Comment 16 Myles C. Maxfield 2016-02-17 10:09:02 PST
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.
Comment 17 Simon Fraser (smfr) 2016-02-17 15:57:09 PST
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?
Comment 18 Chris Dumez 2016-02-17 16:12:43 PST
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?
Comment 19 Myles C. Maxfield 2016-02-17 22:28:06 PST
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.
Comment 20 Myles C. Maxfield 2016-02-17 22:28:52 PST
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?
Comment 21 Simon Fraser (smfr) 2016-02-17 22:39:07 PST
(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()?
Comment 22 Myles C. Maxfield 2016-02-17 23:26:01 PST
Committed r196747: <http://trac.webkit.org/changeset/196747>