[WebKit-devel] KDE Webkit status update...

[Dawit A.]

On Thursday 01 October 2009 12:14:11 Dawit A. wrote:
> > One comment from my side: I would like to put kdewebkit (the binding
> >  between kdelibs <-> QtWebKit into kdelibs/webkit ASAP. I think the
> > current state of this part is quite good. Most bugs have been fixed
> > there. This way many apps/libs could use KWeb* instead of QWeb so they
> > could profit from the KDE- integration. Any objections / ideas?
> 
> I have no objections to that. That portion has been stable for a while now
>  and has all the KDE integration already....

Actually, I changed my mind on this because I went and reviewed the API in 
KWebView and KWebPage. Here are the issues I see with it right now:

KWebView:
=======
** Why was there a need to override the page() and setPage(...) functions 
there ? The functions in the parent class are not meant to be reimplemented by 
design since they are non-virtual. It is intended to be used such that people 
call setPage(...) with their own implementation of QWebPage if they so choose 
and then when they access it using QWebPage::page. Granted they have to cast 
it into KWebPage, but that is simple enough. I personally think that is how 
the API was intended to be used by design.

** What is the point of setCustomContextMenu function and the associated 
showContextMenu signal ?  If it is to make it easier to recieve the 
contextMenu event for applications that use the api without having to inherit 
from the class, then there is already a proper way to do that in QWidget. Set 
the contextMenuPolicy property on the widget, i.e. KWebView, to 
Qt::CustomContextMenu. and install an event filter on KWebView. Then handle the 
CustomerContextMenu coming from KWebView.  There is no need for additional 
hacks like this at all.

** loadUrl(...) should be renamed to load(...) and made into a regular 
function, not a SLOT, just like those in its parent class.


KWebPage:
=======
** To a lesser extent, KWebPage's setAllowExternalContent and 
isExternalContentAllowed functions have the same issue as the KWebView's 
page() and setPage(...)  functions. Calling KWebPage::networkAccessManager() 
actually returns a custom instance of KIO::AccessManager, i.e. 
NetworkAccessManager. So all one has to do to access the above functions, 
which are wrappers for the ones that exist in KIO::AccessManager, is to simply 
cast whatever is returned by QWebPage::networkAccessManager to 
KIO::AcessManager. However, I can see how both these scenarios are not obvious 
to the developer without either reading the source code or a detailed class 
documentation. In fact I do not have any objections if these two functions 
remain.


Documentation
==========
All the developer facing functions and classes need to be fully and properly 
documented fully and properly as evidenced by some of the points above.


Anyhow, I am going to commit my recent fixes and then clean up the API 
afterwards if there are no valid objections...