[Webkit-unassigned] [Bug 88222] Improve the performance of pushScope in StyleResolver

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from Takashi Sakamoto <tasak at google.com>  2012-06-07 04:34:57 PST ---
(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

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