Since we have a WebKit2 port, we need to apply this implementation policy.
Created attachment 64503 [details] proposed patch
Comment on attachment 64503 [details] proposed patch WebCore/ChangeLog:16 + (WebCore::Cursor::Cursor): Remove the ifdef. Also removed for EFL because it seems like totally useless since the EFL Can you wrap these lines? It's just too long.
Created attachment 64565 [details] proposed patch Changelog lines have been shortened somewhat :) Added copyright for CursorQt.cpp.
Comment on attachment 64565 [details] proposed patch WebCore/ChangeLog:15 + * platform/Cursor.h: Typedef PlatformCursor to be a QCursor* to be able create it dinamically. dynamically WebCore/ChangeLog:17 + useless since the EFL PlatformCursor is const char* so there is no problem with initalizing it to zero. dont make a EFL change as part of this patch please WebCore/platform/qt/CursorQt.cpp:73 + #endif I guess you need to return 0 after this WebCore/platform/qt/CursorQt.cpp:83 + m_platformCursor = new QCursor(Qt::ArrowCursor); wouldn't a lookup table be better? WebCore/platform/qt/CursorQt.cpp:86 + m_platformCursor = new QCursor(Qt::CrossCursor); Where are these being deleted? Does the Qt QWidget::setCursor stuff still work after this change? Please test.
> > WebCore/platform/qt/CursorQt.cpp:73 > + #endif > I guess you need to return 0 after this Good catch. > > WebCore/platform/qt/CursorQt.cpp:83 > + m_platformCursor = new QCursor(Qt::ArrowCursor); > wouldn't a lookup table be better? There is not other way of mapping from cursor types to QCursor objects than hard coding it so the best lookup table is a switch :) > > WebCore/platform/qt/CursorQt.cpp:86 > + m_platformCursor = new QCursor(Qt::CrossCursor); > Where are these being deleted? Right, it should be deleted in the destructor. > > > Does the Qt QWidget::setCursor stuff still work after this change? Please test. I did not see any regressions however I just did a quick browsing with the change. I would use the bots for testing. I will upload a new patch that fixes the problems you find (with the EFL and the spelling thing).
Antonio, you know how this is supposed to work and how to test it. can you help?
Created attachment 64566 [details] patch v3
Comment on attachment 64566 [details] patch v3 Augh, this could leak QCursors. Need to be fixed again.
Created attachment 64574 [details] proposed patch Fixed ownership of m_platformCursor. Create new object in the copy constructor and op= instead of introducing an explicit refcounted wrapper for QCursor since sharing is handled internally in QCursor.
Comment on attachment 64574 [details] proposed patch Balazs, the change is generally looking great. However, you are basically doing 3 things at once here, which more than what the bug description entitles: 1) you are moving Qt a lazy cursor creation approach (the WebCore part of the patch), which is nice! 2) you are replacing the code path for calling QWebPageClient::setCursor directly from WidgetQt in favor to cycle its call through hostWindow->chromeClient->QWegPageClient, which is also by itself a nice change! 3) you are making webkit2 to benefit from 1). In my opinion, each of the above items should happen on its own step, so r- (1) is indeed the bigger part, and having each of them landing separately would make tracking for possible regression easier, and also match to the general advertisement of making patch as small as possible. More comment inlined below. > From b6ff5748815e394e2fcd6156299d638e8878d9f3 Mon Sep 17 00:00:00 2001 > From: Balazs Kelemen <kb@inf.u-szeged.hu> > Date: Tue, 17 Aug 2010 13:45:03 +0200 > Subject: [PATCH] [Qt] Use LAZY_NATIVE_CURSOR > > --- > WebCore/ChangeLog | 28 ++ > WebCore/platform/Cursor.h | 7 +- > WebCore/platform/qt/CursorQt.cpp | 444 ++++++++------------------ > WebCore/platform/qt/QWebPageClient.h | 11 +- > WebCore/platform/qt/WidgetQt.cpp | 8 +- > WebKit/qt/ChangeLog | 12 + > WebKit/qt/WebCoreSupport/ChromeClientQt.cpp | 7 +- > WebKit2/ChangeLog | 19 ++ > WebKit2/UIProcess/API/qt/qgraphicswkview.cpp | 8 + > WebKit2/UIProcess/API/qt/qgraphicswkview.h | 6 + > WebKit2/UIProcess/API/qt/qwkpage.cpp | 7 + > WebKit2/UIProcess/API/qt/qwkpage.h | 2 + > WebKit2/UIProcess/API/qt/qwkpage_p.h | 2 +- > 13 files changed, 243 insertions(+), 318 deletions(-) > > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog > index ef8a91b..a8bc288 100644 > --- a/WebCore/ChangeLog > +++ b/WebCore/ChangeLog > @@ -1,3 +1,31 @@ > +2010-08-16 Balazs Kelemen <kb@inf.u-szeged.hu> > + > + Reviewed by NOBODY (OOPS!). > + > + [Qt] Use LAZY_NATIVE_CURSOR > + > + https://bugs.webkit.org/show_bug.cgi?id=44062 > + > + No functional change so new tests. > + > + Change Cursor behaviour to match the LAZY_NATIVE_CURSOR policy. This is *the* change the bug title implies. > + Propagate the setCursor callback to the PageClient via the HostWindow instead of preassuming > + the concrete type of the ChromeClient (what was generally wrong and actually incompatible with WebKit2). This should be a follow up. > + (WebCore::Widget::setCursor): Propagete the callback forward through HostWindow. > + > 2010-08-17 Philippe Normand <pnormand@igalia.com> Typo: propagete > > -const Cursor& grabbingCursor() > +void Cursor::ensurePlatformCursor() const > { who calls this method? > + m_platformCursor = new QCursor(QPixmap(QLatin1String(":/webkit/resources/verticalTextCursor.png")), 7, 7); > + break; > + case Cell: > + m_platformCursor = new QCursor(QPixmap(QLatin1String(":/webkit/resources/cellCursor.png")), 7, 7); > + break; > + case ContextMenu: > + m_platformCursor = new QCursor(QPixmap(QLatin1String(":/webkit/resources/contextMenuCursor.png")), 3, 2); > + break; > + case Alias: > + m_platformCursor = new QCursor(QPixmap(QLatin1String(":/webkit/resources/aliasCursor.png")), 11, 3); > + break; > + case Progress: > + m_platformCursor = new QCursor(QPixmap(QLatin1String(":/webkit/resources/progressCursor.png")), 3, 2); > + break; > + case Copy: > + m_platformCursor = new QCursor(QPixmap(QLatin1String(":/webkit/resources/copyCursor.png")), 3, 2); > + break; > + case ZoomIn: > + m_platformCursor = new QCursor(QPixmap(QLatin1String(":/webkit/resources/zoomInCursor.png")), 7, 7); > + break; > + case ZoomOut: > + m_platformCursor = new QCursor(QPixmap(QLatin1String(":/webkit/resources/zoomOutCursor.png")), 7, 7); > + break; where these hardcoded hotspots come from? 7 , 7 11, 3 3 , 2 ... > diff --git a/WebCore/platform/qt/WidgetQt.cpp b/WebCore/platform/qt/WidgetQt.cpp > index 0903b6e..e64d655 100644 > --- a/WebCore/platform/qt/WidgetQt.cpp > +++ b/WebCore/platform/qt/WidgetQt.cpp > @@ -78,10 +78,10 @@ void Widget::setFocus(bool focused) > void Widget::setCursor(const Cursor& cursor) > { > #ifndef QT_NO_CURSOR > - QWebPageClient* pageClient = root()->hostWindow()->platformPageClient(); > - > - if (pageClient) > - pageClient->setCursor(cursor.impl()); > + ScrollView* view = root(); > + if (!view) > + return; > + view->hostWindow()->setCursor(cursor); > #endif > } Do you have a case where root() is null here? It should not be null at all, since was not being null-checked before. I'd rather add a assert(root()). > diff --git a/WebKit/qt/ChangeLog b/WebKit/qt/ChangeLog > index 2c964d3..f59bdc6 100644 > --- a/WebKit/qt/ChangeLog > +++ b/WebKit/qt/ChangeLog > @@ -1,3 +1,15 @@ > +2010-08-16 Balazs Kelemen <kb@inf.u-szeged.hu> > + > + Reviewed by NOBODY (OOPS!). > + > + [Qt] Use LAZY_NATIVE_CURSOR > + > + https://bugs.webkit.org/show_bug.cgi?id=44062 > + > + * WebCoreSupport/ChromeClientQt.cpp: > + (WebCore::ChromeClientQt::setCursor): Implemented. Propagete the callback forward to the > + platform specific PageClient (QWebPageCLient). typo: propagete > #include <QPainter> > @@ -64,6 +65,8 @@ QGraphicsWKView::QGraphicsWKView(WKPageNamespaceRef pageNamespaceRef, BackingSto > connect(d->page, SIGNAL(loadProgress(int)), this, SIGNAL(loadProgress(int))); > connect(d->page, SIGNAL(initialLayoutCompleted()), this, SIGNAL(initialLayoutCompleted())); > connect(d->page, SIGNAL(urlChanged(const QUrl&)), this, SIGNAL(urlChanged(const QUrl&))); > + > + connect(d->page, SIGNAL(cursorChanged(const QCursor&)), this, SLOT(updateCursor(const QCursor&))); > } drop the extra new line. Nice work!
We need to make sure this works with the fallback cursors. I can set setCursor with Qt that that new cursor should be shown always until I unset it, there the latest webcore cursor should be shown instead.
> > In my opinion, each of the above items should happen on its own step, so r- Roger that, I will separate the 3 step. > > > + (WebCore::Widget::setCursor): Propagete the callback forward through HostWindow. > > + > > 2010-08-17 Philippe Normand <pnormand@igalia.com> > > Typo: propagete That is what I wrote but the correct spelling is propagate :) > > > > > > -const Cursor& grabbingCursor() > > +void Cursor::ensurePlatformCursor() const > > { > > who calls this method? Cursor::platformCursor. It is in the common part of the Cursor class (Cursor.cpp) > > > + m_platformCursor = new QCursor(QPixmap(QLatin1String(":/webkit/resources/verticalTextCursor.png")), 7, 7); > > + break; > > + case Cell: > > + m_platformCursor = new QCursor(QPixmap(QLatin1String(":/webkit/resources/cellCursor.png")), 7, 7); > > + break; > > + case ContextMenu: > > + m_platformCursor = new QCursor(QPixmap(QLatin1String(":/webkit/resources/contextMenuCursor.png")), 3, 2); > > + break; > > + case Alias: > > + m_platformCursor = new QCursor(QPixmap(QLatin1String(":/webkit/resources/aliasCursor.png")), 11, 3); > > + break; > > + case Progress: > > + m_platformCursor = new QCursor(QPixmap(QLatin1String(":/webkit/resources/progressCursor.png")), 3, 2); > > + break; > > + case Copy: > > + m_platformCursor = new QCursor(QPixmap(QLatin1String(":/webkit/resources/copyCursor.png")), 3, 2); > > + break; > > + case ZoomIn: > > + m_platformCursor = new QCursor(QPixmap(QLatin1String(":/webkit/resources/zoomInCursor.png")), 7, 7); > > + break; > > + case ZoomOut: > > + m_platformCursor = new QCursor(QPixmap(QLatin1String(":/webkit/resources/zoomOutCursor.png")), 7, 7); > > + break; > > where these hardcoded hotspots come from? From the original CursorQt.cpp, in the Cursors class. I just copy-pasted the paths. > > 7 , 7 > 11, 3 > 3 , 2 > ... > > > diff --git a/WebCore/platform/qt/WidgetQt.cpp b/WebCore/platform/qt/WidgetQt.cpp > > index 0903b6e..e64d655 100644 > > --- a/WebCore/platform/qt/WidgetQt.cpp > > +++ b/WebCore/platform/qt/WidgetQt.cpp > > @@ -78,10 +78,10 @@ void Widget::setFocus(bool focused) > > void Widget::setCursor(const Cursor& cursor) > > { > > #ifndef QT_NO_CURSOR > > - QWebPageClient* pageClient = root()->hostWindow()->platformPageClient(); > > - > > - if (pageClient) > > - pageClient->setCursor(cursor.impl()); > > + ScrollView* view = root(); > > + if (!view) > > + return; > > + view->hostWindow()->setCursor(cursor); > > #endif > > } > > Do you have a case where root() is null here? It should not be null at all, since was not being null-checked before. I'd rather add a assert(root()). > I followed the mac and win Widget implementations with this null check here. root() returns with zero if the root element in the widget tree is not a FrameView so I think this null check is necessary. Follow up patches will be coming.
Created attachment 64705 [details] webcore part
Comment on attachment 64705 [details] webcore part r=me
(In reply to comment #11) > We need to make sure this works with the fallback cursors. I can set setCursor with Qt that that new cursor should be shown always until I unset it, there the latest webcore cursor should be shown instead. It would be great to get that finally fixed! Maybe with lock/unlock methods(?)
(In reply to comment #15) > (In reply to comment #11) > > We need to make sure this works with the fallback cursors. I can set setCursor with Qt that that new cursor should be shown always until I unset it, there the latest webcore cursor should be shown instead. > > It would be great to get that finally fixed! Maybe with lock/unlock methods(?) I do not think that the second part of my changes would change any behavior. I do not understand how the current code achieves the behavior that Kenneth described since the PageClients (PageClientQWidget and PageClientQGraphicsWidget) are just calling setCursor on the widget. Could you give me some pointers?
Comment on attachment 64705 [details] webcore part Clearing flags on attachment: 64705 Committed r65631: <http://trac.webkit.org/changeset/65631>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/65631 might have broken SnowLeopard Intel Release (Tests) The following changes are on the blame list: http://trac.webkit.org/changeset/65630 http://trac.webkit.org/changeset/65631
The story continues in bug 44250.