[webkit-reviews] review denied: [Bug 69343] [GTK] Initial implementation of back forward list for WebKit2 GTK+ API : [Attachment 110491] Updated patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 69343: [GTK] Initial implementation of back forward list for WebKit2 GTK+
API
https://bugs.webkit.org/show_bug.cgi?id=69343

Attachment 110491: Updated patch
https://bugs.webkit.org/attachment.cgi?id=110491&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=110491&action=review


Looks good. Only giving r- because of the memory leak. I guess you'll need to
convert the unit test. Let me know if you want me to do that -- perfectly
willing to since I created this mess.

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:30
> + * @Short_description: List of visited pages
> + * @Title: WebKitBackForwardList
> + * @See_also: #WebKitWebView, #WebKitBackForwardListItem

In our old documentation the character after '@' is not capitalized. I'm not
sure if this matter, but for the sake of consistency, why don't we keep it a
small character.

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:33
> + * go back and forward to the most recent page. Items are inserted

go back and forward to the most recent page -> used to navigate to recent pages
?

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:42
> + * specified item. All other methods returning
#WebKitBackForwardListItem<!-- -->s

Does '<!-- -->' add a space? If so might be better to just write "one or more"
before it instead of having a space and then an 's'

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:108
> + * Return value: (transfer none): a #WebKitWebHistoryItem,
> + *	 or %NULL if @back_forward_list is empty.

No comma necessary before or

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:124
> + * Returns: (transfer none): the #WebKitBackForwardListItem
> + *	 preceding the current item, or %NULL.

Ditto. Please make this change below as well.

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:216
> + *	 items succeeding the current item

succeeding -> following ?

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:232
> + *	 items succeeding the current item, limited by @limit

succeeding -> following

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardListItem.cpp:78
> +typedef HashMap<WKBackForwardListItemRef, GRefPtr<WebKitBackForwardListItem>
> HistoryItemsMap;
> +
> +WebKitBackForwardListItem*
webkitBackForwardListItemGetOrCreate(WKBackForwardListItemRef wkListItem)
> +{
> +    DEFINE_STATIC_LOCAL(HistoryItemsMap, itemsMap, ());
> +
> +    if (!wkListItem)
> +	   return 0;
> +
> +    GRefPtr<WebKitBackForwardListItem> listItem = itemsMap.get(wkListItem);
> +    if (listItem)
> +	   return listItem.get();
> +
> +    listItem =
adoptGRef(WEBKIT_BACK_FORWARD_LIST_ITEM(g_object_new(WEBKIT_TYPE_BACK_FORWARD_L
IST_ITEM, NULL)));
> +    listItem->priv->wkListItem = wkListItem;
> +    itemsMap.set(wkListItem, listItem);

This means that the items will leak indefinitely because there's a reference to
the GObject in the HashMap and a reference to the WKItem in the GObject. It's
very common to to access the back forward list (for instance via the back and
forward buttons in the browser). Perhaps there's a way not to leak memory here?
I wonder if it's useful to have a pile of GObjects that are g_object_unref'd in
an idle? This could be quite useful across the entire library.

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardListItem.cpp:131
> + * webkit_back_forward_list_item_get_original_uri:
> + * @back_forward_list_item: a #WebKitWebHistoryItem
> + *
> + * Returns: the original URI of @back_forward_list_item

Do you mind explaining a bit more how the original URI differs from the URI?