Bug 118548

Summary: [CoordinatedGraphics][WK2] Correct wrong usage of m_contentPosition variable in the WebView.
Product: WebKit Reporter: Eunmi Lee <enmi.lee>
Component: WebKit2Assignee: Eunmi Lee <enmi.lee>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cmarcelo, commit-queue, lucas.de.marchi, luiz, noam, praveen.j, rniwa, ryuan.choi, zeno
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 125943    
Bug Blocks: 107631    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
none
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
none
Patch
none
Patch none

Description Eunmi Lee 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.
Comment 1 Eunmi Lee 2013-07-10 21:41:05 PDT
Created attachment 206426 [details]
Patch
Comment 2 Build Bot 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
Comment 3 Build Bot 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
Comment 4 Gyuyoung Kim 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].
Comment 5 Noam Rosenthal 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.
Comment 6 Eunmi Lee 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.
Comment 7 Eunmi Lee 2013-07-18 01:44:13 PDT
Created attachment 206965 [details]
Patch

Re-write the changelog.
Comment 8 Noam Rosenthal 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.
Comment 9 Eunmi Lee 2013-07-18 04:47:50 PDT
Created attachment 206980 [details]
Patch

Re-write changelog.
Comment 10 Build Bot 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
Comment 11 Build Bot 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
Comment 12 Eunmi Lee 2013-07-18 19:28:06 PDT
Created attachment 207049 [details]
Patch

Rebase.
Comment 13 Eunmi Lee 2013-10-18 03:17:53 PDT
Created attachment 214554 [details]
Patch

re-write Changelog.
Comment 14 Praveen Jadhav 2013-11-26 16:51:29 PST
*** Bug 124396 has been marked as a duplicate of this bug. ***
Comment 15 Ryuan Choi 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?
Comment 16 WebKit Commit Bot 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>
Comment 17 WebKit Commit Bot 2013-11-28 23:48:48 PST
All reviewed patches have been landed.  Closing bug.