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

[bugzilla-daemon at webkit.org]

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


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #109635|review?                     |review-
               Flag|                            |




--- Comment #3 from Martin Robinson <mrobinson at webkit.org>  2011-10-05 10:05:59 PST ---
(From update of attachment 109635)
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.

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