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

[Urs Wolfer]

On Friday 02 October 2009 15:45:29 Dawit A. wrote:
> 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:

Great to hear that you have reviewed this code :)
> 
> 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.

Yes, this is true. IMHO both ways are not really nice. It it would be virtual, 
we would have no issues. You suggest removing the getter and setter, right?

> ** 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.

Yes, IIRC it was created for that. (Michael, if you are reading this, please 
correct me if I am wrong.) But I think your suggestion sounds like a fair 
alternative; so go ahead and drop it.

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

Right.


> 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.

isExternalContentAllowed is not overwritten from QWebPage. So I think this 
function makes sense (or did i get something wrong?). About the getter again: 
if we drop the one above, why not this one also? It's not really nice API, I 
agree; though it simplifies ussage a little bit.

> 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.

Of course, yeah. But since most things are just overwritten for internal 
resonons, we just need to document "useful" things. I will read trough the 
headers soon.


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

Please do so! I will check afterwards the code as well.

Anyway, before we will move anything to kdelibs, it has to go into kdereview. 
I will try to do this step as soon as I find a bit of time.

Bye
urs