Bug 100673

Summary: [Qt] Animations jump when the page is suspended
Product: WebKit Reporter: Jocelyn Turcotte <jturcotte>
Component: Layout and RenderingAssignee: Noam Rosenthal <noam>
Status: RESOLVED FIXED    
Severity: Normal CC: jturcotte, noam, ossy, rafael.lobo, webkit.review.bot
Priority: P3 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 100769    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch none

Jocelyn Turcotte
Reported 2012-10-29 07:52:03 PDT
In the leaves and poster circle demos in MiniBrowser, starting to pan the page shows a completely out of sync frame while the page is suspended. The animation resumes correctly at its previous position when the page is resumed.
Attachments
Patch (5.23 KB, patch)
2012-10-29 15:04 PDT, Noam Rosenthal 		no flags
DetailsFormatted DiffDiff
Patch (5.74 KB, patch)
2012-10-29 16:48 PDT, Noam Rosenthal 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Noam Rosenthal
Comment 1 2012-10-29 08:06:07 PDT
Are you looking at this, or should I?
Jocelyn Turcotte
Comment 2 2012-10-29 08:11:28 PDT
I didn't investigate yet, I'll do if you don't.
Noam Rosenthal
Comment 3 2012-10-29 08:32:13 PDT
I'll take a look today.
Noam Rosenthal
Comment 4 2012-10-29 15:04:05 PDT
Created attachment 171318 [details] Patch
Rafael Brandao
Comment 5 2012-10-29 16:43:05 PDT
Comment on attachment 171318 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171318&action=review I have a few comments. :-) > Source/WebCore/platform/graphics/GraphicsLayerAnimation.cpp:265 > +void GraphicsLayerAnimation::pause(double pauseTime) I think domTime/documentTime would make more clear what kind of time unit we expect here. Isn't "pauseTime" is already implicit? > Source/WebCore/platform/graphics/GraphicsLayerAnimation.cpp:267 > // FIXME: should apply offset here. Do we need this comment? > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:811 > + m_lastAnimationStartTime = WTF::currentTime() - timeOffset; Is timeOffset the starting time for WebProcess? I still have to trouble to follow the logic of those offsets. :-(
Rafael Brandao
Comment 6 2012-10-29 16:44:01 PDT
Meh, sorry for all the typos on last message. :-(
Noam Rosenthal
Comment 7 2012-10-29 16:48:57 PDT
Created attachment 171336 [details] Patch
Noam Rosenthal
Comment 8 2012-10-29 16:50:33 PDT
(In reply to comment #5) > I think domTime/documentTime would make more clear what kind of time unit we expect here. Isn't "pauseTime" is already implicit? Yes, it is :) > > Source/WebCore/platform/graphics/GraphicsLayerAnimation.cpp:267 > > // FIXME: should apply offset here. > > Do we need this comment? No, we don't :) > > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:811 > > + m_lastAnimationStartTime = WTF::currentTime() - timeOffset; > > Is timeOffset the starting time for WebProcess? I still have to trouble to follow the logic of those offsets. :-( These offsets are for animation delay. For some reason we receive them here as "negative delay" - it's the actual delay, but that time has already passed. I renamed it to delayAsNegativeTimeOffset, any better suggestion?
Rafael Brandao
Comment 9 2012-10-29 17:13:10 PDT
> > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:811 > > > + m_lastAnimationStartTime = WTF::currentTime() - timeOffset; > > > > Is timeOffset the starting time for WebProcess? I still have to trouble to follow the logic of those offsets. :-( > > These offsets are for animation delay. For some reason we receive them here as "negative delay" - it's the actual delay, but that time has already passed. I renamed it to delayAsNegativeTimeOffset, any better suggestion? Hmm, I think your suggestion sounds awesome! :-D
WebKit Review Bot
Comment 10 2012-10-30 07:11:48 PDT
Comment on attachment 171336 [details] Patch Clearing flags on attachment: 171336 Committed r132907: <http://trac.webkit.org/changeset/132907>
WebKit Review Bot
Comment 11 2012-10-30 07:11:52 PDT
All reviewed patches have been landed. Closing bug.
Csaba Osztrogonác
Comment 12 2012-10-30 10:08:28 PDT
(In reply to comment #10) > (From update of attachment 171336 [details]) > Clearing flags on attachment: 171336 > > Committed r132907: <http://trac.webkit.org/changeset/132907> It caused a regression - https://bugs.webkit.org/show_bug.cgi?id=100769 Could you check it please?
Note You need to log in before you can comment on or make changes to this bug.