Moving fontSelector from StyleResolver to StyleEngine.

Source/core/dom/StyleEngine.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  *           (C) 2006 Alexey Proskuryakov (ap@webkit.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2011, 2012 Apple Inc. All rights reserved.
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  * Copyright (C) 2008, 2009, 2011, 2012 Google Inc. All rights reserved.
  * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) Research In Motion Limited 2010-2011. All rights reserved.
@@ skipping 12 matching lines @@
  * along with this library; see the file COPYING.LIB.  If not, write to
  * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
  * Boston, MA 02110-1301, USA.
  */
 
 #include "config.h"
 #include "core/dom/StyleEngine.h"
 
 #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"
 #include "core/dom/Element.h"
 #include "core/dom/ProcessingInstruction.h"
 #include "core/dom/ShadowTreeStyleSheetCollection.h"
 #include "core/dom/shadow/ShadowRoot.h"
 #include "core/html/HTMLIFrameElement.h"
 #include "core/html/HTMLImport.h"
 #include "core/html/HTMLLinkElement.h"
@@ skipping 20 matching lines @@
     , m_usesSiblingRules(false)
     , m_usesSiblingRulesOverride(false)
     , m_usesFirstLineRules(false)
     , m_usesFirstLetterRules(false)
     , m_usesRemUnits(false)
     , m_maxDirectAdjacentSelectors(0)
     , m_ignorePendingStylesheets(false)
     , 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)
         m_authorStyleSheets[i]->clearOwnerNode();
 }
 
 inline Document* StyleEngine::master()
 {
     if (isMaster())
         return &m_document;
@@ skipping 365 matching lines @@
     for (TreeScopeSet::iterator it = begin; it != end; ++it) {
         if (StyleSheetCollection* collection = m_styleSheetCollectionMap.get(*it)) {
             m_resolver->setBuildScopedStyleTreeInDocumentOrder(!collection->scopingNodesForStyleScoped());
             m_resolver->appendAuthorStyleSheets(0, collection->activeAuthorStyleSheets());
         }
     }
     m_resolver->finishAppendAuthorStyleSheets();
     m_resolver->setBuildScopedStyleTreeInDocumentOrder(false);
 }
 
 void StyleEngine::createResolver()

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

Should this ASSERT(m_fontSelector)?

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

I see. Done.

 {
     // It is a programming error to attempt to resolve style on a Document
     // which is not in a frame. Code which hits this should have checked
     // Document::isActive() before calling into code which could get here.
 
     ASSERT(m_document.frame());
 
     m_resolver = adoptPtr(new StyleResolver(m_document));
     appendActiveAuthorStyleSheets();
+    m_fontSelector->registerForInvalidationCallbacks(m_resolver.get());
     combineCSSFeatureFlags(m_resolver->ensureRuleFeatureSet());
 }
 
 void StyleEngine::clearResolver()

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

Should this ASSERT(m_fontSelector)?

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

I added ASSERT(m_fontSelector || !m_resolver).

 {
     ASSERT(!m_document.inStyleRecalc());
     ASSERT(isMaster() || !m_resolver);
+    if (m_resolver)
+        m_fontSelector->unregisterForInvalidationCallbacks(m_resolver.get());
     m_resolver.clear();
 }
 
 void StyleEngine::clearMasterResolver()
 {
     if (Document* master = this->master())
         master->styleEngine()->clearResolver();
 }
 
 unsigned StyleEngine::resolverAccessCount() const
 {
     return m_resolver ? m_resolver->accessCount() : 0;
 }
 
 void StyleEngine::resolverThrowawayTimerFired(Timer<StyleEngine>*)
 {
     if (resolverAccessCount() == m_lastResolverAccessCount)
         clearResolver();
     m_lastResolverAccessCount = resolverAccessCount();
 }
 
-CSSFontSelector* StyleEngine::fontSelector()
-{
-    return m_resolver ? m_resolver->fontSelector() : 0;
-}
-
 void StyleEngine::didAttach()
 {
     m_resolverThrowawayTimer.startRepeating(60);
 }
 
 void StyleEngine::didDetach()
 {
     m_resolverThrowawayTimer.stop();
     clearResolver();
 }
@@ skipping 23 matching lines @@
     m_didCalculateResolver = true;
     if (m_document.didLayoutWithPendingStylesheets() && !hasPendingSheets())
         change.setNeedsRepaint();
 
     if (updateActiveStyleSheets(mode))
         change.setNeedsStyleRecalc();
 
     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());
 }
+
+}