[WebKit-devel] Reviewing kdewebkit

[Dawit A.]

On Wednesday 11 November 2009 21:21:24 Andrea Diamantini wrote:
> Hi all,
> 
> Today I tried having a mini review of kdewebkit lib and API. I came out
>  with a lot of questions and (probably) no real hints :) Anyway, here you
>  are firsts:
> 
> 1.
> I noticed the private implementation of KIO::AccessManager does an heavy
>  use of its parent, calling it for every createRequest() call. Shouldn't be
>  better adding a KWebPage pointer there and using it?

That can and probably will be remedied. It was orignally implemented to simply 
guard against someone setting the parent at a later point than the 
construction of the object. However, I doubt we need to worry about such 
corner cases specially when it can potentially have adverse impact on 
performance...

> 2.
> In KWebPage we have downloadRequest public function. Doc suggests we can
> connect it to QWebPage::downloadRequested signal. But this function is
>  *not* a slot. Am I missing something or it has been just forgotten?

No, it is not forgotten. The function documentation states and I quote

"For example, you can call this function when you receive
@ref QWebPage::downloadRequested signal to download the
the request through KIO."

It does not say you can connect to this slot because obviously it is not a 
slot as you already stated. ;)  The documentation simply is attempting to give 
you an an example of how you can use the said function, i.e. from your own 
slot that connects to the said signal.

> 3.
> I'm having some problems with KWebView signals.
> Surely it's just me, but seeing for example the linkShiftClicked() signal I
> ran to connect it to KWebPage::downloadRequest(). And I couldn't because of
> parameters. :(

So what you actually want is a

public Q_SLOTS:
	void downloadRequest(const QUrl& url);

Well that can be added and made to simply call downloadRequest(...). Note that 
KWebPage does not currently have any slots because its parent does not have 
any and tries to model itself after the parent class. Remember it is simply a 
convenience wrapper class, i.e. drop in replacement for the parent.

> 4.
> I think the qt 4.6 checks in the code are unuseful because and should be
> removed as Qt 4.6 is going to be an hard dependency of the whole KDE 4.4

You would not say that if you were developing the class and do not have Qt 
4.6... ;) IOW, that is a non-issue that can be removed once KDE 4.4 has been 
released...