Bug 73190

Created attachment 116704 [details] patch (requires 67718, 67790)

Comment on attachment 116704 [details] patch (requires 67718, 67790) Attachment 116704 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10671138

Comment on attachment 116704 [details] patch (requires 67718, 67790) Attachment 116704 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10677092

Comment on attachment 116704 [details] patch (requires 67718, 67790) Attachment 116704 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10676181

Comment on attachment 116704 [details] patch (requires 67718, 67790) View in context: https://bugs.webkit.org/attachment.cgi?id=116704&action=review > Source/WebCore/css/CSSStyleSelector.cpp:273 > +class ScopedRuleSets { > + WTF_MAKE_NONCOPYABLE(ScopedRuleSets); This class adds little value over the HashMap (a place to hang collectFeatures function essentially). I think you should go with a typedef instead. > Source/WebCore/css/CSSStyleSelector.cpp:364 > +// Determine the scope of a style sheet (if any). Returns 'false' if it's a scoped style sheet in an invalid location. > +static bool determineScope(CSSStyleSheet* sheet, const Element*& scope) This could be a member of the CSSStyleSheet. You should get rid of the boolean parameter, the function should never be invoked in cases it would be false. > Source/WebCore/css/CSSStyleSelector.cpp:381 > + Document* document = styleElement->document(); > + if (!document) > + return false; This can never happen. > Source/WebCore/css/CSSStyleSelector.cpp:487 > + if (sheet->isCSSStyleSheet() && !sheet->disabled()) { > + CSSStyleSheet* cssSheet = static_cast<CSSStyleSheet*>(sheet); > + const Element* scope = 0; > + > + if (!determineScope(cssSheet, scope)) > + continue; > + if (scope) > + m_scopedAuthorStyles->ensureExists(scope)->addRulesFromSheet(cssSheet, *m_medium, this, scope); > + else > + m_authorStyle->addRulesFromSheet(cssSheet, *m_medium, this); > + } There doesn't seem to be any code for handling dynamic addition and removal of scoped stylesheets. I suppose you are currently recalculating the entire style selector (and so the entire document style) if scoped sheets change? This may be ok initially but you should be careful that the architecture can handle dynamic changes efficiently. As I understand use cases for scoped stylesheet may often be dynamic and the style recalc should be limited to the scope only. > Source/WebCore/css/CSSStyleSelector.cpp:712 > + if (m_checker.parentStackIsConsistent(m_parentNode)) { > + // Fast branch: access enclosing scopes directly from parent stack (but start with current element). > + if (m_element->hasScopedHTMLStyleChild()) { > + RuleSet* ruleSet = m_scopedAuthorStyles->get(m_element); > + if (ruleSet) > + matchRuleSet(ruleSet, firstRuleIndex, lastRuleIndex, includeEmptyRules); > + } > + int scopeIndex = m_checker.getInnermostStyleScopeElementIndex(); > + while (scopeIndex >= 0) { > + const Element* scope = m_checker.getElementWithIndex(scopeIndex); > + RuleSet* ruleSet = m_scopedAuthorStyles->get(scope); > + if (ruleSet) > + matchRuleSet(ruleSet, firstRuleIndex, lastRuleIndex, includeEmptyRules); > + scopeIndex = m_checker.getEnclosingStyleScopeElementIndex(scopeIndex); > + } > + } else { > + // Slow branch: climb tree, check for style scopes at every element. > + for (const Node* node = m_element; node; node = node->parentNode()) { > + if (!node->isElementNode()) > + continue; > + const Element* element = toElement(node); > + if (!element->hasScopedHTMLStyleChild()) > + continue; > + RuleSet* ruleSet = m_scopedAuthorStyles->get(element); > + if (ruleSet) > + matchRuleSet(ruleSet, firstRuleIndex, lastRuleIndex, includeEmptyRules); > + } > + } You should have a separate function that determines the active RuleSets (as a Vector probably). In this function you should just run through them and do the matching. > Source/WebCore/css/CSSStyleSelector.cpp:850 > + // If we didn't match any rules, we're done. Unnecessary comment. > Source/WebCore/css/CSSStyleSelector.cpp:854 > + // Sort the set of matched rules. Unnecessary comment. > Source/WebCore/css/CSSStyleSelector.cpp:864 > + m_ruleList->append(m_matchedRules[i]->rule()); > + } > + } else { Use early return instead. > Source/WebCore/css/CSSStyleSelector.cpp:1014 > + if (currentNode->isElementNode() && toElement(currentNode)->hasScopedHTMLStyleChild()) > + continue; > if (currentNode->renderStyle() == parentStyle && currentNode->lastChild()) { I think this test is unnecessary. currentNode->renderStyle() == parentStyle can't be true in that case. > Source/WebCore/css/CSSStyleSelector.cpp:1828 > - if (m_checker.checkSelector(s, e)) > + // FIXME (BUG 72472): We don't add @-webkit-region rules of scoped style sheets for the moment, > + // so all region rules are global by default. Verify whether that can stand or needs changin'. > + if (m_checker.checkSelector(s, e, 0)) Extra parameter here. > Source/WebCore/css/CSSStyleSelector.cpp:2069 > -void RuleSet::addRulesFromSheet(CSSStyleSheet* sheet, const MediaQueryEvaluator& medium, CSSStyleSelector* styleSelector) > +void RuleSet::addRulesFromSheet(CSSStyleSheet* sheet, const MediaQueryEvaluator& medium, CSSStyleSelector* styleSelector, const Element* scope) You should probably add scope field to RuleSet instead of passing it as parameter. That will be useful for scoped matching in the future I think. > Source/WebCore/css/CSSStyleSelector.cpp:2111 > + // > + // FIXME(BUG 72461): We don't add @font-face rules of scoped style sheets for the moment. > + // Find a way to have scoped @font-face rules. > + if (!scope) { Early return style with continue might look slightly nicer. Lose the second line of the comment, it adds no value. Same comments for other instances in this function. > Source/WebCore/css/CSSStyleSelector.cpp:2141 > + } else if (rule->isRegionStyleRule() && styleSelector) { > + // FIXME (BUG 72472): We don't add @-webkit-region rules of scoped style sheets for the moment. > styleSelector->addRegionStyleRule(static_cast<CSSRegionStyleRule*>(rule)); There is no code change along with this comment. > Source/WebCore/css/CSSStyleSelector.cpp:2259 > +RuleSet* ScopedRuleSets::ensureExists(const Element* scope) > +{ > + RuleSet* ruleSet = m_scopedRuleSetMap.get(scope); > + if (!ruleSet) { > + ruleSet = new RuleSet; > + m_scopedRuleSetMap.set(scope, ruleSet); > + } > + return ruleSet; > +} You can do this with a single hash lookup using add(). > Source/WebCore/css/SelectorChecker.h:97 > + // The following methods work only with a consistent ParentStackFrame vector > + Element* getElementWithIndex(int index) const; // Index elements, with the root element starting at 0. > + int getInnermostStyleScopeElementIndex() const; // Return the index of the element that contains the closest <style scoped> > + int getEnclosingStyleScopeElementIndex(int index); // Return the index of the closest ancestor (!) element that contains <style scoped>, starting from the given index WebKit coding style does not use get* in accessors. The interface is generally strange (but see the next comment about getting rid of it entirely). > Source/WebCore/css/SelectorChecker.h:125 > struct ParentStackFrame { > - ParentStackFrame() { } > - ParentStackFrame(Element* element) : element(element) { } > - Element* element; > + ParentStackFrame(Element* element, size_t styleScopeFrameIndex) : element(element), closestStyleScopeFrameIndex(styleScopeFrameIndex) { } > + Element* const element; > + const int closestStyleScopeFrameIndex; // index of closest ParentStackFrame that points to an element that is a style scope (may be this), or -1 > Vector<unsigned, 4> identifierHashes; > }; It would be better to have a separate scope stack. I see no obvious benefit from having it part of the ParentStackFrame and doing the index dance here. The stack would better live in CSSStyleSelector too. Then you could save RuleSet pointers there along with the scope element and save lots of hash lookups.

(In reply to comment #5) I'm currently working on an updated patch - just a few clarification questions below: > (From update of attachment 116704 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116704&action=review > > > Source/WebCore/css/CSSStyleSelector.cpp:273 > > +class ScopedRuleSets { > > + WTF_MAKE_NONCOPYABLE(ScopedRuleSets); > > This class adds little value over the HashMap (a place to hang collectFeatures function essentially). I think you should go with a typedef instead. I'd need it at least for the destructor (?). > There doesn't seem to be any code for handling dynamic addition and removal of scoped stylesheets. I suppose you are currently recalculating the entire style selector (and so the entire document style) if scoped sheets change? The code for handling of dynamic addition and removal, as well as the 'scoped' attribute is in the patch for bug 67790. > This may be ok initially but you should be careful that the architecture can handle dynamic changes efficiently. As I understand use cases for scoped stylesheet may often be dynamic and the style recalc should be limited to the scope only. Yes, that would be a good optimization, but is only feasible for adding/removing of entire <style scoped> elements, not when changing the 'scoped' attribute (however, we should continue discussion of this at 67790). > > Source/WebCore/css/CSSStyleSelector.cpp:712 > [...snip matchAuthorRules code...] > You should have a separate function that determines the active RuleSets (as a Vector probably). In this function you should just run through them and do the matching. Given that matchAuthorRules would be the only client of this, wouldn't creation of the vector be unnecessary overhead? > > Source/WebCore/css/CSSStyleSelector.cpp:1014 > > + if (currentNode->isElementNode() && toElement(currentNode)->hasScopedHTMLStyleChild()) > > + continue; > > if (currentNode->renderStyle() == parentStyle && currentNode->lastChild()) { > > I think this test is unnecessary. currentNode->renderStyle() == parentStyle can't be true in that case. Not sure I understand - this line is not by me... ^_^; > > Source/WebCore/css/CSSStyleSelector.cpp:1828 > > - if (m_checker.checkSelector(s, e)) > > + // FIXME (BUG 72472): We don't add @-webkit-region rules of scoped style sheets for the moment, > > + // so all region rules are global by default. Verify whether that can stand or needs changin'. > > + if (m_checker.checkSelector(s, e, 0)) > > Extra parameter here. Good catch, thanks!! > > Source/WebCore/css/CSSStyleSelector.cpp:2069 > > -void RuleSet::addRulesFromSheet(CSSStyleSheet* sheet, const MediaQueryEvaluator& medium, CSSStyleSelector* styleSelector) > > +void RuleSet::addRulesFromSheet(CSSStyleSheet* sheet, const MediaQueryEvaluator& medium, CSSStyleSelector* styleSelector, const Element* scope) > > You should probably add scope field to RuleSet instead of passing it as parameter. That will be useful for scoped matching in the future I think. As I have implemented it so far, RuleSets (and RuleData, etc.) don't store the scope, and thus don't need to be updated themselves on DOM changes. The scope is just passed in as a parameter to the scoping functions (which will be later handy for queryScopedSelector/find/matches/... as well). > > Source/WebCore/css/CSSStyleSelector.cpp:2259 > > +RuleSet* ScopedRuleSets::ensureExists(const Element* scope) > > +{ > > + RuleSet* ruleSet = m_scopedRuleSetMap.get(scope); > > + if (!ruleSet) { > > + ruleSet = new RuleSet; > > + m_scopedRuleSetMap.set(scope, ruleSet); > > + } > > + return ruleSet; > > +} > > You can do this with a single hash lookup using add(). But that would create and potentially discard a complete RuleSet. One way to improve efficiency here could be to have ScopedRuleSets pre-allocate a spare RuleSet that is used with add() and - if add() actually uses it, pre-allocating a new spare. Another, more generic, way would be to add a function to HashMap that only takes a key and allocates a new value type via default constructor if necessary. What do you think? > > Source/WebCore/css/SelectorChecker.h:125 > > struct ParentStackFrame { > > - ParentStackFrame() { } > > - ParentStackFrame(Element* element) : element(element) { } > > - Element* element; > > + ParentStackFrame(Element* element, size_t styleScopeFrameIndex) : element(element), closestStyleScopeFrameIndex(styleScopeFrameIndex) { } > > + Element* const element; > > + const int closestStyleScopeFrameIndex; // index of closest ParentStackFrame that points to an element that is a style scope (may be this), or -1 > > Vector<unsigned, 4> identifierHashes; > > }; > > It would be better to have a separate scope stack. I see no obvious benefit from having it part of the ParentStackFrame and doing the index dance here. The stack would better live in CSSStyleSelector too. Then you could save RuleSet pointers there along with the scope element and save lots of hash lookups. Hm, I am implementing this as suggested at the moment, but this has the drawback that AFAICT I would need to replicate the logic in SelectorChecker::pushParent(), which strikes me as quite fragile (?). I do agree however, that the current implementation with the "index dance" is strange and should be replaced by something better.

(In reply to comment #6) > But that would create and potentially discard a complete RuleSet. One way to improve efficiency here could be to have ScopedRuleSets pre-allocate a spare RuleSet that is used with add() and - if add() actually uses it, pre-allocating a new spare. Another, more generic, way would be to add a function to HashMap that only takes a key and allocates a new value type via default constructor if necessary. Argh, first think then type! Given that this uses a pointer, the above is obviously bogus... >_<

Created attachment 116935 [details] work-in-progress Work-in-progress patch - fixed review requests, but some open questions (see above) and also needs some more debuggin'.

(In reply to comment #6) > I'd need it at least for the destructor (?). True, that somewhat motivates the class at the moment. However, we should make RuleSets refcounted if they are going to be more dynamic. That takes care of that. > Yes, that would be a good optimization, but is only feasible for adding/removing of entire <style scoped> elements That's the case I meant. > Given that matchAuthorRules would be the only client of this, wouldn't creation of the vector be unnecessary overhead? Doubt it is something to worry about. Plus it opens avenues for caching. Note also that if you implement the separate scope stack, the stack _is_ the vector here so you don't need to do anything in the common case. > > > Source/WebCore/css/CSSStyleSelector.cpp:1014 > > > + if (currentNode->isElementNode() && toElement(currentNode)->hasScopedHTMLStyleChild()) > > > + continue; > > > if (currentNode->renderStyle() == parentStyle && currentNode->lastChild()) { > > > > I think this test is unnecessary. currentNode->renderStyle() == parentStyle can't be true in that case. > > Not sure I understand - this line is not by me... ^_^; What I meant is that your additional hasScopedHTMLStyleChild() test is probably not necessary here because (currentNode->renderStyle() == parentStyle test will never be true for parents with scoped child (as they have not been allowed to share style in the first place due to other checks). > As I have implemented it so far, RuleSets (and RuleData, etc.) don't store the scope, and thus don't need to be updated themselves on DOM changes. The scope is just passed in as a parameter to the scoping functions (which will be later handy for queryScopedSelector/find/matches/... as well). Scope is kinda instrinsic feature of a RuleSet so it is not something that you should try hard to avoid. > But that would create and potentially discard a complete RuleSet. One way to improve efficiency here could be to have ScopedRuleSets pre-allocate a spare RuleSet that is used with add() and - if add() actually uses it, pre-allocating a new spare. Another, more generic, way would be to add a function to HashMap that only takes a key and allocates a new value type via default constructor if necessary. See for example http://trac.webkit.org/browser/trunk/Source/WebCore/css/CSSPrimitiveValueCache.cpp#L113 > Hm, I am implementing this as suggested at the moment, but this has the drawback that AFAICT I would need to replicate the logic in SelectorChecker::pushParent(), which strikes me as quite fragile (?). > I do agree however, that the current implementation with the "index dance" is strange and should be replaced by something better. I don't think so. I suspect you should manage with some rather simple logic here. Note that you can still rely on the ancestor identifier stack for consistency checks. (your stack is consistent only as long as that one is).

Created attachment 117130 [details] Patch (requires 67718, 67790)

Comment on attachment 117130 [details] Patch (requires 67718, 67790) Attachment 117130 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10711021

Comment on attachment 117130 [details] Patch (requires 67718, 67790) Attachment 117130 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10704071

(In reply to comment #9) > (In reply to comment #6) > > I'd need it at least for the destructor (?). > > True, that somewhat motivates the class at the moment. However, we should make RuleSets refcounted if they are going to be more dynamic. That takes care of that. I take that should be a separate patch (?). > What I meant is that your additional hasScopedHTMLStyleChild() test is probably not necessary here because (currentNode->renderStyle() == parentStyle test will never be true for parents with scoped child (as they have not been allowed to share style in the first place due to other checks). Got it - removed. > Scope is kinda instrinsic feature of a RuleSet so it is not something that you should try hard to avoid. That's the point - I argue that it's _not_ an intrinsic feature of a RuleSet, but of selector matching only (!). Consider that in a scoped element.find() there is no RuleSet, either. Or when a <style> element from a <template> is being 'stamped" all over the place, as will be the case, e.g., with the planned component model. This view of things will allow all instances to share the same RuleSet, only using different scopes for each instance. > > Hm, I am implementing this as suggested at the moment, but this has the drawback that AFAICT I would need to replicate the logic in SelectorChecker::pushParent(), which strikes me as quite fragile (?). > > I do agree however, that the current implementation with the "index dance" is strange and should be replaced by something better. > > I don't think so. I suspect you should manage with some rather simple logic here. Note that you can still rely on the ancestor identifier stack for consistency checks. (your stack is consistent only as long as that one is). The scoping element stack needs to mirror all 4 cases of SelectorChecker::pushParent: new root, start from the middle of the tree, just push to stack, or inconsistent state. The consistency check you mention only helps with the last case, but the first 3 are hard to distinguish "from the outside" (without replicating the logic - or maybe I'm just overlooking something). In the new patch I refactored the push/popParent logic. While not too terrific (could be considered manipulating private data structures from another class), the function naming should make the intent clear(er).

:P Is there a way to prevent the EWS from auto-running?

Comment on attachment 117130 [details] Patch (requires 67718, 67790) Attachment 117130 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10690066

Comment on attachment 117130 [details] Patch (requires 67718, 67790) It is going to the right direction but I'd still like to see more of my comments addressed. CSSStyleSelector::matchAuthorRules() code for getting the scoped RuleSets should be factored into a function. It would also be nice if the scoping element stack would point to the RuleSets directly instead of inderectly via element and a hash lookip (though it can be done later too). ScopedRuleSets class still doesn't really justify its existence etc.

Created attachment 118198 [details] patch (requires 67790), with flag new patch, removed ScopedRuleSets, improved m_scopingElementStack

Comment on attachment 118198 [details] patch (requires 67790), with flag Attachment 118198 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10753117

Posted a new patch, that now also includes the compile-time flag. (In reply to comment #16) > (From update of attachment 117130 [details]) > It is going to the right direction but I'd still like to see more of my comments addressed. CSSStyleSelector::matchAuthorRules() code for getting the scoped RuleSets should be factored into a function. I have to admit I still don't get the benefit of this: The fast code already goes through the already existent vector as you suggested, and for the slow path to set up a new & different vector, just to then iterate over it seems overhead without benefit. OTOH a function that returns/fills a vector would add overhead in the fast branch, which seems even less wanted. > It would also be nice if the scoping element stack would point to the RuleSets directly instead of inderectly via element and a hash lookip (though it can be done later too). changed to hold a pair<Element*, RuleSet*> - need the Element* as criterion for popParent(), and in the matching functions in follow-up patch for 73192. > ScopedRuleSets class still doesn't really justify its existence etc. removed.

Comment on attachment 118198 [details] patch (requires 67790), with flag Attachment 118198 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10750111

Comment on attachment 118198 [details] patch (requires 67790), with flag Attachment 118198 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10750113

Created attachment 119172 [details] patch (requires 67790), with flag small bugfix update to previous patch

New Year request for review! :)

Created attachment 124058 [details] patch, updated, deconflicted

Created attachment 124078 [details] patch, alternative, using iterator Alternative version of patch: This uses an struct and helper functions to allow iteration over all relevant scoping RuleSets.

Comment on attachment 124078 [details] patch, alternative, using iterator View in context: https://bugs.webkit.org/attachment.cgi?id=124078&action=review > Source/WebCore/css/CSSStyleSelector.cpp:807 > +struct AuthorRuleSetIterator { > + AuthorRuleSetIterator() : m_scope(0), m_ruleSet(0), m_stackIndex(-1) > + { } I think the iterator factored like this is somewhat confusing. It mixes the (common) case of prebuilt m_scopingElementStack with the rare case of it having to be reconstructed. There is no particular reason for it to handle the global author style. (there are also style problems with it not having the normal WebKit iterator interface) As far as I can see you could avoid most of the confusing code by simply rebuilding the parent stack when needed. if (!m_checker.parentStackIsConsistent()) rebuildScopeElementStack(); or similar. Then the scope matching is always just a matter iterating over m_scopingElementStack and the code should be much simpler. We should eventually fix the code so that the stack is always consistent anyway. The same code could be called from pushParent() too when the stack is rebuilt. > Source/WebCore/css/CSSStyleSelector.h:458 > + // Vector (used as stack) that keeps track of scoping elements (i.e., elements with a <style scoped> child) > + // encountered during tree iteration for style resolution. > + Vector<std::pair<const Element*, RuleSet*> > m_scopingElementStack; No need for std::. I prefer a struct over the pair<> as the resulting code reads better.

(In reply to comment #26) > As far as I can see you could avoid most of the confusing code by simply rebuilding the parent stack when needed. > > if (!m_checker.parentStackIsConsistent()) > rebuildScopeElementStack(); > > or similar. Well, there is the comment in SelectorChecker.cpp: // We may get invoked for some random elements in some wacky cases during style resolve. // Pause maintaining the stack in this case. which I took to mean that this (i.e., the slow case) may happen during tree traversal that otherwise uses the fast case. If I clobber the stacks with the new element introduced in the slow case, this will then interfere when the regular tree iteration is resumed, no? It also makes me a bit nervous that m_scopingElementStack needs to be synchronous with SelectorChecker::m_parentStack, but this is not encapsulated - what do you think of moving those into a separate class altogether?

Created attachment 124514 [details] patch, removed iterator Interim version to the previous comment: removed the iterator class, operate on vectors directly.

Created attachment 124525 [details] Patch New patch, as discussed on IRC.

Comment on attachment 124525 [details] Patch r=me

Comment on attachment 124525 [details] Patch Clearing flags on attachment: 124525 Committed r106331: <http://trac.webkit.org/changeset/106331>

All reviewed patches have been landed. Closing bug.