Bug 105760 – [TexMap] Rename current[Transform|Opacity|Filters] in TextureMapperLayer.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dongseong Hwang]

We use effective members to keep changeable values that animations change. This patch puts 'effective' prefix before those members.

In addition, add setPlatformTransform() to keep m_platformTransform after next calling setEffectiveTransform().

[Dongseong Hwang]

Created attachment 180734 [details]
Patch

[Noam Rosenthal]

Comment on attachment 180734 [details]
Patch

This seems unnecessary; platformTransform/opacity is only used for the root layer, which can't get a transform/opacity from anywhere else, which makes it redundant to multiply platformTransform with any other transform.

[Dongseong Hwang]

Created attachment 180767 [details]
Patch

[Dongseong Hwang]

(In reply to comment #2)
> (From update of attachment 180734 [details])
> This seems unnecessary; platformTransform/opacity is only used for the root layer, which can't get a transform/opacity from anywhere else, which makes it redundant to multiply platformTransform with any other transform.

After reading your comment, I understand setTransform and setOpacity called from TextureMapperLayerClientQt require m_state.[transform|opacity] respectively.

And I think effective prefix is still needed because I misunderstood the purpose of members a while.

So I re-post renaming patch as well as some refactoring about setTransform and setOpacity.

[Dongseong Hwang]

(In reply to comment #4)
> After reading your comment, I understand setTransform and setOpacity called from TextureMapperLayerClientQt require m_state.[transform|opacity] respectively.
s/require/require to change/

[Dongseong Hwang]

Created attachment 180768 [details]
Patch

[Dongseong Hwang]

(In reply to comment #6)
> Created an attachment (id=180768) [details]
> Patch

I found we don't need to keep setOpacity and setTransform public api in TextureMapperLayer only for root layer. LayerTreeRenderer does not use both API.

[Noam Rosenthal]

Comment on attachment 180768 [details]
Patch

The name effectiveOpacity is confusing; effectiveOpacity is the opacity multiplied by all the ancestors, e.g. a layer with opacity .5 and a parent with opacity .6 will have an opacity of 0.3. Same goes for effectiveTransform.

d

[Dongseong Hwang]

(In reply to comment #8)
> (From update of attachment 180768 [details])
> The name effectiveOpacity is confusing; effectiveOpacity is the opacity multiplied by all the ancestors, e.g. a layer with opacity .5 and a parent with opacity .6 will have an opacity of 0.3. Same goes for effectiveTransform.

It is why I want to change the name. Could you suggest better one?

[Dongseong Hwang]

Created attachment 181137 [details]
Patch

[Dongseong Hwang]

(In reply to comment #10)
> Created an attachment (id=181137) [details]
> Patch

rebase to upstream.

Could you review this bug?

[Noam Rosenthal]

Comment on attachment 181137 [details]
Patch

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

Let's change from m_effectiveXXX to m_currentXXX.
Effective is the multiplied value; current simply means that it's post-animation and not pre-animation.

> Source/WebKit/qt/ChangeLog:10
> +        It can reduce the number of public API of TextureMapperLayer.

This removes unnecessary public API for TextureMapperLayer.

[Dongseong Hwang]

Created attachment 181643 [details]
Patch

[Dongseong Hwang]

(In reply to comment #12)
> (From update of attachment 181137 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181137&action=review

Thanks for review!

> Let's change from m_effectiveXXX to m_currentXXX.
> Effective is the multiplied value; current simply means that it's post-animation and not pre-animation.

'current' prefix is good although I revived effectiveXXX from old CoordinatedGraphicsLayer :)

> This removes unnecessary public API for TextureMapperLayer.

Done.

In addition, I removed all setXXX methods because only setAnimatedXXX uses them.

[Noam Rosenthal]

> 'current' prefix is good although I revived effectiveXXX from old CoordinatedGraphicsLayer :)
Yes, it was wrong there :)

[WebKit Review Bot]

Comment on attachment 181643 [details]
Patch

Clearing flags on attachment: 181643

Committed r139526: <http://trac.webkit.org/changeset/139526>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.