Bug 75780

Summary: [Qt][WK2] Accelerated and non-accelerated animations need to be synchronized
Product: WebKit Reporter: Noam Rosenthal <noam>
Component: Layout and RenderingAssignee: Noam Rosenthal <noam>
Status: RESOLVED FIXED    
Severity: Normal CC: hausmann, jturcotte, kenneth, menard, webkit.review.bot, zoltan
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 82522, 82523, 82525, 82533, 82534    
Bug Blocks:    
Attachments:
Description Flags
Test
none
Patch
none
Patch
none
Patch for landing none

Description Noam Rosenthal 2012-01-07 09:44:54 PST
In order to avoid an overload of IPC messages during each animation frame, we've moved the animation logic to the UI process. However, this makes the animation go out of sync when there's both an accelerated and a non-accelerated animation running at the same time.
The solution may include keeping the animation logic in the UI process, but moving the frame timers to the web-process, sending timed frame events to the UI process without the rest of the animation info.
Comment 1 Noam Rosenthal 2012-02-29 14:58:33 PST
Created attachment 129525 [details]
Test
Comment 2 Noam Rosenthal 2012-03-28 10:39:32 PDT
Created attachment 134331 [details]
Patch
Comment 3 Jocelyn Turcotte 2012-03-28 12:22:17 PDT
Comment on attachment 134331 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134331&action=review

> Source/WebKit2/Shared/WebLayerTreeInfo.cpp:35
> +    SimpleArgumentCoder<WebLayerInfo>::encode(encoder, *this);

Could you add a comment somewhere that this requires WebLayerInfo to be a POD type?

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:-128
> -void WebLayerTreeRenderer::syncAnimations()

It would be very nice if the animation removal was a separate patch, or the other way around.

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:-176
> -    bool needsToUpdateImageTiles = layerInfo.imageIsUpdated || (layerInfo.contentsRect != layer->contentsRect() && layerInfo.imageBackingStoreID);

You could remove the imageIsUpdated flag from WebLayerInfo if you don't use it anymore.

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:169
> +            child = createLayer(childID).leakPtr();

Is this layer deleted anywhere on destruction? Anyway this looks like moved code.

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:279
> +    QImage image = bitmap->createQImage();

Is this intentional?

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:79
> +    m_shouldUpdateVisibleRect = true;

I don't see this used anywhere, am I missing something?

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:464
> +    float scale = shouldUseTiledBackingStore() ? m_contentsScale : 1;

The way this function is used here is misleading, its name could be improved or a comment might help.
This could also go in a different patch.

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:556
>  void WebGraphicsLayer::updateContentBuffers()
>  {
> +    if (m_image) {
> +        if (!m_layerInfo.imageBackingStoreID)
> +            m_layerInfo.imageBackingStoreID = m_webGraphicsLayerClient->adoptImageBackingStore(m_image.get());
> +    }
> +

Is it voluntary to move it back there?

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:645
> +bool WebGraphicsLayer::selfOrAncestorMightHaveNonAffineTransforms()

I would remove "Might".

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:650
> -    if (!m_transformAnimations.isEmpty())
> +    if (!m_layerTransform.combined().isAffine())
>          return true;
>  
> -    if (parent())
> -        return toWebGraphicsLayer(parent())->selfOrAncestorHasActiveTransformAnimations();
> -
>      return false;

return !m_layerTranform.combined().isAffine()
Comment 4 Noam Rosenthal 2012-03-28 12:55:40 PDT
Comment on attachment 134331 [details]
Patch

Separating to a few patches.
Comment 5 Noam Rosenthal 2012-03-28 15:12:32 PDT
Created attachment 134418 [details]
Patch
Comment 6 Jocelyn Turcotte 2012-03-29 04:32:40 PDT
Comment on attachment 134418 [details]
Patch

Much softer on the brain, thanks :)
This one LGTM.
Comment 7 Jocelyn Turcotte 2012-03-29 05:07:13 PDT
Comment on attachment 134418 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134418&action=review

Sorry, forgot a small nitpick.

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:642
> -    if (!m_transformAnimations.isEmpty())
> -        return true;
> -
>      if (!m_layerTransform.combined().isAffine())
>          return true;

return !m_layerTransform.combined().isAffine(); would look better here. Same comment as before, so just tell me if you think it doesn't make sense.
Comment 8 Noam Rosenthal 2012-03-29 06:42:48 PDT
(In reply to comment #7)
> (From update of attachment 134418 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134418&action=review
> 
> Sorry, forgot a small nitpick.
> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:642
> > -    if (!m_transformAnimations.isEmpty())
> > -        return true;
> > -
> >      if (!m_layerTransform.combined().isAffine())
> >          return true;
> 
> return !m_layerTransform.combined().isAffine(); would look better here. Same comment as before, so just tell me if you think it doesn't make sense.

I agree, just missed that.
Comment 9 Kenneth Rohde Christiansen 2012-04-03 07:09:01 PDT
Comment on attachment 134418 [details]
Patch

dont land before we have the whole feature reviewed :)
Comment 10 Noam Rosenthal 2012-04-09 10:52:55 PDT
Created attachment 136266 [details]
Patch for landing
Comment 11 WebKit Review Bot 2012-04-09 14:59:29 PDT
Comment on attachment 136266 [details]
Patch for landing

Clearing flags on attachment: 136266

Committed r113628: <http://trac.webkit.org/changeset/113628>
Comment 12 WebKit Review Bot 2012-04-09 14:59:35 PDT
All reviewed patches have been landed.  Closing bug.