[CoordinatedGraphics] Regressions in WebView's contentScaleFactor/contentPosition

• Component: WebKit2
• Status: RESOLVED FIXED

[Nick Diego Yamane (diegoyam)]

Bug 118548 changed the way contentScaleFactor and contentPosition are manipulated in WebView. After that, we've been dealing with some unexpected behaviors in contentPosition-related stuff. Some Nix API tests are failing and we got some regressions in panning and zoom features of Nix's MiniBrowser. It seems like whenever we set WebView's contentPosition we have to multiply/scale it by the current scale factor explictly, that "solved" most of those issues mentioned before, but I suppose that is not intended.

We should make sure we really need to store contentPosition as a scaled value, as it's being done in https://github.com/WebKitNix/webkitnix/blob/master/Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp#L515. Besides that, WebView::pageDidRequestScroll is passing the non-scaled value to client callback, that is at least weird, maybe a separate issue.

Macelo Lira and Thiago Lacerda are currently working on some API/Layout tests to make sure this kind of regression won't be missed in future changes.

It would be good to have some feedback from Noam and some EFL guys using CoordinatedGraphics and contentScaleFactor related stuff.

[Nick Diego Yamane (diegoyam)]

Created attachment 219584 [details]
Proposed Patch

This patch modifies WebView to store contentPosition as a non-scaled value and refactor EFL functions functions that use contentPosition API to do not assume it is stored scaled anymore.

[Eunmi Lee]

Comment on attachment 219584 [details]
Proposed Patch

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

This patch is LGTM.
I added patch in Bug 118548 to make usage of contentPosition consistent by regarding it as scaled value.
But as you mentioned, we have to multiply it by the current scale factor whenever set WebView's contentPosition.
Codes are more simple after changing contentPosition as a non-scaled value :)

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1403
>      FloatPoint newPosition(oldPosition.x + offset.width(), oldPosition.y + offset.height());

If we divide offset with contentScale, we don't have to multiply oldPosition and divide newPosition.

[Nick Diego Yamane (diegoyam)]

(In reply to comment #2)
> (From update of attachment 219584 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219584&action=review
> 
> This patch is LGTM.
> I added patch in Bug 118548 to make usage of contentPosition consistent by regarding it as scaled value.
> But as you mentioned, we have to multiply it by the current scale factor whenever set WebView's contentPosition.
> Codes are more simple after changing contentPosition as a non-scaled value :)
> 
> > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1403
> >      FloatPoint newPosition(oldPosition.x + offset.width(), oldPosition.y + offset.height());
> 
> If we divide offset with contentScale, we don't have to multiply oldPosition and divide newPosition.

Thanks for the suggestion, indeed that will remove some redundancy from that calculation, I'll send an updated patch applying this change.

[Nick Diego Yamane (diegoyam)]

Created attachment 219649 [details]
Proposed patch

Simplifies calculations of newPosition when scrolling in EwkView, as suggested by Eummi.

[WebKit Commit Bot]

Comment on attachment 219649 [details]
Proposed patch

Rejecting attachment 219649 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 219649 [details]
Proposed patch

Rejecting attachment 219649 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 219649 [details]
Proposed patch

Rejecting attachment 219649 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 219649 [details]
Proposed patch

Clearing flags on attachment: 219649

Committed r160833: <http://trac.webkit.org/changeset/160833>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.