Bug 109037

Summary: [GTK][AC] Clutter required version up to 1.12
Product: WebKit Reporter: ChangSeok Oh <kevin.cs.oh>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gns, joone, webkit.review.bot, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 105699    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description ChangSeok Oh 2013-02-06 03:01:42 PST
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
Comment 1 ChangSeok Oh 2013-02-06 03:17:08 PST
Created attachment 186812 [details]
Patch
Comment 2 Gustavo Noronha (kov) 2013-02-06 04:19:08 PST
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.
Comment 3 ChangSeok Oh 2013-02-07 11:05:01 PST
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
Comment 4 ChangSeok Oh 2013-02-07 11:57:01 PST
Created attachment 187142 [details]
Patch
Comment 5 Martin Robinson 2013-02-07 12:43:05 PST
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
> +2013-02-07  ChangSeok Oh  <shivamidow@gmail.com>
> +
> +        [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.
Comment 6 ChangSeok Oh 2013-02-07 12:50:45 PST
Created attachment 187153 [details]
Patch
Comment 7 Gustavo Noronha (kov) 2013-02-07 13:24:00 PST
(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!
Comment 8 WebKit Review Bot 2013-02-07 13:38:45 PST
Comment on attachment 187153 [details]
Patch

Clearing flags on attachment: 187153

Committed r142172: <http://trac.webkit.org/changeset/142172>
Comment 9 WebKit Review Bot 2013-02-07 13:38:50 PST
All reviewed patches have been landed.  Closing bug.