Bug 63859

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 RenderingAssignee: 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:
Description Flags
To reproduce the bug.
none
Testcase with pause API
none
LayoutTest with the pause API
none
Patch none

Description Young Han Lee 2011-07-02 06:18:46 PDT
Created attachment 99551 [details]
To reproduce the bug.

The attached test passes when it doesn't use the pause API.
Comment 1 Young Han Lee 2011-07-02 06:29:10 PDT
Created attachment 99552 [details]
Testcase with pause API

Oops, I uploaded the wrong test.
Comment 2 Young Han Lee 2011-07-02 06:32:08 PDT
Created attachment 99553 [details]
LayoutTest with the pause API

again :-(
Comment 3 Young Han Lee 2011-07-02 12:03:49 PDT
Created attachment 99556 [details]
Patch
Comment 4 Simon Fraser (smfr) 2011-07-03 09:02:00 PDT
Comment on attachment 99556 [details]
Patch

I don't think this will do the right thing for accelerated animations.
Comment 5 Young Han Lee 2011-07-05 11:14:22 PDT
(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.
Comment 6 Simon Fraser (smfr) 2011-07-05 11:24:55 PDT
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?
Comment 7 Young Han Lee 2011-07-06 10:24:48 PDT
(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.
Comment 8 Simon Fraser (smfr) 2011-07-06 10:31:11 PDT
(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
Comment 9 Young Han Lee 2011-07-07 07:53:24 PDT
(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 10 Simon Fraser (smfr) 2011-07-07 08:00:01 PDT
Comment on attachment 99556 [details]
Patch

OK
Comment 11 Young Han Lee 2011-07-07 08:07:18 PDT
Thank you for your review, simon :)
Comment 12 Young Han Lee 2011-07-11 10:26:15 PDT
Simon,
could you commit this, please?
I'm not yet committer.
Comment 13 WebKit Review Bot 2011-07-11 11:20:35 PDT
Comment on attachment 99556 [details]
Patch

Clearing flags on attachment: 99556

Committed r90765: <http://trac.webkit.org/changeset/90765>
Comment 14 WebKit Review Bot 2011-07-11 11:20:40 PDT
All reviewed patches have been landed.  Closing bug.