Bug 209996 - Additional sanity checks in compareAnimationsByCompositeOrder()
Summary: Additional sanity checks in compareAnimationsByCompositeOrder()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Doug Kelly
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-03 18:28 PDT by Doug Kelly
Modified: 2020-04-04 15:39 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.00 KB, patch)
2020-04-03 18:37 PDT, Doug Kelly
no flags Details | Formatted Diff | Diff
Patch (6.14 KB, patch)
2020-04-04 09:13 PDT, Doug Kelly
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Doug Kelly 2020-04-03 18:28:45 PDT
compareAnimationsByCompositeOrder() is used by std::sort which requires strict weak ordering.  Add additional checks to ensure strict weak ordering is maintained.

<rdar://problem/60199826>
Comment 1 Doug Kelly 2020-04-03 18:37:15 PDT
Created attachment 395427 [details]
Patch
Comment 2 Geoffrey Garen 2020-04-04 08:27:31 PDT
Comment on attachment 395427 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395427&action=review

> Source/WebCore/animation/WebAnimationUtilities.cpp:87
> +        } else
> +            return !rhsIsCSSAnimation;

If you put this code first as an early return, you can eliminate the nested indentation for twenty lines of code above. I think that would read more clearly.
Comment 3 Doug Kelly 2020-04-04 08:35:54 PDT
Comment on attachment 395427 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395427&action=review

>> Source/WebCore/animation/WebAnimationUtilities.cpp:87
>> +            return !rhsIsCSSAnimation;
> 
> If you put this code first as an early return, you can eliminate the nested indentation for twenty lines of code above. I think that would read more clearly.

I think that makes sense... it can probably apply to the same case above, too. :)
Comment 4 Doug Kelly 2020-04-04 09:13:55 PDT
Created attachment 395450 [details]
Patch
Comment 5 Geoffrey Garen 2020-04-04 15:04:25 PDT
Comment on attachment 395450 [details]
Patch

r=me
Comment 6 EWS 2020-04-04 15:39:23 PDT
Committed r259538: <https://trac.webkit.org/changeset/259538>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395450 [details].