[Webkit-unassigned] [Bug 14811] [gtk] [request] add a webkit_gtk_page_go_to_history_item function

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jan 18 17:20:21 PST 2008


http://bugs.webkit.org/show_bug.cgi?id=14811





------- Comment #16 from jmalonzo at gmail.com  2008-01-18 17:20 PDT -------
(In reply to comment #14)
> >+ * webkit_web_back_forward_list_get_back_item:
> >+ * webkit_web_back_forward_list_get_forward_item:
> 
> I find 'back_item' and 'forward_item' a bit awkward, I would prefer the terms
> 'previous_item' and 'next_item' instead.

back_item and forward_item makes sense for a BackForwardList.

> >+ * webkit_web_back_forward_list_get_item_at_index:
> 
> Consistent naming is desirable, in relation to Glib and Gtk that is. I suggest
> _get_nth_item.

Yup.

> >+ * webkit_web_back_forward_list_get_back_list_count:
> 
> I already commented that back_item feels awkward to me. Further more 'count'
> feels wrong. What about _get_n_previous_items, provided we want the renaming?
> Else it could be _get_n_back_items.

I renamed this to webkit_web_back_forward_list_get_back_list_length. Ditto with
with the forward list. _get_n_back_items suggests that you want to retrieve N
items from the back list. Ditto with _get_n_forward_items.

> >+ * webkit_web_back_forward_list_get_capacity:
> >+ * webkit_web_back_forward_list_set_capacity:
> 
> Consitency again. There is nothing in Glib or Gtk using that terminology. Au
> contraire _get_limit and _set_limit are used with GtkRecentManager.

Yup.

<snip>

> >+        g_free(historyItem->priv->title);
> >+
> >+    if (historyItem->priv->alternateTitle)
> >+        g_free(historyItem->priv->alternateTitle);
> >+
> >+    if (historyItem->priv->uri)
> >+        g_free(historyItem->priv->uri);
> >+
> >+    if (historyItem->priv->originalUri)
> >+        g_free(historyItem->priv->originalUri);
> >+
> >+    G_OBJECT_CLASS(webkit_web_history_item_parent_class)->finalize(object);
> 
> No need to check whether character arrays are empty, g_free does this for you.

Thanks, didn't know that.

> >+WebKitWebHistoryItem* _web_history_item_new_with_core_item(WebCore::HistoryItem* item)
> >+{
> >+    WebKitWebHistoryItem* object;
> >+
> >+    object = kit(item);
> >+
> >+    if (!object) {
> >+        object = WEBKIT_WEB_HISTORY_ITEM(g_object_new(WEBKIT_TYPE_WEB_HISTORY_ITEM, NULL));
> >+        object->priv->historyItem = item;
> >+        webkit_history_item_add(object, object->priv->historyItem);
> >+    }
> >+
> >+    return object;
> >+}
> 
> object seems like a strange choice for a variable name here. And you can merge
> the initialization.

What do you mean by merge the initialization?


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.



More information about the webkit-unassigned mailing list