On Sunday 25 October 2009 20:02:51 Urs Wolfer wrote:
> I have just moved the kdewebkit lib from
>  playground/libs/webkitkde/kdewebkit into kdereview.

Hi,
a few comments from my side.

- First, it looks pretty minimalistic (in the line of other K* vs Q* classes), 
anyone who says that it adds bloat hasn't looked at the code.

- Why does it link against XML and UiTools? I removed those two from link 
targets and got no error.

- Qt uses "WId" as typedef for window IDs, is there a reason you use 
qlonglong?

- Some comments about "setAllowExternalContent()":

-- I don't understand why it is exposed to both the page and the view. What 
happens when I set it on the view, but change pages later? What happens if I 
move a page to a different view?

-- When the getter is named "isExternalContentAllowed", setter should be named 
"setExternalContentAllowed".

-- The API docs say "remote" content, while the functions use "external". I 
think "remote" is the standard term for non-local content, and should be used 
on the function names as well.

-- Maybe add a Q_PROPERTY for this, makes it scriptable.

- authorizedRequest() -> isRequestAuthorized()

- The API for meta data in KWebPage is limited to QString, while KIO::MetaData 
also allows QVariant for the values. Also, I am not sure if all those meta 
data functions are needed in KWebPage at all. If you change more than one 
value, you keep calling them again and again. I would simply expose some 
setMetaData() and metaData() functions working on the KIO::MetaData directly, 
but I am not sure if it simplifies things or makes them more complicated. But 
from plain intuition, using the maps directly seems more natural.

- KWebPluginFactory::create() has an ugly "two QStringList" way of specifying 
arguments, it should really use a QMap here.

Just my few bucks.

Christoph Feck (kdepepo)