RESOLVED FIXED 118548
[CoordinatedGraphics][WK2] Correct wrong usage of m_contentPosition variable in the WebView.
https://bugs.webkit.org/show_bug.cgi?id=118548
Summary [CoordinatedGraphics][WK2] Correct wrong usage of m_contentPosition variable ...
Eunmi Lee
Reported 2013-07-10 21:34:19 PDT
The m_contentPosition variable is scaled position which is scaled with content scale factor. It can be confused with real position (which is known in the webcore), so it should be used carefully. I fix the wrong usage of m_contentPosition in this bug.
Attachments
Patch (2.51 KB, patch)
2013-07-10 21:41 PDT, Eunmi Lee
no flags
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (540.66 KB, application/zip)
2013-07-11 00:47 PDT, Build Bot
no flags
Patch (2.84 KB, patch)
2013-07-18 01:44 PDT, Eunmi Lee
no flags
Patch (2.65 KB, patch)
2013-07-18 04:47 PDT, Eunmi Lee
no flags
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (707.03 KB, application/zip)
2013-07-18 06:56 PDT, Build Bot
no flags
Patch (2.72 KB, patch)
2013-07-18 19:28 PDT, Eunmi Lee
no flags
Patch (2.60 KB, patch)
2013-10-18 03:17 PDT, Eunmi Lee
no flags
Eunmi Lee
Comment 1 2013-07-10 21:41:05 PDT
Build Bot
Comment 2 2013-07-11 00:47:38 PDT
Comment on attachment 206426 [details] Patch Attachment 206426 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1048524 New failing tests: fullscreen/full-screen-iframe-with-max-width-height.html
Build Bot
Comment 3 2013-07-11 00:47:40 PDT
Created attachment 206430 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.3
Gyuyoung Kim
Comment 4 2013-07-16 18:53:12 PDT
Comment on attachment 206426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206426&action=review Noam, could you take a look this patch ? > Source/WebKit2/ChangeLog:3 > + [EFL][WK2] Correct wrong usage of m_contentPosition variable in the WebView. It looks you need to change [EFL] with [CoordnatiedGraphics].
Noam Rosenthal
Comment 5 2013-07-17 19:09:58 PDT
Comment on attachment 206426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206426&action=review > Source/WebKit2/ChangeLog:10 > + The m_contentPosition variable is scaled position which is scaled with > + content scale factor. It can be confused with real position which is > + known in the webcore, so it should be used carefully. I don't get what this says, or what this patch does differently. > Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:248 > + FloatPoint position = contentPosition(); > + position.scale(1 / m_contentScaleFactor, 1 / m_contentScaleFactor); > + FloatRect visibleContentsRect(position, visibleContentsSize()); This does not explain itself, nor it is explained in the Changelog.
Eunmi Lee
Comment 6 2013-07-18 00:25:51 PDT
Comment on attachment 206426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206426&action=review >> Source/WebKit2/ChangeLog:3 >> + [EFL][WK2] Correct wrong usage of m_contentPosition variable in the WebView. > > It looks you need to change [EFL] with [CoordnatiedGraphics]. Yes, I will change that. >> Source/WebKit2/ChangeLog:10 >> + known in the webcore, so it should be used carefully. > > I don't get what this says, or what this patch does differently. OK, I will re-write description. >> Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:248 >> + FloatRect visibleContentsRect(position, visibleContentsSize()); > > This does not explain itself, nor it is explained in the Changelog. the contentPosition() returns the scaled position, so we have to divide that with content scale factor before setting to the visibleContentrect. I will add description about this to the changelog.
Eunmi Lee
Comment 7 2013-07-18 01:44:13 PDT
Created attachment 206965 [details] Patch Re-write the changelog.
Noam Rosenthal
Comment 8 2013-07-18 02:40:14 PDT
Comment on attachment 206965 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206965&action=review > Source/WebKit2/ChangeLog:16 > + > + The CoordinatedGraphics has its own scaling logic which is called as > + contents scaling, so the content position of WebCore should be used > + after scaling with contents scale factor in the CoordinatedGraphics. > + The scaled content position is maintained in the WebView as a > + m_contentPosition, and we have to recognize that is scaled value. > + > + The m_contentPosition is regarded as a non-scaled value in the > + transformToScene() and updateViewportSize(), so I fix them correctly. > + Correct English please... I hate being grammar police (I'm not a native English speaker myself) but we have to maintain readability standard in the Changelog.
Eunmi Lee
Comment 9 2013-07-18 04:47:50 PDT
Created attachment 206980 [details] Patch Re-write changelog.
Build Bot
Comment 10 2013-07-18 06:56:21 PDT
Comment on attachment 206980 [details] Patch Attachment 206980 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1090892 New failing tests: media/video-zoom.html
Build Bot
Comment 11 2013-07-18 06:56:23 PDT
Created attachment 206988 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.3
Eunmi Lee
Comment 12 2013-07-18 19:28:06 PDT
Created attachment 207049 [details] Patch Rebase.
Eunmi Lee
Comment 13 2013-10-18 03:17:53 PDT
Created attachment 214554 [details] Patch re-write Changelog.
Praveen Jadhav
Comment 14 2013-11-26 16:51:29 PST
*** Bug 124396 has been marked as a duplicate of this bug. ***
Ryuan Choi
Comment 15 2013-11-28 18:24:41 PST
(In reply to comment #13) > Created an attachment (id=214554) [details] > Patch > > re-write Changelog. I think that we need this. Are there still issues to block this?
WebKit Commit Bot
Comment 16 2013-11-28 23:48:43 PST
Comment on attachment 214554 [details] Patch Clearing flags on attachment: 214554 Committed r159864: <http://trac.webkit.org/changeset/159864>
WebKit Commit Bot
Comment 17 2013-11-28 23:48:48 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.