[webkit-reviews] review denied: [Bug 44062] [Qt] Use LAZY_NATIVE_CURSOR : [Attachment 64574] proposed patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Aug 17 21:33:58 PDT 2010
Antonio Gomes <tonikitoo at webkit.org> has denied Balazs Kelemen
<kb at inf.u-szeged.hu>'s request for review:
Bug 44062: [Qt] Use LAZY_NATIVE_CURSOR
https://bugs.webkit.org/show_bug.cgi?id=44062
Attachment 64574: proposed patch
https://bugs.webkit.org/attachment.cgi?id=64574&action=review
------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
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!
More information about the webkit-reviews
mailing list