Bug 110685 – [Texmap] LayoutTests/compositing/animation/state-at-end-event-transform-layer.html shows a red square where it shouldn't

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Noam Rosenthal]

I believe this is a regression since r140825.
m_shouldUpdateCurrentTransformFromGraphicsLayer is set to false at some point, but never reset to true.
We don't treat end of animations correctly!

[Noam Rosenthal]

Created attachment 189916 [details]
Patch

[Martin Robinson]

Comment on attachment 189916 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Removed m_shouldUpdateTransformm_shouldUpdateCurrentTransformFromGraphicsLayer etc., as they don't do

Looks like m_shouldUpdateTransformm_shouldUpdateCurrentTransformFromGraphicsLayer might be missing a space.

[Noam Rosenthal]

Created attachment 190069 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 190069 [details]
Patch for landing

Clearing flags on attachment: 190069

Committed r143947: <http://trac.webkit.org/changeset/143947>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Allan Sandfeld Jensen]

Actually the patch you reverted here made a significant impact on Qt WK1. Has the issues been solved in a different way now?

[Noam Rosenthal]

(In reply to comment #6)
> Actually the patch you reverted here made a significant impact on Qt WK1. Has the issues been solved in a different way now?

This is not a revert. Can you be more specific than "significant impact", and are there tests that cover this?

[Allan Sandfeld Jensen]

(In reply to comment #7)
> (In reply to comment #6)
> > Actually the patch you reverted here made a significant impact on Qt WK1. Has the issues been solved in a different way now?
> 
> This is not a revert. Can you be more specific than "significant impact", and are there tests that cover this?

I thought the introduction of these variables was the main part of r140825, so it just look like a revert to me. R140825 solved an annoying flicker issue that was only observable in WK1 and had no tests, I merged it into Qt5.0, so I am trying to understand this patch, to see if merging r140825 was a mistake, or I need this patch too or anything in between.

[Noam Rosenthal]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Actually the patch you reverted here made a significant impact on Qt WK1. Has the issues been solved in a different way now?
> > 
> > This is not a revert. Can you be more specific than "significant impact", and are there tests that cover this?
> 
> I thought the introduction of these variables was the main part of r140825, so it just look like a revert to me. R140825 solved an annoying flicker issue that was only observable in WK1 and had no tests, I merged it into Qt5.0, so I am trying to understand this patch, to see if merging r140825 was a mistake, or I need this patch too or anything in between.

In any case those variables were wrong - they are set to false once and then never reset to true, unless I've missed something.

[Alexander Paschenko]

> In any case those variables were wrong - they are set to false once and then never reset to true, unless I've missed something.

I believe you have because actually they do get set to true inside m_shouldUpdateCurrentTransformFromGraphicsLayer. Please see
https://bug-102501-attachments.webkit.org/attachment.cgi?id=184733
it reads, among other things,

    if (changeMask & TransformChange)
        m_shouldUpdateCurrentTransformFromGraphicsLayer = true;

    if (changeMask & OpacityChange)
        m_shouldUpdateCurrentOpacityFromGraphicsLayer = true;

#if ENABLE(CSS_FILTERS)
    if (changeMask & FilterChange)
        m_shouldUpdateCurrentFiltersFromGraphicsLayer = true;
#endif

[Alexander Paschenko]

(In reply to comment #10)
> they do get set to true inside m_shouldUpdateCurrentTransformFromGraphicsLayer

Sorry, I meant that the patch sets these variables to true inside TextureMapperLayer::flushCompositingStateForThisLayerOnly.

[Noam Rosenthal]

(In reply to comment #11)
> (In reply to comment #10)
> > they do get set to true inside m_shouldUpdateCurrentTransformFromGraphicsLayer
> 
> Sorry, I meant that the patch sets these variables to true inside TextureMapperLayer::flushCompositingStateForThisLayerOnly.

OK, I think it got lost at a later patch then, probably in the GLTM refactoring.

[Noam Rosenthal]

(In reply to comment #11)
> (In reply to comment #10)
> > they do get set to true inside m_shouldUpdateCurrentTransformFromGraphicsLayer
> 
> Sorry, I meant that the patch sets these variables to true inside TextureMapperLayer::flushCompositingStateForThisLayerOnly.

You're right, but it's still the wrong fix. When an animation doesn't fillForward, it should revert to its original position at end of animation. Your patch makes it so that animations are always considered to have a fillsForward flag.