kdewebkit moved to kdereview

[Benjamin Meyer]

On Oct 26, 2009, at 1:01 AM, Dawit A. wrote:

> On Sunday 25 October 2009 23:48:27 Benjamin Meyer wrote:
>>> Factory in the kdewebkit module that specifically does that...
>>>
>>>>> * KDE dialogs for javascript dialogs (alert, file picker, ..)
>>>>
>>>> The KDE File dialog should already be used starting with I believe
>>>> KDE
>>>> 4.3 when the static QFileDialog pointers were overloaded so anytime
>>>> you call the static QFileDialog functions it will use the KDE File
>>>> Dialog.  As for the alert dialog while it does use QMessageBox,
>>>> QMessageBox should be using the KDE icon from the style which is  
>>>> the
>>>> only thing I could see the KMessageBox adding.
>>>>
>>>> So what goes kdewebkit bring us that we don't either already have  
>>>> or
>>>> will be getting with Qt4.6?
>>>
>>> See my response/example few threads back. But basically two out of
>>> the three
>>> public classes, KWebView and KWebPage, in kdewebkit are nothing more
>>> than
>>> simple convenience wrappers that provide complete KDE integration
>>> out of the
>>> box. That is you get the critical KIO, KCookieJar integration by
>>> doing nothing
>>> more than "KWebView *view = new KWebView (parent);".
>>
>> The code example doesn't show why there needs to be a KWebView.
>
> It does exactly what a convenience class is supposed to do...
>
> #1. Calls setPage(new KWebPage(this)) in its ctor...
> #2. Provides access to a couple of functions specific to our  
> implementation of
> QNetworkAccessManager...
> #3. Provides three signals related to mouse clicks...
>
>> Looking at the example the majority of the code bloat was on the kde
>> side and if all it does is set the network manager and plugin factory
>> we should make it easy for people to still use their existing  
>> QtWebKit
>> code and only inject two lines with something like:
>>
>> webPage->setNetworkAccessManager(new KNetworkAccessManager());
>> webPage->setPluginFactory(new KWebPluginFactory());
>
> With the exception of the missing CookieJar integration and the  
> incorrect
> class names, there is absolutely nothing that stops you from doing  
> just that!
> As I have already stated these are convenient classes to make the  
> life of a
> KDE app developer easier!  Can you please clarify where exactly is  
> the "code
> bloat" ???
>
>> A while back I was looking at the code and it looked like the  
>> KWebView
>> and KWebPage were tied together.
>
> And the keyword with that statement is a "while back". A lot of  
> things have
> changed with the kdewebkit module in the past couple of months...
>
>> It would be nice if I could stick a QWebPage on a KWebView or a  
>> KWebPage in
>> a QWebView.
>
> Nothing stops you from doing that either! But you loose the KDE  
> integration
> you get for free and will have to do the integration manually  
> yourself!
>
>> Application will want to overload these.
>
> Overload what ? Do you mean reimplement these classes ? If so, then  
> u can do
> that just the same way you can their parent classes. Infact the  
> webkitpart
> does that very thing itself, i.e. reimplements both KWebView and  
> KWebPage...
>
>> Also KWebPage should never require KWebView because applications  
>> will want
>> to use KWebPage without KWebView or QWebView, but as a stand along  
>> object
>> (many applications already do this).
>
> It does not! You can see the code for yourself at
> http://websvn.kde.org/trunk/kdereview/kdewebkit/


Glad to see that was improved.  Looking at the api here are some notes:

QWebView's isExternalContentAllowed function docs should point to  
KWebPage's documentation to indicate that the functions are nothing  
more then wrappers around the functions in KWebPage.  In both classes  
what is "external" is never defined in the docs.  And perhaps they  
should also point to KIO::AccessManager's external functions as they  
are both nothing more then wrappers around those functions.

openUrlInNewTab docs are incorrect.  It is also emited if you ctrl  
click on a link.

openUrlInNewWindow seems to be missing for when the user would click  
on a link with those keyboard modifiers.

What about the about when the user wants to open the link in a new tab  
and have it be selected?

The wheel event code does not do the same thing in qt 4.5 and 4.6.   
Should this wheel behavior actually go upstream into qtwebkit?

There seems to be missing convenience functions to get the kde network  
access manager and plugin factory

As QL1S is defined might as well use it in the code everywhere

KWebPage sets a ton of icons on the actions when the KWebPage is  
created.  This slows down the startup time and really upstream should  
get a patch to use QIcon::fromTheme if it doesn't already.  Same goes  
for the shortcuts.

In the constructor this bit of code seems wrong.  Does a view() exist  
at this point?  What if I set a view later?  Could this crash?
   qlonglong windowId = view()->window()->winId();

The code style is inconsistent, sometime is it foo &bar other times it  
is foo& bar

setSessionMetaData and setRequestMetaData don't have getters and the  
docs don't link to the kio docs and give a hint that they are nothing  
more then frontends

chooseFile, javaScriptAlert javaScriptPrompt, and javaScriptConfirm  
shouldn't need to be overloaded.  KFileDialog should show up and the  
messagebox's shown by QtWebKit should be identical to the ones  
KWebPage has.  (ignoring the null pointer checks that are missing that  
will cause a crash)

The userAgentForUrl look like it has issues  Is  
KProtocolManager::userAgentForHost(url); setup to only return KHTML  
results? messing around with hardcoded string offsets of the  
QWebPage::userAgentForUrl() I think is asking for trouble.  If you  
need the WebKit version you can get that from qtwebkitversion.h  It  
would be nice if this was just
{
  return KProtocalManager::userAgentForHost(url, defaultUserAgent);
}

meant to remove this?
//kDebug() << userAgent;

KWebPage::createPlugin shouldn't do anything by default.  That is what  
KWebPluginFactory is for.  Besides the current code might have  
security issues.

KWebPage::downloadRequest
- Shouldn't this call to kget over dcop?
- Isn't there a option if it should pop up the file dialog?
- Might be nice to add api to KIO::file_copy to take a QNetworkRequest

authorizedRequest looks nice, we can drop in the adblock network code  
I wrote for Arora into that very easily.

-Benjamin Meyer