Bug 109037 – [GTK][AC] Clutter required version up to 1.12

• Component: WebKitGTK
• Status: RESOLVED FIXED

[ChangSeok Oh]

Current clutter required version is 1.8.2. It's too old, so we have lost a change to use new conveient apis.
The official stable clutter version is 1.12.2. so I think it's time to move to 1.12.x

[ChangSeok Oh]

Created attachment 186812 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 186812 [details]
Patch

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

> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:218
> +    if (priv->matrix) {
> +        float width = clutter_actor_get_width(actor);
> +        float height = clutter_actor_get_height(actor);
> +        if (width <= 1.0 || height <= 1.0)
> +            return;

This is a change in behaviour, right? Would be good to separate "bringing stuff up-to-date" with behaviour changes.

> Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:396
> -    FloatPoint scaledPosition;
> -    FloatPoint3D scaledAnchorPoint;
> -    FloatSize scaledSize;
> -
> -    // FIXME: Need to support scaling
> -    scaledPosition = m_position;
> -    scaledAnchorPoint = m_anchorPoint;
> -    scaledSize = m_size;
> -
> -    FloatRect adjustedBounds(m_boundsOrigin , scaledSize);
> -    FloatPoint adjustedPosition(scaledPosition.x() + scaledAnchorPoint.x() * scaledSize.width(), scaledPosition.y() + scaledAnchorPoint.y() * scaledSize.height());
> -
> -    clutter_actor_set_size(CLUTTER_ACTOR(m_layer.get()), adjustedBounds.width(), adjustedBounds.height());
> -    clutter_actor_set_position(CLUTTER_ACTOR(m_layer.get()), adjustedPosition.x(), adjustedPosition.y());
> -    graphicsLayerActorSetAnchorPoint(m_layer.get(), scaledAnchorPoint.x(), scaledAnchorPoint.y(), scaledAnchorPoint.z());
> +    // FIXME: Need to support page scaling.
> +    clutter_actor_set_size(CLUTTER_ACTOR(m_layer.get()), m_size.width(), m_size.height());
> +    clutter_actor_set_position(CLUTTER_ACTOR(m_layer.get()), m_position.x(), m_position.y());
> +    graphicsLayerActorSetAnchorPoint(m_layer.get(), m_anchorPoint.x(), m_anchorPoint.y(), m_anchorPoint.z());

Ditto. You're now ignoring the calculations done for adjusted*(), that might be OK (I think it might break AC on zoomed pages, though, did you check it doesn't?), but should be its own change.

[ChangSeok Oh]

Comment on attachment 186812 [details]
Patch

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

>> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:218
>> +            return;
> 
> This is a change in behaviour, right? Would be good to separate "bringing stuff up-to-date" with behaviour changes.

Right, let me open a new bug to handle this kind of stuff.

>> Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:396
>> +    graphicsLayerActorSetAnchorPoint(m_layer.get(), m_anchorPoint.x(), m_anchorPoint.y(), m_anchorPoint.z());
> 
> Ditto. You're now ignoring the calculations done for adjusted*(), that might be OK (I think it might break AC on zoomed pages, though, did you check it doesn't?), but should be its own change.

O.K. Let's deal with this in an another bug. Current implementations doesn't support page scaling, too.
I think using adjusted* values is wrong here for clutterActor position. Actually, clutter coordinate system doesn't need to recalculate ajusted* value because it has a different coordinate system over mac port. Basically, the position of a CALayer(?) is specified in terms of the location of the layer's anchorPoint in mac port, so mac port need to recalculate the layer position after changing anchor position. See https://developer.apple.com/library/ios/#documentation/Cocoa/Conceptual/CoreAnimation_guide/CoreAnimationBasics/CoreAnimationBasics.html#//apple_ref/doc/uid/TP40004514-CH2-SW15

[ChangSeok Oh]

Created attachment 187142 [details]
Patch

[Martin Robinson]

Comment on attachment 187142 [details]
Patch

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

Looks good apart from the changelog.

> ChangeLog:20
> [phone]  ChangSeok Oh  <[e-mail]>
> +
> +        [GTK][AC] Clutter required version up to 1.12
> +        https://bugs.webkit.org/show_bug.cgi?id=109037
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        The clutter requried version is changed to 1.12.
> +
> +        * configure.ac:
> +

You've got a double changelog here so you cannot use the commit queue. There must be a bug with the script because I saw this problem today as well.

[ChangSeok Oh]

Created attachment 187153 [details]
Patch

[Gustavo Noronha (kov)]

(In reply to comment #3)
> O.K. Let's deal with this in an another bug. Current implementations doesn't support page scaling, too.
> I think using adjusted* values is wrong here for clutterActor position. Actually, clutter coordinate system doesn't need to recalculate ajusted* value because it has a different coordinate system over mac port. Basically, the position of a CALayer(?) is specified in terms of the location of the layer's anchorPoint in mac port, so mac port need to recalculate the layer position after changing anchor position. See https://developer.apple.com/library/ios/#documentation/Cocoa/Conceptual/CoreAnimation_guide/CoreAnimationBasics/CoreAnimationBasics.html#//apple_ref/doc/uid/TP40004514-CH2-SW15

Makes sense. We used to use an anchor point as well, but Emanuele Aina has made it use clutter's new pivot point instead like you've done here. Let me know when you post the other patch!

[WebKit Review Bot]

Comment on attachment 187153 [details]
Patch

Clearing flags on attachment: 187153

Committed r142172: <http://trac.webkit.org/changeset/142172>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.