[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-core-devel
Subject: Re: kdewebkit moved to kdereview
From: Christoph Feck <christoph () maxiom ! de>
Date: 2009-10-28 3:31:50
Message-ID: 200910280431.50243.christoph () maxiom ! de
[Download RAW message or body]
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)
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic