WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
ChangSeok Oh
Comment 1
2013-02-20 02:13:15 PST
Created
attachment 189267
[details]
Patch
ChangSeok Oh
Comment 2
2013-03-05 09:31:06 PST
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
Comment 3
2013-03-29 22:14:28 PDT
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 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.
Top of Page
Format For Printing
XML
Clone This Bug