[Webkit-unassigned] [Bug 44062] [Qt] Use LAZY_NATIVE_CURSOR
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Aug 17 21:33:59 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=44062
Antonio Gomes <tonikitoo at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #64574|review?, commit-queue? |review-
Flag| |
--- Comment #10 from Antonio Gomes <tonikitoo at webkit.org> 2010-08-17 21:33:58 PST ---
(From update of attachment 64574)
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 at 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 at 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 at 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 at 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!
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list