Attachment 272436 Details for Bug 154101 – Patch for Committing

[patch] Patch for Committing

bug-154101-20160227212849.patch (text/plain), 12.61 KB, created by Myles C. Maxfield on 2016-02-27 21:29:06 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Myles C. Maxfield

Created: 2016-02-27 21:29:06 PST

Size: 12.61 KB

patch

obsolete

>Subversion Revision: 197263
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 66bf45805c0433e49b6e04ba3be76817c49aca56..3201f9291c63d920394b638ec70cf87d4c243f9f 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,51 @@
>+2016-02-27  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-02-27  Chris Dumez  <cdumez@apple.com>
> 
>         Prevent cross-origin access to Location.assign() / Location.reload()
>diff --git a/Source/WebCore/css/CSSFontFaceSet.cpp b/Source/WebCore/css/CSSFontFaceSet.cpp
>index dbcfb7a2c7efd57a850609a6a50621414466a823..575414e8d42e1c2c52414f5a43ce69a4fa81d0c1 100644
>--- a/Source/WebCore/css/CSSFontFaceSet.cpp
>+++ b/Source/WebCore/css/CSSFontFaceSet.cpp
>@@ -242,6 +242,8 @@ void CSSFontFaceSet::clear()
>     m_facesLookupTable.clear();
>     m_locallyInstalledFacesLookupTable.clear();
>     m_cache.clear();
>+    m_facesPartitionIndex = 0;
>+    m_status = Status::Loaded;
> }
> 
> CSSFontFace& CSSFontFaceSet::operator[](size_t i)
>diff --git a/Source/WebCore/css/CSSFontSelector.cpp b/Source/WebCore/css/CSSFontSelector.cpp
>index 8a74c98ad0471fdbb4a204d6d692c87595a9eaa9..eb893acc97191bca31618d1ff4655d75e77ea5d3 100644
>--- a/Source/WebCore/css/CSSFontSelector.cpp
>+++ b/Source/WebCore/css/CSSFontSelector.cpp
>@@ -70,10 +70,6 @@ CSSFontSelector::CSSFontSelector(Document& document)
>     , m_uniqueId(++fontSelectorId)
>     , 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);
>     m_cssFontFaceSet->addClient(*this);
>@@ -101,8 +97,33 @@ bool CSSFontSelector::isEmpty() const
>     return !m_cssFontFaceSet->faceCount();
> }
> 
>+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);
>     RefPtr<CSSValue> fontStyle = style.getPropertyCSSValue(CSSPropertyFontStyle);
>@@ -234,6 +255,8 @@ static const AtomicString& resolveGenericFamily(Document* document, const FontDe
> 
> 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;
> 
>@@ -267,7 +290,6 @@ void CSSFontSelector::clearDocument()
> 
>     m_document = nullptr;
> 
>-    // FIXME: This object should outlive the Document.
>     m_cssFontFaceSet->clear();
>     m_clients.clear();
> }
>diff --git a/Source/WebCore/css/CSSFontSelector.h b/Source/WebCore/css/CSSFontSelector.h
>index c112e88f747659c890550261b5ed519da86fff17..188e32c2f79263c00d5c8cb2962097cae02b7346 100644
>--- a/Source/WebCore/css/CSSFontSelector.h
>+++ b/Source/WebCore/css/CSSFontSelector.h
>@@ -65,6 +65,8 @@ public:
>     virtual RefPtr<Font> fallbackFontAt(const FontDescription&, size_t) override;
> 
>     void clearDocument();
>+    void buildStarted();
>+    void buildCompleted();
> 
>     void addFontFaceRule(StyleRuleFontFace&, bool isInitiatingElementInUserAgentShadowTree);
> 
>@@ -91,6 +93,12 @@ private:
> 
>     void beginLoadTimerFired();
> 
>+    struct PendingFontFaceRule {
>+        StyleRuleFontFace& styleRuleFontFace;
>+        bool isInitiatingElementInUserAgentShadowTree;
>+    };
>+    Vector<PendingFontFaceRule> m_stagingArea;
>+
>     Document* m_document;
>     RefPtr<FontFaceSet> m_fontFaceSet;
>     Ref<CSSFontFaceSet> m_cssFontFaceSet;
>@@ -102,6 +110,7 @@ private:
>     unsigned m_uniqueId;
>     unsigned m_version;
>     bool m_creatingFont { false };
>+    bool m_buildIsUnderway { false };
> };
> 
> } // namespace WebCore
>diff --git a/Source/WebCore/css/CSSStyleSheet.cpp b/Source/WebCore/css/CSSStyleSheet.cpp
>index 1fc893925acf0a97c217039101a78d36c94ebfdc..0372c6c937c4aa777c418de63037ed054ea6711f 100644
>--- a/Source/WebCore/css/CSSStyleSheet.cpp
>+++ b/Source/WebCore/css/CSSStyleSheet.cpp
>@@ -423,7 +423,7 @@ CSSStyleSheet::RuleMutationScope::RuleMutationScope(CSSStyleSheet* sheet, RuleMu
> }
> 
> CSSStyleSheet::RuleMutationScope::RuleMutationScope(CSSRule* rule)
>-    : m_styleSheet(rule ? rule->parentStyleSheet() : 0)
>+    : m_styleSheet(rule ? rule->parentStyleSheet() : nullptr)
>     , m_mutationType(OtherMutation)
>     , m_contentsWereClonedForMutation(ContentsWereNotClonedForMutation)
>     , m_insertedKeyframesRule(nullptr)
>diff --git a/Source/WebCore/css/SourceSizeList.cpp b/Source/WebCore/css/SourceSizeList.cpp
>index 8d1d016b74924aa17e10a9b1cd1422c1be44f34a..c1eba35836b92dfc9c6e807b360aa0dbb38b0914 100644
>--- a/Source/WebCore/css/SourceSizeList.cpp
>+++ b/Source/WebCore/css/SourceSizeList.cpp
>@@ -70,6 +70,7 @@ float parseSizesAttribute(StringView sizesAttribute, RenderView* view, Frame* fr
> {
>     if (!view)
>         return 0;
>+    view->document().updateStyleIfNeeded();
>     RenderStyle& style = view->style();
>     for (auto& sourceSize : CSSParser(CSSStrictMode).parseSizesAttribute(sizesAttribute)) {
>         if (match(WTFMove(sourceSize.expression), style, frame))
>diff --git a/Source/WebCore/css/StyleResolver.cpp b/Source/WebCore/css/StyleResolver.cpp
>index c8045716a0c25ce1d6e63f03e2ac5957e4a97fed..1176d904d9c8f447c55ff5f80b6a051ae4da7686 100644
>--- a/Source/WebCore/css/StyleResolver.cpp
>+++ b/Source/WebCore/css/StyleResolver.cpp
>@@ -283,6 +283,9 @@ StyleResolver::StyleResolver(Document& document)
> void StyleResolver::appendAuthorStyleSheets(const Vector<RefPtr<CSSStyleSheet>>& styleSheets)
> {
>     m_ruleSets.appendAuthorStyleSheets(styleSheets, m_medium.get(), m_inspectorCSSOMWrappers, this);
>+
>+    document().fontSelector().buildCompleted();
>+
>     if (auto renderView = document().renderView())
>         renderView->style().fontCascade().update(&document().fontSelector());
> 
>diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
>index 64636d07fd286c71ba9385d07682723fbaa7af56..593242c94e9933f58ced65a07c196811f033e843 100644
>--- a/Source/WebCore/dom/Document.cpp
>+++ b/Source/WebCore/dom/Document.cpp
>@@ -539,6 +539,7 @@ Document::Document(Frame* frame, const URL& url, unsigned documentClasses, unsig
> #if ENABLE(TEMPLATE_ELEMENT)
>     , m_templateDocumentHost(nullptr)
> #endif
>+    , m_fontSelector(CSSFontSelector::create(*this))
> #if ENABLE(WEB_REPLAY)
>     , m_inputCursor(EmptyInputCursor::create())
> #endif
>@@ -573,6 +574,8 @@ Document::Document(Frame* frame, const URL& url, unsigned documentClasses, unsig
>     initSecurityContext();
>     initDNSPrefetch();
> 
>+    m_fontSelector->registerForInvalidationCallbacks(*this);
>+
>     for (auto& nodeListAndCollectionCount : m_nodeListAndCollectionCounts)
>         nodeListAndCollectionCount = 0;
> }
>@@ -649,6 +652,8 @@ Document::~Document()
>     extensionStyleSheets().detachFromDocument();
> 
>     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
>     // load the initial empty document and the SVGDocument with the same DocumentLoader).
>@@ -2189,26 +2194,12 @@ void Document::fontsNeedUpdate(FontSelector&)
>     scheduleForcedStyleRecalc();
> }
> 
>-CSSFontSelector& Document::fontSelector()
>-{
>-    if (!m_fontSelector) {
>-        m_fontSelector = CSSFontSelector::create(*this);
>-        m_fontSelector->registerForInvalidationCallbacks(*this);
>-    }
>-    return *m_fontSelector;
>-}
>-
> void Document::clearStyleResolver()
> {
>     m_styleResolver = nullptr;
>     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();
> }
> 
> void Document::createRenderTree()
>diff --git a/Source/WebCore/dom/Document.h b/Source/WebCore/dom/Document.h
>index f5da7167d49153495a6d168ff17d94597c24defb..2e6dad664e66b2628577b9359fe9e6fcedefdca8 100644
>--- a/Source/WebCore/dom/Document.h
>+++ b/Source/WebCore/dom/Document.h
>@@ -503,7 +503,7 @@ public:
>     }
>     StyleResolver& userAgentShadowTreeStyleResolver();
> 
>-    CSSFontSelector& fontSelector();
>+    CSSFontSelector& fontSelector() { return m_fontSelector; }
> 
>     void notifyRemovePendingSheetIfNeeded();
> 
>@@ -1739,7 +1739,7 @@ private:
>     std::unique_ptr<CustomElementDefinitions> m_customElementDefinitions;
> #endif
> 
>-    RefPtr<CSSFontSelector> m_fontSelector;
>+    Ref<CSSFontSelector> m_fontSelector;
> 
> #if ENABLE(WEB_REPLAY)
>     RefPtr<JSC::InputCursor> m_inputCursor;
>diff --git a/Source/WebCore/style/StyleTreeResolver.cpp b/Source/WebCore/style/StyleTreeResolver.cpp
>index 2743deee89c567d03f47c6240a5b86389364563f..506923bd772edd8ca701a6dc335b1caf107716ee 100644
>--- a/Source/WebCore/style/StyleTreeResolver.cpp
>+++ b/Source/WebCore/style/StyleTreeResolver.cpp
>@@ -960,8 +960,10 @@ void TreeResolver::resolve(Change change)
>     auto& renderView = *m_document.renderView();
> 
>     Element* documentElement = m_document.documentElement();
>-    if (!documentElement)
>+    if (!documentElement) {
>+        m_document.ensureStyleResolver();
>         return;
>+    }
>     if (change != Force && !documentElement->childNeedsStyleRecalc() && !documentElement->needsStyleRecalc())
>         return;
>

Flags:

buildbot: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 154101: 271048 | 272086 | 272092 | 272098 | 272103 | 272436 | 272438 | 272440 | 272550 | 280682 | 280772 | 280773