[webkit-reviews] review denied: [Bug 73190] <style scoped>: Implement scoped stylesheets and basic application : [Attachment 116704] patch (requires 67718, 67790)
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Nov 28 04:23:13 PST 2011
Antti Koivisto <koivisto at iki.fi> has denied Roland Steiner
<rolandsteiner at chromium.org>'s request for review:
Bug 73190: <style scoped>: Implement scoped stylesheets and basic application
https://bugs.webkit.org/show_bug.cgi?id=73190
Attachment 116704: patch (requires 67718, 67790)
https://bugs.webkit.org/attachment.cgi?id=116704&action=review
------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
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.
More information about the webkit-reviews
mailing list