From 0af96f055deddc6cd84fa690a2e499ee3659488a Mon Sep 17 00:00:00 2001 From: Elliott Sprehn Date: Thu, 11 Dec 2014 15:34:24 -0800 Subject: [PATCH] Turn StyleSharing to 11. In Sky we can style share if our TreeScopes have the same styles, our :host styles are the same, and we'd inherit the same styles. This allows a lot of simplification to the style sharing logic since we don't need to deal with descendant selectors or tree boundary crossing rules: - We can remove the logic that was checking that we were distributed to the same insertion points since there's no ::content selectors. - We can check the actual inherited values instead of looking at the parentOrSHadowHostNode(). We used to look at the node in Blink because we were checking that you'd get the same descendant selectors applied. In Sky we instead want to make sure you'd inherit the same values. This also means we don't need the element().parentOrShadowHostElement() != parent case in the SharedStyleFinder which was trying to deal with descendant selectors again. I also removed the checks that were redundant with the checks inside supportsStyleSharing() which we always check before adding sharing candidates. Finally by refactoring the code to make the TreeScope style check work it exposed that the Document::styleSheets() and TreeScope::styleSheets() APIs are now dead. A future patch will delete the now dead StyleSheetList class as well. This change makes the city-list application share between all the items in the list, and all of the headers of the same type now share as well. R=ojan@chromium.org Review URL: https://codereview.chromium.org/796713002 --- .../core/css/resolver/SharedStyleFinder.cpp | 46 +++++-------------- engine/core/css/resolver/SharedStyleFinder.h | 2 + engine/core/css/resolver/StyleResolver.cpp | 1 + engine/core/dom/Document.cpp | 9 ---- engine/core/dom/Document.h | 4 -- engine/core/dom/Element.cpp | 9 +--- engine/core/dom/TreeScope.cpp | 19 ++++++++ engine/core/dom/TreeScope.h | 3 +- engine/core/dom/shadow/ElementShadow.cpp | 22 ++------- engine/core/dom/shadow/ShadowRoot.cpp | 11 ----- engine/core/dom/shadow/ShadowRoot.h | 2 - engine/core/dom/shadow/ShadowRootRareData.h | 5 -- 12 files changed, 42 insertions(+), 91 deletions(-) diff --git a/engine/core/css/resolver/SharedStyleFinder.cpp b/engine/core/css/resolver/SharedStyleFinder.cpp index abdb826e085..67f52705bba 100644 --- a/engine/core/css/resolver/SharedStyleFinder.cpp +++ b/engine/core/css/resolver/SharedStyleFinder.cpp @@ -102,60 +102,37 @@ bool SharedStyleFinder::sharingCandidateCanShareHostStyles(Element& candidate) c return elementShadow->hasSameStyles(candidateShadow); } -bool SharedStyleFinder::sharingCandidateDistributedToSameInsertionPoint(Element& candidate) const -{ - Vector, 8> insertionPoints, candidateInsertionPoints; - collectDestinationInsertionPoints(element(), insertionPoints); - collectDestinationInsertionPoints(candidate, candidateInsertionPoints); - if (insertionPoints.size() != candidateInsertionPoints.size()) - return false; - for (size_t i = 0; i < insertionPoints.size(); ++i) { - if (insertionPoints[i] != candidateInsertionPoints[i]) - return false; - } - return true; -} - bool SharedStyleFinder::canShareStyleWithElement(Element& candidate) const { + ASSERT(candidate.supportsStyleSharing()); + if (element() == candidate) return false; - Element* parent = candidate.parentOrShadowHostElement(); + if (candidate.tagQName() != element().tagQName()) + return false; + if (candidate.needsStyleRecalc()) + return false; RenderStyle* style = candidate.renderStyle(); if (!style) return false; if (!style->isSharable()) return false; + ContainerNode* parent = NodeRenderingTraversal::parent(&candidate); if (!parent) return false; - if (element().parentOrShadowHostElement()->renderStyle() != parent->renderStyle()) + RenderStyle* parentStyle = parent->renderStyle(); + if (!parentStyle) return false; - if (candidate.tagQName() != element().tagQName()) - return false; - if (candidate.inlineStyle()) - return false; - if (candidate.needsStyleRecalc()) + if (m_renderingParent->renderStyle()->inheritedNotEqual(parentStyle)) return false; if (!sharingCandidateHasIdenticalStyleAffectingAttributes(candidate)) return false; - if (candidate.hasID() && m_features.hasSelectorForId(candidate.idForStyleResolution())) - return false; if (!sharingCandidateCanShareHostStyles(candidate)) return false; - if (!sharingCandidateDistributedToSameInsertionPoint(candidate)) + if (!candidate.treeScope().hasSameStyles(element().treeScope())) return false; if (candidate.isUnresolvedCustomElement() != element().isUnresolvedCustomElement()) return false; - - if (element().parentOrShadowHostElement() != parent) { - if (!parent->isStyledElement()) - return false; - if (parent->inlineStyle()) - return false; - if (parent->hasID() && m_features.hasSelectorForId(parent->idForStyleResolution())) - return false; - } - return true; } @@ -200,6 +177,7 @@ RenderStyle* SharedStyleFinder::findSharedStyle() // Cache whether context.element() is affected by any known class selectors. m_elementAffectedByClassRules = element().hasClass() && classNamesAffectedByRules(element()); + m_renderingParent = NodeRenderingTraversal::parent(&element()); Element* shareElement = findElementForStyleSharing(); diff --git a/engine/core/css/resolver/SharedStyleFinder.h b/engine/core/css/resolver/SharedStyleFinder.h index 6c602019a27..46602619e11 100644 --- a/engine/core/css/resolver/SharedStyleFinder.h +++ b/engine/core/css/resolver/SharedStyleFinder.h @@ -46,6 +46,7 @@ public: , m_features(features) , m_styleResolver(styleResolver) , m_context(context) + , m_renderingParent(nullptr) { } RenderStyle* findSharedStyle(); @@ -71,6 +72,7 @@ private: const RuleFeatureSet& m_features; StyleResolver& m_styleResolver; const ElementResolveContext& m_context; + ContainerNode* m_renderingParent; }; } // namespace blink diff --git a/engine/core/css/resolver/StyleResolver.cpp b/engine/core/css/resolver/StyleResolver.cpp index 8d9e3d28e60..a46d0e475ad 100644 --- a/engine/core/css/resolver/StyleResolver.cpp +++ b/engine/core/css/resolver/StyleResolver.cpp @@ -261,6 +261,7 @@ void StyleResolver::addToStyleSharingList(Element& element) // otherwise we could leave stale pointers in there. if (!document().inStyleRecalc()) return; + ASSERT(element.supportsStyleSharing()); INCREMENT_STYLE_STATS_COUNTER(*this, sharedStyleCandidates); StyleSharingList& list = styleSharingList(); if (list.size() >= styleSharingListSize) diff --git a/engine/core/dom/Document.cpp b/engine/core/dom/Document.cpp index fac351b476a..1af6efc3124 100644 --- a/engine/core/dom/Document.cpp +++ b/engine/core/dom/Document.cpp @@ -331,8 +331,6 @@ Document::~Document() #endif #if !ENABLE(OILPAN) - if (m_styleSheetList) - m_styleSheetList->detachFromDocument(); if (m_importsController) HTMLImportsController::removeFrom(*this); @@ -1677,13 +1675,6 @@ PassRefPtr Document::cloneDocumentWithoutChildren() return create(DocumentInit(url()).withRegistrationContext(registrationContext())); } -StyleSheetList* Document::styleSheets() -{ - if (!m_styleSheetList) - m_styleSheetList = StyleSheetList::create(this); - return m_styleSheetList.get(); -} - void Document::evaluateMediaQueryListIfNeeded() { if (!m_evaluateMediaQueriesOnStyleRecalc) diff --git a/engine/core/dom/Document.h b/engine/core/dom/Document.h index a407de4d911..b767dbbe811 100644 --- a/engine/core/dom/Document.h +++ b/engine/core/dom/Document.h @@ -233,9 +233,6 @@ public: bool isRenderingReady() const { return haveImportsLoaded(); } bool isScriptExecutionReady() const { return isRenderingReady(); } - // This is a DOM function. - StyleSheetList* styleSheets(); - StyleEngine* styleEngine() { return m_styleEngine.get(); } // Called when one or more stylesheets in the document may have been added, removed, or changed. @@ -719,7 +716,6 @@ private: MutationObserverOptions m_mutationObserverTypes; OwnPtr m_styleEngine; - RefPtr m_styleSheetList; TextLinkColors m_textLinkColors; diff --git a/engine/core/dom/Element.cpp b/engine/core/dom/Element.cpp index dd35d8b8b15..171c4d084b9 100644 --- a/engine/core/dom/Element.cpp +++ b/engine/core/dom/Element.cpp @@ -990,15 +990,8 @@ StyleRecalcChange Element::recalcOwnStyle(StyleRecalcChange change) ASSERT(oldStyle); if (RenderObject* renderer = this->renderer()) { - if (localChange != NoChange) { + if (localChange != NoChange) renderer->setStyle(newStyle.get()); - } else { - // Although no change occurred, we use the new style so that the cousin style sharing code won't get - // fooled into believing this style is the same. - // FIXME: We may be able to remove this hack, see discussion in - // https://codereview.chromium.org/30453002/ - renderer->setStyleInternal(newStyle.get()); - } } if (styleChangeType() >= SubtreeStyleChange) diff --git a/engine/core/dom/TreeScope.cpp b/engine/core/dom/TreeScope.cpp index 446fc244d49..b4cd28df9e8 100644 --- a/engine/core/dom/TreeScope.cpp +++ b/engine/core/dom/TreeScope.cpp @@ -28,6 +28,7 @@ #include "sky/engine/core/dom/TreeScope.h" #include "gen/sky/core/HTMLNames.h" +#include "sky/engine/core/css/StyleSheetList.h" #include "sky/engine/core/css/resolver/ScopedStyleResolver.h" #include "sky/engine/core/dom/ContainerNode.h" #include "sky/engine/core/dom/Document.h" @@ -420,4 +421,22 @@ void TreeScope::setNeedsStyleRecalcForViewportUnits() } } +bool TreeScope::hasSameStyles(TreeScope& other) +{ + if (this == &other) + return true; + const Vector >& list = document().styleEngine()->styleSheetsForStyleSheetList(*this); + const Vector >& otherList = document().styleEngine()->styleSheetsForStyleSheetList(other); + + if (list.size() != otherList.size()) + return false; + + for (size_t i = 0; i < list.size(); i++) { + if (list[i]->contents() != otherList[i]->contents()) + return false; + } + + return true; +} + } // namespace blink diff --git a/engine/core/dom/TreeScope.h b/engine/core/dom/TreeScope.h index 3715293d339..cd8691bae1b 100644 --- a/engine/core/dom/TreeScope.h +++ b/engine/core/dom/TreeScope.h @@ -38,8 +38,8 @@ class DOMSelection; class Document; class Element; class HitTestResult; -class ScopedStyleResolver; class Node; +class ScopedStyleResolver; // A class which inherits both Node and TreeScope must call clearRareData() in its destructor // so that the Node destructor no longer does problematic NodeList cache manipulation in @@ -77,6 +77,7 @@ public: ContainerNode& rootNode() const { return *m_rootNode; } + bool hasSameStyles(TreeScope&); #if !ENABLE(OILPAN) // Nodes belonging to this scope hold guard references - diff --git a/engine/core/dom/shadow/ElementShadow.cpp b/engine/core/dom/shadow/ElementShadow.cpp index 00b8079b99d..c2f60f6379b 100644 --- a/engine/core/dom/shadow/ElementShadow.cpp +++ b/engine/core/dom/shadow/ElementShadow.cpp @@ -208,23 +208,11 @@ bool ElementShadow::hasSameStyles(const ElementShadow* other) const { ShadowRoot* root = m_shadowRoot; ShadowRoot* otherRoot = other->shadowRoot(); - if (root || otherRoot) { - if (!root || !otherRoot) - return false; - - StyleSheetList* list = root->styleSheets(); - StyleSheetList* otherList = otherRoot->styleSheets(); - - if (list->length() != otherList->length()) - return false; - - for (size_t i = 0; i < list->length(); i++) { - if (list->item(i)->contents() != otherList->item(i)->contents()) - return false; - } - } - - return true; + if (!root && !otherRoot) + return true; + if (root && otherRoot) + return root->hasSameStyles(*otherRoot); + return false; } const InsertionPoint* ElementShadow::finalDestinationInsertionPointFor(const Node* key) const diff --git a/engine/core/dom/shadow/ShadowRoot.cpp b/engine/core/dom/shadow/ShadowRoot.cpp index cc325872964..20862cdeea1 100644 --- a/engine/core/dom/shadow/ShadowRoot.cpp +++ b/engine/core/dom/shadow/ShadowRoot.cpp @@ -58,9 +58,6 @@ ShadowRoot::ShadowRoot(Document& document) ShadowRoot::~ShadowRoot() { - if (m_shadowRootRareData && m_shadowRootRareData->styleSheets()) - m_shadowRootRareData->styleSheets()->detachFromDocument(); - document().styleEngine()->didRemoveShadowRoot(this); // We cannot let ContainerNode destructor call willBeDeletedFromDocument() @@ -235,12 +232,4 @@ const Vector >& ShadowRoot::descendantInsertionPoints() return m_shadowRootRareData->descendantInsertionPoints(); } -StyleSheetList* ShadowRoot::styleSheets() -{ - if (!ensureShadowRootRareData()->styleSheets()) - m_shadowRootRareData->setStyleSheets(StyleSheetList::create(this)); - - return m_shadowRootRareData->styleSheets(); -} - } diff --git a/engine/core/dom/shadow/ShadowRoot.h b/engine/core/dom/shadow/ShadowRoot.h index e263f28db57..258e82853df 100644 --- a/engine/core/dom/shadow/ShadowRoot.h +++ b/engine/core/dom/shadow/ShadowRoot.h @@ -87,8 +87,6 @@ public: PassRefPtr cloneNode(bool, ExceptionState&); PassRefPtr cloneNode(ExceptionState& exceptionState) { return cloneNode(true, exceptionState); } - StyleSheetList* styleSheets(); - private: ShadowRoot(Document&); virtual ~ShadowRoot(); diff --git a/engine/core/dom/shadow/ShadowRootRareData.h b/engine/core/dom/shadow/ShadowRootRareData.h index 8063812083a..58fae89df35 100644 --- a/engine/core/dom/shadow/ShadowRootRareData.h +++ b/engine/core/dom/shadow/ShadowRootRareData.h @@ -60,15 +60,10 @@ public: void setDescendantInsertionPoints(Vector >& list) { m_descendantInsertionPoints.swap(list); } void clearDescendantInsertionPoints() { m_descendantInsertionPoints.clear(); } - StyleSheetList* styleSheets() { return m_styleSheetList.get(); } - void setStyleSheets(PassRefPtr styleSheetList) { m_styleSheetList = styleSheetList; } - - private: unsigned m_descendantContentElementCount; unsigned m_childShadowRootCount; Vector > m_descendantInsertionPoints; - RefPtr m_styleSheetList; }; inline void ShadowRootRareData::didAddInsertionPoint(InsertionPoint* point)