Bug 110314

Created attachment 189267 [details] Patch

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

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

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.

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.

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.

Comment on attachment 189267 [details] Patch Clearing flags on attachment: 189267 Committed r147549: <http://trac.webkit.org/changeset/147549>

All reviewed patches have been landed. Closing bug.

(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! ;)