[webkit-reviews] review granted: [Bug 126044] Create a skeleton for CSS Selector code generation : [Attachment 219722] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat Dec 21 14:25:07 PST 2013
Antti Koivisto <koivisto at iki.fi> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 126044: Create a skeleton for CSS Selector code generation
https://bugs.webkit.org/show_bug.cgi?id=126044
Attachment 219722: Patch
https://bugs.webkit.org/attachment.cgi?id=219722&action=review
------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=219722&action=review
Looks great!
> Source/WebCore/cssjit/CSSSelectorCompiler.cpp:200
> + SelectorFragment fragment;
I like SelectorFragment. Maybe we should use the fragment terminology
everywhere.
> Source/WebCore/cssjit/CSSSelectorCompiler.cpp:210
> + switch (selector->m_match) {
> + case CSSSelector::Tag:
> + ASSERT(!fragment.tagName);
> + fragment.tagName = &(selector->tagQName());
> + break;
> + default:
> + goto CannotHandleSelector;
> + }
It would be better to list all match types instead of having a default.
> Source/WebCore/cssjit/CSSSelectorCompiler.cpp:235
> + return;
> +
> +CannotHandleSelector:
> + m_selectorFragments.clear();
I suspect this would read better if you factored the fragment initialisation to
a function. You could avoid clumsy gotos.
> Source/WebCore/cssjit/CSSSelectorCompiler.cpp:693
> + m_stackAllocator.merge(std::move(successStack),
std::move(adjacentTailStack), std::move(descendantTailStack));
> + } else if (fragment.backtrackingFlags &
BacktrackingFlag::DirectAdjacentTail) {
You could do early returns instead of else.
> Source/WebCore/cssjit/CSSSelectorCompiler.h:67
> +namespace CSSSelectorCompiler {
I would prefer SelectorCompiler. It is not ambiguous and we try to use CSS*
prefix to refer to CSSOM types only. Same with CSSSelectorCompilationStatus.
> Source/WebCore/dom/Element.h:377
> + static void setChildrenAffectedByDirectAdjacentRules(Element*);
> void setChildrenAffectedByDirectAdjacentRules() {
setFlag(ChildrenAffectedByDirectAdjacentRulesFlag); }
> - void setChildrenAffectedByForwardPositionalRules();
> + static void setChildrenAffectedByForwardPositionalRules(Element*);
> + void setChildrenAffectedByForwardPositionalRules() {
setChildrenAffectedByForwardPositionalRules(this); }
Should really eliminate the remaining side effects at some point.
More information about the webkit-reviews
mailing list