[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





--- Comment #6 from Carlos Garcia Campos <cgarcia at igalia.com>  2011-10-06 08:29:06 PST ---
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 109635 [details] [details] [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.
> 
> It's just my opinion, but WebKitWebBackFowardList seems a little redudant. It's the WebKit, so I imagine it's a web back-foward list.  Porting is going to be really tough, but this is just a simple search and replace. Let's not let the old API hold us back.

I agree, and I also prefer WebKitBackForwardList, I'll rename it.

> > > 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().
> 
> You should use a C++ style cast in this cast const_cast<etc>(...). This brings up an interesting problem. The hash you have has a subtle bug. If the WK item is freed and another is reallocated in the same spot the cache will return the wrong item. You'll need to ensure that they are the same item when returning them, I think.

I used the pointer because items are cached by WebProcess.

> > > > 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.
> 
> G_BEGIN_DECLS/G_END_DECLS essentially does an extern "C"{ }. This prevents the symbol names from being mangled by the compiler. I think removing those lines will prevent this and make it harder for people to use the private API.

Ok, I see.

> > > > 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.
> 
> I just think it would be nice to keep the clients in a group at the end. So:
>  WebKitWebContext* context
> GRefPtr<WebKitWebBackForwardList> backForwardList;
> Other property;
> Other property;
> 
> GRefPtr<WebKitWebLoaderClient> loaderClient;
> GRefPtr<WebKitWebLoaderClient> otherClient;
> 
> Just a nit!

As you wish :-)

> > > > 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. 
> 
> I was just suggesting putting the result of the cast in the local page variable, instead of splitting the conversion from WebKitWebView to the toAPI(page) across two lines.

I think it doesn't make any difference, we still need two lines, that would only make the first line longer.

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