Bug 63859 - Transition LayoutTest using pause API shows wrong result if it tries to pause a transition after its delay time.
Summary: Transition LayoutTest using pause API shows wrong result if it tries to pause...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Minor
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-02 06:18 PDT by Young Han Lee
Modified: 2011-07-11 11:20 PDT (History)
3 users (show)

See Also:


Attachments
To reproduce the bug. (1013 bytes, text/html)
2011-07-02 06:18 PDT, Young Han Lee
no flags Details
Testcase with pause API (1009 bytes, text/html)
2011-07-02 06:29 PDT, Young Han Lee
no flags Details
LayoutTest with the pause API (933 bytes, text/html)
2011-07-02 06:32 PDT, Young Han Lee
no flags Details
Patch (4.08 KB, patch)
2011-07-02 12:03 PDT, Young Han Lee
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.