Bug 118548 – [CoordinatedGraphics][WK2] Correct wrong usage of m_contentPosition variable in the WebView.

• Component: WebKit2
• Status: RESOLVED FIXED

[Eunmi Lee]

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.

[Eunmi Lee]

Created attachment 206426 [details]
Patch

[Build Bot]

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]

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 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 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 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]

Created attachment 206965 [details]
Patch

Re-write the changelog.

[Noam Rosenthal]

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]

Created attachment 206980 [details]
Patch

Re-write changelog.

[Build Bot]

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]

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]

Created attachment 207049 [details]
Patch

Rebase.

[Eunmi Lee]

Created attachment 214554 [details]
Patch

re-write Changelog.

[Praveen Jadhav]

*** Bug 124396 has been marked as a duplicate of this bug. ***

[Ryuan Choi]

(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 on attachment 214554 [details]
Patch

Clearing flags on attachment: 214554

Committed r159864: <http://trac.webkit.org/changeset/159864>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.