[webkit-reviews] review granted: [Bug 96421] Split per-resolve logic out from StyleResolver. : [Attachment 185933] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Takashi Sakamoto <tasak at google.com>'s
request for review:
Bug 96421: Split per-resolve logic out from StyleResolver.
https://bugs.webkit.org/show_bug.cgi?id=96421

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=185933&action=review


I do not think that "State"'s eventual destiny is as a struct, but rather as a
class which holds all the useful data about an in-progress element resolve, and
some related logic.  But I also think that this is a great step in the right
direction and we don't have to get everythign right on the first pass. :)  I'm
happy to r+ this, but I Think you should give antti 12 hours to make sure he
doesn't want another round.

> Source/WebCore/css/SVGCSSStyleSelector.cpp:568
> +	       IntPoint location(item->x->computeLength<int>(state.style.get(),
state.rootElementStyle),

This .get() is very unfortunate. :(  But again, we could add an accessor in
another pass.

> Source/WebCore/css/StyleResolver.cpp:980
> +    State& state = m_state;
> +    state.pseudoStyle = pseudoID;

I think this function eventually moves onto State.

> Source/WebCore/css/StyleResolver.cpp:1620
> -    return m_style.release();
> +    return state.style.release();

This will end up being takeStyle(), presumably.

> Source/WebCore/css/StyleResolver.h:481
> +	   // FIXME(bug 108563): to make it easier to review, these member
> +	   // variables are public. However we should add methods to access
> +	   // these variables.

Agreed. :)  As well as move some of the clear(), and init() logic into this
class.

> Source/WebCore/css/StyleResolver.h:497
> +	   bool distributedToInsertionPoint;
> +
> +	   bool elementAffectedByClassRules;

Also in a second pass we should shrink the size of this class by using
bitfields.  It doesn't cost us anything and has some marginal benefit.

> Source/WebCore/css/StyleResolver.h:503
> +	   // A buffer used to hold the set of matched rules for an element,
> +	   // and a temporary buffer used for merge sorting.
> +	   Vector<const RuleData*, 32> matchedRules;

Seems like a rather large internal capacity for this, I wonder how it was
tuned.