kdewebkit moved to kdereview

[Benjamin Meyer]

Thanks for fixing up the other stuff.

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

Hmmm I guess openUrlInNewWindows is just as bad for the same reason.   
How about calling it something like middleClickedOnLink() ?

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

Sorry 4.4 and 4.5  Seeing as how KDE requires 4.5 I guess we can just  
remove that.

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

I think this is something that could go upstream.  It is worth asking  
on #qtwebkit at least or on the qtwebkit mailinglist.

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

Yes, but those return a QNetworkAccessManager pointer.  It would be  
nice to have a function that returned the kde network object or 0 if  
it is not set (i.e. do the cast for me)

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

Yah it is a new function in 4.6.  It lets you request freedesktop  
icons and it is obtained from the theme.  For those actions that have  
freedesktop icon names we should fix upstream to help reduce the  
startup load.

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

Thinking about this some more perhaps this means that any application  
that uses QWebPage a webpage can cause file dialog popups to appear.   
Perhaps having a property to handle downloadRequests via kio should be  
added and have it off by default.

-Benjamin Meyer