kdewebkit moved to kdereview

[Lukas Appelhans]

Am Montag 26 Oktober 2009 20:38:08 schrieb 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...
Well as KGet lies in KDENetwork noone guarantees that it is installed... in 
the Webkit-Part is already a tweak on calling a download manager like KGet...

I'm not the expert here though, maybe Urs can clarify... :)

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