Remove FIRST_LINE_INHERITED fake pseudo style
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 28 Nov 2016 15:58:35 +0000 (15:58 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 28 Nov 2016 15:58:35 +0000 (15:58 +0000)
https://bugs.webkit.org/show_bug.cgi?id=165071

Reviewed by Andreas Kling.

Source/WebCore:

These are create during layout an then cached to the RenderStyle. Cache computed first line style to
RenderObject rare data instead, avoiding style mutation an other confusing messiness.

* rendering/RenderElement.cpp:
(WebCore::RenderElement::RenderElement):
(WebCore::RenderElement::computeFirstLineStyle):
(WebCore::RenderElement::firstLineStyle):

    Cache the first line style.

(WebCore::RenderElement::invalidateCachedFirstLineStyle):
(WebCore::RenderElement::styleWillChange):

    Invalidate subtree if we have cached first line style.

(WebCore::RenderElement::getUncachedPseudoStyle):
(WebCore::RenderElement::uncachedFirstLineStyle): Deleted.
(WebCore::RenderElement::cachedFirstLineStyle): Deleted.
* rendering/RenderElement.h:
* rendering/RenderObject.cpp:
(WebCore::RenderObject::rareDataMap):
(WebCore::RenderObject::rareData):
(WebCore::RenderObject::ensureRareData):
* rendering/RenderObject.h:

    Stop copying rare data objects.

* rendering/style/RenderStyle.cpp:
(WebCore::RenderStyle::changeRequiresLayout):

    Use the normal mechanism for invalidating layout for first-line instead of a hack in pseudoStyleCacheIsInvalid.

* rendering/style/RenderStyleConstants.h:
* style/RenderTreeUpdater.cpp:
(WebCore::pseudoStyleCacheIsInvalid):

    Simplify.

LayoutTests:

Expand the test case a bit.

* fast/css/pseudo-cache-stale-expected.html:
* fast/css/pseudo-cache-stale.html:

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

LayoutTests/ChangeLog
LayoutTests/fast/css/pseudo-cache-stale-expected.html
LayoutTests/fast/css/pseudo-cache-stale.html
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderElement.cpp
Source/WebCore/rendering/RenderElement.h
Source/WebCore/rendering/RenderObject.cpp
Source/WebCore/rendering/RenderObject.h
Source/WebCore/rendering/style/RenderStyle.cpp
Source/WebCore/rendering/style/RenderStyleConstants.h
Source/WebCore/style/RenderTreeUpdater.cpp

index f6d7a81..64f34c9 100644 (file)
@@ -1,3 +1,15 @@
+2016-11-28  Antti Koivisto  <antti@apple.com>
+
+        Remove FIRST_LINE_INHERITED fake pseudo style
+        https://bugs.webkit.org/show_bug.cgi?id=165071
+
+        Reviewed by Andreas Kling.
+
+        Expand the test case a bit.
+
+        * fast/css/pseudo-cache-stale-expected.html:
+        * fast/css/pseudo-cache-stale.html:
+
 2016-11-28  Per Arne Vollan  <pvollan@apple.com>
 
         [Win] WebCrypto tests are failing.
index f777f71..d3e4f83 100644 (file)
@@ -10,5 +10,8 @@
     <div id="first-line" style="font-size: xx-large;">
         This sentence should be extra-extra-large.
     </div>
+    <div id="first-line" style="font-size: xx-large;">
+        This sentence should be extra-extra-large too.
+    </div>
     <input type="search" placeholder="This should be green">
 </body>
index d6c0629..2d5fc5f 100644 (file)
         font-size: xx-large;
     }
 
+    .green #first-line-inherit:first-line {
+        font-size: xx-large;
+    }
+
     .green input::placeholder {
         color: green;
     }
@@ -26,6 +30,9 @@
     <div id="first-line">
         This sentence should be extra-extra-large.
     </div>
+    <div id="first-line-inherit">
+        <span>This sentence should be extra-extra-large too.</span>
+    </div>
     <input type="search" placeholder="This should be green">
     <script>
         var target = document.body;
index 186783f..49666d0 100644 (file)
@@ -1,3 +1,48 @@
+2016-11-28  Antti Koivisto  <antti@apple.com>
+
+        Remove FIRST_LINE_INHERITED fake pseudo style
+        https://bugs.webkit.org/show_bug.cgi?id=165071
+
+        Reviewed by Andreas Kling.
+
+        These are create during layout an then cached to the RenderStyle. Cache computed first line style to
+        RenderObject rare data instead, avoiding style mutation an other confusing messiness.
+
+        * rendering/RenderElement.cpp:
+        (WebCore::RenderElement::RenderElement):
+        (WebCore::RenderElement::computeFirstLineStyle):
+        (WebCore::RenderElement::firstLineStyle):
+
+            Cache the first line style.
+
+        (WebCore::RenderElement::invalidateCachedFirstLineStyle):
+        (WebCore::RenderElement::styleWillChange):
+
+            Invalidate subtree if we have cached first line style.
+
+        (WebCore::RenderElement::getUncachedPseudoStyle):
+        (WebCore::RenderElement::uncachedFirstLineStyle): Deleted.
+        (WebCore::RenderElement::cachedFirstLineStyle): Deleted.
+        * rendering/RenderElement.h:
+        * rendering/RenderObject.cpp:
+        (WebCore::RenderObject::rareDataMap):
+        (WebCore::RenderObject::rareData):
+        (WebCore::RenderObject::ensureRareData):
+        * rendering/RenderObject.h:
+
+            Stop copying rare data objects.
+
+        * rendering/style/RenderStyle.cpp:
+        (WebCore::RenderStyle::changeRequiresLayout):
+
+            Use the normal mechanism for invalidating layout for first-line instead of a hack in pseudoStyleCacheIsInvalid.
+
+        * rendering/style/RenderStyleConstants.h:
+        * style/RenderTreeUpdater.cpp:
+        (WebCore::pseudoStyleCacheIsInvalid):
+
+            Simplify.
+
 2016-11-28  Miguel Gomez  <magomez@igalia.com>
 
         [GTK] Dramatic increase on memory usage since 2.14.x
index 090d5e2..fc37d77 100644 (file)
@@ -104,6 +104,7 @@ inline RenderElement::RenderElement(ContainerNode& elementOrDocument, RenderStyl
     , m_hasCounterNodeMap(false)
     , m_isCSSAnimating(false)
     , m_hasContinuation(false)
+    , m_hasValidCachedFirstLineStyle(false)
     , m_renderBlockHasMarginBeforeQuirk(false)
     , m_renderBlockHasMarginAfterQuirk(false)
     , m_renderBlockShouldForceRelayoutChildren(false)
@@ -215,48 +216,44 @@ RenderPtr<RenderElement> RenderElement::createFor(Element& element, RenderStyle&
     return nullptr;
 }
 
-std::unique_ptr<RenderStyle> RenderElement::uncachedFirstLineStyle(RenderStyle* style) const
-{
-    if (!view().usesFirstLineRules())
-        return nullptr;
-
-    RenderElement& rendererForFirstLineStyle = isBeforeOrAfterContent() ? *parent() : const_cast<RenderElement&>(*this);
-
-    if (rendererForFirstLineStyle.isRenderBlockFlow() || rendererForFirstLineStyle.isRenderButton()) {
-        if (RenderBlock* firstLineBlock = rendererForFirstLineStyle.firstLineBlock())
-            return firstLineBlock->getUncachedPseudoStyle(PseudoStyleRequest(FIRST_LINE), style, firstLineBlock == this ? style : nullptr);
-    } else if (!rendererForFirstLineStyle.isAnonymous() && rendererForFirstLineStyle.isRenderInline()) {
-        auto& parentStyle = rendererForFirstLineStyle.parent()->firstLineStyle();
-        if (&parentStyle != &rendererForFirstLineStyle.parent()->style())
-            return rendererForFirstLineStyle.getUncachedPseudoStyle(PseudoStyleRequest(FIRST_LINE_INHERITED), &parentStyle, style);
-    }
-    return nullptr;
-}
-
-const RenderStyle* RenderElement::cachedFirstLineStyle() const
+std::unique_ptr<RenderStyle> RenderElement::computeFirstLineStyle() const
 {
     ASSERT(view().usesFirstLineRules());
 
     RenderElement& rendererForFirstLineStyle = isBeforeOrAfterContent() ? *parent() : const_cast<RenderElement&>(*this);
 
     if (rendererForFirstLineStyle.isRenderBlockFlow() || rendererForFirstLineStyle.isRenderButton()) {
-        if (RenderBlock* firstLineBlock = rendererForFirstLineStyle.firstLineBlock())
-            return firstLineBlock->getCachedPseudoStyle(FIRST_LINE, &style());
-    } else if (!rendererForFirstLineStyle.isAnonymous() && rendererForFirstLineStyle.isRenderInline()) {
-        auto& parentStyle = rendererForFirstLineStyle.parent()->firstLineStyle();
-        if (&parentStyle != &rendererForFirstLineStyle.parent()->style()) {
-            // A first-line style is in effect. Cache a first-line style for ourselves.
-            rendererForFirstLineStyle.m_style.setHasPseudoStyle(FIRST_LINE_INHERITED);
-            return rendererForFirstLineStyle.getCachedPseudoStyle(FIRST_LINE_INHERITED, &parentStyle);
-        }
+        RenderBlock* firstLineBlock = rendererForFirstLineStyle.firstLineBlock();
+        if (!firstLineBlock)
+            return nullptr;
+        auto* firstLineStyle = firstLineBlock->getCachedPseudoStyle(FIRST_LINE, &style());
+        if (!firstLineStyle)
+            return nullptr;
+        return RenderStyle::clonePtr(*firstLineStyle);
     }
 
-    return &style();
+    if (rendererForFirstLineStyle.isAnonymous() || !rendererForFirstLineStyle.isRenderInline())
+        return nullptr;
+
+    auto& parentStyle = rendererForFirstLineStyle.parent()->firstLineStyle();
+    if (&parentStyle == &rendererForFirstLineStyle.parent()->style())
+        return nullptr;
+    return rendererForFirstLineStyle.element()->styleResolver().styleForElement(*element(), &parentStyle).renderStyle;
 }
 
 const RenderStyle& RenderElement::firstLineStyle() const
 {
-    return view().usesFirstLineRules() ? *cachedFirstLineStyle() : style();
+    if (!view().usesFirstLineRules())
+        return style();
+
+    if (!m_hasValidCachedFirstLineStyle) {
+        auto firstLineStyle = computeFirstLineStyle();
+        if (firstLineStyle || hasRareData())
+            const_cast<RenderElement&>(*this).ensureRareData().cachedFirstLineStyle = WTFMove(firstLineStyle);
+        m_hasValidCachedFirstLineStyle = true;
+    }
+
+    return (hasRareData() && rareData().cachedFirstLineStyle) ? *rareData().cachedFirstLineStyle : style();
 }
 
 StyleDifference RenderElement::adjustStyleDifference(StyleDifference diff, unsigned contextSensitiveProperties) const
@@ -818,6 +815,16 @@ static inline bool rendererHasBackground(const RenderElement* renderer)
     return renderer && renderer->hasBackground();
 }
 
+void RenderElement::invalidateCachedFirstLineStyle()
+{
+    if (!m_hasValidCachedFirstLineStyle)
+        return;
+    m_hasValidCachedFirstLineStyle = false;
+    // Invalidate the subtree as descendant's first line style may depend on ancestor's.
+    for (auto& descendant : descendantsOfType<RenderElement>(*this))
+        descendant.m_hasValidCachedFirstLineStyle = false;
+}
+
 void RenderElement::styleWillChange(StyleDifference diff, const RenderStyle& newStyle)
 {
     auto* oldStyle = hasInitializedStyle() ? &style() : nullptr;
@@ -877,6 +884,9 @@ void RenderElement::styleWillChange(StyleDifference diff, const RenderStyle& new
             setFloating(false);
             clearPositionedState();
         }
+        if (newStyle.hasPseudoStyle(FIRST_LINE) || oldStyle->hasPseudoStyle(FIRST_LINE))
+            invalidateCachedFirstLineStyle();
+
         setHorizontalWritingMode(true);
         setHasVisibleBoxDecorations(false);
         setHasOverflowClip(false);
@@ -1545,12 +1555,7 @@ std::unique_ptr<RenderStyle> RenderElement::getUncachedPseudoStyle(const PseudoS
 
     auto& styleResolver = element()->styleResolver();
 
-    std::unique_ptr<RenderStyle> style;
-    if (pseudoStyleRequest.pseudoId == FIRST_LINE_INHERITED) {
-        style = styleResolver.styleForElement(*element(), parentStyle).renderStyle;
-        style->setStyleType(FIRST_LINE_INHERITED);
-    } else
-        style = styleResolver.pseudoStyleForElement(*element(), pseudoStyleRequest, *parentStyle);
+    std::unique_ptr<RenderStyle> style = styleResolver.pseudoStyleForElement(*element(), pseudoStyleRequest, *parentStyle);
 
     if (style)
         Style::loadPendingResources(*style, document(), element());
index 38d4bec..230c1c7 100644 (file)
@@ -131,10 +131,6 @@ public:
     // Return the renderer whose background style is used to paint the root background. Should only be called on the renderer for which isDocumentElementRenderer() is true.
     RenderElement& rendererForRootBackground();
 
-    // Used only by Element::pseudoStyleCacheIsInvalid to get a first line style based off of a
-    // given new style, without accessing the cache.
-    std::unique_ptr<RenderStyle> uncachedFirstLineStyle(RenderStyle*) const;
-
     // Updates only the local style ptr of the object. Does not update the state of the object,
     // and so only should be called when the style is known not to have changed (or from setStyle).
     void setStyleInternal(RenderStyle&& style) { m_style = WTFMove(style); }
@@ -306,7 +302,8 @@ private:
     void updateShapeImage(const ShapeValue*, const ShapeValue*);
 
     StyleDifference adjustStyleDifference(StyleDifference, unsigned contextSensitiveProperties) const;
-    const RenderStyle* cachedFirstLineStyle() const;
+    std::unique_ptr<RenderStyle> computeFirstLineStyle() const;
+    void invalidateCachedFirstLineStyle();
 
     void newImageAnimationFrameAvailable(CachedImage&) final;
 
@@ -328,6 +325,7 @@ private:
     unsigned m_hasCounterNodeMap : 1;
     unsigned m_isCSSAnimating : 1;
     unsigned m_hasContinuation : 1;
+    mutable unsigned m_hasValidCachedFirstLineStyle : 1;
 
     unsigned m_renderBlockHasMarginBeforeQuirk : 1;
     unsigned m_renderBlockHasMarginAfterQuirk : 1;
index 0bcb5d2..61efd14 100644 (file)
@@ -1978,24 +1978,22 @@ void RenderObject::setVisibleInViewportState(VisibleInViewportState visible)
         ensureRareData().setVisibleInViewportState(visible);
 }
 
-RenderObject::RareDataHash& RenderObject::rareDataMap()
+RenderObject::RareDataMap& RenderObject::rareDataMap()
 {
-    static NeverDestroyed<RareDataHash> map;
+    static NeverDestroyed<RareDataMap> map;
     return map;
 }
 
-RenderObject::RenderObjectRareData RenderObject::rareData() const
+const RenderObject::RenderObjectRareData& RenderObject::rareData() const
 {
-    if (!hasRareData())
-        return RenderObjectRareData();
-
-    return rareDataMap().get(this);
+    ASSERT(hasRareData());
+    return *rareDataMap().get(this);
 }
 
 RenderObject::RenderObjectRareData& RenderObject::ensureRareData()
 {
     setHasRareData(true);
-    return rareDataMap().add(this, RenderObjectRareData()).iterator->value;
+    return *rareDataMap().ensure(this, [] { return std::make_unique<RenderObjectRareData>(); }).iterator->value;
 }
 
 void RenderObject::removeRareData()
index 2647509..024f30a 100644 (file)
@@ -970,6 +970,7 @@ private:
 
     RenderObjectBitfields m_bitfields;
 
+    // FIXME: This should be RenderElementRareData.
     class RenderObjectRareData {
     public:
         RenderObjectRareData()
@@ -989,17 +990,16 @@ private:
         // From RenderElement
         ADD_BOOLEAN_BITFIELD(isRegisteredForVisibleInViewportCallback, IsRegisteredForVisibleInViewportCallback);
         ADD_ENUM_BITFIELD(visibleInViewportState, VisibleInViewportState, VisibleInViewportState, 2);
-
+        std::unique_ptr<RenderStyle> cachedFirstLineStyle;
     };
     
-    RenderObjectRareData rareData() const;
+    const RenderObject::RenderObjectRareData& rareData() const;
     RenderObjectRareData& ensureRareData();
     void removeRareData();
     
-    // Note: RenderObjectRareData is stored by value.
-    typedef HashMap<const RenderObject*, RenderObjectRareData> RareDataHash;
+    typedef HashMap<const RenderObject*, std::unique_ptr<RenderObjectRareData>> RareDataMap;
 
-    static RareDataHash& rareDataMap();
+    static RareDataMap& rareDataMap();
 
 #undef ADD_BOOLEAN_BITFIELD
 };
index 037189a..9ae9a19 100644 (file)
@@ -759,7 +759,22 @@ bool RenderStyle::changeRequiresLayout(const RenderStyle& other, unsigned& chang
                 return true;
         }
     }
-    
+
+    bool hasFirstLineStyle = hasPseudoStyle(FIRST_LINE);
+    if (hasFirstLineStyle != other.hasPseudoStyle(FIRST_LINE))
+        return true;
+    if (hasFirstLineStyle) {
+        auto* firstLineStyle = getCachedPseudoStyle(FIRST_LINE);
+        if (!firstLineStyle)
+            return true;
+        auto* otherFirstLineStyle = other.getCachedPseudoStyle(FIRST_LINE);
+        if (!otherFirstLineStyle)
+            return true;
+        // FIXME: Not all first line style changes actually need layout.
+        if (*firstLineStyle != *otherFirstLineStyle)
+            return true;
+    }
+
     return false;
 }
 
index 24f8008..95d348b 100644 (file)
@@ -77,7 +77,7 @@ enum StyleDifferenceContextSensitiveProperty {
 // Static pseudo styles. Dynamic ones are produced on the fly.
 enum PseudoId : unsigned char {
     // The order must be NOP ID, public IDs, and then internal IDs.
-    NOPSEUDO, FIRST_LINE, FIRST_LETTER, BEFORE, AFTER, SELECTION, FIRST_LINE_INHERITED, SCROLLBAR,
+    NOPSEUDO, FIRST_LINE, FIRST_LETTER, BEFORE, AFTER, SELECTION, SCROLLBAR,
     // Internal IDs follow:
     SCROLLBAR_THUMB, SCROLLBAR_BUTTON, SCROLLBAR_TRACK, SCROLLBAR_TRACK_PIECE, SCROLLBAR_CORNER, RESIZER,
     AFTER_LAST_INTERNAL_PSEUDOID,
index 51c693c..08b63e3 100644 (file)
@@ -235,24 +235,12 @@ static bool pseudoStyleCacheIsInvalid(RenderElement* renderer, RenderStyle* newS
         return false;
 
     for (auto& cache : *pseudoStyleCache) {
-        std::unique_ptr<RenderStyle> newPseudoStyle;
         PseudoId pseudoId = cache->styleType();
-        if (pseudoId == FIRST_LINE || pseudoId == FIRST_LINE_INHERITED)
-            newPseudoStyle = renderer->uncachedFirstLineStyle(newStyle);
-        else
-            newPseudoStyle = renderer->getUncachedPseudoStyle(PseudoStyleRequest(pseudoId), newStyle, newStyle);
+        std::unique_ptr<RenderStyle> newPseudoStyle = renderer->getUncachedPseudoStyle(PseudoStyleRequest(pseudoId), newStyle, newStyle);
         if (!newPseudoStyle)
             return true;
         if (*newPseudoStyle != *cache) {
-            if (pseudoId < FIRST_INTERNAL_PSEUDOID)
-                newStyle->setHasPseudoStyle(pseudoId);
             newStyle->addCachedPseudoStyle(WTFMove(newPseudoStyle));
-            if (pseudoId == FIRST_LINE || pseudoId == FIRST_LINE_INHERITED) {
-                // FIXME: We should do an actual diff to determine whether a repaint vs. layout
-                // is needed, but for now just assume a layout will be required. The diff code
-                // in RenderObject::setStyle would need to be factored out so that it could be reused.
-                renderer->setNeedsLayoutAndPrefWidthsRecalc();
-            }
             return true;
         }
     }