Bug 14811 – [gtk] [request] add a webkit_gtk_page_go_to_history_item function

• Component: WebKit API
• Status: RESOLVED FIXED

[Diego Escalante Urrelo]

The attached patch works fine in Epiphany, we pass a number like -1 or 1, etc.

[Diego Escalante Urrelo]

Created attachment 15738 [details]
proposed function

This works for me.

[Holger Freyther]

As shortly mentioned on irc. How should the user of the API know which 'i' is allowed? And what a positive and negative means?

There are at least two things to check:
   -A WebCore doesn't crash when giving i's larger than the history.
   -Add another set of methods to query the number of items one can go backward and forward. Get the title for the position i or alternatively return a list (GList) of HistoryItems and add a method to go to a HistoryItem.

[Holger Freyther]

Thanks to Oliver for reminding me. We should always take a look at WebView when adding methods. WebView is Mac's API for WebCore and it is used by a lot of applications like Safari, Mail, but also instant messaging, dashboard, iWeb (for editing) and a lot of other places.

http://developer.apple.com/documentation/Cocoa/Reference/WebKit/Classes/WebView_Class/Reference/Reference.html

has the following methods for history tracking:
– setMaintainsBackForwardList:  
– backForwardList  
– canGoBack  
– goBack  
– goBack:  
– canGoForward  
– goForward  
– goForward:  
– goToBackForwardItem:  

And I think all of them make sense and we should copy them.

[Diego Escalante Urrelo]

(In reply to comment #2)
> As shortly mentioned on irc. How should the user of the API know which 'i' is
> allowed? And what a positive and negative means?
> 
> There are at least two things to check:
>    -A WebCore doesn't crash when giving i's larger than the history.
Just tried a lot of times with random numbers (as provided by g_random_int()), seems like WebCore is fail-proof on this. Also I renamed the function, so for this point if you are satisfied I'll attach a patch for go_to_back_forward_item.

>    -Add another set of methods to query the number of items one can go backward
> and forward. Get the title for the position i or alternatively return a list
> (GList) of HistoryItems and add a method to go to a HistoryItem.
Agree with that. Any idea on how to do it (the right way)?

[Adam Roben (:aroben)]

Comment on attachment 15738 [details]
proposed function

Marking r- for now so the API can be reconsidered.

[Xan Lopez]

FWIW, the proposed method is used in Epiphany to go back/forward several positions in the history from the Back/Forward dropdown menus in the toolbar.

There is a matching API in mozilla to do this, not that it means that it's good design :)

[Jan Alonzo]

Created attachment 17688 [details]
RFC: History API 

Hi! I would like to request for feedback regarding a proposed API for histories. The attached patch adds the following WebView API to GTK+ WebView:

* setMaintainsBackForwardList()
* backForwardList()
* goToBackForwardItem(WebHistoryItem)

backForwardList() returns a WebKitBackForwardList which just encapsulate WebCore::BackForwardList.

WebHistoryItem also just encapsulate a WebCore::HistoryItem but doesn't provide an API for all of HistoryItem's methods like alternateTitle, etc..

I would like to hear your thoughts about this as well as points for improvement if any.

The patch is not for review as it's still lacking an implementation, though I'm already working on that atm.

cheers.

[Christian Dywan]

*** Bug 14913 has been marked as a duplicate of this bug. ***

[Jan Alonzo]

Created attachment 18298 [details]
back/forward list and history item patch

This patch wraps WebCore's BackForwardList and HistoryItem. Also, use the back/forward list when traversing previous and next items in the WebKitWebView.

Feedback appreciated.

[Alp Toker]

Comment on attachment 18298 [details]
back/forward list and history item patch

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.

[Jan Alonzo]

Created attachment 18468 [details]
updated gtk port back/forward list and history item patch

Hi alp

I have cleaned up and updated the patch as per your suggestions. Thanks. 

Please let me know if you any more suggestions or feedback about the patch. Cheers.

[Alp Toker]

Created attachment 18470 [details]
API fix and documentation

New symbols added, old symbols deprecated.

[Alp Toker]

Comment on attachment 18470 [details]
API fix and documentation

Attached this to the wrong bug, oops!

[Christian Dywan]

Comment on attachment 18468 [details]
updated gtk port back/forward list and history item patch

>+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.

[Xan Lopez]

(In reply to comment #14)
> (From update of attachment 18468 [details] [edit])
> >+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.
> 

That line sets the priv pointer in the struct to the object private data, so it does something. And accessing directly the priv pointer from within the object implementation is an extremely widely used technique/optimization in the GTK+ world.

[Jan Alonzo]

(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?

[Jan Alonzo]

Created attachment 18537 [details]
updated back forward patch

Updated patch to cleanup g_free usage, use _nth_item instead of _item_at_index, use _limit instead of _capacity, use _back_length instead of _back_list_count - ditto with _forward_list_count, and documentation fixes.

cheers

[Alp Toker]

I didn't review this fully yet.

+static void webkit_web_back_forward_list_finalize(GObject* object)
+{
+    G_OBJECT_CLASS(webkit_web_back_forward_list_parent_class)->finalize(object);
+}

Is this redundant?

[Christian Dywan]

(In reply to comment #17)
> Created an attachment (id=18537) [edit]
> updated back forward patch

> webkit_web_back_forward_list_new_with_view

This should be _new_with_web_view.

[Jan Alonzo]

(In reply to comment #18)
> I didn't review this fully yet.
> 
> +static void webkit_web_back_forward_list_finalize(GObject* object)
> +{
> +   
> G_OBJECT_CLASS(webkit_web_back_forward_list_parent_class)->finalize(object);
> +}
> 
> Is this redundant?

hi alp! why do you think it's redundant?

[Xan Lopez]

(In reply to comment #20)
> (In reply to comment #18)
> > I didn't review this fully yet.
> > 
> > +static void webkit_web_back_forward_list_finalize(GObject* object)
> > +{
> > +   
> > G_OBJECT_CLASS(webkit_web_back_forward_list_parent_class)->finalize(object);
> > +}
> > 
> > Is this redundant?
> 
> hi alp! why do you think it's redundant?
> 

It only chains to the parent class impl, so if you are not planning to do anything else it is useless.

[Jan Alonzo]

Created attachment 18577 [details]
updated patch 

@Christian, thanks. Renamed it to _new_with_web_view

@alp, @xan: Thanks. Removed that line.

cheers,

[Xan Lopez]

(In reply to comment #22)
> Created an attachment (id=18577) [edit]
> updated patch 
> 
> @Christian, thanks. Renamed it to _new_with_web_view
> 
> @alp, @xan: Thanks. Removed that line.

Hehe, now it's wrong!

If you are not going to do anything in finalize just don't set your handler in class_init and you will automatically use your parent's. If you *do* set your handler though, you have to chain.

So remove the function and the assignment in class_init.

> 
> cheers,
> 

[Jan Alonzo]

Created attachment 18578 [details]
updated patch to remove finalize

@xan: lol, thanks a lot. updated the patch to remove finalize.

[Alp Toker]

+    if (!webViewData->corePage || !webViewData->corePage->backForwardList()->enabled()) {
+        return NULL;
+    }

Shouldn't have braces.

+const gchar* webkit_web_history_item_get_original_uri (WebKitWebHistoryItem* webHistoryItem)

Shouldn't have a space before the parenthesis. I saw this in a few places.

+ * Returns the time @webHistoryItem was last visited

This doc seems unclear to me. The time /since/ it was last visited, or was something else meant?

[Pierre-Luc Beaudoin]

I think _WebKitWebHistoryItemPrivate should be define in webkitprivate.h like it has been done with WebView, WebFrame and NetworkRequest.

[Christian Dywan]

(In reply to comment #26)
> I think _WebKitWebHistoryItemPrivate should be define in webkitprivate.h like
> it has been done with WebView, WebFrame and NetworkRequest.

I don't think so. A separate header for privates in one of the quirks we should rather get rid of instead of adding to.

[Alp Toker]

(In reply to comment #27)
> (In reply to comment #26)
> > I think _WebKitWebHistoryItemPrivate should be define in webkitprivate.h like
> > it has been done with WebView, WebFrame and NetworkRequest.
> 
> I don't think so. A separate header for privates in one of the quirks we should
> rather get rid of instead of adding to.
> 

Agreed. Privates should be private and scoped to one file wherever possible. (webkitprivate.h should have been called webkitinternal.h really, but doesn't matter really.)

[Pierre-Luc Beaudoin]

In webkit_web_history_item_get_uri and webkit_web_history_item_get_title you are making copies of the strings to save into WebHistoryItemPrivate.  Is there a reason?

[Christian Dywan]

(In reply to comment #29)
> In webkit_web_history_item_get_uri and webkit_web_history_item_get_title you
> are making copies of the strings to save into WebHistoryItemPrivate.  Is there
> a reason?

The reason is that by convention developers expect a constant string. And since WebKit doesn't use plain character arrays internally it needs to act accordingly. You can't rely on the .utf8() buffer's lifetime.

[Alp Toker]

Comment on attachment 18578 [details]
updated patch to remove finalize

Clearing r?

Please fix the minor issues I listed and sync this with SVN TOT. I think it's good for landing then.

[Alp Toker]

Jan, is there any chance of getting this cleaned up soon? I'd like to get it in before #17006 (a big move of source files) happens so it doesn't fall further out of sync.

[Jan Alonzo]

Created attachment 18693 [details]
cleanup coding style and doc

Hi alp! I've cleaned up the coding style and fix the lastVisitedTime() doc. Let me know if there are still issues.

Regards,

[Jan Alonzo]

Comment on attachment 18693 [details]
cleanup coding style and doc

Found a bug, removing for review flag..

[Jan Alonzo]

Created attachment 18701 [details]
updated patch

This is the correct patch. The previous patch gives a segfault on web_view_finalize.

[Alp Toker]

Comment on attachment 18701 [details]
updated patch

Will fix up incorrect whitespace in the .pro file before landing.

Some of the null checks after core() may be excessive, but it's ok to stay safe for now.

[Alp Toker]

Landed in r29821.

We should keep an eye on this API and see how apps make use of it, make sure there's no redundancy etc. as usual.