[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