[Webkit-unassigned] [Bug 43783] Make CSS Style Selector non-recursive

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=43783


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #66201|review?                     |review+
               Flag|                            |




--- Comment #13 from Darin Adler <darin at apple.com>  2010-10-13 17:46:13 PST ---
(From update of attachment 66201)
View in context: https://bugs.webkit.org/attachment.cgi?id=66201&action=review

> WebCore/css/CSSStyleSelector.cpp:1922
> +    CallState(State state_, CSSSelector* selector_, Element* element_, bool isSubSelector_, bool encounteredLink_, RenderStyle* elementStyle_, RenderStyle* elementParentStyle_)
> +        : state(state_)
> +        , selector(selector_)
> +        , element(element_)
> +        , isSubSelector(isSubSelector_)
> +        , encounteredLink(encounteredLink_)
> +        , elementStyle(elementStyle_)
> +        , elementParentStyle(elementParentStyle_)
> +    {
> +    }

There is no reason for all those trailing underscores. The C++ language works fine with initializer arguments named the same as the data members, so there’s no reason to have this strangeness.

> WebCore/css/CSSStyleSelector.cpp:1953
> +    // Therefore we have to maintain a call stack by ourselves so that we can check selector iteratively.

This should say "check selectors iteratively", with an "s".

> WebCore/css/CSSStyleSelector.cpp:2064
> +            default:
> +                return false;

It’s unfortunate we have a default case, because including it turns off gcc’s warning that is given when we forget to handle an enum value.

> WebCore/css/CSSStyleSelector.cpp:2070
> +            while (true) {
> +                if (callStack.isEmpty())
> +                    return false;

A more traditional way to write this is:

    while (!callStack.isEmpty()) {
        // loop body
    }
    return false;

That seems better than using an infinite loop.

> WebCore/css/CSSStyleSelector.cpp:2107
> +                default:
> +                    ASSERT_NOT_REACHED();

It’s unfortunate we have a default case, because including it turns off gcc’s warning that is given when we forget to handle an enum value.

> WebCore/css/CSSStyleSelector.h:234
> +            struct CallState;
> +            class CallStack;

I’m not sure these two need to be CSSStyleSelector members. I think it would be better if they were private to the implementation file and not mentioned in the header at all. We could give them slightly more specific names and do that. I guess either way is OK.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.