[Webkit-unassigned] [Bug 69343] [GTK] Initial implementation of back forward list for WebKit2 GTK+ API

Thu Oct 20 11:23:20 PDT 2011

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

--- Comment #18 from Martin Robinson <mrobinson at webkit.org>  2011-10-20 11:23:19 PST ---
(From update of attachment 111759)
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.

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