Bug 116683 – [WK2][CoordinatedGraphics][EFL] WKViewUserViewportToContents() function doesn't do what it says

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Marcelo Lira]

One would expect WKViewUserViewportToContents to convert WebView coordinates to page contents coordinates, but currently it doesn't consider the page scale factor.

Examples of what should be the results of WKViewUserViewportToContents:

1. The viewport shows a page on position (0,0) and the scale factor is 0.0.
   viewport (10,10) -> contents(10,10)
2. Using WKViewSetContentPosition we scroll the page under the viewport to show the contents in position (100,100).
   viewport(10,10) -> contents(110,110)
3. Using WKViewSetContentScaleFactor we set the page scale to 2.0.
   viewport(10,10) -> contents(105,105)

Elsewhere, EwkView::createGLSurface() uses WKViewUserViewportToContents, but it doesn't expect these scroll and scaling operations to go into the calculations. Just the translation of the WKView in relation to the GL surface where it is drawn. As it is now WKViewUserViewportToContents works for EwkView, but it shouldn't.

[Marcelo Lira]

Created attachment 202730 [details]
Patch

[Noam Rosenthal]

Comment on attachment 202730 [details]
Patch

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

> Source/WebKit2/ChangeLog:15
> +        WKViewUserViewportToScene.

You mean WKViewUserViewportToContents.

[Marcelo Lira]

Created attachment 202992 [details]
Patch

[Noam Rosenthal]

Comment on attachment 202992 [details]
Patch

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

> Source/WebKit2/ChangeLog:8
> +        WKViewUserViewportToContents now convert WebView coordinates to

convert -> converts

> Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.cpp:111
> +    const WebCore::IntPoint& result = toImpl(viewRef)->userViewportToScene(toIntPoint(point));

no need for a const& when it's an IntPoint, it doesn't perform better (or worse) :)

> Tools/TestWebKitAPI/Tests/WebKit2/WKViewUserViewportToContents.cpp:33
> +TEST(WebKitNix, WKViewUserViewportToContents)

This is not really a WebKitNix test...
Also this doesn't feel like the right directory for this test.

[Marcelo Lira]

Created attachment 203052 [details]
Patch

[Marcelo Lira]

Comment on attachment 202992 [details]
Patch

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

>> Tools/TestWebKitAPI/Tests/WebKit2/WKViewUserViewportToContents.cpp:33
>> +TEST(WebKitNix, WKViewUserViewportToContents)
> 
> This is not really a WebKitNix test...
> Also this doesn't feel like the right directory for this test.

At first I felt this was the best directory for the test, since it's not tied to a particular port.
But now you made me doubt. Should we have a CoordinatedGraphics directory under WebKit2?

[Noam Rosenthal]

(In reply to comment #6)
> (From update of attachment 202992 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202992&action=review
> 
> >> Tools/TestWebKitAPI/Tests/WebKit2/WKViewUserViewportToContents.cpp:33
> >> +TEST(WebKitNix, WKViewUserViewportToContents)
> > 
> > This is not really a WebKitNix test...
> > Also this doesn't feel like the right directory for this test.
> 
> At first I felt this was the best directory for the test, since it's not tied to a particular port.
> But now you made me doubt. Should we have a CoordinatedGraphics directory under WebKit2?
Sounds right.

[Marcelo Lira]

Created attachment 203060 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 203060 [details]
Patch

Clearing flags on attachment: 203060

Committed r150893: <http://trac.webkit.org/changeset/150893>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.