From kde-core-devel  Wed Oct 28 05:16:48 2009
From: "Dawit A." <adawit () kde ! org>
Date: Wed, 28 Oct 2009 05:16:48 +0000
To: kde-core-devel
Subject: Re: kdewebkit moved to kdereview
Message-Id: <200910280116.48351.adawit () kde ! org>
X-MARC-Message: https://marc.info/?l=kde-core-devel&m=125670716214826

On Tuesday 27 October 2009 23:31:50 Christoph Feck wrote:
> On Sunday 25 October 2009 20:02:51 Urs Wolfer wrote:
> - Qt uses "WId" as typedef for window IDs, is there a reason you use
> qlonglong?

The API is not about Qt but KDE. The window-id is modeled after the one used 
in KIO which is a 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?

That works exactly like how any of the setter functions such as 
setNetworkAccessManager, setCookieJar etc work in QWebPage.
 
> -- 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.

Those APIs have to stay that way because the ones they refer to or are a 
convenience for in KIO::AccessManager have already been released in KDE 4.1. 
With that in mind, changing the names in these classes would not buy us 
anything.

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

No objection to this though I fail to see the benefit. I guess it needs to be 
added to KWebView only ??

> - authorizedRequest() -> isRequestAuthorized()

No because authorizedRequest() is actually authorizedRequest(const QUrl&). It 
is not a simple bool check function since It takes a url as a parameter. I 
could be wrong, but I believe Qt follows a similar mantra as well...
 
> - The API for meta data in KWebPage is limited to QString, while
>  KIO::MetaData also allows QVariant for the values. 

KIO::MetaData inherits QMap<QString, QString>. The QVariant conversion 
functions do not do what you think they do... They are there to simply convert 
a QMap<QString,QVariant> to QMap<QString,QString>. Nothing more...

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

There is a reason why those maps are not exposed. They are internal 
implementation detail. In other words, we do not want to expose them. Hence 
the APIs. In reality, you only set a meta data, be it a per session or a per 
request one, and never worry about removing it or querying it. Those functions 
are there for completeness. Just in case you need them for whatever reason...

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

See QWebPluginFactory::create().

Thanks for the input/suggestions...