Bug 44062 – [Qt] Use LAZY_NATIVE_CURSOR

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Balazs Kelemen]

Since we have a WebKit2 port, we need to apply this implementation policy.

[Balazs Kelemen]

Created attachment 64503 [details]
proposed patch

[Ariya Hidayat]

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.

[Balazs Kelemen]

Created attachment 64565 [details]
proposed patch

Changelog lines have been shortened somewhat :)
Added copyright for CursorQt.cpp.

[Kenneth Rohde Christiansen]

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.

[Balazs Kelemen]

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

[Kenneth Rohde Christiansen]

Antonio, you know how this is supposed to work and how to test it. can you help?

[Balazs Kelemen]

Created attachment 64566 [details]
patch v3

[Balazs Kelemen]

Comment on attachment 64566 [details]
patch v3

Augh, this could leak QCursors. Need to be fixed again.

[Balazs Kelemen]

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.

[Antonio Gomes]

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 <[e-mail]>
> 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 @@
> [phone]  Balazs Kelemen  <[e-mail]>
> +
> +        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  <[e-mail]>

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 @@
> [phone]  Balazs Kelemen  <[e-mail]>
> +
> +        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!

[Kenneth Rohde Christiansen]

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.

[Balazs Kelemen]

> 
> 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  <[e-mail]>
> 
> 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.

[Balazs Kelemen]

Created attachment 64705 [details]
webcore part

[Antonio Gomes]

Comment on attachment 64705 [details]
webcore part

r=me

[Antonio Gomes]

(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(?)

[Balazs Kelemen]

(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?

[WebKit Commit Bot]

Comment on attachment 64705 [details]
webcore part

Clearing flags on attachment: 64705

Committed r65631: <http://trac.webkit.org/changeset/65631>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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

[Balazs Kelemen]

The story continues in bug 44250.