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

[Michael Howell]

On Friday 02 October 2009 13:40:39 Urs Wolfer wrote:
> 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 :)
+1

> > 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?
KWebView::page() was overwritten to avoid casting. It's okay to call 
QWebView::page(), so being non-virtual isn't a huge problem. Overriding 
KWebView::setPage() is simply to make it harder to set page() to some non-
KWebPage. It's possible, but doesn't really work...

> > ** 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.
+1. I wasn't aware of Qt::CustomContextMenu.

> > ** 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.
It does make sense. It'd make even more sense if KWebView also had the 
external content stuff, the same way QWebView has the QWebPage functions.

> > 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
> _______________________________________________
> WebKit-devel mailing list
> WebKit-devel at kde.org
> https://mail.kde.org/mailman/listinfo/webkit-devel
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/webkit-devel/attachments/20091002/ca12c833/attachment.sig