[webkit-reviews] review denied: [Bug 69343] [GTK] Initial implementation of back forward list for WebKit2 GTK+ API : [Attachment 109635] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 5 10:05:58 PDT 2011

Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 69343: [GTK] Initial implementation of back forward list for WebKit2 GTK+

Attachment 109635: Patch

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=109635&action=review

Looks quite nice. Why not WebKitBackForwradList instead of

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

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 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
> +
> +WebKitWebBackForwardList*
> +WebKitWebBackForwardListItem*
> +

Might as well let these symbols be mangled and by omittng the

> 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 =
> +    priv->backForwardList =

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.

More information about the webkit-reviews mailing list