[webkit-reviews] review granted: [Bug 60137] Mismatched multiple box-shadows do not transition as expected : [Attachment 93245] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has granted Simon Fraser (smfr) <simon.fraser at apple.com>'s
request for review:
Bug 60137: Mismatched multiple box-shadows do not transition as expected
https://bugs.webkit.org/show_bug.cgi?id=60137

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

------- Additional Comments from mitz at webkit.org
View in context: https://bugs.webkit.org/attachment.cgi?id=93245&action=review

r+ but please consider addressing at least some of the comments

> Source/WebCore/page/animation/AnimationBase.cpp:330
> +static inline int shadowListLength(const ShadowData* shadow)

Why not size_t?

> Source/WebCore/page/animation/AnimationBase.cpp:394
> +	   if (fromLength == toLength || ((!fromLength || fromLength == 1) &&
(!toLength || toLength == 1))) {

If you make shadowListLength return an unsigned type, you will be able to write
“(!fromLength || fromLength == 1)” as “fromLength <= 1”

> Source/WebCore/page/animation/AnimationBase.cpp:444
> +	   Vector<const ShadowData*> fromShadows(fromLength);
> +	   Vector<const ShadowData*> toShadows(toLength);
> +	   
> +	   while (shadowA) {
> +	       fromShadows.prepend(shadowA);
> +	       shadowA = shadowA->next();
> +	   }
> +
> +	   while (shadowB) {
> +	       toShadows.prepend(shadowB);
> +	       shadowB = shadowB->next();
> +	   }

This is an expensive way of doing things. You initialize fromShadows with
fromLength 0 ShadowData pointers in it, then you prepend another fromLength
pointers to that. You should do something like
for (size_t i = fromLength; i >= 0; --i) {
    fromShadows[i - 1] = shadowA;
    shadowA = shadowA->next();
}

And it would be better to give these vectors inline capacity to avoid heap
allocation.