Skip to content

Commit

Permalink
[Shadow DOM] Refactoring: invalidateParentDistributionIfNecessary() c…
Browse files Browse the repository at this point in the history
…alls are too intrusive

https://bugs.webkit.org/show_bug.cgi?id=106305

Reviewed by Dimitri Glazkov.

Scattering invalidateParentDistributionIfNecessary() looks bad because
- it has long name whose terminology is cryptic for people who don't know much about Shadow DOM standard.
- its calls are always paired with setNeedsStyleRecalc() and people do setNeedsStyleRecalc()
  need to be aware about distribution feature bit tracking. Separate invalidateParentDistributionIfNecessary()
  call doesn't help that recognition.

This change introduces Element::didAffectSelector() to replace a setNeedsStyleRecalc()-i37y() call sequence.
SelectRuleFeatureSet::FeatureRule is renamed AffectedSelectorType so that it explains its purpose
in a bit more plain WebKit term.

No new tests. Refactoring.

* dom/Document.cpp:
(WebCore::Document::setCSSTarget): Adopted didAffectSelector.
* dom/Element.cpp:
(WebCore::Element::didAffectSelector): Added.
(WebCore):
* dom/Element.h:
(Element):
* dom/ElementShadow.cpp:
(WebCore::ElementShadow::didAffectSelector): Morphed from invalidateParentDistributionIfNecessary().
* dom/ElementShadow.h:
(ElementShadow):
* html/HTMLAnchorElement.cpp:
(WebCore::HTMLAnchorElement::parseAttribute): Adopted didAffectSelector
* html/HTMLDetailsElement.cpp:
* html/HTMLFormControlElement.cpp:
(WebCore::HTMLFormControlElement::disabledAttributeChanged): Adopted didAffectSelector
* html/HTMLInputElement.cpp:
(WebCore::HTMLInputElement::setChecked): Adopted didAffectSelector
(WebCore::HTMLInputElement::setIndeterminate): Adopted didAffectSelector
* html/HTMLOptGroupElement.cpp:
(WebCore::HTMLOptGroupElement::parseAttribute): Adopted didAffectSelector
* html/HTMLOptionElement.cpp:
(WebCore::HTMLOptionElement::parseAttribute): Adopted didAffectSelector
(WebCore::HTMLOptionElement::setSelectedState): Adopted didAffectSelector
* html/HTMLProgressElement.cpp:
(WebCore::HTMLProgressElement::didElementStateChange): Adopted didAffectSelector
* html/HTMLSummaryElement.cpp:
* html/shadow/HTMLContentElement.cpp:
* html/shadow/SelectRuleFeatureSet.cpp:
(WebCore::SelectRuleFeatureSet::collectFeaturesFromSelector): Followed renaming.
* html/shadow/SelectRuleFeatureSet.h: Followed renaming.
(WebCore::SelectRuleFeatureSet::hasSelectorForChecked):
(WebCore::SelectRuleFeatureSet::hasSelectorForEnabled):
(WebCore::SelectRuleFeatureSet::hasSelectorForDisabled):
(WebCore::SelectRuleFeatureSet::hasSelectorForIndeterminate):
(WebCore::SelectRuleFeatureSet::hasSelectorForLink):
(WebCore::SelectRuleFeatureSet::hasSelectorForTarget):
(WebCore::SelectRuleFeatureSet::hasSelectorForVisited):
(WebCore::SelectRuleFeatureSet::hasSelectorFor):
(WebCore::SelectRuleFeatureSet::setSelectRuleFeature):


Canonical link: https://commits.webkit.org/124520@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@139064 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
omo committed Jan 8, 2013
1 parent 3ea9700 commit ef3e9bc
Show file tree
Hide file tree
Showing 17 changed files with 115 additions and 72 deletions.
60 changes: 60 additions & 0 deletions Source/WebCore/ChangeLog
@@ -1,3 +1,63 @@
2013-01-08 Hajime Morrita <morrita@google.com>

[Shadow DOM] Refactoring: invalidateParentDistributionIfNecessary() calls are too intrusive
https://bugs.webkit.org/show_bug.cgi?id=106305

Reviewed by Dimitri Glazkov.

Scattering invalidateParentDistributionIfNecessary() looks bad because
- it has long name whose terminology is cryptic for people who don't know much about Shadow DOM standard.
- its calls are always paired with setNeedsStyleRecalc() and people do setNeedsStyleRecalc()
need to be aware about distribution feature bit tracking. Separate invalidateParentDistributionIfNecessary()
call doesn't help that recognition.

This change introduces Element::didAffectSelector() to replace a setNeedsStyleRecalc()-i37y() call sequence.
SelectRuleFeatureSet::FeatureRule is renamed AffectedSelectorType so that it explains its purpose
in a bit more plain WebKit term.

No new tests. Refactoring.

* dom/Document.cpp:
(WebCore::Document::setCSSTarget): Adopted didAffectSelector.
* dom/Element.cpp:
(WebCore::Element::didAffectSelector): Added.
(WebCore):
* dom/Element.h:
(Element):
* dom/ElementShadow.cpp:
(WebCore::ElementShadow::didAffectSelector): Morphed from invalidateParentDistributionIfNecessary().
* dom/ElementShadow.h:
(ElementShadow):
* html/HTMLAnchorElement.cpp:
(WebCore::HTMLAnchorElement::parseAttribute): Adopted didAffectSelector
* html/HTMLDetailsElement.cpp:
* html/HTMLFormControlElement.cpp:
(WebCore::HTMLFormControlElement::disabledAttributeChanged): Adopted didAffectSelector
* html/HTMLInputElement.cpp:
(WebCore::HTMLInputElement::setChecked): Adopted didAffectSelector
(WebCore::HTMLInputElement::setIndeterminate): Adopted didAffectSelector
* html/HTMLOptGroupElement.cpp:
(WebCore::HTMLOptGroupElement::parseAttribute): Adopted didAffectSelector
* html/HTMLOptionElement.cpp:
(WebCore::HTMLOptionElement::parseAttribute): Adopted didAffectSelector
(WebCore::HTMLOptionElement::setSelectedState): Adopted didAffectSelector
* html/HTMLProgressElement.cpp:
(WebCore::HTMLProgressElement::didElementStateChange): Adopted didAffectSelector
* html/HTMLSummaryElement.cpp:
* html/shadow/HTMLContentElement.cpp:
* html/shadow/SelectRuleFeatureSet.cpp:
(WebCore::SelectRuleFeatureSet::collectFeaturesFromSelector): Followed renaming.
* html/shadow/SelectRuleFeatureSet.h: Followed renaming.
(WebCore::SelectRuleFeatureSet::hasSelectorForChecked):
(WebCore::SelectRuleFeatureSet::hasSelectorForEnabled):
(WebCore::SelectRuleFeatureSet::hasSelectorForDisabled):
(WebCore::SelectRuleFeatureSet::hasSelectorForIndeterminate):
(WebCore::SelectRuleFeatureSet::hasSelectorForLink):
(WebCore::SelectRuleFeatureSet::hasSelectorForTarget):
(WebCore::SelectRuleFeatureSet::hasSelectorForVisited):
(WebCore::SelectRuleFeatureSet::hasSelectorFor):
(WebCore::SelectRuleFeatureSet::setSelectRuleFeature):

2013-01-08 Sergio Villar Senin <svillar@igalia.com>

[Qt] Fix build with --web-audio
Expand Down
12 changes: 4 additions & 8 deletions Source/WebCore/dom/Document.cpp
Expand Up @@ -3438,15 +3438,11 @@ void Document::getFocusableNodes(Vector<RefPtr<Node> >& nodes)

void Document::setCSSTarget(Element* n)
{
if (m_cssTarget) {
m_cssTarget->setNeedsStyleRecalc();
invalidateParentDistributionIfNecessary(m_cssTarget, SelectRuleFeatureSet::RuleFeatureTarget);
}
if (m_cssTarget)
m_cssTarget->didAffectSelector(AffectedSelectorTarget);
m_cssTarget = n;
if (n) {
n->setNeedsStyleRecalc();
invalidateParentDistributionIfNecessary(n, SelectRuleFeatureSet::RuleFeatureTarget);
}
if (n)
n->didAffectSelector(AffectedSelectorTarget);
}

void Document::registerNodeList(LiveNodeListBase* list)
Expand Down
7 changes: 7 additions & 0 deletions Source/WebCore/dom/Element.cpp
Expand Up @@ -1409,6 +1409,13 @@ ElementShadow* Element::ensureShadow()
return data->shadow();
}

void Element::didAffectSelector(AffectedSelectorMask mask)
{
setNeedsStyleRecalc();
if (ElementShadow* elementShadow = shadowOfParentForDistribution(this))
elementShadow->didAffectSelector(mask);
}

PassRefPtr<ShadowRoot> Element::createShadowRoot(ExceptionCode& ec)
{
return ShadowRoot::create(this, ec);
Expand Down
12 changes: 12 additions & 0 deletions Source/WebCore/dom/Element.h
Expand Up @@ -47,6 +47,17 @@ class PseudoElement;
class RenderRegion;
class ShadowRoot;

enum AffectedSelectorType {
AffectedSelectorChecked = 1,
AffectedSelectorEnabled = 1 << 1,
AffectedSelectorDisabled = 1 << 2,
AffectedSelectorIndeterminate = 1 << 3,
AffectedSelectorLink = 1 << 4,
AffectedSelectorTarget = 1 << 5,
AffectedSelectorVisited = 1 << 6
};
typedef int AffectedSelectorMask;

enum SpellcheckAttributeState {
SpellcheckAttributeTrue,
SpellcheckAttributeFalse,
Expand Down Expand Up @@ -272,6 +283,7 @@ class Element : public ContainerNode {
virtual RenderObject* createRenderer(RenderArena*, RenderStyle*);
virtual bool rendererIsNeeded(const NodeRenderingContext&);
void recalcStyle(StyleChange = NoChange);
void didAffectSelector(AffectedSelectorMask);

ElementShadow* shadow() const;
ElementShadow* ensureShadow();
Expand Down
12 changes: 4 additions & 8 deletions Source/WebCore/dom/ElementShadow.cpp
Expand Up @@ -265,15 +265,11 @@ void ElementShadow::reportMemoryUsage(MemoryObjectInfo* memoryObjectInfo) const
info.addMember(m_distributor);
}

void invalidateParentDistributionIfNecessary(Element* element, SelectRuleFeatureSet::SelectRuleFeatureMask updatedFeatures)
void ElementShadow::didAffectSelector(AffectedSelectorMask mask)
{
ElementShadow* elementShadow = shadowOfParentForDistribution(element);
if (!elementShadow)
return;

elementShadow->ensureSelectFeatureSetCollected();
if (elementShadow->selectRuleFeatureSet().hasSelectorFor(updatedFeatures))
elementShadow->invalidateDistribution();
ensureSelectFeatureSetCollected();
if (selectRuleFeatureSet().hasSelectorFor(mask))
invalidateDistribution();
}

} // namespace
4 changes: 1 addition & 3 deletions Source/WebCore/dom/ElementShadow.h
Expand Up @@ -71,14 +71,14 @@ class ElementShadow {
ContentDistributor& distributor();
const ContentDistributor& distributor() const;

void didAffectSelector(AffectedSelectorMask);
bool shouldCollectSelectFeatureSet() const { return m_shouldCollectSelectFeatureSet; }
void setShouldCollectSelectFeatureSet();
void ensureSelectFeatureSetCollected();

const SelectRuleFeatureSet& selectRuleFeatureSet() const;

void reportMemoryUsage(MemoryObjectInfo*) const;

private:
void invalidateDistribution(Element* host);

Expand All @@ -90,8 +90,6 @@ class ElementShadow {
bool m_shouldCollectSelectFeatureSet : 1;
};

void invalidateParentDistributionIfNecessary(Element*, SelectRuleFeatureSet::SelectRuleFeatureMask updatedFeatures);

inline ShadowRoot* ElementShadow::youngestShadowRoot() const
{
return m_shadowRoots.head();
Expand Down
7 changes: 2 additions & 5 deletions Source/WebCore/html/HTMLAnchorElement.cpp
Expand Up @@ -26,7 +26,6 @@

#include "Attribute.h"
#include "DNS.h"
#include "ElementShadow.h"
#include "EventNames.h"
#include "Frame.h"
#include "FrameLoaderClient.h"
Expand Down Expand Up @@ -219,10 +218,8 @@ void HTMLAnchorElement::parseAttribute(const QualifiedName& name, const AtomicSt
if (name == hrefAttr) {
bool wasLink = isLink();
setIsLink(!value.isNull());
if (wasLink != isLink()) {
setNeedsStyleRecalc();
invalidateParentDistributionIfNecessary(this, SelectRuleFeatureSet::RuleFeatureLink | SelectRuleFeatureSet::RuleFeatureVisited | SelectRuleFeatureSet::RuleFeatureEnabled);
}
if (wasLink != isLink())
didAffectSelector(AffectedSelectorLink | AffectedSelectorVisited | AffectedSelectorEnabled);
if (isLink()) {
String parsedURL = stripLeadingAndTrailingHTMLSpaces(value);
if (document()->isDNSPrefetchEnabled()) {
Expand Down
1 change: 0 additions & 1 deletion Source/WebCore/html/HTMLDetailsElement.cpp
Expand Up @@ -22,7 +22,6 @@
#include "HTMLDetailsElement.h"

#if ENABLE(DETAILS_ELEMENT)
#include "ElementShadow.h"
#include "HTMLContentElement.h"
#include "HTMLNames.h"
#include "HTMLSummaryElement.h"
Expand Down
3 changes: 1 addition & 2 deletions Source/WebCore/html/HTMLFormControlElement.cpp
Expand Up @@ -150,8 +150,7 @@ void HTMLFormControlElement::parseAttribute(const QualifiedName& name, const Ato
void HTMLFormControlElement::disabledAttributeChanged()
{
setNeedsWillValidateCheck();
setNeedsStyleRecalc();
invalidateParentDistributionIfNecessary(this, SelectRuleFeatureSet::RuleFeatureDisabled | SelectRuleFeatureSet::RuleFeatureEnabled);
didAffectSelector(AffectedSelectorDisabled | AffectedSelectorEnabled);
if (renderer() && renderer()->style()->hasAppearance())
renderer()->theme()->stateChanged(renderer(), EnabledState);
}
Expand Down
5 changes: 2 additions & 3 deletions Source/WebCore/html/HTMLInputElement.cpp
Expand Up @@ -883,7 +883,7 @@ void HTMLInputElement::setChecked(bool nowChecked, TextFieldEventBehavior eventB
dispatchFormControlChangeEvent();
}

invalidateParentDistributionIfNecessary(this, SelectRuleFeatureSet::RuleFeatureChecked);
didAffectSelector(AffectedSelectorChecked);
}

void HTMLInputElement::setIndeterminate(bool newValue)
Expand All @@ -893,8 +893,7 @@ void HTMLInputElement::setIndeterminate(bool newValue)

m_isIndeterminate = newValue;

setNeedsStyleRecalc();
invalidateParentDistributionIfNecessary(this, SelectRuleFeatureSet::RuleFeatureIndeterminate);
didAffectSelector(AffectedSelectorIndeterminate);

if (renderer() && renderer()->style()->hasAppearance())
renderer()->theme()->stateChanged(renderer(), CheckedState);
Expand Down
3 changes: 1 addition & 2 deletions Source/WebCore/html/HTMLOptGroupElement.cpp
Expand Up @@ -26,7 +26,6 @@
#include "HTMLOptGroupElement.h"

#include "Document.h"
#include "ElementShadow.h"
#include "HTMLNames.h"
#include "HTMLSelectElement.h"
#include "RenderMenuList.h"
Expand Down Expand Up @@ -85,7 +84,7 @@ void HTMLOptGroupElement::parseAttribute(const QualifiedName& name, const Atomic
recalcSelectOptions();

if (name == disabledAttr)
invalidateParentDistributionIfNecessary(this, SelectRuleFeatureSet::RuleFeatureDisabled | SelectRuleFeatureSet::RuleFeatureEnabled);
didAffectSelector(AffectedSelectorDisabled | AffectedSelectorEnabled);
}

void HTMLOptGroupElement::recalcSelectOptions()
Expand Down
7 changes: 2 additions & 5 deletions Source/WebCore/html/HTMLOptionElement.cpp
Expand Up @@ -29,7 +29,6 @@

#include "Attribute.h"
#include "Document.h"
#include "ElementShadow.h"
#include "ExceptionCode.h"
#include "HTMLDataListElement.h"
#include "HTMLNames.h"
Expand Down Expand Up @@ -204,8 +203,7 @@ void HTMLOptionElement::parseAttribute(const QualifiedName& name, const AtomicSt
bool oldDisabled = m_disabled;
m_disabled = !value.isNull();
if (oldDisabled != m_disabled) {
setNeedsStyleRecalc();
invalidateParentDistributionIfNecessary(this, SelectRuleFeatureSet::RuleFeatureDisabled | SelectRuleFeatureSet::RuleFeatureEnabled);
didAffectSelector(AffectedSelectorDisabled | AffectedSelectorEnabled);
if (renderer() && renderer()->style()->hasAppearance())
renderer()->theme()->stateChanged(renderer(), EnabledState);
}
Expand Down Expand Up @@ -258,8 +256,7 @@ void HTMLOptionElement::setSelectedState(bool selected)
return;

m_isSelected = selected;
setNeedsStyleRecalc();
invalidateParentDistributionIfNecessary(this, SelectRuleFeatureSet::RuleFeatureChecked);
didAffectSelector(AffectedSelectorChecked);

if (HTMLSelectElement* select = ownerSelectElement())
select->invalidateSelectedItems();
Expand Down
8 changes: 2 additions & 6 deletions Source/WebCore/html/HTMLProgressElement.cpp
Expand Up @@ -23,7 +23,6 @@

#if ENABLE(PROGRESS_ELEMENT)
#include "Attribute.h"
#include "ElementShadow.h"
#include "EventNames.h"
#include "ExceptionCode.h"
#include "HTMLDivElement.h"
Expand All @@ -32,7 +31,6 @@
#include "NodeRenderingContext.h"
#include "ProgressShadowElement.h"
#include "RenderProgress.h"
#include "SelectRuleFeatureSet.h"
#include "ShadowRoot.h"
#include <wtf/StdLibExtras.h>

Expand Down Expand Up @@ -160,10 +158,8 @@ void HTMLProgressElement::didElementStateChange()
if (RenderProgress* render = renderProgress()) {
bool wasDeterminate = render->isDeterminate();
render->updateFromElement();
if (wasDeterminate != isDeterminate()) {
setNeedsStyleRecalc();
invalidateParentDistributionIfNecessary(this, SelectRuleFeatureSet::RuleFeatureIndeterminate);
}
if (wasDeterminate != isDeterminate())
didAffectSelector(AffectedSelectorIndeterminate);
}
}

Expand Down
1 change: 0 additions & 1 deletion Source/WebCore/html/HTMLSummaryElement.cpp
Expand Up @@ -23,7 +23,6 @@

#if ENABLE(DETAILS_ELEMENT)
#include "DetailsMarkerControl.h"
#include "ElementShadow.h"
#include "HTMLContentElement.h"
#include "HTMLDetailsElement.h"
#include "HTMLNames.h"
Expand Down
1 change: 0 additions & 1 deletion Source/WebCore/html/shadow/HTMLContentElement.cpp
Expand Up @@ -30,7 +30,6 @@
#include "CSSParser.h"
#include "ContentDistributor.h"
#include "ContentSelectorQuery.h"
#include "ElementShadow.h"
#include "HTMLNames.h"
#include "QualifiedName.h"
#include "RuntimeEnabledFeatures.h"
Expand Down
14 changes: 7 additions & 7 deletions Source/WebCore/html/shadow/SelectRuleFeatureSet.cpp
Expand Up @@ -58,25 +58,25 @@ void SelectRuleFeatureSet::collectFeaturesFromSelector(const CSSSelector* select

switch (selector->pseudoType()) {
case CSSSelector::PseudoChecked:
setSelectRuleFeature(RuleFeatureChecked);
setSelectRuleFeature(AffectedSelectorChecked);
break;
case CSSSelector::PseudoEnabled:
setSelectRuleFeature(RuleFeatureEnabled);
setSelectRuleFeature(AffectedSelectorEnabled);
break;
case CSSSelector::PseudoDisabled:
setSelectRuleFeature(RuleFeatureDisabled);
setSelectRuleFeature(AffectedSelectorDisabled);
break;
case CSSSelector::PseudoIndeterminate:
setSelectRuleFeature(RuleFeatureIndeterminate);
setSelectRuleFeature(AffectedSelectorIndeterminate);
break;
case CSSSelector::PseudoLink:
setSelectRuleFeature(RuleFeatureLink);
setSelectRuleFeature(AffectedSelectorLink);
break;
case CSSSelector::PseudoTarget:
setSelectRuleFeature(RuleFeatureTarget);
setSelectRuleFeature(AffectedSelectorTarget);
break;
case CSSSelector::PseudoVisited:
setSelectRuleFeature(RuleFeatureVisited);
setSelectRuleFeature(AffectedSelectorVisited);
break;
default:
break;
Expand Down
30 changes: 10 additions & 20 deletions Source/WebCore/html/shadow/SelectRuleFeatureSet.h
Expand Up @@ -31,6 +31,7 @@
#ifndef SelectRuleFeatureSet_h
#define SelectRuleFeatureSet_h

#include "Element.h"
#include "RuleFeature.h"

namespace WebCore {
Expand All @@ -47,29 +48,18 @@ class SelectRuleFeatureSet {
bool hasSelectorForClass(const AtomicString&) const;
bool hasSelectorForAttribute(const AtomicString&) const;

bool hasSelectorForChecked() const { return hasSelectorFor(RuleFeatureChecked); }
bool hasSelectorForEnabled() const { return hasSelectorFor(RuleFeatureEnabled); }
bool hasSelectorForDisabled() const { return hasSelectorFor(RuleFeatureDisabled); }
bool hasSelectorForIndeterminate() const { return hasSelectorFor(RuleFeatureIndeterminate); }
bool hasSelectorForLink() const { return hasSelectorFor(RuleFeatureLink); }
bool hasSelectorForTarget() const { return hasSelectorFor(RuleFeatureTarget); }
bool hasSelectorForVisited() const { return hasSelectorFor(RuleFeatureVisited); }
bool hasSelectorForChecked() const { return hasSelectorFor(AffectedSelectorChecked); }
bool hasSelectorForEnabled() const { return hasSelectorFor(AffectedSelectorEnabled); }
bool hasSelectorForDisabled() const { return hasSelectorFor(AffectedSelectorDisabled); }
bool hasSelectorForIndeterminate() const { return hasSelectorFor(AffectedSelectorIndeterminate); }
bool hasSelectorForLink() const { return hasSelectorFor(AffectedSelectorLink); }
bool hasSelectorForTarget() const { return hasSelectorFor(AffectedSelectorTarget); }
bool hasSelectorForVisited() const { return hasSelectorFor(AffectedSelectorVisited); }

enum SelectRuleFeature {
RuleFeatureChecked = 1,
RuleFeatureEnabled = 1 << 1,
RuleFeatureDisabled = 1 << 2,
RuleFeatureIndeterminate = 1 << 3,
RuleFeatureLink = 1 << 4,
RuleFeatureTarget = 1 << 5,
RuleFeatureVisited = 1 << 6
};
typedef int SelectRuleFeatureMask;

bool hasSelectorFor(SelectRuleFeatureMask features) const { return m_featureFlags & features; }
bool hasSelectorFor(AffectedSelectorMask features) const { return m_featureFlags & features; }

private:
void setSelectRuleFeature(SelectRuleFeature feature) { m_featureFlags |= feature; }
void setSelectRuleFeature(AffectedSelectorType feature) { m_featureFlags |= feature; }

RuleFeatureSet m_cssRuleFeatureSet;
int m_featureFlags;
Expand Down

0 comments on commit ef3e9bc

Please sign in to comment.