[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
Wed Jan 16 14:51:06 PST 2008
http://bugs.webkit.org/show_bug.cgi?id=14811
------- Comment #14 from christian at imendio.com 2008-01-16 14:51 PDT -------
(From update of attachment 18468)
>+static void webkit_web_back_forward_list_dispose(GObject* object)
>+{
>+ WebKitWebBackForwardList* webbflist = WEBKIT_WEB_BACK_FORWARD_LIST(object);
>+
>+ delete webbflist->priv->backForwardList;
>+}
Personally I find 'webbflist' unreadable.
Besides you shouldn't use the private pointer directly.
>+static void webkit_web_back_forward_list_init(WebKitWebBackForwardList* object)
>+{
>+ object->priv = WEBKIT_WEB_BACK_FORWARD_LIST_GET_PRIVATE(object);
>+}
This line is superfluous. It doesn't do anything.
>+WebKitWebBackForwardList* webkit_web_back_forward_list_new(WebKitWebView* webView)
This feels wrong, semantically. You don't create an independant instance but
you want to retrieve a list that is tied to the web view. What about
webkit_web_view_get_back_forward_list instead?
>+ * 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.
>+ * 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.
>+ * 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.
>+ * 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.
>+WebCore::BackForwardList* WebKit::core(WebKitWebBackForwardList* webBackForwardList)
>+{
>+ g_return_val_if_fail(WEBKIT_IS_WEB_BACK_FORWARD_LIST(webBackForwardList), NULL);
>+
>+ return webBackForwardList->priv ? webBackForwardList->priv->backForwardList : 0;
Again, you shouldn't use the private pointer directly.
>+static void webkit_history_item_add(WebKitWebHistoryItem* witem, WebCore::HistoryItem* hitem)
>+{
>+ g_return_if_fail(WEBKIT_IS_WEB_HISTORY_ITEM(witem));
>+
>+ GHashTable* table = webkit_history_items();
>+
>+ hitem->ref();
>+ g_hash_table_insert(table, hitem, g_object_ref(witem));
I find hitem and witem slightly confusing, Names such as historyItem and
coreHistoryItem/ coreItem would be far more readable.
>+static void webkit_web_history_item_dispose(GObject* object)
>+{
>+ WebKitWebHistoryItem* item = WEBKIT_WEB_HISTORY_ITEM(object);
>+
>+ webkit_history_item_remove(item->priv->historyItem);
>+
>+ delete item->priv->historyItem;
>+
>+ /* destroy table if empty */
>+ GHashTable* table = webkit_history_items();
>+ if (!g_hash_table_size(table))
>+ g_hash_table_destroy(table);
>+
>+ G_OBJECT_CLASS(webkit_web_history_item_parent_class)->dispose(object);
Again, please don't use the private pointer directly.
>+static void webkit_web_history_item_finalize(GObject* object)
>+{
>+ WebKitWebHistoryItem* historyItem = WEBKIT_WEB_HISTORY_ITEM(object);
>+
>+ if (historyItem->priv->title)
>+ 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.
See above, private.
>+static void webkit_web_history_item_init(WebKitWebHistoryItem* object)
>+{
>+ object->priv = WEBKIT_WEB_HISTORY_ITEM_GET_PRIVATE(object);
>+}
See above, superfluous.
>+
>+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.
See above, private.
>+ * webkit_web_history_item_new:
>+ *
>+ * Creates a new #WebKitWebHistoryItem
>+ *
>+ * Return value: the new #WebKitWebHistoryItem
>+ */
>+WebKitWebHistoryItem* webkit_web_history_item_new(void)
>+{
>+ WebKitWebHistoryItem* object = WEBKIT_WEB_HISTORY_ITEM(g_object_new(WEBKIT_TYPE_WEB_HISTORY_ITEM, NULL));
>+
>+ object->priv->historyItem = new WebCore::HistoryItem();
>+
>+ webkit_history_item_add(object, object->priv->historyItem);
>+
>+ return object;
>+}
See above, object?
See above, private.
>+WebKitWebHistoryItem* webkit_web_history_item_new_with_data(const gchar* uri, const gchar* title)
>+{
>+ WebCore::KURL historyUri(uri);
>+ WebCore::String historyTitle(title);
>+
>+ WebKitWebHistoryItem* object = webkit_web_history_item_new();
>+
>+ object->priv->historyItem = new WebCore::HistoryItem(historyUri, historyTitle);
>+
>+ webkit_history_item_add(object, object->priv->historyItem);
>+
>+ return object;
See above, object?
See above, private.
>+const gchar* webkit_web_history_item_get_title(WebKitWebHistoryItem* webHistoryItem)
>+{
>+ g_return_val_if_fail(WEBKIT_IS_WEB_HISTORY_ITEM(webHistoryItem), NULL);
>+
>+ WebCore::HistoryItem* item = core(WEBKIT_WEB_HISTORY_ITEM(webHistoryItem));
>+
>+ if (!item)
>+ return NULL;
>+
>+ WebCore::String title = item->title();
>+
>+ if (webHistoryItem->priv->title)
>+ g_free(webHistoryItem->priv->title);
>+
>+ webHistoryItem->priv->title = g_strdup(title.utf8().data());
>+
>+ return webHistoryItem->priv->title;
>+}
No need to check whether title is empty.
See above, private.
>+ * webkit_web_history_item_get_alternate_title:
>+ * @webHistoryItem: a #WebKitWebHistoryItem
>+ *
>+ * Returns the alternate title for @this
>+ *
>+ * webkit_web_history_item_set_alternate_title:
>+ * @webHistoryItem: a #WebKitWebHistoryItem
>+ * @title: the alternate title for @this history item
>+ *
>+ * Sets the alternate title for @this
>+ */
What is this? To a new user of the api this has no (obvious) meaning, so please
explain how and what for to use the alternate title.
>+ * webkit_web_history_item_get_original_uri:
>+ * @webHistoryItem: a #WebKitWebHistoryItem
>+ *
>+ * Returns the original URI of @this
Again, please explain what this is, one sentence is enough.
>+struct _WebKitWebHistoryItemClass {
>+ GObjectClass parent;
>+
>+ const gchar* (*get_title) (WebKitWebHistoryItem* history_item);
>+ const gchar* (*get_alternate_title) (WebKitWebHistoryItem* history_item);
>+ const gchar* (*get_uri) (WebKitWebHistoryItem* history_item);
>+ const gchar* (*get_original_uri) (WebKitWebHistoryItem* history_item);
>+ void (*set_alternate_title) (WebKitWebHistoryItem* history_item, const gchar* title);
>+ gdouble (*get_last_visited_time) (WebKitWebHistoryItem* history_item);
>+};
What is this? Looks like Signals.
--
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