Bug 88222 – Improve the performance of pushScope in StyleResolver

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Takashi Sakamoto]

As StyleResolver::setupScopeStack has a bug that m_scopeStackParent is always set to be NULL, setupScopeStack is always invoked if scoped author styles exist.

[Takashi Sakamoto]

Created attachment 145558 [details]
Patch

[Hajime Morrita]

Comment on attachment 145558 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145558&action=review

> Source/WebCore/ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).

The "Reviewed By" line should be jut after the bug URL. See others.

> Source/WebCore/css/StyleResolver.cpp:563
>      for (; parent; parent = parent->parentOrHostNode()) {

It would be better to introduce new local variable for this loop instead of reusing the parameter.
It's generally a bad style to reuse it and that is a source of this bug.

> Source/WebCore/css/StyleResolver.cpp:597
> +            const ScopeStackFrame& lastFrame = m_scopeStack[m_scopeStack.size() - 1];

Vector::last() is preferred. Then it's easy to pack the conditional into the single if clause.
Also, why do we need isEmpty() check?

[Takashi Sakamoto]

Created attachment 146251 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #2)
> (From update of attachment 145558 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145558&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Reviewed by NOBODY (OOPS!).
> 
> The "Reviewed By" line should be jut after the bug URL. See others.

Done. However, I think, this is the default order generated by prepare-ChangeLog script.

> > Source/WebCore/css/StyleResolver.cpp:563
> >      for (; parent; parent = parent->parentOrHostNode()) {
> 
> It would be better to introduce new local variable for this loop instead of reusing the parameter.
> It's generally a bad style to reuse it and that is a source of this bug.

I see. Done.

> > Source/WebCore/css/StyleResolver.cpp:597
> > +            const ScopeStackFrame& lastFrame = m_scopeStack[m_scopeStack.size() - 1];
> 
> Vector::last() is preferred. Then it's easy to pack the conditional into the single if clause.

Done.

> Also, why do we need isEmpty() check?

Yes, we need. Because pushScope doesn't add any ruleset if a scope doesn't have any rules, but m_scopeStackParent is always updated to avoid ruleset recalculation.

And I also tested whether we could remove isEmpty() check or not and obtained many crash logs.

Best regards,
Takashi Sakamoto

[Hajime Morrita]

Comment on attachment 146251 [details]
Patch

Okay, let's land this.

[WebKit Review Bot]

Comment on attachment 146251 [details]
Patch

Clearing flags on attachment: 146251

Committed r119782: <http://trac.webkit.org/changeset/119782>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.