Bug 116683 - [WK2][CoordinatedGraphics][EFL] WKViewUserViewportToContents() function doesn't do what it says
Summary: [WK2][CoordinatedGraphics][EFL] WKViewUserViewportToContents() function doesn...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Marcelo Lira
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-23 11:05 PDT by Marcelo Lira
Modified: 2013-05-29 07:34 PDT (History)
13 users (show)

See Also:


Attachments
Patch (12.93 KB, patch)
2013-05-23 11:20 PDT, Marcelo Lira
no flags Details | Formatted Diff | Diff
Patch (13.03 KB, patch)
2013-05-27 09:46 PDT, Marcelo Lira
no flags Details | Formatted Diff | Diff
Patch (13.09 KB, patch)
2013-05-28 07:28 PDT, Marcelo Lira
no flags Details | Formatted Diff | Diff
Patch (13.17 KB, patch)
2013-05-28 09:15 PDT, Marcelo Lira
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marcelo Lira 2013-05-23 11:05:28 PDT
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.
Comment 1 Marcelo Lira 2013-05-23 11:20:46 PDT
Created attachment 202730 [details]
Patch
Comment 2 Noam Rosenthal 2013-05-25 01:52:24 PDT
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.
Comment 3 Marcelo Lira 2013-05-27 09:46:44 PDT
Created attachment 202992 [details]
Patch
Comment 4 Noam Rosenthal 2013-05-28 07:05:31 PDT
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.
Comment 5 Marcelo Lira 2013-05-28 07:28:59 PDT
Created attachment 203052 [details]
Patch
Comment 6 Marcelo Lira 2013-05-28 07:38:15 PDT
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?
Comment 7 Noam Rosenthal 2013-05-28 07:48:50 PDT
(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.
Comment 8 Marcelo Lira 2013-05-28 09:15:55 PDT
Created attachment 203060 [details]
Patch
Comment 9 WebKit Commit Bot 2013-05-29 07:34:06 PDT
Comment on attachment 203060 [details]
Patch

Clearing flags on attachment: 203060

Committed r150893: <http://trac.webkit.org/changeset/150893>
Comment 10 WebKit Commit Bot 2013-05-29 07:34:10 PDT
All reviewed patches have been landed.  Closing bug.