[webkit-reviews] review denied: [Bug 54373] CSS '+' combinator with more than 2 combinations doesn't work : [Attachment 82601] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Leo Yang
<leo.yang at torchmobile.com.cn>'s request for review:
Bug 54373: CSS '+' combinator with more than 2 combinations doesn't work
https://bugs.webkit.org/show_bug.cgi?id=54373

Attachment 82601: Patch
https://bugs.webkit.org/attachment.cgi?id=82601&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=82601&action=review

Good first start.  Don't forget you need to patch checkForSiblingStyleChanges
as well.  Will need a dynamic test to cover that code also.

> Source/WebCore/css/CSSStyleSelector.cpp:2243
> +		   if (e->parentNode() && e->parentNode()->isElementNode()) {
> +		       RenderStyle* parentStyle = elementStyle ?
elementParentStyle : e->parentNode()->renderStyle();
> +		       if (parentStyle)
> +			  
parentStyle->setChildrenAffectedByDirectAdjacentRules();

If you're going to put a bit on the children, then you don't need to set a bit
on the parent any longer.  Just remove this code.

> Source/WebCore/dom/Element.cpp:1101
> +	   if (hasDirectAdjacentRules && n->isElementNode() && n->renderStyle()
&& n->renderStyle()->affectedByDirectAdjacentRules())

You need to be smarter about this.  You're looking for runs of nodes that have
the affected bit set.  For example if a node n has changed, then you want to
recalc all the elements that follow n that are affected by direct adjacent
rules.	You don't need to do this if n hasn't actually changed.  You're doing
too much recalc as a result.

> Source/WebCore/rendering/style/RenderStyle.h:136
> -    unsigned m_childIndex : 21; // Plenty of bits to cache an index.
> +    bool m_affectedByDirectAdjacentRules: 1;
> +    unsigned m_childIndex : 20; // Plenty of bits to cache an index.

Should be able to leave this alone if you just remove the childrenaffected bit.