Summary: | Transition LayoutTest using pause API shows wrong result if it tries to pause a transition after its delay time. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Young Han Lee <joybro201> | ||||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Minor | CC: | cmarrin, simon.fraser, webkit.review.bot | ||||||||||
Priority: | P3 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Created attachment 99552 [details]
Testcase with pause API
Oops, I uploaded the wrong test.
Created attachment 99553 [details]
LayoutTest with the pause API
again :-(
Created attachment 99556 [details]
Patch
Comment on attachment 99556 [details]
Patch
I don't think this will do the right thing for accelerated animations.
(In reply to comment #4) > (From update of attachment 99556 [details]) > I don't think this will do the right thing for accelerated animations. Can you be more specific? I failed to find out how this change can break the accelerated animation, and I made sure this change doesn't break both the animation and the transition layout tests on the Qt port enabling accelerated compositing. I'm not sure that Qt does accelerated animations the same way that Mac does. Does it ever return true from its GraphicsLayer::addAnimation() method? (In reply to comment #6) > I'm not sure that Qt does accelerated animations the same way that Mac does. Does it ever return true from its GraphicsLayer::addAnimation() method? I just ran layout-tests for animations, transitions and compositing on the Mac port in which accelerated compositing is enabled in default, and made sure no testcase fails by this patch. It seems no problem :) > if (!m_startTime) { > // If we haven't started yet, make it as if we started. > m_compAnim->animationController()->receivedStartTimeResponse(currentTime()); > } > > ASSERT(m_startTime); As I wrote in the changelog, the above code has not worked at all, so the next Assertion always fails when the m_startTime is zero like Bug 59475. In other words, this change could affect the testcases that have hit assertion failure, and doesn't make things worse at least. (In reply to comment #7) > (In reply to comment #6) > > I'm not sure that Qt does accelerated animations the same way that Mac does. Does it ever return true from its GraphicsLayer::addAnimation() method? > > I just ran layout-tests for animations, transitions and compositing on the Mac port in which accelerated compositing is enabled in default, and made sure no testcase fails by this patch. > > It seems no problem :) Did you look at pixel results? I think the failure would only show there. Simon (In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > I'm not sure that Qt does accelerated animations the same way that Mac does. Does it ever return true from its GraphicsLayer::addAnimation() method? > > > > I just ran layout-tests for animations, transitions and compositing on the Mac port in which accelerated compositing is enabled in default, and made sure no testcase fails by this patch. > > > > It seems no problem :) > > Did you look at pixel results? I think the failure would only show there. > > Simon Yes, I also ran pixel tests a while ago and there was nothing wrong, too. If you tell me why you think this change causes failure on accelerated animation, I can dig into that more. Comment on attachment 99556 [details]
Patch
OK
Thank you for your review, simon :) Simon, could you commit this, please? I'm not yet committer. Comment on attachment 99556 [details] Patch Clearing flags on attachment: 99556 Committed r90765: <http://trac.webkit.org/changeset/90765> All reviewed patches have been landed. Closing bug. |
Created attachment 99551 [details] To reproduce the bug. The attached test passes when it doesn't use the pause API.