[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+
API
https://bugs.webkit.org/show_bug.cgi?id=69343
Attachment 109635: Patch
https://bugs.webkit.org/attachment.cgi?id=109635&action=review
------- 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
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.
More information about the webkit-reviews
mailing list