Bug 185299 - REGRESSION (r230574): Interrupted hardware transitions don't behave correctly
Summary: REGRESSION (r230574): Interrupted hardware transitions don't behave correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on: 185668
Blocks:
  Show dependency treegraph
 
Reported: 2018-05-04 06:55 PDT by Antoine Quint
Modified: 2018-05-16 10:27 PDT (History)
6 users (show)

See Also:


Attachments
Patch for EWS, not for review. (7.10 KB, patch)
2018-05-04 07:01 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (7.22 KB, patch)
2018-05-04 12:16 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (10.38 KB, patch)
2018-05-14 04:41 PDT, Antoine Quint
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2018-05-04 06:55:20 PDT
The fix for https://bugs.webkit.org/show_bug.cgi?id=184518 broke interrupted transitions and suspended animations when moving to a different tab.
Comment 1 Antoine Quint 2018-05-04 06:56:10 PDT
<rdar://problem/39630230>
Comment 2 Antoine Quint 2018-05-04 07:01:28 PDT
Created attachment 339541 [details]
Patch for EWS, not for review.
Comment 3 Simon Fraser (smfr) 2018-05-04 08:24:01 PDT
Comment on attachment 339541 [details]
Patch for EWS, not for review.

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3118
> +    if (m_animationsToProcess.contains(animation.m_name))
> +        m_animationsToProcess.remove(animation.m_name);

You have two hash lookups when you could use only one. Either just unconditionally remove, or find and remove using the iterator.
Comment 4 Antoine Quint 2018-05-04 09:31:47 PDT
(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 339541 [details]
> Patch for EWS, not for review.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339541&action=review
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3118
> > +    if (m_animationsToProcess.contains(animation.m_name))
> > +        m_animationsToProcess.remove(animation.m_name);
> 
> You have two hash lookups when you could use only one. Either just
> unconditionally remove, or find and remove using the iterator.

Oh right, remove returns a bool, so I guess it works whether there is a key or not.

Can you think of a way to test this? I think the only way to check whether an animation is going is if we can get at the play state of CA animations. Is this possible?
Comment 5 Antoine Quint 2018-05-04 12:16:06 PDT
Created attachment 339582 [details]
Patch
Comment 6 Simon Fraser (smfr) 2018-05-04 13:04:30 PDT
I think you could test this via a long-running transition with a negative start delay; hover and uncover (or changes styles) and then ref-test that the box is not in its untransformed location.
Comment 7 Antoine Quint 2018-05-14 04:41:21 PDT
Created attachment 340306 [details]
Patch
Comment 8 Antoine Quint 2018-05-14 11:16:07 PDT
Committed r231765: <https://trac.webkit.org/changeset/231765>
Comment 9 Dawei Fenton (:realdawei) 2018-05-15 16:34:58 PDT
The test for this issue (transitions/interrupted-transition-hardware.html is flaky on the bots: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=transitions%2Finterrupted-transition-hardware.html
Comment 10 WebKit Commit Bot 2018-05-15 16:56:59 PDT
Re-opened since this is blocked by bug 185668
Comment 11 Antoine Quint 2018-05-16 01:28:01 PDT
Committed r231840: <https://trac.webkit.org/changeset/231840>
Comment 12 Antoine Quint 2018-05-16 06:09:46 PDT
Committed r231842: <https://trac.webkit.org/changeset/231842>
Comment 13 Antoine Quint 2018-05-16 10:27:08 PDT
Committed r231851: <https://trac.webkit.org/changeset/231851>