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

[Dawit A.]

On Friday 02 October 2009 18:59:29 Michael Howell wrote:
[snipped]

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

By their very design, intentionally or otherwise, the authors of QWebView want 
you to go either one of two routes. Either downcast whatever page() returns or 
use composition which would be more complex and unnecessarily complicated in 
this case. Since the only casting you have to do is when you call page(), it 
should not be that much of a burden on anyone using the API. Actually it is a 
very good thing in this case because it will be self documenting the down cast 
you do. And if they do not need to use any of the KWebPage specific functions, 
then they do not need to perform the cast. Almost always it is a good idea to 
avoid function overriddes because of the potential for unforseen problems or 
misuses down the line.  In other words, make the users of the API downcast for 
their own good. :)

Oh and as far as making sure a KWebPage object is created everytime you create 
an instance of KWebView, all you have to do is is call the setPage function in 
its constructor just like we do with setCookieJar or setPluginFactory function 
calls...

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

Yes, this is fine with me as well. It just needs to be better documented as 
with all the other functions...

One more additional issue which I am most definitely going to fix is SearchBar 
being exposed through a public QWidget* searchBar() accessor function. This is 
complete no no to me especially since the SearchBar API itself is private! 
Anyhow, it is simple enough to address this matter through few slots and 
perhaps signals...