Bug 69343 – [GTK] Initial implementation of back forward list for WebKit2 GTK+ API

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

It's currently missing.

[Carlos Garcia Campos]

Created attachment 109635 [details]
Patch

Initial impementation of bf list. The API is a bit different from webkit1. In wk1 we have WebkitWebHistoryItem, which is a bit confusing, it made me think there was a WebKitWebHistory object. So, I've renamed it to WebKitWebBackForwardListItem. C API doesn't expose AddItem method, so I haven't included it in GTK+ API for now, I've asked apple guys if there's a reason for not exposing such a method. I've tried to not add methods present in wk1 API that don't seem to be used, to the patch a bit smaller too, we can add them in following patches if they are actually needed.

[WebKit Review Bot]

Attachment 109635 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Last 3072 characters of output:
rent_item is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardList.cpp:130:  webkit_web_back_forward_list_get_back_item is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardList.cpp:146:  webkit_web_back_forward_list_get_forward_item is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardList.cpp:163:  webkit_web_back_forward_list_get_nth_item is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardList.cpp:176:  webkit_web_back_forward_list_get_length is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardList.cpp:192:  webkit_web_back_forward_list_get_back_list is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardList.cpp:208:  webkit_web_back_forward_list_get_back_list_with_limit is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardList.cpp:222:  webkit_web_back_forward_list_get_forward_list is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardList.cpp:238:  webkit_web_back_forward_list_get_forward_list_with_limit is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:271:  webkit_web_view_get_back_forward_list is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardListItem.cpp:44:  webkit_web_back_forward_list_item_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardListItem.cpp:51:  webkit_web_back_forward_list_item_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardListItem.cpp:81:  webkit_web_back_forward_list_item_get_uri is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardListItem.cpp:94:  webkit_web_back_forward_list_item_get_title is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardListItem.cpp:107:  webkit_web_back_forward_list_item_get_original_uri is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 67 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 109635 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109635&action=review

Looks quite nice. Why not WebKitBackForwradList instead of WebkitWebBackForwardList?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardList.cpp:38
> +static void webkit_web_back_forward_list_init(WebKitWebBackForwardList* webList)

webList -> list if you rename?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardList.cpp:56
> +    static GHashTable* items = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, g_object_unref);

NULL -> 0

> Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardList.cpp:70
> +    g_hash_table_insert(historyItems(), (gpointer)wkItem, item);

No need for a cast here. Maybe this should happen in webkitWebBackForwardListItemCreate so that if we use it somewhere else we don't have to duplicate this?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardList.cpp:75
> +static GList* webkitWebBackForwardListCreateList(WKArrayRef wkList)

I like the convention wkFoo. We should use that everywhere!

> Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardList.cpp:82
> +    guint listCount = WKArrayGetSize(wkList);
> +    if (!listCount)
> +        return 0;

I don't think this optimization adds much. You can probably just omit it.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardList.cpp:109
> + * Returns the current item in @back_forward_list.

I think this should be expanded a bit. What is the current item exactly and how does the navigation on the page change it? When the user navigates does this BF list update in real-time or do they have to refetch it?

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardList.cpp:130
>> +WebKitWebBackForwardListItem* webkit_web_back_forward_list_get_back_item(WebKitWebBackForwardList* webBackForwardList)
> 
> webkit_web_back_forward_list_get_back_item is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]

I'll work on fixing these annoying style bot issues next.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardListPrivate.h:38
> +G_BEGIN_DECLS
> +
> +WebKitWebBackForwardList* webkitWebBackForwardListCreate(WKBackForwardListRef);
> +WebKitWebBackForwardListItem* webkitWebBackForwardListItemCreate(WKBackForwardListItemRef);
> +
> +G_END_DECLS

Might as well let these symbols be mangled and by omittng the G_BEGIN_DECLS/G_END_DECLS declarations.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:47
>      WebKitWebContext* context;
>  
>      GRefPtr<WebKitWebLoaderClient> loaderClient;
> +    GRefPtr<WebKitWebBackForwardList> backForwardList;

Please move this under context. Let's keep the clients in a clump together.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:62
> +    WebPageProxy* page = webkitWebViewBaseGetPage(WEBKIT_WEB_VIEW_BASE(webView));
> +    priv->backForwardList = adoptGRef(webkitWebBackForwardListCreate(WKPageGetBackForwardList(toAPI(page))));

Why not cache the result of the toAPI call instead?

> Source/WebKit2/UIProcess/API/gtk/tests/testbackforwardlist.c:1
> +/*

I guess this will change once we finish overhauling the unit tests.

[Carlos Garcia Campos]

(In reply to comment #3)
> (From update of attachment 109635 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109635&action=review
> 
> Looks quite nice. Why not WebKitBackForwradList instead of WebkitWebBackForwardList?

Just because wk1 API uses this name, it would make porting easier.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardList.cpp:38
> > +static void webkit_web_back_forward_list_init(WebKitWebBackForwardList* webList)
> 
> webList -> list if you rename?

Yes.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardList.cpp:56
> > +    static GHashTable* items = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, g_object_unref);
> 
> NULL -> 0

Right.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardList.cpp:70
> > +    g_hash_table_insert(historyItems(), (gpointer)wkItem, item);
> 
> No need for a cast here. Maybe this should happen in webkitWebBackForwardListItemCreate so that if we use it somewhere else we don't have to duplicate this?

Yes, the cast is needed, because wkItem is const. Moving it to webkitWebBackForwardListItemCreate() we wouldn't need GetOrCreate().

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardList.cpp:75
> > +static GList* webkitWebBackForwardListCreateList(WKArrayRef wkList)
> 
> I like the convention wkFoo. We should use that everywhere!

Ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardList.cpp:82
> > +    guint listCount = WKArrayGetSize(wkList);
> > +    if (!listCount)
> > +        return 0;
> 
> I don't think this optimization adds much. You can probably just omit it.

Right, the loop won't iterate anyway, so it will return NULL too.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardList.cpp:109
> > + * Returns the current item in @back_forward_list.
> 
> I think this should be expanded a bit. What is the current item exactly and how does the navigation on the page change it? When the user navigates does this BF list update in real-time or do they have to refetch it?

Ok.

> >> Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardList.cpp:130
> >> +WebKitWebBackForwardListItem* webkit_web_back_forward_list_get_back_item(WebKitWebBackForwardList* webBackForwardList)
> > 
> > webkit_web_back_forward_list_get_back_item is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> 
> I'll work on fixing these annoying style bot issues next.

Remember I already filed a bug for this:

https://bugs.webkit.org/show_bug.cgi?id=65179

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardListPrivate.h:38
> > +G_BEGIN_DECLS
> > +
> > +WebKitWebBackForwardList* webkitWebBackForwardListCreate(WKBackForwardListRef);
> > +WebKitWebBackForwardListItem* webkitWebBackForwardListItemCreate(WKBackForwardListItemRef);
> > +
> > +G_END_DECLS
> 
> Might as well let these symbols be mangled and by omittng the G_BEGIN_DECLS/G_END_DECLS declarations.

I don't understand what you mean.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:47
> >      WebKitWebContext* context;
> >  
> >      GRefPtr<WebKitWebLoaderClient> loaderClient;
> > +    GRefPtr<WebKitWebBackForwardList> backForwardList;
> 
> Please move this under context. Let's keep the clients in a clump together.

WebContext is not a client, it's a property.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:62
> > +    WebPageProxy* page = webkitWebViewBaseGetPage(WEBKIT_WEB_VIEW_BASE(webView));
> > +    priv->backForwardList = adoptGRef(webkitWebBackForwardListCreate(WKPageGetBackForwardList(toAPI(page))));
> 
> Why not cache the result of the toAPI call instead?

Cache where? toAPI is just a cast, the page is already cached in parent class. 

> > Source/WebKit2/UIProcess/API/gtk/tests/testbackforwardlist.c:1
> > +/*
> 
> I guess this will change once we finish overhauling the unit tests.

Yes

[Martin Robinson]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 109635 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=109635&action=review
> > 
> > Looks quite nice. Why not WebKitBackForwradList instead of WebkitWebBackForwardList?
> 
> Just because wk1 API uses this name, it would make porting easier.

It's just my opinion, but WebKitWebBackFowardList seems a little redudant. It's the WebKit, so I imagine it's a web back-foward list.  Porting is going to be really tough, but this is just a simple search and replace. Let's not let the old API hold us back.

> > No need for a cast here. Maybe this should happen in webkitWebBackForwardListItemCreate so that if we use it somewhere else we don't have to duplicate this?
> 
> Yes, the cast is needed, because wkItem is const. Moving it to 
webkitWebBackForwardListItemCreate() we wouldn't need GetOrCreate().

You should use a C++ style cast in this cast const_cast<etc>(...). This brings up an interesting problem. The hash you have has a subtle bug. If the WK item is freed and another is reallocated in the same spot the cache will return the wrong item. You'll need to ensure that they are the same item when returning them, I think.

> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardListPrivate.h:38
> > > +G_BEGIN_DECLS
> > > +
> > > +WebKitWebBackForwardList* webkitWebBackForwardListCreate(WKBackForwardListRef);
> > > +WebKitWebBackForwardListItem* webkitWebBackForwardListItemCreate(WKBackForwardListItemRef);
> > > +
> > > +G_END_DECLS
> > 
> > Might as well let these symbols be mangled and by omittng the G_BEGIN_DECLS/G_END_DECLS declarations.
> 
> I don't understand what you mean.

G_BEGIN_DECLS/G_END_DECLS essentially does an extern "C"{ }. This prevents the symbol names from being mangled by the compiler. I think removing those lines will prevent this and make it harder for people to use the private API.

> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:47
> > >      WebKitWebContext* context;
> > >  
> > >      GRefPtr<WebKitWebLoaderClient> loaderClient;
> > > +    GRefPtr<WebKitWebBackForwardList> backForwardList;
> > 
> > Please move this under context. Let's keep the clients in a clump together.

I just think it would be nice to keep the clients in a group at the end. So:
 WebKitWebContext* context
GRefPtr<WebKitWebBackForwardList> backForwardList;
Other property;
Other property;

GRefPtr<WebKitWebLoaderClient> loaderClient;
GRefPtr<WebKitWebLoaderClient> otherClient;

Just a nit!

> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:62
> > > +    WebPageProxy* page = webkitWebViewBaseGetPage(WEBKIT_WEB_VIEW_BASE(webView));
> > > +    priv->backForwardList = adoptGRef(webkitWebBackForwardListCreate(WKPageGetBackForwardList(toAPI(page))));
> > 
> > Why not cache the result of the toAPI call instead?
> 
> Cache where? toAPI is just a cast, the page is already cached in parent class. 

I was just suggesting putting the result of the cast in the local page variable, instead of splitting the conversion from WebKitWebView to the toAPI(page) across two lines.

[Carlos Garcia Campos]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 109635 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=109635&action=review
> > > 
> > > Looks quite nice. Why not WebKitBackForwradList instead of WebkitWebBackForwardList?
> > 
> > Just because wk1 API uses this name, it would make porting easier.
> 
> It's just my opinion, but WebKitWebBackFowardList seems a little redudant. It's the WebKit, so I imagine it's a web back-foward list.  Porting is going to be really tough, but this is just a simple search and replace. Let's not let the old API hold us back.

I agree, and I also prefer WebKitBackForwardList, I'll rename it.

> > > No need for a cast here. Maybe this should happen in webkitWebBackForwardListItemCreate so that if we use it somewhere else we don't have to duplicate this?
> > 
> > Yes, the cast is needed, because wkItem is const. Moving it to 
> webkitWebBackForwardListItemCreate() we wouldn't need GetOrCreate().
> 
> You should use a C++ style cast in this cast const_cast<etc>(...). This brings up an interesting problem. The hash you have has a subtle bug. If the WK item is freed and another is reallocated in the same spot the cache will return the wrong item. You'll need to ensure that they are the same item when returning them, I think.

I used the pointer because items are cached by WebProcess.

> > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebBackForwardListPrivate.h:38
> > > > +G_BEGIN_DECLS
> > > > +
> > > > +WebKitWebBackForwardList* webkitWebBackForwardListCreate(WKBackForwardListRef);
> > > > +WebKitWebBackForwardListItem* webkitWebBackForwardListItemCreate(WKBackForwardListItemRef);
> > > > +
> > > > +G_END_DECLS
> > > 
> > > Might as well let these symbols be mangled and by omittng the G_BEGIN_DECLS/G_END_DECLS declarations.
> > 
> > I don't understand what you mean.
> 
> G_BEGIN_DECLS/G_END_DECLS essentially does an extern "C"{ }. This prevents the symbol names from being mangled by the compiler. I think removing those lines will prevent this and make it harder for people to use the private API.

Ok, I see.

> > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:47
> > > >      WebKitWebContext* context;
> > > >  
> > > >      GRefPtr<WebKitWebLoaderClient> loaderClient;
> > > > +    GRefPtr<WebKitWebBackForwardList> backForwardList;
> > > 
> > > Please move this under context. Let's keep the clients in a clump together.
> 
> I just think it would be nice to keep the clients in a group at the end. So:
>  WebKitWebContext* context
> GRefPtr<WebKitWebBackForwardList> backForwardList;
> Other property;
> Other property;
> 
> GRefPtr<WebKitWebLoaderClient> loaderClient;
> GRefPtr<WebKitWebLoaderClient> otherClient;
> 
> Just a nit!

As you wish :-)

> > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:62
> > > > +    WebPageProxy* page = webkitWebViewBaseGetPage(WEBKIT_WEB_VIEW_BASE(webView));
> > > > +    priv->backForwardList = adoptGRef(webkitWebBackForwardListCreate(WKPageGetBackForwardList(toAPI(page))));
> > > 
> > > Why not cache the result of the toAPI call instead?
> > 
> > Cache where? toAPI is just a cast, the page is already cached in parent class. 
> 
> I was just suggesting putting the result of the cast in the local page variable, instead of splitting the conversion from WebKitWebView to the toAPI(page) across two lines.

I think it doesn't make any difference, we still need two lines, that would only make the first line longer.

[Martin Robinson]

(In reply to comment #6)

> > You should use a C++ style cast in this cast const_cast<etc>(...). This brings up an interesting problem. The hash you have has a subtle bug. If the WK item is freed and another is reallocated in the same spot the cache will return the wrong item. You'll need to ensure that they are the same item when returning them, I think.
> 
> I used the pointer because items are cached by WebProcess.

You'll still need to check that the item is the same in getOrCreate. Otherwise you might return the wrong cached GTK+ item for an particular WK item.

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #6)
> 
> > > You should use a C++ style cast in this cast const_cast<etc>(...). This brings up an interesting problem. The hash you have has a subtle bug. If the WK item is freed and another is reallocated in the same spot the cache will return the wrong item. You'll need to ensure that they are the same item when returning them, I think.
> > 
> > I used the pointer because items are cached by WebProcess.
> 
> You'll still need to check that the item is the same in getOrCreate. Otherwise you might return the wrong cached GTK+ item for an particular WK item.

Right, I now keep a reference of the item to make sure it's not freed.

[Carlos Garcia Campos]

(In reply to comment #5)
> > > No need for a cast here. Maybe this should happen in webkitWebBackForwardListItemCreate so that if we use it somewhere else we don't have to duplicate this?
> > 
> > Yes, the cast is needed, because wkItem is const. Moving it to 
> webkitWebBackForwardListItemCreate() we wouldn't need GetOrCreate().
> 
> You should use a C++ style cast in this cast const_cast<etc>(...). This brings up an interesting problem. The hash you have has a subtle bug. If the WK item is freed and another is reallocated in the same spot the cache will return the wrong item. You'll need to ensure that they are the same item when returning them, I think.

None of the C++ casts work here . . .

[Carlos Garcia Campos]

Created attachment 110130 [details]
Updated patch

Updated patch addressing all comments, expect the C++ style cast.

[Carlos Garcia Campos]

Created attachment 110491 [details]
Updated patch

- Use a HashMap instead of a GHashTable for list items, this way we don't need the cast
- Add tests for can_go_back and can_go_forward, since these methods depend on the back forward list too.

[Martin Robinson]

Comment on attachment 110491 [details]
Updated patch

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

[Carlos Garcia Campos]

(In reply to comment #12)
> (From update of attachment 110491 [details])
> 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.

No, it doesn't matter, I'll change it anyway.
 
> > 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 ?

OK.

> > 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'

Of course not, it's needed to not break the link.

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

Ok.

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

Ok.

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

OK.

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

Ok.

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

Yes, it's the same we are doing in webkit1.

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

We are not leaking memory when returning items, since we don't transfer ownership of items to the caller, the only reference it's owned by the hashmap. 

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

I don't see the point, we are caching list items, it's not a memory leak, we might limit the cache size, though. Items are cached by WebProcessProxy too, so at least the memory of the WK items won't be freed in any case.

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

Added a comment in get_uri() documentation, pointing to this one.

[Carlos Garcia Campos]

Created attachment 110819 [details]
Updated patch

Use new tests stuff and adressed review comments. It depend on bug #70010 too.

[Carlos Garcia Campos]

Created attachment 111588 [details]
New patch

This new patch fixes the problem of list items not being freed. Now list items have a floating reference when they are created and the global cache doesn't keep any reference to the items. Every bf list holds a reference to all of their items, so that when a bf list creates a new item, it consumes the floating reference, and all other bf lists that get this item from the global cache will increase the reference counter. This way an item will be valid as long as it belongs to a bf list. When an item is removed from all bf lists it, its reference counter is 0 and the object is deleted. When an item is deleted it's removed from the global cache.

I've added a changed signal to bf list that is emitted when the list changes (current item is updated, an item is added or one or more items are removed). It allow us to delete objects from our cache, and users will be notified that those items have been removed. 

To make sure items are not leaked I've added a way to watch objects in unit tests. Test class contains a HashSet of objects that are added manually, and removed from the HashSet when the object is finalized. In the class destructor the HashSet should be empty if all watched objects have been deleted. 

I detected a circular dependency between WebView and WebLoaderClient that made the view not being released. I've fixed it to make unit tests pass, but it won't be needed when bug #69358 is fixed.

[Carlos Garcia Campos]

Created attachment 111595 [details]
Another update

Minor update just to use ASSERT instead of g_assert in webkitBackForwardListItemFinalized()

[Carlos Garcia Campos]

Created attachment 111759 [details]
Patch updated to current git master

[Martin Robinson]

Comment on attachment 111759 [details]
Patch updated to current git master

View in context: https://bugs.webkit.org/attachment.cgi?id=111759&action=review

This looks pretty good, but I really dislike returning NULL for empty strings (see below). I also have some suggestions for the test. I would r+ this with those fixes, but I'd like to hear your response first.

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:36
> + * WebKitBackForwardList maintains a list of visited pages used to
> + * used to navigate to recent pages. Items are inserted in the list
> + * in the order they are visited.

Double "used to" here.

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:138
> +        WebKitBackForwardListItem* item = webkitBackForwardListItemGetOrCreate(wkItem);
> +        if (!item)
> +            continue;
> +
> +        list = g_list_prepend(list, item);

I think it would be better to do:

list = g_list_prepend(list, webkitBackForwardListItemGetOrCreate(wkItem));

and then ASSERT(item) before returning in webkitBackForwardListGetOrCreateItem.

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

Might as well use "Returns" everywhere. :)

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:205
> + * Returns the item that succeeds the current item.

succeeds -> follows

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardListItem.cpp:63
> +typedef HashMap<WKBackForwardListItemRef, WebKitBackForwardListItem* > HistoryItemsMap;

No need for the extra space after WebKitBackForwardListItem*. This is only needed when there are two '>' in a row to avoid disambiguities during C++ compilation.

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardListItem.cpp:90
> +                      static_cast<gpointer>(const_cast<OpaqueWKBackForwardListItem*>(wkListItem)));

You can probably omit the surrounding static_cast<gpointer>(). gpointer is a void* and you shouldn't need to cast a non-const pointer to void*.

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardListItem.cpp:104
> + * Returns: the URI of @back_forward_list_item.

Better say or %NULL or return an empty string. I would prefer returning an empty string, but it's up to you. In what situations can this be an empty string? If there's a particular case it's good to mention it.

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardListItem.cpp:124
> + * Returns: the page title of @back_forward_list_item.
> + */

Now here I *definitely* think you should return an empty string. An empty title is different than not having a title. For instance one could make a page that has <title></title>. I don't think that's a NULL case. I don't trust all consumers of the API to null check the return value of these methods, so empty strings everywhere makes the API a bit safer.

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardListItem.cpp:153
> +    WKRetainPtr<WKURLRef> wkOriginalURI(AdoptWK, WKBackForwardListItemCopyOriginalURL(priv->wkListItem.get()));
> +    if (toImpl(wkOriginalURI.get())->string().isEmpty())
> +        return 0;

Please return an empty string instead of null. If the original URI is "" the appropriate result from this function is "".

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:64
> +static void webkitWebViewSetLoaderClient(WebKitWebView* webView, WebKitWebLoaderClient* loaderClient, WKPageRef wkPage)
> +{
> +    webView->priv->loaderClient = loaderClient;
> +    webkitWebLoaderClientAttachLoaderClientToPage(loaderClient, wkPage);
> +}
> +

This change is unrelated. Please remove it from this patch and open a new bug for it. I'm not sure I see the point of splitting it out though.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:72
> -    webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), webkitWebContextGetWKContext(priv->context), 0);
> +    WebPageProxy* page = webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), webkitWebContextGetWKContext(priv->context), 0);
>  
>      static GRefPtr<WebKitWebLoaderClient> defaultLoaderClient = adoptGRef(WEBKIT_WEB_LOADER_CLIENT(g_object_new(WEBKIT_TYPE_WEB_LOADER_CLIENT, NULL)));
> -    webkit_web_view_set_loader_client(webView, defaultLoaderClient.get());
> +    webkitWebViewSetLoaderClient(webView, defaultLoaderClient.get(), toAPI(page));

Please just include the back-foward stuff in this patch.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:247
>      WebPageProxy* page = webkitWebViewBaseGetPage(WEBKIT_WEB_VIEW_BASE(webView));
> -    webkitWebLoaderClientAttachLoaderClientToPage(loaderClient, toAPI(page));
> -    priv->loaderClient = loaderClient;
> +    webkitWebViewSetLoaderClient(webView, loaderClient, toAPI(page));

See above. :)

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:427
> -/*
> +/**

Again, I think you should split this out into a new patch. r+ from me for fixing this small doc problem, just in another commit.

> Source/WebKit2/UIProcess/API/gtk/tests/TestBackForwardList.cpp:26
> +#include <string.h>

Please use cstring here instead of string.h

> Source/WebKit2/UIProcess/API/gtk/tests/TestBackForwardList.cpp:30
> +static const size_t wkBackForwardListLimit = 100;

I think this "wk" should just be "k", since that's the way we typically name globals.

> Source/WebKit2/UIProcess/API/gtk/tests/TestBackForwardList.cpp:64
> +    static void checkItem(WebKitBackForwardListItem* item, const gchar* title, const gchar* uri, const gchar* originalURI)

Please use char insetad of gchar.

> Source/WebKit2/UIProcess/API/gtk/tests/TestBackForwardList.cpp:79
> +    static void checkList(WebKitBackForwardList* list, guint type, WebKitBackForwardListItem** items, guint nItems)

Please use unsigned instead of guint.

> Source/WebKit2/UIProcess/API/gtk/tests/TestBackForwardList.cpp:86
> +        for (GList* listItem = listItems; listItem; listItem = g_list_next(listItem), i++) {

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/TestBackForwardList.cpp:141
> +    void waitUntilLoadFinished()
> +    {
> +        m_hasChanged = false;
> +        LoadTrackingTest::waitUntilLoadFinished();
> +        g_assert(m_hasChanged);
> +    }
> +
> +    void waitUntilLoadFinishedAndCheckRemovedItems(GList* removedItems)
> +    {
> +        m_expectedRemovedItems = removedItems;
> +        waitUntilLoadFinished();
> +        m_expectedRemovedItems = 0;

I think a more generic way to do this would simply be to keep a Vector of added items and a Vector of removed items. After the test runs, compare the lists to what you expect in the test. I think this approach pushes too many assertions into the fixture itself.

> Source/WebKit2/UIProcess/API/gtk/tests/TestBackForwardList.cpp:145
> +    gulong m_changedFlags;

Please use unsigned long here.

> Source/WebKit2/UIProcess/API/gtk/tests/TestMain.h:57
> +    void watchObject(GObject* object)

I think it's good to rename this to something that even an imbecile like me can understand just by looking at it. My suggestion is assertObjectIsDeletedWhenTestFinishes.

[Carlos Garcia Campos]

(In reply to comment #18)
> (From update of attachment 111759 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111759&action=review
> 
> This looks pretty good, but I really dislike returning NULL for empty strings (see below). I also have some suggestions for the test. I would r+ this with those fixes, but I'd like to hear your response first.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:36
> > + * WebKitBackForwardList maintains a list of visited pages used to
> > + * used to navigate to recent pages. Items are inserted in the list
> > + * in the order they are visited.
> 
> Double "used to" here.

oops.

> > Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:138
> > +        WebKitBackForwardListItem* item = webkitBackForwardListItemGetOrCreate(wkItem);
> > +        if (!item)
> > +            continue;
> > +
> > +        list = g_list_prepend(list, item);
> 
> I think it would be better to do:
> 
> list = g_list_prepend(list, webkitBackForwardListItemGetOrCreate(wkItem));
> 
> and then ASSERT(item) before returning in webkitBackForwardListGetOrCreateItem.

Agree, much better, webkitBackForwardListItemGetOrCreate(wkItem) should always return a valid pointer.

> > Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:176
> > + * Return value: (transfer none): a #WebKitWebHistoryItem
> > + *    or %NULL if @back_forward_list is empty.
> 
> Might as well use "Returns" everywhere. :)

Indeed, and #WebKitWebHistoryItem doesn't even exist . . . I'm sorry, I'll fix it

> > Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardList.cpp:205
> > + * Returns the item that succeeds the current item.
> 
> succeeds -> follows

Ok

> > Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardListItem.cpp:63
> > +typedef HashMap<WKBackForwardListItemRef, WebKitBackForwardListItem* > HistoryItemsMap;
> 
> No need for the extra space after WebKitBackForwardListItem*. This is only needed when there are two '>' in a row to avoid disambiguities during C++ compilation.

ah, ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardListItem.cpp:90
> > +                      static_cast<gpointer>(const_cast<OpaqueWKBackForwardListItem*>(wkListItem)));
> 
> You can probably omit the surrounding static_cast<gpointer>(). gpointer is a void* and you shouldn't need to cast a non-const pointer to void*.

I think I tried that, and compiler still complained, but I'll try again.

> > Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardListItem.cpp:104
> > + * Returns: the URI of @back_forward_list_item.
> 
> Better say or %NULL or return an empty string. I would prefer returning an empty string, but it's up to you. In what situations can this be an empty string? If there's a particular case it's good to mention it.

hmm, sure, I forgot to add or %NULL in the docs, sorry. I still think it's better to return NULL instad of an empty string, I think it's a common pattern in gnome platform and what users would expect.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:64
> > +static void webkitWebViewSetLoaderClient(WebKitWebView* webView, WebKitWebLoaderClient* loaderClient, WKPageRef wkPage)
> > +{
> > +    webView->priv->loaderClient = loaderClient;
> > +    webkitWebLoaderClientAttachLoaderClientToPage(loaderClient, wkPage);
> > +}
> > +
> 
> This change is unrelated. Please remove it from this patch and open a new bug for it. I'm not sure I see the point of splitting it out though.

in constructor we don't want to check web view is a web view (g_return macro) and that loaderClient is different than current one, so better move the common code that don't need checks to a helper function. I'll file a separate bug report for this.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:72
> > -    webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), webkitWebContextGetWKContext(priv->context), 0);
> > +    WebPageProxy* page = webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), webkitWebContextGetWKContext(priv->context), 0);
> >  
> >      static GRefPtr<WebKitWebLoaderClient> defaultLoaderClient = adoptGRef(WEBKIT_WEB_LOADER_CLIENT(g_object_new(WEBKIT_TYPE_WEB_LOADER_CLIENT, NULL)));
> > -    webkit_web_view_set_loader_client(webView, defaultLoaderClient.get());
> > +    webkitWebViewSetLoaderClient(webView, defaultLoaderClient.get(), toAPI(page));
> 
> Please just include the back-foward stuff in this patch.

Ok, ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:247
> >      WebPageProxy* page = webkitWebViewBaseGetPage(WEBKIT_WEB_VIEW_BASE(webView));
> > -    webkitWebLoaderClientAttachLoaderClientToPage(loaderClient, toAPI(page));
> > -    priv->loaderClient = loaderClient;
> > +    webkitWebViewSetLoaderClient(webView, loaderClient, toAPI(page));
> 
> See above. :)

Yes, sorry :-P

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:427
> > -/*
> > +/**
> 
> Again, I think you should split this out into a new patch. r+ from me for fixing this small doc problem, just in another commit.

https://bugs.webkit.org/show_bug.cgi?id=70587

> > Source/WebKit2/UIProcess/API/gtk/tests/TestBackForwardList.cpp:26
> > +#include <string.h>
> 
> Please use cstring here instead of string.h

I think we need it for strlen()

> > Source/WebKit2/UIProcess/API/gtk/tests/TestBackForwardList.cpp:30
> > +static const size_t wkBackForwardListLimit = 100;
> 
> I think this "wk" should just be "k", since that's the way we typically name globals.

Ok.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestBackForwardList.cpp:64
> > +    static void checkItem(WebKitBackForwardListItem* item, const gchar* title, const gchar* uri, const gchar* originalURI)
> 
> Please use char insetad of gchar.

Ok.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestBackForwardList.cpp:79
> > +    static void checkList(WebKitBackForwardList* list, guint type, WebKitBackForwardListItem** items, guint nItems)
> 
> Please use unsigned instead of guint.

Ok.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestBackForwardList.cpp:86
> > +        for (GList* listItem = listItems; listItem; listItem = g_list_next(listItem), i++) {
> 
> Ditto.

Ok.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestBackForwardList.cpp:141
> > +    void waitUntilLoadFinished()
> > +    {
> > +        m_hasChanged = false;
> > +        LoadTrackingTest::waitUntilLoadFinished();
> > +        g_assert(m_hasChanged);
> > +    }
> > +
> > +    void waitUntilLoadFinishedAndCheckRemovedItems(GList* removedItems)
> > +    {
> > +        m_expectedRemovedItems = removedItems;
> > +        waitUntilLoadFinished();
> > +        m_expectedRemovedItems = 0;
> 
> I think a more generic way to do this would simply be to keep a Vector of added items and a Vector of removed items. After the test runs, compare the lists to what you expect in the test. I think this approach pushes too many assertions into the fixture itself.

Why is it better to compare two vectors instead of just checking a HashSet is empty?

> > Source/WebKit2/UIProcess/API/gtk/tests/TestBackForwardList.cpp:145
> > +    gulong m_changedFlags;
> 
> Please use unsigned long here.

Ok.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestMain.h:57
> > +    void watchObject(GObject* object)
> 
> I think it's good to rename this to something that even an imbecile like me can understand just by looking at it. My suggestion is assertObjectIsDeletedWhenTestFinishes.

Ok.

[Carlos Garcia Campos]

Created attachment 111959 [details]
New patch

Another update addressing all review comments, except returning empty strings, I still think we should return NULL instead, for the following reasons:

 - It's a common pattern in GNOME platform, so we should be consistent with our platform
 - For this reason, this is what our users expect, look at this ephy commit for example: 
   http://git.gnome.org/browse/epiphany/commit/?id=9ea9e684b4cb5563d5b395a93b923771b0f6bffc
   Even though wk1 returns empty strings, both new and old code in that patch checks for NULL, not to for ""
 - Mac API returns NULL too (well, nil): see http://trac.webkit.org/browser/trunk/Source/WebKit/mac/History/WebHistoryItem.mm#L167
 - It avoids unnecessary allocation and string comparison

[Martin Robinson]

Comment on attachment 111959 [details]
New patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111959&action=review

Okay! We should later use a similar helper to the Mac port for dealing with the empty string -> null return. I think that would cut down on the code and codify this way of writing the API.

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardListItem.cpp:104
> + * Returns: the URI of @back_forward_list_item or %NULL.

Do you mind appending "when the URI is empty" here?

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardListItem.cpp:123
> + * Returns: the page title of @back_forward_list_item or %NULL.

...and "when the title is empty" here?

> Source/WebKit2/UIProcess/API/gtk/WebKitBackForwardListItem.cpp:144
> + * Returns: the original URI of @back_forward_list_item or %NULL.

...and "when the URI is empty" here?

[Carlos Garcia Campos]

Committed r98245: <http://trac.webkit.org/changeset/98245>

[Sam P.]

Would it be possible to expose the AddItem method?  The lack of AddItem is a big blocker for porting the Sugar Browse activity [1] to webkit2.

Browse uses AddItem to load and save a tabs history, which is very important to our browser.

[1]  https://github.com/sugarlabs/browse-activity

[Michael Catanzaro]

(In reply to comment #23)
> Would it be possible to expose the AddItem method?  The lack of AddItem is a
> big blocker for porting the Sugar Browse activity [1] to webkit2.
> 
> Browse uses AddItem to load and save a tabs history, which is very important
> to our browser.
> 
> [1]  https://github.com/sugarlabs/browse-activity

I want it for Epiphany too. Since WK2 we lost the ability to move back/forward lists from one tab to a newly-created tab. This should be a new bug report, though.