Bug 153414

Summary: Decouple font creation from font loading
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, dino, jonlee, koivisto, simon.fraser, thorton
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 154104    
Bug Blocks: 153346    
Attachments:
Description Flags
WIP
none
Needs Windows SVG Font Implementation
none
Needs Windows SVG Font Implementation
none
Needs Windows SVG Font Implementation
none
Archive of layout-test-results from ews105 for mac-yosemite-wk2
none
Archive of layout-test-results from ews117 for mac-yosemite
none
Archive of layout-test-results from ews100 for mac-yosemite
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews114 for mac-yosemite
none
Archive of layout-test-results from ews107 for mac-yosemite-wk2
none
Archive of layout-test-results from ews101 for mac-yosemite
none
Patch
none
Patch
none
Patch darin: review+

Description Myles C. Maxfield 2016-01-24 19:14:05 PST
Modularize CSSFontFaceSource
Comment 1 Myles C. Maxfield 2016-01-24 19:15:11 PST
Created attachment 269717 [details]
WIP
Comment 2 Myles C. Maxfield 2016-01-25 17:14:12 PST
Created attachment 269820 [details]
Needs Windows SVG Font Implementation
Comment 3 Myles C. Maxfield 2016-01-25 17:39:38 PST
Created attachment 269827 [details]
Needs Windows SVG Font Implementation
Comment 4 Myles C. Maxfield 2016-01-25 17:48:27 PST
Created attachment 269829 [details]
Needs Windows SVG Font Implementation
Comment 5 Build Bot 2016-01-25 18:44:55 PST
Comment on attachment 269829 [details]
Needs Windows SVG Font Implementation

Attachment 269829 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/739131

New failing tests:
platform/mac/svg/fonts/svg-font-general.html
svg/custom/svg-fonts-fallback.xhtml
Comment 6 Build Bot 2016-01-25 18:44:58 PST
Created attachment 269837 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 7 Build Bot 2016-01-25 18:50:25 PST
Comment on attachment 269829 [details]
Needs Windows SVG Font Implementation

Attachment 269829 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/739138

New failing tests:
platform/mac/svg/fonts/svg-font-general.html
Comment 8 Build Bot 2016-01-25 18:50:28 PST
Created attachment 269838 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 9 Build Bot 2016-01-25 19:37:38 PST
Comment on attachment 269829 [details]
Needs Windows SVG Font Implementation

Attachment 269829 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/739292

New failing tests:
platform/mac/svg/fonts/svg-font-general.html
Comment 10 Build Bot 2016-01-25 19:37:40 PST
Created attachment 269845 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 11 Myles C. Maxfield 2016-01-26 00:27:40 PST
Created attachment 269861 [details]
Patch
Comment 12 Myles C. Maxfield 2016-01-26 14:28:28 PST
Created attachment 269923 [details]
Patch
Comment 13 Myles C. Maxfield 2016-01-26 17:48:35 PST
Created attachment 269959 [details]
Patch
Comment 14 Myles C. Maxfield 2016-01-26 17:59:16 PST
Created attachment 269963 [details]
Patch
Comment 15 Build Bot 2016-01-26 18:58:29 PST
Comment on attachment 269963 [details]
Patch

Attachment 269963 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/743634

Number of test failures exceeded the failure limit.
Comment 16 Build Bot 2016-01-26 18:58:33 PST
Created attachment 269970 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 17 Build Bot 2016-01-26 19:00:00 PST
Comment on attachment 269963 [details]
Patch

Attachment 269963 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/743676

Number of test failures exceeded the failure limit.
Comment 18 Build Bot 2016-01-26 19:00:04 PST
Created attachment 269971 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 19 Build Bot 2016-01-26 19:26:55 PST
Comment on attachment 269963 [details]
Patch

Attachment 269963 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/743771

Number of test failures exceeded the failure limit.
Comment 20 Build Bot 2016-01-26 19:26:58 PST
Created attachment 269976 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 21 Myles C. Maxfield 2016-01-27 01:15:30 PST
I can't reproduce these test failures on my local machine .... retrying patch now.
Comment 22 Myles C. Maxfield 2016-01-27 01:20:17 PST
Created attachment 269992 [details]
Patch
Comment 23 Myles C. Maxfield 2016-01-27 01:43:51 PST
Created attachment 269993 [details]
Patch
Comment 24 Antti Koivisto 2016-01-27 17:50:43 PST
Comment on attachment 269993 [details]
Patch

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

> Source/WebCore/ChangeLog:42
> +        - FontFaceSource
> +          - LocalFontFaceSource
> +          - InDocumentSVGFontFaceSource (when the SVG converter is not enabled; otherwise, this class does not exist)
> +          - ByteBasedFontFaceSource
> +            - ImmediateFontFaceSource
> +            - InDocumentSVGFontFaceSource (when the SVG converter is enabled; otherwise, this class does not exist)
> +            - RemoteFontFaceSource
> +              - DeprecatedRemoteSVGFontFaceSource (Only used when the SVG converter is not enabled; otherwise,
> +                    ByteBasedFontFaceSource handles this case)

I'm not sure I understand this class hierarchy and purpose of each type. Could you explain it? 

ImmediateFontFaceSource doesn't seem to be instantiated by this patch. It should probably be part of a patch that does instantiate it.

Two different implementation of InDocumentSVGFontFaceSource is confusing. If we need them both, could they have different names? ("DeprecatedInDocumentSVGFontFaceSource")
Comment 25 Antti Koivisto 2016-01-27 20:28:02 PST
Comment on attachment 269993 [details]
Patch

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

r- based on my confusion over what is going on in ByteBasedFontFaceSource::bufferProvided

> Source/WebCore/css/ByteBasedFontFaceSource.cpp:109
> +void ByteBasedFontFaceSource::bufferProvided(JSC::ArrayBuffer& arrayBuffer)
> +{
> +    RefPtr<SharedBuffer> buffer = SharedBuffer::create(static_cast<const char*>(arrayBuffer.data()), arrayBuffer.byteLength());
> +    if (!buffer) {
> +        setState(State::Failed);
> +        return;
> +    }
> +
> +    bufferProvided(*buffer);
> +}

This is not needed without ImmediateFontFaceSource.

> Source/WebCore/css/ByteBasedFontFaceSource.cpp:114
> +    bool wrapping = true;

I don't know what this variable means.

> Source/WebCore/css/ByteBasedFontFaceSource.cpp:115
> +    RefPtr<SharedBuffer> active = &buffer;

activeBuffer

> Source/WebCore/css/ByteBasedFontFaceSource.cpp:121
> +        switch (convertWOFF(buffer, receiver)) {
> +        case ConversionStatus::Unnecessary:

It might be nicer to call conversion function only after first testing isWOFF(). Then there wouldn't be need for strange "Unnecessary" return value.

> Source/WebCore/css/ByteBasedFontFaceSource.cpp:126
> +        case ConversionStatus::Success:
> +            wrapping = false;
> +            active = SharedBuffer::adoptVector(receiver);
> +            break;

So here we have successful WOFF conversion...

> Source/WebCore/css/ByteBasedFontFaceSource.cpp:133
> +        // Step 2: Try to convert SVG to OTF.
> +        switch (convertSVGFont(*active, receiver)) {

...and then we try to convert the result as SVG font. Why?

It seems strange that we try to convert any non-WOFF case as SVG, including feeding it to SVG parser. Shouldn't this only happen with known SVG fonts?

> Source/WebCore/css/ByteBasedFontFaceSource.h:42
> +class ByteBasedFontFaceSource : public FontFaceSource {

ByteBasedFontFaceSource is not a great name. I don't have specific suggestions though.

> Source/WebCore/css/ByteBasedFontFaceSource.h:44
> +public:
> +    ByteBasedFontFaceSource(CSSFontFace& owner, const AtomicString& remoteURI = AtomicString(), bool performConversion = true);

Constructor can be protected.
Bool should be an enum.

> Source/WebCore/css/ByteBasedFontFaceSource.h:50
> +    virtual bool bufferProvided(SharedBuffer&);

It is completely unclear what the bool return value means. It should be replaced with an enum.

I think only reason this is virtual is DeprecatedRemoteSVGFontFaceSource.

> Source/WebCore/css/CSSFontFace.cpp:89
> +    for (size_t index = 0; index < m_sources.size(); ) {
> +        switch (m_sources[index]->state()) {
> +        case FontFaceSource::State::Pending:
> +            m_sources[index]->load();
> +            break;

Maybe you could test for Pending before the switch and have ASSERT_NOT_REACHED for it inside the switch (as it is legal only as the first transition). Then you could use range-for and wouldn't need to increment index manually.

> Source/WebCore/css/DeprecatedInDocumentSVGFontFaceSource.h:37
> +#define DeprecatedInDocumentSVGFontFaceSource_h
> +
> +#include "FontFaceSource.h"
> +
> +#if !ENABLE(SVG_OTF_CONVERTER)
> +
> +namespace WebCore {
> +
> +class SVGFontFaceElement;
> +
> +class InDocumentSVGFontFaceSource final : public FontFaceSource {

Class name and file name should match.

> Source/WebCore/css/DeprecatedRemoteSVGFontFaceSource.cpp:41
> +DeprecatedRemoteSVGFontFaceSource::DeprecatedRemoteSVGFontFaceSource(CSSFontFace& owner, CSSFontSelector& fontSelector, CachedFont& cachedFont, const AtomicString& remoteURI)

Can we just eliminate this class and have ENABLE(SVG_OTF_CONVERTER) ifdef within ByteBasedFontFaceSource? It seems to me that this would simplify things.

> Source/WebCore/css/FontFaceSource.h:44
> +    FontFaceSource(CSSFontFace& owner);

Constructor could be protected.

> Source/WebCore/css/FontFaceSource.h:51
> +    //                      => Succeeded
> +    //                    //
> +    // Pending => Loading
> +    //                    \\.
> +    //                      => Failed

setState should assert that only these transitions occur. Or perhaps remove setState and have function for these specific transitions only (like current load()).

> Source/WebCore/css/FontFaceSource.h:66
> +    RefPtr<Font> font(const FontDescription&, bool syntheticBold, bool syntheticItalic, const FontFeatureSettings& fontFaceFeatureSettings, const FontVariantSettings& fontFaceVariantSettings);

Those bools should be enums.
FontFeatureSettings/FontVariantSettings don't need a variable name in the header.

> Source/WebCore/css/FontFaceSource.h:107
> +    virtual bool shouldCache() const { return true; }

This is not dynamic. It could be passed as constructor parameter instead.

> Source/WebCore/css/ImmediateFontFaceSource.h:33
> +class ImmediateFontFaceSource final : public ByteBasedFontFaceSource {

Should add this in another patch.
Comment 26 Myles C. Maxfield 2016-01-27 21:21:14 PST
(In reply to comment #24)
> Comment on attachment 269993 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269993&action=review
> 
> > Source/WebCore/ChangeLog:42
> > +        - FontFaceSource
> > +          - LocalFontFaceSource
> > +          - InDocumentSVGFontFaceSource (when the SVG converter is not enabled; otherwise, this class does not exist)
> > +          - ByteBasedFontFaceSource
> > +            - ImmediateFontFaceSource
> > +            - InDocumentSVGFontFaceSource (when the SVG converter is enabled; otherwise, this class does not exist)
> > +            - RemoteFontFaceSource
> > +              - DeprecatedRemoteSVGFontFaceSource (Only used when the SVG converter is not enabled; otherwise,
> > +                    ByteBasedFontFaceSource handles this case)
> 
> I'm not sure I understand this class hierarchy and purpose of each type.
> Could you explain it? 

I think ... you're right. I think I've created a monster. In the effort to simplify things, I think I've made them more complicated.

I'm going to rework this patch.

> 
> ImmediateFontFaceSource doesn't seem to be instantiated by this patch. It
> should probably be part of a patch that does instantiate it.
> 
> Two different implementation of InDocumentSVGFontFaceSource is confusing. If
> we need them both, could they have different names?
> ("DeprecatedInDocumentSVGFontFaceSource")
Comment 27 Myles C. Maxfield 2016-02-08 17:00:13 PST
Created attachment 270896 [details]
Patch
Comment 28 Myles C. Maxfield 2016-02-08 17:00:43 PST
I've reined it in, and I think it's much better now.
Comment 29 Darin Adler 2016-02-09 09:37:53 PST
Comment on attachment 270896 [details]
Patch

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

> Source/WebCore/css/CSSFontFace.cpp:43
>      size_t size = m_sources.size();
>      for (size_t i = 0; i < size; i++) {

Should use a modern for loop.

> Source/WebCore/css/CSSFontFace.cpp:76
> +    CSSFontSelector& fontSelector = (*m_segmentedFontFaces.begin())->fontSelector();
> +    fontSelector.fontLoaded();

No reason to put this into a local variable. Should just be a one liner.

> Source/WebCore/css/CSSFontFace.cpp:94
> +    for (size_t i = 0; i < m_sources.size(); ) {

Really not fond of the new structure here, where we use the trick of not incrementing "i" to run the loop a second time when a font face is in "pending" state after we call load on it. Can we write this in a more straightforward way? Should be a modern for loop. All that we would need to do is to put the "pending" logic outside the switch statement instead of using a "retry" approach. Also makes it clearer that there is no infinite loop possible if we do it that way.

> Source/WebCore/css/CSSFontFace.cpp:101
> +#if !ASSERT_DISABLED
> +        ASSERT(!previousI || !previousStatus || i != previousI.value() || source->status() != previousStatus.value());
> +        previousI = i;
> +        previousStatus = source->status();
>  #endif

This assertion is super confusing! I assume this is all about that one peculiar case where we run the loop twice that I complained about above.

> Source/WebCore/css/CSSFontFace.cpp:106
> +        switch (source->status()) {
> +        case CSSFontFaceSource::Status::Pending:
> +            source->load(fontSelector);
> +            break;

Fixing this strangeness would be as simple as putting this before the switch:

    if (source->status() == CSSFontFaceSource::Status::Pending)
        source->load(fontSelector);

Then the switch can assert that the status is not pending below. And then we can get rid of all the loop crazy. Or we could change the definition of load so that it is guaranteed to quietly do nothing if the source is not pending and then it’s even cleaner, just:

    source->load(fontSelector);

> Source/WebCore/css/CSSFontFace.cpp:111
> +                return result.release();

Should be WTFMove, not release().

> Source/WebCore/css/CSSFontFaceSource.cpp:75
> +CSSFontFaceSource::CSSFontFaceSource(CSSFontFace& owner, const String& str, CachedFont* font, SVGFontFaceElement* fontFace)

Could we use a word instead of "str" please? Also really unclear what the string is given that vague type and name.

> Source/WebCore/css/CSSFontFaceSource.h:61
> +    CSSFontFaceSource(CSSFontFace& owner, const String&, CachedFont* = nullptr, SVGFontFaceElement* = nullptr);

Argument name "owner" doesn’t seem necessary. On the flip side, completely unclear what the string is for is without an argument name. The type doesn’t make it clear, and even the member name m_string is unacceptably vague.

> Source/WebCore/css/CSSFontFaceSource.h:65
> +    void setStatus(Status);

This should be private.

> Source/WebCore/loader/cache/CachedFont.cpp:76
> -        static_cast<CachedFontClient*>(c)->fontLoaded(this);
> +        static_cast<CachedFontClient*>(c)->fontLoaded(*this);

Since you are modifying this code anyway, how about a name instead of a letter for "c"?

> Source/WebCore/loader/cache/CachedFont.cpp:153
>      CachedResourceClientWalker<CachedFontClient> w(m_clients);
>      while (CachedFontClient* c = w.next())
> -         c->fontLoaded(this);
> +        c->fontLoaded(*this);

Since you are modifying this code anyway, how about names instead of letters for "w" and "c"?
Comment 30 Myles C. Maxfield 2016-02-09 11:50:11 PST
Committed r196322: <http://trac.webkit.org/changeset/196322>
Comment 31 Darin Adler 2016-02-10 09:17:23 PST
Comment on attachment 270896 [details]
Patch

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

> Source/WebCore/css/CSSFontFaceSource.cpp:150
> +    if (!m_svgFontFaceElement->parentNode() || !is<SVGFontElement>(m_svgFontFaceElement->parentNode()))

Just noticed this line of code has an extra null check. The is function after the || already checks for null so the expression before the "||" can just be deleted entirely.
Comment 32 Darin Adler 2016-02-10 09:18:30 PST
Comment on attachment 270896 [details]
Patch

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

> Source/WebCore/css/CSSFontFaceSource.h:59
> +    //                      => Succeeded
> +    //                    //
> +    // Pending => Loading
> +    //                    \\.
> +    //                      => Failed
> +    enum class Status {
> +        Pending,
> +        Loading,
> +        Success,
> +        Failure
> +    };

Just noticed that the diagram has a stray "." in it, and also uses different names from the enum values.
Comment 33 Myles C. Maxfield 2016-02-10 11:54:49 PST
(In reply to comment #32)
> Comment on attachment 270896 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270896&action=review
> 
> > Source/WebCore/css/CSSFontFaceSource.h:59
> > +    //                      => Succeeded
> > +    //                    //
> > +    // Pending => Loading
> > +    //                    \\.
> > +    //                      => Failed
> > +    enum class Status {
> > +        Pending,
> > +        Loading,
> > +        Success,
> > +        Failure
> > +    };
> 
> Just noticed that the diagram has a stray "." in it, and also uses different
> names from the enum values.

The period is because one of the ports interprets backslash-newline as a multi-line comment, and throws a compile error. This is the only way I know of to avoid that.

Your other comments are addressed in r196376.