Bug 110314 – [GTK][AC] Implement matrix keyframe animations with clutter ac backend

• Component: WebKitGTK
• Status: RESOLVED FIXED

[ChangSeok Oh]

This bug is for adding matrix keyframe animation implementation.

[ChangSeok Oh]

Created attachment 189267 [details]
Patch

[ChangSeok Oh]

You can find the same patch for webkit-clutter here, http://cgit.collabora.com/git/webkit-clutter.git/commit/?h=wip/changseok/unreviewed&id=c4a0149b278963106d6d33c50438873a938ac85a

[ChangSeok Oh]

Maybe the address changed for webkit-clutter.
Here is a new one.
http://cgit.collabora.com/git/webkit-clutter.git/commit/?h=wip/changseok/unreviewed&id=f609e018f18232c1c4cd40c7fdb0f7751aa5eb65

[Gustavo Noronha (kov)]

Comment on attachment 189267 [details]
Patch

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

> Source/WebCore/platform/graphics/clutter/PlatformClutterAnimation.cpp:540
> +    GOwnPtr<GValue> keyValues(g_new0(GValue, nKeyframes));

This will go wrong. GOwnPtr will expect a single GValue pointer here but you want an array. We either need a GOwnArrayPtr like the one used for OwnPtr above, or manual handling of this array. I suggest using manual handling.

> Source/WebCore/platform/graphics/clutter/PlatformClutterAnimation.cpp:559
> +    for (unsigned i = 0; i < nKeyframes; ++i)
> +        g_value_unset(&keyValues.get()[i]);
> +}

Then here you'll have to free all of the values instead of unsetting.

[ChangSeok Oh]

Comment on attachment 189267 [details]
Patch

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

Thank you for the comment, but would you consider it again if you don't mind?
I leave some notes, please point out if there is my misunderstanding. :)

>> Source/WebCore/platform/graphics/clutter/PlatformClutterAnimation.cpp:540
>> +    GOwnPtr<GValue> keyValues(g_new0(GValue, nKeyframes));
> 
> This will go wrong. GOwnPtr will expect a single GValue pointer here but you want an array. We either need a GOwnArrayPtr like the one used for OwnPtr above, or manual handling of this array. I suggest using manual handling.

Whether we allocate a single GValue or Gvalue array, we can and should free it by using g_free. Because we used g_new0 here to allocate it. Of course I know GOwnPtr<GValue> is not for array type but it works as we expected. It will call 'g_free' when finalizing this function.

>> Source/WebCore/platform/graphics/clutter/PlatformClutterAnimation.cpp:559
>> +}
> 
> Then here you'll have to free all of the values instead of unsetting.

I think we should call g_value_unset here since the GValues initialized and set CLUTTER_TYPE_MATRIX which is an object type not a primitive type. In my humble opinion, we should clear the deep allocation inside of each GValue before freeing it.

[Gustavo Noronha (kov)]

Comment on attachment 189267 [details]
Patch

I think that's abusing GOwnPtr, makes things a bit less hackable, but I don't feel too strongly, so OK, let's go with your change.

[WebKit Review Bot]

Comment on attachment 189267 [details]
Patch

Clearing flags on attachment: 189267

Committed r147549: <http://trac.webkit.org/changeset/147549>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[ChangSeok Oh]

(In reply to comment #6)
> (From update of attachment 189267 [details])
> I think that's abusing GOwnPtr, makes things a bit less hackable, but I don't feel too strongly, so OK, let's go with your change.

Thanks :)
Let me improve those lines to make less hackable later! ;)