[webkit-reviews] review denied: [Bug 119479] Inserting a JS generated keyframe animation shouldn't trigger a whole document style recalc : [Attachment 213201] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ralph T <ralpht+bugs at gmail.com>'s
request for review:
Bug 119479: Inserting a JS generated keyframe animation shouldn't trigger a
whole document style recalc
https://bugs.webkit.org/show_bug.cgi?id=119479

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

------- Additional Comments from Darin Adler <darin at apple.com>
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.