RESOLVED FIXED 110314
[GTK][AC] Implement matrix keyframe animations with clutter ac backend
https://bugs.webkit.org/show_bug.cgi?id=110314
Summary [GTK][AC] Implement matrix keyframe animations with clutter ac backend
ChangSeok Oh
Reported 2013-02-20 01:42:11 PST
This bug is for adding matrix keyframe animation implementation.
Attachments
Patch (6.16 KB, patch)
2013-02-20 02:13 PST, ChangSeok Oh
no flags
ChangSeok Oh
Comment 1 2013-02-20 02:13:15 PST
ChangSeok Oh
Comment 2 2013-03-05 09:31:06 PST
ChangSeok Oh
Comment 3 2013-03-29 22:14:28 PDT
Gustavo Noronha (kov)
Comment 4 2013-04-02 06:02:02 PDT
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 5 2013-04-03 02:48:54 PDT
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 6 2013-04-03 05:51:33 PDT
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 7 2013-04-03 06:21:52 PDT
Comment on attachment 189267 [details] Patch Clearing flags on attachment: 189267 Committed r147549: <http://trac.webkit.org/changeset/147549>
WebKit Review Bot
Comment 8 2013-04-03 06:21:55 PDT
All reviewed patches have been landed. Closing bug.
ChangSeok Oh
Comment 9 2013-04-03 08:31:05 PDT
(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! ;)
Note You need to log in before you can comment on or make changes to this bug.