[webkit-reviews] review denied: [Bug 21594] [Gtk] Use GOwnPtr for code that needs it : [Attachment 56658] Patch which doesn't change the order of release of members

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 21594: [Gtk] Use GOwnPtr for code that needs it
https://bugs.webkit.org/show_bug.cgi?id=21594

Attachment 56658: Patch which doesn't change the order of release of members
https://bugs.webkit.org/attachment.cgi?id=56658&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:1141
 +	    GRefPtr<GtkMenu> menu(priv->currentMenu);
Doesn't GRefPtr have a .release() method?  Or would that require a GPassRefPtr?


WebKit/gtk/webkit/webkitwebview.cpp:1098
 +	priv->tooltipText.clear();
Do all of these need to be explicitly cleared?	It seems not.

WebKit/gtk/webkit/webkitwebview.cpp:3879
 +	    priv->encoding.set(g_strdup(encoding.utf8().data()));
Seems we could store the CString instead of the raw char* and that would be
cleaner here.

WebKit/gtk/webkit/webkitwebview.cpp:3880
 +	    return priv->encoding.get();
the we would just return encoding.data() instead of having to have an OwnPtr
store it.

WebKit/gtk/webkit/webkitwebview.cpp:3879
 +	    priv->encoding.set(g_strdup(encoding.utf8().data()));
Doesn't this copy twice?  We only need to copy once by creating the utf8
CString, no?

WebKit/gtk/webkit/webkitwebview.cpp:3921
 +	    priv->customEncoding.set(g_strdup(overrideEncoding.utf8().data()));

Here again, couldn't we just store the CSTring?

WebKit/gtk/webkit/webkitwebview.cpp:4201
 +	priv->iconURI.set(g_strdup(iconURL.utf8().data()));
And again.. CString.

This is definitely better.  But I think there are too many manual clear()
calls, and the string handling could use CString storage instead to save a copy
(and make the code simpler).