Changeset 201799 in webkit

Timestamp:

Jun 8, 2016 1:00:22 AM (8 years ago)

Author:

mmaxfield@apple.com

Message:

Extend CSSFontSelector's lifetime to be longer than the Document's lifetime
​https://bugs.webkit.org/show_bug.cgi?id=154101

Reviewed by Darin Adler.

Rather than destroying the Document's CSSFontSelector, instead, the object should
live for the lifetime of the document, and it should instead be asked to clear its
contents.

This is important for the CSS Font Loading API, where the identity of objects the
CSSFontSelector references needs to persist throughout the lifetime of the
Document. This patch represents the first step to implementing this correctly.
The second step is for the CSSFontSelector to perform a diff instead of a
wholesale clear of its contents. Once this is done, font loading objects can
survive through a call to Document::clearStyleResolver().

This patch gives the CSSFontSelector two states: building underway and building not
underway. The state is building underway in between calls to clearStyleResolver()
and when the style resolver gets built back up. Otherwise, the state is building
not underway. Because of this new design, creation of all FontFace objects can be
postponed until a state transition from building underway to building not underway.
A subsequent patch will perform the diff at this point. An ASSERT() makes sure that
we never service a font lookup request while Building.

No new tests because there is no behavior change.

• css/CSSFontFaceSet.cpp:

(WebCore::CSSFontFaceSet::clear):

• css/CSSFontSelector.cpp:

(WebCore::CSSFontSelector::buildStarted):
(WebCore::CSSFontSelector::buildCompleted):
(WebCore::CSSFontSelector::addFontFaceRule):
(WebCore::CSSFontSelector::fontRangesForFamily):
(WebCore::CSSFontSelector::CSSFontSelector): Deleted.
(WebCore::CSSFontSelector::clearDocument): Deleted.

• css/CSSFontSelector.h:
• css/StyleResolver.cpp:

(WebCore::StyleResolver::appendAuthorStyleSheets):

• dom/Document.cpp:

(WebCore::Document::Document):
(WebCore::Document::~Document):
(WebCore::Document::clearStyleResolver):
(WebCore::Document::fontSelector): Deleted.

• dom/Document.h:

(WebCore::Document::fontSelector):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• css/CSSFontFaceSet.cpp (modified) (1 diff)
• css/CSSFontSelector.cpp (modified) (4 diffs)
• css/CSSFontSelector.h (modified) (3 diffs)
• css/SourceSizeList.cpp (modified) (1 diff)
• css/StyleResolver.cpp (modified) (1 diff)
• dom/Document.cpp (modified) (5 diffs)
• dom/Document.h (modified) (2 diffs)
• style/StyleTreeResolver.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2016-06-08  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        Extend CSSFontSelector's lifetime to be longer than the Document's lifetime
+        https://bugs.webkit.org/show_bug.cgi?id=154101
+
+        Reviewed by Darin Adler.
+
+        Rather than destroying the Document's CSSFontSelector, instead, the object should
+        live for the lifetime of the document, and it should instead be asked to clear its
+        contents.
+
+        This is important for the CSS Font Loading API, where the identity of objects the
+        CSSFontSelector references needs to persist throughout the lifetime of the
+        Document. This patch represents the first step to implementing this correctly.
+        The second step is for the CSSFontSelector to perform a diff instead of a
+        wholesale clear of its contents. Once this is done, font loading objects can
+        survive through a call to Document::clearStyleResolver().
+
+        This patch gives the CSSFontSelector two states: building underway and building not
+        underway. The state is building underway in between calls to clearStyleResolver()
+        and when the style resolver gets built back up. Otherwise, the state is building
+        not underway. Because of this new design, creation of all FontFace objects can be
+        postponed until a state transition from building underway to building not underway.
+        A subsequent patch will perform the diff at this point. An ASSERT() makes sure that
+        we never service a font lookup request while Building.
+
+        No new tests because there is no behavior change.
+
+        * css/CSSFontFaceSet.cpp:
+        (WebCore::CSSFontFaceSet::clear):
+        * css/CSSFontSelector.cpp:
+        (WebCore::CSSFontSelector::buildStarted):
+        (WebCore::CSSFontSelector::buildCompleted):
+        (WebCore::CSSFontSelector::addFontFaceRule):
+        (WebCore::CSSFontSelector::fontRangesForFamily):
+        (WebCore::CSSFontSelector::CSSFontSelector): Deleted.
+        (WebCore::CSSFontSelector::clearDocument): Deleted.
+        * css/CSSFontSelector.h:
+        * css/StyleResolver.cpp:
+        (WebCore::StyleResolver::appendAuthorStyleSheets):
+        * dom/Document.cpp:
+        (WebCore::Document::Document):
+        (WebCore::Document::~Document):
+        (WebCore::Document::clearStyleResolver):
+        (WebCore::Document::fontSelector): Deleted.
+        * dom/Document.h:
+        (WebCore::Document::fontSelector):
+
 2016-06-08  Adam Bergkvist  <adam.bergkvist@ericsson.com>
 

trunk/Source/WebCore/css/CSSFontFaceSet.cpp

@@ -245 +245 @@
     m_locallyInstalledFacesLookupTable.clear();
     m_cache.clear();
+    m_facesPartitionIndex = 0;
+    m_status = Status::Loaded;
 }
 

trunk/Source/WebCore/css/CSSFontSelector.cpp

@@ -72 +72 @@
     , m_version(0)
 {
-    // FIXME: An old comment used to say there was no need to hold a reference to m_document
-    // because "we are guaranteed to be destroyed before the document". But there does not
-    // seem to be any such guarantee.
-
     ASSERT(m_document);
     FontCache::singleton().addClient(*this);
@@ -103 +99 @@
 }
 
+void CSSFontSelector::buildStarted()
+{
+    m_buildIsUnderway = true;
+    ++m_version;
+}
+
+void CSSFontSelector::buildCompleted()
+{
+    if (!m_buildIsUnderway)
+        return;
+
+    m_buildIsUnderway = false;
+
+    m_cssFontFaceSet->clear();
+
+    for (auto& item : m_stagingArea)
+        addFontFaceRule(item.styleRuleFontFace, item.isInitiatingElementInUserAgentShadowTree);
+    m_stagingArea.clear();
+}
+
 void CSSFontSelector::addFontFaceRule(StyleRuleFontFace& fontFaceRule, bool isInitiatingElementInUserAgentShadowTree)
 {
+    if (m_buildIsUnderway) {
+        m_stagingArea.append({fontFaceRule, isInitiatingElementInUserAgentShadowTree});
+        return;
+    }
+
     const StyleProperties& style = fontFaceRule.properties();
     RefPtr<CSSValue> fontFamily = style.getPropertyCSSValue(CSSPropertyFontFamily);
@@ -236 +257 @@
 FontRanges CSSFontSelector::fontRangesForFamily(const FontDescription& fontDescription, const AtomicString& familyName)
 {
+    ASSERT(!m_buildIsUnderway); // If this ASSERT() fires, it usually means you forgot a document.updateStyleIfNeeded() somewhere.
+
     // FIXME: The spec (and Firefox) says user specified generic families (sans-serif etc.) should be resolved before the @font-face lookup too.
     bool resolveGenericFamilyFirst = familyName == standardFamily;
@@ -269 +292 @@
     m_document = nullptr;
 
-    // FIXME: This object should outlive the Document.
     m_cssFontFaceSet->clear();
     m_clients.clear();

trunk/Source/WebCore/css/CSSFontSelector.h

@@ -66 +66 @@
 
     void clearDocument();
+    void buildStarted();
+    void buildCompleted();
 
     void addFontFaceRule(StyleRuleFontFace&, bool isInitiatingElementInUserAgentShadowTree);
@@ -92 +94 @@
     void beginLoadTimerFired();
 
+    struct PendingFontFaceRule {
+        StyleRuleFontFace& styleRuleFontFace;
+        bool isInitiatingElementInUserAgentShadowTree;
+    };
+    Vector<PendingFontFaceRule> m_stagingArea;
+
     Document* m_document;
     RefPtr<FontFaceSet> m_fontFaceSet;
@@ -103 +111 @@
     unsigned m_version;
     bool m_creatingFont { false };
+    bool m_buildIsUnderway { false };
 };
 

trunk/Source/WebCore/css/SourceSizeList.cpp

@@ -48 +48 @@
         if (!primitiveValue.isLength())
             return defaultLength(style, renderer);
+
+        // Because we evaluate "sizes" at parse time (before style has been resolved), the font metrics used for these specific units
+        // are not available. The font selector's internal consistency isn't guaranteed just yet, so we can just temporarily clear
+        // the pointer to it for the duration of the unit evaluation. This is acceptible because the style always comes from the
+        // RenderView, which has its font information hardcoded in resolveForDocument() to be -webkit-standard, whose operations
+        // don't require a font selector.
+        if (!primitiveValue.isCalculated() && (primitiveValue.primitiveType() == CSSPrimitiveValue::CSS_EXS || primitiveValue.primitiveType() == CSSPrimitiveValue::CSS_CHS)) {
+            RefPtr<FontSelector> fontSelector = style.fontCascade().fontSelector();
+            style.fontCascade().update(nullptr);
+            float result = primitiveValue.computeLength<float>(conversionData);
+            style.fontCascade().update(fontSelector.get());
+            return result;
+        }
+
         return primitiveValue.computeLength<float>(conversionData);
     }

trunk/Source/WebCore/css/StyleResolver.cpp

@@ -297 +297 @@
 {
     m_ruleSets.appendAuthorStyleSheets(styleSheets, &m_mediaQueryEvaluator, m_inspectorCSSOMWrappers, this);
+
+    document().fontSelector().buildCompleted();
+
     if (auto renderView = document().renderView())
         renderView->style().fontCascade().update(&document().fontSelector());

trunk/Source/WebCore/dom/Document.cpp

@@ -530 +530 @@
 #endif
     , m_templateDocumentHost(nullptr)
+    , m_fontSelector(CSSFontSelector::create(*this))
 #if ENABLE(WEB_REPLAY)
     , m_inputCursor(EmptyInputCursor::create())
@@ -563 +564 @@
     initSecurityContext();
     initDNSPrefetch();
+
+    m_fontSelector->registerForInvalidationCallbacks(*this);
 
     for (auto& nodeListAndCollectionCount : m_nodeListAndCollectionCounts)
@@ -638 +641 @@
 
     clearStyleResolver(); // We need to destroy CSSFontSelector before destroying m_cachedResourceLoader.
+    m_fontSelector->clearDocument();
+    m_fontSelector->unregisterForInvalidationCallbacks(*this);
 
     // It's possible for multiple Documents to end up referencing the same CachedResourceLoader (e.g., SVGImages
@@ -2209 +2214 @@
 }
 
-CSSFontSelector& Document::fontSelector()
-{
-    if (!m_fontSelector) {
-        m_fontSelector = CSSFontSelector::create(*this);
-        m_fontSelector->registerForInvalidationCallbacks(*this);
-    }
-    return *m_fontSelector;
-}
-
 void Document::clearStyleResolver()
 {
@@ -2223 +2219 @@
     m_userAgentShadowTreeStyleResolver = nullptr;
 
-    // FIXME: It would be better if the FontSelector could survive this operation.
-    if (m_fontSelector) {
-        m_fontSelector->clearDocument();
-        m_fontSelector->unregisterForInvalidationCallbacks(*this);
-        m_fontSelector = nullptr;
-    }
+    m_fontSelector->buildStarted();
 }
 

trunk/Source/WebCore/dom/Document.h

@@ -505 +505 @@
     StyleResolver& userAgentShadowTreeStyleResolver();
 
-    CSSFontSelector& fontSelector();
+    CSSFontSelector& fontSelector() { return m_fontSelector; }
 
     void notifyRemovePendingSheetIfNeeded();
@@ -1737 +1737 @@
 #endif
 
-    RefPtr<CSSFontSelector> m_fontSelector;
+    Ref<CSSFontSelector> m_fontSelector;
 
 #if ENABLE(WEB_REPLAY)

trunk/Source/WebCore/style/StyleTreeResolver.cpp

@@ -493 +493 @@
 
     Element* documentElement = m_document.documentElement();
-    if (!documentElement)
+    if (!documentElement) {
+        m_document.ensureStyleResolver();
         return nullptr;
+    }
     if (change != Force && !documentElement->childNeedsStyleRecalc() && !documentElement->needsStyleRecalc())
         return nullptr;