[webkit-reviews] review denied: [Bug 14811] [gtk] [request] add a webkit_gtk_page_go_to_history_item function : [Attachment 18298] back/forward list and history item patch

[bugzilla-daemon at webkit.org]

Alp Toker <alp at atoker.com> has denied Jan Alonzo <jmalonzo at gmail.com>'s request
for review:
Bug 14811: [gtk] [request] add a webkit_gtk_page_go_to_history_item function
http://bugs.webkit.org/show_bug.cgi?id=14811

Attachment 18298: back/forward list and history item patch
http://bugs.webkit.org/attachment.cgi?id=18298&action=edit

------- Additional Comments from Alp Toker <alp at atoker.com>
This is looking good.

A few issues:

+    return g_strdup(title.utf8().data());

We can't return a newly allocated string as a const gchar*. Ideally we should
cache the string being returned (this is what we do elsewhere in the public
API). Depending on the situation there are different strategies but we have to
follow the GTK+ conventions here or programmers will cause leaks by not freeing
returned strings.

bdash notes that there's no need to check before deleting:

if (historyItemPriv->historyItem)
	delete historyItemPriv->historyItem

Now that we've decided to use NULL in API code, you shouldn't return 0 for a
const gchar* return, but NULL.

I didn't loook too carefully yet but kit() and core() are usually light
operations that don't make new allocations or add references, so not following
that expectation may lead to leaks.