Bug 94384 – screenToWindow and windowToScreen are not implemented in Qt5 WebKit2

• Component: WebKit Qt
• Status: RESOLVED INVALID

[Olivier Crête]

That means that WebCore has no way of knowing where a point is on the actual screen. Which is a problem on platforms where we can only do overlay video. Patch forthcoming.

[Olivier Crête]

Created attachment 159211 [details]
Qt WebKit2: Implement windowToScreen and screenToWindow in QtPageClient

[Eric Seidel (no email)]

Comment on attachment 159211 [details]
Qt WebKit2: Implement windowToScreen and screenToWindow in QtPageClient

I assume this causes tests to pass?

[Olivier Crête]

I don't think it helps with any test case as we only use it with an private/closed source/unpublished MediaPlayerPrivate.. I think the upstream code does everything using relative coordinates so it doesn't need this function to be implemented.

[Andras Becsi]

Comment on attachment 159211 [details]
Qt WebKit2: Implement windowToScreen and screenToWindow in QtPageClient

View in context: https://bugs.webkit.org/attachment.cgi?id=159211&action=review

I think this patch is incorrect and might event break layout tests.

> Source/WebKit2/UIProcess/qt/QtPageClient.cpp:196
> +    QPointF itemPoint(point);

I find the name "itemPoint" a bit confusing for a point in absolute screen coords. Could be screenPoint maybe?

> Source/WebKit2/UIProcess/qt/QtPageClient.cpp:203
> +    return window->mapFromGlobal(screenPoint.toPoint());

This seems wrong to me since mapFromGlobal translates global screen coordinates to window coordinates and you previously mapped the incoming screen position to QQuickWebView item coordinates.

> Source/WebKit2/UIProcess/qt/QtPageClient.cpp:212
> +    QRectF itemRect(rect);
> +    QRectF screenRect = m_webView->mapRectToScene(itemRect);

The incoming rect should be in window coordinates and not in QQuickWebView item coordinates (the view itself might be displaced within the window because of the URL bar or other items) thus this conversion seem also wrong to me.

[Olivier Crête]

Do you have any suggestion on how to do it better? I'm especially confused on how the QWindow and QQuickItem coordinates match. Aren't scene coordinates just another name for window coordinates or are they different? In screenToWindow(), should I do m_webView->mapToScene(window->mapFromGlobal(point)) or am I completely lost?

For windowToScreen, the parameter is indeed displaced because of the URL bar and that's what I was trying to fix in the first place. My understanding was that the "window" here is actually the "webkit window", a QQuickWebView, not the actual QWindow, hence the need to map them to the scene.

[Andras Becsi]

Comment on attachment 159211 [details]
Qt WebKit2: Implement windowToScreen and screenToWindow in QtPageClient

(In reply to comment #5)
> Do you have any suggestion on how to do it better? I'm especially confused on how the QWindow and QQuickItem coordinates match. Aren't scene coordinates just another name for window coordinates or are they different? In screenToWindow(), should I do m_webView->mapToScene(window->mapFromGlobal(point)) or am I completely lost?
>
I'm not sure I understand in what way you are trying to use these functions so I can not help you with that.

> For windowToScreen, the parameter is indeed displaced because of the URL bar and that's what I was trying to fix in the first place. My understanding was that the "window" here is actually the "webkit window", a QQuickWebView, not the actual QWindow, hence the need to map them to the scene.

AFAICT screenToWindow should do window->mapFromGlobal(point) and windowToScreen should return IntRect(window->mapToGlobal(rect.location()), rect.size()).

[Olivier Crête]

(In reply to comment #6)
> (From update of attachment 159211 [details])
> (In reply to comment #5)
> > Do you have any suggestion on how to do it better? I'm especially confused on how the QWindow and QQuickItem coordinates match. Aren't scene coordinates just another name for window coordinates or are they different? In screenToWindow(), should I do m_webView->mapToScene(window->mapFromGlobal(point)) or am I completely lost?
> >
> I'm not sure I understand in what way you are trying to use these functions so I can not help you with that.

While writing a custom MediaPlayerPrivate, I'm trying to get the absolute screen coordinates of a HTML <video> element so I can get a hardware overlay to draw at the right spot. I should mention that windowToScreen() currently works with the patch for that one specific use-case.

[Olivier Crête]

Follow up to my last comment, so I'm calling Element::screenRect() which ends up calling windowToScreen().

[Olivier Crête]

(In reply to comment #6)
> AFAICT screenToWindow should do window->mapFromGlobal(point) and windowToScreen should return IntRect(window->mapToGlobal(rect.location()), rect.size()).

The way I understand it is that the "window" referred to by screenToWindow() and windowToScreen() is the "webkit" window which is a QQuickWebView, while the QWindow "window" that I mapTo/FromGlobal is on contains that QQuickWebView, but can also contain other thigns (like a URL bar!), so I somehow need to convert between them.

[Andras Becsi]

(In reply to comment #9)
> (In reply to comment #6)
> > AFAICT screenToWindow should do window->mapFromGlobal(point) and windowToScreen should return IntRect(window->mapToGlobal(rect.location()), rect.size()).
> 
> The way I understand it is that the "window" referred to by screenToWindow() and windowToScreen() is the "webkit" window which is a QQuickWebView, while the QWindow "window" that I mapTo/FromGlobal is on contains that QQuickWebView, but can also contain other thigns (like a URL bar!), so I somehow need to convert between them.

Right, the window is indeed the rootView so for the default case your windowToScreen seems ok, but screenToWindow is incorrect.

Although I think these would produce wrong results if the page is scaled from the UI (pinch zoom) or a custom device pixel ratio is set, since the ScrollView only adjusts the coordinates with the scroll offset.

[Olivier Crête]

Created attachment 162932 [details]
Patch v2

Hopefully, I got it right this time.

[Early Warning System Bot]

Comment on attachment 162932 [details]
Patch v2

Attachment 162932 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13777873

[Andras Becsi]

Comment on attachment 162932 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=162932&action=review

This looks better :)

> Source/WebKit2/ChangeLog:8
> +        QtWebKit2: Implement windowToScreen and screenToWindow in QtPageClient
> +
> +        Enables WebCore to find where on the Screen a point is.
> +
> +        screenToWindow and windowToScreen are not implemented in Qt5 WebKit2
> +        https://bugs.webkit.org/show_bug.cgi?id=94384

The ChangeLog should start with the bug title then the bug URL and then the description.
To match the usual naming convention the bug title should probably be: [Qt][WK2] QtPageClient's screenToWindow and windowToScreen are not implemented

> Source/WebKit2/UIProcess/qt/QtPageClient.cpp:194
> +    if (!m_webView)
> +        return point;

Since the QtPageClient is created and initialized by QQuickWebView the m_webView pointer here should be valid for the entire lifetime of QtPageClient, thus this check can be an ASSERT instead.

> Source/WebKit2/UIProcess/qt/QtPageClient.cpp:204
> +    if (!m_webView)
> +        return point;

This can be omitted, see above.

> Source/WebKit2/UIProcess/qt/QtPageClient.cpp:206
> +    return m_webView->mapFromScene(windowPoint);

As the EWS points out the Qt-WK2 build fails because toPoint() is missing here.

> Source/WebKit2/UIProcess/qt/QtPageClient.cpp:212
> +    if (!m_webView)
> +        return rect;

ASSERT instead, see above.

[Simon Hausmann]

Comment on attachment 162932 [details]
Patch v2

I would like to see a test for this. The patch also needs to be fixed to actually compile.

And Andras has also a couple of good points :)

[Olivier Crête]

Created attachment 163131 [details]
Updated again

I think I got everything except a test. I'm not sure exactly how to test this kind of thing ?

[Philippe Normand]

(In reply to comment #15)
> Created an attachment (id=163131) [details]
> Updated again
> 
> I think I got everything except a test. I'm not sure exactly how to test this kind of thing ?

If this patch needs review it should be marked r? :)

[Alexey Proskuryakov]

The Qt port has been removed from WebKit, resolving this bug.