kdewebkit moved to kdereview

[Dawit A.]

On Monday 26 October 2009 02:24:29 Benjamin Meyer wrote:
[snipped]
> 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.

Fixed the documentation...

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

Fixed the documentation and renamed the signal to 'openUrlInNewWindow'.

Whether you want to handle that signal by opening a new window or a new tab or 
how that tab should be positioned afterwards is an application specific issue. 
This class have no means of even knowing that your application uses tabs.

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

Ahhh... There is only a "#if QT_VERSION < 0x040500" in that code to make it 
work with QtWebKit from Qt 4.4 so I do not understand the 4.6 reference in 
your comment...

As far as that function is concerned, it is only used to do page zooming. I am 
not entirely sure that this would be accepted upstream since one can implement 
this functionality by simply installing an event filter on the QWebPage. It is 
added here following the theme of a convenience function...

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

Ahem... I do not follow... There is already a QWebPage::networkAccessManager() 
and  QWebPage::pluginFactory().

KWebPluginFactory is a pure re-implementation of existing virtual member 
functions and does not provide any additional methods. For the network access 
manager, if you invoked QWebPage::setNetworkAccessManager(...) with an 
instance to a re-implementation, then you already have access to that instance 
or you should simply use qobject_cast to down cast what is returned by 
QWebPage::networkAccessManager....  So I fail to understand what you mean 
here...

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

Done with the exception of the icon names which might be removed based on your 
suggestions below...

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

QIcon::fromTheme ? New function in Qt 4.6 ? I do not see that function in the 
QIcon class documentation. I think there is no objection to do that if it is 
possible, but you have to elaborate on what you mean...

> 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();

Fixed. For what it is worth the current implementation does not crash when 
used from webkitpart, but your point is valid. It might crash depening on how 
the class is used...

> 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

Fixed though I personally think the getters in practise will be rarely used. 
However, point well taken and in fact I have even added functions to remove 
the meta data entries simply for completeness sake.

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

Right. I tested this out and it seems to work as you stated ; so the re-
implementations of those functions have been removed...

> The userAgentForUrl look like it has issues  Is
> KProtocolManager::userAgentForHost(url); setup to only return KHTML
> results?

For the record, there is no such thing as "KHTML results" when it comes to 
user-agent strings in KDE. All user-agent settings are KIO specific. In other 
words, the user-agent settings coming from KProtocolManager apply to all 
applications that use KIO, not just KHTML.

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

That is fixed now. The only thing the userAgentForHost re-implementation should 
be doing is using KProtocolManager::userAgentForHost(...) unless that function 
returns the default user agent string. When it does return the default 
useragent string, it is supposed to revert to using QWebPage::userAgentForUrl.

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

Fixed. Yes, that should actually not be necessary. Even if it was to be left 
it should have been done such that it was restricted to local (read: file) 
resource.

> KWebPage::downloadRequest
> - Shouldn't this call to kget over dcop?

Yes and that is on the TODO list. This code was lifted out of khtml ; so it 
probably needs to be fixed there as well...

> - Isn't there a option if it should pop up the file dialog?

Where would it get the destination filename from ? Hmm... maybe that 
information can be passed through QNetworkRequest::setAttribute function using 
user specific attributes...

> - Might be nice to add api to KIO::file_copy to take a QNetworkRequest

Probably...

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

No objection there. The webkitpart re-implementation already does do ad block 
filtering...

Thanks for your input/suggestions/critique...