kdewebkit moved to kdereview

[Dawit A.]

On Wednesday 28 October 2009 18:43:02 Benjamin Meyer wrote:
> 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() ?

hmm... Sounds good to me but what the CTRL+click causing this very signal to 
be emitted ?

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

Yes, that is bagage carried over from the initial work. In fact that function 
might need some more work since attempt to zoom in/out of pages quickly 
produces noticable lag.

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

No objection to it going upstream...
 
> >> 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)

Well the problem with that is I have to use the same accessor name, 
networkAccessManager(),  which will effectively hide the member function in the 
parent class. Hence, you will not be able to get back a reference to 
QNetworkAccessManager if do not use a KIO::AccessManager on this class. That 
behavior would be different from the version in the parent class. Or are you 
asking I use a different name perhaps something like "netAccessManager()", but 
that would be even more confusing...
 
> > 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.

Already did a #if for this and I do not mind this stuff going upstream for the 
ones that can be done there, specially if it reduces unecessary load time. How 
about the shortcut keys ? Are they already taken care of by QKeySequence ?
 
> >> 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.

I do not really follow what you are saying here. What do you mean by "any 
application that uses QWebPage a webpage can cause file dialog popups to 
appear" ?