Bug 14811

Summary: [gtk] [request] add a webkit_gtk_page_go_to_history_item function
Product: WebKit Reporter: Diego Escalante Urrelo <diegoe>
Component: WebKit APIAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: alp, christian, cosimoc, diegoe, gwright, jmalonzo, xan.lopez
Priority: P2 Keywords: Gtk
Version: 523.x (Safari 3)   
Hardware: PC   
OS: OS X 10.4   
Attachments:
Description Flags
proposed function
aroben: review-
RFC: History API
none
back/forward list and history item patch
alp: review-
updated gtk port back/forward list and history item patch
none
API fix and documentation
none
updated back forward patch
none
updated patch
none
updated patch to remove finalize
none
cleanup coding style and doc
none
updated patch alp: review+

Description Diego Escalante Urrelo 2007-07-30 01:40:03 PDT
The attached patch works fine in Epiphany, we pass a number like -1 or 1, etc.
Comment 1 Diego Escalante Urrelo 2007-07-30 01:40:54 PDT
Created attachment 15738 [details]
proposed function

This works for me.
Comment 2 Holger Freyther 2007-07-30 01:57:44 PDT
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.
Comment 3 Holger Freyther 2007-07-30 02:13:59 PDT
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.
Comment 4 Diego Escalante Urrelo 2007-07-30 03:09:59 PDT
(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)?

Comment 5 Adam Roben (:aroben) 2007-08-18 16:54:53 PDT
Comment on attachment 15738 [details]
proposed function

Marking r- for now so the API can be reconsidered.
Comment 6 Xan Lopez 2007-10-06 14:44:53 PDT
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 :)
Comment 7 Jan Alonzo 2007-12-03 15:59:54 PST
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.
Comment 8 Christian Dywan 2007-12-27 14:40:59 PST
*** Bug 14913 has been marked as a duplicate of this bug. ***
Comment 9 Jan Alonzo 2008-01-06 03:01:58 PST
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.
Comment 10 Alp Toker 2008-01-07 15:28:45 PST
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.
Comment 11 Jan Alonzo 2008-01-16 03:31:32 PST
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.
Comment 12 Alp Toker 2008-01-16 05:43:36 PST
Created attachment 18470 [details]
API fix and documentation

New symbols added, old symbols deprecated.
Comment 13 Alp Toker 2008-01-16 05:45:45 PST
Comment on attachment 18470 [details]
API fix and documentation

Attached this to the wrong bug, oops!
Comment 14 Christian Dywan 2008-01-16 14:51:05 PST
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.
Comment 15 Xan Lopez 2008-01-16 15:14:45 PST
(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.
Comment 16 Jan Alonzo 2008-01-18 17:20:21 PST
(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?
Comment 17 Jan Alonzo 2008-01-18 19:45:08 PST
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
Comment 18 Alp Toker 2008-01-21 09:06:20 PST
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?
Comment 19 Christian Dywan 2008-01-21 09:55:10 PST
(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.
Comment 20 Jan Alonzo 2008-01-21 10:28:22 PST
(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?

Comment 21 Xan Lopez 2008-01-21 10:30:45 PST
(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.
Comment 22 Jan Alonzo 2008-01-21 10:38:40 PST
Created attachment 18577 [details]
updated patch 

@Christian, thanks. Renamed it to _new_with_web_view

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

cheers,
Comment 23 Xan Lopez 2008-01-21 10:49:30 PST
(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,
> 

Comment 24 Jan Alonzo 2008-01-21 11:25:08 PST
Created attachment 18578 [details]
updated patch to remove finalize

@xan: lol, thanks a lot. updated the patch to remove finalize.
Comment 25 Alp Toker 2008-01-21 13:53:56 PST
+    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?

Comment 26 Pierre-Luc Beaudoin 2008-01-23 09:32:49 PST
I think _WebKitWebHistoryItemPrivate should be define in webkitprivate.h like it has been done with WebView, WebFrame and NetworkRequest.
Comment 27 Christian Dywan 2008-01-23 09:40:12 PST
(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.
Comment 28 Alp Toker 2008-01-23 09:54:32 PST
(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.)
Comment 29 Pierre-Luc Beaudoin 2008-01-23 14:31:34 PST
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?
Comment 30 Christian Dywan 2008-01-24 01:23:54 PST
(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.
Comment 31 Alp Toker 2008-01-25 08:11:56 PST
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.
Comment 32 Alp Toker 2008-01-25 08:20:26 PST
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.
Comment 33 Jan Alonzo 2008-01-25 22:00:01 PST
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,
Comment 34 Jan Alonzo 2008-01-25 23:41:07 PST
Comment on attachment 18693 [details]
cleanup coding style and doc

Found a bug, removing for review flag..
Comment 35 Jan Alonzo 2008-01-26 00:16:45 PST
Created attachment 18701 [details]
updated patch

This is the correct patch. The previous patch gives a segfault on web_view_finalize.
Comment 36 Alp Toker 2008-01-27 11:44:55 PST
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.
Comment 37 Alp Toker 2008-01-27 11:51:41 PST
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.