Bug 63859 – Transition LayoutTest using pause API shows wrong result if it tries to pause a transition after its delay time.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Young Han Lee]

Created attachment 99551 [details]
To reproduce the bug.

The attached test passes when it doesn't use the pause API.

[Young Han Lee]

Created attachment 99552 [details]
Testcase with pause API

Oops, I uploaded the wrong test.

[Young Han Lee]

Created attachment 99553 [details]
LayoutTest with the pause API

again :-(

[Young Han Lee]

Created attachment 99556 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 99556 [details]
Patch

I don't think this will do the right thing for accelerated animations.

[Young Han Lee]

(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.

[Simon Fraser (smfr)]

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?

[Young Han Lee]

(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.

[Simon Fraser (smfr)]

(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

[Young Han Lee]

(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.

[Simon Fraser (smfr)]

Comment on attachment 99556 [details]
Patch

OK

[Young Han Lee]

Thank you for your review, simon :)

[Young Han Lee]

Simon,
could you commit this, please?
I'm not yet committer.

[WebKit Review Bot]

Comment on attachment 99556 [details]
Patch

Clearing flags on attachment: 99556

Committed r90765: <http://trac.webkit.org/changeset/90765>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.