Moving fontSelector from StyleResolver to StyleEngine.

Source/core/dom/StyleEngine.cpp

Index: Source/core/dom/StyleEngine.cpp
diff --git a/Source/core/dom/StyleEngine.cpp b/Source/core/dom/StyleEngine.cpp
index bea7b4954ea14bef55b78a15e653bc42595db129..b7f4a225a3f7feadef7918984bb7d5a070fdb304 100644
--- a/Source/core/dom/StyleEngine.cpp
+++ b/Source/core/dom/StyleEngine.cpp
@@ -30,6 +30,7 @@
 
 #include "HTMLNames.h"
 #include "SVGNames.h"
+#include "core/css/CSSFontSelector.h"
 #include "core/css/CSSStyleSheet.h"
 #include "core/css/StyleInvalidationAnalysis.h"
 #include "core/css/StyleSheetContents.h"
@@ -70,11 +71,18 @@ StyleEngine::StyleEngine(Document& document)
     , m_didCalculateResolver(false)
     , m_lastResolverAccessCount(0)
     , m_resolverThrowawayTimer(this, &StyleEngine::resolverThrowawayTimerFired)
+    , m_fontSelector(document.frame() ? CSSFontSelector::create(&document) : 0)

[ojan, 2013/12/05 18:57:15]

Is this null checking document.frame() for import documents? If so, I think it
could use an explanatory comment.

[tasak, 2013/12/06 07:13:47]

Yes, and also for HTMLTemplateElement.
I found, if we have no null checking here, http/tests/htmlimports and tests
containing <template> will crash.

c.f.
STDERR: Received signal 11 SEGV_MAPERR 000000000028
STDERR:  [0x7fe9da748168] base::debug::StackTrace::StackTrace()
STDERR:  [0x7fe9da747a6f] base::debug::(anonymous
namespace)::StackDumpSignalHandler()
STDERR:  [0x7fe9cf6c6cb0] <unknown>
STDERR:  [0x7fe9d6d6678e] WebCore::Frame::settings()
STDERR:  [0x7fe9d6a03cad] WebCore::CSSFontSelector::CSSFontSelector()
STDERR:  [0x7fe9d6452b85] WebCore::CSSFontSelector::create()
STDERR:  [0x7fe9d6450338] WebCore::StyleEngine::StyleEngine()
STDERR:  [0x7fe9d6375596] WebCore::StyleEngine::create()
STDERR:  [0x7fe9d635f5b7] WebCore::Document::Document()
STDERR:  [0x7fe9d65ce4a0] WebCore::HTMLDocument::HTMLDocument()
STDERR:  [0x7fe9d536d260] WebCore::HTMLDocument::create()
STDERR:  [0x7fe9d65f354a] WebCore::HTMLImportData::startWritingAndParsing()
STDERR:  [0x7fe9d65f3365] WebCore::HTMLImportData::responseReceived()

and

STDERR: Received signal 11 SEGV_MAPERR 000000000028
STDERR:  [0x7f2d201c3168] base::debug::StackTrace::StackTrace()
STDERR:  [0x7f2d201c2a6f] base::debug::(anonymous
namespace)::StackDumpSignalHandler()
STDERR:  [0x7f2d15141cb0] <unknown>
STDERR:  [0x7f2d1c7e178e] WebCore::Frame::settings()
STDERR:  [0x7f2d1c47ecad] WebCore::CSSFontSelector::CSSFontSelector()
STDERR:  [0x7f2d1becdb85] WebCore::CSSFontSelector::create()
STDERR:  [0x7f2d1becb338] WebCore::StyleEngine::StyleEngine()
STDERR:  [0x7f2d1bdf0596] WebCore::StyleEngine::create()
STDERR:  [0x7f2d1bdda5b7] WebCore::Document::Document()
STDERR:  [0x7f2d1c0494a0] WebCore::HTMLDocument::HTMLDocument()
STDERR:  [0x7f2d1ade8260] WebCore::HTMLDocument::create()
STDERR:  [0x7f2d1bdee2f8] WebCore::Document::ensureTemplateDocument()
STDERR:  [0x7f2d1c0bd313] WebCore::HTMLTemplateElement::content()

 {
 }
 
 StyleEngine::~StyleEngine()
 {
+    if (m_fontSelector) {

[ojan, 2013/12/05 18:57:15]

Why do we need to do this?

[tasak, 2013/12/06 07:13:47]

I see. Looking at ~CSSFontSelector, we don't need this.

+        if (m_resolver)
+            m_fontSelector->unregisterForInvalidationCallbacks(m_resolver.get());
+        m_fontSelector->clearDocument();
+    }
+
     for (unsigned i = 0; i < m_injectedAuthorStyleSheets.size(); ++i)
         m_injectedAuthorStyleSheets[i]->clearOwnerNode();
     for (unsigned i = 0; i < m_authorStyleSheets.size(); ++i)
@@ -470,6 +478,7 @@ void StyleEngine::createResolver()
 
     m_resolver = adoptPtr(new StyleResolver(m_document));
     appendActiveAuthorStyleSheets();
+    m_fontSelector->registerForInvalidationCallbacks(m_resolver.get());
     combineCSSFeatureFlags(m_resolver->ensureRuleFeatureSet());
 }
 
@@ -477,6 +486,8 @@ void StyleEngine::clearResolver()
 {
     ASSERT(!m_document.inStyleRecalc());
     ASSERT(isMaster() || !m_resolver);
+    if (m_resolver)
+        m_fontSelector->unregisterForInvalidationCallbacks(m_resolver.get());
     m_resolver.clear();
 }
 
@@ -498,11 +509,6 @@ void StyleEngine::resolverThrowawayTimerFired(Timer<StyleEngine>*)
     m_lastResolverAccessCount = resolverAccessCount();
 }
 
-CSSFontSelector* StyleEngine::fontSelector()
-{
-    return m_resolver ? m_resolver->fontSelector() : 0;
-}
-
 void StyleEngine::didAttach()
 {
     m_resolverThrowawayTimer.startRepeating(60);
@@ -546,4 +552,19 @@ StyleResolverChange StyleEngine::resolverChanged(RecalcStyleTime time, StyleReso
     return change;
 }
 
+void StyleEngine::resetFontSelector()
+{
+    if (!m_document.frame())

[ojan, 2013/12/05 18:57:15]

Should this be if (!m_fontSelector)?

[tasak, 2013/12/06 07:13:47]

I think, should be if (!m_fontSelector || !m_document.isActive()).

I also added ASSERT(m_document.frame()).

+        return;
+
+    m_fontSelector->clearDocument();
+    if (m_resolver) {
+        m_fontSelector->unregisterForInvalidationCallbacks(m_resolver.get());
+        m_resolver->invalidateMatchedPropertiesCache();
+    }
+    m_fontSelector = CSSFontSelector::create(&m_document);
+    if (m_resolver)
+        m_fontSelector->registerForInvalidationCallbacks(m_resolver.get());
+}
+
 }