Remove "rem" unit optimization for document element font size changes
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 30 Sep 2016 17:00:22 +0000 (17:00 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 30 Sep 2016 17:00:22 +0000 (17:00 +0000)
https://bugs.webkit.org/show_bug.cgi?id=162778

Reviewed by Alex Christensen.

We awkwardly track from the parser level if any stylesheet in a document uses any rem units. This is only used to minimally
optimize a case where document element's (<html>) font size changes dynamically.

In practice such changes are rare. Browsing around I couldn't find a single case where this optimization got used.
Even if it was used it would be of low value as a full style resolution is likely to happen anyway (as font inherits)
and the only thing really saved is that we don't need to invalidate the matched properties cache.

* css/CSSGrammar.y.in:
* css/StyleSheetContents.cpp:
(WebCore::StyleSheetContents::StyleSheetContents):
* css/StyleSheetContents.h:
* dom/AuthorStyleSheets.cpp:
(WebCore::AuthorStyleSheets::updateActiveStyleSheets):
* dom/AuthorStyleSheets.h:
(WebCore::AuthorStyleSheets::usesRemUnits): Deleted.
(WebCore::AuthorStyleSheets::setUsesRemUnit): Deleted.
* dom/Document.cpp:
(WebCore::Document::recalcStyle):
(WebCore::Document::updateBaseURL):
* style/StyleTreeResolver.cpp:
(WebCore::Style::TreeResolver::resolveElement):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@206641 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/css/CSSGrammar.y.in
Source/WebCore/css/StyleSheetContents.cpp
Source/WebCore/css/StyleSheetContents.h
Source/WebCore/dom/AuthorStyleSheets.cpp
Source/WebCore/dom/AuthorStyleSheets.h
Source/WebCore/dom/Document.cpp
Source/WebCore/style/StyleTreeResolver.cpp

index cc39d81..1301589 100644 (file)
@@ -1,3 +1,32 @@
+2016-09-30  Antti Koivisto  <antti@apple.com>
+
+        Remove "rem" unit optimization for document element font size changes
+        https://bugs.webkit.org/show_bug.cgi?id=162778
+
+        Reviewed by Alex Christensen.
+
+        We awkwardly track from the parser level if any stylesheet in a document uses any rem units. This is only used to minimally
+        optimize a case where document element's (<html>) font size changes dynamically.
+
+        In practice such changes are rare. Browsing around I couldn't find a single case where this optimization got used.
+        Even if it was used it would be of low value as a full style resolution is likely to happen anyway (as font inherits)
+        and the only thing really saved is that we don't need to invalidate the matched properties cache.
+
+        * css/CSSGrammar.y.in:
+        * css/StyleSheetContents.cpp:
+        (WebCore::StyleSheetContents::StyleSheetContents):
+        * css/StyleSheetContents.h:
+        * dom/AuthorStyleSheets.cpp:
+        (WebCore::AuthorStyleSheets::updateActiveStyleSheets):
+        * dom/AuthorStyleSheets.h:
+        (WebCore::AuthorStyleSheets::usesRemUnits): Deleted.
+        (WebCore::AuthorStyleSheets::setUsesRemUnit): Deleted.
+        * dom/Document.cpp:
+        (WebCore::Document::recalcStyle):
+        (WebCore::Document::updateBaseURL):
+        * style/StyleTreeResolver.cpp:
+        (WebCore::Style::TreeResolver::resolveElement):
+
 2016-09-30  Zalan Bujtas  <zalan@apple.com>
 
         RenderLayer::clipRects may return nullptr.
index ce8b5e6..2f51cf8 100644 (file)
@@ -1728,8 +1728,6 @@ unary_term:
       $$.id = CSSValueInvalid;
       $$.fValue = $1;
       $$.unit = CSSPrimitiveValue::CSS_REMS;
-      if (parser->m_styleSheet)
-          parser->m_styleSheet->parserSetUsesRemUnits();
   }
   | CHS { $$.id = CSSValueInvalid; $$.fValue = $1; $$.unit = CSSPrimitiveValue::CSS_CHS; }
   | VW { $$.id = CSSValueInvalid; $$.fValue = $1; $$.unit = CSSPrimitiveValue::CSS_VW; }
index 1d649ca..05d96d1 100644 (file)
@@ -67,7 +67,6 @@ StyleSheetContents::StyleSheetContents(StyleRuleImport* ownerRule, const String&
     , m_isUserStyleSheet(ownerRule && ownerRule->parentStyleSheet() && ownerRule->parentStyleSheet()->isUserStyleSheet())
     , m_hasSyntacticallyValidCSSHeader(true)
     , m_didLoadErrorOccur(false)
-    , m_usesRemUnits(false)
     , m_usesStyleBasedEditability(false)
     , m_isMutable(false)
     , m_isInMemoryCache(false)
@@ -88,7 +87,6 @@ StyleSheetContents::StyleSheetContents(const StyleSheetContents& o)
     , m_isUserStyleSheet(o.m_isUserStyleSheet)
     , m_hasSyntacticallyValidCSSHeader(o.m_hasSyntacticallyValidCSSHeader)
     , m_didLoadErrorOccur(false)
-    , m_usesRemUnits(o.m_usesRemUnits)
     , m_usesStyleBasedEditability(o.m_usesStyleBasedEditability)
     , m_isMutable(false)
     , m_isInMemoryCache(false)
index 59d8a99..052072c 100644 (file)
@@ -96,7 +96,6 @@ public:
     void parserAddNamespace(const AtomicString& prefix, const AtomicString& uri);
     void parserAppendRule(Ref<StyleRuleBase>&&);
     void parserSetEncodingFromCharsetRule(const String& encoding); 
-    void parserSetUsesRemUnits() { m_usesRemUnits = true; }
     void parserSetUsesStyleBasedEditability() { m_usesStyleBasedEditability = true; }
 
     void clearRules();
@@ -122,7 +121,6 @@ public:
     unsigned ruleCount() const;
     StyleRuleBase* ruleAt(unsigned index) const;
 
-    bool usesRemUnits() const { return m_usesRemUnits; }
     bool usesStyleBasedEditability() const { return m_usesStyleBasedEditability; }
 
     unsigned estimatedSizeInBytes() const;
@@ -166,7 +164,6 @@ private:
     bool m_isUserStyleSheet : 1;
     bool m_hasSyntacticallyValidCSSHeader : 1;
     bool m_didLoadErrorOccur : 1;
-    bool m_usesRemUnits : 1;
     bool m_usesStyleBasedEditability : 1;
     bool m_isMutable : 1;
     bool m_isInMemoryCache : 1;
index 6a9d4fd..cbd0bed 100644 (file)
@@ -330,8 +330,6 @@ void AuthorStyleSheets::updateActiveStyleSheets(UpdateType updateType)
     InspectorInstrumentation::activeStyleSheetsUpdated(m_document);
 
     for (const auto& sheet : m_activeStyleSheets) {
-        if (sheet->contents().usesRemUnits())
-            m_usesRemUnits = true;
         if (sheet->contents().usesStyleBasedEditability())
             m_usesStyleBasedEditability = true;
     }
index c514c37..5e42237 100644 (file)
@@ -76,8 +76,6 @@ public:
 
     bool hasPendingSheets() const { return m_pendingStyleSheetCount > 0; }
 
-    bool usesRemUnits() const { return m_usesRemUnits; }
-    void setUsesRemUnit(bool b) { m_usesRemUnits = b; }
     bool usesStyleBasedEditability() { return m_usesStyleBasedEditability; }
 
     bool activeStyleSheetsContains(const CSSStyleSheet*) const;
@@ -131,7 +129,6 @@ private:
     String m_preferredStylesheetSetName;
     String m_selectedStylesheetSetName;
 
-    bool m_usesRemUnits { false };
     bool m_usesStyleBasedEditability { false };
 };
 
index 126d999..464eb38 100644 (file)
@@ -1866,12 +1866,6 @@ void Document::recalcStyle(Style::Change change)
 
     InspectorInstrumentationCookie cookie = InspectorInstrumentation::willRecalculateStyle(*this);
 
-    // FIXME: We never reset this flags.
-    if (m_elementSheet && m_elementSheet->contents().usesRemUnits())
-        authorStyleSheets().setUsesRemUnit(true);
-    // We don't call setUsesStyleBasedEditability here because the whole point of the flag is to avoid style recalc.
-    // i.e. updating the flag here would be too late.
-
     m_inStyleRecalc = true;
     bool updatedCompositingLayers = false;
     {
@@ -2972,12 +2966,9 @@ void Document::updateBaseURL()
     if (m_elementSheet) {
         // Element sheet is silly. It never contains anything.
         ASSERT(!m_elementSheet->contents().ruleCount());
-        bool usesRemUnits = m_elementSheet->contents().usesRemUnits();
         bool usesStyleBasedEditability = m_elementSheet->contents().usesStyleBasedEditability();
         m_elementSheet = CSSStyleSheet::createInline(*this, m_baseURL);
         // FIXME: So we are not really the parser. The right fix is to eliminate the element sheet completely.
-        if (usesRemUnits)
-            m_elementSheet->contents().parserSetUsesRemUnits();
         if (usesStyleBasedEditability)
             m_elementSheet->contents().parserSetUsesStyleBasedEditability();
     }
index d5ee1ef..b4ae062 100644 (file)
@@ -214,10 +214,9 @@ ElementUpdate TreeResolver::resolveElement(Element& element)
         m_documentElementStyle = RenderStyle::clonePtr(*update.style);
         scope().styleResolver.setOverrideDocumentElementStyle(m_documentElementStyle.get());
 
-        // If "rem" units are used anywhere in the document, and if the document element's font size changes, then force font updating
-        // all the way down the tree. This is simpler than having to maintain a cache of objects (and such font size changes should be rare anyway).
-        if (m_document.authorStyleSheets().usesRemUnits() && update.change != NoChange && existingStyle && existingStyle->fontSize() != update.style->fontSize()) {
-            // Cached RenderStyles may depend on the rem units.
+        if (update.change != NoChange && existingStyle && existingStyle->fontSize() != update.style->fontSize()) {
+            // "rem" units are relative to the document element's font size so we need to recompute everything.
+            // In practice this is rare.
             scope().styleResolver.invalidateMatchedPropertiesCache();
             update.change = Force;
         }