Bug 60137 – Mismatched multiple box-shadows do not transition as expected

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Simon Fraser (smfr)]

Created attachment 92188 [details]
Testcase

A transition from one inset box-shadow to two shadows, one inset and one outset, doesn't work as expected. See testcase.

[Simon Fraser (smfr)]

<rdar://problem/9311120>

[Simon Fraser (smfr)]

The issue here is that shadows are processed in reverse order, and the last shadows have different types in the testcase.

[Simon Fraser (smfr)]

Created attachment 92193 [details]
Patch

[Early Warning System Bot]

Attachment 92193 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8551434

[WebKit Review Bot]

Attachment 92193 [details] did not pass chromium-ews:
Output: http://queues.webkit.org/results/8567003

[Simon Fraser (smfr)]

Created attachment 93245 [details]
Patch

Comment on attachment 93245 [details]
Patch

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.

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/86351

[WebKit Review Bot]

http://trac.webkit.org/changeset/86351 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
svg/custom/fill-opacity-update.svg