[Webkit-unassigned] [Bug 119479] Inserting a JS generated keyframe animation shouldn't trigger a whole document style recalc

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #213201|review?                     |review-
               Flag|                            |




--- Comment #11 from Darin Adler <darin at apple.com>  2013-10-02 16:01:06 PST ---
(From update of attachment 213201)
View in context: https://bugs.webkit.org/attachment.cgi?id=213201&action=review

This seems like a great optimization. I think there is a slightly tighter way to code it.

> Source/WebCore/css/CSSStyleSheet.cpp:175
> +        ASSERT(insertedRule->type() == StyleRuleBase::Keyframes);
> +        StyleResolver* resolver = 0;
> +        if (owner && (resolver = owner->styleResolverIfExists()))
> +            resolver->addKeyframeStyle(static_cast<StyleRuleKeyframes*>(insertedRule));

It’s not helpful to initialize resolver to 0 since that value is never used. And also, this coding style is a bit tortured. It doesn’t really make sense to null-check owner here since we already checked that earlier. I think it should end up like this:

    if (StyleResolver* resolver = owner->styleResolverIfExists())
        return resolver->addKeyframeStyle(insertedRule);

If we did need a null check for owner, I would have suggested an early return.

> Source/WebCore/css/CSSStyleSheet.cpp:427
> +    , m_insertedRule(0)

Please use nullptr instead of 0 in new code.

> Source/WebCore/css/CSSStyleSheet.h:88
> +    enum RuleMutationType { OtherMutation, RuleInsertion, KeyframesRuleInsertion };

I don’t think a specific type of rule should be treated as a separate mutation type, especially if we have to pass the actual rule in, I suggest just using RuleInsertion combined with an inserted keyframes rule. Or eliminate the concept of RuleMutationType entirely (see below).

> Source/WebCore/css/CSSStyleSheet.h:94
> +        RuleMutationScope(CSSStyleSheet*, RuleMutationType = OtherMutation, StyleRuleBase* insertedRule = 0);

Please use nullptr instead of 0 in new code.

I suggest we use the type StyleRuleKeyframes* here instead of StyleRuleBase*. If we decide to use this in a more general way later, we could change that, but no reason to be needlessly general.

Another way to go would be to get rid of RuleMutationType entirely, and only pass in the inserted rule. Then a rule of nullptr would mean “other mutation”. Either way would be OK with me.

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