Changeset 234825 in webkit


Ignore:
Timestamp:
Aug 13, 2018, 4:36:09 PM (7 years ago)
Author:
achristensen@apple.com
Message:

Make CSSSelectorList a little more sane
​https://bugs.webkit.org/show_bug.cgi?id=188539

Reviewed by Simon Fraser.

This patch does four things:

  1. Use a UniqueArray<CSSSelector> instead of a raw pointer and manually calling destructors.
  2. Use move semantics a little bit better.
  3. Add a CSSSelectorList&& to the StyleRule and StyleRulePage because every time we create either

one of those objects we call a setter to give it a CSSSelectorList. That's what constructor arguments are for.

  1. Don't use CSSSelectorList.componentCount(), which iterates all components, to determine if it's empty.

Use first() instead.

  • css/CSSPageRule.cpp:

(WebCore::CSSPageRule::setSelectorText):

  • css/CSSSelectorList.cpp:

(WebCore::CSSSelectorList::CSSSelectorList):
(WebCore::CSSSelectorList::componentCount const):
(WebCore::CSSSelectorList::listSize const):
(WebCore::CSSSelectorList::operator=):
(WebCore::CSSSelectorList::deleteSelectors): Deleted.

  • css/CSSSelectorList.h:

(WebCore::CSSSelectorList::CSSSelectorList):
(WebCore::CSSSelectorList::first const):
(WebCore::CSSSelectorList::indexOfNextSelectorAfter const):
(WebCore::CSSSelectorList::~CSSSelectorList): Deleted.
(WebCore::CSSSelectorList::adoptSelectorArray): Deleted.
(WebCore::CSSSelectorList::hasOneSelector const): Deleted.

  • css/CSSStyleRule.cpp:

(WebCore::CSSStyleRule::setSelectorText):

  • css/StyleRule.cpp:

(WebCore::StyleRule::StyleRule):
(WebCore::StyleRule::createForSplitting):
(WebCore::StyleRulePage::StyleRulePage):

  • css/StyleRule.h:
  • css/parser/CSSParserImpl.cpp:

(WebCore::CSSParserImpl::consumePageRule):
(WebCore::CSSParserImpl::consumeStyleRule):

  • css/parser/CSSSelectorParser.cpp:

(WebCore::CSSSelectorParser::consumePseudo):

Location:
trunk/Source/WebCore
Files:
9 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r234818 r234825  
     12018-08-13  Alex Christensen  <achristensen@webkit.org>
     2
     3        Make CSSSelectorList a little more sane
     4        https://bugs.webkit.org/show_bug.cgi?id=188539
     5
     6        Reviewed by Simon Fraser.
     7
     8        This patch does four things:
     9        1. Use a UniqueArray<CSSSelector> instead of a raw pointer and manually calling destructors.
     10        2. Use move semantics a little bit better.
     11        3. Add a CSSSelectorList&& to the StyleRule and StyleRulePage because every time we create either
     12        one of those objects we call a setter to give it a CSSSelectorList.  That's what constructor arguments are for.
     13        4. Don't use CSSSelectorList.componentCount(), which iterates all components, to determine if it's empty.
     14        Use first() instead.
     15
     16        * css/CSSPageRule.cpp:
     17        (WebCore::CSSPageRule::setSelectorText):
     18        * css/CSSSelectorList.cpp:
     19        (WebCore::CSSSelectorList::CSSSelectorList):
     20        (WebCore::CSSSelectorList::componentCount const):
     21        (WebCore::CSSSelectorList::listSize const):
     22        (WebCore::CSSSelectorList::operator=):
     23        (WebCore::CSSSelectorList::deleteSelectors): Deleted.
     24        * css/CSSSelectorList.h:
     25        (WebCore::CSSSelectorList::CSSSelectorList):
     26        (WebCore::CSSSelectorList::first const):
     27        (WebCore::CSSSelectorList::indexOfNextSelectorAfter const):
     28        (WebCore::CSSSelectorList::~CSSSelectorList): Deleted.
     29        (WebCore::CSSSelectorList::adoptSelectorArray): Deleted.
     30        (WebCore::CSSSelectorList::hasOneSelector const): Deleted.
     31        * css/CSSStyleRule.cpp:
     32        (WebCore::CSSStyleRule::setSelectorText):
     33        * css/StyleRule.cpp:
     34        (WebCore::StyleRule::StyleRule):
     35        (WebCore::StyleRule::createForSplitting):
     36        (WebCore::StyleRulePage::StyleRulePage):
     37        * css/StyleRule.h:
     38        * css/parser/CSSParserImpl.cpp:
     39        (WebCore::CSSParserImpl::consumePageRule):
     40        (WebCore::CSSParserImpl::consumeStyleRule):
     41        * css/parser/CSSSelectorParser.cpp:
     42        (WebCore::CSSSelectorParser::consumePseudo):
     43
    1442018-08-13  Ali Juma  <ajuma@chromium.org>
    245

  • trunk/Source/WebCore/css/CSSPageRule.cpp

    r219237 r234825  
    7878    CSSStyleSheet::RuleMutationScope mutationScope(this);
    7979
    80     m_pageRule->wrapperAdoptSelectorList(selectorList);
     80    m_pageRule->wrapperAdoptSelectorList(WTFMove(selectorList));
    8181}
    8282

  • trunk/Source/WebCore/css/CSSSelectorList.cpp

    r234814 r234825  
    3838    ASSERT_WITH_SECURITY_IMPLICATION(otherComponentCount);
    3939
    40     m_selectorArray = reinterpret_cast<CSSSelector*>(fastMalloc(sizeof(CSSSelector) * otherComponentCount));
     40    m_selectorArray = makeUniqueArray<CSSSelector>(otherComponentCount);
    4141    for (unsigned i = 0; i < otherComponentCount; ++i)
    4242        new (NotNull, &m_selectorArray[i]) CSSSelector(other.m_selectorArray[i]);
    … …  
    4444
    4545CSSSelectorList::CSSSelectorList(CSSSelectorList&& other)
    46     : m_selectorArray(other.m_selectorArray)
     46    : m_selectorArray(WTFMove(other.m_selectorArray))
    4747{
    48     other.m_selectorArray = nullptr;
    4948}
    5049
    … …  
    5958    }
    6059    ASSERT(flattenedSize);
    61     m_selectorArray = reinterpret_cast<CSSSelector*>(fastMalloc(sizeof(CSSSelector) * flattenedSize));
     60    m_selectorArray = makeUniqueArray<CSSSelector>(flattenedSize);
    6261    size_t arrayIndex = 0;
    6362    for (size_t i = 0; i < selectorVector.size(); ++i) {
    … …  
    8887    if (!m_selectorArray)
    8988        return 0;
    90     CSSSelector* current = m_selectorArray;
     89    CSSSelector* current = m_selectorArray.get();
    9190    while (!current->isLastInSelectorList())
    9291        ++current;
    93     return (current - m_selectorArray) + 1;
     92    return (current - m_selectorArray.get()) + 1;
    9493}
    9594
    … …  
    9998        return 0;
    10099    unsigned size = 1;
    101     CSSSelector* current = m_selectorArray;
     100    CSSSelector* current = m_selectorArray.get();
    102101    while (!current->isLastInSelectorList()) {
    103102        if (current->isLastInTagHistory())
    … …  
    110109CSSSelectorList& CSSSelectorList::operator=(CSSSelectorList&& other)
    111110{
    112     deleteSelectors();
    113     m_selectorArray = other.m_selectorArray;
    114     other.m_selectorArray = nullptr;
    115 
     111    m_selectorArray = WTFMove(other.m_selectorArray);
    116112    return *this;
    117 }
    118 
    119 void CSSSelectorList::deleteSelectors()
    120 {
    121     if (!m_selectorArray)
    122         return;
    123 
    124     CSSSelector* selectorArray = m_selectorArray;
    125     m_selectorArray = nullptr;
    126 
    127     bool isLastSelector = false;
    128     for (CSSSelector* s = selectorArray; !isLastSelector; ++s) {
    129         isLastSelector = s->isLastInSelectorList();
    130         s->~CSSSelector();
    131     }
    132     fastFree(selectorArray);
    133113}
    134114

  • trunk/Source/WebCore/css/CSSSelectorList.h

    r234814 r234825  
    2828#include "CSSSelector.h"
    2929#include <memory>
     30#include <wtf/UniqueArray.h>
    3031
    3132namespace WebCore {
    … …  
    3637    WTF_MAKE_FAST_ALLOCATED;
    3738public:
    38     CSSSelectorList() : m_selectorArray(0) { }
     39    CSSSelectorList() = default;
    3940    CSSSelectorList(const CSSSelectorList&);
    4041    CSSSelectorList(CSSSelectorList&&);
    4142    CSSSelectorList(Vector<std::unique_ptr<CSSParserSelector>>&&);
    42 
    43     ~CSSSelectorList() { deleteSelectors(); }
    44 
    45     void adoptSelectorArray(CSSSelector* selectors) { ASSERT(!m_selectorArray); m_selectorArray = selectors; }
     43    CSSSelectorList(UniqueArray<CSSSelector>&& array)
     44        : m_selectorArray(WTFMove(array)) { }
    4645
    4746    bool isValid() const { return !!m_selectorArray; }
    48     const CSSSelector* first() const { return m_selectorArray; }
     47    const CSSSelector* first() const { return m_selectorArray.get(); }
    4948    static const CSSSelector* next(const CSSSelector*);
    50     bool hasOneSelector() const { return m_selectorArray && !next(m_selectorArray); }
    5149    const CSSSelector* selectorAt(size_t index) const { return &m_selectorArray[index]; }
    5250
    … …  
    5755        if (!current)
    5856            return notFound;
    59         return current - m_selectorArray;
     57        return current - m_selectorArray.get();
    6058    }
    6159
    … …  
    7674    // End of a multipart selector is indicated by m_isLastInTagHistory bit in the last item.
    7775    // End of the array is indicated by m_isLastInSelectorList bit in the last item.
    78     CSSSelector* m_selectorArray;
     76    UniqueArray<CSSSelector> m_selectorArray;
    7977};
    8078

  • trunk/Source/WebCore/css/CSSStyleRule.cpp

    r218890 r234825  
    103103    CSSStyleSheet::RuleMutationScope mutationScope(this);
    104104
    105     m_styleRule->wrapperAdoptSelectorList(selectorList);
     105    m_styleRule->wrapperAdoptSelectorList(WTFMove(selectorList));
    106106
    107107    if (hasCachedSelectorText()) {

  • trunk/Source/WebCore/css/StyleRule.cpp

    r228453 r234825  
    182182}
    183183
    184 StyleRule::StyleRule(Ref<StylePropertiesBase>&& properties, bool hasDocumentSecurityOrigin)
     184StyleRule::StyleRule(Ref<StylePropertiesBase>&& properties, bool hasDocumentSecurityOrigin, CSSSelectorList&& selectors)
    185185    : StyleRuleBase(Style, hasDocumentSecurityOrigin)
    186186    , m_properties(WTFMove(properties))
     187    , m_selectorList(WTFMove(selectors))
    187188{
    188189}
    … …  
    214215{
    215216    ASSERT_WITH_SECURITY_IMPLICATION(!selectors.isEmpty());
    216     CSSSelector* selectorListArray = reinterpret_cast<CSSSelector*>(fastMalloc(sizeof(CSSSelector) * selectors.size()));
     217    auto selectorListArray = makeUniqueArray<CSSSelector>(selectors.size());
    217218    for (unsigned i = 0; i < selectors.size(); ++i)
    218219        new (NotNull, &selectorListArray[i]) CSSSelector(*selectors.at(i));
    219220    selectorListArray[selectors.size() - 1].setLastInSelectorList();
    220     auto rule = StyleRule::create(WTFMove(properties), hasDocumentSecurityOrigin);
    221     rule.get().parserAdoptSelectorArray(selectorListArray);
    222     return rule;
     221    return StyleRule::create(WTFMove(properties), hasDocumentSecurityOrigin, WTFMove(selectorListArray));
    223222}
    224223
    … …  
    249248}
    250249
    251 StyleRulePage::StyleRulePage(Ref<StyleProperties>&& properties)
     250StyleRulePage::StyleRulePage(Ref<StyleProperties>&& properties, CSSSelectorList&& selectors)
    252251    : StyleRuleBase(Page)
    253252    , m_properties(WTFMove(properties))
     253    , m_selectorList(WTFMove(selectors))
    254254{
    255255}

  • trunk/Source/WebCore/css/StyleRule.h

    r234814 r234825  
    119119    WTF_MAKE_FAST_ALLOCATED;
    120120public:
    121     static Ref<StyleRule> create(Ref<StylePropertiesBase>&& properties, bool hasDocumentSecurityOrigin)
    122     {
    123         return adoptRef(*new StyleRule(WTFMove(properties), hasDocumentSecurityOrigin));
     121    static Ref<StyleRule> create(Ref<StylePropertiesBase>&& properties, bool hasDocumentSecurityOrigin, CSSSelectorList&& selectors)
     122    {
     123        return adoptRef(*new StyleRule(WTFMove(properties), hasDocumentSecurityOrigin, WTFMove(selectors)));
    124124    }
    125125   
    … …  
    134134    using StyleRuleBase::hasDocumentSecurityOrigin;
    135135
    136     void wrapperAdoptSelectorList(CSSSelectorList& selectors)
     136    void wrapperAdoptSelectorList(CSSSelectorList&& selectors)
    137137    {
    138138        m_selectorList = WTFMove(selectors);
    … …  
    141141#endif
    142142    }
    143     void parserAdoptSelectorArray(CSSSelector* selectors) { m_selectorList.adoptSelectorArray(selectors); }
    144143
    145144    Ref<StyleRule> copy() const { return adoptRef(*new StyleRule(*this)); }
    … …  
    163162
    164163private:
    165     StyleRule(Ref<StylePropertiesBase>&&, bool hasDocumentSecurityOrigin);
     164    StyleRule(Ref<StylePropertiesBase>&&, bool hasDocumentSecurityOrigin, CSSSelectorList&&);
    166165    StyleRule(const StyleRule&);
    167166
    … …  
    201200class StyleRulePage final : public StyleRuleBase {
    202201public:
    203     static Ref<StyleRulePage> create(Ref<StyleProperties>&& properties) { return adoptRef(*new StyleRulePage(WTFMove(properties))); }
     202    static Ref<StyleRulePage> create(Ref<StyleProperties>&& properties, CSSSelectorList&& selectors) { return adoptRef(*new StyleRulePage(WTFMove(properties), WTFMove(selectors))); }
    204203
    205204    ~StyleRulePage();
    … …  
    209208    MutableStyleProperties& mutableProperties();
    210209
    211     void wrapperAdoptSelectorList(CSSSelectorList& selectors) { m_selectorList = WTFMove(selectors); }
     210    void wrapperAdoptSelectorList(CSSSelectorList&& selectors) { m_selectorList = WTFMove(selectors); }
    212211
    213212    Ref<StyleRulePage> copy() const { return adoptRef(*new StyleRulePage(*this)); }
    214213
    215214private:
    216     explicit StyleRulePage(Ref<StyleProperties>&&);
     215    explicit StyleRulePage(Ref<StyleProperties>&&, CSSSelectorList&&);
    217216    StyleRulePage(const StyleRulePage&);
    218217   

  • trunk/Source/WebCore/css/parser/CSSParserImpl.cpp

    r234814 r234825  
    669669    consumeDeclarationList(block, StyleRule::Style);
    670670   
    671     RefPtr<StyleRulePage> page = StyleRulePage::create(createStyleProperties(m_parsedProperties, m_context.mode));
    672     page->wrapperAdoptSelectorList(selectorList);
    673     return page;
     671    return StyleRulePage::create(createStyleProperties(m_parsedProperties, m_context.mode), WTFMove(selectorList));
    674672}
    675673
    … …  
    727725        return nullptr; // Parse error, invalid selector list
    728726
    729     RefPtr<StyleRule> rule;
    730727    if (m_observerWrapper)
    731728        observeSelectors(*m_observerWrapper, prelude);
    … …  
    739736        blockCopy.consumeWhitespace();
    740737        if (!blockCopy.atEnd()) {
    741             rule = StyleRule::create(createDeferredStyleProperties(block), m_context.hasDocumentSecurityOrigin);
    742             rule->wrapperAdoptSelectorList(selectorList);
    743             return rule;
     738            return StyleRule::create(createDeferredStyleProperties(block), m_context.hasDocumentSecurityOrigin, WTFMove(selectorList));
    744739        }
    745740    }
    746741
    747742    consumeDeclarationList(block, StyleRule::Style);
    748     rule = StyleRule::create(createStyleProperties(m_parsedProperties, m_context.mode), m_context.hasDocumentSecurityOrigin);
    749     rule->wrapperAdoptSelectorList(selectorList);
    750     return rule;
     743    return StyleRule::create(createStyleProperties(m_parsedProperties, m_context.mode), m_context.hasDocumentSecurityOrigin, WTFMove(selectorList));
    751744}
    752745

  • trunk/Source/WebCore/css/parser/CSSSelectorParser.cpp

    r234814 r234825  
    531531            std::unique_ptr<CSSSelectorList> selectorList = std::unique_ptr<CSSSelectorList>(new CSSSelectorList());
    532532            *selectorList = consumeComplexSelectorList(block);
    533             if (!selectorList->componentCount() || !block.atEnd())
     533            if (!selectorList->first() || !block.atEnd())
    534534                return nullptr;
    535535            selector->setSelectorList(WTFMove(selectorList));
    … …  
    560560                std::unique_ptr<CSSSelectorList> selectorList = std::unique_ptr<CSSSelectorList>(new CSSSelectorList());
    561561                *selectorList = consumeComplexSelectorList(block);
    562                 if (!selectorList->componentCount() || !block.atEnd())
     562                if (!selectorList->first() || !block.atEnd())
    563563                    return nullptr;
    564564                selector->setSelectorList(WTFMove(selectorList));
    … …  
    578578            std::unique_ptr<CSSSelectorList> selectorList = std::unique_ptr<CSSSelectorList>(new CSSSelectorList());
    579579            *selectorList = consumeComplexSelectorList(block);
    580             if (!selectorList->componentCount() || !block.atEnd())
     580            if (!selectorList->first() || !block.atEnd())
    581581                return nullptr;
    582582            selector->setSelectorList(WTFMove(selectorList));
    … …  
    587587            std::unique_ptr<CSSSelectorList> selectorList = std::unique_ptr<CSSSelectorList>(new CSSSelectorList());
    588588            *selectorList = consumeCompoundSelectorList(block);
    589             if (!selectorList->componentCount() || !block.atEnd())
     589            if (!selectorList->first() || !block.atEnd())
    590590                return nullptr;
    591591            selector->setSelectorList(WTFMove(selectorList));
Note: See TracChangeset for help on using the changeset viewer.