[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





--- Comment #14 from Ralph T <ralpht+bugs at gmail.com>  2013-10-03 12:32:49 PST ---
(In reply to comment #11)

Thanks for the review, Darin!

> > 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.

I reworked this and folded into the existing RuleInsertion branch since they're now share the same conditions. It's a lot better now :).

> 
> > Source/WebCore/css/CSSStyleSheet.cpp:427
> > +    , m_insertedRule(0)
> 
> Please use nullptr instead of 0 in new code.

Done.

> > Source/WebCore/css/CSSStyleSheet.h:94
> > +        RuleMutationScope(CSSStyleSheet*, RuleMutationType = OtherMutation, StyleRuleBase* insertedRule = 0);
> 
> Please use nullptr instead of 0 in new code.
> 

Done.

> 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.

I did this, and changed the name to m_insertedKeyframesRule.

> 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.

I kept the enum because maybe there will be other optimized cases in the future (like deleting keyframes?).

I still plan on making a new test for dynamically creating animations (I haven't done this before; it's going to look a bit like animations/change-keyframes.html).

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