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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 5 23:53:04 PDT 2011


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





--- Comment #4 from Carlos Garcia Campos <cgarcia at igalia.com>  2011-10-05 23:53:04 PST ---
(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

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



More information about the webkit-unassigned mailing list